Skip to content

Add lint: panicking_query_methods and panicking_world_methods#95

Merged
BD103 merged 19 commits intomainfrom
panicking-query-methods
Sep 27, 2024
Merged

Add lint: panicking_query_methods and panicking_world_methods#95
BD103 merged 19 commits intomainfrom
panicking-query-methods

Conversation

@BD103
Copy link
Copy Markdown
Member

@BD103 BD103 commented Sep 20, 2024 •
edited
Loading

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

@BD103 BD103 added A-Linter Related to the linter and custom lints C-Feature Make something new possible labels Sep 20, 2024
@janhohenheim
Copy link
Copy Markdown
Member

janhohenheim commented Sep 20, 2024

There's a little list of panicking APIs in bevyengine/bevy#14275 FYI :)

@BD103
Copy link
Copy Markdown
Member Author

BD103 commented Sep 23, 2024

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

@BD103 BD103 marked this pull request as draft September 23, 2024 19:23
@BD103 BD103 changed the title Add lint: panicking_query_methods Add lint: panicking_query_methods and panicking_world_methods Sep 24, 2024
@BD103 BD103 marked this pull request as ready for review September 24, 2024 02:38
@BD103
Copy link
Copy Markdown
Member Author

BD103 commented Sep 24, 2024

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

@BD103
Copy link
Copy Markdown
Member Author

BD103 commented Sep 25, 2024

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

@BD103 BD103 added S-Blocked This cannot move forward until something else changes and removed S-Blocked This cannot move forward until something else changes labels Sep 25, 2024
TimJentzsch
TimJentzsch approved these changes Sep 27, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly some doc nits, but nothing blocking

@BD103 BD103 enabled auto-merge (squash) September 27, 2024 18:45
@BD103 BD103 merged commit 09d994f into main Sep 27, 2024
@BD103 BD103 deleted the panicking-query-methods branch September 27, 2024 18:51
@alice-i-cecile alice-i-cecile mentioned this pull request Oct 12, 2024
Closed
@BD103 BD103 mentioned this pull request Nov 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TimJentzsch TimJentzsch TimJentzsch approved these changes

Assignees

No one assigned

Labels

A-Linter Related to the linter and custom lints C-Feature Make something new possible

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add lint: Calling panicking methods like single() instead of get_ variants

3 participants

@BD103 @janhohenheim @TimJentzsch