mirror of
https://github.com/rust-lang/rust.git
synced 2025-11-18 03:25:51 +00:00
20480075bd
Do not fire unhandled attribute assertion on multi-segment `AttributeType::Normal` attributes with builtin attribute as first segment ### The Problem In #128581 I introduced an assertion to check that all builtin attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes. Unfortunately, the assertion had correctness problems as revealed in #128622. The match on attribute path segments looked like ```rs,ignore // Normal handler [sym::should_panic] => /* check is implemented */ // Fallback handler [name, ..] => match BUILTIN_ATTRIBUTE_MAP.get(name) { // checked below Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) => {} Some(_) => { if !name.as_str().starts_with("rustc_") { span_bug!( attr.span, "builtin attribute {name:?} not handled by `CheckAttrVisitor`" ) } } None => (), } ``` However, it failed to account for edge cases such as an attribute whose: 1. path segments *starts* with a segment matching the name of a builtin attribute such as `should_panic`, and 2. the first segment's symbol does not start with `rustc_`, and 3. the matched builtin attribute is also of `AttributeType::Normal` attribute type upon registration with the builtin attribute map. These conditions when all satisfied cause the span bug to be issued for e.g. `#[should_panic::skip]` because the `[sym::should_panic]` arm is not matched (since it's `[sym::should_panic, sym::skip]`). ### Proposed Solution This PR tries to remedy that by adjusting all normal/specific handlers to not match exactly on a single segment, but instead match a prefix segment. i.e. ```rs,ignore // Normal handler, notice the `, ..` rest pattern [sym::should_panic, ..] => /* check is implemented */ // Fallback handler [name, ..] => match BUILTIN_ATTRIBUTE_MAP.get(name) { // checked below Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) => {} Some(_) => { if !name.as_str().starts_with("rustc_") { span_bug!( attr.span, "builtin attribute {name:?} not handled by `CheckAttrVisitor`" ) } } None => (), } ``` ### Review Remarks This PR contains 2 commits: 1. The first commit adds a regression test. This will ICE without the `CheckAttrVisitor` changes. 2. The second commit adjusts `CheckAttrVisitor` assertion logic. Once this commit is applied, the test should no longer ICE and produce the expected bless stderr. Fixes #128622. r? ``@nnethercote`` (since you reviewed #128581)