Skip to content

Better closure requirement propagation V2 #154271

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.

Sign up for GitHub

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

Open
LorrensP-2158466 wants to merge 1 commit into
base: main
Choose a base branch
from
+52 −1
Open

Better closure requirement propagation V2 #154271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion compiler/rustc_borrowck/src/region_infer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,9 +713,35 @@ impl<'tcx> RegionInferenceContext<'tcx> {
for ur in self.scc_values.universal_regions_outlived_by(r_scc) {
found_outlived_universal_region = true;
debug!("universal_region_outlived_by ur={:?}", ur);
let non_local_ub = self.universal_region_relations.non_local_upper_bounds(ur);
let mut non_local_ub = self.universal_region_relations.non_local_upper_bounds(ur);
debug!(?non_local_ub);

// We don't need to propagate every `T: 'ub` for soundness:
// Say we have the following outlives constraints given (`'b: 'a` == `a -> b`):
//
// a
// \
// -> c
// /
// b
Copy link
Copy Markdown
Contributor

@lcnr lcnr Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

that's still odd and the test feels confusing/easier to fix than the one I wrote 🤔

In your test we have

'a: 'c
'b: 'c
T: 'a

We should clearly not propagate T: 'c and 'c: 'a. It's weird that we look at at universal_regions_outlived_by at all. Why are we not directly looking at the non_local_upper_bounds of lower_bound?

At least only look at the smallest such universal region

Copy link
Copy Markdown
Contributor Author

@LorrensP-2158466 LorrensP-2158466 Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

We should clearly not propagate T: 'c and 'c: 'a

And that doesn't happen, I don't know what you mean with this.

It's weird that we look at at universal_regions_outlived_by at all. Why are we not directly looking at the non_local_upper_bounds of lower_bound

The function comment:

    /// Once we've promoted T, we have to "promote" `'X` to some region
    /// that is "external" to the closure. Generally speaking, a region
    /// may be the union of some points in the closure body as well as
    /// various free lifetimes. We can ignore the points in the closure
    /// body: if the type T can be expressed in terms of external regions,
    /// we know it outlives the points in the closure body. That
    /// just leaves the free regions.

Otherwise I have no clue, dev-guide didn't give a reason. If I try to do non_local_upper_bounds of lower_bound the compiler panics:

thread 'rustc' (6493769) panicked at compiler/rustc_borrowck/src/type_check/free_region_relations.rs:113:9:
assertion failed: self.universal_regions.is_universal_region(fr0)

At least only look at the smallest such universal region

How is this possible?

Copy link
Copy Markdown
Contributor

@lcnr lcnr Mar 30, 2026

Choose a reason for hiding this comment

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

We should clearly not propagate T: 'c and 'c: 'a

on stable we propagate T: 'c as we separately require/know 'c: 'a, do we not? I guess we don't propagate 'c: 'a

Copy link
Copy Markdown
Contributor

@lcnr lcnr Mar 30, 2026

Choose a reason for hiding this comment

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

If I try to do non_local_upper_bounds of lower_bound the compiler panics:

thread 'rustc' (6493769) panicked at compiler/rustc_borrowck/src/type_check/free_region_relations.rs:113:9:
assertion failed: self.universal_regions.is_universal_region(fr0)

At least only look at the smallest such universal region

How is this possible?

I guess we currently don't have a function to just do that right away, unsure.

We could call universal_regions_outlived_by and then manually filter the universals to ones which are not outlived by any other of the returned regions.

There's ReverseSccGraph.upper_bounds which seems useful?

How is this possible?

I guess the core answer here is

  • we have a way to get all universal regions which are required universal_regions_outlived_by
  • and then have a way to test whether universal_regions outlive each other

so this is definitely possible, worst case we have to reimpl it

Copy link
Copy Markdown
Contributor Author

@LorrensP-2158466 LorrensP-2158466 Apr 3, 2026

Choose a reason for hiding this comment

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

on stable we propagate T: 'c as we separately require/know 'c: 'a, do we not? I guess we don't propagate 'c: 'a

I have feeling that you are mixing up 'a: 'c with 'c: 'a, because I don't see why the compiler might do this. [In this example] Clearly we should never propogate the latter.
Propagating T: 'c is wrong, and does currently happen on nightly/stable.

Also, I tried your suggestion, though I couldn't get access to ReverseSccGraph.upper_bounds, so i did this instead:

debug(?universal_regions);
let minimal_universal_regions: Vec<_> = universal_regions
    .iter()
    .copied()
    .filter(|&a| {
        !universal_regions
            .iter()
            .copied()
            .any(|b| b != a && self.universal_region_relations.outlives(b, a))
    })
    .collect();
debug(?minimal_universal_regions);
let universal_regions = if minimal_universal_regions.is_empty() {
    universal_regions
} else {
    minimal_universal_regions
};
debug(?universal_regions);

It fixes the test of this PR, but your "harder" test still miscompiles. Because when doing the type test T:'a, this happens:

0ms DEBUG rustc_borrowck::region_infer universal_regions=['?2, '?5]
0ms DEBUG rustc_borrowck::region_infer minimal_universal_regions=['?2]
0ms DEBUG rustc_borrowck::region_infer universal_regions=['?2]
0ms DEBUG rustc_borrowck::region_infer universal_region_outlived_by ur='?2
0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_upper_bound(fr='?2)
0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_bound: external_parents=['?2]
0ms DEBUG rustc_borrowck::region_infer non_local_ub=['?2]
0ms DEBUG rustc_borrowck::region_infer adding closure requirement, requirement=ClosureOutlivesRequirement { subject: Ty(ClosureOutlivesSubjectTy { inner: T/#1 }), outlived_free_region: '?2, blame_span: lcnr_test.rs:19:38: 19:53 (#0), category: Boring }

?2 = 'a, and ?5 = 'c.
As you can see, we propagate: T: 'a.

And for T: 'c, this happens:

0ms DEBUG rustc_borrowck::region_infer universal_regions=['?5]
 0ms DEBUG rustc_borrowck::region_infer minimal_universal_regions=['?5]
 0ms DEBUG rustc_borrowck::region_infer universal_regions=['?5]
 0ms DEBUG rustc_borrowck::region_infer universal_region_outlived_by ur='?5
 0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_upper_bound(fr='?5)
 0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_bound: external_parents=['?3, '?2]
 0ms DEBUG rustc_borrowck::region_infer non_local_ub=['?3, '?2]
 0ms DEBUG rustc_borrowck::region_infer adding closure requirement, requirement=ClosureOutlivesRequirement { subject: Ty(ClosureOutlivesSubjectTy { inner: T/#1 }), outlived_free_region: '?3, blame_span: lcnr_test.rs:19:38: 19:53 (#0), category: Boring }
 0ms DEBUG rustc_borrowck::region_infer adding closure requirement, requirement=ClosureOutlivesRequirement { subject: Ty(ClosureOutlivesSubjectTy { inner: T/#1 }), outlived_free_region: '?2, blame_span: lcnr_test.rs:19:38: 19:53 (#0), category: Boring }

?2 = 'a, ?3 = 'b and?5 = 'c.

So it's still thinking that to prove T: 'c, we need both T: 'a and T: 'c to hold true, because the type test is looked at in "isolation". That's why I asked in zulip that maybe we need "OR" constraints here.

Copy link
Copy Markdown
Contributor Author

@LorrensP-2158466 LorrensP-2158466 Apr 3, 2026

Choose a reason for hiding this comment

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

I do still have a feeling that I am not completely understanding what you think is the correct fix.
How I understand the current issue: When type testing T: 'c, we don't actually know that we are already (or will) require T': a, so the only way to prove that T: 'c is that T should outlive either 'a or 'b, right?

//
// And we are doing the type test `T: 'a`.
//
// The `lower_bound_universal_regions` of `'a` are `['a, 'c]`. The `upper_bounds` of `'a`
// is `['a]`, so we propagate `T: 'a`.
// The `upper_bounds` of `'c` are `['a, 'b]`, propagating `T: 'a` is correct;
// `T: 'b` is redundant because it provides no value to proving `T: 'a`.
//
// So we filter the set of upper_bounds to regions already outliving `lower_bound`,
// but if this subset is empty, we fall back to the original one.
let subset: Vec<_> = non_local_ub
.iter()
.copied()
.filter(|ub| self.eval_outlives(*ub, lower_bound))
.collect();
non_local_ub = if subset.is_empty() { non_local_ub } else { subset };
debug!(?non_local_ub, "upper_bounds after filtering");

// This is slightly too conservative. To show T: '1, given `'2: '1`
// and `'3: '1` we only need to prove that T: '2 *or* T: '3, but to
// avoid potential non-determinism we approximate this by requiring
Expand Down
25 changes: 25 additions & 0 deletions tests/ui/nll/closure-requirements/type-test-issue-154267.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//@ check-pass
// This test checks that the compiler doesn't propagate `T: 'b` during the `T: 'a` type test.
// If it did, it would fail to compile, even though the program is sound.

struct Arg<'a: 'c, 'b: 'c, 'c, T> {
field: *mut (&'a (), &'b (), &'c (), T),
}

impl<'a, 'b, 'c, T> Arg<'a, 'b, 'c, T> {
fn constrain(self)
where
T: 'a,
{
}
}

fn takes_closure<'a, 'b, T>(_: impl for<'c> FnOnce(Arg<'a, 'b, 'c, T>)) {}

// We have `'a: 'c` and `'b: 'c`, requiring `T: 'a` in `constrain` should not need
// `T: 'b` here.
fn error<'a, 'b, T: 'a>() {
takes_closure::<'a, 'b, T>(|arg| arg.constrain());
}

fn main() {}
Loading