Add lint: `&mut Commands` function argument

[BD103]

Quoting @ItsDoot in https://github.com/bevyengine/bevy/issues/15657:

How can Bevy's documentation be improved?

We should more clearly and directly recommend that developers write their helper functions to take Commands (and friends) directly, instead of passing as references (&Commands). And by doing so, these helpers can look just like systems. As an example:

fn my_system(commands: Commands, foo: Query<&Foo>) {
    // we should recommend this:
    good_helper(commands.reborrow(), foo.reborrow());
    // instead of this:
    bad_helper(&mut commands, &foo);
}

// This is kind of annoying:
fn bad_helper(commands: &mut Commands, foo: &Query<&Foo>) {
    // ...
}

// This is easier to deal with:
fn good_helper(commands: Commands, foo: Query<&Foo>) {
    // ...
}

We should recommend reborrowing in the top level docs for Commands and Query.

We can write a lint that would warn against taking &mut Commands in a function! It would probably be in the pedantic category, though.

[janhohenheim]

TIL about reborrow 👀

[MrGVSV]

Working on this now!

[MrGVSV]

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.

[MrGVSV]

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?

[BD103]

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.

[MrGVSV]

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