fix(resolve): Warn on Self-Type Lifetime Elision for Non-Self Types

[TKanX]

View all comments

Summary

FCW lint self_lifetime_elision_not_applicable: fires when a self parameter contains & but does not syntactically contain Self.

Tracking: #140611

Context

Self-type lifetime elision in rustc_resolve relies on a name-matching hack (is_self_ty) that shallow-compares Res values between the self parameter type and the impl self type. As @compiler-errors identified in #140611, this hack cannot see through type aliases (different Res) and only fires for Struct|Union|Enum|PrimTy impls (excludes TyAlias). When it misses, find_lifetime_for_self returns Empty, self-elision silently fails, and a later parameter's lifetime may leak into the return type.

@lcnr proposed restricting self-elision to genuine Self and FCW'ing everything else. This PR implements that.

Behavior

pub struct W<'a>(&'a ());
pub type Alias<'a> = W<'a>;

Case	Code	Before	After
f1	impl Alias { fn f(self: &W) -> &() }	silent wrong '_	FCW
f2	impl Alias { fn f(self: &Alias) -> &() }	silent wrong '_	FCW
f3	impl Alias { fn f(&self) -> &() }	correct	unchanged
f4	impl W { fn f(self: &W) -> &() }	correct (hack fires)	FCW
f5	impl W { fn f(self: &Alias) -> &() }	silent wrong '_	FCW
f6	impl W { fn f(&self) -> &() }	correct	unchanged

A follow-up PR will remove the hack after a warning period.

r? @lcnr
cc @compiler-errors @traviscross

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

[lcnr]

before discussing this in a lang meeting, can you extend the PR description a bit to summarize the reasoning and discussion behind this?

[TKanX]

my crate ptagger showed up on the affected crates list, even though i did not use type aliases in the project.

See case f4.

Logs

[INFO] cloning repository https://github.com/Paladynee/ptagger
[INFO] running `Command { std: "git" "-c" "credential.helper=" "-c" "credential.helper=/workspace/cargo-home/bin/git-credential-null" "clone" "--bare" "https://github.com/Paladynee/ptagger" "/workspace/cache/git-repos/https%3A%2F%2Fgithub.com%2FPaladynee%2Fptagger", kill_on_drop: false }`
[INFO] [stderr] Cloning into bare repository '/workspace/cache/git-repos/https%3A%2F%2Fgithub.com%2FPaladynee%2Fptagger'...
[INFO] running `Command { std: "git" "rev-parse" "HEAD", kill_on_drop: false }`
[INFO] [stdout] 9c95a5631d419e64c5725971bfe89102a729b1ec
[INFO] checking Paladynee/ptagger against try#39605810efb1a0d52f955bfba9e4eec7a01d5deb for pr-153692
[INFO] running `Command { std: "git" "clone" "/workspace/cache/git-repos/https%3A%2F%2Fgithub.com%2FPaladynee%2Fptagger" "/workspace/builds/worker-7-tc2/source", kill_on_drop: false }`
[INFO] [stderr] Cloning into '/workspace/builds/worker-7-tc2/source'...
[INFO] [stderr] done.
[INFO] started tweaking git repo https://github.com/Paladynee/ptagger
[INFO] finished tweaking git repo https://github.com/Paladynee/ptagger
[INFO] tweaked toml for git repo https://github.com/Paladynee/ptagger written to /workspace/builds/worker-7-tc2/source/Cargo.toml
[INFO] validating manifest of git repo https://github.com/Paladynee/ptagger on toolchain 39605810efb1a0d52f955bfba9e4eec7a01d5deb
[INFO] running `Command { std: CARGO_HOME="/workspace/cargo-home" RUSTUP_HOME="/workspace/rustup-home" "/workspace/cargo-home/bin/cargo" "+39605810efb1a0d52f955bfba9e4eec7a01d5deb" "metadata" "--manifest-path" "Cargo.toml" "--no-deps", kill_on_drop: false }`
[INFO] crate git repo https://github.com/Paladynee/ptagger already has a lockfile, it will not be regenerated
[INFO] running `Command { std: CARGO_HOME="/workspace/cargo-home" RUSTUP_HOME="/workspace/rustup-home" "/workspace/cargo-home/bin/cargo" "+39605810efb1a0d52f955bfba9e4eec7a01d5deb" "fetch" "--manifest-path" "Cargo.toml", kill_on_drop: false }`
[INFO] running `Command { std: "docker" "create" "-v" "/var/lib/crater-agent-workspace/builds/worker-7-tc2/target:/opt/rustwide/target:rw,Z" "-v" "/var/lib/crater-agent-workspace/builds/worker-7-tc2/source:/opt/rustwide/workdir:ro,Z" "-v" "/var/lib/crater-agent-workspace/cargo-home:/opt/rustwide/cargo-home:ro,Z" "-v" "/var/lib/crater-agent-workspace/rustup-home:/opt/rustwide/rustup-home:ro,Z" "-e" "SOURCE_DIR=/opt/rustwide/workdir" "-e" "CARGO_TARGET_DIR=/opt/rustwide/target" "-e" "CARGO_HOME=/opt/rustwide/cargo-home" "-e" "RUSTUP_HOME=/opt/rustwide/rustup-home" "-w" "/opt/rustwide/workdir" "-m" "1610612736" "--user" "0:0" "--network" "none" "ghcr.io/rust-lang/crates-build-env/linux@sha256:d429b63d4308055ea97f60fb1d3dfca48854a00942f1bd2ad806beaf015945ec" "/opt/rustwide/cargo-home/bin/cargo" "+39605810efb1a0d52f955bfba9e4eec7a01d5deb" "metadata" "--no-deps" "--format-version=1", kill_on_drop: false }`
[INFO] [stdout] 94c4d4501534a86d5da79ca00f5a941d0ac2606921b8158b39e414293d42963c
[INFO] running `Command { std: "docker" "start" "-a" "94c4d4501534a86d5da79ca00f5a941d0ac2606921b8158b39e414293d42963c", kill_on_drop: false }`
[INFO] running `Command { std: "docker" "inspect" "94c4d4501534a86d5da79ca00f5a941d0ac2606921b8158b39e414293d42963c", kill_on_drop: false }`
[INFO] running `Command { std: "docker" "rm" "-f" "94c4d4501534a86d5da79ca00f5a941d0ac2606921b8158b39e414293d42963c", kill_on_drop: false }`
[INFO] [stdout] 94c4d4501534a86d5da79ca00f5a941d0ac2606921b8158b39e414293d42963c
[INFO] running `Command { std: "docker" "create" "-v" "/var/lib/crater-agent-workspace/builds/worker-7-tc2/target:/opt/rustwide/target:rw,Z" "-v" "/var/lib/crater-agent-workspace/builds/worker-7-tc2/source:/opt/rustwide/workdir:ro,Z" "-v" "/var/lib/crater-agent-workspace/cargo-home:/opt/rustwide/cargo-home:ro,Z" "-v" "/var/lib/crater-agent-workspace/rustup-home:/opt/rustwide/rustup-home:ro,Z" "-e" "SOURCE_DIR=/opt/rustwide/workdir" "-e" "CARGO_TARGET_DIR=/opt/rustwide/target" "-e" "CARGO_INCREMENTAL=0" "-e" "RUST_BACKTRACE=full" "-e" "RUSTFLAGS=--cap-lints=forbid" "-e" "RUSTDOCFLAGS=--cap-lints=forbid" "-e" "CARGO_HOME=/opt/rustwide/cargo-home" "-e" "RUSTUP_HOME=/opt/rustwide/rustup-home" "-w" "/opt/rustwide/workdir" "-m" "1610612736" "--user" "0:0" "--network" "none" "ghcr.io/rust-lang/crates-build-env/linux@sha256:d429b63d4308055ea97f60fb1d3dfca48854a00942f1bd2ad806beaf015945ec" "/opt/rustwide/cargo-home/bin/cargo" "+39605810efb1a0d52f955bfba9e4eec7a01d5deb" "check" "--frozen" "--all" "--all-targets" "--message-format=json", kill_on_drop: false }`
[INFO] [stdout] 6ddf651ad52ef0d3c479726084ea28e900564b0c0e0557a054af7b0ae8300310
[INFO] running `Command { std: "docker" "start" "-a" "6ddf651ad52ef0d3c479726084ea28e900564b0c0e0557a054af7b0ae8300310", kill_on_drop: false }`
[INFO] [stderr]     Checking ptagger v0.1.0 (/opt/rustwide/workdir)
[INFO] [stdout] warning: unused import: `core::marker::PhantomData`
[INFO] [stdout]  --> src/hightagged.rs:3:5
[INFO] [stdout]   |
[INFO] [stdout] 3 | use core::marker::PhantomData;
[INFO] [stdout]   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]   |
[INFO] [stdout]   = note: `#[warn(unused_imports)]` (part of `#[warn(unused)]`) on by default
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] warning: unused imports: `DerefMut` and `Deref`
[INFO] [stdout]  --> src/hightagged.rs:5:17
[INFO] [stdout]   |
[INFO] [stdout] 5 | use core::ops::{Deref, DerefMut, Range};
[INFO] [stdout]   |                 ^^^^^  ^^^^^^^^
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: `self` parameter type does not contain `Self`
[INFO] [stdout]    --> src/lowtagged.rs:129:29
[INFO] [stdout]     |
[INFO] [stdout] 129 |     pub fn write_data(self: &mut TaggedRefLow<'a, T>, data: usize) {
[INFO] [stdout]     |                             ^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = help: use `&self`, `&mut self`, or `self: &Self` for correct lifetime elision
[INFO] [stdout]     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
[INFO] [stdout]     = note: for more information, see issue #140611 <https://github.com/rust-lang/rust/issues/140611>
[INFO] [stdout]     = note: `#[deny(self_lifetime_elision_not_applicable)]` (part of `#[deny(future_incompatible)]`) on by default
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: `self` parameter type does not contain `Self`
[INFO] [stdout]    --> src/lowtagged.rs:176:31
[INFO] [stdout]     |
[INFO] [stdout] 176 |     pub fn extract_data(self: &TaggedMutRefLow<'a, T>) -> usize {
[INFO] [stdout]     |                               ^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = help: use `&self`, `&mut self`, or `self: &Self` for correct lifetime elision
[INFO] [stdout]     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
[INFO] [stdout]     = note: for more information, see issue #140611 <https://github.com/rust-lang/rust/issues/140611>
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: `self` parameter type does not contain `Self`
[INFO] [stdout]    --> src/lowtagged.rs:181:29
[INFO] [stdout]     |
[INFO] [stdout] 181 |     pub fn write_data(self: &mut TaggedMutRefLow<'a, T>, data: usize) {
[INFO] [stdout]     |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = help: use `&self`, `&mut self`, or `self: &Self` for correct lifetime elision
[INFO] [stdout]     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
[INFO] [stdout]     = note: for more information, see issue #140611 <https://github.com/rust-lang/rust/issues/140611>
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] warning: unused import: `core::marker::PhantomData`
[INFO] [stdout]  --> src/hightagged.rs:3:5
[INFO] [stdout]   |
[INFO] [stdout] 3 | use core::marker::PhantomData;
[INFO] [stdout]   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]   |
[INFO] [stdout]   = note: `#[warn(unused_imports)]` (part of `#[warn(unused)]`) on by default
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] warning: unused imports: `DerefMut` and `Deref`
[INFO] [stdout]  --> src/hightagged.rs:5:17
[INFO] [stdout]   |
[INFO] [stdout] 5 | use core::ops::{Deref, DerefMut, Range};
[INFO] [stdout]   |                 ^^^^^  ^^^^^^^^
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: `self` parameter type does not contain `Self`
[INFO] [stdout]    --> src/lowtagged.rs:129:29
[INFO] [stdout]     |
[INFO] [stdout] 129 |     pub fn write_data(self: &mut TaggedRefLow<'a, T>, data: usize) {
[INFO] [stdout]     |                             ^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = help: use `&self`, `&mut self`, or `self: &Self` for correct lifetime elision
[INFO] [stdout]     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
[INFO] [stdout]     = note: for more information, see issue #140611 <https://github.com/rust-lang/rust/issues/140611>
[INFO] [stdout]     = note: `#[deny(self_lifetime_elision_not_applicable)]` (part of `#[deny(future_incompatible)]`) on by default
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: `self` parameter type does not contain `Self`
[INFO] [stdout]    --> src/lowtagged.rs:176:31
[INFO] [stdout]     |
[INFO] [stdout] 176 |     pub fn extract_data(self: &TaggedMutRefLow<'a, T>) -> usize {
[INFO] [stdout]     |                               ^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = help: use `&self`, `&mut self`, or `self: &Self` for correct lifetime elision
[INFO] [stdout]     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
[INFO] [stdout]     = note: for more information, see issue #140611 <https://github.com/rust-lang/rust/issues/140611>
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: `self` parameter type does not contain `Self`
[INFO] [stdout]    --> src/lowtagged.rs:181:29
[INFO] [stdout]     |
[INFO] [stdout] 181 |     pub fn write_data(self: &mut TaggedMutRefLow<'a, T>, data: usize) {
[INFO] [stdout]     |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]     |
[INFO] [stdout]     = help: use `&self`, `&mut self`, or `self: &Self` for correct lifetime elision
[INFO] [stdout]     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
[INFO] [stdout]     = note: for more information, see issue #140611 <https://github.com/rust-lang/rust/issues/140611>
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] warning: associated function `sign_extend_47` is never used
[INFO] [stdout]   --> src/hightagged.rs:95:8
[INFO] [stdout]    |
[INFO] [stdout] 43 | impl<T> TaggedPtrHigh48<T> {
[INFO] [stdout]    | -------------------------- associated function in this implementation
[INFO] [stdout] ...
[INFO] [stdout] 95 |     fn sign_extend_47(val: usize) -> usize {
[INFO] [stdout]    |        ^^^^^^^^^^^^^^
[INFO] [stdout]    |
[INFO] [stdout]    = note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stderr] error: could not compile `ptagger` (lib) due to 3 previous errors; 3 warnings emitted
[INFO] [stdout] warning: associated function `sign_extend_47` is never used
[INFO] [stdout]   --> src/hightagged.rs:95:8
[INFO] [stdout]    |
[INFO] [stdout] 43 | impl<T> TaggedPtrHigh48<T> {
[INFO] [stdout]    | -------------------------- associated function in this implementation
[INFO] [stdout] ...
[INFO] [stdout] 95 |     fn sign_extend_47(val: usize) -> usize {
[INFO] [stdout]    |        ^^^^^^^^^^^^^^
[INFO] [stdout]    |
[INFO] [stdout]    = note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stderr] warning: build failed, waiting for other jobs to finish...
[INFO] [stderr] error: could not compile `ptagger` (lib test) due to 3 previous errors; 3 warnings emitted
[INFO] running `Command { std: "docker" "inspect" "6ddf651ad52ef0d3c479726084ea28e900564b0c0e0557a054af7b0ae8300310", kill_on_drop: false }`
[INFO] running `Command { std: "docker" "rm" "-f" "6ddf651ad52ef0d3c479726084ea28e900564b0c0e0557a054af7b0ae8300310", kill_on_drop: false }`
[INFO] [stdout] 6ddf651ad52ef0d3c479726084ea28e900564b0c0e0557a054af7b0ae8300310

@Paladynee

[TKanX]

@lcnr what should we do next?

[scottmcm]

We chatted a bunch in the lang meeting today about the errors like this one:

[INFO] [stdout] error: `self` parameter type does not contain `Self`
[INFO] [stdout]    --> src/lowtagged.rs:176:31
[INFO] [stdout]     |
[INFO] [stdout] 176 |     pub fn extract_data(self: &TaggedMutRefLow<'a, T>) -> usize {
[INFO] [stdout]     |                               ^^^^^^^^^^^^^^^^^^^^^^^

Notably, that case just has usize as a return type, which contains no lifetimes, and similarly a bunch of the other errors have () as the return type, which of course also contains no lifetimes.

So there's two reasons that it seems like that shouldn't actually be FCWing:

• There's no lifetime elision going on, in a sense, because with no output lifetimes there's nothing being tied together, and thus there's no reason that this would be an issue.
• Or if you do think of this as lifetime elision, it's the kind that doesn't care about the self rule because if it was an associated function instead of a method, this would do exactly the same thing.

Thus it feels like there's no reason we'd need to change this case, at least in the context of a PR titled involving "lifetime elision". (There was as aside about maybe linting "that's not your self type?" anyway as a stylistic thing, but that didn't reach team consensus and probably isn't this issue anyway.)

We were thinking thus that

• This should exempt those cases without lifetimes in the return type.
• Then we can re-run crater on just the things that failed, which should be a quick run, to hopefully reduce the total impact here.

Does that make sense? @lcnr, would that impact the original goal of doing this? It seemed to us like if there are only input '_ lifetimes, they should just be fresh ones, and it didn't seem like we'd have to break those cases.

[lcnr]

Does that make sense? @lcnr, would that impact the original goal of doing this?

I haven't been following this too closely, so I can't tell. I think this change seems fine

The issue is that trying to decide whether types are equal on the HIR does not work, so trying to distinguish between "reference to self type" vs "reference in self type" and so on is just incredibly hard.

I could definitely imagine us supporting self: &TaggedMutRefLow<'a, T> and only emitting a FCW if there is actually any lifetime elision which may depend on the self type.

My vibe is:

• i don't have a good sense of what would be a good end state here
• it's certainly safer to have less elision for now and force things to ambig
• explicitly telling users when lifetime elision does something surprising is very desirable to me, i.e. linting on f1, f2, and f5

[traviscross]

This makes sense to me. Thanks @TKanX for putting this forward. I'll file a concern for what @scottmcm mentioned:

This should exempt those cases without lifetimes in the return type.

As a clarification that we discussed in the meeting, our intended direction here is not to remove these cases from the language, but simply to move people off of them so that we can later change the behavior to be the one we believe to be correct (of course, we may first bump this lint to deny-by-default in order to get there).

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• should-example-cases-without-lifetimes (fix(resolve): Warn on Self-Type Lifetime Elision for Non-Self Types #153692 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

@rfcbot fcp concern should-example-cases-without-lifetimes

[nikomatsakis]

@rfcbot reviewed

[rustbot]

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[TKanX]

@rfcbot fcp concern should-example-cases-without-lifetimes

d1ab06e

@traviscross @scottmcm

[traviscross]

@bors try

TODO: @craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-153692/retry-regressed-list.txt p=1

[rust-bors]

☀️ Try build successful (CI)
Build commit: 48c9d93 (48c9d93cf65d550d124c40108859fd993778c157, parent: 4b9792692fbb675174d4d2082e7c37b2bc930e71)

[traviscross]

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-153692/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-153692-1 created and queued.
🤖 Automatically detected try build 48c9d93
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-153692-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-153692-1 is completed!
📊 151 regressed and 0 fixed (7183 total)
📊 887 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-153692-1/retry-regressed-list.txt ↩