raise ambiguity error on attribute macro that could be tool attribute

[mejrs]

View all comments

See #162597 (comment) for clean crater run; the errors are unrelated/spurious.

We need some sort of ambiguity error in other to implement the register tool rfc:

Fixing name resolution errors

Note that register_tool changes name resolution, and may give errors if you have a crate named some_tool.
The compiler will suggest ways to fix the new errors.

If a tool name conflicts with a crate name, you can disambiguate the crate with ::some_tool:

#![register_tool(some_tool)]
extern crate some_tool;

#[some_tool::attribute] //~ ERROR: is this the tool or a proc-macro?
fn bar() {
   // ...
}

#[::some_tool::attribute] // OK: This is the proc-macro defined in the crate.
fn foo() {
  // ...
}

However, if you want to go on to use a tool attribute,
you must rename the crate so it doesn't conflict:

#![register_tool(some_tool)]
extern crate some_tool as my_library;

#[some_tool::attribute] // OK: This is the attribute specified by the tool.
fn bar() {
   // ...
}

Alternatively, if you only want to use lints, you can use register_lint_tool instead of register_tool, which will avoid resolution errors.

Overlaps like this are expected to be rare in practice.

This PR implements the ambiguity error (not the "leading :: to disambiguate" part).

This is a breaking change if:

• someone has a crate or module named diagnostic|miri|rust_analyzer|clippy|rustfmt and exports an attribute macro from it
• it is then used it as #[(diagnostic|miri|rust_analyzer|clippy|rustfmt)::attr_macro] (rather than use module_or_crate::attr_macro; #[attr_macro])
• the same applies for tools registered with register_tool, but that's unstable.

See #158146 for context. This is a simpler version of that PR.

This does not implement unconditionally resolving these as tool attributes but does reserve the ability to do so. This would allow things like this to compile:

mod rustfmt {}

#[rustfmt::skip]
fn main() {}

and see also #98291 for a case where it would be nice to assume e.g. #[rustfmt::skip] is actually a tool attribute and thus inert.

[mejrs]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8c2ec58 (8c2ec5867b7cd0db3869fc1c128b867c2e8c0129)
Base parent: c4c4a57 (c4c4a576936e9e67717d0deb8e74e02dd5dd10de)

[mejrs]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: be3da1e (be3da1e8c043bb6f1f335463d3b1335cdf50b03c)
Base parent: 018018e (018018e881e2db0956f229dbb543e21f058d1ce7)

[mejrs]

@craterbot check

[craterbot]

👌 Experiment pr-162597 created and queued.
🤖 Automatically detected try build be3da1e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-162597 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-162597 is completed!
📊 3 regressed and 1 fixed (1108363 total)
📊 6349 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-162597/retry-regressed-list.txt ↩

[mejrs]

Looks like the crater run is clean, so I guess we can do this (If the lang team agrees, of course)

r? @petrochenkov

[petrochenkov]

So the main drawback here is the same as with built-in attributes - with these rules you cannot add a new built-in tool module without a breakage (maybe theoretical).

But if register_tool is stabilized, then new built-in tools are hopefully never added, and most of existing built-in tools are migrated to explicit registration in the next edition, so this is not going to be a big problem.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[mejrs]

@rustbot ready

So the main drawback here is the same as with built-in attributes - with these rules you cannot add a new built-in tool module without a breakage (maybe theoretical).

But if register_tool is stabilized, then new built-in tools are hopefully never added, and most of existing built-in tools are migrated to explicit registration in the next edition, so this is not going to be a big problem.

As far as new builtin tools go, the only idea I've seen is a lint tool for lint helpers in #t-lang > Extend the diagnostics attr for a lint helper @ 💬. Interestingly, an attribute macro is never used in this way (at least nowhere on github) so that breakage likely is only theoretical.

[mejrs]

By the way what's the process for merging something like this, where it's part of a rfc and technically a breaking change but has a clean crater run? Does any team need to be involved?