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.

Sign up for GitHub

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

Jump to bottom

Assert that all attributes are actually checked via CheckAttrVisitor and aren't accidentally usable on completely unrelated HIR nodes #128581

Merged
merged 2 commits into from
Aug 3, 2024
Merged

Assert that all attributes are actually checked via CheckAttrVisitor and aren't accidentally usable on completely unrelated HIR nodes #128581

merged 2 commits into from
Aug 3, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Aug 3, 2024

@oli-obk's #128444 with unreachable case removed to avoid that PR bitrotting away.
Based on #128402.

This PR will make adding a new attribute ICE on any use of that attribute unless it gets a handler added in rustc_passes::CheckAttrVisitor.

r? @nnethercote (since you were the reviewer of the original PR)

oli-obk and others added 2 commits August 3, 2024 02:26
@oli-obk @jieyouxu
Assert that all attributes are actually checked via `CheckAttrVisitor…
ed010dd 
…` and aren't accidentally usable on completely unrelated HIR nodes

Co-authored-by: Jieyou Xu <[email protected]>
@oli-obk @jieyouxu
Add test for coroutine attribute
33cb334
@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 3, 2024
@jieyouxu jieyouxu mentioned this pull request Aug 3, 2024
Closed
jieyouxu
jieyouxu commented Aug 3, 2024
View reviewed changes
compiler/rustc_passes/src/check_attr.rs
@@ -376,6 +428,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> {

/// Checks that `#[optimize(..)]` is applied to a function/closure/method,
/// or to an impl block or module.
// FIXME(#128488): this should probably be elevated to an error?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this FIXME for #128488, but elevating this to error may require a RFC tweak.

@nnethercote
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 3, 2024

📌 Commit 33cb334 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 3, 2024
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 3, 2024
@tgross35
Rollup merge of rust-lang#128581 - jieyouxu:checked-attr, r=nnethercote
6b3160e 
Assert that all attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes

`@oli-obk's` rust-lang#128444 with unreachable case removed to avoid that PR bitrotting away.
Based on rust-lang#128402.

This PR will make adding a new attribute ICE on any use of that attribute unless it gets a handler added in `rustc_passes::CheckAttrVisitor`.

r? `@nnethercote` (since you were the reviewer of the original PR)
@tgross35 tgross35 mentioned this pull request Aug 3, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
@bors
Auto merge of rust-lang#128608 - tgross35:rollup-kibt0o5, r=tgross35
cacd19d 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127095 (Migrate `reproducible-build-2` and `stable-symbol-names` `run-make` tests to rmake)
 - rust-lang#127921 (Stabilize unsafe extern blocks (RFC 3484))
 - rust-lang#128466 (Update the stdarch submodule)
 - rust-lang#128530 (Implement `UncheckedIterator` directly for `RepeatN`)
 - rust-lang#128581 (Assert that all attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes)
 - rust-lang#128603 (Update run-make/used to use `any_symbol_contains`)

Failed merges:

 - rust-lang#128410 (Migrate `remap-path-prefix-dwarf` `run-make` test to rmake)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 3, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
@bors
Auto merge of rust-lang#128614 - matthiaskrgr:rollup-d2fextz, r=matth…
64ebd39 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#127921 (Stabilize unsafe extern blocks (RFC 3484))
 - rust-lang#128283 (bootstrap: fix bug preventing the use of custom targets)
 - rust-lang#128530 (Implement `UncheckedIterator` directly for `RepeatN`)
 - rust-lang#128551 (chore: refactor backtrace style in panic)
 - rust-lang#128573 (Simplify `body` usage in rustdoc)
 - rust-lang#128581 (Assert that all attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes)
 - rust-lang#128603 (Update run-make/used to use `any_symbol_contains`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3a9d432 into rust-lang:master Aug 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
@rust-timer
Unrolled build for rust-lang#128581
a008b38 
Rollup merge of rust-lang#128581 - jieyouxu:checked-attr, r=nnethercote

Assert that all attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes

``@oli-obk's`` rust-lang#128444 with unreachable case removed to avoid that PR bitrotting away.
Based on rust-lang#128402.

This PR will make adding a new attribute ICE on any use of that attribute unless it gets a handler added in `rustc_passes::CheckAttrVisitor`.

r? ``@nnethercote`` (since you were the reviewer of the original PR)
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 4, 2024
Closed
@jieyouxu jieyouxu deleted the checked-attr branch August 4, 2024 01:09
rust-cloud-vms bot pushed a commit to jieyouxu/rust that referenced this pull request Aug 4, 2024
@jieyouxu
check_attr: cover multi-segment attributes on specific check arms
9d0eaa2 
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>.
@jieyouxu jieyouxu mentioned this pull request Aug 4, 2024
Merged
@clubby789 clubby789 mentioned this pull request Aug 4, 2024
Open
9 tasks
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2024
@matthiaskrgr
Rollup merge of rust-lang#128623 - jieyouxu:check-attr-ice, r=nnether…
30cc5fb 
…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)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2024
@matthiaskrgr
Rollup merge of rust-lang#128623 - jieyouxu:check-attr-ice, r=nnether…
2048007 
…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)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
@rust-timer
Unrolled build for rust-lang#128623
39e577f 
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)
@Kobzol
Copy link
Contributor

Kobzol commented Aug 5, 2024

@rust-timer build a008b38

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a008b38): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (primary -1.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.2% [-1.2%, -1.2%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -1.2% [-1.2%, -1.2%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 758.56s -> 756.614s (-0.26%)
Artifact size: 336.83 MiB -> 336.83 MiB (-0.00%)

@jieyouxu
Copy link
Member Author

jieyouxu commented Aug 5, 2024

Those metrics seem a bit noisy

@Kobzol
Copy link
Contributor

Kobzol commented Aug 5, 2024

I was looking for a particular regression, it wasn't caused by this PR.

@jieyouxu jieyouxu mentioned this pull request Aug 6, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 6, 2024
@matthiaskrgr
Rollup merge of rust-lang#128718 - jieyouxu:check-cfg_attr, r=nnether…
38c2e62 
…cote

Consider `cfg_attr` checked by `CheckAttrVisitor`

I forgor about `cfg_attr` in rust-lang#128581, it should be treated like `cfg`.

Fixes rust-lang#128716.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 6, 2024
@matthiaskrgr
Rollup merge of rust-lang#128718 - jieyouxu:check-cfg_attr, r=nnether…
3158a86 
…cote

Consider `cfg_attr` checked by `CheckAttrVisitor`

I forgor about `cfg_attr` in rust-lang#128581, it should be treated like `cfg`.

Fixes rust-lang#128716.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2024
@rust-timer
Unrolled build for rust-lang#128718
7c5f974 
Rollup merge of rust-lang#128718 - jieyouxu:check-cfg_attr, r=nnethercote

Consider `cfg_attr` checked by `CheckAttrVisitor`

I forgor about `cfg_attr` in rust-lang#128581, it should be treated like `cfg`.

Fixes rust-lang#128716.
@jieyouxu jieyouxu mentioned this pull request Aug 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nnethercote nnethercote

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
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jieyouxu @nnethercote @bors @Kobzol @rust-timer @rustbot @oli-obk