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

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

Merged
merged 4 commits into from
Aug 19, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 2, 2022

Judging from #71668 (comment) the consensus nowadays seems to be that we should never consider an unsafe block unused if it was required with deny(unsafe_op_in_unsafe_fn), no matter whether that lint is actually enabled or not. So let's adjust rustc accordingly.

The first commit does the change, the 2nd does some cleanup.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 2, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2022
@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2022

Note that I barely understand the code I am touching here, so I hope this makes sense. I did this entirely based on getting the tests to behave the way I want them to...

@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2022

Ah, that conflict is a bummer. I can rebase but then I cannot locally test this any more due to #100062.

@RalfJung RalfJung force-pushed the unused-unsafe-in-unsafe-fn branch from 16ef42f to 3e44ca9 Compare August 2, 2022 21:14
@LeSeulArtichaut
Copy link
Contributor

This probably conflicts with #99379

@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2022

@LeSeulArtichaut ah, that's why the changes I had to do in the two unsafety checkers are so different.

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Aug 2, 2022

I'd still expect the change to THIR unsafeck to be easier to reason about, simply because we don't have to do a separate visit of the HIR to check for unused blocks :)

Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

The changes make sense to me, for what it's worth.

This appears pretty clearly in the first commit, but for the record, the way I understand this change is that since we want to consider that we have #[deny(unsafe_op_in_unsafe_fn)], which means that we always have SomeDisallowedInUnsafeFn.

compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_transform/src/check_unsafety.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2022

This appears pretty clearly in the first commit, but for the record, the way I understand this change is that since we want to consider that we have #[deny(unsafe_op_in_unsafe_fn)], which means that we always have SomeDisallowedInUnsafeFn.

Yeah after doing all this I realized that AllAllowedInUnsafeFn is now redundant. But just from reading the code this was not at all clear. Nothing in the docs of UsedUnsafeBlockData indicates that it has anything to do with unsafe_op_in_unsafe_fn. I thought it had to do with allow(unused_unsafe).

Co-authored-by: Léo Lanteri Thauvin <[email protected]>
@RalfJung
Copy link
Member Author

r? compiler

@rust-highfive rust-highfive assigned oli-obk and unassigned wesleywiser Aug 16, 2022
@RalfJung
Copy link
Member Author

r? compiler

@rust-highfive rust-highfive assigned cjgillot and unassigned oli-obk Aug 16, 2022
@jackh726
Copy link
Member

@bors r+

r? @jackh726

@bors
Copy link
Contributor

bors commented Aug 16, 2022

📌 Commit 86e2ca3 has been approved by jackh726

It is now in the queue for this repository.

@rust-highfive rust-highfive assigned jackh726 and unassigned cjgillot Aug 16, 2022
@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 16, 2022
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Aug 16, 2022
…n, r=jackh726

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

Judging from rust-lang#71668 (comment) the consensus nowadays seems to be that we should never consider an unsafe block unused if it was required with `deny(unsafe_op_in_unsafe_fn)`, no matter whether that lint is actually enabled or not. So let's adjust rustc accordingly.

The first commit does the change, the 2nd does some cleanup.
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Aug 16, 2022
…n, r=jackh726

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

Judging from rust-lang#71668 (comment) the consensus nowadays seems to be that we should never consider an unsafe block unused if it was required with `deny(unsafe_op_in_unsafe_fn)`, no matter whether that lint is actually enabled or not. So let's adjust rustc accordingly.

The first commit does the change, the 2nd does some cleanup.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#99576 (Do not allow `Drop` impl on foreign fundamental types)
 - rust-lang#100081 (never consider unsafe blocks unused if they would be required with deny(unsafe_op_in_unsafe_fn))
 - rust-lang#100208 (make NOP dyn casts not require anything about the vtable)
 - rust-lang#100494 (Cleanup rustdoc themes)
 - rust-lang#100522 (Only check the `DefId` for the recursion check in MIR inliner.)
 - rust-lang#100592 (Manually implement Debug for ImportKind.)
 - rust-lang#100598 (Don't fix builtin index when Where clause is found)
 - rust-lang#100721 (Add diagnostics lints to `rustc_type_ir` module)
 - rust-lang#100731 (rustdoc: count deref and non-deref as same set of used methods)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2fe2975 into rust-lang:master Aug 19, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 19, 2022
@RalfJung RalfJung deleted the unused-unsafe-in-unsafe-fn branch August 21, 2022 12:52
jakobhellermann added a commit to jakobhellermann/bevy that referenced this pull request Sep 9, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2023
…safe_fn, r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang#100081
* Have `cargo fix` able to fix the lint, done in rust-lang#112017
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 15, 2023
…r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang/rust#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang/rust#100081
* Have `cargo fix` able to fix the lint, done in rust-lang/rust#112017
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
…r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang/rust#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang/rust#100081
* Have `cargo fix` able to fix the lint, done in rust-lang/rust#112017
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
…r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang/rust#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang/rust#100081
* Have `cargo fix` able to fix the lint, done in rust-lang/rust#112017
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.

9 participants