Skip to content

Add MVP suggestion for unsafe_op_in_unsafe_fn #112017

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

Merged
merged 5 commits into from
Jun 13, 2023
Merged
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
2 changes: 2 additions & 0 deletions compiler/rustc_mir_transform/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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 body in an unsafe block
.note = an unsafe function restricts its caller, but its body is safe by default

mir_transform_unused_unsafe = unnecessary `unsafe` block
.label = because it's nested under this `unsafe` block
Expand Down
31 changes: 25 additions & 6 deletions compiler/rustc_mir_transform/src/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,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 details = errors::RequiresUnsafeDetail { violation: details, span: source_info.span };
Expand Down Expand Up @@ -561,12 +563,29 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: LocalDefId) {
op_in_unsafe_fn_allowed,
});
}
UnsafetyViolationKind::UnsafeFn => tcx.emit_spanned_lint(
UNSAFE_OP_IN_UNSAFE_FN,
lint_root,
source_info.span,
errors::UnsafeOpInUnsafeFn { details },
),
UnsafetyViolationKind::UnsafeFn => {
tcx.emit_spanned_lint(
UNSAFE_OP_IN_UNSAFE_FN,
lint_root,
source_info.span,
errors::UnsafeOpInUnsafeFn {
details,
suggest_unsafe_block: suggest_unsafe_block.then(|| {
let hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
let fn_sig = tcx
.hir()
.fn_sig_by_hir_id(hir_id)
.expect("this violation only occurs in fn");
let body = tcx.hir().body_owned_by(def_id);
let body_span = tcx.hir().body(body).value.span;
let start = tcx.sess.source_map().start_point(body_span).shrink_to_hi();
let end = tcx.sess.source_map().end_point(body_span).shrink_to_lo();
(start, end, fn_sig.span)
}),
},
);
suggest_unsafe_block = false;
}
}
}

Expand Down
25 changes: 20 additions & 5 deletions compiler/rustc_mir_transform/src/errors.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use rustc_errors::{
DecorateLint, DiagnosticBuilder, DiagnosticMessage, EmissionGuarantee, Handler, IntoDiagnostic,
Applicability, DecorateLint, DiagnosticBuilder, DiagnosticMessage, EmissionGuarantee, Handler,
IntoDiagnostic,
};
use rustc_macros::{Diagnostic, LintDiagnostic, Subdiagnostic};
use rustc_middle::mir::{AssertKind, UnsafetyViolationDetails};
Expand Down Expand Up @@ -130,6 +131,12 @@ impl RequiresUnsafeDetail {

pub(crate) struct UnsafeOpInUnsafeFn {
pub details: RequiresUnsafeDetail,

/// These spans point to:
/// 1. the start of the function body
/// 2. the end of the function body
/// 3. the function signature
pub suggest_unsafe_block: Option<(Span, Span, Span)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

impl<'a> DecorateLint<'a, ()> for UnsafeOpInUnsafeFn {
Expand All @@ -138,13 +145,21 @@ impl<'a> DecorateLint<'a, ()> for UnsafeOpInUnsafeFn {
self,
diag: &'b mut DiagnosticBuilder<'a, ()>,
) -> &'b mut DiagnosticBuilder<'a, ()> {
let desc = diag
.handler()
.expect("lint should not yet be emitted")
.eagerly_translate_to_string(self.details.label(), [].into_iter());
let handler = diag.handler().expect("lint should not yet be emitted");
let desc = handler.eagerly_translate_to_string(self.details.label(), [].into_iter());
diag.set_arg("details", desc);
diag.span_label(self.details.span, self.details.label());
diag.note(self.details.note());

if let Some((start, end, fn_sig)) = self.suggest_unsafe_block {
diag.span_note(fn_sig, crate::fluent_generated::mir_transform_note);
diag.tool_only_multipart_suggestion(
crate::fluent_generated::mir_transform_suggestion,
vec![(start, " unsafe {".into()), (end, "}".into())],
Applicability::MaybeIncorrect,
);
}

diag
}

Expand Down
4 changes: 4 additions & 0 deletions tests/ui/unsafe/auxiliary/external_unsafe_macro.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
pub unsafe fn unsf() {}

#[macro_export]
macro_rules! unsafe_macro { () => ($crate::unsf()) }
10 changes: 10 additions & 0 deletions tests/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.mir.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ LL | unsf();
| ^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:11:1
|
LL | unsafe fn deny_level() {
| ^^^^^^^^^^^^^^^^^^^^^^
note: the lint level is defined here
--> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:4:9
|
Expand Down Expand Up @@ -46,6 +51,11 @@ LL | unsf();
| ^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:27:1
|
LL | unsafe fn warning_level() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
note: the lint level is defined here
--> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:26:8
|
Expand Down
66 changes: 66 additions & 0 deletions tests/ui/unsafe/wrapping-unsafe-block-sugg.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// run-rustfix
// aux-build:external_unsafe_macro.rs

#![deny(unsafe_op_in_unsafe_fn)] //~ NOTE

extern crate external_unsafe_macro;

unsafe fn unsf() {}

pub unsafe fn foo() { unsafe {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
unsf(); //~ ERROR call to unsafe function is unsafe
//~^ NOTE
//~| NOTE
unsf(); //~ ERROR call to unsafe function is unsafe
//~^ NOTE
//~| NOTE
}}

pub unsafe fn bar(x: *const i32) -> i32 { unsafe {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
let y = *x; //~ ERROR dereference of raw pointer is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
y + *x //~ ERROR dereference of raw pointer is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
}}

static mut BAZ: i32 = 0;
pub unsafe fn baz() -> i32 { unsafe {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
let y = BAZ; //~ ERROR use of mutable static is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
y + BAZ //~ ERROR use of mutable static is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
}}

macro_rules! unsafe_macro { () => (unsf()) }
//~^ ERROR call to unsafe function is unsafe
//~| NOTE
//~| NOTE
//~| ERROR call to unsafe function is unsafe
//~| NOTE
//~| NOTE

pub unsafe fn unsafe_in_macro() { unsafe {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
unsafe_macro!();
//~^ NOTE
//~| NOTE
unsafe_macro!();
//~^ NOTE
//~| NOTE
}}

pub unsafe fn unsafe_in_external_macro() {
// FIXME: https://github.com/rust-lang/rust/issues/112504
// FIXME: ~^ NOTE an unsafe function restricts its caller, but its body is safe by default
external_unsafe_macro::unsafe_macro!();
external_unsafe_macro::unsafe_macro!();
}

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

#![deny(unsafe_op_in_unsafe_fn)] //~ NOTE

extern crate external_unsafe_macro;

unsafe fn unsf() {}

pub unsafe fn foo() {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
unsf(); //~ ERROR call to unsafe function is unsafe
//~^ NOTE
//~| NOTE
unsf(); //~ ERROR call to unsafe function is unsafe
//~^ NOTE
//~| NOTE
}

pub unsafe fn bar(x: *const i32) -> i32 {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
let y = *x; //~ ERROR dereference of raw pointer is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
y + *x //~ ERROR dereference of raw pointer is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
}

static mut BAZ: i32 = 0;
pub unsafe fn baz() -> i32 {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
let y = BAZ; //~ ERROR use of mutable static is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
y + BAZ //~ ERROR use of mutable static is unsafe and requires unsafe block
//~^ NOTE
//~| NOTE
}

macro_rules! unsafe_macro { () => (unsf()) }
//~^ ERROR call to unsafe function is unsafe
//~| NOTE
//~| NOTE
//~| ERROR call to unsafe function is unsafe
//~| NOTE
//~| NOTE

pub unsafe fn unsafe_in_macro() {
//~^ NOTE an unsafe function restricts its caller, but its body is safe by default
unsafe_macro!();
//~^ NOTE
//~| NOTE
unsafe_macro!();
//~^ NOTE
//~| NOTE
}

pub unsafe fn unsafe_in_external_macro() {
// FIXME: https://github.com/rust-lang/rust/issues/112504
// FIXME: ~^ NOTE an unsafe function restricts its caller, but its body is safe by default
external_unsafe_macro::unsafe_macro!();
external_unsafe_macro::unsafe_macro!();
}

fn main() {}
99 changes: 99 additions & 0 deletions tests/ui/unsafe/wrapping-unsafe-block-sugg.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
error: call to unsafe function is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:12:5
|
LL | unsf();
| ^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/wrapping-unsafe-block-sugg.rs:10:1
|
LL | pub unsafe fn foo() {
| ^^^^^^^^^^^^^^^^^^^
note: the lint level is defined here
--> $DIR/wrapping-unsafe-block-sugg.rs:4:9
|
LL | #![deny(unsafe_op_in_unsafe_fn)]
| ^^^^^^^^^^^^^^^^^^^^^^

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

error: dereference of raw pointer is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:22:13
|
LL | let y = *x;
| ^^ dereference of raw pointer
|
= note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/wrapping-unsafe-block-sugg.rs:20:1
|
LL | pub unsafe fn bar(x: *const i32) -> i32 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: dereference of raw pointer is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:25:9
|
LL | y + *x
| ^^ dereference of raw pointer
|
= note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior

error: use of mutable static is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:33:13
|
LL | let y = BAZ;
| ^^^ use of mutable static
|
= note: mutable statics can be mutated by multiple threads: aliasing violations or data races will cause undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/wrapping-unsafe-block-sugg.rs:31:1
|
LL | pub unsafe fn baz() -> i32 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: use of mutable static is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:36:9
|
LL | y + BAZ
| ^^^ use of mutable static
|
= note: mutable statics can be mutated by multiple threads: aliasing violations or data races will cause undefined behavior

error: call to unsafe function is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:41:36
|
LL | macro_rules! unsafe_macro { () => (unsf()) }
| ^^^^^^ call to unsafe function
...
LL | unsafe_macro!();
| --------------- in this macro invocation
|
= note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
--> $DIR/wrapping-unsafe-block-sugg.rs:49:1
|
LL | pub unsafe fn unsafe_in_macro() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: this error originates in the macro `unsafe_macro` (in Nightly builds, run with -Z macro-backtrace for more info)

error: call to unsafe function is unsafe and requires unsafe block (error E0133)
--> $DIR/wrapping-unsafe-block-sugg.rs:41:36
|
LL | macro_rules! unsafe_macro { () => (unsf()) }
| ^^^^^^ call to unsafe function
...
LL | unsafe_macro!();
| --------------- in this macro invocation
|
= note: consult the function's documentation for information on how to avoid undefined behavior
= note: this error originates in the macro `unsafe_macro` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 8 previous errors