TIL about reborrow 👀

Working on this now!

I think if we're going to standardize on this via a lint, it would make sense to extend it to all re-borrwable types, including (in 0.15):

• bevy::ecs::change_detection::MutUntyped
• bevy::ecs::prelude::ResMut
• bevy::ecs::prelude::NonSendMut
• bevy::ecs::prelude::Mut
• bevy::ecs::prelude::Commands
• bevy::ecs::prelude::EntityCommands
• bevy::ecs::prelude::Query
• bevy::ecs::prelude::Deferred
• bevy::ecs::prelude::EntityMut
• bevy::ecs::prelude::FilteredResourcesMut
• bevy::ecs::ptr::PtrMut
• bevy::ecs::world::DeferredWorld
• bevy::ecs::world::FilteredEntityMut
• bevy::ecs::world::EntityMutExcept
• bevy_ecs::query::iter::QueryIterationCursor

Additionally, as raised by @tim-blackbird here, there are some cases where this doesn't make sense. Specifically, immutable references. In some cases, it makes more sense for a helper to take &Query rather than a re-borrowed Query, such as when already immutably iterating through the Query.

Therefore, we should probably only apply this lint where we are mutably borrowing the type.

Looks like we'd also need to decide how to handle the EntityCommands problem. Obviously, users could simply opt-out of the lint in such cases, but does that add more friction than desired for a lint like this?

Specifically, immutable references.

Do all reborrow() implementations require a mutable reference? If so, then we shouldn't lint on immutable references at all.

To my understanding, Commands internally holds a &mut Buffer to the actual data. We're linting against &mut Commands because it ends up being &mut &mut Buffer, which is slower and hurts caching. &&mut Buffer, on the other hand, is distinct because it is immutable. There shouldn't be a lint in this case because you cannot simplify the structure further.

As an aside, you should be able to determine if a reference is mutable or not using MutTy, which is accessible from rustc_hir::Ty::kind's Ref variant.

Specifically, immutable references.

Do all reborrow() implementations require a mutable reference? If so, then we shouldn't lint on immutable references at all.

Yeah I just wanted to explicitly call it out

But yeah before I go any further with my implementation, are we at all concerned about not being able to return references using a reborrowed instance?

// `EntityCommands<'1>` contains `Commands<'1, '1>`, which is why this example doesn't
// separate the `'w` and `'s` lifetimes. Though any configuration of lifetimes seems
// to result in similar issues.
fn helper<'a, 'b: 'a>(mut commands: Commands<'b, 'b>) -> EntityCommands<'a> {
    commands.spawn_empty()
}

error[E0515]: cannot return value referencing function parameter `commands`
  --> src/foo.rs:36:5
   |
36 |     commands.spawn_empty()
   |     --------^^^^^^^^^^^^^^
   |     |
   |     returns a value referencing data owned by the current function
   |     `commands` is borrowed here

The same goes for other types like Query returning a reference to components or ResMut returning a mutable reference to a field.

Do we see this as a reason not to add the lint? Or are these just considered exceptions to the lint?

Or perhaps should the lint attempt to check if using a reborrowed parameter would result in the function's return not being possible? I'm not immediately sure how to check that. I'm not sure we can just compare the lifetimes, since the real issue is that we're returning a reference to data "owned" by the function. Maybe there's a way to check for that specifically? Actually, I think we can just see if we're returning any data with a lifetime tied to the reference 🤔

Anyways, yeah curious to hear others' thoughts on this particular problem.

Do we see this as a reason not to add the lint? Or are these just considered exceptions to the lint?

I think the lint is still generally useful, so we should definitely keep it, but it should never warn on code that cannot be fixed.

For this case, if you cannot guarantee the user will be able to fix the lint, it should not be emitted in the first place.

It's much less annoying for a lint to miss a few lintable pieces of code than for a lint to warn on too many pieces of code.

To draw this to a conclusion, though, it's ok for the lint to have a few bugs in it! We're both still learning, so it's fine if there's an unaccounted edge case that slips through. Hopefully users will be able to test the linter enough to iron them out. :)

For this case, if you cannot guarantee the user will be able to fix the lint, it should not be emitted in the first place.

Makes sense. This is the approach I tried to take in my PR. The goal is that we skip over these "impossible" cases.