borrowck: Restore alias rigidity from HIR typeck - #161926
Conversation
HIR writeback already normalizes closure types. Reuse that result instead of normalizing again in a temporary inference context that would drop region constraints.
rustbot
added
S-waiting-on-review
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? types |
rustbot
added
the
T-types
|
r? adwinwhite :> |
| /// Rigidness is shared across inference contexts and compiler phases. For example, aliases | ||
| /// normalized during HIR typeck can remain rigid when the resulting type is later used by | ||
| /// borrowck. They must be made non-rigid again if they enter a typing mode or parameter | ||
| /// environment in which further normalization may be possible. | ||
| /// |
There was a problem hiding this comment.
| /// Rigidness is shared across inference contexts and compiler phases. For example, aliases | |
| /// normalized during HIR typeck can remain rigid when the resulting type is later used by | |
| /// borrowck. They must be made non-rigid again if they enter a typing mode or parameter | |
| /// environment in which further normalization may be possible. | |
| /// | |
| /// Rigidness becomes outdated when the surrounding typing mode or param env changes, | |
| /// because further normalization might be possible. | |
| /// We should also note that rigidness can be shared within some typing mode groups | |
| /// if the param env is the same, e.g., `Typeck/PostTypeckUntilBorrowck` and | |
| /// `PostAnalysis/Codegen`. | |
| /// |
There was a problem hiding this comment.
yeah, I think I wrote that comment around this PR instead of explaining the actual rule. The flag becomes stale when the typing mode or param env changes and more normalization could become possible. Typeck/PostTypeckUntilBorrowck can still share it when the param env is the same, and the same goes for PostAnalysis/Codegen.
I changed the docs along those lines. I like this wording more because it explains when rigidness stops being valid, instead of making HIR typeck and borrowck sound like a special case.
| let defining_ty = | ||
| tcx.type_of(body_def_id).instantiate_identity().skip_normalization(); | ||
| let defining_ty = if tcx.next_trait_solver_globally() { | ||
| // Closure types come from HIR typeck results, where they were already | ||
| // normalized during writeback. Wrapping them in an `EarlyBinder` | ||
| // conservatively makes aliases non-rigid, so restore their rigidness | ||
| // instead of normalizing them again during borrowck. | ||
| ty::set_aliases_to_rigid(tcx, defining_ty) | ||
| } else { | ||
| defining_ty | ||
| }; |
There was a problem hiding this comment.
should do the same for the mir build one?
There was a problem hiding this comment.
yeah, did that one too.
I initially left MIR build alone because the borrowck case looked like the real problem. That normalization used a fresh inference context, so any region constraints created there were thrown away. MIR build keeps its inference context around for the builder, so it was not quite the same bug.
Still, both places read the same closure type from HIR writeback. EarlyBinder::bind makes its aliases non-rigid because it has to be conservative, but this path stays in the same typing mode group with the same param env. Running the solver again does not buy us anything there.
I replaced the MIR build normalization with set_aliases_to_rigid too and removed the helper that was only there to normalize it. I think this is cleaner. MIR build and borrowck now agree that the HIR typeck result can be reused, and the coroutine regression still passes.
HIR writeback already normalizes closure types. Restore alias rigidity after identity instantiation instead of running the solver again while building MIR.
Document when changes to the typing mode or parameter environment make rigidness stale, and when compatible mode groups may reuse it.
|
Thanks! |
rust-bors
Bot
added
S-waiting-on-bors
…_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 of 8 pull requests Successful merges: - #161301 (libcore: expose volatile atomic operations) - #161379 (Use better generic type parameter names for `Extend` and `FromIterator`) - #161926 (borrowck: Restore alias rigidity from HIR typeck) - #161956 (Remove unused `perform_locally_with_next_solver`) - #162026 (Emit delayed bug instead of ICEing when `TypeOutlives` goal fails) - #162034 (Make the LLVM version mismatch ICE a fatal error) - #162037 (LLVM wrapper cleanups) - #162043 (_ an unused parameter)
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
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (85335e5): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.7%, secondary -0.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: missing data |
7 participants
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