support non-defining uses of opaques in borrowck #145244
Merged
+1,317
−1,403
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-CI
lcnr
force-pushed
the
handle-opaque-types-before-region-inference
branch
from
0d7a748 to
e276bbf
Compare
lcnr
commented
Aug 11, 2025
| for error in errors { | ||
| guar = Some(match error { | ||
| DeferredOpaqueTypeError::InvalidOpaqueTypeArgs(err) => err.report(self.infcx), | ||
| DeferredOpaqueTypeError::LifetimeMismatchOpaqueParam(err) => { | ||
| self.infcx.dcx().emit_err(err) | ||
| } | ||
| DeferredOpaqueTypeError::UnexpectedHiddenRegion { |
There was a problem hiding this comment.
just moved from RegionErrorKind::UnexpectedHiddenRegion
lcnr
force-pushed
the
handle-opaque-types-before-region-inference
branch
3 times, most recently
from
14e5264 to
5536a31
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
force-pushed
the
handle-opaque-types-before-region-inference
branch
from
5536a31 to
298629b
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
force-pushed
the
handle-opaque-types-before-region-inference
branch
from
298629b to
9af9a2c
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
changed the title
lcnr
commented
Aug 11, 2025
| // FIXME(-Znext-solver): enable this test | ||
| //@ revisions: current next | ||
| //@ ignore-compare-mode-next-solver (explicit revisions) | ||
| //@[next] compile-flags: -Znext-solver |
There was a problem hiding this comment.
idk why I've added this FIXME to these 3 tests, only one of them actually failed with the new solver before.
lcnr
commented
Aug 11, 2025
lcnr
changed the title
lcnr
commented
Aug 11, 2025
lcnr
commented
Aug 11, 2025
lcnr
commented
Aug 11, 2025
lcnr
commented
Aug 11, 2025
| rcx.scc_values.add_region(member, min_choice_scc); | ||
| } | ||
|
|
||
| struct CollectMemberConstraintsVisitor<'a, 'b, 'tcx> { |
There was a problem hiding this comment.
equiv to existing register_member_constraints
lcnr
commented
Aug 11, 2025
lcnr
commented
Aug 11, 2025
lcnr
force-pushed
the
handle-opaque-types-before-region-inference
branch
3 times, most recently
from
9a309c7 to
30579d4
Compare
lcnr
force-pushed
the
handle-opaque-types-before-region-inference
branch
from
86063b3 to
c2a0fa8
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=BoxyUwU |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 20, 2025
…nce, r=BoxyUwU support non-defining uses of opaques in borrowck Reimplements the first part of #139587, but limited to only the new solver. To do so I also entirely rewrite the way we handle opaque types in borrowck even on stable. This should not impact behavior however. We now support revealing uses during MIR borrowck with the new solver: ```rust fn foo<'a>(x: &'a u32) -> impl Sized + use<'a> { let local = 1; foo::<'_>(&local); x } ``` ### How do opaque types work right now Whenever we use an opaque type during type checking, we remember this use in the `opaque_type_storage` of the `InferCtxt`. Right now, we collect all *member constraints* at the end of MIR type check by looking at all uses from the `opaque_type_storage`. We then apply these constraints while computing the region values for each SCC. This does not add actual region constraints but directly updates the final region values. This means we need to manually handle any constraints from member constraints for diagnostics. We do this by separately tracking `applied_member_constraints` in the `RegionInferenceContext`. After we've finished computing the region values, it is now immutable and we check whether all member constraints hold. If not, we error. We now map the hidden types of our defining uses to the defining scope. This assumes that all member constraints apply. To handle non-member regions, we simply map any region in the hidden type we fail to map to a choice region to `'erased` https://github.com/rust-lang/rust/blob/b1b26b834d85e84b46aa8f8f3ce210a1627aa85f/compiler/rustc_borrowck/src/region_infer/opaque_types.rs#L126-L132 ### How do we handle opaque types with this PR MIR type check still works the same by populating the `opaque_type_storage` whenever we use an opaque type. We now have a new step `fn handle_opaque_type_uses` which happens between MIR type check and `compute_regions`. This step looks at all opaque type uses in the storage and first checks whether they are defining: are the arguments of the `opaque_type_key` unique region parameters. *With the new solver we silently ignore any *non-defining* uses here. The old solver emits an errors.* `fn compute_concrete_opaque_types`: We then collect all member constraints for the defining uses and apply them just like we did before. However, we do it on a temporary region graph which is only used while computing the concrete opaque types. We then use this region graph to compute the concrete type which we then store in the `root_cx`. `fn apply_computed_concrete_opaque_types`: Now that we know the final concrete type of each opaque type and have mapped them to the definition of the opaque. We iterate over all opaque type uses and equate their hidden type with the instantiated final concrete type. This is the step which actually mutates the region graph. The actual region checking can now entirely ignores opaque types (outside of the `ConstraintCategory` from checking the opaque type uses). ### Diagnostics issue (chill) Because we now simply use type equality to "apply member constraints" we get ordinary `OutlivesConstraint`s, even if the regions were already related to another. This is generally not an issue, expect that it can *hide* the actual region constraints which resulted in the final value of the opaque. The constraints we get from checking against the final opaque type definition relies on constraints we used to compute that definition. I mostly handle this by updating `find_constraint_path_between_regions` to first ignore member constraints in its search and only if that does not find a path, retry while considering member constraints. ### Diagnostics issue (not chill) A separate issue is that `find_constraint_paths_between_regions` currently looks up member constraints by their **scc**, not by region value: https://github.com/rust-lang/rust/blob/2c1ac85679678dfe5cce7ea8037735b0349ceaf3/compiler/rustc_borrowck/src/region_infer/mod.rs#L1768-L1775 This means that in the `borrowck-4` test, the resulting constraint path is currently ``` ('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)), ('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)), ('?3: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1)) ``` Here `'?3` is equal to `'?4`, but the reason why it's in the opaque is that it's related to `'?4`. With my PR this will be correctly tracked so we end up with ``` ('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)), ('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)), ('?3: '?4) due to Single(bb0[6]) (None) (Assignment) (ConstraintSccIndex(1): ConstraintSccIndex(1)), ('?4: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1)), ``` This additional `Assignment` step then worsens the error message as we stop talking about the fact that the closures is returned from the function. Fixing this is hard. I've looked into this and it's making me sad :< Properly handling this requires some deeper changes to MIR borrowck diagnostics and that seems like too much for this PR. Given that this only impacts a single test, it seems acceptable to me. r? `@ghost`
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 125ff8a (parent) -> 922958c (this PR) Test differencesShow 41 test diffsStage 1
Stage 2
Additionally, 23 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 922958cffe059e9c156835df19d199ccd861c36a --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
This was referenced Aug 21, 2025
|
Finished benchmarking commit (922958c): comparison URL. Overall result: ❌ regressions - 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 2.9%, secondary -1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -4.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.807s -> 471.588s (-0.05%) |
|
I think recomputing the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reimplements the first part of #139587, but limited to only the new solver. To do so I also entirely rewrite the way we handle opaque types in borrowck even on stable. This should not impact behavior however.
We now support revealing uses during MIR borrowck with the new solver:
How do opaque types work right now
Whenever we use an opaque type during type checking, we remember this use in the
opaque_type_storageof theInferCtxt.Right now, we collect all member constraints at the end of MIR type check by looking at all uses from the
opaque_type_storage. We then apply these constraints while computing the region values for each SCC. This does not add actual region constraints but directly updates the final region values.This means we need to manually handle any constraints from member constraints for diagnostics. We do this by separately tracking
applied_member_constraintsin theRegionInferenceContext.After we've finished computing the region values, it is now immutable and we check whether all member constraints hold. If not, we error.
We now map the hidden types of our defining uses to the defining scope. This assumes that all member constraints apply. To handle non-member regions, we simply map any region in the hidden type we fail to map to a choice region to
'erasedrust/compiler/rustc_borrowck/src/region_infer/opaque_types.rs
Lines 126 to 132 in b1b26b8
How do we handle opaque types with this PR
MIR type check still works the same by populating the
opaque_type_storagewhenever we use an opaque type.We now have a new step
fn handle_opaque_type_useswhich happens between MIR type check andcompute_regions.This step looks at all opaque type uses in the storage and first checks whether they are defining: are the arguments of the
opaque_type_keyunique region parameters. With the new solver we silently ignore any non-defining uses here. The old solver emits an errors.fn compute_concrete_opaque_types: We then collect all member constraints for the defining uses and apply them just like we did before. However, we do it on a temporary region graph which is only used while computing the concrete opaque types. We then use this region graph to compute the concrete type which we then store in theroot_cx.fn apply_computed_concrete_opaque_types: Now that we know the final concrete type of each opaque type and have mapped them to the definition of the opaque. We iterate over all opaque type uses and equate their hidden type with the instantiated final concrete type. This is the step which actually mutates the region graph.The actual region checking can now entirely ignores opaque types (outside of the
ConstraintCategoryfrom checking the opaque type uses).Diagnostics issue (chill)
Because we now simply use type equality to "apply member constraints" we get ordinary
OutlivesConstraints, even if the regions were already related to another.This is generally not an issue, expect that it can hide the actual region constraints which resulted in the final value of the opaque. The constraints we get from checking against the final opaque type definition relies on constraints we used to compute that definition.
I mostly handle this by updating
find_constraint_path_between_regionsto first ignore member constraints in its search and only if that does not find a path, retry while considering member constraints.Diagnostics issue (not chill)
A separate issue is that
find_constraint_paths_between_regionscurrently looks up member constraints by their scc, not by region value:rust/compiler/rustc_borrowck/src/region_infer/mod.rs
Lines 1768 to 1775 in 2c1ac85
This means that in the
borrowck-4test, the resulting constraint path is currentlyHere
'?3is equal to'?4, but the reason why it's in the opaque is that it's related to'?4. With my PR this will be correctly tracked so we end up withThis additional
Assignmentstep then worsens the error message as we stop talking about the fact that the closures is returned from the function. Fixing this is hard. I've looked into this and it's making me sad :< Properly handling this requires some deeper changes to MIR borrowck diagnostics and that seems like too much for this PR. Given that this only impacts a single test, it seems acceptable to me.r? @ghost