Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not fire unhandled attribute assertion on multi-segment AttributeType::Normal attributes with builtin attribute as first segment #128623

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Aug 4, 2024

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

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

// 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)

PR rust-lang#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

```rust,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]`).

See <rust-lang#128622>.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 4, 2024
@jieyouxu jieyouxu changed the title Do not fire unhandled attribute assertion on multi-segment AttributeType::CrateLevel attributes with builtin attribute as first segment Do not fire unhandled attribute assertion on multi-segment AttributeType::Normal attributes with builtin attribute as first segment Aug 4, 2024
@nnethercote
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2024

📌 Commit 9d0eaa2 has been approved by nnethercote

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2024
…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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#127655 (turn `invalid_type_param_default` into a `FutureReleaseErrorReportInDeps`)
 - rust-lang#127907 (built-in derive: remove BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE hack and lint)
 - rust-lang#127974 (force compiling std from source if modified)
 - rust-lang#128309 (Implement cursors for `BTreeSet`)
 - rust-lang#128500 (Add test for updating enum discriminant through pointer)
 - rust-lang#128623 (Do not fire unhandled attribute assertion on multi-segment `AttributeType::Normal` attributes with builtin attribute as first segment)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2048007 into rust-lang:master Aug 5, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
Rollup merge of rust-lang#128623 - jieyouxu:check-attr-ice, r=nnethercote

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)
@rustbot rustbot added this to the 1.82.0 milestone Aug 5, 2024
@jieyouxu jieyouxu deleted the check-attr-ice branch August 5, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: builtin attribute "should_panic" not handled by CheckAttrVisitor
4 participants