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.

Sign up for GitHub

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

Jump to bottom

Add a new mismatched-lifetime-syntaxes lint #138677

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

Add a new mismatched-lifetime-syntaxes lint #138677

wants to merge 4 commits into from
+1,696 −856

Conversation

shepmaster
Copy link
Member

@shepmaster shepmaster commented Mar 18, 2025 •
edited
Loading

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 a 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
@shepmaster shepmaster mentioned this pull request Mar 18, 2025
Draft
3 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
shepmaster
shepmaster commented Mar 18, 2025
View reviewed changes
compiler/rustc_lint/src/lints.rs Outdated
Comment on lines 3179 to 3128
// TODO: how avoid calling it lifetime_name2?
diag.arg("lifetime_name2", self.lifetime_name);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called lifetime_name2 because the nested MismatchedElidedLifetimeStylesSuggestion::Named also declares a fluent variable called lifetime_name and they clobber each other. I'm pretty sure this is me doing it wrong, but I have not yet found the right way.

@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.

Sign in to view
traviscross
traviscross previously requested changes Mar 19, 2025
View reviewed changes
@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.

Sign in to view
@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.

Sign in to view
@traviscross traviscross dismissed their stale review March 20, 2025 19:33

Addressed.

@bors
Copy link
Contributor

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.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@shepmaster
Move eager translation to a method on Diag
152e056 
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
Distinguish hidden and anonymous lifetimes in references in HIR
59fe399 
An upcoming lint will want to be able to know if a reference has a
hidden lifetime (`&u8`) or an anonymous lifetime (`&'_ u8`). The code
previously treated hidden lifetimes as anonymous.

To preserve diagnostic quality, lifetime suggestions now need to know
if they are in the context of a reference or a path, as there is
different syntax for each. For example, a reference changes from `&u8`
to `&'a u8` (add a space with no comma), while a path changes from `T`
to `T<'a>` (add angle brackets) or `T<X>` to `T<'a, X>` (add a comma
and a space).

There's a minor ugliness introduced to -Zunpretty=hir output, as
there's a non-idiomatic space character introduced after the
ampersand, but it's called unpretty for a reason!
@shepmaster
Introduce the mismatched_lifetime_syntaxes lint
fc85611
@shepmaster
Replace elided_named_lifetimes with mismatched_lifetime_syntaxes
1cd8999
@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 1625496 to 1cd8999 Compare March 25, 2025 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traviscross traviscross

Assignees

@wesleywiser wesleywiser

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shepmaster @rustbot @rust-log-analyzer @bors @traviscross @wesleywiser