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

Add MVP suggestion for unsafe_op_in_unsafe_fn #99827

Closed
wants to merge 1 commit into from
Closed

Add MVP suggestion for unsafe_op_in_unsafe_fn #99827

wants to merge 1 commit into from

Conversation

LeSeulArtichaut
Copy link
Contributor

cc tracking issue #71668

@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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

r? @jackh726

(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 Jul 27, 2022
compiler-errors
compiler-errors reviewed Jul 27, 2022
View reviewed changes
compiler/rustc_mir_transform/src/check_unsafety.rs
err.multipart_suggestion_verbose(
"consider wrapping the function in an unsafe block",
suggestion,
Applicability::MaybeIncorrect,
Copy link
Member

Choose a reason for hiding this comment

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

perhaps HasPlaceholders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs for Applicability state

The suggestion cannot be applied automatically because it will not result in valid Rust code.

But I think the goal is to be able to apply a "quick fix" to make the code compile. Can HasPlaceholders suggestion still be applied "auto-manually"? I have never used rustfix (:sweat_smile:) and I'm not sure how the different Applicability settings behave for the end user.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe not, good point.

@LeSeulArtichaut
Copy link
Contributor Author

Some changes occurred to MIR optimizations

This is actually unsafeck, sorry for the pings everyone. Should MIR unsafeck be excluded from the notification group?

@JakobDegen
Copy link
Contributor

I'm not so sure that we should recommend wrapping the entire function in an unsafe block. I believe the advice is still generally to reduce the scope of unsafe blocks as much as possible, and that should still apply here I think.

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Jul 27, 2022
Open
5 tasks
@LeSeulArtichaut LeSeulArtichaut marked this pull request as draft July 27, 2022 22:50
@saethlin
Copy link
Member

At least in the past, I've gotten the sense that the clippy and to a greater extent the compiler only lint on and make suggestions for things where there is strong community consensus. I do not think there is consensus on where the scope of an unsafe block should go, so I think any suggestion made will be annoying to a significant fraction of users.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 28, 2022 •
edited
Loading

@JakobDegen While that's true in general, based on discussion in the last lang meeting, we generally felt that:

  • There may be an expectation that fine-grained unsafe blocks may each require their own comment on safety considerations, which may not be desirable (e.g. fine-grained unsafe blocks might not finish discharging all safety obligations).
  • Coarse-grained unsafe blocks are a more obvious "placeholder" of an automatic change, and it's easier to split them to taste than to join a pile of small ones (especially if there were multiple in an expression).

Based on that, a coarse-grained block plus a mention of "or use finer-grained unsafe blocks" should be fine here, especially as a first pass.

@JakobDegen
Copy link
Contributor

Hmm, ok. I'm not so sure I agree, but then I can't really say I'm a fan of the alternative either so it doesn't really make a difference to me

Mark-Simulacrum
Mark-Simulacrum reviewed Jul 30, 2022
View reviewed changes
src/test/ui/unsafe/wrapping-unsafe-block-sugg.stderr
LL | #![deny(unsafe_op_in_unsafe_fn)]
| ^^^^^^^^^^^^^^^^^^^^^^
= note: consult the function's documentation for information on how to avoid undefined behavior
help: consider wrapping the function in an unsafe block
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this help text is useful -- I assume it's not necessary for the suggestion to be emitted for us to show it to humans too?

I feel like something more human-applicable here would be better, along the lines of "help: an unsafe function restricts callers, but it's body is safe by default". But I'm struggling with something succinct here.

It's probably a good idea to update the text in E0133 with some more expansive commentary on the effects of unsafe_op_in_unsafe_fn in any case, since it currently says nothing about that case.

Copy link
Contributor Author

@LeSeulArtichaut LeSeulArtichaut Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure that this help text is useful -- I assume it's not necessary for the suggestion to be emitted for us to show it to humans too?

We could show only the message without the code, or hide the suggestion entirely.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Aug 19, 2022

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

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2022
@jackh726
Copy link
Member

@LeSeulArtichaut changed this since it's marked Draft, but let me know if you'd rather have a review before continuing

@LeSeulArtichaut
Copy link
Contributor Author

I won't have time to take care of this, sorry...

@LeSeulArtichaut LeSeulArtichaut deleted the unsafe-block-rustfix branch November 21, 2022 13:12
This was referenced May 27, 2023
Merged
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 29, 2023
@matthiaskrgr
Rollup merge of rust-lang#112037 - Nemo157:e0133-unsafe_op_in_unsafe_…
c5a93ba 
…fn, r=petrochenkov

Add details about `unsafe_op_in_unsafe_fn` to E0133

This was mentioned in rust-lang#99827 (comment)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2023
@bors
Auto merge of rust-lang#112017 - Nemo157:unsafe-block-rustfix, r=eholk
5683791 
Add MVP suggestion for `unsafe_op_in_unsafe_fn`

Rebase of rust-lang#99827

cc tracking issue rust-lang#71668

No real changes since the original PR, just migrated the new suggestion to use fluent messages and added a couple more testcases, AFAICT from the discussion there were no outstanding changes requested.
oli-obk pushed a commit to oli-obk/miri that referenced this pull request Jun 15, 2023
@bors
Auto merge of #112017 - Nemo157:unsafe-block-rustfix, r=eholk
c2ff66e 
Add MVP suggestion for `unsafe_op_in_unsafe_fn`

Rebase of rust-lang/rust#99827

cc tracking issue rust-lang/rust#71668

No real changes since the original PR, just migrated the new suggestion to use fluent messages and added a couple more testcases, AFAICT from the discussion there were no outstanding changes requested.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
@bors
Auto merge of #112017 - Nemo157:unsafe-block-rustfix, r=eholk
932cc99 
Add MVP suggestion for `unsafe_op_in_unsafe_fn`

Rebase of rust-lang/rust#99827

cc tracking issue rust-lang/rust#71668

No real changes since the original PR, just migrated the new suggestion to use fluent messages and added a couple more testcases, AFAICT from the discussion there were no outstanding changes requested.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
@bors
Auto merge of #112017 - Nemo157:unsafe-block-rustfix, r=eholk
6080b9c 
Add MVP suggestion for `unsafe_op_in_unsafe_fn`

Rebase of rust-lang/rust#99827

cc tracking issue rust-lang/rust#71668

No real changes since the original PR, just migrated the new suggestion to use fluent messages and added a couple more testcases, AFAICT from the discussion there were no outstanding changes requested.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshtriplett joshtriplett joshtriplett left review comments

@RalfJung RalfJung RalfJung left review comments

@compiler-errors compiler-errors compiler-errors left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@jackh726 jackh726

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.