From b3cd48228b6e10e6394d7ced21ea2a149f8151c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois?= Date: Sat, 6 Nov 2021 20:53:11 +0000 Subject: [PATCH] add detailed errors (#2994) # Objective - Improve error descriptions and help understand how to fix them - I noticed one today that could be expanded, it seemed like a good starting point ## Solution - Start something like https://github.com/rust-lang/rust/tree/master/compiler/rustc_error_codes/src/error_codes - Remove sentence about Rust mutability rules which is not very helpful in the error message I decided to start the error code with B for Bevy so that they're not confused with error code from rust (which starts with E) Longer term, there are a few more evolutions that can continue this: - the code samples should be compiled check, and even executed for some of them to check they have the correct error code in a panic - the error could be build on a page in the website like https://doc.rust-lang.org/error-index.html - most panic should have their own error code --- Cargo.toml | 2 +- crates/bevy_ecs/src/system/system_param.rs | 14 ++-- errors/B0001.md | 91 ++++++++++++++++++++++ errors/B0002.md | 44 +++++++++++ errors/Cargo.toml | 7 ++ errors/src/lib.rs | 5 ++ 6 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 errors/B0001.md create mode 100644 errors/B0002.md create mode 100644 errors/Cargo.toml create mode 100644 errors/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index ff97fb1b45..830f6131ab 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,7 @@ repository = "https://github.com/bevyengine/bevy" [workspace] exclude = ["benches"] -members = ["crates/*", "examples/ios", "tools/ci"] +members = ["crates/*", "examples/ios", "tools/ci", "errors"] [features] default = [ diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index f9f5f60794..b544ed5a6c 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -185,7 +185,7 @@ fn assert_component_access_compatibility( .map(|component_id| world.components.get_info(component_id).unwrap().name()) .collect::>(); let accesses = conflicting_components.join(", "); - panic!("Query<{}, {}> in system {} accesses component(s) {} in a way that conflicts with a previous system parameter. Allowing this would break Rust's mutability rules. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `QuerySet`.", + panic!("error[B0001]: Query<{}, {}> in system {} accesses component(s) {} in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `QuerySet`.", query_type, filter_type, system_name, accesses); } @@ -287,7 +287,7 @@ unsafe impl SystemParamState for ResState { let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( - "Res<{}> in system {} conflicts with a previous ResMut<{0}> access. Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", + "error[B0002]: Res<{}> in system {} conflicts with a previous ResMut<{0}> access. Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } combined_access.add_read(component_id); @@ -398,11 +398,11 @@ unsafe impl SystemParamState for ResMutState { let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( - "ResMut<{}> in system {} conflicts with a previous ResMut<{0}> access. Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", + "error[B0002]: ResMut<{}> in system {} conflicts with a previous ResMut<{0}> access. Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } else if combined_access.has_read(component_id) { panic!( - "ResMut<{}> in system {} conflicts with a previous Res<{0}> access. Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", + "error[B0002]: ResMut<{}> in system {} conflicts with a previous Res<{0}> access. Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } combined_access.add_write(component_id); @@ -775,7 +775,7 @@ unsafe impl SystemParamState for NonSendState { let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( - "NonSend<{}> in system {} conflicts with a previous mutable resource access ({0}). Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", + "error[B0002]: NonSend<{}> in system {} conflicts with a previous mutable resource access ({0}). Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } combined_access.add_read(component_id); @@ -891,11 +891,11 @@ unsafe impl SystemParamState for NonSendMutState { let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( - "NonSendMut<{}> in system {} conflicts with a previous mutable resource access ({0}). Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", + "error[B0002]: NonSendMut<{}> in system {} conflicts with a previous mutable resource access ({0}). Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } else if combined_access.has_read(component_id) { panic!( - "NonSendMut<{}> in system {} conflicts with a previous immutable resource access ({0}). Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", + "error[B0002]: NonSendMut<{}> in system {} conflicts with a previous immutable resource access ({0}). Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } combined_access.add_write(component_id); diff --git a/errors/B0001.md b/errors/B0001.md new file mode 100644 index 0000000000..9e95cbe3cf --- /dev/null +++ b/errors/B0001.md @@ -0,0 +1,91 @@ +# B0001 + +To keep [Rust rules on references](https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html#the-rules-of-references) (either one mutable reference or any number of immutable references) on a component, it is not possible to have two queries on the same component when one request mutable access to it in the same system. + +Erroneous code example: + +```rust,should_panic +use bevy::prelude::*; + +#[derive(Component)] +struct Player; + +#[derive(Component)] +struct Enemy; + +fn move_enemies_to_player( + mut enemies: Query<&mut Transform, With>, + player: Query<&Transform, With>, +) { + // ... +} + +fn main() { + App::new() + .add_plugins(DefaultPlugins) + .add_system(move_enemies_to_player) + .run(); +} +``` + +This will panic, as it's not possible to have both a mutable and an immutable query on `Transform` at the same time. + +You have two solutions: + +Solution #1: use disjoint queries using [`Without`](https://docs.rs/bevy/*/bevy/ecs/query/struct.Without.html) + +As a `Player` entity won't be an `Enemy` at the same time, those two queries will acutally never target the same entity. This can be encoded in the query filter with [`Without`](https://docs.rs/bevy/*/bevy/ecs/query/struct.Without.html): + +```rust,no_run +use bevy::prelude::*; + +#[derive(Component)] +struct Player; + +#[derive(Component)] +struct Enemy; + +fn move_enemies_to_player( + mut enemies: Query<&mut Transform, With>, + player: Query<&Transform, (With, Without)>, +) { + // ... +} + +fn main() { + App::new() + .add_plugins(DefaultPlugins) + .add_system(move_enemies_to_player) + .run(); +} +``` + +Solution #2: use a [`QuerySet`](https://docs.rs/bevy/*/bevy/ecs/system/struct.QuerySet.html) + +A [`QuerySet`](https://docs.rs/bevy/*/bevy/ecs/system/struct.QuerySet.html) will let you have conflicting queries as a parameter, but you will still be responsible of not using them at the same time in your system. + +```rust,no_run +use bevy::prelude::*; + +#[derive(Component)] +struct Player; + +#[derive(Component)] +struct Enemy; + +fn move_enemies_to_player( + mut transforms: QuerySet<( + QueryState<&mut Transform, With>, + QueryState<&Transform, With>, + )>, +) { + // ... +} + +fn main() { + App::new() + .add_plugins(DefaultPlugins) + .add_system(move_enemies_to_player) + .run(); +} +``` diff --git a/errors/B0002.md b/errors/B0002.md new file mode 100644 index 0000000000..3be7bb8039 --- /dev/null +++ b/errors/B0002.md @@ -0,0 +1,44 @@ +# B0002 + +To keep [Rust rules on references](https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html#the-rules-of-references) (either one mutable reference or any number of immutable references) on a resource, it is not possible to have more than one resource of a kind if one is mutable in the same system. This can happen between [`Res`](https://docs.rs/bevy/*/bevy/ecs/system/struct.Res.html) and [`ResMut`](https://docs.rs/bevy/*/bevy/ecs/system/struct.ResMut.html) for the same `T`, or between [`NonSend`](https://docs.rs/bevy/*/bevy/ecs/system/struct.NonSend.html) and [`NonSendMut`](https://docs.rs/bevy/*/bevy/ecs/system/struct.NonSendMut.html) for the same `T`. + +Erroneous code example: + +```rust,should_panic +use bevy::prelude::*; + +fn update_materials( + mut material_updater: ResMut>, + current_materials: Res>, +) { + // ... +} + +fn main() { + App::new() + .add_plugins(DefaultPlugins) + .add_system(update_materials) + .run(); +} +``` + +This will panic, as it's not possible to have both a mutable and an immutable resource on `State` at the same time. + +As a mutable resource already provide access to the current resource value, you can remove the immutable resource. + +```rust,no_run +use bevy::prelude::*; + +fn update_materials( + mut material_updater: ResMut>, +) { + // ... +} + +fn main() { + App::new() + .add_plugins(DefaultPlugins) + .add_system(update_materials) + .run(); +} +``` diff --git a/errors/Cargo.toml b/errors/Cargo.toml new file mode 100644 index 0000000000..a64061f078 --- /dev/null +++ b/errors/Cargo.toml @@ -0,0 +1,7 @@ +[package] +name = "errors" +version = "0.1.0" +edition = "2021" + +[dependencies] +bevy = { path = "../" } diff --git a/errors/src/lib.rs b/errors/src/lib.rs new file mode 100644 index 0000000000..efa56cf95b --- /dev/null +++ b/errors/src/lib.rs @@ -0,0 +1,5 @@ +#[doc = include_str!("../B0001.md")] +pub struct B0001; + +#[doc = include_str!("../B0002.md")] +pub struct B0002;