TheBevyFlock / bevy_cli

A Bevy CLI tool and linter.
https://thebevyflock.github.io/bevy_cli/
Apache License 2.0
52 stars 7 forks source link

Add lint: `panicking_query_methods` and `panicking_world_methods` #95

Closed BD103 closed 2 months ago

BD103 commented 2 months ago

As per @alice-i-cecile's request! Closes #58.

This adds the bevy::panicking_query_methods lint, which searches for usage of panicking methods like Query::single(). It is currently Allow-by-default, so you need to explicitly opt-in to use it. (In the project it's under the Restriction category.)

This lint checks both Query and QueryState. I will write a lint for resources in another PR. :)

Testing

Wow, I'm so glad you asked! (You asked how to test this lint, right?)

I used the following example when testing. Paste it into bevy_lint/examples/lint_test.rs, run cd bevy_lint, then run cargo build && cargo run -- --example lint_test.

#![feature(register_tool)]
#![register_tool(bevy)]

#![deny(bevy::panicking_query_methods)]
#![deny(bevy::panicking_world_methods)]

use bevy::prelude::*;

#[derive(Component)]
struct MyComponent;

#[derive(Resource)]
struct MyResource;

fn main() {
    App::new().add_systems(Update, my_system);

    let mut world = World::new();

    let mut query_state = QueryState::<&mut MyComponent>::new(&mut world);
    let _ = query_state.single_mut(&mut world);

    world.insert_resource(MyResource);
    let _ = world.resource::<MyResource>();
}

fn my_system(query: Query<&MyComponent>) {
    let _ = query.many([]);
}

The warnings should look like this:

error: called a `QueryState` method that can panic when a non-panicking alternative exists
  --> bevy_lint/examples/lint_test.rs:21:25
   |
21 |     let _ = query_state.single_mut(&mut world);
   |                         ^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: use `query_state.get_single_mut(&mut world)` and handle the `Option` or `Result`
note: the lint level is defined here
  --> bevy_lint/examples/lint_test.rs:4:9
   |
4  | #![deny(bevy::panicking_query_methods)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: called a `World` method that can panic when a non-panicking alternative exists
  --> bevy_lint/examples/lint_test.rs:24:19
   |
24 |     let _ = world.resource::<MyResource>();
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: use `world.get_resource::<MyResource>()` and handle the `Option` or `Result`
note: the lint level is defined here
  --> bevy_lint/examples/lint_test.rs:5:9
   |
5  | #![deny(bevy::panicking_world_methods)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: called a `Query` method that can panic when a non-panicking alternative exists
  --> bevy_lint/examples/lint_test.rs:28:19
   |
28 |     let _ = query.many([]);
   |                   ^^^^^^^^
   |
   = help: use `query.get_many([])` and handle the `Option` or `Result`

Fiddle around with it and try to break it! Good luck, though. Unless you use #94, this is the most foolproof lint I've written so far! >:D

janhohenheim commented 2 months ago

There's a little list of panicking APIs in https://github.com/bevyengine/bevy/issues/14275 FYI :)

BD103 commented 2 months ago

I'm going to modify this to also check for panicking World methods.

BD103 commented 2 months ago

Ok, I believe this is now ready for review again. It now supports linting against World, as well as Query and QueryState.

BD103 commented 2 months ago

I merged #106 directly into this, so that will need to be merged before this can.