Skip to content

Improve uninit/zeroed lint #66044

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 3 commits into from
Nov 7, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
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
25 changes: 23 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,22 @@ 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 path, _, ref args) = expr.kind {
// Find problematic calls to `MaybeUninit::assume_init`.
if path.ident.name == sym::assume_init {
Copy link
Member Author

Choose a reason for hiding this comment

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

This one seems odd to me. I am comparing just the name of the method -- but what if there is a trait or another type or so that also had this method? This looks like I am working pre-resolution, which seems fragile. Isn't there any way to get the full path to the method that will actually be called here?

I looked at clippy but couldn't find anything less fragile there either. Cc @oli-obk @Manishearth

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use diagnostics items instead? #60966

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I have an Option<HirId>; I guess I could unwrap that and then call owner_def_id to get the DefId that is_diagnostic_item needs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, that does not work. The lint just never fires. Looks like the HirId I get there is not pointing to the method being called -- that kind of makes sense if the information we have here is pre-method-resolution. (Probably it's the HirId of the method call itself.)

For Call, I can call

cx.tables.qpath_res(qpath, path_expr.hir_id).opt_def_id()?

to get the def_id of the callee. Isn't there something similar for MethodCall?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, maybe I need to construct a QPath::TypeRelative? I have a PathSegment, but I'd still also need a Ty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I found a way:

cx.tables.type_dependent_def_id(expr.hir_id)?

Here, expr is the MethodCall itself.

// 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 +1968,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