fix multiple issues when promoting type-test subject #108691

aliemjay · 2023-03-03T05:13:16Z

Multiple interdependent fixes. See linked issues for a short description of each.

When Promoting a type-test T: 'a from within the closure back to its parent function, there are a couple pre-existing bugs and limitations. They were exposed by the recent changes to opaque types because the type-test subject (T) is no longer a simple ParamTy.

Commit 1:
Fixes #108635
Fixes #107426

Commit 2:
Fixes #108639

Commit 3:
Fixes #107516

rustbot · 2023-03-03T05:13:23Z

r? @Nilstrieb

(rustbot has picked a reviewer for you, use r? to override)

aliemjay · 2023-03-03T05:14:37Z

r? types

Needs some cleanup and updating comments.
@rustbot author
@rustbot label A-borrow-checker

Don't require a region to have an `external_name` in order to be promoted.

Smarter and simpler!

aliemjay · 2023-03-03T11:25:58Z

@rustbot review

jackh726 · 2023-03-03T13:05:40Z

Wow @aliemjay. Impressive.

jackh726 · 2023-03-04T21:21:04Z

Want to go ahead and queue a crater run since these kinds of changes are always senstive.

@bors try

bors · 2023-03-04T21:21:14Z

⌛ Trying commit 427dc18 with merge 303604bb848977091b2fdfc36579c25e56c6889c...

bors · 2023-03-05T00:05:17Z

☀️ Try build successful - checks-actions
Build commit: 303604bb848977091b2fdfc36579c25e56c6889c (303604bb848977091b2fdfc36579c25e56c6889c)

jackh726 · 2023-03-05T16:20:46Z

@craterbot check

craterbot · 2023-03-05T16:21:38Z

🚧 Experiment pr-108691 is now running

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

jackh726 · 2023-03-06T05:08:05Z

compiler/rustc_middle/src/mir/query.rs

+/// Represents a `ty::Ty` for use in [`ClosureOutlivesSubject`].
+///
+/// This indirection is necessary because the type may include `ReVar` regions,
+/// which is what we use internally within NLL code,
+/// and we can't use `ReVar`s in a query response.
+#[derive(Copy, Clone, Debug, TyEncodable, TyDecodable, HashStable)]
+pub struct ClosureOutlivesSubjectTy<'tcx> {
+    inner: Ty<'tcx>,
+}
+
+impl<'tcx> ClosureOutlivesSubjectTy<'tcx> {
+    // All regions of `ty` must be of kind `ReVar`
+    // and must point to an early-bound region in the closure's signature.
+    pub fn new(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Self {
+        let inner = tcx.fold_regions(ty, |r, depth| match r.kind() {
+            ty::ReVar(vid) => {
+                let br = ty::BoundRegion {
+                    var: ty::BoundVar::new(vid.index()),
+                    kind: ty::BrAnon(0u32, None),
+                };
+                tcx.mk_re_late_bound(depth, br)
+            }
+            _ => bug!("unexpected region in ClosureOutlivesSubjectTy: {r:?}"),
+        });
+
+        Self { inner }
+    }
+
+    pub fn instantiate(
+        self,
+        tcx: TyCtxt<'tcx>,
+        mut map: impl FnMut(ty::RegionVid) -> ty::Region<'tcx>,
+    ) -> Ty<'tcx> {
+        tcx.fold_regions(self.inner, |r, depth| match r.kind() {
+            ty::ReLateBound(debruijn, br) => {
+                debug_assert_eq!(debruijn, depth);
+                map(ty::RegionVid::new(br.var.index()))
+            }
+            _ => bug!("unexpected region {r:?}"),
+        })
+    }
+}
+


I don't really like this hack. Trying to think of a better alternative though.

I think it would be better to "formalize" this in some sense by having some way to have explicit "binders" of the captured regions, Or expanding on the concept of external_name a bit.

Is it a hard-and-fast rule that ty::Binder is the only binder for ReLateBound regions? I don't think so -- we already have Canonical<T> which serves as a binder too. The only limitation is that the type can't implement TypeFoldable/Visitable because these only support ty::Binder.

I tweaked it a bit in a later commit to make it more clear.

I think the concern for me is less about Binder, and more so that we aren't being explicit about the "bound variables" contained within (and what they mean to those that receive the "canonical" form).

What seems better to me is some more explicit "closure nameable" set of "bound vars" that this has access to. When borrow checking the context around the closure, the mapping between the "real" variables and the ones available to the closure is clear (maybe even trivial), but even if not in that context, it's still "just a set of bound vars".

I agree. This binder needs to store more context, but this is preexisting problem - the parent struct ClosureOutlivesRequirements is already littered by RegionVids with no context. Actually this is exactly the reason I haven't made this binder type more generic like RegionVarBinder<T> and made it explicit in the docs that it should be used in the context of ClosureOutlivesRequirements. The RegionVids there are already used to index into the early-bound regions of the closure type.

Yeah, definitely a preexisting problem that's just more pointy now. I think we can land, but we should chat about a followup for this (and if it's something you want to pursue or something we should offload to someone else).

I'm happy to work on a followup PR. Let me open an issue to track this..

I'm happy to work on a followup PR. Let me open an issue to track this..

did this happen?

oops I missed that. I've opened #111310.

craterbot · 2023-03-06T14:05:03Z

🎉 Experiment pr-108691 is completed!
📊 32 regressed and 4 fixed (257564 total)
📰 Open the full report.

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

jackh726 · 2023-03-07T00:05:19Z

@bors r+ rollup=never

bors · 2023-03-07T00:05:21Z

📌 Commit 427dc18 has been approved by jackh726

It is now in the queue for this repository.

jackh726 · 2023-03-07T00:09:59Z

Going to both stable and beta nominate. Unfortunately, we're going to miss the 1.67 stable to backport, but it's probably still worth getting this into 1.68.

bors · 2023-03-07T03:50:25Z

⌛ Testing commit 427dc18 with merge 8824994...

bors · 2023-03-07T06:49:31Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 8824994 to master...

rust-timer · 2023-03-07T08:06:48Z

Finished benchmarking commit (8824994): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

lcnr · 2023-03-07T07:58:00Z

compiler/rustc_middle/src/mir/query.rs

+            ty::ReVar(vid) => {
+                let br = ty::BoundRegion {
+                    var: ty::BoundVar::new(vid.index()),
+                    kind: ty::BrAnon(0u32, None),


doesn't matter too much, but this should be vid.index() instead of 0, shouldn't it?

yeah I changed it in 97381d2.

apiraino · 2023-03-09T15:21:21Z

Beta backport accepted as per compiler team on Zulip

Stable backport declined.

@rustbot label +beta-accepted -stable-nominated

…k-Simulacrum [beta] backport This PR backports: - rust-lang#108901: fix: evaluate with wrong obligation stack - rust-lang#108754: Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous - rust-lang#108691: fix multiple issues when promoting type-test subject It also bumps to the released stable. r? `@Mark-Simulacrum`

rustbot assigned Noratrieb Mar 3, 2023

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 Mar 3, 2023

rustbot assigned lcnr and unassigned Noratrieb Mar 3, 2023

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-borrow-checker Area: The borrow checker and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2023

This was referenced Mar 3, 2023

1.67 regression with ……::{opaque#0}<'_> does not live long enough error #107516

Closed

Regression 1.66 "some_fn::{opaque#0}<'_> does not live long enough" #107426

Closed

aliemjay added 4 commits March 3, 2023 11:10

add known-bug tests

20b20b2

promote subject even if it has unnamed regions

09524bf

Don't require a region to have an `external_name` in order to be promoted.

smarter algorithm for finding an equal region

9d74bff

Smarter and simpler!

ignore bivariant regions in opaque types

10da771

aliemjay force-pushed the closure-subject branch from 74d14a3 to 10da771 Compare March 3, 2023 11:13

aliemjay marked this pull request as ready for review March 3, 2023 11:25

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 3, 2023

This comment has been minimized.

Sign in to view

aliemjay added 2 commits March 4, 2023 11:19

tweak ClosureOutlivesSubjectTy

97381d2

tweak debug output and bless tests

427dc18

jackh726 reviewed Mar 6, 2023

View reviewed changes

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 6, 2023

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 Mar 7, 2023

jackh726 added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Mar 7, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 7, 2023

bors merged commit 8824994 into rust-lang:master Mar 7, 2023

rustbot added this to the 1.70.0 milestone Mar 7, 2023

lcnr reviewed Mar 7, 2023

View reviewed changes

aliemjay deleted the closure-subject branch March 7, 2023 08:37

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 9, 2023

apiraino removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Mar 9, 2023

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 11, 2023

Mark-Simulacrum modified the milestones: 1.70.0, 1.69.0 Mar 11, 2023

Mark-Simulacrum mentioned this pull request Mar 11, 2023

[beta] backport #109033

Merged

aliemjay mentioned this pull request May 7, 2023

don't use region variables in borrowck query response #111310

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix multiple issues when promoting type-test subject #108691

fix multiple issues when promoting type-test subject #108691

aliemjay commented Mar 3, 2023 •

edited

Loading

rustbot commented Mar 3, 2023

aliemjay commented Mar 3, 2023 •

edited

Loading

aliemjay commented Mar 3, 2023

jackh726 commented Mar 3, 2023

This comment has been minimized.

jackh726 commented Mar 4, 2023

bors commented Mar 4, 2023

bors commented Mar 5, 2023

jackh726 commented Mar 5, 2023

craterbot commented Mar 5, 2023

jackh726 Mar 6, 2023

aliemjay Mar 6, 2023

jackh726 Mar 6, 2023

aliemjay Mar 6, 2023

jackh726 Mar 7, 2023

aliemjay Mar 7, 2023

lcnr May 3, 2023

aliemjay May 7, 2023

craterbot commented Mar 6, 2023

jackh726 commented Mar 7, 2023

bors commented Mar 7, 2023

jackh726 commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

rust-timer commented Mar 7, 2023

lcnr Mar 7, 2023

aliemjay Mar 7, 2023

apiraino commented Mar 9, 2023 •

edited

Loading

fix multiple issues when promoting type-test subject #108691

fix multiple issues when promoting type-test subject #108691

Conversation

aliemjay commented Mar 3, 2023 • edited Loading

rustbot commented Mar 3, 2023

aliemjay commented Mar 3, 2023 • edited Loading

aliemjay commented Mar 3, 2023

jackh726 commented Mar 3, 2023

This comment has been minimized.

jackh726 commented Mar 4, 2023

bors commented Mar 4, 2023

bors commented Mar 5, 2023

jackh726 commented Mar 5, 2023

craterbot commented Mar 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craterbot commented Mar 6, 2023

jackh726 commented Mar 7, 2023

bors commented Mar 7, 2023

jackh726 commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

rust-timer commented Mar 7, 2023

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apiraino commented Mar 9, 2023 • edited Loading

aliemjay commented Mar 3, 2023 •

edited

Loading

aliemjay commented Mar 3, 2023 •

edited

Loading

apiraino commented Mar 9, 2023 •

edited

Loading