forked from rust-lang/rust
-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Rollup merge of rust-lang#128623 - jieyouxu:check-attr-ice, r=nnether…
…cote Do not fire unhandled attribute assertion on multi-segment `AttributeType::Normal` attributes with builtin attribute as first segment ### The Problem In rust-lang#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 rust-lang#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 rust-lang#128622. r? ``@nnethercote`` (since you reviewed rust-lang#128581)
- Loading branch information
Showing
3 changed files
with
152 additions
and
71 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| //! Regression test for #128622. | ||
| //! | ||
| //! PR #128581 introduced an assertion that all builtin attributes are actually checked via | ||
| //! `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes. | ||
| //! Unfortunately, the check had correctness problems. | ||
| //! | ||
| //! The match on attribute path segments looked like | ||
| //! | ||
| //! ```rs,ignore | ||
| //! [sym::should_panic] => /* check is implemented */ | ||
| //! 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 builtin attribute such as `should_panic` | ||
| //! 2. which does not start with `rustc_`, and | ||
| //! 3. is also an `AttributeType::Normal` attribute 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]`). | ||
| //! | ||
| //! This test checks that the span bug is not fired for such cases. | ||
| //! | ||
| //! issue: rust-lang/rust#128622 | ||
|
|
||
| // Notably, `should_panic` is a `AttributeType::Normal` attribute that is checked separately. | ||
|
|
||
| struct Foo { | ||
| #[should_panic::skip] | ||
| //~^ ERROR failed to resolve | ||
| pub field: u8, | ||
|
|
||
| #[should_panic::a::b::c] | ||
| //~^ ERROR failed to resolve | ||
| pub field2: u8, | ||
| } | ||
|
|
||
| fn foo() {} | ||
|
|
||
| fn main() { | ||
| #[deny::skip] | ||
| //~^ ERROR failed to resolve | ||
| foo(); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| error[E0433]: failed to resolve: use of undeclared crate or module `should_panic` | ||
| --> $DIR/check-builtin-attr-ice.rs:43:7 | ||
| | | ||
| LL | #[should_panic::skip] | ||
| | ^^^^^^^^^^^^ use of undeclared crate or module `should_panic` | ||
|
|
||
| error[E0433]: failed to resolve: use of undeclared crate or module `should_panic` | ||
| --> $DIR/check-builtin-attr-ice.rs:47:7 | ||
| | | ||
| LL | #[should_panic::a::b::c] | ||
| | ^^^^^^^^^^^^ use of undeclared crate or module `should_panic` | ||
|
|
||
| error[E0433]: failed to resolve: use of undeclared crate or module `deny` | ||
| --> $DIR/check-builtin-attr-ice.rs:55:7 | ||
| | | ||
| LL | #[deny::skip] | ||
| | ^^^^ use of undeclared crate or module `deny` | ||
|
|
||
| error: aborting due to 3 previous errors | ||
|
|
||
| For more information about this error, try `rustc --explain E0433`. |