Don't build ParamEnv and do trait solving in ItemCtxts when lowering IATs
#140247
Conversation
rustbot
added
S-waiting-on-review
|
This PR changes a file inside HIR ty lowering was modified cc @fmease |
|
☔ The latest upstream changes (presumably #140388) made this pull request unmergeable. Please resolve the merge conflicts. |
BoxyUwU
force-pushed
the
iats_no_query_cycles
branch
from
3361c7d to
34ce403
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#135562 (Add ignore value suggestion in closure body) - rust-lang#139635 (Finalize repeat expr inference behaviour with inferred repeat counts) - rust-lang#139668 (Handle regions equivalent to 'static in non_local_bounds) - rust-lang#140218 (HIR ty lowering: Clean up & refactor the lowering of type-relative paths) - rust-lang#140435 (use uX::from instead of _ as uX in non - const contexts) - rust-lang#141130 (rustc_on_unimplemented cleanups) - rust-lang#141286 (Querify `coroutine_hidden_types`) Failed merges: - rust-lang#140247 (Don't build `ParamEnv` and do trait solving in `ItemCtxt`s when lowering IATs) r? `@ghost` `@rustbot` modify labels: rollup
|
☔ The latest upstream changes (presumably #141396) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
BoxyUwU
force-pushed
the
iats_no_query_cycles
branch
2 times, most recently
from
aee1a22 to
701d3b8
Compare
|
I should probably update this to use DeepRejectCtxt and handle LTAs correctly like discussed at the all hands 🤔 I was just gonna do it in a followup PR but if I'm having to rebase and figure out new crashes tests then I may aswell just do it all now |
BoxyUwU
force-pushed
the
iats_no_query_cycles
branch
from
701d3b8 to
276daf2
Compare
BoxyUwU
force-pushed
the
iats_no_query_cycles
branch
from
276daf2 to
5867bb5
Compare
| // We make an infcx and replace any escaping vars with placeholders so that IAT res | ||
| // with type/const bound vars in arguments is slightly smarter. `for<T> <Foo<T>>::IAT` | ||
| // would otherwise unify with `impl Foo<u8>` when ideally we would not. | ||
| let infcx = self.tcx().infer_ctxt().build(TypingMode::non_body_analysis()); |
There was a problem hiding this comment.
Creating an infcx just to make placeholders feels pretty bad lol
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
iats_no_query_cycles
branch
from
5867bb5 to
02b87a8
Compare
| if candidates.is_empty() { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
Removing this masked a bunch of mgca ICEs I think. We now emit errors in all cases where IAT resolution fails to find any candidates which makes some delayed bugs no longer trigger. I've replaced a few of the crashes tests with examples that don't rely on IATs and I removed the two others that I couldn't find a reproducer for that didn't seem like a dupe of the other three.
BoxyUwU
force-pushed
the
iats_no_query_cycles
branch
from
02b87a8 to
ccec360
Compare
BoxyUwU
added
S-waiting-on-review
Fixes #108491
Fixes #125879
This was due to updating inhabited predicate stuff which I had to do to make constructing ADTs with IATs in fields not ICE
Fixes #136678 (but no test added, I don't rly care about weird IAT edge cases under GCE)
Fixes #138240
Fixes #138266
These ICEs got masked but we have 3 other ICEs that look like dupes so I'm not too worried about this. Caused by mgca not properly lowering paths in type signatures to forbid infers or something, and now we happen to emit an error when we previously didnt.
Avoids doing "fully correct" candidate selection for IATs during hir ty lowering when in item signatures as it almost always leads to a query cycle from trying to build a
ParamEnv. I replaced it with a useDeepRejectCtxtwhich should be able to handle this kind of conservative "could these types unify" while in a context where we don't want to do type equality.A potential alternative would be to replace all the generic parameters in the self type with inference variables as then it would be "truly" environment agnostic and we could just use an empty environment. I think this is somewhat subpar as we would be weakening how much we used the self type to guide candidate selection and that's the only way for the user to attempt to specify which IAT to resolve to.
In general that's also why I think doing something hacky here instead of just throwing our hands up and saying "ItemCtxts shouldnt make inference variables" and not filtering any candidates out, is a good idea (there is no way for users to disambiguate such cases).
I'm not really sure how this interacts with #126651, though I'm also not really sure its super important to support projecting IATs from IAT self types given we don't even support
T::Assoc::Otherfor trait-associated types so didn't give much thought to how this might fit in with that.r? @compiler-errors
cc @fmease