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) |
||
---|---|---|
.. | ||
assembly | ||
auxiliary | ||
codegen | ||
codegen-units | ||
coverage | ||
coverage-run-rustdoc | ||
crashes | ||
debuginfo | ||
incremental | ||
mir-opt | ||
pretty | ||
run-make | ||
run-pass-valgrind | ||
rustdoc | ||
rustdoc-gui | ||
rustdoc-js | ||
rustdoc-js-std | ||
rustdoc-json | ||
rustdoc-ui | ||
ui | ||
ui-fulldeps | ||
COMPILER_TESTS.md |