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

Improve uninit/zeroed lint #66044

Merged
merged 3 commits into from
Nov 7, 2019
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
1 change: 1 addition & 0 deletions src/libcore/mem/maybe_uninit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ impl<T> MaybeUninit<T> {
/// ```
#[stable(feature = "maybe_uninit", since = "1.36.0")]
#[inline(always)]
#[cfg_attr(all(not(bootstrap)), rustc_diagnostic_item = "assume_init")]
pub unsafe fn assume_init(self) -> T {
intrinsics::panic_if_uninhabited::<T>();
ManuallyDrop::into_inner(self.value)
Expand Down
3 changes: 3 additions & 0 deletions src/librustc/lint/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,9 @@ impl<'a, 'tcx> LateContext<'a, 'tcx> {

/// Check if a `DefId`'s path matches the given absolute type path usage.
///
/// Anonymous scopes such as `extern` imports are matched with `kw::Invalid`;
/// inherent `impl` blocks are matched with the name of the type.
///
/// # Examples
///
/// ```rust,ignore (no context or def id available)
Expand Down
26 changes: 24 additions & 2 deletions src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1911,8 +1911,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidValue {
// `Invalid` represents the empty string and matches that.
const TRANSMUTE_PATH: &[Symbol] =
&[sym::core, sym::intrinsics, kw::Invalid, sym::transmute];
const MU_ZEROED_PATH: &[Symbol] =
&[sym::core, sym::mem, sym::maybe_uninit, sym::MaybeUninit, sym::zeroed];
const MU_UNINIT_PATH: &[Symbol] =
&[sym::core, sym::mem, sym::maybe_uninit, sym::MaybeUninit, sym::uninit];

if let hir::ExprKind::Call(ref path_expr, ref args) = expr.kind {
// Find calls to `mem::{uninitialized,zeroed}` methods.
if let hir::ExprKind::Path(ref qpath) = path_expr.kind {
let def_id = cx.tables.qpath_res(qpath, path_expr.hir_id).opt_def_id()?;

Expand All @@ -1927,8 +1932,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidValue {
return Some(InitKind::Zeroed);
}
}
// FIXME: Also detect `MaybeUninit::zeroed().assume_init()` and
// `MaybeUninit::uninit().assume_init()`.
}
} else if let hir::ExprKind::MethodCall(_, _, ref args) = expr.kind {
// Find problematic calls to `MaybeUninit::assume_init`.
let def_id = cx.tables.type_dependent_def_id(expr.hir_id)?;
if cx.tcx.is_diagnostic_item(sym::assume_init, def_id) {
// This is a call to *some* method named `assume_init`.
// See if the `self` parameter is one of the dangerous constructors.
if let hir::ExprKind::Call(ref path_expr, _) = args[0].kind {
if let hir::ExprKind::Path(ref qpath) = path_expr.kind {
let def_id = cx.tables.qpath_res(qpath, path_expr.hir_id).opt_def_id()?;
if cx.match_def_path(def_id, MU_ZEROED_PATH) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this (and the other one below) also use diagnostics items?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see #66075. This is partially pre-existing. I didn't feel like rewriting the entire lint at once.^^

Copy link
Contributor

Choose a reason for hiding this comment

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

yea we should be moving away from match_def_path where we can. That's the reason diagnostic items were introduced

Copy link
Contributor

Choose a reason for hiding this comment

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

@RalfJung Sure that's fair about the old code... but these are additions?

Copy link
Member Author

Choose a reason for hiding this comment

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

But I guess this is as good as any a time to resolve the first bullet point from that issue. Not sure when I'll get to it, though.

return Some(InitKind::Zeroed);
} else if cx.match_def_path(def_id, MU_UNINIT_PATH) {
return Some(InitKind::Uninit);
}
}
}
}
}

Expand All @@ -1949,6 +1969,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidValue {
Adt(..) if ty.is_box() => Some((format!("`Box` must be non-null"), None)),
FnPtr(..) => Some((format!("Function pointers must be non-null"), None)),
Never => Some((format!("The never type (`!`) has no valid value"), None)),
RawPtr(tm) if matches!(tm.ty.kind, Dynamic(..)) => // raw ptr to dyn Trait
Some((format!("The vtable of a wide raw pointer must be non-null"), None)),
// Primitive types with other constraints.
Bool if init == InitKind::Uninit =>
Some((format!("Booleans must be `true` or `false`"), None)),
Expand Down
1 change: 1 addition & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#![feature(box_patterns)]
#![feature(box_syntax)]
#![feature(nll)]
#![feature(matches_macro)]

#![recursion_limit="256"]

Expand Down
4 changes: 4 additions & 0 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ symbols! {
associated_type_bounds,
associated_type_defaults,
associated_types,
assume_init,
async_await,
async_closure,
attr,
Expand Down Expand Up @@ -417,6 +418,8 @@ symbols! {
match_beginning_vert,
match_default_bindings,
may_dangle,
maybe_uninit,
MaybeUninit,
mem,
member_constraints,
message,
Expand Down Expand Up @@ -709,6 +712,7 @@ symbols! {
underscore_imports,
underscore_lifetimes,
uniform_paths,
uninit,
uninitialized,
universal_impl_trait,
unmarked_api,
Expand Down
9 changes: 9 additions & 0 deletions src/test/ui/lint/uninitialized-zeroed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ fn main() {
let _val: NonNull<i32> = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: NonNull<i32> = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: *const dyn Send = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: *const dyn Send = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

// Things that can be zero, but not uninit.
let _val: bool = mem::zeroed();
let _val: bool = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized
Expand All @@ -82,10 +85,16 @@ fn main() {
let _val: &'static [i32] = mem::transmute((0usize, 0usize)); //~ ERROR: does not permit zero-initialization
let _val: NonZeroU32 = mem::transmute(0); //~ ERROR: does not permit zero-initialization

// `MaybeUninit` cases
let _val: NonNull<i32> = MaybeUninit::zeroed().assume_init(); //~ ERROR: does not permit zero-initialization
let _val: NonNull<i32> = MaybeUninit::uninit().assume_init(); //~ ERROR: does not permit being left uninitialized
let _val: bool = MaybeUninit::uninit().assume_init(); //~ ERROR: does not permit being left uninitialized

// Some more types that should work just fine.
let _val: Option<&'static i32> = mem::zeroed();
let _val: Option<fn()> = mem::zeroed();
let _val: MaybeUninit<&'static i32> = mem::zeroed();
let _val: i32 = mem::zeroed();
let _val: bool = MaybeUninit::zeroed().assume_init();
}
}
69 changes: 62 additions & 7 deletions src/test/ui/lint/uninitialized-zeroed.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,30 @@ LL | let _val: NonNull<i32> = mem::uninitialized();
|
= note: std::ptr::NonNull<i32> must be non-null

error: the type `*const dyn std::marker::Send` does not permit zero-initialization
--> $DIR/uninitialized-zeroed.rs:70:37
|
LL | let _val: *const dyn Send = mem::zeroed();
| ^^^^^^^^^^^^^
| |
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead
|
= note: The vtable of a wide raw pointer must be non-null

error: the type `*const dyn std::marker::Send` does not permit being left uninitialized
--> $DIR/uninitialized-zeroed.rs:71:37
|
LL | let _val: *const dyn Send = mem::uninitialized();
| ^^^^^^^^^^^^^^^^^^^^
| |
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead
|
= note: The vtable of a wide raw pointer must be non-null

error: the type `bool` does not permit being left uninitialized
--> $DIR/uninitialized-zeroed.rs:72:26
--> $DIR/uninitialized-zeroed.rs:75:26
|
LL | let _val: bool = mem::uninitialized();
| ^^^^^^^^^^^^^^^^^^^^
Expand All @@ -319,7 +341,7 @@ LL | let _val: bool = mem::uninitialized();
= note: Booleans must be `true` or `false`

error: the type `Wrap<char>` does not permit being left uninitialized
--> $DIR/uninitialized-zeroed.rs:75:32
--> $DIR/uninitialized-zeroed.rs:78:32
|
LL | let _val: Wrap<char> = mem::uninitialized();
| ^^^^^^^^^^^^^^^^^^^^
Expand All @@ -334,7 +356,7 @@ LL | struct Wrap<T> { wrapped: T }
| ^^^^^^^^^^

error: the type `NonBig` does not permit being left uninitialized
--> $DIR/uninitialized-zeroed.rs:78:28
--> $DIR/uninitialized-zeroed.rs:81:28
|
LL | let _val: NonBig = mem::uninitialized();
| ^^^^^^^^^^^^^^^^^^^^
Expand All @@ -345,7 +367,7 @@ LL | let _val: NonBig = mem::uninitialized();
= note: NonBig must be initialized inside its custom valid range

error: the type `&'static i32` does not permit zero-initialization
--> $DIR/uninitialized-zeroed.rs:81:34
--> $DIR/uninitialized-zeroed.rs:84:34
|
LL | let _val: &'static i32 = mem::transmute(0usize);
| ^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -356,7 +378,7 @@ LL | let _val: &'static i32 = mem::transmute(0usize);
= note: References must be non-null

error: the type `&'static [i32]` does not permit zero-initialization
--> $DIR/uninitialized-zeroed.rs:82:36
--> $DIR/uninitialized-zeroed.rs:85:36
|
LL | let _val: &'static [i32] = mem::transmute((0usize, 0usize));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -367,7 +389,7 @@ LL | let _val: &'static [i32] = mem::transmute((0usize, 0usize));
= note: References must be non-null

error: the type `std::num::NonZeroU32` does not permit zero-initialization
--> $DIR/uninitialized-zeroed.rs:83:32
--> $DIR/uninitialized-zeroed.rs:86:32
|
LL | let _val: NonZeroU32 = mem::transmute(0);
| ^^^^^^^^^^^^^^^^^
Expand All @@ -377,5 +399,38 @@ LL | let _val: NonZeroU32 = mem::transmute(0);
|
= note: std::num::NonZeroU32 must be non-null

error: aborting due to 30 previous errors
error: the type `std::ptr::NonNull<i32>` does not permit zero-initialization
--> $DIR/uninitialized-zeroed.rs:89:34
|
LL | let _val: NonNull<i32> = MaybeUninit::zeroed().assume_init();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead
|
= note: std::ptr::NonNull<i32> must be non-null

error: the type `std::ptr::NonNull<i32>` does not permit being left uninitialized
--> $DIR/uninitialized-zeroed.rs:90:34
|
LL | let _val: NonNull<i32> = MaybeUninit::uninit().assume_init();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead
|
= note: std::ptr::NonNull<i32> must be non-null

error: the type `bool` does not permit being left uninitialized
--> $DIR/uninitialized-zeroed.rs:91:26
|
LL | let _val: bool = MaybeUninit::uninit().assume_init();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead
|
= note: Booleans must be `true` or `false`

error: aborting due to 35 previous errors