From b27cbda32b67b4fdd9113ed894b810fc8f3e180d Mon Sep 17 00:00:00 2001 From: Andrea Nall Date: Sun, 7 Mar 2021 21:45:41 -0600 Subject: [PATCH 1/4] make is_normalizable more strict --- clippy_utils/src/lib.rs | 42 +++++++++++++++++++++++++++++++++--- tests/ui/crashes/ice-6840.rs | 23 ++++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 tests/ui/crashes/ice-6840.rs diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 3845667802d8..999b39852cd5 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -85,6 +85,7 @@ use rustc_trait_selection::traits::query::normalize::AtExt; use smallvec::SmallVec; use crate::consts::{constant, Constant}; +use std::collections::HashMap; pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option) -> Option { if let Ok(version) = RustcVersion::parse(msrv) { @@ -1488,10 +1489,45 @@ pub fn match_function_call<'tcx>( /// 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: ty::ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool { - cx.tcx.infer_ctxt().enter(|infcx| { + is_normalizable_helper(cx, param_env, ty, &mut HashMap::new()) +} + +fn is_normalizable_helper<'tcx>( + cx: &LateContext<'tcx>, + param_env: ty::ParamEnv<'tcx>, + ty: Ty<'tcx>, + cache: &mut HashMap, bool>, +) -> bool { + if let Some(&cached_result) = cache.get(ty) { + return cached_result; + } + cache.insert(ty, false); // prevent recursive loops + let result = cx.tcx.infer_ctxt().enter(|infcx| { let cause = rustc_middle::traits::ObligationCause::dummy(); - infcx.at(&cause, param_env).normalize(ty).is_ok() - }) + if infcx.at(&cause, param_env).normalize(ty).is_err() { + false + } else { + match ty.kind() { + ty::Adt(def, substs) => !def.variants.iter().any(|variant| { + variant + .fields + .iter() + .any(|field| !is_normalizable_helper(cx, param_env, field.ty(cx.tcx, substs), cache)) + }), + ty::Ref(_, pointee, _) | ty::RawPtr(ty::TypeAndMut { ty: pointee, .. }) => { + is_normalizable_helper(cx, param_env, pointee, cache) + }, + ty::Array(inner_ty, _) | ty::Slice(inner_ty) => is_normalizable_helper(cx, param_env, inner_ty, cache), + ty::Tuple(tys) => !tys.iter().any(|inner| match inner.unpack() { + GenericArgKind::Type(inner_ty) => !is_normalizable_helper(cx, param_env, inner_ty, cache), + _ => false, + }), + _ => true, + } + } + }); + cache.insert(ty, result); + result } pub fn match_def_path<'tcx>(cx: &LateContext<'tcx>, did: DefId, syms: &[&str]) -> bool { diff --git a/tests/ui/crashes/ice-6840.rs b/tests/ui/crashes/ice-6840.rs new file mode 100644 index 000000000000..a749eefb6355 --- /dev/null +++ b/tests/ui/crashes/ice-6840.rs @@ -0,0 +1,23 @@ +//! This is a reproducer for the ICE 6840: https://github.com/rust-lang/rust-clippy/issues/6840. +//! The ICE is caused by `TyCtxt::layout_of` and `is_normalizable` not being strict enough +#![allow(dead_code)] +use std::collections::HashMap; + +pub trait Rule { + type DependencyKey; +} + +pub struct RuleEdges { + dependencies: R::DependencyKey, +} + +type RuleDependencyEdges = HashMap>; + +// and additional potential variants +type RuleDependencyEdgesArray = HashMap; 8]>; +type RuleDependencyEdgesSlice = HashMap]>; +type RuleDependencyEdgesRef = HashMap>; +type RuleDependencyEdgesRaw = HashMap>; +type RuleDependencyEdgesTuple = HashMap, RuleEdges)>; + +fn main() {} From e322c773e3db22aadd60ce9a1803076fcbfeef2d Mon Sep 17 00:00:00 2001 From: Andrea Nall Date: Mon, 8 Mar 2021 23:03:45 -0600 Subject: [PATCH 2/4] use TyS::walk --- clippy_utils/src/lib.rs | 19 ++++++++----------- tests/ui/crashes/ice-6840.rs | 10 +++++++++- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 999b39852cd5..ac29a09ae0ee 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1504,9 +1504,7 @@ fn is_normalizable_helper<'tcx>( cache.insert(ty, false); // prevent recursive loops let result = cx.tcx.infer_ctxt().enter(|infcx| { let cause = rustc_middle::traits::ObligationCause::dummy(); - if infcx.at(&cause, param_env).normalize(ty).is_err() { - false - } else { + if infcx.at(&cause, param_env).normalize(ty).is_ok() { match ty.kind() { ty::Adt(def, substs) => !def.variants.iter().any(|variant| { variant @@ -1514,16 +1512,15 @@ fn is_normalizable_helper<'tcx>( .iter() .any(|field| !is_normalizable_helper(cx, param_env, field.ty(cx.tcx, substs), cache)) }), - ty::Ref(_, pointee, _) | ty::RawPtr(ty::TypeAndMut { ty: pointee, .. }) => { - is_normalizable_helper(cx, param_env, pointee, cache) - }, - ty::Array(inner_ty, _) | ty::Slice(inner_ty) => is_normalizable_helper(cx, param_env, inner_ty, cache), - ty::Tuple(tys) => !tys.iter().any(|inner| match inner.unpack() { - GenericArgKind::Type(inner_ty) => !is_normalizable_helper(cx, param_env, inner_ty, cache), - _ => false, + _ => !ty.walk().any(|generic_arg| !match generic_arg.unpack() { + GenericArgKind::Type(inner_ty) if inner_ty != ty => { + is_normalizable_helper(cx, param_env, inner_ty, cache) + }, + _ => true, // if inner_ty == ty, we've already checked it }), - _ => true, } + } else { + false } }); cache.insert(ty, result); diff --git a/tests/ui/crashes/ice-6840.rs b/tests/ui/crashes/ice-6840.rs index a749eefb6355..d789f60c5d5a 100644 --- a/tests/ui/crashes/ice-6840.rs +++ b/tests/ui/crashes/ice-6840.rs @@ -13,11 +13,19 @@ pub struct RuleEdges { type RuleDependencyEdges = HashMap>; -// and additional potential variants +// reproducer from the GitHub issue ends here +// but check some additional variants type RuleDependencyEdgesArray = HashMap; 8]>; type RuleDependencyEdgesSlice = HashMap]>; type RuleDependencyEdgesRef = HashMap>; type RuleDependencyEdgesRaw = HashMap>; type RuleDependencyEdgesTuple = HashMap, RuleEdges)>; +// and an additional checks to make sure fix doesn't have stack-overflow issue +// on self-containing types +pub struct SelfContaining { + inner: Box, +} +type SelfContainingEdges = HashMap; + fn main() {} From e812a8abdebd6e4d03aad84b4f7d671a0cb49fdf Mon Sep 17 00:00:00 2001 From: Andrea Nall Date: Mon, 8 Mar 2021 23:08:52 -0600 Subject: [PATCH 3/4] use `.all` instead of negative use of `.any` --- clippy_utils/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index ac29a09ae0ee..c0722f37f494 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1506,13 +1506,13 @@ fn is_normalizable_helper<'tcx>( let cause = rustc_middle::traits::ObligationCause::dummy(); if infcx.at(&cause, param_env).normalize(ty).is_ok() { match ty.kind() { - ty::Adt(def, substs) => !def.variants.iter().any(|variant| { + ty::Adt(def, substs) => def.variants.iter().all(|variant| { variant .fields .iter() - .any(|field| !is_normalizable_helper(cx, param_env, field.ty(cx.tcx, substs), cache)) + .all(|field| is_normalizable_helper(cx, param_env, field.ty(cx.tcx, substs), cache)) }), - _ => !ty.walk().any(|generic_arg| !match generic_arg.unpack() { + _ => 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) }, From 9707599714c755c16e0e90b1c91420341704e5fe Mon Sep 17 00:00:00 2001 From: Andrea Nall Date: Tue, 9 Mar 2021 08:30:33 -0600 Subject: [PATCH 4/4] add comment for when can be removed --- clippy_utils/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index c0722f37f494..44eb37387e76 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1486,6 +1486,9 @@ pub fn match_function_call<'tcx>( None } +// FIXME: Per https://doc.rust-lang.org/nightly/nightly-rustc/rustc_trait_selection/infer/at/struct.At.html#method.normalize +// this function can be removed once the `normalizie` method does not panic when normalization does +// not succeed /// 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: ty::ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool { @@ -1501,7 +1504,8 @@ fn is_normalizable_helper<'tcx>( if let Some(&cached_result) = cache.get(ty) { return cached_result; } - cache.insert(ty, false); // prevent recursive loops + // prevent recursive loops, false-negative is better than endless loop leading to stack overflow + cache.insert(ty, false); let result = cx.tcx.infer_ctxt().enter(|infcx| { let cause = rustc_middle::traits::ObligationCause::dummy(); if infcx.at(&cause, param_env).normalize(ty).is_ok() {