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

Suggestion for single_use_lifetimes suggests code that fails to compile #117965

Closed
jhpratt opened this issue Nov 16, 2023 · 2 comments · Fixed by #120148
Closed

Suggestion for single_use_lifetimes suggests code that fails to compile #117965

jhpratt opened this issue Nov 16, 2023 · 2 comments · Fixed by #120148
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. F-lint-single_use_lifetimes `single_use_lifetimes` lint T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics

Comments

@jhpratt
Copy link
Member

jhpratt commented Nov 16, 2023

playground

#![deny(single_use_lifetimes)]

pub enum Data<'a> {
    Borrowed(&'a str),
    Owned(String),
}

impl<'a> Data<'a> {
    pub fn get<'b: 'a>(&'b self) -> &'a str {
        match &self {
            Self::Borrowed(val) => val,
            Self::Owned(val) => &val,
        }
    }
}

This code, as expected, triggers the lint. This is the compiler output:

error: lifetime parameter `'b` only used once
 --> src/lib.rs:9:16
  |
9 |     pub fn get<'b: 'a>(&'b self) -> &'a str {
  |                ^^       -- ...is used only here
  |                |
  |                this lifetime...
  |
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(single_use_lifetimes)]
  |         ^^^^^^^^^^^^^^^^^^^^
help: elide the single-use lifetime
  |
9 -     pub fn get<'b: 'a>(&'b self) -> &'a str {
9 +     pub fn get(&self) -> &'a str {
  |

Applying the suggested change, we then get a lifetime error.

error: lifetime may not live long enough
  --> src/lib.rs:10:9
   |
8  |   impl<'a> Data<'a> {
   |        -- lifetime `'a` defined here
9  |       pub fn get(&self) -> &'a str {
   |                  - let's call the lifetime of this reference `'1`
10 | /         match &self {
11 | |             Self::Borrowed(val) => val,
12 | |             Self::Owned(val) => &val,
13 | |         }
   | |_________^ method was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`

In this specific instance, &'a str must also be changed to &str. That'll then trigger the lint on impl<'a> Data<'a>, with no ensuing issues.

I have not checked this, but I my suspicion is any place where a lifetime bound is also present in the return type (or possibly even a parameter) will result in a similar failure to compile the suggested code. Figuring out which lifetimes to suggest removal of in other parts of the function signature probably isn't straightforward.

This example is not theoretical, unfortunately. I ran into this in real-world code and it caused me quite a bit of time to figure out. The example is the minimal reproduction of the bad suggestion.

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-diagnostics Area: Messages for errors, warnings, and lints A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 16, 2023
@jruderman
Copy link
Contributor

Should the single_use_lifetimes lint even trigger for 'b? It's only "used" once directly, but it's also linked to another lifetime through the bound 'b: 'a.

@jruderman
Copy link
Contributor

cc #44752

@rustbot label +F-lint-single_use_lifetimes

@rustbot rustbot added the F-lint-single_use_lifetimes `single_use_lifetimes` lint label Nov 16, 2023
Xanewok added a commit to Xanewok/slang that referenced this issue Dec 12, 2023
Unfortunately, we can't turn it on because it has false positives that
suggest a code that fails to compile, i.e.
```rust
enum Errors<'err> {
    #[error("An item with the name '{0}' already exists.")]
    ExistingItem(&'err Identifier),
    #[error("A variant referencing '{0}' already exists.")]
    ExistingVariant(&'err Identifier),
    #[error("An expression with the name '{0}' already exists.")]
    ExistingExpression(&'err Identifier),
}
```

suggests to remove the `'err` but it's impossible to define the enum
using the elided lifetime, i.e. `enum Errors<'_>`.

See rust-lang/rust#117965 for the existing bug report.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 20, 2024
`single_use_lifetimes`: Don't suggest deleting lifetimes with bounds

Closes rust-lang#117965

```
9 |     pub fn get<'b: 'a>(&'b self) -> &'a str {
  |                ^^       -- ...is used only here
  |                |
  |                this lifetime...
```

In this example, I think the `&'b self` can be replaced with the bound itself, yielding `&'a self`, but this would require a deeper refactor. Happy to do as a follow-on PR if desired.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 20, 2024
Rollup merge of rust-lang#120148 - trevyn:issue-117965, r=cjgillot

`single_use_lifetimes`: Don't suggest deleting lifetimes with bounds

Closes rust-lang#117965

```
9 |     pub fn get<'b: 'a>(&'b self) -> &'a str {
  |                ^^       -- ...is used only here
  |                |
  |                this lifetime...
```

In this example, I think the `&'b self` can be replaced with the bound itself, yielding `&'a self`, but this would require a deeper refactor. Happy to do as a follow-on PR if desired.
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-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. F-lint-single_use_lifetimes `single_use_lifetimes` lint T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants