Elided lifetime changes in rust_2018_idioms lint is very noisy and results in dramatically degraded APIs for Bevy

[alice-i-cecile]

Problem

With the upcoming release of Rust 2024 edition, we're concerned that rust_2018_idioms will be deny by default.

[Editorial comment (TC): This is not an edition item for Rust 2024, and the edition is not accepting any new items, so we can say definitely that this will not be tied to the release of Rust 2024. See here.]

We investigated what these changes will entail for Bevy in bevyengine/bevy#15916, and the impact is quite severe. Our primary user-facing system and query APIs are littered with meaningless lifetimes.

This is a much worse experience with no upside for us, and Bevy and its entire ecosystem will have to manually allow this lint on every project.

Proposed solution

We would appreciate if the elided lifetimes lint could be split out from the rest of the rust_2018_idioms linting, which we generally liked the effect of.

Ideally this would be off by default as well, to avoid needing to teach new users to turn it off as a critical part of project setup.

[alice-i-cecile]

Here's an example of some in-production code after this lint was applied:

pub fn update_text2d_layout(
    // Text items which should be reprocessed again, generally when the font hasn't loaded yet.
    mut queue: Local<'_, HashSet<Entity>>,
    mut textures: ResMut<'_, Assets<Image>>,
    fonts: Res<'_, Assets<Font>>,
    windows: Query<'_, '_, &Window, With<PrimaryWindow>>,
    mut scale_factor_changed: EventReader<'_, '_, WindowScaleFactorChanged>,
    mut texture_atlases: ResMut<'_, Assets<TextureAtlasLayout>>,
    mut font_atlas_sets: ResMut<'_, FontAtlasSets>,
    mut text_pipeline: ResMut<'_, TextPipeline>,
    mut text_query: Query<'_, '_, (
        Entity,
        Ref<'_, TextLayout>,
        Ref<'_, TextBounds>,
        &mut TextLayoutInfo,
        &mut ComputedTextBlock,
    )>,
    mut text_reader: Text2dReader<'_, '_>,
    mut font_system: ResMut<'_, CosmicFontSystem>,
    mut swash_cache: ResMut<'_, SwashCache>,
) {

[jieyouxu]

Since this seems to have potential non-minimal ecosystem impact, nominating for T-lang discussion.
@rustbot label +I-lang-nominated

[alice-i-cecile]

Turning this on by default is tracked in #54910 and #91639.

[epage]

I recently found that I didn't have rust_2018_idioms set to warn in my projects and did so. I found I had many elided lifetimes and I almost always found it to made the code harder to read as the signal to noise ratio went down. I was a big fan of the explicitness back when it first came out but it seems like its actually redundant in many cases.

Take a Display impl.

impl Display for PartialVersion {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        // ...
    }
}

The fact that data is being borrowed has little use for the person reading or writing this function. This might seem like a trivial case but its also a case that new users will be more likely to run into and requires introducing more concepts for them to write basic idiomatic code.

I can see more of a case for it in return values as it the fact that a borrow occurs there is much more likely to affect people.

[RalfJung]

I can see more of a case for it in return values as it the fact that a borrow occurs there is much more likely to affect people.

Yeah, I agree -- borrows that only last for the duration of the call are very non-intrusive and can often be ignored. But when lifetimes show up in the return value, they can start impacting how one has to arrange the code on the caller side.

@alice-i-cecile does Bevy have any cases of elided lifetimes in return types where you think elision is desirable, or does it only affect argument types? (I guess this may be hard to say because the lint fires so often that you don't notice the return type cases among the noise...)

[jhpratt]

For what it's worth, I've had this set to deny for years in my projects and have found it to be useful. That's not to say that I'm opposed to splitting the lint up, however.

The lint may be able to be improved to where it's actually necessary, but my assumption was that something like this was intended to become a hard error in the future, just as other edition changes are.

[djc]

For the sake of discussion, I want to challenge this issue a bit. I don't want to diminish the impact this has on people new to Rust, but I do think we should be careful about this change, and this feels like a pretty kneejerk response ("very noisy", "dramatically degraded", "will have to manually allow").

We investigated what these changes will entail for Bevy in bevyengine/bevy#15916, and the impact is quite severe. Our primary user-facing system and query APIs are littered with meaningless lifetimes.

This is a much worse experience with no upside for us, and Bevy and its entire ecosystem will have to manually allow this lint on every project.

I also don't think code sample is a good defense of the status quo versus this lint:

pub fn update_text2d_layout(
    // Text items which should be reprocessed again, generally when the font hasn't loaded yet.
    mut queue: Local<'_, HashSet<Entity>>,
    mut textures: ResMut<'_, Assets<Image>>,
    fonts: Res<'_, Assets<Font>>,
    windows: Query<'_, '_, &Window, With<PrimaryWindow>>,
    mut scale_factor_changed: EventReader<'_, '_, WindowScaleFactorChanged>,
    mut texture_atlases: ResMut<'_, Assets<TextureAtlasLayout>>,
    mut font_atlas_sets: ResMut<'_, FontAtlasSets>,
    mut text_pipeline: ResMut<'_, TextPipeline>,
    mut text_query: Query<'_, '_, (
        Entity,
        Ref<'_, TextLayout>,
        Ref<'_, TextBounds>,
        &mut TextLayoutInfo,
        &mut ComputedTextBlock,
    )>,
    mut text_reader: Text2dReader<'_, '_>,
    mut font_system: ResMut<'_, CosmicFontSystem>,
    mut swash_cache: ResMut<'_, SwashCache>,
) {

Here is a 12-argument function where all of the arguments' types take at least one lifetime, and one of the arguments also takes a tuple as a generic with another 5 nested type arguments. Of the 869 characters, 70 are "used up" by lifetime annotations (including suffix space). Some of these types have names that don't seem very meaningful without a bunch of context (Res, ResMut, Ref), and yet it feels like you're arguing that the lifetime annotations will be a substantial (relative) factor to inhibit understanding this code.

(I recently participated in a trait debugging research study, and one of the "tests" was a code sample relying on Bevy's IntoSystemConfigs trait implementations, and even with my substantial experience -- although not having used Bevy at all -- I ended up being unable to address the compiler error.)

Note that I was not a fan of this change when it was initially introduced in the 2018 edition (when I was a much less experienced Rustacean). However, I've since run into issues (which I unfortunately can't recall exactly right now) where I definitely got confused by the omission of lifetime arguments which were present in some arguments. Especially for owned argument types, IMO the lifetime parameter is an important signal that the lifetime of the value is limited, since in my mind at least the nature of Rust's ownership usually confers 'static ownership.

[koute]

If I had any influence over Rust (I don't) then this would be one of the very few things I would hard veto. YMMV, but for me personally the extra lifetime annotations are completely useless noise. I don't want them in my code. This is not kneejerk reaction. I enabled this lint on one of my private projects that's over 100k lines of code (similar to what Bevy did) and after seeing the result I'm 100% confident I don't want it. I like that the lifetimes are elided as this makes the code easier to read for me, and every time I tweak lifetimes I only have to change them in a few places instead of literally changing them in hundreds (or even thousands) of places all over my codebase. I would probably go as far as to say that if this ever becomes a deny-by-default lint that cannot be disabled then I will keep on permanently using a Rust edition where it can be disabled.

So I fully agree with Bevy's stance here and understand why they don't want this lint either.

Rust already has a reputation of being ugly and noisy with lifetimes, this is just going to make it worse. I frequently see people in the wild (experienced programmers from other languages) complaining that you need too many lifetime annotations in Rust, that they're ugly, that the annotations are viral and "infect" the codebase (their words, not mine), etc.; I almost never see people complaining that we don't have enough of them. That said, I don't hang out in places where Rust beginners congregate, so if the consensus is that the extra elision can be a problem for beginners then so be it. That might indeed be a good reason to introduce the extra lifetime annotations in principle. But I don't understand why we can't fix the problem through better error messages and improved IDE support?

(I recently participated in a trait debugging research study, and one of the "tests" was a code sample relying on Bevy's IntoSystemConfigs trait implementations, and even with my substantial experience -- although not having used Bevy at all -- I ended up being unable to address the compiler error.)

So would having this lint enabled made a difference and allowed you to address the compilation error? And couldn't the same be accomplished by having a better error message?

[djc]

and every time I tweak lifetimes I only have to change them in a few places instead of literally changing them in hundreds (or even thousands) of places all over my codebase.

This is definitely a pain. I'd argue that this could potentially also be addressed with better tooling, like a Rust-Analyzer action that adds the lifetime annotation for you (maybe that already exists? I haven't needed to do this a lot).

(I recently participated in a trait debugging research study, and one of the "tests" was a code sample relying on Bevy's IntoSystemConfigs trait implementations, and even with my substantial experience -- although not having used Bevy at all -- I ended up being unable to address the compiler error.)

So would having this lint enabled made a difference and allowed you to address the compilation error? And couldn't the same be accomplished by having a better error message?

In the study, the problem was not with lifetimes -- I relayed this anecdote to confer that I think this particular issue is of relatively minor importance when it comes to making writing/debugging Bevy code.

It is of course possible that better diagnostics would allow me to understand the problem more easily, but I still feel like lifetime annotations are an important signal that help me better understand the code.

[GoldsteinE]

For what it's worth, I've had this set to deny for years in my projects and have found it to be useful. That's not to say that I'm opposed to splitting the lint up, however.

I think it heavily depends on the project. Bevy is somewhat of a worst case here: it has a lot of lifetimes in paths (which is already not typical for much of Rust code), and most of them are just “for the duration of this function”, so you nearly never actually care about them. I think having the lint is valuable for some projects, but hard-denying it is going to break some valid usecases — like Bevy.

Of the 869 characters, 70 are "used up" by lifetime annotations

I don’t think it’s about character count. EventReader<'_, '_, WindowScaleFactorChanged> is just harder to read than EventReader<WindowScaleFactorChanged> while not adding much relevant information (I’d argue that the second '_ doesn’t add any relevant information at all — we already know that the struct is borrowed in some way, knowing that there’re two different lifetimes somewhere inside doesn’t add much value).

like a Rust-Analyzer action that adds the lifetime annotation for you

That would create a giant diff that would be a huge pain to review and which would pollute git blame when merged. Automating making irrelevant changes helps a bit, but irrelevant changes are still a problem.

[laundmo]

Some of these types have names that don't seem very meaningful without a bunch of context (Res, ResMut, Ref), and yet it feels like you're arguing that the lifetime annotations will be a substantial (relative) factor to inhibit understanding this code.

One of the greatest features of Bevy, especially for people new to Rust and learning it through Bevy like I did, is that it abstracts away lifetimes quite successfully.

Res and ResMut are core to how Bevy does this - they are types which any Bevy user will learn quite quickly and ultimately are pretty simple for said user: "Ask Bevy to provide the Resource (mutably)". This is a far easier to grasp than the necessity of '_ everywhere.

I also don't think code sample is a good defense of the status quo versus this lint

While this is a worst-case example, many smaller function signatures would also be affected. Basically, every user-provided function which uses Bevys Dependency Injection pattern, which for many projects may well be almost every function, will struggle with this. A simple setup function has this diff:

- fn setup(mut commands: Commands) {
+ fn setup(mut commands: Commands<'_, '_>) {

A slightly more complex setup function

fn setup(
-    mut commands: Commands,
+    mut commands: Commands<'_, '_>,
-    mut meshes: ResMut<Assets<Mesh>>,
+    mut meshes: ResMut<'_, Assets<Mesh>>,
-    mut materials: ResMut<Assets<StandardMaterial>>,
+    mut materials: ResMut<'_, Assets<StandardMaterial>>,
) {

Basic game entity movement

- fn move_cube(mut cubes: Query<(&mut Transform, &mut CubeState)>, timer: Res<Time>) {
+ fn move_cube(mut cubes: Query<'_, '_, (&mut Transform, &mut CubeState)>, timer: Res<'_, Time>) {

These changes make Rust far less readable for, often, relatively little gain. What purpose is a lint like this if the default recommendation for everyone starting to use the language will inevitably become "remember to allow this lint"?

[shepmaster]

Much of this discussion has already occurred in the tracking issue. I highly encourage people to read all the comments (yes, even the hidden ones!) and contribute over there to keep discussion together.

Highlights include the fact that it's generally understood that the lint needs to be split into smaller parts and that there's work underway to do so. A quick demo of what warnings might look like for Bevy is even available.

[alice-i-cecile]

does Bevy have any cases of elided lifetimes in return types where you think elision is desirable, or does it only affect argument types? (I guess this may be hard to say because the lint fires so often that you don't notice the return type cases among the noise...)

We'd generally be fine with this. We're not returning things with lifetimes as part of the normal use of our API, and I think that this communicates much more important information. Returning something with a lifetime has implications on how it can be used :)

[cart]

Frankly if this became the default user experience for consumers of the bevy crate, I would immediately start considering other language options.

[cart]

(the return position lint seems fine though)

[cart]

Looks like I jumped the gun here / didn't read all of the comments (there is no intent to ship the lint causing everyone concern). Sorry for that. You'll have to forgive me for acting swiftly / rashly as this amounts to an existential threat to hundreds of peoples work over the course of many years.

[traviscross]

@rustbot labels -A-edition-2024

@alice-i-cecile: If you could, could you please link to what led you to believe this is true?:

With the upcoming release of Rust 2024 edition, rust_2018_idioms will be deny by default.

For reference, this program is currently accepted in nightly Rust 2024:

//@ edition: 2024
//#![deny(rust_2018_idioms)]

struct S<'a>(&'a ());

fn f(x: &()) -> S {
    S(x)
}

fn main() {}

Playground link

There are no open edition items for Rust 2024 that would change this behavior.

[alice-i-cecile]

This was due to the presence of #54910 in the Lang Edition 2024 project :)

[traviscross]

Thanks, that's helpful. It was just an idea. There were a lot of ideas that didn't become actual edition items.

We've passed the go / no go date for all edition items for Rust 2024. The probability of this change happening in a way tied to the release of the Rust 2024 edition is now zero.

[musjj]

Will anything change for async functions: #103470?

Right now, elided lifetimes is a hard error for async functions:

struct Foo<'a> {
    a: &'a String,
}

fn hello(_foo: Foo) {} // no warning

async fn hello(_foo: Foo) {} // implicit elided lifetime not allowed here

It feels really confusing for it to be allowed in regular functions, but not async ones.

[shepmaster]

My take is that it's unlikely to change in the near future.

I have discussion of choices in my pull request (look for "Existing hard errors").