Fix ambiguous cases of multiple & in elided self lifetimes

[adetaylor]

This change proposes simpler rules to identify the lifetime on self parameters which may be used to elide a return type lifetime.

The old rules

(copied from this comment)

Most of the code can be found in late.rs and acts on AST types. The function resolve_fn_params, in the success case, returns a single lifetime which can be used to elide the lifetime of return types.

Here's how:

• If the first parameter is called self then we search that parameter using "self search rules", below
• If no unique applicable lifetime was found, search all other parameters using "regular parameter search rules", below

(In practice the code does extra work to assemble good diagnostic information, so it's not quite laid out like the above.)

self search rules

This is primarily handled in find_lifetime_for_self , and is described slightly here already. The code:

1. Recursively walks the type of the self parameter (there's some complexity about resolving various special cases, but it's essentially just walking the type as far as I can see)
2. Each time we find a reference anywhere in the type, if the direct referent is Self (either spelled Self or by some alias resolution which I don't fully understand), then we'll add that to a set of candidate lifetimes
3. If there's exactly one such unique lifetime candidate found, we return this lifetime.

Regular parameter search rules

1. Find all the lifetimes in each parameter, including implicit, explicit etc.
2. If there's exactly one parameter containing lifetimes, and if that parameter contains exactly one (unique) lifetime, and if we didn't find a self lifetime parameter already, we'll return this lifetime.

The new rules

There are no changes to the "regular parameter search rules" or to the overall flow, only to the self search rules which are now:

1. Recursively walks the type of the self parameter, searching for lifetimes of reference types whose referent contains Self.¹
2. Keep a record of:
   • Whether 0, 1 or n unique lifetimes are found on references encountered during the walk
3. If no lifetime was found, we don't return a lifetime. (This means other parameters' lifetimes may be used for return type lifetime elision).
4. If there's one lifetime found, we return the lifetime.
5. If multiple lifetimes were found, we abort elision entirely (other parameters' lifetimes won't be used).

Examples that were accepted before and will now be rejected

fn a(self: &Box<&Self>) -> &u32
fn b(self: &Pin<&mut Self>) -> &String
fn c(self: &mut &Self) -> Option<&Self>
fn d(self: &mut &Box<Self>, arg: &usize) -> &usize // previously used the lt from arg

Examples that change the elided lifetime

fn e(self: &mut Box<Self>, arg: &usize) -> &usize
//         ^ new                ^ previous

Examples that were rejected before and will now be accepted

fn f(self: &Box<Self>) -> &u32

---

edit: old PR description:

  struct Concrete(u32);

  impl Concrete {
      fn m(self: &Box<Self>) -> &u32 {
          &self.0
      }
  }

resulted in a confusing error.

  impl Concrete {
      fn n(self: &Box<&Self>) -> &u32 {
          &self.0
      }
  }

resulted in no error or warning, despite apparent ambiguity over the elided lifetime.

Fixes #117715

Footnotes

1. this prevents us from considering lifetimes from inside of the self-type ↩

[rustbot]

r? @cjgillot

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

[adetaylor]

@cjgillot hello, I didn't know rustbot would assign a reviewer for a draft PR. This isn't yet ready.

[clubby789]

hello, I didn't know rustbot would assign a reviewer for a draft PR. This isn't yet ready.

Btw, you can use r<question mark> ghost in your initial mesage to prevent a reviewer being assigned

[adetaylor]

I've now marked this as ready for review. Per discussion on #117715, this will need a crater run.

It's possible that we want to make the tests a bit more thorough before merging, but it would be good to initially get feedback on the approach (and maybe a crater run to find out if it's doomed)

[lcnr]

let's do a crater run. I am still unsure about the exact behavior we want and probably have to look into this myself to figure it out 🤔 but for now

@bors try

[bors]

⌛ Trying commit 15ce755 with merge 57fd743...

[bors]

☀️ Try build successful - checks-actions
Build commit: 57fd743 (57fd74335bb14b59b719dc3cc5bc4257e73f33ce)

[bors]

☀️ Try build successful - checks-actions
Build commit: 57fd743 (57fd74335bb14b59b719dc3cc5bc4257e73f33ce)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-117967 created and queued.
🤖 Automatically detected try build 57fd743
🔍 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]

🚧 Experiment pr-117967 is now running

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

[craterbot]

🎉 Experiment pr-117967 is completed!
📊 51 regressed and 5 fixed (398975 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

[cjgillot]

r? @lcnr

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[traviscross]

@lcnr: Heads-up that FCP completed on this, so this is good to go forward as far as lang is concerned.

[lcnr]

missed that this finished FCP already. Thanks for the good work @adetaylor and thanks for sticking with it for this long!

@bors r+ rollup=never

[lcnr]

@bors r+ rollup=never

[bors]

📌 Commit a22130e has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit a22130e with merge 5753b30...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 5753b30 to master...

[rust-timer]

Finished benchmarking commit (5753b30): 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.

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

Max RSS (memory usage)

Results (primary 1.8%)

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

Cycles

Results (secondary -2.5%)

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

Binary size

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

Bootstrap: 767.488s -> 768.843s (0.18%)
Artifact size: 328.72 MiB -> 328.71 MiB (-0.00%)