search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.91k stars 12.68k forks source link

Tracking issue for future-incompatibility lint `elided_lifetimes_in_associated_constant` #115010

Open compiler-errors opened 1 year ago

compiler-errors commented 1 year ago

This is a tracking issue for the elided_lifetimes_in_associated_constant future-incompatibility lint.

The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What

The elided_lifetimes_in_associated_constant lint detects elided lifetimes that became erroneously allowed in associated constants after #97313.

struct Foo;
impl Foo {
    const STR: &str = "hello, world";
}

They current desugar to fresh early-bound lifetimes on the parent impl, like:

struct Foo;
impl<'a> Foo {
    const STR: &'a str = "hello, world";
}

This is in contrast to the way that elided lifetimes are treated in item-level consts (where elided lifetimes are resolved to 'static), and this behavior was also never formally decided -- static-in-const lifetime rules do not apply to associated consts (https://github.com/rust-lang/rfcs/pull/1623#issuecomment-239559757, https://github.com/rust-lang/rust/issues/38831#issuecomment-305864245).

Why

It was never decided what to do with elided lifetimes in consts, and it is not clear that the current behavior is optimal here. This is to stop the leaking by making sure existing elided lifetimes are fixed back to using 'static as they were required to before version 1.64, and that new usages of elided lifetimes in consts are not introduced.

How to fix

Replace the elided lifetimes with 'static (or manually introduce or reference another lifetime):

struct Foo;
impl Foo {
    const STR: &'static str = "hello, world";
}

Tracking

jonathanstrong commented 1 year ago

I'm old enough to remember when clippy used to nag me to change &'static str to &str for consts. Ugh.

tgross35 commented 1 year ago

Is it possible to loosen the implicit lifetime to 'static lifetime for literals at least? This change is kind of churny and doesn't really seem to align with expected behavior.

compiler-errors commented 1 year ago

@tgross35: that's behavior that @rust-lang/lang needs to stabilize separately. I'll nominate this issue to see if opinions have changed since #38831, but this lint is specifically aimed at making sure people are not relying on behavior that was never formally stabilized.

Question for T-lang: What should we do here?

repi commented 1 year ago

The clippy warn-by-default lint redundant_static_lifetimes conflicts with this warning with latest nightly, so users would have to disable one or the other which is unfortunate.

compiler-errors commented 1 year ago

@repi: The redundant_static_lifetimes lint does not trigger on associated consts: https://github.com/rust-lang/rust/blob/b2515fa741eea89b82a2c94048f934bbcbd3bd48/src/tools/clippy/clippy_lints/src/redundant_static_lifetimes.rs#L104-L108 (and should have been fixed since rust-lang/rust-clippy#2443)

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a3f5fbfff746ad971f4ca458633990e6

Can you share code where you encountered a conflict between this future-compat warning and the clippy lint?

repi commented 1 year ago

dug a bit deeper and I was wrong, this clippy lint indeed doesn't trigger on associated consts. sorry.

was able to resolve all of our lint warnings on this without clippy::redundant_static_lifetimes triggering.

compiler-errors commented 1 year ago

Lang team consensus is that we should go with (A.) above:

A. Continue with this lint, accepting churn, with the possibility of relaxing this (i.e. resolving '_ to 'static in associated consts) in the future

Since this was inadvertently stabilized and a separate RFC would need to be authored if it's desired to make elision work in associated consts.

See notes here: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Triage.20meeting.202023-08-29/near/388257641

QuineDot commented 1 year ago

Heads up: This lint as currently written accepts code that is not accepted in Rust 1.72 (nor 1.63).

From 1.64 to current stable, anonymous lifetimes were allowed, but they weren't specifically 'static; instead, each one was independent. The independent-ness caused things like nested elided lifetimes and ambiguous trait bounds for trait objects to fail.

Under the change which accompanied this lint, elided lifetimes are no longer independent, they are specifically 'static. This allows more things to compile.

Example (Rust 1.63 fails; Rust 1.72 fails; nightly succeeds):

struct S;
impl S {
    const C: &&str = &"";
}

Perhaps this will be considered fine since the lint is supposedly going to be deny, but I thought I should mention it. If the lint doesn't go the way you anticipate, you may be unwittingly opting into a specific behavior ('static).

The change also means that contravariant cases that worked from 1.64-1.72 fail on nightly:

struct Contra<'a>(PhantomData<fn(&'a str)>);

struct S;
impl S {
    const C: Contra<'_> = Contra(PhantomData);
}

fn f<'a>() {
    let _: Contra<'a> = S::C;
}

But probably that's not ecosystem-breaking like the covariant case is.

More examples that probably add nothing
[The last three examples in this section](https://quinedot.github.io/rust-learning/dyn-elision-trait-bounds.html#static-contexts) (before "`impl` Headers") don't compile in Rust 1.63 (prior to #97313 landing) due to the use of `'_`. The first example compiles from 1.64 to current stable, but the second two examples did not. *All three compile on nightly (with this lint).* --- [This example fails on 1.63 due to the `'_` in `dyn Single<'_>`.](https://rust.godbolt.org/z/d8aTaq9Ea) It [compiles on 1.72.](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7212efbde2948f1769dce35986253f7b) ```rust use core::marker::PhantomData; trait Single<'a>: 'a + Send + Sync {} struct L<'l, 'm>(&'l str, &'m str); impl<'a, 'b> L<'a, 'b> { const ECS: PhantomData>> = PhantomData; const SCS: PhantomData>> = PhantomData; const S_ECS: PhantomData + 'static>> = Self::ECS; const S_SCS: PhantomData + 'static>> = Self::SCS; } ``` [This example fails on 1.63 due to the `'_` in `dyn Double<'a, '_>`.](https://rust.godbolt.org/z/b3c83EbEj) It [fails on 1.72](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1e4f554acbbb39b01905549ff45d301e) because it considers the elided trait object lifetime ambiguous in the face of two independent lifetimes. ```rust use core::marker::PhantomData; trait Double<'a, 'b>: 'a + 'b + Send + Sync {} struct L<'l, 'm>(&'l str, &'m str); impl<'a, 'b> L<'a, 'b> { const EBCD: PhantomData>> = PhantomData; } ``` [This example fails on 1.63](https://rust.godbolt.org/z/6fdPEbh3n) due to the use of `'_` and eliding `&` lifetimes. [It fails on 1.72](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c7806531eae921cd56875d91f05d000b) because some of the anonymous lifetimes aren't considered to be implicitly longer than the reference lifetime. (N.b. the spans are also broken in the error. Comment out the `ECS` case to see better spans on the `RECS` case.) ```rust use core::marker::PhantomData; trait Single<'a>: 'a + Send + Sync {} struct R<'l, 'm, 'r>(&'l str, &'m str, &'r ()); impl<'a, 'b, 'r> R<'a, 'b, 'r> where 'a: 'r, 'b: 'r { const ECS: PhantomData<&dyn Single<'_>> = PhantomData; const RECS: PhantomData<&'r dyn Single<'_>> = PhantomData; } ```
compiler-errors commented 1 year ago

@QuineDot: thanks for the comment.

Yeah, I'm aware of this, and chose to lower to 'static to avoid additional errors like #114706, but yeah, perhaps we shouldn't allow this. I'll modify the warning code to continue to lower to fresh lifetimes to preserve 1.64-1.73 behavior behind the warning.

holly-hacker commented 1 year ago

Apologies if I miss something obvious here, I've only been using Rust for a few years and haven't touched its internals, but why can this...

struct Foo;
impl Foo {
    const STR: &str = "hello, world";
}

... not be desugared into this?

struct Foo;
impl Foo {
    const STR: &'static str = "hello, world";
}

Is this some limitation of the compiler, or is this a backwards-incompatible change? If it's backwards-incompatible, isn't it better to change this behavior in a Rust edition?

I find it strange that const fields can have non-'static lifetimes in the first place, so I assume (nearly) nobody is depending on the fact that their associated consts have a lifetime that may be shorter than 'static.

tgross35 commented 1 year ago

Apologies if I miss something obvious here, I've only been using Rust for a few years and haven't touched its internals, but why can this...

[...]

... not be desugared into this?

[...]

@holly-hacker the problem is that this is loosening bounds from the current behavior, which needs to go through the review process since it is part of a bigger picture. The notes where this was discussed are here: https://hackmd.io/4br69DHGRy2EIm-JY5dTgw#%E2%80%9CTracking-Issue-for-ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT-future-compat-lint%E2%80%9D-rust115010, I think that the RFC linked there illustrate some of the potential issues.

It seems like generally everyone is in agreement that the current behavior is not expected, it's just not yet clear how to make expected and actual behavior align. It might be possible to special-case something for the common cases - but those discussions and decisions will take time (I just opened some discussion on Zulip about this)

Is this some limitation of the compiler, or is this a backwards-incompatible change? If it's backwards-incompatible, isn't it better to change this behavior in a Rust edition?

Lints can be added outside of edition boundaries since they can always be disabled (editions are usually things that aren't configurable)