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

Add MVP suggestion for unsafe_op_in_unsafe_fn #99827

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions compiler/rustc_mir_transform/src/check_unsafety.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::struct_span_err;
use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::hir_id::HirId;
Expand Down Expand Up @@ -569,6 +569,8 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: LocalDefId) {
}

let UnsafetyCheckResult { violations, unused_unsafes, .. } = tcx.unsafety_check_result(def_id);
// Only suggest wrapping the entire function body in an unsafe block once
let mut suggest_unsafe_block = true;

for &UnsafetyViolation { source_info, lint_root, kind, details } in violations.iter() {
let (description, note) = details.description_and_note();
Expand Down Expand Up @@ -597,13 +599,16 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: LocalDefId) {
lint_root,
source_info.span,
|lint| {
lint.build(&format!(
let mut err = lint.build(&format!(
"{} is unsafe and requires unsafe block (error E0133)",
description,
))
.span_label(source_info.span, description)
.note(note)
.emit();
));
err.span_label(source_info.span, description).note(note);
if suggest_unsafe_block {
suggest_wrapping_unsafe_block(tcx, def_id, &mut err);
suggest_unsafe_block = false;
}
err.emit();
},
),
}
Expand All @@ -614,6 +619,25 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: LocalDefId) {
}
}

fn suggest_wrapping_unsafe_block(
tcx: TyCtxt<'_>,
def_id: LocalDefId,
err: &mut DiagnosticBuilder<'_, ()>,
) {
let body_span = tcx.hir().body(tcx.hir().body_owned_by(def_id)).value.span;

let suggestion = vec![
(tcx.sess.source_map().start_point(body_span).shrink_to_hi(), " unsafe {".into()),
(tcx.sess.source_map().end_point(body_span).shrink_to_lo(), "}".into()),
];

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.

);
}

fn unsafe_op_in_unsafe_fn_allowed(tcx: TyCtxt<'_>, id: HirId) -> bool {
tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, id).0 == Level::Allow
}
16 changes: 16 additions & 0 deletions src/test/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.mir.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ note: the lint level is defined here
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
|
LL ~ unsafe fn deny_level() { unsafe {
LL | unsf();
...
LL |
LL ~ }}
|

error: dereference of raw pointer is unsafe and requires unsafe block (error E0133)
--> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:15:5
Expand Down Expand Up @@ -52,6 +60,14 @@ LL | #[deny(warnings)]
| ^^^^^^^^
= note: `#[deny(unsafe_op_in_unsafe_fn)]` implied by `#[deny(warnings)]`
= note: consult the function's documentation for information on how to avoid undefined behavior
help: consider wrapping the function in an unsafe block
|
LL ~ unsafe fn warning_level() { unsafe {
LL | unsf();
...
LL |
LL ~ }}
|

error: dereference of raw pointer is unsafe and requires unsafe block (error E0133)
--> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:31:5
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/unsafe/wrapping-unsafe-block-sugg.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// run-rustfix

#![deny(unsafe_op_in_unsafe_fn)]

unsafe fn unsf() {}

pub unsafe fn foo() { unsafe {
unsf(); //~ ERROR call to unsafe function is unsafe
unsf(); //~ ERROR call to unsafe function is unsafe
}}

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/unsafe/wrapping-unsafe-block-sugg.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// run-rustfix

#![deny(unsafe_op_in_unsafe_fn)]

unsafe fn unsf() {}

pub unsafe fn foo() {
unsf(); //~ ERROR call to unsafe function is unsafe
unsf(); //~ ERROR call to unsafe function is unsafe
}

fn main() {}
30 changes: 30 additions & 0 deletions src/test/ui/unsafe/wrapping-unsafe-block-sugg.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
error: call to unsafe function is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:8:5
|
LL | unsf();
| ^^^^^^ call to unsafe function
|
note: the lint level is defined here
--> $DIR/wrapping-unsafe-block-sugg.rs:3:9
|
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

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.

|
LL ~ pub unsafe fn foo() { unsafe {
LL | unsf();
LL | unsf();
LL ~ }}
|

error: call to unsafe function is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:9:5
|
LL | unsf();
| ^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior

error: aborting due to 2 previous errors