-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
fix ICE when freshTy(0) leaks into sizedness and fudge paths #151732
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
fix ICE when freshTy(0) leaks into sizedness and fudge paths #151732
Conversation
FreshTy is used as trait_object_dummy_self for dyn types and should not reach has_trivial_sizedness or assemble_object_bound_candidates. add checks to skip the fast path and return early for FreshTy types to prevent ICEs in the new trait solver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for trying to fix this but this is not the right fix. trait_object_dummy_self should never escape HIR ty lowering, it's lower_trait_object_ty's fault that it currently does. This issue has been known for around 2 years and there are other similar GH issues which are still open.
See e.g., this source code comment I've recently added:
rust/compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_trait.rs
Lines 61 to 65 in 78df2f9
| // FIXME(mgca): We actually leak `trait_object_dummy_self` if the type of any assoc | |
| // const mentions `Self` (in "Self projections" which we intentionally | |
| // allow). That's because we query feed the instantiated type to `type_of`. | |
| // That's really bad, dummy self should never escape lowering! It leads us | |
| // to accept less code we'd like to support and can lead to ICEs later. |
Or this one:
Lines 1 to 5 in 78df2f9
| // FIXME(mgca): Ideally this would compile -- at least if the user annotated the instantiated type | |
| // of the assoc const (but we don't have the syntax for this (yet)). In any case, we | |
| // should not leak `trait_object_dummy_self` (defined as `FreshTy(0)` under the hood) | |
| // to the rest of the compiler and by extension the user via diagnostics. | |
| //@ known-bug: unknown |
The proper fix will be different, I don't yet have a clear picture in my head though.
|
Reminder, once the PR becomes ready for a review, use |
|
I don't think it makes sense to keep this PR open since the final approach will look completely different. Therefore I'm going to close this. Boxy, please correct me if I'm wrong. CC https://github.com/rust-lang/rust/blob/main/tests/ui/const-generics/associated-const-bindings/dyn-compat-self-const-projections-in-assoc-const-ty.rs and maybe also #123140. These kinds of issues are intentionally kept unsolved right now (see e.g., PR description of #150843). |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
using
-Znext-solver=globallywithdyn A<Ty = i32, CT = 0>was able to trigger a couple of different ICEs: first whentrait_object_dummy_self (FreshTy(0))leaked into sizedness fast paths and object candidates, and then whenfudge_inference_if_okran intoFresh*vars and hit anunreachable!(i saw second one while fixing the original issue). this patch makes those code paths either bail out early or just treat Fresh* as regular types (including forwarding them throughInferenceFudger)also, of course i added a test
about the issue: when we lower
dyn A<Ty = i32, CT = 0>with the new trait solver, we use an internal dummySelfcalledtrait_object_dummy_self, represented asFreshTy(0)(andFreshIntTy/FreshFloatTyfor the other cases). that means the solver ends up with goals likeFreshTy(0): MetaSizedorFreshTy(0): A<…>, even thoughFreshTy(0)is just theSelfof this dyn object, not a real user type. several fast paths and bits of diagnostics assumed they would never seeFresh*there (has_trivial_sizedness, sizedness assembly, for_each_relevant_impl, fudge_inference_if_ok) and buged or hitunreachable!, so oncetrait_object_dummy_selfstarted going through those paths, we got ICEs instead of normal errors.fixes #151709