Skip to content

Consider captured regions for opaque type region liveness.#156027

Merged
rust-bors[bot] merged 8 commits into
rust-lang:mainfrom
jackh726:opaque-liveness
Jul 10, 2026
Merged

Consider captured regions for opaque type region liveness.#156027
rust-bors[bot] merged 8 commits into
rust-lang:mainfrom
jackh726:opaque-liveness

Conversation

@jackh726

@jackh726 jackh726 commented May 1, 2026

Copy link
Copy Markdown
Member

View all comments

Fixes #153215

For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds.

r? lqd

@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 May 1, 2026
@lqd

lqd commented May 1, 2026

Copy link
Copy Markdown
Member

Until I can look at this next week, @bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 1, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request May 1, 2026
Consider captured regions for opaque type region liveness.
@rust-bors

rust-bors Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 97f0332 (97f0332b563f5fd8ae70b57a13a7b23b27db3012, parent: 0469a92a76c327df972cb6c1356934b7a0c6b86d)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (97f0332): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.3%)

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
Improvements ✅
(primary)
-2.3% [-2.3%, -2.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.3% [-2.3%, -2.3%] 1

Cycles

Results (secondary 2.0%)

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)
4.1% [3.3%, 4.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) - - 0

Binary size

Results (primary 0.2%, secondary 0.2%)

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

mean range count
Regressions ❌
(primary)
0.2% [0.1%, 0.4%] 33
Regressions ❌
(secondary)
0.2% [0.0%, 0.4%] 24
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.1%, 0.4%] 33

Bootstrap: 481.713s -> 483.106s (0.29%)
Artifact size: 390.96 MiB -> 390.99 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 1, 2026

@lqd lqd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also started down this direction so some of this makes sense to me. We stopped because it felt fragile and test coverage was, and still is, weak. So with that caveat this generally feels fine to me, but again, I don't know enough about this area to say what problems there could be.

This direction felt less nice than what lcnr described in the t-types meeting where we described the issue, so I'll defer to them.

View changes since this review

Comment thread compiler/rustc_middle/src/queries.rs Outdated
Comment thread compiler/rustc_trait_selection/src/traits/outlives_for_liveness.rs Outdated
Comment on lines +55 to +59
// Thinking about it, I was originally a bit concerned about something like `'a: 'static`, and
// whether or not we need to mark `'a` as live. I don't think *today* we do, since I think regions
// that outlive `'static` are special enough, but I *could* imagine some world where we need to be
// more careful about this. Given I can't find a test that goes wrong, I'm going to leave in this
// optimization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was also a bit worried, and we should expand test coverage here. I'm not confident enough in any of this to trust the two tests we have.

Comment thread compiler/rustc_trait_selection/src/traits/outlives_for_liveness.rs Outdated
Comment thread compiler/rustc_trait_selection/src/traits/outlives_for_liveness.rs Outdated
Comment on lines +319 to +321
// Unfortunately, we have to use a new `InferCtxt` each call, because
// region constraints get added and solved there and we need to test each
// call individually.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is also unfortunate, combined with the fact that it's being called a quadratic number of times (thankfully the N should be small in most cases).

@lcnr lcnr self-assigned this May 5, 2026
@rustbot

This comment has been minimized.

Comment thread compiler/rustc_trait_selection/src/traits/outlives_for_liveness.rs Outdated
@jackh726

jackh726 commented Jun 3, 2026

Copy link
Copy Markdown
Member Author

@bors try

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Jun 3, 2026
Consider captured regions for opaque type region liveness.
@rust-bors

rust-bors Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 8e819ff (8e819ff5434466e9a823e03871a1775ac7d84ddf, parent: 53509ca37e3b507887607c2f4a7f23bd4838f099)

@jackh726

jackh726 commented Jun 3, 2026

Copy link
Copy Markdown
Member Author

@craterbot check

@craterbot

Copy link
Copy Markdown
Collaborator

👌 Experiment pr-156027 created and queued.
🤖 Automatically detected try build 8e819ff
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2026
@jackh726

jackh726 commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@craterbot p=2

We want to get this in so that we can do a nightly preview of Polonius

@craterbot

Copy link
Copy Markdown
Collaborator

📝 Configuration of the pr-156027 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

Copy link
Copy Markdown
Collaborator

🚧 Experiment pr-156027 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

Copy link
Copy Markdown
Collaborator

🎉 Experiment pr-156027 is completed!
📊 65 regressed and 1 fixed (964870 total)
📊 5391 spurious results on the retry-regressed-list.txt, consider a retry1 if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

  1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-156027/retry-regressed-list.txt

@craterbot craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 7, 2026

@lcnr lcnr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still feel unresolved wrt the why of this change, which makes reviewing the concrete behavior kind of unpleasant. gonna spend some time looking at the non-polonius thing this allows us to fix?

View changes since this review

/// (and so is `T`, since `T: 'static` implies `T: 'a`)
/// - `bar` outlives `'static`, so we know that no args are potentially live and we can return an empty set
/// - `baz` has no outlives bound, so return `None` and let the caller decide what to do
query live_args_for_alias_from_outlives_bounds(kind: ty::AliasTyKind<'tcx>) -> &'tcx Option<ty::EarlyBinder<'tcx, Vec<ty::GenericArg<'tcx>>>> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There should no difference between AliasTyKind and DefId wrt what information they give you. Both are just a DefId, where AliasTyKind also has some information about DefKind in there.

Reconstructing an AliasTy from a DefId is intentionally vaguely annoying 🤔 even though I'd expect that the query system is significantly better at dealing with raw DefIds I guess it's whatever whether to use AliasTy or DefId here

let parent_param_env = tcx.param_env(parent_def_id);
tracing::debug!(?parent_param_env);

// Map the outlives regions to the parent regions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean concretely for this loop 🤔 this is a lot of nothing which feels unrelated to the actual semantics of this function?

Like, we take self_identity_args and give back a parent_outlives_regions. I do not understand why this is necessary or what exactly it even does. What does parent_outlives_regions represent, why do we need to do this etc?

Comment thread compiler/rustc_trait_selection/src/traits/outlives_for_liveness.rs Outdated
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment was marked as outdated.

jackh726 added 2 commits July 9, 2026 00:01
…arams, use tcx.assumed_wf_types, and test polonius_alpha in gat-outlives-implied test
@lqd

lqd commented Jul 9, 2026

Copy link
Copy Markdown
Member

I am still left feeling unclear why polonius cares about liveness the way it does

NLLs introduced liveness to allow invalidations on dead loans, and its regions are sets of CFG points. The points where the regions are live, the regions they outlive (in a flow-insensitive manner) and vice-versa, and so on are used to compute the CFG points that represent regions. A loan is considered live at a point if the region where it was introduced contains this point (modulo kills, unimportant here).

To support flow-sensitivity, polonius regions are sets of loans, and loan liveness needs to track the outlives relationships between regions more precisely: a loan is considered live if it is contained in a live region. Polonius thus needs the accurate liveness of regions to check if an invalidation is indeed an error. As an NLL superset, if a loan is contained in a dead region and is invalidated, polonius will accept that as well. If the region is unexpectedly dead like in the #153215 examples, this is of course a soundness issue, and this PR fixes the current inaccuracy in liveness for some regions related to aliases.

The approach makes sense to me, and we've talked about it with jack and niko, and despite the complexity I think this is an improvement we need. There may be ways to simplify in the near future, a part of it is caused by the mapping and our current encoding of opaque types, etc. lcnr has told jack that we can land this if I'm also happy with it, and I am. It does get us closer to enabling polonius alpha on nightly.

If there are more improvements to do after this handful of review rounds, I think it's fine to do them in follow-ups.

Let's do this 🚀, @bors r+

@rust-bors

rust-bors Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 055af58 has been approved by lqd

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Jul 9, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Jul 9, 2026
Consider captured regions for opaque type region liveness.

Fixes rust-lang#153215

For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds.

r? lqd
rust-bors Bot pushed a commit that referenced this pull request Jul 9, 2026
…uwer

Rollup of 24 pull requests

Successful merges:

 - #150946 (intrinsics: Add a fallback for non-const libm float functions)
 - #158510 (Enable `static_position_independent_executables` on all gnu and musl targets)
 - #158541 (Move `std::io::Write` to `core::io`)
 - #158899 (Fix `unaligned_volatile_store` by removing `MemFlags::UNALIGNED`)
 - #156027 (Consider captured regions for opaque type region liveness.)
 - #156370 (Reject linked dylib EII default overrides)
 - #156508 (Infer all anonymous lifetimes in assoc consts as `'static`)
 - #157561 (rustdoc: do not include extra stuff in span)
 - #158617 (allow mGCA const arguments to fall back to anon consts)
 - #158645 (Fix splat ICEs and ban it in closures)
 - #158859 (Improve `-Zls` diagnostic message on `.rs` files)
 - #158988 (Redo `TokenStreamIter`)
 - #158347 (Improve generic parameters handling for #[diagnostic::on_const])
 - #158384 (Allow BackwardIncompatibleDropHint in polonius legacy)
 - #158722 (delegation: do not always inherit `ConstArgHasType` predicates)
 - #158739 (view-types: HIR lowering)
 - #158877 (borrowck: Keep returned `path` from `best_blame_constraint()` consistent)
 - #158883 (tests: fix enum-match.rs to handle LLVM 23)
 - #158886 (Add documentation for the `no_std` attribute)
 - #158940 (Implement feature `char_to_u32`)
 - #158951 (Merge three `MaxUniverse`s into one)
 - #158961 (Reapply "LLVM 23: Run AssignGUIDPass in some places")
 - #158995 (Use REST API in linkchecker script)
 - #158996 ([compiler] Implement `PartialOrd` via `Ord` for `Span` and newtype_indexes)
jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 9, 2026
Consider captured regions for opaque type region liveness.

Fixes rust-lang#153215

For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds.

r? lqd
rust-bors Bot pushed a commit that referenced this pull request Jul 10, 2026
Rollup of 24 pull requests

Successful merges:

 - #150946 (intrinsics: Add a fallback for non-const libm float functions)
 - #158541 (Move `std::io::Write` to `core::io`)
 - #156027 (Consider captured regions for opaque type region liveness.)
 - #156370 (Reject linked dylib EII default overrides)
 - #156508 (Infer all anonymous lifetimes in assoc consts as `'static`)
 - #157561 (rustdoc: do not include extra stuff in span)
 - #158617 (allow mGCA const arguments to fall back to anon consts)
 - #158645 (Fix splat ICEs and ban it in closures)
 - #158859 (Improve `-Zls` diagnostic message on `.rs` files)
 - #158988 (Redo `TokenStreamIter`)
 - #158347 (Improve generic parameters handling for #[diagnostic::on_const])
 - #158384 (Allow BackwardIncompatibleDropHint in polonius legacy)
 - #158722 (delegation: do not always inherit `ConstArgHasType` predicates)
 - #158739 (view-types: HIR lowering)
 - #158877 (borrowck: Keep returned `path` from `best_blame_constraint()` consistent)
 - #158883 (tests: fix enum-match.rs to handle LLVM 23)
 - #158886 (Add documentation for the `no_std` attribute)
 - #158940 (Implement feature `char_to_u32`)
 - #158951 (Merge three `MaxUniverse`s into one)
 - #158960 (Fix bootstrap submodule path prefix matching)
 - #158961 (Reapply "LLVM 23: Run AssignGUIDPass in some places")
 - #158995 (Use REST API in linkchecker script)
 - #158996 ([compiler] Implement `PartialOrd` via `Ord` for `Span` and newtype_indexes)
 - #159036 (bootstrap: expand '@argfile' arguments to rustc shim)
@rust-bors rust-bors Bot merged commit cf5bd99 into rust-lang:main Jul 10, 2026
13 checks passed
@rustbot rustbot added this to the 1.99.0 milestone Jul 10, 2026
rust-timer added a commit that referenced this pull request Jul 10, 2026
Rollup merge of #156027 - jackh726:opaque-liveness, r=lqd

Consider captured regions for opaque type region liveness.

Fixes #153215

For opaques, when we're calculating liveness for opaques, we want to consider any captured lifetimes that can outlive the opaque type, which is more than just the outlives bounds.

r? lqd
@JonathanBrouwer

Copy link
Copy Markdown
Contributor

@rust-timer build 9b082ed

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (9b082ed): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@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.3%, 0.4%] 2
Regressions ❌
(secondary)
0.1% [0.0%, 0.2%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.0% [-0.1%, -0.0%] 3
All ❌✅ (primary) 0.4% [0.3%, 0.4%] 2

Max RSS (memory usage)

Results (primary -1.3%, secondary -0.1%)

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)
4.0% [0.7%, 8.8%] 5
Improvements ✅
(primary)
-1.3% [-3.5%, -0.7%] 6
Improvements ✅
(secondary)
-3.6% [-7.8%, -1.2%] 6
All ❌✅ (primary) -1.3% [-3.5%, -0.7%] 6

Cycles

Results (secondary -0.5%)

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)
4.0% [2.0%, 9.1%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.8% [-9.4%, -1.8%] 7
All ❌✅ (primary) - - 0

Binary size

Results (primary 0.2%, secondary 0.2%)

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

mean range count
Regressions ❌
(primary)
0.2% [0.0%, 0.8%] 75
Regressions ❌
(secondary)
0.2% [0.0%, 0.8%] 47
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.0%, 0.8%] 75

Bootstrap: 490.768s -> 490.144s (-0.13%)
Artifact size: 389.74 MiB -> 389.16 MiB (-0.15%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 10, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Contributor

@rustbot label: +perf-regression-triaged
Noise

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 10, 2026
@jackh726 jackh726 deleted the opaque-liveness branch July 10, 2026 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

free region visitor for liveness marking regions dead and polonius alpha soundness

8 participants