Add new ambiguity error for tools

[nbdd0121]

Tracking issue: #66079

This implements the resolution changes of RFC 3808.

A new ambiguity error is added, and the tool attributes is no longer being affected by #[no_implicit_prelude] per the RFC. This is a breaking change.

To avoid widespread ambiguity errors, the attribute tools are only resolved if they are resolved as part of macro path (this could be further split to just attribute macros, but at which point we are kind of adding a new type of namespace?

Mark as draft until the implementation detail is agreed and a crater run is conducted.

r? @petrochenkov

[petrochenkov]

the tool attributes is no longer being affected by #[no_implicit_prelude] per the RFC

This is a mistake in the RFC, the relevant comment - https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rfcs/pull/3808#discussion_r2075190210.
The behavior in case of no_implicit_prelude is unimportant and shouldn't be documented, we should just do whatever is more convenient.
There was a similar case in #139493 (comment), where the same decision was eventually made for no_implicit_prelude (doing what is simpler, without caring about the specific behavior).

[petrochenkov]

Introduction of the new ScopeSet is also a major hack, we shouldn't do it.
Relevant comment - https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rfcs/pull/3808#discussion_r2075205321

Another relevant comment chain - https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rfcs/pull/3808#discussion_r2075215063

(Perhaps together with the "ambiguity is always an error" change if we can pull it off, to maximize future possibilities.)

From the PR description it looks like we cannot actually pull it off.
To reduce the breakage we can go from "always an error" to e.g. "always an error, unless it's tool vs extern crate", and introduce scope priorities compatible with the current behavior on stable.
(The priorities can then be changed on the next edition, if really necessary.)

[petrochenkov]

cc @jyn514

[petrochenkov]

Blocked on #158038.
@rustbot blocked

[petrochenkov]

To reduce the breakage we can go from "always an error" to e.g. "always an error, unless it's tool vs extern crate", and introduce scope priorities compatible with the current behavior on stable.

I'd like to experiment with this myself a bit, probably this weekend.

[nbdd0121]

Introduction of the new ScopeSet is also a major hack, we shouldn't do it. Relevant comment - https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rfcs/pull/3808#discussion_r2075205321

Yes, I also dislike this part, but it feels to me that otherwise this is going to be a significant breakage. The fact that rustc itself breaks due to diagnostic tool attribute is a very strong indicator that the RFC text as written is not the way forward.

From the PR description it looks like we cannot actually pull it off. To reduce the breakage we can go from "always an error" to e.g. "always an error, unless it's tool vs extern crate", and introduce scope priorities compatible with the current behavior on stable. (The priorities can then be changed on the next edition, if really necessary.)

The error is between tool vs modules in rustc. But in the wild perhaps extern crate is more likely to cause breakage.

Instead of tweaking the ambiguity rules, I wonder if we should just handle the resolution of attributes differently?
The way built-in attributes are resolved really feel a hack to me. I wonder if we could just do this:

• Check if attribute is a tool-attribute or built-in attribute
• Try resolve the full path in macro namespace

And raise an ambiguity error if both resolves. This way the tools are no longer in any namespace. I feel that this also a more logical way of handling built-in attributes -- they really feel that they are part of the language and should not be in the macro namespace.

[nbdd0121]

See also https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Tool.20name.20resolution.20in.20.60register_tool.60.20RFC/near/604270506

[petrochenkov]

• Check if attribute is a tool-attribute or built-in attribute
• Try resolve the full path in macro namespace

And raise an ambiguity error if both resolves.

That's exactly what we are doing for built-in attributes right now.
The downside is that adding a new built-in attribute is a breaking change, in theory and often in practice.
Relevant recent zulip thread - #t-compiler > Handling breakage from built-in attributes, and one more linked from it.

If we introduce the notion of built-in tool attributes for diagnostic (first check if diagnostic::on_unimplemented is a built-in, then resolve it, then report an error if there's an ambiguity), then introducing new diagnostic attributes will become a breaking change as well.

[petrochenkov]

otherwise this is going to be a significant breakage

Well, technically there is a way to avoid breakage - by keeping the currently implemented rules :)

But it contradicts to this key point of the RFC:

The syntax for external attributes is carefully designed such that you do not need to do name resolution in order to recognize the attributes. As long as register_attribute_tool(your_tool) is present at the crate root, #[your_tool::your_attribute] will always be an inert attribute you can parse directly; it can never be a re-export of a different item, nor a reference to a local item.

Basically if rustfmt sees #[rustfmt::skip] literally, it knows that it's a tool attribute, and the compiler should either arrive to the same conclusion, or produce and error.

rustfmt treats #[rustfmt::skip] and let x = rustfmt::skip; differently, doesn't recognize the latter and ignores it, so path resolution should either be very conservative (error on any ambiguity, what the RFC proposes), or depend on the outer context of the path (sub-namespaces, more or less what this PR implements).

So the choices are either

• to break things, or
• to not follow the key point of the RFC, or
• to introduce more sub-namespaces.

Sigh.

[petrochenkov]

Note: other attributes that are recognized textually (derive helpers - by proc macros, or built-ins - by the compiler) currently follow the key point of the RFC.
Both of them produce errors on any ambiguities, and employ a sub-namespace in the macro namespace.

[mejrs]

Well, technically there is a way to avoid breakage - by keeping the currently implemented rules :)

But it contradicts to this key point of the RFC:

The syntax for external attributes is carefully designed such that you do not need to do name resolution in order to recognize the attributes. As long as register_attribute_tool(your_tool) is present at the crate root, #[your_tool::your_attribute] will always be an inert attribute you can parse directly; it can never be a re-export of a different item, nor a reference to a local item.

Basically if rustfmt sees #[rustfmt::skip] literally, it knows that it's a tool attribute, and the compiler should either arrive to the same conclusion, or produce and error.

Is this exclusively for the benefit of tools that run pre-(name res/expansion)? Or are there other benefits as well?

[petrochenkov]

@mejrs
Yes, only for pre-expansion tools.
For post-expansion tools it's not an issue, if they see an attribute they recognize it will definitely be an inert tool attribute.
Besides that I don't see any benefits for the language or compiler.

[nbdd0121]

If tool-attributes are always resolved regardless the attribute exists or not (and error raised if there's ambiguity), adding new diagnostics attribute won't be a breaking change (but it will be a breaking change for the initial resolver change, or a new pre-defined tool is added). However the point of adding built-in attribute being a breaking change still stands.

Another approach is to add an ambiguity warning when a macro attribute conflicts with a tool attribute, and we still prefer the macro attribute?