Add MVP suggestion for `unsafe_op_in_unsafe_fn` #112017

Nemo157 · 2023-05-27T12:41:05Z

Rebase of #99827

cc tracking issue #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.

rustbot · 2023-05-27T12:41:13Z

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-05-27T12:41:15Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Nemo157 · 2023-05-27T12:45:08Z

tests/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.mir.stderr

@@ -10,6 +10,14 @@ note: the lint level is defined here
   |
 LL | #![deny(unsafe_op_in_unsafe_fn)]
   |         ^^^^^^^^^^^^^^^^^^^^^^
+help: consider wrapping the function in an unsafe block


Actually, there was one outstanding point:

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.

@Mark-Simulacrum — #99827 (comment)

Any suggestions on whether to remove the message completely, or a better message to replace it with?

The existing suggestion I gave: not providing a in-code suggestion and instead provide just the help text, e.g., "help: an unsafe function restricts callers, but it's body is safe by default".

compiler/rustc_mir_transform/src/errors.rs

eholk

Looks good. I made a few minor suggestions, which you can take or leave at your discretion. Feel free to r=me once you're ready.

eholk · 2023-06-13T00:59:04Z

compiler/rustc_mir_transform/messages.ftl

@@ -55,6 +55,8 @@ mir_transform_unaligned_packed_ref = reference to packed field is unaligned
 mir_transform_union_access_label = access to union field
 mir_transform_union_access_note = the field may not be properly initialized: using uninitialized data will cause undefined behavior
 mir_transform_unsafe_op_in_unsafe_fn = {$details} is unsafe and requires unsafe block (error E0133)
+    .suggestion = consider wrapping the function in an unsafe block


Suggested change

.suggestion = consider wrapping the function in an unsafe block

.suggestion = consider wrapping the function body in an unsafe block

To me wrapping the function in an unsafe block would mean something like this, even though it's nonsensical.

unsafe { unsafe fn foo() { ... } }

eholk · 2023-06-13T01:01:10Z

compiler/rustc_mir_transform/src/errors.rs

@@ -130,6 +131,7 @@ impl RequiresUnsafeDetail {

 pub(crate) struct UnsafeOpInUnsafeFn {
    pub details: RequiresUnsafeDetail,
+    pub suggest_unsafe_block: Option<(Span, Span, Span)>,


Maybe add a doc comment describing what each of the three spans means?

Nemo157 rebase notes: Migrated the changes to the lint into fluent

…ck added

…e macros

Nemo157 · 2023-06-13T13:50:35Z

@bors r=eholk

bors · 2023-06-13T13:50:37Z

📌 Commit 802c1d5 has been approved by eholk

It is now in the queue for this repository.

bors · 2023-06-13T15:58:03Z

⌛ Testing commit 802c1d5 with merge 5683791...

bors · 2023-06-13T19:03:19Z

☀️ Test successful - checks-actions
Approved by: eholk
Pushing 5683791 to master...

rust-timer · 2023-06-13T20:19:05Z

Finished benchmarking commit (5683791): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.795s -> 648.13s (0.05%)

…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

…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

rustbot assigned eholk May 27, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 27, 2023

Nemo157 commented May 27, 2023

View reviewed changes

compiler/rustc_mir_transform/src/errors.rs Outdated Show resolved Hide resolved

Nemo157 force-pushed the unsafe-block-rustfix branch from 1ba07cb to ae5f4cd Compare May 28, 2023 10:58

Nemo157 mentioned this pull request May 28, 2023

Change unsafe_op_in_unsafe_fn to be warn-by-default from edition 2024 #112038

Merged

Dylan-DPC added the A-edition-2024 Area: The 2024 edition label May 28, 2023

This comment was marked as outdated.

Sign in to view

Nemo157 force-pushed the unsafe-block-rustfix branch from 3016a8e to 51e330a Compare June 11, 2023 10:16

This comment was marked as outdated.

Sign in to view

Nemo157 force-pushed the unsafe-block-rustfix branch from 51e330a to d0298c1 Compare June 11, 2023 11:56

eholk approved these changes Jun 13, 2023

View reviewed changes

ojeda mentioned this pull request Jun 13, 2023

Rust wanted features Rust-for-Linux/linux#354

Open

36 tasks

LeSeulArtichaut and others added 5 commits June 13, 2023 15:46

Add MVP suggestion for unsafe_op_in_unsafe_fn

975152c

Nemo157 rebase notes: Migrated the changes to the lint into fluent

Test that a couple more types of unsafe-ops get a wrapping unsafe blo…

aca61b2

…ck added

Hide suggestion to wrap function in unsafe block

62a712a

Add note about unsafe functions body not being unsafe

8f3e876

Add test cases for suggestions with unsafe operations contained insid…

802c1d5

…e macros

Nemo157 force-pushed the unsafe-block-rustfix branch from d0298c1 to 802c1d5 Compare June 13, 2023 13:49

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 Jun 13, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 13, 2023

bors merged commit 5683791 into rust-lang:master Jun 13, 2023

rustbot added this to the 1.72.0 milestone Jun 13, 2023

bors mentioned this pull request Jun 13, 2023

Fix target_feature 1.1 should print the list of missing target features #109710

Closed

Nemo157 deleted the unsafe-block-rustfix branch June 13, 2023 19:44

Nemo157 mentioned this pull request Sep 23, 2023

Tracking Issue for "unsafe blocks in unsafe fn" (RFC #2585) #71668

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MVP suggestion for `unsafe_op_in_unsafe_fn` #112017

Add MVP suggestion for `unsafe_op_in_unsafe_fn` #112017

Nemo157 commented May 27, 2023

rustbot commented May 27, 2023

rustbot commented May 27, 2023

Nemo157 May 27, 2023

Mark-Simulacrum May 27, 2023

This comment was marked as outdated.

This comment was marked as outdated.

eholk left a comment

eholk Jun 13, 2023

eholk Jun 13, 2023

Nemo157 commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

rust-timer commented Jun 13, 2023

	.suggestion = consider wrapping the function in an unsafe block
	.suggestion = consider wrapping the function body in an unsafe block

Add MVP suggestion for unsafe_op_in_unsafe_fn #112017

Add MVP suggestion for unsafe_op_in_unsafe_fn #112017

Conversation

Nemo157 commented May 27, 2023

rustbot commented May 27, 2023

rustbot commented May 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

eholk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nemo157 commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

bors commented Jun 13, 2023

rust-timer commented Jun 13, 2023

Overall result: ❌ regressions - no action needed

Add MVP suggestion for `unsafe_op_in_unsafe_fn` #112017

Add MVP suggestion for `unsafe_op_in_unsafe_fn` #112017