Add a new `mismatched-lifetime-syntaxes` lint by shepmaster · Pull Request #138677 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

The lang-team discussed this and I attempted to summarize their decision. The summary-of-the-summary is:

Using two different kinds of syntax for elided lifetimes is confusing. In rare cases, it may even lead to unsound code! Some examples:
// Lint will warn about these
fn(v: ContainsLifetime) -> ContainsLifetime<'_>;
fn(&'static u8) -> &u8;
Matching up references with no lifetime syntax, references with anonymous lifetime syntax, and paths with anonymous lifetime syntax is an exception to the simplest possible rule:
// Lint will not warn about these
fn(&u8) -> &'_ u8;
fn(&'_ u8) -> &u8;
fn(&u8) -> ContainsLifetime<'_>;
Having a lint for consistent syntax of elided lifetimes will make the future goal of warning-by-default for paths participating in elision much simpler.

This new lint attempts to accomplish the goal of enforcing consistent syntax. In the process, it supersedes and replaces the existing elided-named-lifetimes lint, which means it starts out life as warn-by-default.

EDIT(jieyouxu): for the bigger picture, this lint is part of a set of lints that tries to encourage more consistent usage of lifetime syntaxes to make it less confusing for users. The language team FCP'd the overall direction for the set of lints:

We have three categories of lifetime syntaxes: named, elided, and hidden.

Named means that an explicitly-bound generic lifetime parameter or 'static was used, e.g. W<'a>, &'a ().

Elided means that an anonymous lifetime was used with syntactic indication, e.g. W<'_>, &'_ (), &().

Hidden means that an anonymous lifetime was used without syntactic indication, e.g. W.

We correspondingly add four lints.

mismatched_lifetime_syntaxes fires when more than one of the lifetime syntaxes are used in a signature to refer to the same generic lifetime parameter.

hidden_lifetimes_in_input_paths fires when a function input parameter type uses the hidden lifetime syntax.

hidden_lifetimes_in_output_paths fires when a function return type uses the hidden lifetime syntax.

hidden_lifetimes_in_type_paths fires when a type elsewhere is a path that uses the hidden lifetime syntax.

The first of these we will make immediately warn-by-default.

Examples of all of these cases are here:

Playground link

As before, there is some mapping of old to new lint names and groups:

elided_lifetime_in_path maps to the hidden_lifetimes_in_paths group

elided_lifetimes_in_paths maps to the hidden_lifetimes_in_paths group

the hidden_lifetimes_in_paths group expands to hidden_lifetimes_in_input_paths, hidden_lifetimes_in_output_paths, and hidden_lifetimes_in_type_paths

All three hidden_lifetimes_* lints will still be a part of the rust_2018_idioms group.

I notice also that in the PR, the elided_named_lifetimes lint is superseded by the mismatched_lifetime_syntaxes one, and that sounds right to me as well.

(Note that "in the PR" in the quote refers to #120808, where this PR #138677 splits out from #120808 the elided_named_lifetimes -> mismatched_lifetime_syntaxes case.)

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Mar 18, 2025

This comment has been minimized.

Thanks to:

@GrigorenkoPV for the original implementation of elided_named_lifetimes.
@compiler-errors for the push to write the lint using HIR instead of in rustc_resolve.
@estebank for the diagnostic help and wording feedback.

This comment has been minimized.

shepmaster changed the title~~Add a new mismatched_elided_lifetime_styles lint~~ Add a new mismatched-lifetime-syntaxes lint

Mar 20, 2025

This comment has been minimized.

This comment was marked as off-topic.

This comment was marked as resolved.

rustbot added the A-translation

Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic

label

Mar 23, 2025

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

Based on #138677 (comment), AFAICT this is ready on the lang side, and it looks good on the impl side, with concerns addressed. There's an outstanding non-blocking follow-up that's tracked in a separate issue. So:

@bors r=traviscross,jieyouxu rollup=never

Thanks a lot @shepmaster for working on this and addressing all the feedback 🩵

📌 Commit d35ad94 has been approved by traviscross,jieyouxu

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jun 5, 2025

Agreed, and to echo it, thanks to @shepmaster for pushing this forward.

What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 076ec59 (parent) -> ccf3198 (this PR)

Test differences

Show 314 test diffs

Stage 1

[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/example-from-issue48686.rs: [missing] -> pass (J0)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/missing-lifetime-kind.rs: [missing] -> pass (J0)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/not-tied-to-crate.rs: [missing] -> pass (J0)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/static.rs: [missing] -> pass (J0)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes.rs: [missing] -> pass (J0)
[ui] tests/ui/lint/elided-named-lifetimes/example-from-issue48686.rs: pass -> [missing] (J0)
[ui] tests/ui/lint/elided-named-lifetimes/missing-lifetime-kind.rs: pass -> [missing] (J0)
[ui] tests/ui/lint/elided-named-lifetimes/not-tied-to-crate.rs: pass -> [missing] (J0)
[ui] tests/ui/lint/elided-named-lifetimes/static.rs: pass -> [missing] (J0)

Stage 2

[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/example-from-issue48686.rs: [missing] -> pass (J1)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/missing-lifetime-kind.rs: [missing] -> pass (J1)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/not-tied-to-crate.rs: [missing] -> pass (J1)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes-details/static.rs: [missing] -> pass (J1)
[ui] tests/ui/lifetimes/mismatched-lifetime-syntaxes.rs: [missing] -> pass (J1)
[ui] tests/ui/lint/elided-named-lifetimes/example-from-issue48686.rs: pass -> [missing] (J1)
[ui] tests/ui/lint/elided-named-lifetimes/missing-lifetime-kind.rs: pass -> [missing] (J1)
[ui] tests/ui/lint/elided-named-lifetimes/not-tied-to-crate.rs: pass -> [missing] (J1)
[ui] tests/ui/lint/elided-named-lifetimes/static.rs: pass -> [missing] (J1)

Additionally, 296 doctest diffs were found. These are ignored, as they are noisy.

Job group index

J0: x86_64-gnu-llvm-19-3, x86_64-gnu-llvm-20-3
J1: aarch64-apple, aarch64-gnu, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, test-various, x86_64-apple-2, x86_64-gnu, x86_64-gnu-llvm-19-2, x86_64-gnu-llvm-20-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1 Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml --
test-dashboard ccf3198de316b488ee17441935182e9d5292b4d3 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-x86_64-apple: 6996.6s -> 10155.4s (45.1%)
x86_64-apple-1: 7661.2s -> 9584.7s (25.1%)
mingw-check-1: 1995.4s -> 2487.7s (24.7%)
dist-i686-mingw: 8003.9s -> 8617.1s (7.7%)
x86_64-msvc-1: 8836.7s -> 8190.9s (-7.3%)
x86_64-gnu-debug: 5901.7s -> 6305.2s (6.8%)
dist-i586-gnu-i586-i686-musl: 5402.0s -> 5047.8s (-6.6%)
x86_64-mingw-1: 9107.3s -> 8538.7s (-6.2%)
test-various: 4403.3s -> 4655.6s (5.7%)
dist-arm-linux-gnueabi: 4664.2s -> 4890.3s (4.8%) How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

This was referenced

Jun 5, 2025

Finished benchmarking commit (ccf3198): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean	range	count
Regressions ❌ (primary)	1.6%	[0.2%, 9.7%]	72
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	1.6%	[0.2%, 9.7%]	72

Max RSS (memory usage)

Results (primary 1.8%, secondary 2.3%)

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%	[0.6%, 3.2%]	57
Regressions ❌ (secondary)	2.3%	[1.5%, 4.5%]	7
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[-1.0%, 3.2%]	58

Cycles

Results (primary 2.6%)

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)	2.6%	[0.7%, 5.6%]	22
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[0.7%, 5.6%]	22

Binary size

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

Bootstrap: 750.189s -> 750.435s (0.03%)
Artifact size: 371.75 MiB -> 371.90 MiB (0.04%)

shepmaster deleted the consistent-elided-lifetime-syntax branch

June 6, 2025 12:23

TheLostLambda added a commit to TheLostLambda/byos-queuer that referenced this pull request

Jun 6, 2025

Those regressions are really bad... 9% on stm32f4xx-hal opt builds should amount to pretty all gains that have been painfully obtained the last 5 years...

Any plans to address this?

@jieyouxu Are those triggers based on stm32f4xx-hal code itself or coming from the stm32f4 crate? If the latter, there's probably very little that can be done about that since it's generated code... We could only try updating the test case then which is at 0.22.1 now.

As per the summary, this lint should not be triggered when the styles match, which makes sense. However, this lint triggers on functions taking a reference and returning a lifetimed adt when both use the 'hidden lifetimes' style. Is this intentional? In Bevy this triggers 231 warnings of this form and 0 true positives.

Edit: Ah, lang team decided that for references, hidden lifetimes count as elided instead.
I understand the reasoning insofar as this allows using an unnamed lifetime for an adt without having to use an unnamed lifetime for the a reference with the same lifetime, as it's clear that references have a lifetime:

fn function(&self) -> Lifetimed<'_>;

However, this doesn't mean the following should be forbidden by this lint:

fn function(&self) -> Lifetimed;

Of course, it may make sense to lint against the later case, such as via a hidden_lifetimes_in_output_paths lint proposed in #120808, but doing so in mismatched-lifetime-syntaxes is confusing and arguably incorrect.

this doesn't mean the following should be forbidden by this lint

Does the lint documentation clarify the language team's perspective as to why those are two different categories?

@therealprof are you seeing any other regressions in your builds or just looking at the perf report? note that the 9% is incremental, which will not be hit for cargo dependencies.
overall, this shouldn't really have an impact on the dependency, since most of the regression is in emitting the lint, which doesn't happen for deps

Does the lint documentation clarify the language team's perspective as to why those are two different categories?

It clarifies that a reference without a marked lifetime is classified as ellided rather than hidden. It does not clarify why.

As far as I can tell based on the comments and meeting notes, this classification is there to avoid linting against that, while potentially stylistically incongruent, are not confusing. That this reclassification causes this lint to trigger in my second example seems unintentional, which is why I asked.

I too wonder about if triggering this lint on fn function(&self) -> Lifetimed; is intentional, or merely a result of two locally reasonable decisions?

Yes, in general it would seem confusing to use different syntaxes. Yes, &str can reasonably be called "elided" and Lifetimed "hidden".

But is the effect of these decisions a desirable one? Is forbidding/discouraging fn function(&self) -> Lifetimed; really desirable, or does it just mean that the programmer needs to sprinkle needless "null" lifetime annotations to make the linter happy?

Lifetime annotations are already tricky for beginners, so does this specific case actually add any clarity for the compiler or the reader? In my opinion it does not.

In fact, to a naive relative newbie such as myself, the linter proposed solution is more inconsistent: fn function(&self) -> Lifetimed<'_>;

Isn't it more inconsistent to force a lifetime annotation on the output, when the input doesn't have one?

Lifetime annotations are already tricky for beginners, so does this specific case actually add any clarity for the compiler or the reader? In my opinion it does not.

In fact, to a naive relative newbie such as myself, the linter proposed solution is more inconsistent: fn function(&self) -> Lifetimed<'_>;

Isn't it more inconsistent to force a lifetime annotation on the output, when the input doesn't have one?

I agree and I made the same comment some hours ago on the Zulip discussion.

Both for making separate discussions manageable (because PR comments often get collapsed or becomes impossible to find) and visible (to not just only people subscribed to this PR), please redirect further feedback/issues to one of:

The performance discussion compiler thread.
The semantics and behavior of the mismatched_lifetime_syntaxes lint language thread. This includes feedback on which cases the lint should or should not warn on.
Open a dedicated issue, and tag it with @rustbot label: +L-mismatched_lifetime_syntaxes.

I'm going to lock this PR because it becomes impossible to follow multiple distinct discussions, all intermixed as top-level comments.

rust-lang locked as too heated and limited conversation to collaborators