Split elided_lifetime_in_paths into finer-grained lints

[shepmaster]

Description

Converts the existing elided_lifetime_in_paths lint into three smaller pieces:

• hidden_lifetimes_in_input_paths — fires for fn(ContainsLifetime) -> ...
• hidden_lifetimes_in_output_paths — fires for fn(...) -> ContainsLifetime
• hidden_lifetimes_in_type_paths — fires for many other usages of ContainsLifetime, such as in a static or in a turbofish

A new group hidden_lifetimes_in_paths is created with the three smaller lints and places that use the old elided_lifetime_in_paths name are updated to match.

#![allow(dead_code)]
#![warn(hidden_lifetimes_in_paths)]

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

fn inputs_and_outputs(v: ContainsLifetime) -> ContainsLifetime { v }
static CL: ContainsLifetime = ContainsLifetime(&42);

fn main() {}

warning: paths containing hidden lifetime parameters are deprecated
 --> a.rs:6:26
  |
6 | fn inputs_and_outputs(v: ContainsLifetime) -> ContainsLifetime { v }
  |                          ^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(hidden_lifetimes_in_input_paths)]` implied by `#[warn(hidden_lifetimes_in_paths)]`
help: indicate the anonymous lifetime
  |
6 | fn inputs_and_outputs(v: ContainsLifetime<'_>) -> ContainsLifetime { v }
  |                                          ++++

warning: paths containing hidden lifetime parameters are deprecated
 --> a.rs:6:47
  |
6 | fn inputs_and_outputs(v: ContainsLifetime) -> ContainsLifetime { v }
  |                                               ^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(hidden_lifetimes_in_output_paths)]` implied by `#[warn(hidden_lifetimes_in_paths)]`
help: indicate the anonymous lifetime
  |
6 | fn inputs_and_outputs(v: ContainsLifetime) -> ContainsLifetime<'_> { v }
  |                                                               ++++

warning: paths containing hidden lifetime parameters are deprecated
 --> a.rs:7:12
  |
7 | static CL: ContainsLifetime = ContainsLifetime(&42);
  |            ^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(hidden_lifetimes_in_type_paths)]` implied by `#[warn(hidden_lifetimes_in_paths)]`
help: indicate the anonymous lifetime
  |
7 | static CL: ContainsLifetime<'_> = ContainsLifetime(&42);
  |                            ++++

Background

In general, we want to discourage function signatures like fn (&T) -> ContainsLifetime, fn (ContainsLifetime) -> &T , and fn (ContainsLifetime) -> ContainsLifetime as it is not obvious that a lifetime flows through the function and back out as there is no visual indication that ContainsLifetime has a lifetime generic (such types are not usually literally called "contains lifetime" 😃).

In #120808 (comment) (and multiple followup comments, e.g. #120808 (comment)), the lang team decided on a plan where we introduce a new warn-by-default lint mismatched_lifetime_syntaxes (superseding elided_named_lifetimes) which helps insure consistency between input and output lifetime syntaxes and then split elided_lifetime_in_paths into three parts. The combination of these lints should be enough to accomplish the original goal.

History

Note that this PR has substantially changed from its original version. Check the (copious) comments below as well as the edit history of this message.

[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]

Thanks for pushing this forward! ❤️

[shepmaster]

r? @jieyouxu

[rustbot]

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

[nnethercote]

Ah, this is unfortunate. Ty is a very common type, an extra 8 bytes just to fit a boolean?

[jieyouxu]

Thanks for working on this! The broad implementation looks good to me modulo some nits, however I have two implementation specifics that I'm not very sure about.

Here's my initial review pass, but I would like to ask for a second opinion from @oli-obk (or @nnethercote) regarding two questions:

1. The "overlapping" mismatched_lifetime_syntaxes and hidden_lifetimes_in_output_paths situation, and whether to perform some kind of "suppression" to have mismatched_lifetime_syntaxes take precedence over hidden_lifetimes_in_output_paths unless hidden_lifetimes_in_output_paths have a stronger lint level. And if that is desired, the more important question is how, since these two lints run in their own lint passes. AFAICT, it doesn't seem easy to stash both lint diagnostics and their respective effective lint levels, then perform some kind of post-diagnostics-construction precedence filter that accounts for the relative lint levels between the two lints.
2. I'm also not very sure about the increase in size of Ty<'_>, since this is a very commonly used type, as @nnethercote pointed out in an earlier review comment. It seems like tracking source of the type is desirable, but I'm not too sure about an alternative scheme that doesn't involve increasing size of Ty<'_>.

I'm going to hand this review off to oli for a second opinion.

[jieyouxu]

Nit (wording): maybe for clarity (I was reading this, and it wasn't immediately clear)

Most of the time, we focus on elided lifetimes in function signatures, but they can also appear in other places! The original lint elided_lifetime_in_paths handled all these cases in one location, but it's more desirable to split it into 3 more fine-grained lints under a new hidden_lifetimes_in_paths lint group umbrella:

1. hidden_lifetimes_in_input_paths
2. hidden_lifetimes_in_output_paths
3. hidden_lifetimes_in_type_paths

[jieyouxu]

Suggestion [WORDING-STRENGTH 1/2]: I think I agree with @tmandry's comment earlier

hidden lifetime parameters in types are deprecated

I also think we should remove this wording at least until we upgrade some lint to warn-by-default.

where if this set of 3 lints are initially allow-by-default in this PR, we should not use "deprecated" in this PR, but instead add "deprecated" in the follow-up PR that bumps these three lints and the hidden_lifetimes_in_paths to warn-by-default, so that the deprecation is part of the follow-up decision to explicitly deprecate this form.

In this PR, I would reword this less strong initially, maybe something like

paths containing hidden lifetime parameters can be confusing

[jieyouxu]

Nit (wording): To make it easier to search for '_, maybe

This lint ensures that lifetime parameters are always explicitly stated, even if it is the placeholder lifetime '_.

Actually hm, this is more so trying to express

This lint ensures that lifetime parameters are explicit in types occuring as function arguments. Where lifetime elision may occur, the placeholder lifetime '_ may be used to make the presence of lifetime constraints explicit.

[jieyouxu]

Discussion: right, I see what you meant by having both mismatched_lifetime_syntaxes and hidden_lifetimes_in_output_paths firing against the same return position type-with-hidden-lifetime.

For reference, the discussion about what a reasonable scheme is to suppress the lower-lint-level of the two lints, or have mismatched_lifetime_syntaxes "shadow" hidden_lifetimes_in_output_paths when both are at the same lint level is at #t-compiler/diagnostics > Avoiding emitting one lint if another has been emitted.

@oli-obk suggested in that thread something among the lines of

Since we already have stashed errors we could have lint marking where we can explicitly mark spans as having linted and other lints can check if spans have been linted

Tho I'm not sure how to cleanly massage e.g. the stashed diagnostics mechanism and StashKey to indicate how to resolve if both lints are to be emitted (since they'd be ran in separate lint passes). I'll hand the review over to oli for this part.

[jieyouxu]

Suggestion [WORDING-STRENGTH 2/2]: same here, maybe lessen the "strength" conveyed in this initial PR adding this lint as allow-by-default, and defer the lint level bump + wording strengthening (to "deprecate" and "discourage") in the follow-up upgrade-to-warn-by-default PR. I.e. maybe

hidden lifetime parameters in types outside function signatures can be confusing

But I don't feel too strongly about this.

[jieyouxu]

Suggestion: can you add a little more elaboration on what this test intends to exercise? Is it to check that we recurse into items with mods, and/or the lint level can be properly controlled through lint level attributes?

[jieyouxu]

r? @oli-obk (for a second opinion, unless you're busy)