Aggressively use let-else and combinators to reduce indentation #7104

james7132 · 2023-01-06T01:34:25Z

Objective

Improve the code quality of Bevy. The heavy use of if let X = ... results in a relentless push to the right as we nest more and more layers inward. This PR aims to reduce that.

Solution

Use let-else over if let X = ... where possible to reduce heavy indentation, as well as use Option and Result combinators.

This PR seemingly has a huge change, but most of these are indentation changes and no logic in the code has been changed.

james7132 · 2023-01-06T01:35:10Z

Tentatively marking this as controversial as it's a stylistic change with a lot of churn.

mockersf · 2023-01-06T01:37:55Z

I think this should wait for rust-lang/rustfmt#4914

IceSentry · 2023-01-06T07:21:15Z

Yeah, I really like let-else, but the fact that rustfmt doesn't work is a bit annoying right now. Fortunately the tracking issue for the style guide was closed yesterday so hopefully it doesn't block for too long.

james7132 · 2023-01-06T07:56:15Z

If it does block for too long, I may end up closing this and just remaking the PR from scratch, as this level of change is really prone to merge conflicts.

atlv24

This mostly looks good to me, and I would approve, although I'd be lying if i said my eyes didnt glaze over for the second half of this PR. Its a lot of code but I did not spot anything out of the ordinary or obviously wrong.

atlv24 · 2023-01-06T07:03:20Z

crates/bevy_asset/src/io/file_asset_io.rs

+            .unwrap_or_else(|_| {
+                env::current_exe()
+                    .map(|path| {
+                        path.parent()
+                            .map(|exe_parent_path| exe_parent_path.to_owned())
+                            .unwrap()
+                    })
+                    .unwrap()
+            })


Suggested change

.unwrap_or_else(|_| {

env::current_exe()

.map(|path| {

path.parent()

.map(|exe_parent_path| exe_parent_path.to_owned())

.unwrap()

})

.unwrap()

})

.or_else(|| {

env::current_exe()

.and_then(|path| path.parent())

.and_then(|parent_path| parent_path.to_owned())

}).unwrap()

unwrap_or_else does not ever panic, unless you panic in the closure yourself. Neither will or_else into unwrap, but it will create unreachable code for panicking that may not get optimized out under certain configurations.

or_else into unwrap can panic if the closure returns None

atlv24 · 2023-01-06T07:11:46Z

crates/bevy_asset/src/loader.rs

+        let asset = asset.downcast::<T>().unwrap_or_else(|_| {
            panic!(
                "Failed to downcast asset to {}.",
                std::any::type_name::<T>()
-            );
-        }
+            )
+        });


let Some(asset) = asset.downcast::<T>() else { panic!( "Failed to downcast asset to {}.", std::any::type_name::<T>() ) };

or even

let asset = asset.downcast::<T>().expect( format!( "Failed to downcast asset to {}.", std::any::type_name::<T>() ) );

but the latter may have performance impact of formatting even when we're not going to panic

atlv24 · 2023-01-06T07:21:41Z

crates/bevy_ecs/src/system/commands/mod.rs

+            .unwrap_or_else(|| {
+                panic!("error[B0003]: Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.", std::any::type_name::<T>(), self.entity);
+            })


i would suggest expect(format!()) style here but there may be a runtime cost of formatting every time regardless of whether we're going to panic or not

The perf impact is precisely why it's using unwrap_or_else here. We shouldn't be allocating when we aren't panicking.

Maybe breaking that panic message and args up into multiple lines, it's a bit much.

mockersf · 2023-01-06T08:12:42Z

If it does block for too long, I may end up closing this and just remaking the PR from scratch, as this level of change is really prone to merge conflicts.

Yeah this is going to conflict fast...

ghost

Take these with a grain of salt, and excuse me if my suggestions of map_or_else are incorrect.

ghost · 2023-01-06T12:55:30Z

crates/bevy_hierarchy/src/hierarchy.rs

-        }
+    let Some(mut children) = world.get_mut::<Children>(entity) else { return };
+    for e in std::mem::take(&mut children.0) {
+        despawn_with_children_recursive_inner(world, e);


How about:

/// Function for despawning an entity and all its children pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) { // first, make the entity's own parent forget about it let Some(parent) = world.get::<Parent>(entity).map(|parent| parent.0) else { return }; let Some(mut children) = world.get_mut::<Children>(parent) else { return }; children.0.retain(|c| *c != entity); // then despawn the entity and all of its children despawn_with_children_recursive_inner(world, entity); } // Should only be called by `despawn_with_children_recursive`! fn despawn_with_children_recursive_inner(world: &mut World, entity: Entity) { despawn_children(world, entity); if !world.despawn(entity) { debug!("Failed to despawn entity {:?}", entity); } } fn despawn_children(world: &mut World, entity: Entity) { let Some(mut children) = world.get_mut::<Children>(entity) else { return }; for e in std::mem::take(&mut children.0) { despawn_with_children_recursive_inner(world, e); } }

ghost · 2023-01-06T13:27:31Z

crates/bevy_ecs/src/world/mod.rs

@@ -739,11 +739,10 @@ impl World {
    /// Returns an iterator of entities that had components of type `T` removed
    /// since the last call to [`World::clear_trackers`].
    pub fn removed<T: Component>(&self) -> std::iter::Cloned<std::slice::Iter<'_, Entity>> {


self.components .get_id(TypeId::of::<T>()) .map_or_else(|| [].iter().cloned(), |component_id| self.removed_with_id(component_id))

Would this work?

seems map_or_else could be used in a bunch of places.

map_or_else is so ugly with the else first...

ghost · 2023-01-06T13:53:02Z

crates/bevy_ecs/src/world/entity_ref.rs

 fn contains_component_with_type(world: &World, type_id: TypeId, location: EntityLocation) -> bool {
-    if let Some(component_id) = world.components.get_id(type_id) {
-        contains_component_with_id(world, component_id, location)
-    } else {
-        false
-    }
+    world
+        .components
+        .get_id(type_id)
+        .map(|component_id| contains_component_with_id(world, component_id, location))
+        .unwrap_or(false)
 }


world .components .get_id(type_id) .map_or_else(|| false, |component_id| contains_component_with_id(world, component_id, location))

At least for bool, use .map_or(false, ...); it should be slightly less work for the compiler and it reads a bit better.

ghost · 2023-01-06T14:02:54Z

crates/bevy_ecs/src/system/commands/mod.rs

+            .unwrap_or_else(|| {
+                panic!("error[B0003]: Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.", std::any::type_name::<T>(), self.entity);
+            })


Maybe breaking that panic message and args up into multiple lines, it's a bit much.

ghost · 2023-01-06T16:22:51Z

crates/bevy_diagnostic/src/log_diagnostics_plugin.rs

-                    );
-                    return;
-                }
+        let Some(value) = diagnostic.smoothed() else { return };


fn log_diagnostic(diagnostic: &Diagnostic) { let Some(value) = diagnostic.smoothed() else { return }; if diagnostic.get_max_history_length() > 1 { let Some(average) = diagnostic.average() else { return }; info!( target: "diagnostics", // Suffix is only used for 's' or 'ms' currently, // so we reserve two columns for it; however, // Do not reserve columns for the suffix in the average // The ) hugging the value is more aesthetically pleasing "{name:<name_width$}: {value:>11.6}{suffix:2} (avg {average:>.6}{suffix:})", name = diagnostic.name, suffix = diagnostic.suffix, name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH, ); } else { info!( target: "diagnostics", "{name:<name_width$}: {value:>.6}{suffix:}", name = diagnostic.name, suffix = diagnostic.suffix, name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH, ); } }

Maybe?

ghost · 2023-01-06T16:34:54Z

crates/bevy_diagnostic/src/diagnostic.rs

-            if let Some(oldest) = self.history.front() {
-                return Some(newest.time.duration_since(oldest.time));
-            }
+        if let (Some(newest), Some(oldest)) = (self.history.back(), self.history.front()) {


pub fn duration(&self) -> Option<Duration> { if self.history.len() < 2 { return None; } if let (Some(newest), Some(oldest)) = (self.history.back(), self.history.front()) { Some(newest.time.duration_since(oldest.time)) } else { None } }

A bit cleaner IMO, no big deal either way.

I think the last expression can be replaced with

self.history.back().zip(self.history.front()).map(|(newest, oldest)| newest.time.duration_since(oldest.time))

The wonders (to some, horrors) of monadic composition!
I think the whole function body can be replaced with that if it's fine to have Some(Duration(0.0)) (if self.history.len() is 1) (.filter(...)?!)

alice-i-cecile · 2023-04-11T20:41:00Z

Closing in favor of the fresher #8358.

james7132 added 2 commits January 5, 2023 16:59

Use let-else on Options

fc39be8

Use let-else for Result

edf9b4c

james7132 added the C-Code-Quality A section of code that is hard to understand or change label Jan 6, 2023

james7132 marked this pull request as ready for review January 6, 2023 01:34

james7132 added the X-Controversial There is active debate or serious implications around merging this PR label Jan 6, 2023

Fix CI

66d27f2

james7132 added the S-Blocked This cannot move forward until something else changes label Jan 6, 2023

atlv24 reviewed Jan 6, 2023

View reviewed changes

ghost reviewed Jan 6, 2023

View reviewed changes

rparrett mentioned this pull request Feb 7, 2023

Refactor filesystem_watcher_system with let-else #7545

Closed

IceSentry mentioned this pull request Apr 11, 2023

Use let else #8358

Closed

alice-i-cecile closed this Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggressively use let-else and combinators to reduce indentation #7104

Aggressively use let-else and combinators to reduce indentation #7104

james7132 commented Jan 6, 2023

james7132 commented Jan 6, 2023 •

edited

Loading

mockersf commented Jan 6, 2023

IceSentry commented Jan 6, 2023

james7132 commented Jan 6, 2023

atlv24 left a comment

atlv24 Jan 6, 2023

james7132 Jan 6, 2023

atlv24 Jan 6, 2023

atlv24 Jan 6, 2023

atlv24 Jan 6, 2023

james7132 Jan 6, 2023

ghost Jan 6, 2023

mockersf commented Jan 6, 2023

ghost left a comment

ghost Jan 6, 2023

ghost Jan 6, 2023

ghost Jan 6, 2023

mockersf Jan 6, 2023

ghost Jan 6, 2023

vacuus Jan 7, 2023

ghost Jan 6, 2023

ghost Jan 6, 2023

ghost Jan 6, 2023

ghost Jan 6, 2023

vacuus Jan 6, 2023 •

edited

Loading

alice-i-cecile commented Apr 11, 2023

Aggressively use let-else and combinators to reduce indentation #7104

Aggressively use let-else and combinators to reduce indentation #7104

Conversation

james7132 commented Jan 6, 2023

Objective

Solution

james7132 commented Jan 6, 2023 • edited Loading

mockersf commented Jan 6, 2023

IceSentry commented Jan 6, 2023

james7132 commented Jan 6, 2023

atlv24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf commented Jan 6, 2023

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vacuus Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Apr 11, 2023

james7132 commented Jan 6, 2023 •

edited

Loading

vacuus Jan 6, 2023 •

edited

Loading