Fix subst issues with return-position impl Trait in trait

[compiler-errors]

1. Fix an issue where we were rebase impl substs onto trait method substs, instead of trait substs
2. Fix an issue where early-bound regions aren't being mapped correctly for RPITIT hidden types

Fixes #102301
Fixes #102310
Fixes #102334
Fixes #102918

[rust-highfive]

r? @estebank

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

[bors]

☔ The latest upstream changes (presumably #102306) made this pull request unmergeable. Please resolve the merge conflicts.

[cjgillot]

Generics::param_def_id_to_index should give the correct answer. Is e.def_id the wrong definition to refer to?

[compiler-errors]

Yes, technically e.def_id points to the RPIT lifetime on the trait. There is no corresponding RPIT generics for the impl.

[cjgillot]

I see 3 cases:

• there exists some early-bound lifetime on the impl that we could map to;
• the corresponding lifetime in the impl is late-bound, and we should build the correct binder;
• there is no lifetime because there is no unification, and we should probably return re_static.

[compiler-errors]

@cjgillot:

I'm not really sure what you mean by this. The inferred opaque type shouldn't have any late-bound regions -- associated types don't have late-bound regions, do they?

I guess an example might help explain my thinking in this change -- let's look at a manual desugaring of an RPITIT:

RPITIT:

trait Foo {
  fn foo<'a>(&'a self) -> impl Display + 'a;
}

struct Wrapper<T>(T);

impl<T: Display> Foo for Wrapper<T> {
  fn foo<'a>(&'a self) -> impl Display + 'a {
    &self.0
  }
}

GAT:

trait Foo {
  type FooRpit<'a>: Display + 'a;

  fn foo<'a>(&'a self) -> &'a T;
}

struct Wrapper<T>(T);

impl Foo for Wrapper<i32> {
  type FooRpit<'a> = &'a i32;

  fn foo<'a>(&'a self) -> Self::FooRpit<'a> {
    &self.0
  }
}

In the GAT example, when we're projecting <Wrapper<i32> as Foo>::FooRpit<'_#0r> (substs = [Wrapper<i32>, '_#0r]), we first infer that the {impl#0} provides the asociated type. That impl itself is inferred to have the substs [], and the {impl#0}::FooRpit unsubstituted type value is &'a/0 i32, where 'a/0 is an early-bound region with index 0 in the substs list. We rebase the substs above onto the impl substs to get ['_#0r], substitute to get &'_#0r i32.

However, in the RPITIT example {impl#0}::FooRpit isn't actually a real item, so during the RPIT inference step, we need to "renumber" the opaque type's early-bound lifetimes (taken from the opaque item in the trait) counting up from the impl's own substs as if a corresponding associated item existed, so we can later substitute things correctly during projection. Since the impl has no substs of its own, the early bound regions of FooRpit start counting from zero, not one.

If we don't do this, then the type_of value of the corresponding RPITIT to {impl#0}::FooRpit would be &'a/1 i32 (since 'a/1 comes from the trait, which has a Self subst in the 0th position), which would be out of bounds considering the rebased impl substs I mentioned above.

the corresponding lifetime in the impl is late-bound, and we should build the correct binder

If we wrapped things in binders instead, then I think we'd be treating the function foo in the impl like for<'a> fn(&'a self) -> for<'b> &'b i32 even though the lifetime of &self and &i32 in that above example should be related through substs.

[cjgillot]

Thank you for the detailed explanation. This is definitely subtle.

Do you mind adding a comment in the code to explain that we are working around the non-existence of {impl#0}::FooRpit?

You shift the subst index by the length difference between the trait generic length and the impl generic length. I would have expected the difference between the trait method generic length and the impl method generic length, since the opaques are based on them in generics_of. We should perhaps explain that this is ok since we check the number of generics before reaching this point.

r=me with that comment

[bors]

☔ The latest upstream changes (presumably #101632) made this pull request unmergeable. Please resolve the merge conflicts.

[estebank]

r=me after rebasing

[cjgillot]

While trying to understand what happens, I remarked that this scheme leaks inference variables.
For instance, in src/test/ui/async-await/in-trait/issue-102310.rs, you get ty=Opaque(DefId(0:14 ~ issue_102310[ed7a]::{impl#0}::transaction::{opaque#0}), [F, R, '_#0r]).

[compiler-errors]

Fixed by equating the full signature instead of just equating the return types, which allows us to infer things fully.

[compiler-errors]

Also #102903 asserts against this more strongly, since fully_resolve really shouldn't be returning inference variables..

[compiler-errors]

@estebank this could use a quick review of the final commit, since I added that to fix the issue that @cjgillot mentioned above about leaking inference variables.

[compiler-errors]

@bors r=cjgillot

[bors]

📌 Commit 4259f33 has been approved by cjgillot

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4259f33 with merge 8be3ce9...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 8be3ce9 to master...

[rust-timer]

Finished benchmarking commit (8be3ce9): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was 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)	-0.5%	[-0.5%, -0.5%]	1
All ❌✅ (primary)	-	-	0

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)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	3.0%	[2.0%, 3.8%]	20
Improvements ✅ (primary)	-3.8%	[-3.8%, -3.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-3.8%, 2.9%]	2

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.

	mean1	range	count2
Regressions ❌ (primary)	2.2%	[1.6%, 2.6%]	3
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	2.2%	[1.6%, 2.6%]	3

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³