Port improved unused_unsafe check to `-Zthir_unsafeck` by LeSeulArtichaut · Pull Request #99379 · rust-lang/rust

LeSeulArtichaut · 2022-07-17T14:51:48Z

Rebase of dc1056a as proposed in #93678 (comment), cc @steffahn.

r? @nagisa, do you want to review this as a followup of #93678?

steffahn · 2022-07-17T15:06:35Z

I’ll take a look through the further changes made to the MIR-version that came up during review after I had created dc1056a and see if any of those apply to this code as-well. I can probably start looking into this tomorrow or Tuesday.

Haven’t looked at the diffs so far or tried the rebase myself; I assume this is at the moment just a rebase and there weren’t any hard to solve merge conflicts?

LeSeulArtichaut · 2022-07-17T15:24:00Z

I’ll take a look through the further changes made to the MIR-version that came up during review after I had created dc1056a and see if any of those apply to this code as-well. I can probably start looking into this tomorrow or Tuesday.

Thanks a lot, that'd be great if you still have the squashed commits. I've tried to resolve the differences I could spot but that's nowhere near a guarantee.

Haven’t looked at the diffs so far or tried the rebase myself; I assume this is at the moment just a rebase and there weren’t any hard to solve merge conflicts?

Exactly, just enough for the code to compile and for the tests to pass.

steffahn · 2022-07-17T15:47:41Z

that'd be great if you still have the squashed commits

They’re on GitHub, too. Just look for the “force-pushed” messages in the PR to get the relevant commit hashes to look at (I’d assume that 40b1efa45e79b5925906b632f4a8c109107c36b2 is the relevant before-squashing and before-rebasing commit); commits can also be fetched and checked out on the command-line just like branches (git fetch REMOTE_NAME COMMIT_HASH and git checkout COMMIT_HASH¹).

IIRC you can use any of the GitHub forks as REMOTE_NAME when fetching from GitHub, the set of commits available should be the same between forks; so assuming you have some GitHub fork of rustc configured as origin, you can use git fetch origin 40b1efa4; git checkout 40b1efa4, then inspect the history with git log, look at the dates and so on… (at least that’s what I am going to try) ↩

LeSeulArtichaut · 2022-07-18T22:43:55Z

Okay, so I believe we're looking at 59e19357dfc54744980f256a4105e701f7dd99b1...99a1aa904aceb120220a46954c5d270290d4b471.

3be74c3b52adb7272d27562c3c5fc0fcec7672bc is now irrelevant
I've included relevant patches from c4638d8abad4e33db849cfbcfb7f2b27a0228e9c and 99a1aa904aceb120220a46954c5d270290d4b471
the rest is specific to the MIR implementation or touches rustc_middle and therefore is already applied in master

nagisa · 2022-07-23T12:47:08Z

Can’t currently dedicate the significant amount of attention to reviewing this. Reassigning, but might be able to come back to this later if this doesn’t get reviewed by the time autumn rolls around.

r? rust-lang/compiler

michaelwoerister · 2022-07-26T07:33:09Z

Not really my ballpark, sorry. r? rust-lang/compiler

LeSeulArtichaut · 2022-07-26T10:17:54Z

r? rust-lang/compiler

bors · 2022-08-19T22:55:29Z

☔ The latest upstream changes (presumably #100740) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot

I left some questions. Mainly: why do you need to design this as a second pass over the HIR, and not report this straight while visiting THIR?

cjgillot · 2022-08-20T09:59:19Z

compiler/rustc_mir_build/src/check_unsafety.rs

+        used_unsafe_blocks: visitor.used_unsafe_blocks,
+        context: if body_unsafety.is_unsafe() { Context::UnsafeFn(hir_id) } else { Context::Safe },
+    };
+    intravisit::Visitor::visit_body(&mut visitor2, tcx.hir().body(tcx.hir().body_owned_by(hir_id)));


Why does this traversal happen on HIR and not THIR?

cjgillot · 2022-08-20T10:00:41Z

compiler/rustc_mir_build/src/check_unsafety.rs

+                }
+                (Context::UnsafeBlock(hir_id), Some(_)) => UnusedUnsafe::InUnsafeBlock(hir_id),
+            };
+            report_unused_unsafe(self.tcx, block.hir_id, unused_unsafe);


Why don't we report this at the end of UnsafetyVisitor::in_safety_context?

cjgillot · 2022-08-20T10:03:34Z

compiler/rustc_mir_build/src/check_unsafety.rs

+                };
                closure_visitor.visit_expr(&closure_thir[expr]);
                // Unsafe blocks can be used in closures, make sure to take it into account
                self.safety_context = closure_visitor.safety_context;


safety_context has a stack-like behaviour. Should this be an assert_eq?

estebank · 2022-08-19T19:21:51Z

src/test/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.thir.stderr

-   |     ------   ^^^^^^ unnecessary `unsafe` block
-   |     |
-   |     because it's nested under this `unsafe` block


It's a shame we have this output regression here

estebank · 2022-08-22T09:38:38Z

src/test/ui/span/lint-unused-unsafe.thir.stderr

+error: unnecessary `unsafe` block
+  --> $DIR/lint-unused-unsafe.rs:1054:29
+   |
+LL |             let _ = async { unsafe {
+   |                             ^^^^^^ unnecessary `unsafe` block
+   |


This isn't pointing at the reason, the async fn in 1051. Or is it because there's another unsafe block within it before calling the unsafe fn? Either way, we should be pointing at the reason this is unnecessary, otherwise people will have to either already know what the problem is or start changing things at random.

estebank · 2022-08-22T09:39:13Z

src/test/ui/span/lint-unused-unsafe.thir.stderr

+error: unnecessary `unsafe` block
+  --> $DIR/lint-unused-unsafe.rs:1068:29
+   |
+LL |             let _ = async { unsafe {
+   |                             ^^^^^^ unnecessary `unsafe` block


Another case, it should point at the enclosing async fn

estebank · 2022-08-22T09:39:51Z

src/test/ui/span/lint-unused-unsafe.thir.stderr

+error: unnecessary `unsafe` block
+  --> $DIR/lint-unused-unsafe.rs:1069:33
+   |
+LL |     async unsafe fn async_blocks() {
+   |     ------------------------------ because it's nested under this `unsafe` fn
+...
+LL |                 let _ = async { unsafe { let _ = async { unsf() }; }};
+   |                                 ^^^^^^ unnecessary `unsafe` block


Interestingly, this points at the async fn, even though it is nested inside of another unsafe block.

estebank · 2022-08-22T09:42:35Z

src/test/ui/span/lint-unused-unsafe.thir.stderr

+error: unnecessary `unsafe` block
+  --> $DIR/lint-unused-unsafe.rs:21:13
+   |
+LL | fn bad1() { unsafe {} }
+   |             ^^^^^^ unnecessary `unsafe` block


This should mention that within the unsafe block there are no unsafe operations, so the block is unnecessary.

LeSeulArtichaut · 2022-09-10T12:17:31Z

Thanks a lot for the reviews! Unfortunately I won't have time to continue working on this, sorry...

rust-highfive assigned nagisa Jul 17, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2022

Port improved unused_unsafe check to -Zthir_unsafeck

8809979

LeSeulArtichaut force-pushed the thir-unused-unsafe branch from 10d0898 to 8809979 Compare July 18, 2022 22:43

rust-highfive assigned michaelwoerister and unassigned nagisa Jul 23, 2022

rust-highfive assigned estebank and unassigned michaelwoerister Jul 26, 2022

LeSeulArtichaut mentioned this pull request Aug 2, 2022

never consider unsafe blocks unused if they would be required with deny(unsafe_op_in_unsafe_fn) #100081

Merged

cjgillot reviewed Aug 20, 2022

View reviewed changes

estebank reviewed Aug 22, 2022

View reviewed changes

LeSeulArtichaut closed this Sep 10, 2022

pnkfelix mentioned this pull request Oct 6, 2022

Introduce a no-op PlaceMention statement for let _ =. #102256

Merged

LeSeulArtichaut deleted the thir-unused-unsafe branch November 21, 2022 13:13

Uh oh!

Conversation

LeSeulArtichaut commented Jul 17, 2022

Uh oh!

steffahn commented Jul 17, 2022

Uh oh!

LeSeulArtichaut commented Jul 17, 2022

Uh oh!

steffahn commented Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

LeSeulArtichaut commented Jul 18, 2022

Uh oh!

nagisa commented Jul 23, 2022

Uh oh!

michaelwoerister commented Jul 26, 2022

Uh oh!

LeSeulArtichaut commented Jul 26, 2022

Uh oh!

bors commented Aug 19, 2022

Uh oh!

cjgillot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeSeulArtichaut commented Sep 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

steffahn commented Jul 17, 2022 •

edited

Loading