Skip to content

Commit

Permalink
add detailed errors (bevyengine#2994)
Browse files Browse the repository at this point in the history
# 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
  • Loading branch information
mockersf committed Nov 6, 2021
1 parent a5c675f commit b3cd482
Show file tree
Hide file tree
Showing 6 changed files with 155 additions and 8 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
14 changes: 7 additions & 7 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ fn assert_component_access_compatibility(
.map(|component_id| world.components.get_info(component_id).unwrap().name())
.collect::<Vec<&str>>();
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<T>` 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<T>` to create disjoint Queries or merging conflicting Queries into a `QuerySet`.",
query_type, filter_type, system_name, accesses);
}

Expand Down Expand Up @@ -287,7 +287,7 @@ unsafe impl<T: Resource> SystemParamState for ResState<T> {
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::<T>(), system_meta.name);
}
combined_access.add_read(component_id);
Expand Down Expand Up @@ -398,11 +398,11 @@ unsafe impl<T: Resource> SystemParamState for ResMutState<T> {
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::<T>(), 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::<T>(), system_meta.name);
}
combined_access.add_write(component_id);
Expand Down Expand Up @@ -775,7 +775,7 @@ unsafe impl<T: 'static> SystemParamState for NonSendState<T> {
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::<T>(), system_meta.name);
}
combined_access.add_read(component_id);
Expand Down Expand Up @@ -891,11 +891,11 @@ unsafe impl<T: 'static> SystemParamState for NonSendMutState<T> {
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::<T>(), 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::<T>(), system_meta.name);
}
combined_access.add_write(component_id);
Expand Down
91 changes: 91 additions & 0 deletions errors/B0001.md
Original file line number Diff line number Diff line change
@@ -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<Enemy>>,
player: Query<&Transform, With<Player>>,
) {
// ...
}
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<Enemy>>,
player: Query<&Transform, (With<Player>, Without<Enemy>)>,
) {
// ...
}
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<Enemy>>,
QueryState<&Transform, With<Player>>,
)>,
) {
// ...
}
fn main() {
App::new()
.add_plugins(DefaultPlugins)
.add_system(move_enemies_to_player)
.run();
}
```
44 changes: 44 additions & 0 deletions errors/B0002.md
Original file line number Diff line number Diff line change
@@ -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<T>`](https://docs.rs/bevy/*/bevy/ecs/system/struct.Res.html) and [`ResMut<T>`](https://docs.rs/bevy/*/bevy/ecs/system/struct.ResMut.html) for the same `T`, or between [`NonSend<T>`](https://docs.rs/bevy/*/bevy/ecs/system/struct.NonSend.html) and [`NonSendMut<T>`](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<Assets<ColorMaterial>>,
current_materials: Res<Assets<ColorMaterial>>,
) {
// ...
}
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<Assets<ColorMaterial>>,
) {
// ...
}
fn main() {
App::new()
.add_plugins(DefaultPlugins)
.add_system(update_materials)
.run();
}
```
7 changes: 7 additions & 0 deletions errors/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "errors"
version = "0.1.0"
edition = "2021"

[dependencies]
bevy = { path = "../" }
5 changes: 5 additions & 0 deletions errors/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#[doc = include_str!("../B0001.md")]
pub struct B0001;

#[doc = include_str!("../B0002.md")]
pub struct B0002;

0 comments on commit b3cd482

Please sign in to comment.