Split elided_lifetime_in_paths into tied and untied

[shepmaster]

Before merging

• Discuss "tied" / "untied" terminology. Blocking this PR.
• Discuss "deprecated" terminology. Non-blocking.
• Discuss the async fn hard error case. Non-blocking.

Description

Types that contain a reference can be confusing when lifetime elision occurs:

// Confusing
fn foo(_: &u8) -> Bar { todo!() }

// Less confusing
fn foo(_: &u8) -> Bar<'_> { todo!() }

However, the previous lint did not distinguish when these types were not "tying" lifetimes across the function inputs / outputs:

// Maybe a little confusing
fn foo(_: Bar) {}

// More explicit but noisier with less obvious value
fn foo(_: Bar<'_>) {}

We now report different lints for each case, hopefully paving the way to marking the first case (when lifetimes are tied together) as warn-by-default (#91639).

Additionally, when multiple errors occur in the same function during the tied case, they are coalesced into one error. There is also some help text pointing out where the lifetime source is.

To discuss

"Tied" terminology

The lints are called elided_lifetimes_in_paths_tied and elided_lifetimes_in_paths_untied, under a lint group of elided_lifetimes_in_paths. The usage of "tied" and "untied" introduces new terminology for a concept that is not new.

We had a discussion about other phrasing, but no super strong alternative arose. "tied" is not the most beautiful choice, but it's serviceable.

Beyond "tied", a good argument has been made that the current lint name is factually incorrect (tl;dr: both Foo and Foo<'_> perform lifetime elision; Foo has a hidden lifetime while Foo<'_> has explicit syntax). A follow up PR could rename these lints to be more accurate.

Suggested resolution

Do nothing today. This phrasing is good enough to merge and we can always rename and leave aliases.

Alternate resolutions

Decorate the bikeshed for a few days/weeks/months/editions.

"Deprecated" terminology

The current wording of the diagnostic is (emphasis mine)

hidden lifetime parameters in types are deprecated

This suggests that at some point we want to completely stop supporting this and make it into a hard error that cannot be disabled. This seems like a strong prescription, given the turbulence this lint has had throughout its life.

Suggested resolution

Do nothing today. Assume we really do want to deprecate this capability. We may want to evaluate which cases we really want to deprecate (e.g. fn(_: ContainsLifetime) is generally considered harmless) and tweak the error message to better distinguish which patterns are on a path to being removed.

Alternate resolutions

Decide we don't want to deprecate it and pick a softer wording.

Existing hard errors

async and non-async functions are treated differently today:

struct ContainsLifetime<'a>(&'a ());

// This is not even a warning
fn demo_sync(x: ContainsLifetime) -> ContainsLifetime { x }

// This is an error
async fn demo_async(x: ContainsLifetime) -> ContainsLifetime { x }

It seems like the two cases should be the same.

Suggested resolution

Do nothing today. At some point, change elided_lifetimes_in_paths_tied to warn-by-default, then error-by-default (likely over an edition), then become a hard error (as suggested in the "deprecation" discussion). At that point, the two cases should be identical.

Alternate resolutions

We could dial back the async fn hard error to become error-by-default or warn-by-default, matching whatever desired end state we want.

[rustbot]

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or
reassign to another reviewer.

Use r? to explicitly pick a reviewer

[shepmaster]

As I've now tried to add this test twice and to help prevent trying to add it again... this fails because elision can't take place:

fn top_level_nested_to_top_level_nested(v: &ContainsLifetime) -> &ContainsLifetime { v }

error[E0106]: missing lifetime specifiers
 --> src/lib.rs:3:66
  |
3 | fn top_level_nested_to_top_level_nested(v: &ContainsLifetime) -> &ContainsLifetime {
  |                                            -----------------     ^^^^^^^^^^^^^^^^^ expected named lifetime parameter
  |                                                                  |
  |                                                                  expected named lifetime parameter
  |

[pnkfelix]

This generally looks fine. I had a few questions about what we expect to happen in a corner case.

r=me once those questions are addressed in some way (update: well, maybe the r=me is premature given the list of questions that @shepmaster included in the PR description. But I don't think the PR is waiting on review at this point.)

@rustbot label: +S-waiting-on-author -S-waiting-on-review

[pnkfelix]

Oh, I suppose there is still an open question about the use of the "tied"/"untied" terminology, which I admit threw me for a loop at first. I'm not sure which group is the best to handle resolving that question, though. And I'm also not entirely sure that resolving that question should block landing this work.

Is resolving a question like that a matter for WG-diagnostics, or for T-lang?

[shepmaster]

Is resolving a question like that a matter for WG-diagnostics, or for T-lang?

That's a great question that I don't have an answer for. I posed it in the Zulip thread hoping there was some existing terminology. Unfortunately, no one seemed aware of one. "Tied" made some intuitive sense for the small handful of people I asked one-on-one.

It feels like this is something that we must have talked about before and potentially even documented somewhere, but 🤷

[danielhenrymantilla]

If we are to nitpick about this, an elided lifetime refers to both Foo and Foo<'_> (as per the official lifetime elision rules), with the former being implicitly elided, and the latter, explicitly (very unfortunate that the original lint picked that name 😔). Hidden is a more concise word which we could reach for, now.

For the purpose of the lint, I'd also move the adjective around, since elided_lifetimes_in_paths_tied does not roll off the tongue too much.

Finally, on the most controversial/debatable aspect of the exact word being picked here (e.g., "tied"), my own subjective two cents on the topic, would be to not overthink it, and use meaningful: meaningful_lifetimes_hidden_in_paths. The other one could use free: free_lifetimes_hidden_in_paths.

[shepmaster]

I'm sympathetic to this feedback, but some points arise when trying to implement some of the suggestions:

• Calling the lints a_commonpart and b_commonpart means that they won't appear near each other in alphabetical lists (which are numerous).
• There's another lint (ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT) that should probably be updated.
• I'm torn between "hidden lifetimes" and "lifetimes hidden".
• There's a lot of other "elided" lifetimes in rustc_resolve's late.rs. I don't know which style they mean. Having both terms with mixed meanings may make things worse until someone cleans up all the code.

[danielhenrymantilla]

Yeah, fair enough

[shepmaster]

Alright, I've pushed a commit on top that changes to lifetimes_hidden_in_paths / tied_lifetimes_hidden_in_paths / untied_lifetimes_hidden_in_paths. This should allow us to see how we feel about it with some concrete code to look at.

TODO If we want to continue on this route, I did forget to rename the test files themselves.

[shepmaster]

...and I removed that commit from this PR, although I still have it locally. It looks like this PR will have enough trouble getting feedback, so let's go smaller.

[estebank]

When it comes to nomenclature of the lints, I have a bias towards keeping the name of the group as a substring of the individual lints, and I agree with some of daniel's observations, so my personal preference would be to name these lints ELIDED_LIFETIMES_IN_PATHS_EXPLICIT (not 100% sure on this one) and ELIDED_LIFETIMES_IN_PATHS_HIDDEN.

[danielhenrymantilla]

Thanks for pushing this forward! ❤️

[bors]

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

[traviscross]

Calling these hidden seems right to me. Let's check for consensus on this variation:

• The lint group is hidden-lifetimes-in-paths.
• Case 1: The fn(W) -> W and the fn(W) -> W<'_> cases are hidden-lifetimes-in-output-paths.
• Case 2: The fn(W) case is hidden-lifetimes-in-input-paths-only.
• Case 3: The f::<W> / <W>::f() / <T<W>>::f() case is hidden-lifetimes-in-type-paths.

This adds "only" to the second case, as otherwise it would seem logically odd for that one to not fire for:

struct W<'a>(&'a ());
fn f(x: W) -> W { x }

The third case covers:

struct W<'a>(&'a ());
fn f<T>() {}
fn main() {
    _ = f::<W>();
}

And this:

trait Tr { fn m() {}}
impl<T> Tr for T {}
struct W<'a>(&'a ());
struct U<T>(T);
fn main() {
    _ = <W as Tr>::m();
    _ = <U<W> as Tr>::m();
}

And this:

struct W<'a>(&'a ());
static S: W  = W(&());

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• other-instead-of-input resolved by Split elided_lifetime_in_paths into tied and untied #120808 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

Without knowing the specifics of our lint infrastructure, it strikes me that we could lint on the fn(W) -> W case if either hidden-lifetimes-in-input-paths or hidden-lifetimes-in-output-paths were enabled. Is that a possibility?

hidden-lifetimes-in-input-paths-only

It's a bit wordy for me, but if my suggestion is not feasible then I want the PR to land more than I want to continue bikeshedding the name. (edit: I also would have been fine with "tied" fwiw)

[traviscross]

If nothing else, to do what you suggest, we could make hidden-lifetimes-in-input-paths into a lint group that includes hidden-lifetimes-in-input-paths-only and hidden-lifetimes-in-output-paths.

The purpose here, though, is that we probably want to lint sooner and more strongly against hidden-lifetimes-in-output-paths than against hidden-lifetimes-in-input-paths-only, so it seems we'd still need that separated out.

(Agreed it's wordy. Naming is hard.)

[scottmcm]

Exciting to see this moving forward!

The critical thing I want to check: What if you have fn(W) -> W<'_>? That's what, IIRC, I called case 2 originally (different from the 3 cases here), where I really want to lint because while it's not hidden in the output path it's still really important to me that it be linted harder than just fn(W).

What happens with that case for these lints?

(Not worried about the names, we can always change them later if needed.)

[tmandry]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[scottmcm]

Ok, with the updated definition this sounds good -- assuming we'll have tests for it (if we don't already).

Then I can explain hidden-lifetimes-in-output-paths as "there's a hidden lifetime, and that lifetime appears in the output" rather than "it's hidden specifically in the output path.

@rfcbot reviewed

[nikomatsakis]

I am not going to block consensus on this, but I do want to note that I think my ideal might be to factor into two lints, where

• hidden-lifetimes-in-output-paths covers an output path with a hidden lifetime, as we intuitively expected
  • -> W
• mismatched-input-output-lietime covers the two cases where you refer to a lifetime in input/output in different ways:
  • W -> W<'_>
  • W<'a> -> W<'_>

I think all of the above should warn-by-default (and it seemed there was meeting consensus on this point).

If we decide to exclude the W<'a> -> W<'_> case from this lint, that seems ok, but I really would encourage someone to follow up on it!

[shepmaster]

What if you have fn(W) -> W<'_>

fn demo(a: ContainsLifetime) -> ContainsLifetime<'_> {
    a
}

error: hidden lifetime parameters in types are deprecated
 --> s.rs:3:12
  |
3 | fn demo(a: ContainsLifetime) -> ContainsLifetime<'_> {
  |            ^^^^^^^^^^^^^^^^ expected lifetime parameter
  |
  = note: `-D hidden-lifetimes-in-input-paths` implied by `-D hidden-lifetimes-in-paths`
  = help: to override `-D hidden-lifetimes-in-paths` add `#[allow(hidden_lifetimes_in_input_paths)]`
help: indicate the anonymous lifetime
  |
3 | fn demo(a: ContainsLifetime<'_>) -> ContainsLifetime<'_> {
  |                            ++++

[scottmcm]

= note: -D hidden-lifetimes-in-input-paths implied by -D hidden-lifetimes-in-paths

That's the input lint complaining. It's something that we want even when the input lint is disabled.

[shepmaster]

I chatted a bit with @scottmcm to better understand some of the team feelings. Here's my current tentative plan:

1. Rename the current lint from elided_lifetimes_in_paths to hidden_lifetimes_in_paths. This seems to have general consensus as a more accurate name.

2. Introduce a new lint: lifetime-styles-mismatch ¹.

   This lint will fire when a lifetime flows from a function input type to a function output type and the style of the input(s) and output(s) are not the same. There are three styles:

   1. hidden (&u8, ContainsLifetime)
   2. elided (&'_, ContainsLifetime<'_>)
   3. named (&'a u8, ContainsLifetime<'a>).

   These would trigger the lint:

   • fn x(&u8) -> ContainsLifetime<'_> ⚠️ This seems surprising ²
   • fn x(ContainsLifetime<'_>) -> &u8 ⚠️ This seems surprising ²
   • fn x(ContainsLifetime) -> ContainsLifetime<'_>
   • fn x<'a>(&'a u8) -> ContainsLifetime ³
   • fn x<'a>(&'a u8) -> ContainsLifetime<'_> ³
   • fn x(&u8) -> (ContainsLifetime, ContainsLifetime<'_>)

   These would not trigger the lint:

   • fn x(&u8) -> ContainsLifetime
   • fn x(ContainsLifetime<'_>) -> ContainsLifetime<'_>
   • fn x(ContainsLifetime) -> &u8
   • fn x(ContainsLifetime) -> ContainsLifetime
   • fn x<'a>(&'a u8) -> ContainsLifetime<'a>
   • fn x(&'_ u8) -> (ContainsLifetime<'_>, ContainsLifetime<'_>)

3. Split the elided_lifetimes_in_paths lint into 3 pieces (function input types, function output types, other types). The implementation of these lints should remain relatively simple.

As a followup PR / end goal, we would deny both lifetime-styles-mismatch and hidden-lifetimes-in-output-paths.

Footnotes

1. Please start the bikeshed color selection now so I can avoid changing this too many times. ↩

2. @scottmcm says: "I generally consider &u8 like it was written &'_ u8". We could change the categorization to do the same, which would change the behavior of these cases. ↩ ↩²

3. This is actually already caught by the elided_named_lifetimes lint; the proposed new lint may supersede this, but I haven't thought deeply about that yet. ↩ ↩²

[traviscross]

We've moved off of what was originally proposed, so we should restart.

@rfcbot cancel

[rfcbot]

@traviscross proposal cancelled.

[traviscross]

2. Introduce a new lint: lifetime-styles-mismatc

Please start the bikeshed color selection now...

I'd probably call it mismatched-lifetime-styles.

It's a bit imprecise, though, as it doesn't really get at the fact that it's concerned with the styles being mismatched for a particular single lifetime parameter. I.e., the styles are mismatched too in:

fn f<'a>(x: &'a (), y: &()) {}

But that's not what we're getting at. Probably that's OK though.

[traviscross]

There are three styles:

1. hidden (&u8, ContainsLifetime)
2. elided (&'_, ContainsLifetime<'_>)
3. named (&'a u8, ContainsLifetime<'a>).

In my view, &u8 is elided, not hidden. The & (without raw, at least) is explicit for "there's a lifetime here".

[traviscross]

@shepmaster: With the two changes I mentioned made to the proposal, and if you would elaborate your points number 1 and 3 to discuss the full hierarchy of lints, what each would be named, and what each would cover, then this seems OK to me.