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 `NonZero<u8>` should lint, but `NonZero<u8>` to `u8` should not
&& let Ok(from_layout) = cx.tcx.layout_of(cx.param_env.and(from_ty))
Expand Down
13 changes: 3 additions & 10 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::lower_ty;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::layout::LayoutOf as _;
use rustc_middle::ty::{self, Ty, TypeVisitableExt};
use rustc_middle::ty::{self, Ty};
use rustc_session::declare_lint_pass;
use rustc_span::sym;

Expand Down Expand Up @@ -51,13 +50,7 @@ impl LateLintPass<'_> for ZeroSizedMapValues {
&& (is_type_diagnostic_item(cx, ty, sym::HashMap) || is_type_diagnostic_item(cx, ty, sym::BTreeMap))
&& 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
143 changes: 94 additions & 49 deletions clippy_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,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, AssocItem, AssocKind, Binder, BoundRegion, FnSig, GenericArg, GenericArgKind,
GenericArgsRef, GenericParamDefKind, IntTy, ParamEnv, Region, RegionKind, TraitRef, Ty, TyCtxt, TypeSuperVisitable,
Expand Down Expand Up @@ -353,50 +353,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 @@ -977,11 +933,12 @@ 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.
///
/// This function will ICE if called with an improper `ParamEnv`. This can happen
/// when linting in when item, but the type is retrieved from a different item
/// without instantiating the generic arguments. It can also happen when linting a
/// type alias as those do not have a `ParamEnv`.
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 @@ -1340,3 +1297,91 @@ pub fn get_field_by_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, name: Symbol) ->
_ => None,
}
}

#[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 is_zst<'tcx>(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool {
match *ty.kind() {
ty::FnDef(..) | ty::Never => true,
ty::Tuple(tys) => tys.iter().all(|ty| is_zst(tcx, param_env, ty)),
// Zero length arrays are always zero-sized, even for uninhabited types.
ty::Array(_, len) if len.try_eval_target_usize(tcx, param_env).is_some_and(|x| x == 0) => true,
ty::Array(ty, _) | ty::Pat(ty, _) => is_zst(tcx, param_env, ty),
ty::Adt(adt, args) => {
let mut iter = adt.variants().iter().filter(|v| {
!v.fields
.iter()
.any(|f| f.ty(tcx, args).is_privately_uninhabited(tcx, param_env))
});
let is_zst = iter.next().map_or(true, |v| {
v.fields.iter().all(|f| is_zst(tcx, param_env, f.ty(tcx, args)))
});
is_zst && iter.next().is_none()
},
ty::Closure(_, args) => args
.as_closure()
.upvar_tys()
.iter()
.all(|ty| is_zst(tcx, param_env, ty)),
ty::CoroutineWitness(_, args) => args
.iter()
.filter_map(GenericArg::as_type)
.all(|ty| is_zst(tcx, param_env, ty)),
ty::Alias(..) => {
if let Ok(normalized) = tcx.try_normalize_erasing_regions(param_env, ty)
&& normalized != ty
{
is_zst(tcx, param_env, normalized)
} else {
false
}
},
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(_)
| ty::Coroutine(..)
| ty::CoroutineClosure(..) => false,
}
}

if ty.is_privately_uninhabited(tcx, param_env) {
Sizedness::Uninhabited
} else if is_zst(tcx, param_env, ty) {
Sizedness::Zero
} else {
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() {}
18 changes: 17 additions & 1 deletion tests/ui/large_enum_variant.64bit.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -276,5 +276,21 @@ help: consider boxing the large fields to reduce the total size of the enum
LL | Error(Box<PossiblyLargeEnumWithConst<256>>),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: aborting due to 16 previous errors
error: large size difference between variants
--> tests/ui/large_enum_variant.rs:167:1
|
LL | / enum SelfRef<'a> {
LL | | Small,
| | ----- the second-largest variant carries no data at all
LL | | Large([&'a SelfRef<'a>; 1024]),
| | ------------------------------ the largest variant contains at least 8192 bytes
LL | | }
| |_^ the entire enum is at least 8192 bytes
|
help: consider boxing the large fields to reduce the total size of the enum
|
LL | Large(Box<[&'a SelfRef<'a>; 1024]>),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: aborting due to 17 previous errors

5 changes: 5 additions & 0 deletions tests/ui/large_enum_variant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,8 @@ fn main() {
}
);
}

enum SelfRef<'a> {
Small,
Large([&'a SelfRef<'a>; 1024]),
}