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

Add a new mismatched-lifetime-syntaxes lint #138677

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shepmaster
Copy link
Member

@shepmaster shepmaster commented Mar 18, 2025

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.

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

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 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
@rust-log-analyzer

This comment has been minimized.

@shepmaster
Copy link
Member Author

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.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 478a98e to e15c083 Compare March 18, 2025 21:01
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from e15c083 to ca86221 Compare March 20, 2025 13:34
@shepmaster shepmaster changed the title Add a new mismatched_elided_lifetime_styles lint Add a new mismatched-lifetime-syntaxes lint Mar 20, 2025
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from ca86221 to 6b4ba02 Compare March 20, 2025 13:57
@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross dismissed their stale review March 20, 2025 19:33

Addressed.

@bors
Copy link
Collaborator

bors commented Mar 22, 2025

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

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 6b4ba02 to 7ff70d4 Compare March 23, 2025 00:48
@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Mar 23, 2025
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 7ff70d4 to 3a95f66 Compare March 24, 2025 17:29
@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 3a95f66 to 1625496 Compare March 24, 2025 20:01
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 1625496 to 1cd8999 Compare March 25, 2025 00:55
@bors
Copy link
Collaborator

bors commented Mar 28, 2025

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

This will allow us to eagerly translate messages on a top-level
diagnostic, such as a `LintDiagnostic`. As a bonus, we can remove the
awkward closure passed into Subdiagnostic and make better use of
`Into`.
@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch 2 times, most recently from 85b9c5a to 573f332 Compare March 31, 2025 20:22
@rust-log-analyzer

This comment has been minimized.

An upcoming lint will want to be able to know if a lifetime is
hidden (e.g. `&u8`, `ContainsLifetime`) or anonymous: (e.g. `&'_ u8`,
`ContainsLifetime<'_>`). It will also want to know if the lifetime is
related to a reference (`&u8`) or a path (`ContainsLifetime`).
@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 573f332 to 702e59d Compare April 1, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants