From 19ddb6922c6bd270b092d8be1ea64ce3d7a5b485 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sun, 15 Dec 2024 23:21:47 +0100 Subject: [PATCH] Handle more cases in `is_normalizable` By assuming that a recursive type is normalizable within the deeper calls to `is_normalizable_helper()`, more cases can be handled by this function. In order to fix stack overflows, a recursion limit has also been added for recursive generic type instantiations. --- clippy_utils/src/ty/mod.rs | 16 ++++-- tests/ui/crashes/ice-10508a.rs | 19 ++++++++ tests/ui/crashes/ice-10508b.rs | 24 +++++++++ tests/ui/crashes/ice-10508c.rs | 7 +++ tests/ui/large_enum_variant.32bit.stderr | 22 +++++++-- tests/ui/large_enum_variant.64bit.stderr | 54 +++++++++++++++++++-- tests/ui/large_enum_variant.rs | 62 ++++++++++++++++++++++++ 7 files changed, 193 insertions(+), 11 deletions(-) create mode 100644 tests/ui/crashes/ice-10508a.rs create mode 100644 tests/ui/crashes/ice-10508b.rs create mode 100644 tests/ui/crashes/ice-10508c.rs diff --git a/clippy_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index 260d1b801e3d9..45dff91e87733 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -351,20 +351,26 @@ pub fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { /// Checks if `Ty` is normalizable. This function is useful /// to avoid crashes on `layout_of`. pub fn is_normalizable<'tcx>(cx: &LateContext<'tcx>, param_env: ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool { - is_normalizable_helper(cx, param_env, ty, &mut FxHashMap::default()) + is_normalizable_helper(cx, param_env, ty, 0, &mut FxHashMap::default()) } fn is_normalizable_helper<'tcx>( cx: &LateContext<'tcx>, param_env: ParamEnv<'tcx>, ty: Ty<'tcx>, + depth: usize, cache: &mut FxHashMap, bool>, ) -> bool { if let Some(&cached_result) = cache.get(&ty) { return cached_result; } - // prevent recursive loops, false-negative is better than endless loop leading to stack overflow - cache.insert(ty, false); + if !cx.tcx.recursion_limit().value_within_limit(depth) { + return false; + } + // Prevent recursive loops by answering `true` to recursive requests with the same + // type. This will be adjusted when the outermost call analyzes all the type + // components. + cache.insert(ty, true); let infcx = cx.tcx.infer_ctxt().build(cx.typing_mode()); let cause = ObligationCause::dummy(); let result = if infcx.at(&cause, param_env).query_normalize(ty).is_ok() { @@ -373,11 +379,11 @@ fn is_normalizable_helper<'tcx>( variant .fields .iter() - .all(|field| is_normalizable_helper(cx, param_env, field.ty(cx.tcx, args), cache)) + .all(|field| is_normalizable_helper(cx, param_env, field.ty(cx.tcx, args), depth + 1, cache)) }), _ => ty.walk().all(|generic_arg| match generic_arg.unpack() { GenericArgKind::Type(inner_ty) if inner_ty != ty => { - is_normalizable_helper(cx, param_env, inner_ty, cache) + is_normalizable_helper(cx, param_env, inner_ty, depth + 1, cache) }, _ => true, // if inner_ty == ty, we've already checked it }), diff --git a/tests/ui/crashes/ice-10508a.rs b/tests/ui/crashes/ice-10508a.rs new file mode 100644 index 0000000000000..f45057217b436 --- /dev/null +++ b/tests/ui/crashes/ice-10508a.rs @@ -0,0 +1,19 @@ +// Used to overflow in `is_normalizable` + +use std::marker::PhantomData; + +struct Node { + m: PhantomData<&'static T>, +} + +struct Digit { + elem: T, +} + +enum FingerTree { + Single(T), + + Deep(Digit, Box>>), +} + +fn main() {} diff --git a/tests/ui/crashes/ice-10508b.rs b/tests/ui/crashes/ice-10508b.rs new file mode 100644 index 0000000000000..41d4f0234b948 --- /dev/null +++ b/tests/ui/crashes/ice-10508b.rs @@ -0,0 +1,24 @@ +use std::marker::PhantomData; + +struct Digit { + elem: T, +} + +struct Node { + m: PhantomData<&'static T>, +} + +enum FingerTree { + Single(T), + + Deep(Digit, Node>>), +} + +enum Wrapper { + Simple, + Other(FingerTree), +} + +fn main() { + let w = Some(Wrapper::Simple::); +} diff --git a/tests/ui/crashes/ice-10508c.rs b/tests/ui/crashes/ice-10508c.rs new file mode 100644 index 0000000000000..fb84d85fd675a --- /dev/null +++ b/tests/ui/crashes/ice-10508c.rs @@ -0,0 +1,7 @@ +#[derive(Debug)] +struct S { + t: T, + s: Box>, +} + +fn main() {} diff --git a/tests/ui/large_enum_variant.32bit.stderr b/tests/ui/large_enum_variant.32bit.stderr index 7edff0df10f82..36f3d930b5351 100644 --- a/tests/ui/large_enum_variant.32bit.stderr +++ b/tests/ui/large_enum_variant.32bit.stderr @@ -283,14 +283,30 @@ LL | / enum WithRecursion { LL | | Large([u64; 64]), | | ---------------- the largest variant contains at least 512 bytes LL | | Recursive(Box), - | | ----------------------------- the second-largest variant contains at least 0 bytes + | | ----------------------------- the second-largest variant contains at least 4 bytes LL | | } - | |_^ the entire enum is at least 0 bytes + | |_^ the entire enum is at least 516 bytes | help: consider boxing the large fields to reduce the total size of the enum | LL | Large(Box<[u64; 64]>), | ~~~~~~~~~~~~~~ -error: aborting due to 17 previous errors +error: large size difference between variants + --> tests/ui/large_enum_variant.rs:168:1 + | +LL | / enum LargeEnumWithGenericsAndRecursive { +LL | | Ok(), + | | ---- the second-largest variant carries no data at all +LL | | Error(WithRecursionAndGenerics), + | | ------------------------------------ the largest variant contains at least 516 bytes +LL | | } + | |_^ the entire enum is at least 516 bytes + | +help: consider boxing the large fields to reduce the total size of the enum + | +LL | Error(Box>), + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 18 previous errors diff --git a/tests/ui/large_enum_variant.64bit.stderr b/tests/ui/large_enum_variant.64bit.stderr index a52967d962f71..31576a5863fcc 100644 --- a/tests/ui/large_enum_variant.64bit.stderr +++ b/tests/ui/large_enum_variant.64bit.stderr @@ -283,14 +283,62 @@ LL | / enum WithRecursion { LL | | Large([u64; 64]), | | ---------------- the largest variant contains at least 512 bytes LL | | Recursive(Box), - | | ----------------------------- the second-largest variant contains at least 0 bytes + | | ----------------------------- the second-largest variant contains at least 8 bytes LL | | } - | |_^ the entire enum is at least 0 bytes + | |_^ the entire enum is at least 520 bytes | help: consider boxing the large fields to reduce the total size of the enum | LL | Large(Box<[u64; 64]>), | ~~~~~~~~~~~~~~ -error: aborting due to 17 previous errors +error: large size difference between variants + --> tests/ui/large_enum_variant.rs:168:1 + | +LL | / enum LargeEnumWithGenericsAndRecursive { +LL | | Ok(), + | | ---- the second-largest variant carries no data at all +LL | | Error(WithRecursionAndGenerics), + | | ------------------------------------ the largest variant contains at least 520 bytes +LL | | } + | |_^ the entire enum is at least 520 bytes + | +help: consider boxing the large fields to reduce the total size of the enum + | +LL | Error(Box>), + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: large size difference between variants + --> tests/ui/large_enum_variant.rs:203:5 + | +LL | / enum NoWarnings { +LL | | BigBoi(PublishWithBytes), + | | ------------------------ the largest variant contains at least 296 bytes +LL | | _SmallBoi(u8), + | | ------------- the second-largest variant contains at least 1 bytes +LL | | } + | |_____^ the entire enum is at least 296 bytes + | +help: consider boxing the large fields to reduce the total size of the enum + | +LL | BigBoi(Box), + | ~~~~~~~~~~~~~~~~~~~~~ + +error: large size difference between variants + --> tests/ui/large_enum_variant.rs:208:5 + | +LL | / enum MakesClippyAngry { +LL | | BigBoi(PublishWithVec), + | | ---------------------- the largest variant contains at least 224 bytes +LL | | _SmallBoi(u8), + | | ------------- the second-largest variant contains at least 1 bytes +LL | | } + | |_____^ the entire enum is at least 224 bytes + | +help: consider boxing the large fields to reduce the total size of the enum + | +LL | BigBoi(Box), + | ~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 20 previous errors diff --git a/tests/ui/large_enum_variant.rs b/tests/ui/large_enum_variant.rs index fec4a1ee79f23..57722f63b225b 100644 --- a/tests/ui/large_enum_variant.rs +++ b/tests/ui/large_enum_variant.rs @@ -178,3 +178,65 @@ fn main() { } ); } + +mod issue11915 { + use std::sync::atomic::AtomicPtr; + + pub struct Bytes { + ptr: *const u8, + len: usize, + // inlined "trait object" + data: AtomicPtr<()>, + vtable: &'static Vtable, + } + pub(crate) struct Vtable { + /// fn(data, ptr, len) + pub clone: unsafe fn(&AtomicPtr<()>, *const u8, usize) -> Bytes, + /// fn(data, ptr, len) + /// + /// takes `Bytes` to value + pub to_vec: unsafe fn(&AtomicPtr<()>, *const u8, usize) -> Vec, + /// fn(data, ptr, len) + pub drop: unsafe fn(&mut AtomicPtr<()>, *const u8, usize), + } + + enum NoWarnings { + BigBoi(PublishWithBytes), + _SmallBoi(u8), + } + + enum MakesClippyAngry { + BigBoi(PublishWithVec), + _SmallBoi(u8), + } + + struct PublishWithBytes { + _dup: bool, + _retain: bool, + _topic: Bytes, + __topic: Bytes, + ___topic: Bytes, + ____topic: Bytes, + _pkid: u16, + _payload: Bytes, + __payload: Bytes, + ___payload: Bytes, + ____payload: Bytes, + _____payload: Bytes, + } + + struct PublishWithVec { + _dup: bool, + _retain: bool, + _topic: Vec, + __topic: Vec, + ___topic: Vec, + ____topic: Vec, + _pkid: u16, + _payload: Vec, + __payload: Vec, + ___payload: Vec, + ____payload: Vec, + _____payload: Vec, + } +}