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

Remove is_normalizable #12550

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
3 changes: 0 additions & 3 deletions clippy_lints/src/transmute/eager_transmute.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::ty::is_normalizable;
use clippy_utils::{eq_expr_value, path_to_local};
use rustc_abi::WrappingRange;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -84,8 +83,6 @@ pub(super) fn check<'tcx>(
&& path.ident.name == sym!(then_some)
&& is_local_with_projections(transmutable)
&& binops_with_local(cx, transmutable, receiver)
&& is_normalizable(cx, cx.param_env, from_ty)
&& is_normalizable(cx, cx.param_env, to_ty)
// we only want to lint if the target type has a niche that is larger than the one of the source type
// e.g. `u8` to `NonZeroU8` should lint, but `NonZeroU8` to `u8` should not
&& let Ok(from_layout) = cx.tcx.layout_of(cx.param_env.and(from_ty))
Expand Down
15 changes: 4 additions & 11 deletions clippy_lints/src/zero_sized_map_values.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::ty::{is_normalizable, is_type_diagnostic_item};
use clippy_utils::ty::{is_type_diagnostic_item, sizedness_of};
use rustc_hir::{self as hir, HirId, ItemKind, Node};
use rustc_hir_analysis::hir_ty_to_ty;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::layout::LayoutOf as _;
use rustc_middle::ty::{Adt, Ty, TypeVisitableExt};
use rustc_middle::ty::{self, Ty};
use rustc_session::declare_lint_pass;
use rustc_span::sym;

Expand Down Expand Up @@ -49,15 +48,9 @@ impl LateLintPass<'_> for ZeroSizedMapValues {
&& !in_trait_impl(cx, hir_ty.hir_id)
&& let ty = ty_from_hir_ty(cx, hir_ty)
&& (is_type_diagnostic_item(cx, ty, sym::HashMap) || is_type_diagnostic_item(cx, ty, sym::BTreeMap))
&& let Adt(_, args) = ty.kind()
&& let ty::Adt(_, args) = ty.kind()
&& let ty = args.type_at(1)
// Fixes https://github.com/rust-lang/rust-clippy/issues/7447 because of
// https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/src/ty/sty.rs#L968
&& !ty.has_escaping_bound_vars()
// Do this to prevent `layout_of` crashing, being unable to fully normalize `ty`.
&& is_normalizable(cx, cx.param_env, ty)
&& let Ok(layout) = cx.layout_of(ty)
&& layout.is_zst()
&& sizedness_of(cx.tcx, cx.param_env, ty).is_zero()
{
span_lint_and_help(
cx,
Expand Down
162 changes: 112 additions & 50 deletions clippy_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use core::ops::ControlFlow;
use itertools::Itertools;
use rustc_ast::ast::Mutability;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
use rustc_hir::def_id::DefId;
Expand All @@ -16,7 +16,7 @@ use rustc_lint::LateContext;
use rustc_middle::mir::interpret::Scalar;
use rustc_middle::mir::ConstValue;
use rustc_middle::traits::EvaluationResult;
use rustc_middle::ty::layout::ValidityRequirement;
use rustc_middle::ty::layout::{LayoutOf, ValidityRequirement};
use rustc_middle::ty::{
self, AdtDef, AliasTy, AssocKind, Binder, BoundRegion, FnSig, GenericArg, GenericArgKind, GenericArgsRef,
GenericParamDefKind, IntTy, List, ParamEnv, Region, RegionKind, ToPredicate, TraitRef, Ty, TyCtxt,
Expand Down Expand Up @@ -361,50 +361,6 @@ pub fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
}
}

// 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 `normalize` 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: ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool {
is_normalizable_helper(cx, param_env, ty, &mut FxHashMap::default())
}

fn is_normalizable_helper<'tcx>(
cx: &LateContext<'tcx>,
param_env: ParamEnv<'tcx>,
ty: Ty<'tcx>,
cache: &mut FxHashMap<Ty<'tcx>, 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);
let infcx = cx.tcx.infer_ctxt().build();
let cause = ObligationCause::dummy();
let result = if infcx.at(&cause, param_env).query_normalize(ty).is_ok() {
match ty.kind() {
ty::Adt(def, args) => def.variants().iter().all(|variant| {
variant
.fields
.iter()
.all(|field| is_normalizable_helper(cx, param_env, field.ty(cx.tcx, args), 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)
},
_ => true, // if inner_ty == ty, we've already checked it
}),
}
} else {
false
};
cache.insert(ty, result);
result
}

/// Returns `true` if the given type is a non aggregate primitive (a `bool` or `char`, any
/// integer or floating-point number type). For checking aggregation of primitive types (e.g.
/// tuples and slices of primitive type) see `is_recursively_primitive_type`
Expand Down Expand Up @@ -1013,10 +969,6 @@ pub fn adt_and_variant_of_res<'tcx>(cx: &LateContext<'tcx>, res: Res) -> Option<
/// Comes up with an "at least" guesstimate for the type's size, not taking into
/// account the layout of type parameters.
pub fn approx_ty_size<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

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

So if I understand correctly, approx_ty_size shouldn't be called if the Ty might be a type alias because that can have trait projections without having an explicit T: Trait bound and thus would not be in the ParamEnv, which causes layout_of to crash when normalizing it?

Should this be mentioned in the doc comment that it shouldn't be called in those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only when you would want to resolve things in the context of the type alias item. If you're in some other context and just instantiating the alias then it's fine.

Copy link
Member

Choose a reason for hiding this comment

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

Right. Isn't it still worth documenting though? I feel like it's a subtle detail that could easily be missed or people might not even know about it.

use rustc_middle::ty::layout::LayoutOf;
if !is_normalizable(cx, cx.param_env, ty) {
Copy link
Member

Choose a reason for hiding this comment

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

This also fixes #11915, right? iirc that lint uses approx_ty_size indirectly and that failed because of the check here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it.

Copy link
Member

Choose a reason for hiding this comment

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

should this also have a test for that then?

return 0;
}
match (cx.layout_of(ty).map(|layout| layout.size.bytes()), ty.kind()) {
(Ok(size), _) => size,
(Err(_), ty::Tuple(list)) => list.iter().map(|t| approx_ty_size(cx, t)).sum(),
Expand Down Expand Up @@ -1290,3 +1242,113 @@ pub fn normalize_with_regions<'tcx>(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>
pub fn is_manually_drop(ty: Ty<'_>) -> bool {
ty.ty_adt_def().map_or(false, AdtDef::is_manually_drop)
}

#[derive(Clone, Copy)]
pub enum Sizedness {
/// The type is uninhabited. (e.g. `!`)
Uninhabited,
/// The type is zero-sized.
Zero,
/// The type has some other size or an unknown size.
Other,
}
impl Sizedness {
pub fn is_zero(self) -> bool {
matches!(self, Self::Zero)
}

pub fn is_uninhabited(self) -> bool {
matches!(self, Self::Uninhabited)
}
}

/// Calculates the sizedness of a type.
pub fn sizedness_of<'tcx>(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, ty: Ty<'tcx>) -> Sizedness {
fn for_list<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
tys: impl IntoIterator<Item = Ty<'tcx>>,
) -> Sizedness {
let mut res = Sizedness::Zero;
for ty in tys {
match sizedness_of(tcx, param_env, ty) {
Sizedness::Uninhabited => return Sizedness::Uninhabited,
Sizedness::Other => res = Sizedness::Other,
Sizedness::Zero => {},
}
}
res
}

match *ty.kind() {
ty::FnDef(..) => Sizedness::Zero,
ty::Tuple(tys) => for_list(tcx, param_env, tys),
ty::Array(_, len) if len.try_eval_target_usize(tcx, param_env).is_some_and(|x| x == 0) => Sizedness::Zero,
ty::Array(ty, _) => sizedness_of(tcx, param_env, ty),
ty::Adt(adt, args) => {
let mut iter = adt
.variants()
.iter()
.map(|v| for_list(tcx, param_env, v.fields.iter().map(|f| f.ty(tcx, args))))
.filter(|x| !x.is_uninhabited());
match iter.next() {
None => Sizedness::Uninhabited,
Some(Sizedness::Other) => Sizedness::Other,
Some(_) => {
if iter.next().is_some() {
Sizedness::Other
} else {
Sizedness::Zero
}
},
}
},
ty::Closure(_, args) => for_list(tcx, param_env, args.as_closure().upvar_tys().iter()),
ty::Coroutine(_, args) => {
if for_list(tcx, param_env, args.as_coroutine().upvar_tys().iter()).is_uninhabited() {
Sizedness::Uninhabited
} else {
Sizedness::Other
}
},
ty::CoroutineClosure(_, args) => {
if for_list(tcx, param_env, args.as_coroutine_closure().upvar_tys().iter()).is_uninhabited() {
Sizedness::Uninhabited
} else {
Sizedness::Other
}
},
ty::CoroutineWitness(_, args) => for_list(tcx, param_env, args.iter().filter_map(GenericArg::as_type)),
ty::Alias(..) => {
if let Ok(normalized) = tcx
.infer_ctxt()
.build()
.at(&ObligationCause::dummy(), param_env)
.query_normalize(ty)
&& normalized.value != ty
{
sizedness_of(tcx, param_env, normalized.value)
} else {
Sizedness::Other
}
},
ty::Never => Sizedness::Uninhabited,
ty::Bool
| ty::Char
| ty::Int(_)
| ty::Uint(_)
| ty::Float(_)
| ty::RawPtr(..)
| ty::Ref(..)
| ty::FnPtr(..)
| ty::Param(_)
| ty::Bound(..)
| ty::Placeholder(_)
| ty::Infer(_)
| ty::Error(_)
| ty::Dynamic(..)
| ty::Slice(..)
| ty::Str
| ty::Foreign(_) => Sizedness::Other,
}
}
19 changes: 19 additions & 0 deletions tests/ui/crashes/ice-10508.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Used to overflow in `is_normalizable`

use std::marker::PhantomData;

struct Node<T: 'static> {
m: PhantomData<&'static T>,
}

struct Digit<T> {
elem: T,
}

enum FingerTree<T: 'static> {
Single(T),

Deep(Digit<T>, Box<FingerTree<Node<T>>>),
}

fn main() {}