Add lint against `.insert()` chaining.

[NiseVoid]

Calling multiple .insert()s causes more archetype moves and generally just distracts from how simple an operation is. This also applies if .spawn() is called first and .insert is called immediately after. In either of these cases the components can all be combined into a tuple, if the tuple exceeds the limit (16 components) it can be made into a tuple of tuples. Outside of maybe some niche test cases, these commands should always be combined into a single spawn or insert call.

I've come across a lot of bevy examples and third party crates doing this, and because of that many people end up copying this. A lot of this likely stems from pre-0.9, but ever since 0.9 combining them has been the more idiomatic approach (https://bevyengine.org/news/bevy-0-9/#improved-entity-component-apis).

An example diff of spawn + insert + insert -> a single spawn

-commands
-    .spawn(Transform::from_matrix(VOXEL_FROM_WORLD))
-    .insert(IrradianceVolume {
-        voxels: assets.irradiance_volume.clone(),
-        intensity: IRRADIANCE_VOLUME_INTENSITY,
-    })
-    .insert(LightProbe);
+commands.spawn((
+    Transform::from_matrix(VOXEL_FROM_WORLD),
+    IrradianceVolume {
+        voxels: assets.irradiance_volume.clone(),
+        intensity: IRRADIANCE_VOLUME_INTENSITY,
+    },
+    LightProbe,
+));

An example of insert + insert -> a single insert

 commands
     .entity(entity)
-    .insert(Transform::from_matrix(VOXEL_FROM_WORLD))
-    .insert(LightProbe);
+    .insert((
+        Transform::from_matrix(VOXEL_FROM_WORLD),
+        LightProbe,
+    ));

[BD103]

This sounds like a useful lint, but we really need to define when it should be denied.

• How does this lint interact with other EntityCommands::insert_*() methods, such as insert_by_id()?
• How does it interact with other entity commands, such as clear(), remove(), and try_insert()?

• What if the EntityCommands is stored in a variable, then accessed later?

  let mut ec = commands.entity(id);
  ec.insert(Foo);
  // There may or may not be more code here.
  ec.insert(Bar(x));

If we can get a clear set of rules laid down for how this lint operates, I'm all for implementing it!

[NiseVoid]

• How does this lint interact with other EntityCommands::insert_*() methods, such as insert_by_id()?

We could suggest the same thing for any version with a grouped version, but it might be better to leave them out of scope because that change is potentially non-trivial and insert_by_ids is significantly easier to get wrong.

• How does it interact with other entity commands, such as clear(), remove(), and try_insert()?

If try_insert has the same implications for a bundle as separate calls we can group them. remove can also be grouped (.remove::<(A, B)>() instead of .remove::<A>().remove::<B>()).

For any command that doesn't otherwise have important ordering (like .id(), .with_children(), etc) we could still recommend grouping commands on either side of them in a chain.

Any commands like clear and retained that make the order important should make it so that the lint does not show up as they cannot be grouped without changing the functionality (because insert(A).clear().insert(B) is not the same as insert((A, B)).clear() or .clear().insert((A, B)), tho in theory we could recommend removing the insert(A) entirely).

• What if the EntityCommands is stored in a variable, then accessed later?

If there is no branching in between we could recommend the same, all the values could simply be gathered and inserted once at the end. If there is branching however .insert()s become very hard to group.

There are some cases like this that are possible:

if !entity_mut.contains::<Transform>() {
    entity_mut.insert((SpatialBundle::default(), scene_handle));
} else {
    entity_mut.insert(scene_handle);
}

but imo we shouldn't recommend this since command batching would probably make this the uglier solution later down the line, and required components should already make most of these unnecessary

[janhohenheim]

Don't you need chained inserts for inserting two bundles that contain an overlapping component? Same for spawn -> insert.

[NiseVoid]

Correct, but inserting overlapping components is almost never desirable, not to mention that bundles are already on their way out :thinking:

[janhohenheim]

Sure it's undesirable, but lots of 3rd party bundles contain Transform, so it happens very quickly when using dependencies 😅 and AFAIK bundles are still going to be used as part of third party APIs, last I heard? My info might be outdated tho