Skip to content

improve error message shown for unsafe operations #52207

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 2 commits into from
Jul 11, 2018
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: 1 addition & 1 deletion src/librustc/ich/impls_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl_stable_hash_for!(struct mir::LocalDecl<'tcx> {
});
impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, var_hir_id, by_ref, mutability });
impl_stable_hash_for!(struct mir::BasicBlockData<'tcx> { statements, terminator, is_cleanup });
impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, kind });
impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, details, kind });
impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_blocks });

impl<'a> HashStable<StableHashingContext<'a>>
Expand Down
1 change: 1 addition & 0 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2377,6 +2377,7 @@ pub enum UnsafetyViolationKind {
pub struct UnsafetyViolation {
pub source_info: SourceInfo,
pub description: InternedString,
pub details: InternedString,
pub kind: UnsafetyViolationKind,
}

Expand Down
58 changes: 43 additions & 15 deletions src/librustc_mir/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
let func_ty = func.ty(self.mir, self.tcx);
let sig = func_ty.fn_sig(self.tcx);
if let hir::Unsafety::Unsafe = sig.unsafety() {
self.require_unsafe("call to unsafe function")
self.require_unsafe("call to unsafe function",
"consult the function's documentation for information on how to avoid \
undefined behavior")
}
}
}
Expand All @@ -112,7 +114,8 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
}

StatementKind::InlineAsm { .. } => {
self.require_unsafe("use of inline assembly")
self.require_unsafe("use of inline assembly",
"inline assembly is entirely unchecked and can cause undefined behavior")
},
}
self.super_statement(block, statement, location);
Expand Down Expand Up @@ -151,6 +154,11 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
self.register_violations(&[UnsafetyViolation {
source_info,
description: Symbol::intern("borrow of packed field").as_interned_str(),
details:
Symbol::intern("fields of packed structs might be misaligned: \
dereferencing a misaligned pointer or even just creating a \
misaligned reference is undefined behavior")
.as_interned_str(),
kind: UnsafetyViolationKind::BorrowPacked(lint_root)
}], &[]);
}
Expand All @@ -172,7 +180,10 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
let base_ty = base.ty(self.mir, self.tcx).to_ty(self.tcx);
match base_ty.sty {
ty::TyRawPtr(..) => {
self.require_unsafe("dereference of raw pointer")
self.require_unsafe("dereference of raw pointer",
"raw pointers may be NULL, dangling or unaligned; they can violate \
aliasing rules and cause data races: all of these are undefined \
behavior")
}
ty::TyAdt(adt, _) => {
if adt.is_union() {
Expand All @@ -190,12 +201,17 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
if elem_ty.moves_by_default(self.tcx, self.param_env,
self.source_info.span) {
self.require_unsafe(
"assignment to non-`Copy` union field")
"assignment to non-`Copy` union field",
"the previous content of the field will be dropped, which \
causes undefined behavior if the field was not properly \
initialized")
} else {
// write to non-move union, safe
}
} else {
self.require_unsafe("access to union field")
self.require_unsafe("access to union field",
"the field may not be properly initialized: using \
uninitialized data will cause undefined behavior")
}
}
}
Expand All @@ -208,14 +224,21 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
}
&Place::Static(box Static { def_id, ty: _ }) => {
if self.tcx.is_static(def_id) == Some(hir::Mutability::MutMutable) {
self.require_unsafe("use of mutable static");
self.require_unsafe("use of mutable static",
"mutable statics can be mutated by multiple threads: aliasing violations \
or data races will cause undefined behavior");
} else if self.tcx.is_foreign_item(def_id) {
let source_info = self.source_info;
let lint_root =
self.source_scope_local_data[source_info.scope].lint_root;
self.register_violations(&[UnsafetyViolation {
source_info,
description: Symbol::intern("use of extern static").as_interned_str(),
details:
Symbol::intern("extern statics are not controlled by the Rust type \
system: invalid data, aliasing violations or data \
races will cause undefined behavior")
.as_interned_str(),
kind: UnsafetyViolationKind::ExternStatic(lint_root)
}], &[]);
}
Expand All @@ -227,12 +250,14 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {

impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
fn require_unsafe(&mut self,
description: &'static str)
description: &'static str,
details: &'static str)
{
let source_info = self.source_info;
self.register_violations(&[UnsafetyViolation {
source_info,
description: Symbol::intern(description).as_interned_str(),
details: Symbol::intern(details).as_interned_str(),
kind: UnsafetyViolationKind::General,
}], &[]);
}
Expand Down Expand Up @@ -437,33 +462,36 @@ pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) {
} = tcx.unsafety_check_result(def_id);

for &UnsafetyViolation {
source_info, description, kind
source_info, description, details, kind
} in violations.iter() {
// Report an error.
match kind {
UnsafetyViolationKind::General => {
struct_span_err!(
tcx.sess, source_info.span, E0133,
"{} requires unsafe function or block", description)
"{} is unsafe and requires unsafe function or block", description)
.span_label(source_info.span, &description.as_str()[..])
.note(&details.as_str()[..])
.emit();
}
UnsafetyViolationKind::ExternStatic(lint_node_id) => {
tcx.lint_node(SAFE_EXTERN_STATICS,
tcx.lint_node_note(SAFE_EXTERN_STATICS,
lint_node_id,
source_info.span,
&format!("{} requires unsafe function or \
block (error E0133)", &description.as_str()[..]));
&format!("{} is unsafe and requires unsafe function or block \
(error E0133)", &description.as_str()[..]),
&details.as_str()[..]);
}
UnsafetyViolationKind::BorrowPacked(lint_node_id) => {
if let Some(impl_def_id) = builtin_derive_def_id(tcx, def_id) {
tcx.unsafe_derive_on_repr_packed(impl_def_id);
} else {
tcx.lint_node(SAFE_PACKED_BORROWS,
tcx.lint_node_note(SAFE_PACKED_BORROWS,
lint_node_id,
source_info.span,
&format!("{} requires unsafe function or \
block (error E0133)", &description.as_str()[..]));
&format!("{} is unsafe and requires unsafe function or block \
(error E0133)", &description.as_str()[..]),
&details.as_str()[..]);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/foreign-unsafe-fn-called.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ mod test {

fn main() {
test::free();
//~^ ERROR call to unsafe function requires unsafe function or block
//~^ ERROR call to unsafe function is unsafe
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

use std::intrinsics::{init};

// Test that the `forget` and `init` intrinsics are really unsafe
// Test that the `init` intrinsic is really unsafe
pub fn main() {
let stuff = init::<isize>(); //~ ERROR call to unsafe function requires unsafe
let stuff = init::<isize>(); //~ ERROR call to unsafe function is unsafe
}
4 changes: 2 additions & 2 deletions src/test/compile-fail/issue-43733.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ fn __getit() -> std::option::Option<
&'static std::cell::UnsafeCell<
std::option::Option<Foo>>>
{
__KEY.get() //~ ERROR call to unsafe function requires unsafe
__KEY.get() //~ ERROR call to unsafe function is unsafe
}

static FOO: std::thread::LocalKey<Foo> =
std::thread::LocalKey::new(__getit, Default::default);
//~^ ERROR call to unsafe function requires unsafe
//~^ ERROR call to unsafe function is unsafe

fn main() {
FOO.with(|foo| println!("{}", foo.borrow()));
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/issue-45087-unreachable-unsafe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@
fn main() {
return;
*(1 as *mut u32) = 42;
//~^ ERROR dereference of raw pointer requires unsafe
//~^ ERROR dereference of raw pointer is unsafe
}
2 changes: 1 addition & 1 deletion src/test/compile-fail/issue-45729-unsafe-in-generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
fn main() {
let _ = || {
*(1 as *mut u32) = 42;
//~^ ERROR dereference of raw pointer requires unsafe
//~^ ERROR dereference of raw pointer is unsafe
yield;
};
}
4 changes: 2 additions & 2 deletions src/test/compile-fail/issue-47412.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ fn union_field() {
union Union { unit: (), void: Void }
let u = Union { unit: () };
match u.void {}
//~^ ERROR access to union field requires unsafe function or block
//~^ ERROR access to union field is unsafe
}

fn raw_ptr_deref() {
let ptr = std::ptr::null::<Void>();
match *ptr {}
//~^ ERROR dereference of raw pointer requires unsafe function or block
//~^ ERROR dereference of raw pointer is unsafe
}

fn main() {}
8 changes: 4 additions & 4 deletions src/test/compile-fail/safe-extern-statics-mut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ extern {
}

fn main() {
let b = B; //~ ERROR use of mutable static requires unsafe function or block
let rb = &B; //~ ERROR use of mutable static requires unsafe function or block
let xb = XB; //~ ERROR use of mutable static requires unsafe function or block
let xrb = &XB; //~ ERROR use of mutable static requires unsafe function or block
let b = B; //~ ERROR use of mutable static is unsafe
let rb = &B; //~ ERROR use of mutable static is unsafe
let xb = XB; //~ ERROR use of mutable static is unsafe
let xrb = &XB; //~ ERROR use of mutable static is unsafe
}
8 changes: 4 additions & 4 deletions src/test/compile-fail/safe-extern-statics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ extern {
}

fn main() {
let a = A; //~ ERROR use of extern static requires unsafe function or block
let a = A; //~ ERROR use of extern static is unsafe
//~^ WARN this was previously accepted by the compiler
let ra = &A; //~ ERROR use of extern static requires unsafe function or block
let ra = &A; //~ ERROR use of extern static is unsafe
//~^ WARN this was previously accepted by the compiler
let xa = XA; //~ ERROR use of extern static requires unsafe function or block
let xa = XA; //~ ERROR use of extern static is unsafe
//~^ WARN this was previously accepted by the compiler
let xra = &XA; //~ ERROR use of extern static requires unsafe function or block
let xra = &XA; //~ ERROR use of extern static is unsafe
//~^ WARN this was previously accepted by the compiler
}
12 changes: 6 additions & 6 deletions src/test/compile-fail/union/union-unsafe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ union U4<T: Copy> {

fn generic_noncopy<T: Default>() {
let mut u3 = U3 { a: T::default() };
u3.a = T::default(); //~ ERROR assignment to non-`Copy` union field requires unsafe
u3.a = T::default(); //~ ERROR assignment to non-`Copy` union field is unsafe
}

fn generic_copy<T: Copy + Default>() {
Expand All @@ -40,16 +40,16 @@ fn generic_copy<T: Copy + Default>() {

fn main() {
let mut u1 = U1 { a: 10 }; // OK
let a = u1.a; //~ ERROR access to union field requires unsafe
let a = u1.a; //~ ERROR access to union field is unsafe
u1.a = 11; // OK
let U1 { a } = u1; //~ ERROR access to union field requires unsafe
if let U1 { a: 12 } = u1 {} //~ ERROR access to union field requires unsafe
let U1 { a } = u1; //~ ERROR access to union field is unsafe
if let U1 { a: 12 } = u1 {} //~ ERROR access to union field is unsafe
// let U1 { .. } = u1; // OK

let mut u2 = U2 { a: String::from("old") }; // OK
u2.a = String::from("new"); //~ ERROR assignment to non-`Copy` union field requires unsafe
u2.a = String::from("new"); //~ ERROR assignment to non-`Copy` union field is unsafe
let mut u3 = U3 { a: 0 }; // OK
u3.a = 1; // OK
let mut u3 = U3 { a: String::from("old") }; // OK
u3.a = String::from("new"); //~ ERROR assignment to non-`Copy` union field requires unsafe
u3.a = String::from("new"); //~ ERROR assignment to non-`Copy` union field is unsafe
}
2 changes: 1 addition & 1 deletion src/test/compile-fail/unsafe-fn-assign-deref-ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@


fn f(p: *mut u8) {
*p = 0; //~ ERROR dereference of raw pointer requires unsafe function or block
*p = 0; //~ ERROR dereference of raw pointer is unsafe
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/unsafe-fn-called-from-safe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@
unsafe fn f() { return; }

fn main() {
f(); //~ ERROR call to unsafe function requires unsafe function or block
f(); //~ ERROR call to unsafe function is unsafe
}
2 changes: 1 addition & 1 deletion src/test/compile-fail/unsafe-fn-deref-ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@


fn f(p: *const u8) -> u8 {
return *p; //~ ERROR dereference of raw pointer requires unsafe function or block
return *p; //~ ERROR dereference of raw pointer is unsafe
}

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/unsafe-fn-used-as-value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ unsafe fn f() { return; }

fn main() {
let x = f;
x(); //~ ERROR call to unsafe function requires unsafe function or block
x(); //~ ERROR call to unsafe function is unsafe
}
2 changes: 1 addition & 1 deletion src/test/compile-fail/unsafe-move-val-init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,5 @@ use std::intrinsics;
// as unsafe.
fn main() {
intrinsics::move_val_init(1 as *mut u32, 1);
//~^ ERROR dereference of raw pointer requires unsafe function or block
//~^ ERROR dereference of raw pointer is unsafe
}
4 changes: 3 additions & 1 deletion src/test/ui/error-codes/E0133.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
error[E0133]: call to unsafe function requires unsafe function or block
error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
--> $DIR/E0133.rs:14:5
|
LL | f();
| ^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior

error: aborting due to previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ fn main() {
let _ = &good.data2[0]; // ok
}

let _ = &good.data; //~ ERROR borrow of packed field requires unsafe
let _ = &good.data; //~ ERROR borrow of packed field is unsafe
//~| hard error
let _ = &good.data2[0]; //~ ERROR borrow of packed field requires unsafe
let _ = &good.data2[0]; //~ ERROR borrow of packed field is unsafe
//~| hard error
let _ = &*good.data; // ok, behind a pointer
let _ = &good.aligned; // ok, has align 1
Expand Down
27 changes: 27 additions & 0 deletions src/test/ui/issue-27060.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error: borrow of packed field is unsafe and requires unsafe function or block (error E0133)
--> $DIR/issue-27060.rs:36:13
|
LL | let _ = &good.data; //~ ERROR borrow of packed field is unsafe
| ^^^^^^^^^^
|
note: lint level defined here
--> $DIR/issue-27060.rs:23:8
|
LL | #[deny(safe_packed_borrows)]
| ^^^^^^^^^^^^^^^^^^^
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>
= note: fields of packed structs might be misaligned: dereferencing a misaligned pointer or even just creating a misaligned reference is undefined behavior

error: borrow of packed field is unsafe and requires unsafe function or block (error E0133)
--> $DIR/issue-27060.rs:38:13
|
LL | let _ = &good.data2[0]; //~ ERROR borrow of packed field is unsafe
| ^^^^^^^^^^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>
= note: fields of packed structs might be misaligned: dereferencing a misaligned pointer or even just creating a misaligned reference is undefined behavior

error: aborting due to 2 previous errors

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really nice, thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

And it took me only three months since you suggested it...^^

4 changes: 3 additions & 1 deletion src/test/ui/issue-28776.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
error[E0133]: call to unsafe function requires unsafe function or block
error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
--> $DIR/issue-28776.rs:14:5
|
LL | (&ptr::write)(1 as *mut _, 42);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
|
= note: consult the function's documentation for information on how to avoid undefined behavior

error: aborting due to previous error

Expand Down
Loading