add cycle detection for inferred outlives predicates

[cormacrelf]

This avoids a compiler hang. Fixes #102966. The solution is documented in the code, it's basically just a visit stack for the inferred predicate propagation.

@rustbot label +A-inference +A-lifetimes +T-compiler +I-hang

[rust-highfive]

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

[wesleywiser]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 619f916 with merge a08b0ef4b0e3a4d788929838a64a7d7a91dade4a...

[bors]

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

[rust-timer]

Queued a08b0ef4b0e3a4d788929838a64a7d7a91dade4a with parent 1481fd9, future comparison URL.

[rust-timer]

Finished benchmarking commit (a08b0ef4b0e3a4d788929838a64a7d7a91dade4a): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

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

Max RSS (memory usage)

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-	-	0

Cycles

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

Footnotes

1. the arithmetic mean of the percent change ↩

2. number of relevant changes ↩

[wesleywiser]

This looks good to me but I'd like a second pair of eyes from

r? types

[cormacrelf]

This is the main thing I would like some eyes on. I think it's right for Var to get a substituted-through version of its own explicit bound here, because Var doesn't know what RGen<R>::Assoc is, but whatever it is, Node does need it to be : 'node. It seems the cautious approach to add the bound if we're not going to find out what the projection resolves to.

But I don't know and don't see much guidance in the RFC or its tests, as this situation wasn't really contemplated. Can anyone think of a test that might illuminate the correctness of this?

Tangentially, this PR has more tracking of the source of inferred outlives than before. Are there error messages we can improve with this information? Like the example of a "long range error" message in the original RFC.

[compiler-errors]

Sorry, I don't really have much time to review this -- apologies for letting this sit.

r? types

[cormacrelf]

It's alright, I'm not blocked on this; the code I was working on from didn't end up benefiting from allowing a Node to have a lifetime, so I removed them. Nevertheless needs fixing at some point.

[oli-obk]

I don't think we should allow this code in the way you did it. This is probably unsound in case <RGen<R> as Trait<'var>>::Assoc does not resolve to R. I can check out your branch and play with a few examples to try to come up with a real bug case, but I'd prefer to error in case of a cycle and try to resolve cycles by normalizing opportunistically

[cormacrelf]

I'm happy to PR an error bailout if you want. Erroring is the conservative approach in that it does not make anything compile that previously didn't, before we're sure that's ok.

I think you'll find it's sound, though. As I understand it, inferred outlives bounds are a convenience, that relieves you from writing the bounds yourself. The only things the cycle detector can do wrong are:

• Kick in "too early", i.e. not adding enough bounds. Basically, if you fail to add the necessary bounds, rustc is still checking your structs internally for whether the bounds of the types you use are met. The Rust reference type &'a T has a bound on it relating the referent type and the lifetime. You have to prove this any time you use it, and adding a T: 'a bound to struct A<'a, T>(&'a T) is just kicking this down the road to whoever uses A. Before RFC2093, the kicking of that can down the road was not automatic, and so writing that struct definition would result in the &'a T type complaining its bounds weren't met, and you having to writing some manual bounds. That's the worst case scenario.
• Kick in "too late", i.e. add too many bounds. This is definitionally sound. You can never produce unsoundness by adding more constraints on the lifetime of a type parameter. Maybe the resulting type is less versatile than it could be, but it won't be unsound.

For posterity, I did think about the whole "what if RGen<R>::Assoc is not R" thing, and it is a red herring. The equivalence is not how the cycle was formed: you can change it to

trait Trait<'a> { type Assoc; /* no bound */ }
impl<'a, R> Trait<'a> for RGen<R> { type Assoc = String }

... and there is still a cycle and rustc hangs, because the cycle comes from the repeated substitution of Var's R: 'var through projections that create more projections when they go around again. It is true that this case involved producing more names for the same type, but really the problem for the naive propagation algorithm was that it's producing more names, since the cycle still exists when RGen<R>::Assoc is just String. The red herring was tempting because the original algorithm only used type name equality to stop cycles forming, and the obvious thing to try was "use actual type equality instead". I think that is still technically a solution that prevents cycles, since the second time round (and beyond) propagating the associated types will be the same type (e.g. String) again and again. I just found a better solution that doesn't require unifying types and kicks in earlier in the type Assoc = String case.

The ACTUAL solution to this instance of the problem, i.e. "what outlives bounds do we need to add to satisfy the types inside Node and Var", is just "Node gets T::Assoc: 'node". Don't trust me, trust a pre-RFC2093 compiler (1.12.1):

error[E0309]: the associated type `<T as issue::Trait<'node>>::Assoc` may not live long enough
 --> src/issue.rs:5:5
  |
5 |     var: Var<'node, T::Assoc>,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: consider adding an explicit lifetime bound `<T as issue::Trait<'node>>::Assoc: 'node`...
note: ...so that the type `<T as issue::Trait<'node>>::Assoc` will meet its required lifetime bounds
 --> src/issue.rs:5:5
  |
5 |     var: Var<'node, T::Assoc>,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^

When you add that bound, rustc-1.12.1 is happy, because when it checks the fields of Var for their bounds being met, it does in fact look at whether the bound on Node is satisfied, and therefore considers what RGen<R>::Assoc might be.

Since only one additional bound was necessary to infer here, my solution exhibits the problem of "kicking in too late". Perhaps I should revisit the choice not to add the site of the original explicit bound as part of the provenance.

[cormacrelf]

Ohhhh, this is how it can be unsound: #84305. I get you now. I still think this will turn out sound but yeah, I will PR a simple recursion limit first.

[oli-obk]

Suggested change

	let mut round = 1;
	'outer: loop {
	'outer: for round in 1.. {

[oli-obk]

Just for sanity, maybe check the recursion limit here against round (I think that could be a valid PR on its own that we could crater and land to ensure the currently indefinite-loop actually finishes

[oli-obk]

@rustbot author

[lcnr]

waiting and author and doesn't seem to need t-types input rn

https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2023-01-18.20meeting/near/322081814

[JohnCSimon]

@cormacrelf

ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

[Dylan-DPC]

@cormacrelf any updates on this?

[cormacrelf]

Hi, sorry I haven't been working on this. I started to rework this into a more conservative "error with recursion limit exceeded" as requested, but I didn't finish it as it turned out to be a pretty complicated dance to get a new kind of compiler message going. There is just a lot of error messaging API and it is very decoupled.

I haven't been pressed to finish it because the library I wanted this kind of lifetime arrangement for didn't end up needing it: https://github.com/cormacrelf/incremental-rs. But I will get there. I think I have a few commits that are halfway there that I can put up at least.

[JohnCSimon]

@cormacrelf
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive