Remove kw::Empty uses from hir::Lifetime::ident

[nnethercote]

hir::Lifetime::ident is sometimes set to kw::Empty and it's really confusing. This PR stops that. Helps with #137978.

r? @lcnr

[nnethercote]

Best reviewed one commit at a time.

[lcnr]

when does this happen to be an underscore lifetime?

[nnethercote]

When the 'static is elided, e.g. for both C and D here:

struct Name<'a>(&'a str);
const C: Name = Name("name"); // IsAnonInPath::Yes
static D: &str = "";          // IsAnonInPath::No

I didn't even realize 'static could be elided like this!

I will add a comment about this.

[lcnr]

would it be easier to have a LifetimeName::Anon|ElidedInPath? and not have two enums inside of hir::Lifetime?

[nnethercote]

I originally wanted to combine the two enums in exactly this fashion, but it turns out that IsAnonInPath::Yes can occur with four of the five LifetimeName variants, viz:

#![allow(unused)]

//---------------------------------------------------------------------------
struct Name<'a>(&'a str);
const C: Name = Name("name"); // LifetimeRes::Static, name='_, IsAnonInPath::Yes
static D: &str = "";          // LifetimeRes::Static, name='_, IsAnonInPath::No

//---------------------------------------------------------------------------
#[repr(C)]
struct Safe<'a>(&'a u32);
extern "C" {
    fn foo(safe: Safe);     // LifetimeRes::Param, name='_, IsAnonInPath::Yes
}

//---------------------------------------------------------------------------
struct St<'a> { f1: &'a u32 }

fn f() {
    let st = St { f1: &0u32 };  // LifetimeRes::Infer, name='_, IsAnonInPath::Yes
}

//---------------------------------------------------------------------------
// commented out because it causes an error
//struct Baz<'a>(&'a str);    // LifetimeRes::Error, name='_, IsAnonInPath::Yes
//struct Baz2(Baz);

fn main() {}

Once again, lifetimes can be omitted from paths in more places than I realized.

If you wanted to combine ident and res and is_anon_in_path you'd have an enum like this:

enum Combined {
    ParamUserNamed(Ident),         // user-specified name, IsAnonInPath::No
    ParamAnon(Span, IsAnonInPath), // name='_

    Infer(Span, IsAnonInPath),     // name='_

    StaticNamed(Span),             // name='static, IsAnonInPath::No
    StaticAnon(IsAnonInPath),      // name='_

    Error(Ident),                  // user-specified name, IsAnonInPath::No
    ErrorAnon(Span, IsAnonInPath), // name='_

    ImplicitObjectLifetimeDefault, // name='_, IsAnonInPath::No
}

I think that captures all the possibilities and excludes all invalid combinations. I'm generally very much a fan of "don't let your types express invalid values" but this feels like it's taking it too far?

[nnethercote]

I updated, making these changes:

• Renamed IsPathAnon to IsAnonInPath.
• Added a big comment on Lifetime.
• Added an assertion to new_named_lifetime: if IsAnonInPath::Yes, the lifetime name must be '_.
• Expanded pretty-printing test coverage. The main commit now makes some slight improvements to pretty-printing.

[lcnr]

so this PR doesn't change behavior which is fine, however. Even with this cleanup the status quo still feels confusing to me.

I feel like maybe Lifetime should be something like

struct Lifetime {
    hir_id: HirId,

    source: LifetimeSource,

    res: LifetimeName,
}

enum LifetimeSource {
   // Used for everything where we have an explicit lifetime, including `'_` 
   Explicit { name: Ident },
   // Used for fully elided lifetimes, i.e. all cases of `IsAnonInPath::Yes`
   // as well as `&Type` maybe. Idk whether these should be treated differently.
   FullyElided { insert_span: Span },
}

Do you think this approach would make sense? I think merging this PR as is is already an improvement as it makes the weirdness more explicit.

[nnethercote]

   // Used for fully elided lifetimes, i.e. all cases of `IsAnonInPath::Yes`
   // as well as `&Type` maybe. Idk whether these should be treated differently.
   FullyElided { insert_span: Span },

The "as well as &Type maybe" isn't possible. The IsAnonInPath has to be distinct otherwise LifetimeSuggestion won't work. You could do this:

pub enum LifetimeIdent {
    Normal { ident: Ident },
    IsAnonInPath { span: Span },
}

It would avoid the invalid combination of IsAnonInPath::Yes with a non-underscore lifetime ident, but that's the only benefit, and it adds a bunch of matches, etc. I don't think it's worth it.

There might still be a nicer formulation of this code, but I think this PR is worth landing in its current state.

[lcnr]

when is this part of the condition ever false. I would expect all AnonInPath to have empty spans

[nnethercote]

IsAnonInPath::Yes lifetimes are inserted in maybe_insert_elided_lifetimes_in_path. In that function elided_lifetime_span can definitely be non-empty.

However, none of those non-empty spans reach here, at least when running the UI tests. (I checked by adding an assertion.) I don't know if that's guaranteed, or if we just have insufficient test coverage. So I'd prefer to keep the test here for now; it is retained from the original code.

[lcnr]

this is where changing ident to a source enum would be helpful

[lcnr]

a suggestion and one final nit

r=me after fixing the nit if u don't apply the suggestion

[nnethercote]

I didn't change the is_empty check for the reason explained above.

@bors r=lcnr

[bors]

📌 Commit 8d2c63f has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8d2c63f with merge 3f690c2...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 3f690c2 to master...

[github-actions]

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 7586a9f (parent) -> 3f690c2 (this PR)

Test differences

Show 17 test diffs

Stage 1

• [pretty] tests/pretty/hir-lifetimes.rs: [missing] -> pass (J1)

Stage 2

• [pretty] tests/pretty/hir-lifetimes.rs: [missing] -> pass (J0)

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

Job group index

• J0: aarch64-apple, aarch64-gnu, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, x86_64-apple-1, x86_64-gnu, x86_64-gnu-llvm-18-1, x86_64-gnu-llvm-18-2, x86_64-gnu-llvm-19-1, x86_64-gnu-llvm-19-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
• J1: x86_64-gnu-llvm-18-3, x86_64-gnu-llvm-19-3

Job duration changes

1. aarch64-gnu: 6389.9s -> 9187.4s (43.8%)
2. armhf-gnu: 4230.0s -> 4565.5s (7.9%)
3. aarch64-gnu-debug: 6072.9s -> 6518.9s (7.3%)
4. dist-x86_64-apple: 9342.2s -> 10014.0s (7.2%)
5. x86_64-apple-1: 7733.7s -> 8121.6s (5.0%)
6. dist-i686-mingw: 7956.4s -> 8308.4s (4.4%)
7. i686-mingw-1: 7321.4s -> 7573.8s (3.4%)
8. dist-s390x-linux: 5347.5s -> 5521.1s (3.2%)
9. aarch64-apple: 3921.1s -> 4028.3s (2.7%)
10. dist-i586-gnu-i586-i686-musl: 5222.4s -> 5350.5s (2.5%)

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.

[rust-timer]

Finished benchmarking commit (3f690c2): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary -2.1%)

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -0.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)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 778.708s -> 777.401s (-0.17%)
Artifact size: 365.91 MiB -> 365.92 MiB (0.00%)

[shepmaster]

Kindly have a look at #138677 and let me know if you think I’ll have to undo (some of?) these changes.

[nnethercote]

@shepmaster: Oof, that's unfortunate timing, me removing the previously-unused distinction between kw::Empty and kw::UnderscoreLifetime just as you're adding code that depends on it. It looks like you've been working on the lint for a while, and I don't want my cleanup to block it, so we can revert it if necessary. A few questions before doing that.

• I'm confused by the diffs in Add a new mismatched-lifetime-syntaxes lint #138677. In particular, Lifetime::suggestion looks very different to what I expect. Also Lifetime::suggestion_position and LifetimeSuggestionPosition are present, but neither of them exist on trunk. Where do they come from?

• Is there a reason the lint is done on HIR and not the AST? I would have thought the AST would be more appropriate, being closer to the original source code. Indeed, that's the reason I figured my cleanup would be fine, because HIR is further away from source and the &u8 vs &'_ u8 distinction didn't matter.

• I have a follow-up in Improve Lifetime::suggestion #139046, currently in draft form, though it's pretty close to what would have been the final version and the commit messages explain what's going on. It eliminates the make_suggestion closure in much the same way your code does.

[shepmaster]

we can revert it if necessary

I don't believe that will be required. I've rebased on top of your changes and pushed on through. That is my entire set of change, which will be spread out over at least three PRs (two of which are open now) — only the first 4 commits will exist in PR #138677. Specifically related to these changes, I've continued modifying Lifetime by transforming your new field into two. One of the new fields is (I think!) close to the suggestion to have a source enum.

I'm confused by the diffs in Add a new mismatched-lifetime-syntaxes lint #138677. In particular, Lifetime::suggestion looks very different to what I expect.

I'm not totally sure what you'd expect, but hopefully the reworked version of that function reads more cleanly?

Lifetime::suggestion_position and LifetimeSuggestionPosition are present, but neither of them exist on trunk. Where do they come from?

They were deleted in this very PR 😉

Is there a reason the lint is done on HIR and not the AST?

When I asked for help on Zulip, it was suggested that a HIR lint would be nicer. Based on that feedback, I scrapped my original lint changes which lived in rustc_resolve and rewrote them (extending them based on lang-team feedback) as acting on HIR.

I have a follow-up in Improve Lifetime::suggestion #139046, currently in draft form, though it's pretty close to what would have been the final version and the commit messages explain what's going on.

I think there's some strong similarity to my changes there. Notably, I see you are tracking if there are angle brackets, which is also something I needed to add.

It eliminates the make_suggestion closure in much the same way your code does.

My reworked code continues to eliminate the the closure as well. I'm quite happy that duplicated code is being removed.

[nnethercote]

I've rebased on top of your changes and pushed on through

Oh, great news. I will stop working on #139046 until you are finished with the lint, though feel free to steal any ideas from it that work for you :)

Lifetime::suggestion_position and LifetimeSuggestionPosition are present, but neither of them exist on trunk. Where do they come from?

They were deleted in this very PR 😉

lol, I totally forgot I had done that. Sorry for the confusion.