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

Fix unused_unsafe label with unsafe_block_in_unsafe_fn #81110

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Fix unused_unsafe label with unsafe_block_in_unsafe_fn #81110

merged 1 commit into from
Jan 19, 2021

Conversation

LeSeulArtichaut
Copy link
Contributor

Previously, the following code:

#![feature(unsafe_block_in_unsafe_fn)]

unsafe fn foo() {
    unsafe { unsf() }
}

unsafe fn unsf() {}

Would give the following warning:

warning: unnecessary `unsafe` block
 --> src/lib.rs:4:5
  |
4 |     unsafe { unsf() }
  |     ^^^^^^ unnecessary `unsafe` block
  |
  = note: `#[warn(unused_unsafe)]` on by default

which doesn't point out that the block is in an unsafe fn.

Tracking issue: #71668
cc #79208

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Jan 17, 2021
@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Jan 17, 2021
Merged
@RalfJung
Copy link
Member

I don't see the problem here? What you describe in the OP is intended behavior!

unused_unsafe should be "in effect" unless the unsafe_block_in_unsafe_fn lint was enabled. feature(unsafe_block_in_unsafe_fn) doesn't enable the lint; warn(unsafe_block_in_unsafe_fn) does.

@RalfJung
Copy link
Member

IOW, the feature gate itself should not change anything (as is common for feature gates). The feature gate merely unlocks the ability to use some other syntax to change something; in this case, that other syntax is warn(unsafe_block_in_unsafe_fn).

@RalfJung
Copy link
Member

Oh, wait, I misunderstood... I thought you wanted to not show the warning, but instead you want to show more information with the warning? I have no idea why that has anything to do with the feature flag though...

RalfJung
RalfJung reviewed Jan 17, 2021
View reviewed changes
src/test/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.stderr
@@ -74,12 +74,18 @@ LL | #![deny(unused_unsafe)]
error: unnecessary `unsafe` block
--> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:51:5
|
LL | unsafe fn allow_level() {
| ----------------------- because it's nested under this `unsafe` fn
...
LL | unsafe { unsf() }
| ^^^^^^ unnecessary `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.

FWIW, this error is kind of confusing because the "because" comes before the thing it is justifying. I guess there's also the error 'title' thing "unnecessary unsafe block", so it's probably okay, but just reading the spans things are in the wrong order... not sure how we usually handle situations where code order and "justification order" disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how the warning is emitted today (without unsafe_block_in_unsafe_fn). I guess any improvement here should be made in another PR

@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Jan 17, 2021 •
edited
Loading

Right, sorry, I was probably not clear enough in the OP. This PR "restores" the "because it's nested under this unsafe fn" label in the warning when the lint is allowed, to match the behavior when the feature gate is not present.

@RalfJung
Copy link
Member

And as a drive-by fix it replaces a String by &'static str (I first thought that was somehow related but it seems not).

@LeSeulArtichaut LeSeulArtichaut added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. F-unsafe-block-in-unsafe-fn RFC #2585 and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2021
@lcnr
Copy link
Contributor

lcnr commented Jan 17, 2021

@RalfJung do you intend to fully review this PR?

@RalfJung
Copy link
Member

I guess at this point I just as well might.^^
r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned lcnr Jan 17, 2021
@LeSeulArtichaut LeSeulArtichaut force-pushed the fix-unused-unsafe-label branch from 5421029 to 2136a5c Compare January 17, 2021 16:04
@LeSeulArtichaut LeSeulArtichaut added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2021
@LeSeulArtichaut
Copy link
Contributor Author

@RalfJung This should be ready for another review

@RalfJung
Copy link
Member

Looking good, thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jan 17, 2021

📌 Commit 2136a5c has been approved by RalfJung

@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 Jan 17, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 18, 2021
@m-ou-se
Rollup merge of rust-lang#81110 - LeSeulArtichaut:fix-unused-unsafe-l…
d68ed11 
…abel, r=RalfJung

Fix `unused_unsafe` label with `unsafe_block_in_unsafe_fn

Previously, the following code:

```rust
#![feature(unsafe_block_in_unsafe_fn)]

unsafe fn foo() {
    unsafe { unsf() }
}

unsafe fn unsf() {}
```

Would give the following warning:

```
warning: unnecessary `unsafe` block
 --> src/lib.rs:4:5
  |
4 |     unsafe { unsf() }
  |     ^^^^^^ unnecessary `unsafe` block
  |
  = note: `#[warn(unused_unsafe)]` on by default
```
which doesn't point out that the block is in an `unsafe fn`.

Tracking issue: rust-lang#71668
cc rust-lang#79208
@m-ou-se m-ou-se mentioned this pull request Jan 18, 2021
Closed
@bors
Copy link
Contributor

bors commented Jan 19, 2021

⌛ Testing commit 2136a5c with merge 47121d6...

@lcnr lcnr changed the title Fix unused_unsafe label with `unsafe_block_in_unsafe_fn Fix unused_unsafe label with unsafe_block_in_unsafe_fn Jan 19, 2021
@bors
Copy link
Contributor

bors commented Jan 19, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 47121d6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 19, 2021
@bors bors merged commit 47121d6 into rust-lang:master Jan 19, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 19, 2021
@LeSeulArtichaut LeSeulArtichaut deleted the fix-unused-unsafe-label branch January 19, 2021 14:33
This was referenced Jun 10, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
F-unsafe-block-in-unsafe-fn RFC #2585 merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.
6 participants
@LeSeulArtichaut @rust-highfive @RalfJung @lcnr @bors @rustbot