Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

-Znext-solver: support non-defining uses in closures #145925

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

Merged
bors merged 2 commits into rust-lang:master from lcnr:revealing-use-closures-2
Sep 2, 2025

Conversation

Copy link
Contributor

@lcnr lcnr commented Aug 27, 2025
edited
Loading

Cleaned up version of #139587, finishing the implementation of rust-lang/types-team#129. This does not affect stable. The reasoning for why this is the case is subtle however.

What does it do

We split do_mir_borrowck into borrowck_collect_region_constraints and borrowck_check_region_constraints, where borrowck_collect_region_constraints returns an enormous CollectRegionConstraintsResult struct which contains all the relevant data to actually handle opaque type uses and to check the region constraints later on.

query mir_borrowck now simply calls BorrowCheckRootCtxt::do_mir_borrowck which starts by iterating over all nested bodies of the current function - visiting nested bodies before their parents - and computing their CollectRegionConstraintsResult.

After we've collected all constraints it's time to actually compute the concrete types for the opaques defined by this function. With this PR we now compute the concrete types of opaques for each body before using them to check the non-defining uses of any of them.

After we've computed the concrete types by using all bodies, we use apply_computed_concrete_opaque_types for each body to constrain non-defining uses, before finally finishing with borrowck_check_region_constraints. We always visit nested bodies before their parents when doing this.

ClosureRegionRequirements

As we only call borrowck_collect_region_constraints for nested bodies before type checking the parent, we can't simply use the final ClosureRegionRequirements of the nested body during MIR type check. We instead track that we need to apply these requirements in deferred_closure_requirements.

We now manually apply the final closure requirements to each body after handling opaque types.

This works, except that we may need the region constraints of nested bodies to successfully define an opaque type in the parent. This is handled by using a new fn compute_closure_requirements_modulo_opaques which duplicates region checking - while ignoring any errors - before we've added the constraints from apply_computed_concrete_opaque_types. This is necessary for a lot of async tests, as pretty much the entire function is inside of an async block while the opaque type gets defined in the parent.

As an performance optimization we only use fn compute_closure_requirements_modulo_opaques in case the nested body actually depends on any opaque types. Otherwise we eagerly call borrowck_check_region_constraints and apply the final closure region requirements right away.

Impact on stable code

Handling the opaque type uses in the parent function now only uses the closure requirements modulo opaques, while it previously also considered member constraints from nested bodies. External regions are never valid choice regions. Also, member constraints will never constrain a member region if it is required to be outlived by an external region, as that fails the upper-bound check.

// If we have a requirement `'upper_bound: 'member`, equating `'member`
// with some region `'choice` means we now also require `'upper_bound: 'choice`.
// Avoid choice regions for which this does not hold.
for ub in rcx.rev_scc_graph.upper_bounds(member) {
choice_regions
.retain(|&choice_region| rcx.universal_region_relations.outlives(ub, choice_region));
}

Member constraints therefore never add constraints for external regions :>

r? @BoxyUwU

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 27, 2025
Copy link
Contributor Author

lcnr commented Aug 27, 2025

@bors try @rust-timer queue

This comment has been minimized.

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Aug 27, 2025
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 27, 2025

This comment has been minimized.

@lcnr lcnr force-pushed the revealing-use-closures-2 branch 2 times, most recently from f736683 to e9dcabd Compare August 27, 2025 12:01
@lcnr lcnr force-pushed the revealing-use-closures-2 branch 3 times, most recently from eaa44ba to 4d7c068 Compare August 27, 2025 12:16
@lcnr lcnr added WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 27, 2025

This comment has been minimized.

@lcnr lcnr force-pushed the revealing-use-closures-2 branch from 4d7c068 to 22d33e3 Compare August 27, 2025 12:58
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 27, 2025

This comment has been minimized.

Copy link

rust-bors bot commented Aug 27, 2025

☀️ Try build successful (CI)
Build commit: f55097d (f55097dba6b9481e3e3e65d3d7c2279c1a900519, parent: 4f808ba6bf9f1c8dde30d009e73386d984491587)

This comment has been minimized.

@lcnr lcnr force-pushed the revealing-use-closures-2 branch 2 times, most recently from 378c073 to 4406446 Compare August 27, 2025 14:06
@traviscross traviscross added the I-lang-radar Items that are on lang's radar and will need eventual work or consideration. label Aug 27, 2025
@lcnr lcnr changed the title (削除) support non-defining uses in closures (削除ここまで) (追記) -Znext-solver support non-defining uses in closures (追記ここまで) Aug 27, 2025
@lcnr lcnr changed the title (削除) -Znext-solver support non-defining uses in closures (削除ここまで) (追記) -Znext-solver: support non-defining uses in closures (追記ここまで) Aug 27, 2025
Copy link
Collaborator

Finished benchmarking commit (f55097d): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.4% [0.2%, 0.6%] 13
Regressions ❌
(secondary)
0.4% [0.2%, 0.9%] 18
Improvements ✅
(primary)
-0.2% [-0.2%, -0.2%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [-0.2%, 0.6%] 15

Max RSS (memory usage)

Results (primary -1.0%, secondary -2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
1.3% [1.3%, 1.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.1% [-2.5%, -1.6%] 2
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) -1.0% [-2.5%, 1.3%] 3

Cycles

Results (secondary 0.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.8% [0.8%, 0.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 467.222s -> 467.244s (0.00%)
Artifact size: 391.17 MiB -> 391.06 MiB (-0.03%)

This comment has been minimized.

Copy link
Member

@BoxyUwU BoxyUwU left a comment
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this shouldn't affect stable. Member constraints in parent bodies being affected by region requirements from defining uses in child bodies would require an external region being influenced by member constraints which is not possible.

I think it's unfortunate to not properly handle the cyclic dependency of child bodies and parent bodies both having uses of opaques that may depend on region constraints from eachother. I think it's fine to maybe do something about that in the future as part of a larger cleanup of the member constraint algorithm.

r=me once u fix that test

View changes since this review

@lcnr lcnr force-pushed the revealing-use-closures-2 branch from 417a251 to ef9653e Compare September 1, 2025 13:34

This comment has been minimized.

@lcnr lcnr force-pushed the revealing-use-closures-2 branch from ef9653e to 60171d5 Compare September 1, 2025 18:58
Copy link
Member

lqd commented Sep 1, 2025
edited
Loading

since boxy has given an r+: I love it but the last commit could use a more descriptive message than whaddup gamers, before landing :3.

lcnr reacted with thumbs up emoji

This comment has been minimized.

@lcnr lcnr force-pushed the revealing-use-closures-2 branch from 60171d5 to 1c3a6ee Compare September 1, 2025 20:03
@lcnr lcnr force-pushed the revealing-use-closures-2 branch from 1c3a6ee to b8160e9 Compare September 1, 2025 20:08
Copy link
Contributor Author

lcnr commented Sep 1, 2025

@bors r=BoxyUwU rollup=never

Copy link
Collaborator

bors commented Sep 1, 2025

📌 Commit b8160e9 has been approved by BoxyUwU

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2025
Copy link
Collaborator

bors commented Sep 1, 2025

⌛ Testing commit b8160e9 with merge 75ee9ff...

Copy link
Collaborator

bors commented Sep 2, 2025

☀️ Test successful - checks-actions
Approved by: BoxyUwU
Pushing 75ee9ff to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2025
@bors bors merged commit 75ee9ff into rust-lang:master Sep 2, 2025
11 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Sep 2, 2025
Copy link
Contributor

github-actions bot commented Sep 2, 2025

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 7aef4be (parent) -> 75ee9ff (this PR)

Test differences

Show 250 test diffs

Stage 1

  • [ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs: pass -> [missing] (J0)
  • [ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#current: [missing] -> pass (J0)
  • [ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#next: [missing] -> pass (J0)
  • [ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#current: [missing] -> pass (J0)
  • [ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#next: [missing] -> pass (J0)

Stage 2

  • [ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs: pass -> [missing] (J1)
  • [ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#current: [missing] -> pass (J1)
  • [ui] tests/ui/impl-trait/non-defining-uses/double-wrap-with-defining-use.rs#next: [missing] -> pass (J1)
  • [ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#current: [missing] -> pass (J1)
  • [ui] tests/ui/impl-trait/non-defining-uses/recursive-call.rs#next: [missing] -> pass (J1)

Additionally, 240 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
 test-dashboard 75ee9ffd5ed3649c0a09493057adaa8feebb2035 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. pr-check-1: 1335.7s -> 1728.3s (29.4%)
  2. x86_64-rust-for-linux: 2644.5s -> 3170.8s (19.9%)
  3. aarch64-apple: 7320.6s -> 6075.6s (-17.0%)
  4. aarch64-gnu-llvm-19-1: 3235.0s -> 3714.9s (14.8%)
  5. x86_64-gnu-llvm-19: 2381.1s -> 2729.6s (14.6%)
  6. dist-x86_64-apple: 7384.2s -> 6325.8s (-14.3%)
  7. dist-arm-linux-gnueabi: 5505.5s -> 4717.7s (-14.3%)
  8. test-various: 4444.8s -> 5029.7s (13.2%)
  9. x86_64-gnu-debug: 7319.5s -> 8240.0s (12.6%)
  10. i686-gnu-nopt-1: 7291.5s -> 8192.4s (12.4%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

Copy link
Collaborator

Finished benchmarking commit (75ee9ff): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.2% [0.1%, 0.6%] 33
Regressions ❌
(secondary)
0.3% [0.0%, 0.8%] 29
Improvements ✅
(primary)
-0.3% [-0.3%, -0.2%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.3%, 0.6%] 36

Max RSS (memory usage)

Results (primary 0.0%, secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
3.0% [3.0%, 3.0%] 1
Regressions ❌
(secondary)
3.8% [3.8%, 3.8%] 1
Improvements ✅
(primary)
-1.5% [-1.7%, -1.3%] 2
Improvements ✅
(secondary)
-2.5% [-2.8%, -2.3%] 2
All ❌✅ (primary) 0.0% [-1.7%, 3.0%] 3

Cycles

Results (primary 2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 469.026s -> 467.688s (-0.29%)
Artifact size: 388.37 MiB -> 388.42 MiB (0.01%)

Copy link
Member

Kobzol commented Sep 2, 2025

Do you think it's worth it to investigate the regressions?

Copy link
Contributor Author

lcnr commented Sep 3, 2025

I don't think so, it's a small consistent regression and we are doing a bit more work

Kobzol reacted with thumbs up emoji

@lcnr lcnr deleted the revealing-use-closures-2 branch September 3, 2025 20:40
Copy link
Member

Kobzol commented Sep 3, 2025

Ok 👍

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BoxyUwU BoxyUwU BoxyUwU approved these changes

Labels
I-lang-radar Items that are on lang's radar and will need eventual work or consideration. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
1.91.0
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /