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

make invalid_value lint a bit smarter around enums #102281

Merged
merged 2 commits into from
Sep 28, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
160 changes: 109 additions & 51 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::layout::{LayoutError, LayoutOf};
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::Instance;
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_middle::ty::{self, Instance, Ty, TyCtxt, VariantDef};
use rustc_session::lint::{BuiltinLintDiagnostics, FutureIncompatibilityReason};
use rustc_span::edition::Edition;
use rustc_span::source_map::Spanned;
Expand Down Expand Up @@ -2425,12 +2424,56 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
None
}

/// Test if this enum has several actually "existing" variants.
/// Zero-sized uninhabited variants do not always have a tag assigned and thus do not "exist".
fn is_multi_variant<'tcx>(adt: ty::AdtDef<'tcx>) -> bool {
// As an approximation, we only count dataless variants. Those are definitely inhabited.
let existing_variants = adt.variants().iter().filter(|v| v.fields.is_empty()).count();
existing_variants > 1
/// Determines whether the given type is inhabited. `None` means that we don't know.
fn ty_inhabited(ty: Ty<'_>) -> Option<bool> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use tcx.type_uninhabited_from?

Copy link
Member Author

@RalfJung RalfJung Sep 26, 2022

Choose a reason for hiding this comment

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

Good question. Unfortunately the documentation is not helpful:

Computes the set of modules from which this type is visibly uninhabited. To check whether a type is uninhabited at all (not just from a given module), you could check whether the forest is empty.

Now I still don't know if this is an underapproximation or an overapproximation -- is this "definitely uninhabited" or "maybe uninhabited"? No idea which forest it is talking about.^^ 🌲

Copy link
Contributor

Choose a reason for hiding this comment

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

type_uninhabited_from means definitely uninhabited. References and arrays of unknown length are considered inhabited by this query.

The "forest" is the query's return type, I agree this is esoteric. tcx.type_uninhabited_from(param_env.and(ty)).is_empty() means that ty is possibly inhabited (opposite of definitely uninhabited).

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like I can use is_ty_uninhabited_from as a nice wrapper.

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 no then I need a module to check 'from'...

use rustc_type_ir::sty::TyKind::*;
match ty.kind() {
Never => Some(false),
Int(_) | Uint(_) | Float(_) | Bool | Char | RawPtr(_) => Some(true),
// Fallback for more complicated types. (Note that `&!` might be considered
// uninhabited so references are "complicated", too.)
_ => None,
}
}
/// Determines whether a product type formed from a list of types is inhabited.
fn tys_inhabited<'tcx>(tys: impl Iterator<Item = Ty<'tcx>>) -> Option<bool> {
let mut definitely_inhabited = true; // with no fields, we are definitely inhabited.
for ty in tys {
match ty_inhabited(ty) {
// If any type is uninhabited, the product is uninhabited.
Some(false) => return Some(false),
// Otherwise go searching for a `None`.
None => {
// We don't know.
definitely_inhabited = false;
}
Some(true) => {}
}
}
if definitely_inhabited { Some(true) } else { None }
}

fn variant_find_init_error<'tcx>(
cx: &LateContext<'tcx>,
variant: &VariantDef,
substs: ty::SubstsRef<'tcx>,
descr: &str,
init: InitKind,
) -> Option<InitError> {
variant.fields.iter().find_map(|field| {
ty_find_init_error(cx, field.ty(cx.tcx, substs), init).map(|(mut msg, span)| {
cjgillot marked this conversation as resolved.
Show resolved Hide resolved
if span.is_none() {
// Point to this field, should be helpful for figuring
// out where the source of the error is.
let span = cx.tcx.def_span(field.did);
write!(&mut msg, " (in this {descr})").unwrap();
(msg, Some(span))
} else {
// Just forward.
(msg, span)
}
})
})
}

/// Return `Some` only if we are sure this type does *not*
Expand Down Expand Up @@ -2468,14 +2511,15 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
RawPtr(_) if init == InitKind::Uninit => {
Some(("raw pointers must not be uninitialized".to_string(), None))
}
// Recurse and checks for some compound types.
// Recurse and checks for some compound types. (but not unions)
Adt(adt_def, substs) if !adt_def.is_union() => {
// First check if this ADT has a layout attribute (like `NonNull` and friends).
use std::ops::Bound;
match cx.tcx.layout_scalar_valid_range(adt_def.did()) {
// We exploit here that `layout_scalar_valid_range` will never
// return `Bound::Excluded`. (And we have tests checking that we
// handle the attribute correctly.)
// We don't add a span since users cannot declare such types anyway.
(Bound::Included(lo), _) if lo > 0 => {
return Some((format!("`{}` must be non-null", ty), None));
}
Expand All @@ -2492,50 +2536,64 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
}
_ => {}
}
// Now, recurse.
match adt_def.variants().len() {
0 => Some(("enums with no variants have no valid value".to_string(), None)),
1 => {
// Struct, or enum with exactly one variant.
// Proceed recursively, check all fields.
let variant = &adt_def.variant(VariantIdx::from_u32(0));
variant.fields.iter().find_map(|field| {
ty_find_init_error(cx, field.ty(cx.tcx, substs), init).map(
|(mut msg, span)| {
if span.is_none() {
// Point to this field, should be helpful for figuring
// out where the source of the error is.
let span = cx.tcx.def_span(field.did);
write!(
&mut msg,
" (in this {} field)",
adt_def.descr()
)
.unwrap();
(msg, Some(span))
} else {
// Just forward.
(msg, span)
}
},
)
})
}
// Multi-variant enum.
_ => {
if init == InitKind::Uninit && is_multi_variant(*adt_def) {
let span = cx.tcx.def_span(adt_def.did());
Some((
"enums have to be initialized to a variant".to_string(),
Some(span),
))
} else {
// In principle, for zero-initialization we could figure out which variant corresponds
// to tag 0, and check that... but for now we just accept all zero-initializations.
None
}
// Handle structs.
if adt_def.is_struct() {
return variant_find_init_error(
cx,
adt_def.non_enum_variant(),
substs,
"struct field",
init,
);
}
// And now, enums.
let span = cx.tcx.def_span(adt_def.did());
let mut potential_variants = adt_def.variants().iter().filter_map(|variant| {
let inhabited = tys_inhabited(
variant.fields.iter().map(|field| field.ty(cx.tcx, substs)),
);
let definitely_inhabited = match inhabited {
// Entirely skip uninhbaited variants.
Some(false) => return None,
cjgillot marked this conversation as resolved.
Show resolved Hide resolved
// Forward the others, but remember which ones are definitely inhabited.
Some(true) => true,
None => false,
};
Some((variant, definitely_inhabited))
});
let Some(first_variant) = potential_variants.next() else {
return Some(("enums with no inhabited variants have no valid value".to_string(), Some(span)));
};
// So we have at least one potentially inhabited variant. Might we have two?
let Some(second_variant) = potential_variants.next() else {
// There is only one potentially inhabited variant. So we can recursively check that variant!
return variant_find_init_error(
cx,
&first_variant.0,
substs,
"field of the only potentially inhabited enum variant",
init,
);
};
// So we have at least two potentially inhabited variants.
// If we can prove that we have at least two *definitely* inhabited variants,
// then we have a tag and hence leaving this uninit is definitely disallowed.
// (Leaving it zeroed could be okay, depending on which variant is encoded as zero tag.)
if init == InitKind::Uninit {
let definitely_inhabited = (first_variant.1 as usize)
+ (second_variant.1 as usize)
+ potential_variants
.filter(|(_variant, definitely_inhabited)| *definitely_inhabited)
.count();
cjgillot marked this conversation as resolved.
Show resolved Hide resolved
if definitely_inhabited > 1 {
return Some((
"enums with multiple inhabited variants have to be initialized to a variant".to_string(),
Some(span),
));
}
}
// We couldn't find anything wrong here.
None
}
Tuple(..) => {
// Proceed recursively, check all fields.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ LL | const BAR: [empty::Empty; 3] = [unsafe { std::mem::transmute(()) }; 3];
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
|
note: enums with no variants have no valid value (in this struct field)
--> $DIR/validate_uninhabited_zsts.rs:16:22
note: enums with no inhabited variants have no valid value
--> $DIR/validate_uninhabited_zsts.rs:13:5
|
LL | pub struct Empty(Void);
| ^^^^
LL | enum Void {}
| ^^^^^^^^^

error: aborting due to 2 previous errors; 2 warnings emitted

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ LL | const BAR: [empty::Empty; 3] = [unsafe { std::mem::transmute(()) }; 3];
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
|
note: enums with no variants have no valid value (in this struct field)
--> $DIR/validate_uninhabited_zsts.rs:16:22
note: enums with no inhabited variants have no valid value
--> $DIR/validate_uninhabited_zsts.rs:13:5
|
LL | pub struct Empty(Void);
| ^^^^
LL | enum Void {}
| ^^^^^^^^^

error: aborting due to 2 previous errors; 2 warnings emitted

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ enum OneFruit {
Banana,
}

enum OneFruitNonZero {
Apple(!),
Banana(NonZeroU32),
}

enum TwoUninhabited {
A(!),
B(!),
}

#[allow(unused)]
fn generic<T: 'static>() {
unsafe {
Expand Down Expand Up @@ -84,6 +94,12 @@ fn main() {
let _val: [fn(); 2] = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: [fn(); 2] = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

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

let _val: OneFruitNonZero = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: OneFruitNonZero = 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 Down Expand Up @@ -112,6 +128,16 @@ fn main() {
let _val: *const [()] = mem::zeroed();
let _val: *const [()] = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

// Things where 0 is okay due to rustc implementation details,
// but that are not guaranteed to keep working.
let _val: Result<i32, i32> = mem::zeroed();
let _val: Result<i32, i32> = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

// Some things that happen to work due to rustc implementation details,
// but are not guaranteed to keep working.
let _val: OneFruit = mem::zeroed();
let _val: OneFruit = mem::uninitialized();

// Transmute-from-0
let _val: &'static i32 = mem::transmute(0usize); //~ ERROR: does not permit zero-initialization
let _val: &'static [i32] = mem::transmute((0usize, 0usize)); //~ ERROR: does not permit zero-initialization
Expand All @@ -129,9 +155,5 @@ fn main() {
let _val: bool = MaybeUninit::zeroed().assume_init();
let _val: [bool; 0] = MaybeUninit::uninit().assume_init();
let _val: [!; 0] = MaybeUninit::zeroed().assume_init();

// Some things that happen to work due to rustc implementation details,
// but are not guaranteed to keep working.
let _val: OneFruit = mem::uninitialized();
}
}
Loading