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.

Sign up for GitHub

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

Jump to bottom

Remove is_normalizable #12550

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove is_normalizable #12550

wants to merge 1 commit into from

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 24, 2024 •
edited

fixes #11915
Fixes only the first ICE in #10508

is_normalizable is used in a few places to avoid an ICE due to a delayed bug in normalization which occurs when a projection type is used on a type which doesn't implement the correct trait. The only part of clippy that actually needed this check is zero_sized_map_values due to a quirk of how type aliases work (they don't a real ParamEnv). This fixes the need for the check there by manually walking the type to determine if it's zero sized, rather than attempting to compute the type's layout thereby avoid the normalization that triggers the delayed bug.

For an example of the issue with type aliases:

trait Foo { type Foo; }
struct Bar<T: Foo>(T::Foo);

// The `ParamEnv` here just has `T: Sized`, not `T: Sized + Foo`. 
type Baz<T> = &'static Bar<T>;

When trying to compute the layout of &'static Bar<T> we need to determine if what type <Bar<T> as Pointee>::Metadata is. Doing this requires knowing if T::Foo: Sized, but since T doesn't have an associated type Foo in the context of the type alias a delayed bug is issued.

changelog: [large_enum_variant]: correctly get the size of bytes::Bytes.

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2024

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 24, 2024
y21
y21 reviewed Mar 25, 2024
View reviewed changes
clippy_utils/src/ty.rs
@@ -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 {
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?

clippy_utils/src/ty.rs
@@ -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.

clippy_utils/src/ty.rs Outdated
match *ty.kind() {
ty::FnDef(..) => Sizedness::Zero,
ty::Tuple(tys) => for_list(tcx, param_env, tys),
ty::Array(_, len) if len.eval_target_usize(tcx, param_env) == 0 => Sizedness::Zero,
Copy link
Member

Choose a reason for hiding this comment

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

Won't this ICE if len is a generic parameter?

type M<const N: usize> = HashMap<(), [(); N]>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

clippy_utils/src/ty.rs Outdated
Comment on lines 1307 to 1321
ty::Coroutine(_, args) => for_list(tcx, param_env, args.as_coroutine().upvar_tys().iter()),
ty::CoroutineClosure(_, args) => for_list(tcx, param_env, args.as_coroutine_closure().upvar_tys().iter()),
ty::CoroutineWitness(_, args) => for_list(tcx, param_env, args.iter().filter_map(GenericArg::as_type)),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think coroutines are ZSTs if they don't have upvars. At least futures generated by async blocks are always at least of size 1 to tell that they've finished https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=743d622d9887740234d325ae94dbf5f4

Then again, I have no idea if zero_sized_map_values realistically could ever run into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point. Fixed.

The check for an uninhabited coroutine is still conservative, but that should be fine.

@y21
Copy link
Member

y21 commented Mar 25, 2024

Why does layout_of (supposedly?) ice when normalization fails anyway? LayoutError has a NormalizationFailure variant and it seems to be handling the case where normalization fails? Or am I missing something/is this for something else?

@bors
Copy link
Collaborator

bors commented Apr 18, 2024

☔ The latest upstream changes (presumably #12690) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y21 y21 y21 left review comments

Assignees

@y21 y21

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clippy doesn't generate large_enum_variant warning when using bytes::Bytes
4 participants
@Jarcho @rustbot @y21 @bors