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

Handle more cases in is_normalizable #13833

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Dec 15, 2024

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.

Fix #9798
Fix #10508
Fix #11915

changelog: [large_enum_variant]: more precise detection of variants with large size differences

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2024

r? @llogiq

rustbot has assigned @llogiq.
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 Dec 15, 2024
@samueltardieu samueltardieu marked this pull request as ready for review December 15, 2024 23:14
@llogiq
Copy link
Contributor

llogiq commented Dec 16, 2024

Thank you!

@llogiq llogiq added this pull request to the merge queue Dec 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2024
@llogiq llogiq added this pull request to the merge queue Dec 16, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 16, 2024
Consider raw pointers fields as normalizable.

This is only used to determine the layout, and the layout of a raw
pointer is known, whatever the designated type is. As a side effect,
this makes the layout of `Box<T>` known, even inside `T`, and enables
recursive types.

Would there be a drawback in doing this?

I have included the new tests in a separate commit, so that the effect
of the change can be seen in the second commit. It can also be seen on
the lintcheck diff result.

Close #9798.
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Dec 16, 2024

This is only used to determine the layout, and the layout of a raw pointer is known, whatever the designated type is.

This is wrong. The size of both references and raw pointers depend on whether the pointee is Sized.

@samueltardieu
Copy link
Contributor Author

This is only used to determine the layout, and the layout of a raw pointer is known, whatever the designated type is.
This is wrong. The size of both references and raw pointers depend on whether the pointee is Sized.

This is what I meant by "known" instead of "constant", because I thought that at this stage the sizedness of the pointee type would be known. I will try to double-check this.

@Jarcho
Copy link
Contributor

Jarcho commented Dec 16, 2024

The sizedness of pointee types is the reason layout_of caused ICEs in the first place. For most cases it returns an error, but for pointers and references it sometimes caused a delayed bug to be issued.

@llogiq llogiq added this pull request to the merge queue Dec 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2024
@samueltardieu
Copy link
Contributor Author

@llogiq I haven't checked, but it probably fails because of the missing changelog entry. Anyhow, I don't know if we should merge it right now given @Jarcho's comments. I'll set the PR to draft, even if I couldn't notice crashes in lintchecks for example, I'll have to double-check that this cannot happen.

@samueltardieu samueltardieu marked this pull request as draft December 16, 2024 15:58
@y21
Copy link
Member

y21 commented Dec 17, 2024

For reference (maybe this helps), a concrete repro that starts ICE-ing with this change (taken from #12550 which also intends to fix the same issue)

#![warn(clippy::zero_sized_map_values)]

trait Foo { type Foo; }

struct Bar<T: Foo>(T::Foo);

type Bad<T> = std::collections::HashMap<u32, *const Bar<T>>;

fn main() {}

@samueltardieu
Copy link
Contributor Author

Thanks @y21 that will be useful

@llogiq
Copy link
Contributor

llogiq commented Dec 30, 2024

@rustbot author

Please ping me again when there's something to review.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 30, 2024
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.
@samueltardieu samueltardieu force-pushed the normalize-raw-pointers branch from cbfa74b to bd57421 Compare January 2, 2025 20:52
@samueltardieu samueltardieu changed the title Normalize raw pointers Handle more cases in is_normalizable Jan 2, 2025
@samueltardieu
Copy link
Contributor Author

samueltardieu commented Jan 2, 2025

By limiting the recursion and assuming that recursive types are normalizable by default when recursing (which will be fixed when going up if needed), more cases are detected and some ICEs are fixed.

@rustbot review

Ping @llogiq and also ping @Jarcho as this collides with #12550 (but can serve as a stopgap until is_normalizable() is no longer needed).

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 2, 2025
@samueltardieu samueltardieu marked this pull request as ready for review January 2, 2025 20:59
@samueltardieu
Copy link
Contributor Author

@llogiq Do you see anything to be changed here? This has fixed all ICE I know of. Also, looking at the two commits separately, you can see that some diagnostics have been improved (which was the initial goal of this PR before I was made aware of the ICE):

LL | |     Recursive(Box<WithRecursion>),
   | |     ----------------------------- 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
5 participants