borrowck: Normalize non-rigid aliases in NLL type relating - #161012
Conversation
|
r? @camelid rustbot has assigned @camelid. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? me We really shouldn't have non-rigid aliases here. I would guess that we lack a normalization call earlier, likely |
|
Imo we really shouldn't have non-rigid aliases in NLL relating. You were right, copying that into relate_tys was tempting and it's the wrong move. Dropping it.
That still doesn't cover the ICE though. It's So I'm also normalizing the operand at the Yield terminator and the Idk if this is the long-term answer once next-solver is default irl. Maybe NLL just grows real lazy norm later. Pushing asap, ltm if you'd rather I skip the operand/defining_ty bits. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Sorry for the delay :> |
|
@rustbot ready |
|
Sup, @adwinwhite! |
|
Ha, good nits. All four applied, answered each one inline. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…=<try> borrowck: Normalize non-rigid aliases in NLL type relating
1e47fcc to
f8dd6a8
Compare
Sup, @adwinwhite :) I think I addressed what you want. can you re-validate, please? @rustbot ready |
This comment has been minimized.
This comment has been minimized.
f8dd6a8 to
2701241
Compare
|
@bors r+ rollup |
…_aliases, r=adwinwhite borrowck: Normalize non-rigid aliases in NLL type relating Fixes rust-lang#160652 With `-Znext-solver=globally`, yielding from an `impl Iterator` without an explicit `Item` bound ICEs in borrowck. The coroutine defining type returned by `type_of` is unnormalized, so its yield type remains `<impl Iterator as Iterator>::Item`. Skipping normalization propagates that non-rigid alias into both MIR's `CoroutineInfo` and borrowck's `UniversalRegions`; NLL type relating then hits its invariant that non-rigid aliases must already have been normalized. Deeply normalize the instantiated defining type when MIR construction creates `CoroutineInfo` and when borrowck reconstructs `DefiningTy`. That makes the coroutine yield and resume types rigid before NLL compares them. This is intentionally gated to the next solver. The old solver keeps the existing skip-normalization path because deeply normalizing defining types there causes regressions.
Rollup of 7 pull requests Successful merges: - #150075 (Implement clamp_to) - #161034 (Add SVE-accelerated Vec::retain_mut for aarch64) - #161628 (interpret: ensure that calls via no-unwind ABIs do not unwind) - #161012 (borrowck: Normalize non-rigid aliases in NLL type relating) - #161702 (Use `drop_guard` in some places in {core,alloc,std}) - #161813 (Change `is_eligible_for_coverage` from a hook to a query) - #161842 (chore: fix cargo lints)
…_aliases, r=adwinwhite borrowck: Normalize non-rigid aliases in NLL type relating Fixes rust-lang#160652 With `-Znext-solver=globally`, yielding from an `impl Iterator` without an explicit `Item` bound ICEs in borrowck. The coroutine defining type returned by `type_of` is unnormalized, so its yield type remains `<impl Iterator as Iterator>::Item`. Skipping normalization propagates that non-rigid alias into both MIR's `CoroutineInfo` and borrowck's `UniversalRegions`; NLL type relating then hits its invariant that non-rigid aliases must already have been normalized. Deeply normalize the instantiated defining type when MIR construction creates `CoroutineInfo` and when borrowck reconstructs `DefiningTy`. That makes the coroutine yield and resume types rigid before NLL compares them. This is intentionally gated to the next solver. The old solver keeps the existing skip-normalization path because deeply normalizing defining types there causes regressions.
Rollup of 6 pull requests Successful merges: - #161034 (Add SVE-accelerated Vec::retain_mut for aarch64) - #161628 (interpret: ensure that calls via no-unwind ABIs do not unwind) - #161012 (borrowck: Normalize non-rigid aliases in NLL type relating) - #161702 (Use `drop_guard` in some places in {core,alloc,std}) - #161813 (Change `is_eligible_for_coverage` from a hook to a query) - #161842 (chore: fix cargo lints)
Rollup of 7 pull requests Successful merges: - #161034 (Add SVE-accelerated Vec::retain_mut for aarch64) - #161628 (interpret: ensure that calls via no-unwind ABIs do not unwind) - #161012 (borrowck: Normalize non-rigid aliases in NLL type relating) - #161691 (Assorted bootstrap config refactors (part 1/N)) - #161813 (Change `is_eligible_for_coverage` from a hook to a query) - #161842 (chore: fix cargo lints) - #161843 (rustdoc: fix lint `cargo::non_kebab_case_bins`)
Rollup merge of #161012 - Dnreikronos:borrowck/normalize_nll_aliases, r=adwinwhite borrowck: Normalize non-rigid aliases in NLL type relating Fixes #160652 With `-Znext-solver=globally`, yielding from an `impl Iterator` without an explicit `Item` bound ICEs in borrowck. The coroutine defining type returned by `type_of` is unnormalized, so its yield type remains `<impl Iterator as Iterator>::Item`. Skipping normalization propagates that non-rigid alias into both MIR's `CoroutineInfo` and borrowck's `UniversalRegions`; NLL type relating then hits its invariant that non-rigid aliases must already have been normalized. Deeply normalize the instantiated defining type when MIR construction creates `CoroutineInfo` and when borrowck reconstructs `DefiningTy`. That makes the coroutine yield and resume types rigid before NLL compares them. This is intentionally gated to the next solver. The old solver keeps the existing skip-normalization path because deeply normalizing defining types there causes regressions.
| let ocx = ObligationCtxt::new(&infcx); | ||
| let span = tcx.def_span(def_id); | ||
| let cause = ObligationCause::misc(span, def_id); | ||
| ocx.deeply_normalize(&cause, tcx.param_env(def_id), ty).unwrap() |
There was a problem hiding this comment.
that drops region constraints from normalization 🤔
I assume that's fine because the yield type has already been normalized in HIR typeck, so this should just replace the non-rigid aliases with rigid ones in the closure? Could we do that explicitly with a comment explaining what's going on?
Using a local ocx during borrowck feels brittle to me
There was a problem hiding this comment.
sup o/
Yeah, I think you're right about the local ocx. Basically when I wrote this I was mostly following the failure backwards from borrowck..it was getting a non-rigid alias where it expected a rigid one, so normalizing the type there fixed the ICE. It worked, but I don't think it's a great place to do it. Any region constraints created by that normalization live in a temporary inference context and disappear right after.
The part I missed is that HIR typeck had already normalized the closure type. type_of then puts it into an EarlyBinder, which marks the aliases as non-rigid again. So when borrowck gets the type, it looks like it needs normalization even though only the marker changed.
My feeling is that set_aliases_to_rigid says what we actually mean here and avoids running the solver again for no real reason. A comment would help too, because this path is pretty easy to misunderstand. I definitely misunderstood it the first time.
As this PR has already merged, what do you think makes sense as a follow-up? I could open another PR that changes this in borrowck and the MIR builder, since both read the same closure type, but I'm not sure if you'd prefer to keep it smaller.
There was a problem hiding this comment.
I assume that's fine because the yield type has already been normalized in HIR typeck, so this should just replace the non-rigid aliases with rigid ones in the closure?
Oh, I missed that rigidness can be shared between typeck and borrowck.
@Dnreikronos a follow-up would be good, thank you!
There was a problem hiding this comment.
@Dnreikronos can you add that fact as a comment on IsRigid I think? Unsure where we currently document the way rigidness works
There was a problem hiding this comment.
sup o/
ofc, i will work on this today!
Rollup of 7 pull requests Successful merges: - rust-lang/rust#161034 (Add SVE-accelerated Vec::retain_mut for aarch64) - rust-lang/rust#161628 (interpret: ensure that calls via no-unwind ABIs do not unwind) - rust-lang/rust#161012 (borrowck: Normalize non-rigid aliases in NLL type relating) - rust-lang/rust#161691 (Assorted bootstrap config refactors (part 1/N)) - rust-lang/rust#161813 (Change `is_eligible_for_coverage` from a hook to a query) - rust-lang/rust#161842 (chore: fix cargo lints) - rust-lang/rust#161843 (rustdoc: fix lint `cargo::non_kebab_case_bins`)
…_rigidity, r=adwinwhite borrowck: Restore alias rigidity from HIR typeck Follow-up to rust-lang#161012. This came out of the review thread here: rust-lang#161012 (comment) Borrowck was normalizing the closure type again in a fresh inference context. HIR writeback had already done that work. EarlyBinder only marked the aliases as non-rigid again because it has to be conservative. If the second normalization creates region constraints, they disappear with the temporary context. I first thought normalizing again here was fine. After tracing the type back through writeback, I think it makes more sense to trust the result from HIR typeck. Borrowck now restores the rigid flag, and the IsRigid docs explain that this state can carry into borrowck and when it needs to be reset. Tested with the coroutine regression and tidy. cc @adwinwhite @lcnr
…_rigidity, r=adwinwhite borrowck: Restore alias rigidity from HIR typeck Follow-up to rust-lang#161012. This came out of the review thread here: rust-lang#161012 (comment) Borrowck was normalizing the closure type again in a fresh inference context. HIR writeback had already done that work. EarlyBinder only marked the aliases as non-rigid again because it has to be conservative. If the second normalization creates region constraints, they disappear with the temporary context. I first thought normalizing again here was fine. After tracing the type back through writeback, I think it makes more sense to trust the result from HIR typeck. Borrowck now restores the rigid flag, and the IsRigid docs explain that this state can carry into borrowck and when it needs to be reset. Tested with the coroutine regression and tidy. cc @adwinwhite @lcnr
Rollup merge of #161926 - Dnreikronos:borrowck/restore_alias_rigidity, r=adwinwhite borrowck: Restore alias rigidity from HIR typeck Follow-up to #161012. This came out of the review thread here: #161012 (comment) Borrowck was normalizing the closure type again in a fresh inference context. HIR writeback had already done that work. EarlyBinder only marked the aliases as non-rigid again because it has to be conservative. If the second normalization creates region constraints, they disappear with the temporary context. I first thought normalizing again here was fine. After tracing the type back through writeback, I think it makes more sense to trust the result from HIR typeck. Borrowck now restores the rigid flag, and the IsRigid docs explain that this state can carry into borrowck and when it needs to be reset. Tested with the coroutine regression and tidy. cc @adwinwhite @lcnr
View all comments
Fixes #160652
With
-Znext-solver=globally, yielding from animpl Iteratorwithout an explicitItembound ICEs in borrowck. The coroutine defining type returned bytype_ofis unnormalized, so its yield type remains<impl Iterator as Iterator>::Item. Skipping normalization propagates that non-rigid alias into both MIR'sCoroutineInfoand borrowck'sUniversalRegions; NLL type relating then hits its invariant that non-rigid aliases must already have been normalized.Deeply normalize the instantiated defining type when MIR construction creates
CoroutineInfoand when borrowck reconstructsDefiningTy. That makes the coroutine yield and resume types rigid before NLL compares them.This is intentionally gated to the next solver. The old solver keeps the existing skip-normalization path because deeply normalizing defining types there causes regressions.