-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Improve unused_unsafe
lint
#93678
Improve unused_unsafe
lint
#93678
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
By the way, this is the first time I’ve touched rustc source code (prior contributions of mine were standard-library only). Main motivation: Fixes some issues with the current behavior. This PR is more-or-less completely re-implementing the unused_unsafe lint; it’s also only done in the MIR-version of the lint, the set of tests for the On current nightly, unsafe fn unsf() {}
fn inner_ignored() {
unsafe {
#[allow(unused_unsafe)]
unsafe {
unsf()
}
}
} doesn’t create any warnings. This situation is not unrealistic to come by, the inner The reason behind this problem is how the check currently works:
There’s a lot of problems with this approach besides the one presented above. E.g. the MIR-building uses checks for #[allow(unsafe_op_in_unsafe_fn)]
unsafe fn granular_disallow_op_in_unsafe_fn() {
unsafe {
#[deny(unsafe_op_in_unsafe_fn)]
{
unsf();
}
}
}
Here, the intermediate Also closures were problematic and the workaround/algorithms used on current nightly didn’t work properly. (I skipped trying to fully understand what it was supposed to do, because this PR uses a completely different approach.) fn nested() {
unsafe {
unsafe { unsf() }
}
}
vs fn nested() {
let _ = || unsafe {
let _ = || unsafe { unsf() };
};
}
note that this warning kind-of suggests that both unsafe blocks are redundant I also dislike the fact that it always suggests keeping the outermost fn granularity() {
unsafe {
unsafe { unsf() }
unsafe { unsf() }
unsafe { unsf() }
}
} I prefer if
Needless to say, this PR addresses all these points. For context, as far as my understanding goes, the main advantage of skipping inner unsafe blocks was that a test case like fn top_level_used() {
unsafe {
unsf();
unsafe { unsf() }
unsafe { unsf() }
unsafe { unsf() }
}
} should generate some warning because there’s redundant nested As mentioned, during MIR building all the unsafe blocks are kept now, and usage is attributed to them. The way to still generate a warning like
in this case is by emitting a The previous code had a little HIR traversal already anyways to collect a set of all the unsafe blocks (in order to afterwards determine which ones are unused afterwards). This PR uses such a traversal to do additional things including logic like always warn for an The whole logic around One noteworthy design decision I took here: An #![deny(unused_unsafe)]
fn granularity() {
unsafe { //~ ERROR: unnecessary `unsafe` block
unsafe { unsf() }
unsafe { unsf() }
unsafe { unsf() }
}
} warns for the outer #![deny(unused_unsafe)]
fn top_level_ignored() {
#[allow(unused_unsafe)]
unsafe {
#[deny(unused_unsafe)]
{
unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block
unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block
unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block
}
}
} warns on the inner ones. It makes sense to review commit-by-commit; I’ve e.g. split implementation from addressing tests, and within tests, there’s also distinguishing blessing changed output from previous tests from adding new tests. |
Alright, that test failure is interesting… looks like I have some debugging to do. Some problems in handling |
Unsurprisingly, it fails for fn f() {
let x: [(); unsafe { size() }] = [];
}
const unsafe fn size() -> usize { 0 } too. Probably the underlying issues here is that the Hmm, maybe I should go all-in and try to fix #72359? 🤔, ah well, probably better as a separate thing. |
Let’s see if it passes now. |
This comment has been minimized.
This comment has been minimized.
27ea257
to
616482e
Compare
I have approximately zero experience with MIR passes, so it's probably better to find someone else for reviewing this. |
I’m replacing the fix of c14a771 with a different approach. The idea in c14a771 was to still keep the opt-in to visiting all nested bodies, but opt-out of handling anonymous consts. For this to correlate correctly with unsafety checking (which only recurses into closures), it relies on the fact that nested bodies only appear in
The nested-bodies traversal via I’m adding a new commit that uses the default |
I’m adding a fix for #90776. Feel free to tell me if this should rather be in a separate PR. |
I’ve also created a way to integrate this improved I’ll keep that for a separate PR, for if this one gets accepted. |
☔ The latest upstream changes (presumably #92007) made this pull request unmergeable. Please resolve the merge conflicts. |
(The conflict is just some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't reviewed the pass itself yet, but please do rebase. This needs a perf run.
Actually, looks like the PR that had the conflict was reverted 3 days ago, and there is no merge conflict anymore. |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 3cecf81 with merge cba9c92e82f65b5d96c1039773f6812ec659a4f7... |
☀️ Try build successful - checks-actions |
Queued cba9c92e82f65b5d96c1039773f6812ec659a4f7 with parent c5c610a, future comparison URL. |
Finished benchmarking commit (cba9c92e82f65b5d96c1039773f6812ec659a4f7): comparison url. Summary: This benchmark run shows 9 relevant improvements 🎉 but 5 relevant regressions 😿 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. 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 led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
Looks like one of those PRs that introduces using let-else in a lot of places introduced a (trivial to resolve) merge conflict. As before, I'll postpone rebasing until review is complete. |
LGTM. |
So I can rebase? |
Yeah. |
40b1efa
to
99a1aa9
Compare
Done rebasing, and CI is passing. |
Could you also squash the commits together so that a more meaningful history is formed once the merge happens? In particular commits such as those dealing with rustfmt idiosyncrasies, addressing review comments and similar don't really have a lasting historical value. From what I can tell this PR could be 1 to 3 commits, one for lint changes and then the others for adjusting the test suite. |
I thought about this… I’m having a hard time finding any reasonable split, besides integrating everything into a single commit. E.g. tests are already separated by being changes in different files. I guess I can just put everything into a single commit if that’s preferred over the status quo; what do you think? |
A single commit would be good too. It would be awesome if its message is made to contain the text from your long explanation comment too. That way the motivation doesn't get lost when github eventually decides to pull the plug. |
Main motivation: Fixes some issues with the current behavior. This PR is more-or-less completely re-implementing the unused_unsafe lint; it’s also only done in the MIR-version of the lint, the set of tests for the `-Zthir-unsafeck` version no longer succeeds (and is thus disabled, see `lint-unused-unsafe.rs`). On current nightly, ```rs unsafe fn unsf() {} fn inner_ignored() { unsafe { #[allow(unused_unsafe)] unsafe { unsf() } } } ``` doesn’t create any warnings. This situation is not unrealistic to come by, the inner `unsafe` block could e.g. come from a macro. Actually, this PR even includes removal of one unused `unsafe` in the standard library that was missed in a similar situation. (The inner `unsafe` coming from an external macro hides the warning, too.) The reason behind this problem is how the check currently works: * While generating MIR, it already skips nested unsafe blocks (i.e. unsafe nested in other unsafe) so that the inner one is always the one considered unused * To differentiate the cases of no unsafe operations inside the `unsafe` vs. a surrounding `unsafe` block, there’s some ad-hoc magic walking up the HIR to look for surrounding used `unsafe` blocks. There’s a lot of problems with this approach besides the one presented above. E.g. the MIR-building uses checks for `unsafe_op_in_unsafe_fn` lint to decide early whether or not `unsafe` blocks in an `unsafe fn` are redundant and ought to be removed. ```rs unsafe fn granular_disallow_op_in_unsafe_fn() { unsafe { #[deny(unsafe_op_in_unsafe_fn)] { unsf(); } } } ``` ``` error: call to unsafe function is unsafe and requires unsafe block (error E0133) --> src/main.rs:13:13 | 13 | unsf(); | ^^^^^^ call to unsafe function | note: the lint level is defined here --> src/main.rs:11:16 | 11 | #[deny(unsafe_op_in_unsafe_fn)] | ^^^^^^^^^^^^^^^^^^^^^^ = note: consult the function's documentation for information on how to avoid undefined behavior warning: unnecessary `unsafe` block --> src/main.rs:10:5 | 9 | unsafe fn granular_disallow_op_in_unsafe_fn() { | --------------------------------------------- because it's nested under this `unsafe` fn 10 | unsafe { | ^^^^^^ unnecessary `unsafe` block | = note: `#[warn(unused_unsafe)]` on by default ``` Here, the intermediate `unsafe` was ignored, even though it contains a unsafe operation that is not allowed to happen in an `unsafe fn` without an additional `unsafe` block. Also closures were problematic and the workaround/algorithms used on current nightly didn’t work properly. (I skipped trying to fully understand what it was supposed to do, because this PR uses a completely different approach.) ```rs fn nested() { unsafe { unsafe { unsf() } } } ``` ``` warning: unnecessary `unsafe` block --> src/main.rs:10:9 | 9 | unsafe { | ------ because it's nested under this `unsafe` block 10 | unsafe { unsf() } | ^^^^^^ unnecessary `unsafe` block | = note: `#[warn(unused_unsafe)]` on by default ``` vs ```rs fn nested() { let _ = || unsafe { let _ = || unsafe { unsf() }; }; } ``` ``` warning: unnecessary `unsafe` block --> src/main.rs:9:16 | 9 | let _ = || unsafe { | ^^^^^^ unnecessary `unsafe` block | = note: `#[warn(unused_unsafe)]` on by default warning: unnecessary `unsafe` block --> src/main.rs:10:20 | 10 | let _ = || unsafe { unsf() }; | ^^^^^^ unnecessary `unsafe` block ``` *note that this warning kind-of suggests that **both** unsafe blocks are redundant* -------------------------------------------------------------------------------- I also dislike the fact that it always suggests keeping the outermost `unsafe`. E.g. for ```rs fn granularity() { unsafe { unsafe { unsf() } unsafe { unsf() } unsafe { unsf() } } } ``` I prefer if `rustc` suggests removing the more-course outer-level `unsafe` instead of the fine-grained inner `unsafe` blocks, which it currently does on nightly: ``` warning: unnecessary `unsafe` block --> src/main.rs:10:9 | 9 | unsafe { | ------ because it's nested under this `unsafe` block 10 | unsafe { unsf() } | ^^^^^^ unnecessary `unsafe` block | = note: `#[warn(unused_unsafe)]` on by default warning: unnecessary `unsafe` block --> src/main.rs:11:9 | 9 | unsafe { | ------ because it's nested under this `unsafe` block 10 | unsafe { unsf() } 11 | unsafe { unsf() } | ^^^^^^ unnecessary `unsafe` block warning: unnecessary `unsafe` block --> src/main.rs:12:9 | 9 | unsafe { | ------ because it's nested under this `unsafe` block ... 12 | unsafe { unsf() } | ^^^^^^ unnecessary `unsafe` block ``` -------------------------------------------------------------------------------- Needless to say, this PR addresses all these points. For context, as far as my understanding goes, the main advantage of skipping inner unsafe blocks was that a test case like ```rs fn top_level_used() { unsafe { unsf(); unsafe { unsf() } unsafe { unsf() } unsafe { unsf() } } } ``` should generate some warning because there’s redundant nested `unsafe`, however every single `unsafe` block _does_ contain some statement that uses it. Of course this PR doesn’t aim change the warnings on this kind of code example, because the current behavior, warning on all the inner `unsafe` blocks, makes sense in this case. As mentioned, during MIR building all the unsafe blocks *are* kept now, and usage is attributed to them. The way to still generate a warning like ``` warning: unnecessary `unsafe` block --> src/main.rs:11:9 | 9 | unsafe { | ------ because it's nested under this `unsafe` block 10 | unsf(); 11 | unsafe { unsf() } | ^^^^^^ unnecessary `unsafe` block | = note: `#[warn(unused_unsafe)]` on by default warning: unnecessary `unsafe` block --> src/main.rs:12:9 | 9 | unsafe { | ------ because it's nested under this `unsafe` block ... 12 | unsafe { unsf() } | ^^^^^^ unnecessary `unsafe` block warning: unnecessary `unsafe` block --> src/main.rs:13:9 | 9 | unsafe { | ------ because it's nested under this `unsafe` block ... 13 | unsafe { unsf() } | ^^^^^^ unnecessary `unsafe` block ``` in this case is by emitting a `unused_unsafe` warning for all of the `unsafe` blocks that are _within a **used** unsafe block_. The previous code had a little HIR traversal already anyways to collect a set of all the unsafe blocks (in order to afterwards determine which ones are unused afterwards). This PR uses such a traversal to do additional things including logic like _always_ warn for an `unsafe` block that’s inside of another **used** unsafe block. The traversal is expanded to include nested closures in the same go, this simplifies a lot of things. The whole logic around `unsafe_op_in_unsafe_fn` is a little complicated, there’s some test cases of corner-cases in this PR. (The implementation involves differentiating between whether a used unsafe block was used exclusively by operations where `allow(unsafe_op_in_unsafe_fn)` was active.) The main goal was to make sure that code should compile successfully if all the `unused_unsafe`-warnings are addressed _simultaneously_ (by removing the respective `unsafe` blocks) no matter how complicated the patterns of `unsafe_op_in_unsafe_fn` being disallowed and allowed throughout the function are. -------------------------------------------------------------------------------- One noteworthy design decision I took here: An `unsafe` block with `allow(unused_unsafe)` **is considered used** for the purposes of linting about redundant contained unsafe blocks. So while ```rs fn granularity() { unsafe { //~ ERROR: unnecessary `unsafe` block unsafe { unsf() } unsafe { unsf() } unsafe { unsf() } } } ``` warns for the outer `unsafe` block, ```rs fn top_level_ignored() { #[allow(unused_unsafe)] unsafe { #[deny(unused_unsafe)] { unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block unsafe { unsf() } //~ ERROR: unnecessary `unsafe` block } } } ``` warns on the inner ones.
99a1aa9
to
8f8689f
Compare
Alright, I’ve added the motivation to the commit message. As you can see, the code is unchanged: https://github.com/rust-lang/rust/compare/99a1aa904aceb120220a46954c5d270290d4b471..8f8689fb31a4ca67b348198a082dcc81acbb89ba, so CI will still pass. |
@bors r+ |
📌 Commit 8f8689f has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (45e2c28): comparison url. Summary: This benchmark run did not return any relevant results. 11 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Updates THIR behavior to match the changes from rust-lang#93678
Updates THIR behavior to match the changes from rust-lang#93678
…r=oli-obk Update THIR unused_unsafe lint Updates THIR unsafeck behaviour to match the changes from rust-lang#93678
Rollup merge of rust-lang#117158 - matthewjasper:thir-unused-unsafe, r=oli-obk Update THIR unused_unsafe lint Updates THIR unsafeck behaviour to match the changes from rust-lang#93678
I’m going to add some motivation and explanation below, particularly pointing the changes in behavior from this PR.
Edit: Looking for existing issues, looks like this PR fixes #88260.
Edit2: Now also contains code that closes #90776.