Skip to content
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.

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

self: &Box<Self> produces confusing error due to failure to spot elided lifetime #117715

Closed
adetaylor opened this issue Nov 8, 2023 · 11 comments · Fixed by #117967
Closed

self: &Box<Self> produces confusing error due to failure to spot elided lifetime #117715

adetaylor opened this issue Nov 8, 2023 · 11 comments · Fixed by #117967
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@adetaylor
Copy link
Contributor

Code

struct Concrete(u32);

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

Current output

error[E0106]: missing lifetime specifier
 --> src/lib.rs:4:43
  |
4 |     fn m(self: &std::boxed::Box<Self>) -> &u32 {
  |                ----------------------     ^ expected named lifetime parameter
  |
  = help: this function's return type contains a borrowed value, but the signature does not say which `self` it is borrowed from
help: consider introducing a named lifetime parameter
  |
4 |     fn m<'a>(self: &'a std::boxed::Box<Self>) -> &'a u32 {
  |         ++++        ++                            ++

For more information about this error, try `rustc --explain E0106`.
error: could not compile `playground` (lib) due to previous error

Desired output

Ideally, rustc recognizes the elided lifetime in the `self` parameter and uses that.

Alternatively, "the signature does not say which `self` it is borrowed from" is reworded to more accurately describe the error.

Rationale and extra context

No response

Other cases

No response

Anything else?

fn n(self: &std::boxed::Box<Self>, _other: &u32) -> &u32 {
    &self.0
}

results in an error where rustc assumes we're returning the _other parameter, which presumably results from the same failure to spot a lifetime attached to self.

This issue was identified during discussion about a new Rust RFC, here.

I suspect this is caused by something within find_lifetime_for_self which isn't aware of the need to support things like Box as a self type: I haven't spotted the bug yet, I thought it was more important to get it reported properly first.

@adetaylor adetaylor added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 8, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 8, 2023
@saethlin saethlin added A-lifetimes Area: Lifetimes / regions D-confusing Diagnostics: Confusing error or lint that should be reworked. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 9, 2023
@RalfJung
Copy link
Member

RalfJung commented Nov 9, 2023

Yeah that definitely seems wrong.

Cc @rust-lang/types ? not sure who's in charge of lifetime elision

@adetaylor
Copy link
Contributor Author

Some analysis/proposals.

How find_lifetime_for_self currently works

Visits the type.
Recurses inwards, finding all references.
For each reference, if the referent is a "self type", consider the lifetime of that reference. ("Self type" means implicit self or a type which resolves to Self (specifically, either Res::SelfTyParam or Res::SelfTyAlias)
If exactly one such lifetime is found, that's the one which gets used.

Why this isn't good enough

This doesn't work for &Box<Self> because the referent is not the Self type itself.

What we could do

Options:

  1. Simply retrieve all the lifetimes from any type which contains Self anywhere. Since we only elide lifetimes if there's exactly one, there's no risk of ambiguity here - we'll simply require folks to specify a lifetime in this case. However this would be a backwards incompatible change. This currently works: self: &Box<&Self>. That feels bad, but it's a fact. We would no longer elide those lifetimes.
  2. Change the definition of "is self type" to recurse further into the type, to confirm that (a) there's Self somewhere in there, and (b) there's no direct reference to self in there.

What I think we should do

I think that depends on the Rust community's attitude to backwards compatibility. Should we maintain compatibility and continue to elide lifetimes on &Boxed<&Self>? If so, we do option 2. If we're OK breaking compatibility there, we should do option 1.

However, first of all I'll try option 1 and run rustc's unit tests to see if the above analysis is right.

@RalfJung
Copy link
Member

RalfJung commented Nov 15, 2023

Simply retrieve all the lifetimes from any type which contains Self anywhere.

That makes no sense to me. Elision treats the self argument special, not the Self type. This confuses me multiple times in your comment.

@adetaylor
Copy link
Contributor Author

Elision treats the self argument special, not the Self type.

All the code I'm talking about is only ever applied to the self parameter.

@RalfJung
Copy link
Member

RalfJung commented Nov 15, 2023

Oh I see, thanks for clarifying. I would say we should check the entire type for lifetimes. Self does not matter at all. That entire type is the receiver type. We should not have accepted fn(self: &Box<&Self>) -> &u32.

I am still confused, though. We currently entirely ignore the outer lifetime of the argument. In fn n(self: &Box<Self>, other: &u32) -> &u32, the return type gets the second argument's lifetime. Somehow the traversal seems to skip the things surrounding Self entirely not just in order to resolve an ambiguity of the return type lifetime, but even when determining whether there is such an ambiguity in the first place?

So, this must be a breaking change anyway since fn n(self: &Box<Self>, other: &u32) -> &u32 should either be rejected or use the self lifetime, but definitely should not use the other lifetime. That's just a clear spec violation.

@adetaylor
Copy link
Contributor Author

In the case of &Box<&Self>, it's ignoring the outer & because it currently only considers lifetimes which are directly applied to the Self itself, i.e. the inner lifetime.

I'll have a crack at option 1 and see what breaks in the unit tests. I'm sure I'll learn something!

adetaylor added a commit to adetaylor/rust that referenced this issue Nov 16, 2023
  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 rust-lang#117715
@adetaylor
Copy link
Contributor Author

adetaylor commented Nov 16, 2023

The sort of fix I think we need is something like this, however, it's not quite right yet and lots of the existing elision tests fail. Parking it here because I'm unlikely to get time to work on it for a few days at least.

@adetaylor
Copy link
Contributor Author

The simple option doesn't pass tests here due to a test called ref-assoc.rs which contains:

trait Trait {
    type AssocType;
}

struct Struct { }

impl Trait for Struct {
    type AssocType = Self;
}

impl Struct {
    fn ref_AssocType(self: &<Struct as Trait>::AssocType, f: &u32) -> &u32 {
        f
    }
}

(there are some other similar tests too)

Previously, this worked because &<Struct as Trait>::AssocType did not have the & directly next to Self, so self was not considered a valid self for elision purposes.

If we search the whole type hierarchy for Self and search the whole hierarchy for & then we'll suddenly consider self valid for elision purposes, and this test will fail.

So the question is, can we be sure that the Self here in this associated type really is the Self of the method receiver? I think we probably can, and therefore we should alter this test to fit the desired behavior.

There are also some FIXMEs hanging around here, such as this one.

(Again all this code only applies to the first parameter).

@RalfJung
Copy link
Member

RalfJung commented Nov 21, 2023

IMO that test is clearly buggy, the lifetime of the return value is ambiguous. It should either fail because the lifetime is ambiguous or fail because it infers the self lifetime for the return type and then the return expression has the wrong type.

We might want to do a transition here where we first warn that such code will be rejected in the future, before we start rejecting it. Would be good to get a crater run to tell us how common this pattern is.

Cc @rust-lang/types for more input.

adetaylor added a commit to adetaylor/rust that referenced this issue Nov 22, 2023
  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.

This commit changes two aspects of the behavior.

Previously, when examining the self type, we considered lifetimes only if they
were immediately adjacent to Self. We now consider lifetimes anywhere in the
self type.

Secondly, if more than one lifetime is discovered in the self type, we
disregard it as a possible lifetime elision candidate.

This is a compatibility break, and in fact has required some changes to tests
which assumed the earlier behavior.

Fixes rust-lang#117715
@lcnr lcnr added the C-bug Category: This is a bug. label Nov 23, 2023
@lcnr
Copy link
Contributor

lcnr commented Nov 23, 2023

I agree that the current behavior of self: &Box<ConcreteSelf> is a bug in our lifetime elision impl. I would also like to go with approach 1, but would wait for a crater run with that approach to gauge the impact.

@adetaylor
Copy link
Contributor Author

I've marked #117967 as ready for review. It goes with approach 1.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 11, 2023
…r=<try>

Fix lifetime elision

```rust
  struct Concrete(u32);

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

resulted in a confusing error.

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

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

Fixes rust-lang#117715
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 9, 2024
…r=<try>

Fix lifetime elision

```rust
  struct Concrete(u32);

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

resulted in a confusing error.

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

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

Fixes rust-lang#117715
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 10, 2024
…r=<try>

Fix ambiguous cases of multiple & in elided self lifetimes

```rust
  struct Concrete(u32);

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

resulted in a confusing error.

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

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

Fixes rust-lang#117715
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 18, 2024
…r=lcnr

Fix ambiguous cases of multiple & in elided self lifetimes

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](rust-lang#117967 (comment)))

Most of the code can be found in [late.rs](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html) and acts on AST types. The function [resolve_fn_params](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html#2006), 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](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html#2118) , and is described slightly [here](rust-lang#117715 (comment)) 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`.[^1]
2. Keep a record of:
   * Whether 0, 1 or n unique lifetimes are found on references encountered during the walk
4. If no lifetime was found, we don't return a lifetime. (This means other parameters' lifetimes may be used for return type lifetime elision).
5. If there's one lifetime found, we return the lifetime.
6. If multiple lifetimes were found, we abort elision entirely (other parameters' lifetimes won't be used).

[^1]: this prevents us from considering lifetimes from inside of the self-type

## Examples that were accepted before and will now be rejected

```rust
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

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

## Examples that were rejected before and will now be accepted

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

---

*edit: old PR description:*

```rust
  struct Concrete(u32);

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

resulted in a confusing error.

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

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

Fixes rust-lang#117715
@bors bors closed this as completed in 8d1958f Jul 18, 2024
princess-entrapta pushed a commit to princess-entrapta/rust that referenced this issue Jul 19, 2024
  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.

This commit changes two aspects of the behavior.

Previously, when examining the self type, we considered lifetimes only if they
were immediately adjacent to Self. We now consider lifetimes anywhere in the
self type.

Secondly, if more than one lifetime is discovered in the self type, we
disregard it as a possible lifetime elision candidate.

This is a compatibility break, and in fact has required some changes to tests
which assumed the earlier behavior.

Fixes rust-lang#117715
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jul 20, 2024
Fix ambiguous cases of multiple & in elided self lifetimes

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](rust-lang/rust#117967 (comment)))

Most of the code can be found in [late.rs](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html) and acts on AST types. The function [resolve_fn_params](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html#2006), 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](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html#2118) , and is described slightly [here](rust-lang/rust#117715 (comment)) 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`.[^1]
2. Keep a record of:
   * Whether 0, 1 or n unique lifetimes are found on references encountered during the walk
4. If no lifetime was found, we don't return a lifetime. (This means other parameters' lifetimes may be used for return type lifetime elision).
5. If there's one lifetime found, we return the lifetime.
6. If multiple lifetimes were found, we abort elision entirely (other parameters' lifetimes won't be used).

[^1]: this prevents us from considering lifetimes from inside of the self-type

## Examples that were accepted before and will now be rejected

```rust
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

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

## Examples that were rejected before and will now be accepted

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

---

*edit: old PR description:*

```rust
  struct Concrete(u32);

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

resulted in a confusing error.

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

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

Fixes rust-lang/rust#117715
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Jul 28, 2024
Fix ambiguous cases of multiple & in elided self lifetimes

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](rust-lang/rust#117967 (comment)))

Most of the code can be found in [late.rs](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html) and acts on AST types. The function [resolve_fn_params](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html#2006), 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](https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_resolve/late.rs.html#2118) , and is described slightly [here](rust-lang/rust#117715 (comment)) 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`.[^1]
2. Keep a record of:
   * Whether 0, 1 or n unique lifetimes are found on references encountered during the walk
4. If no lifetime was found, we don't return a lifetime. (This means other parameters' lifetimes may be used for return type lifetime elision).
5. If there's one lifetime found, we return the lifetime.
6. If multiple lifetimes were found, we abort elision entirely (other parameters' lifetimes won't be used).

[^1]: this prevents us from considering lifetimes from inside of the self-type

## Examples that were accepted before and will now be rejected

```rust
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

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

## Examples that were rejected before and will now be accepted

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

---

*edit: old PR description:*

```rust
  struct Concrete(u32);

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

resulted in a confusing error.

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

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

Fixes rust-lang/rust#117715
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants