resolve paths in rustc_on_unimplemented

[mejrs]

View all comments

This creates a new (unstable) #[diagnostic::rustc_on_unimplemented] attribute that compares types by defid, rather than stringly at error reporting time. This is necessary to finally fix issues like #112923. This is the first step in our (@estebank and mine) plans to eventually stabilize a subset of rustc_on_unimplemented's filtering api.

It does this by acting as an attribute macro which passes the attribute through ast and resolving and then emits it as an attribute during ast lowering.

For simplicity the functionality is rather limited at the time, that's to be expanded in follow up prs:

• can only resolve identifiers; allowing multi-segment paths and args will require pulling in/writing a parser
• can only resolve adts (not primitives, tuples, slices, pointers etc) atm
• this emits errors, these will have to be lints eventually
• the attribute is lost during token roundstrips, we'll need something like Fix ICE when combining #[eii] with #[core::contracts::ensures] #153796
• PrintAttribute and ast_pretty impls need improvement

r? @petrochenkov
@JonathanBrouwer you might be interested in reviewing as well
cc @estebank @weiznich
@BarronKane you might be interested in looking at the implementation

[rustbot]

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[mejrs]

These are switched during ast lowering, so we never have a Path variant inside a hir::Attribute. An alternative is to make NameValue generic over these two but that introduces a lot of generics everywhere. I tried that first and it was quite messy.

View changes since the review

[mejrs]

I'm not sure about #[visitable(ignore)] here and elsewhere. Can I just put these on everything that doesn't have something "ast-y" like NodeId in it?

View changes since the review

[mejrs]

I decided on a new variant for several reasons:

• I wanted to use ArgParser so I can reuse the implementation in rustc_attr_parsing, keeping that and a MetaItem impl at the same time is too error prone.
• I would like to experiment with making this masquerade like an actual inert attribute so this can be stabilized without it being observable by other macros.

View changes since the review

[rustbot]

The Clippy subtree was changed

cc @rust-lang/clippy

[rustbot]

The Rustfmt subtree was changed

cc @rust-lang/rustfmt

[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.

[petrochenkov]

Is DSL used by rustc_on_unimplemented documented anywhere, besides the source code itself?
Looks like it grew quite a few bells and whistles over the years, some of which are hard to implement correctly.

[mejrs]

It is documented at https://rustc-dev-guide.rust-lang.org/diagnostics.html#rustc_on_unimplemented

[petrochenkov]

(I started reviewing and will continue either tomorrow or on Friday.)

[petrochenkov]

As you could guess from my previous comments in the related threads (paths and types in #[repr] attributes and similar), I don't like any of this.

• AST (or HIR) leaking into Attribute IR, node ids in particular
• inert attributes being actually active
• #[rustc_on_unimplemented] being generally based on hacks, without any regard to the compiler's architecture

[petrochenkov]

If there's growing desire to add some activity to a number of built-in attributes, and there's no desire to turn all of those attributes into macros (which are active by definition), then I guess we have to make some framework for adding more active built-in attributes. This is unfortunate, but maybe bearable.

[petrochenkov]

The general problem is as follows:

• Each built-in attribute has an arbitrary DSL.
• Some attributes want to parse this DSL and in addition to Attribute IR generate some "actual code", i.e. AST nodes, from some parts of the DSL.
  • those added AST nodes are then processed as regular AST - expanded, resolved, lowered to HIR, potentially to THIR/MIR as well, etc.
  • the repr-alias proposal (repr(tag = ...) for type aliases rfcs#3659), would be a good example for this, it wants to support full type nodes, without limits like restricting everything to single identifiers, that allow to solve the limited problem with hacks.

[petrochenkov]

I think it's important for the generated AST (and then HIR, etc) to not be a part of Attribute IR.
In that case the Attribute IR would be usable at all stages of compilation (without dependencies on AST or HIR or other later IRs, #155237 (comment)), and directly encodable/decodable to/from metadata.
The extra AST generated by active attributes could be encoded to metadata through queries, like with any other non-attribute parts of AST/HIR.

I'd also like to decouple the AST generation from rustc_attr_parsing/AttributeIR, because the former needs to know about AST (it's like rustc_parse), and the latter do not (and should not).
Probably through some dyn-trait interface similar to macro expanders (defined in rustc_builtin_macros, called from rustc_expand).

This logic would probably run from rustc_expand immediately on encountering an active built-in attribute, like with the currently existing active built-in attributes (cfg and cfg_attr).

There's some choice in where to put the AST nodes generated by the active attributes.

• They can put them somewhere outside of ast::Attribute, like this PR puts on_unimplemented into struct Trait, or #[define_opaque] (which is currently a macro, but fits into the picture) puts define_opaque into struct Fn.
• Or we can add an ast: Option<AttrAst> node to ast::Attribute (or perhaps ast::{NormalAttr,AttrItem}) to centralize the place for storing the attribute-generated AST nodes.

This is sort of a stylistic choice, in either location they would be processed by all the regular AST passes, visited by AST visitors, etc. Semi-relevant comment - #154708 (comment).
The important part is that they won't be a part of Attribute IR.

[petrochenkov]

One small extra bit of novelty that this PR adds is adding not just an active built-in attribute, but an active built-in tool attribute.
So far tool attributes were never active and were never considered built-in by rustc_(expand,resolve).
Perhaps it makes sense to avoid this as a first step and add some #rustc_on_unimplemented2] instead.

[petrochenkov]

(Not sure what is the correct label here now, something like waiting-on-design, but marking as waiting-on-author to maintain my review queue.)