Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
janhohenheim opened this issue Sep 6, 2024 · 2 comments · Fixed by #95
Closed

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

janhohenheim opened this issue Sep 6, 2024 · 2 comments · Fixed by #95
Assignees
Labels
A-Linter Related to the linter and custom lints C-Feature Make something new possible X-Blessed

Comments

@janhohenheim
Copy link
Member

No description provided.

@janhohenheim janhohenheim added the A-Linter Related to the linter and custom lints label Sep 6, 2024
@BD103 BD103 added the C-Feature Make something new possible label Sep 6, 2024
@BD103 BD103 moved this to Todo in `bevy_lint` Lints Sep 6, 2024
@BD103
Copy link
Member

BD103 commented Sep 17, 2024

Quoting @alice-i-cecile from #85:

Calling .single, .single_mut, .resource, .resource_mut and so on can introduce surprising crashes to your production game.

We should pair this with a recommendation to use equivalent lints around unwrap and expect in clippy, so coders don't just change .single() to .get_single().unwrap().

We probably want to split this into two lints, one for query data and the other for resources.

@alice-i-cecile
Copy link
Member

Related to bevyengine/bevy#14275

@BD103 BD103 self-assigned this Sep 19, 2024
@BD103 BD103 moved this from Todo to In Progress in `bevy_lint` Lints Sep 19, 2024
@BD103 BD103 added this to the `bevy_lint` 0.1.0 milestone Sep 25, 2024
BD103 added a commit that referenced this issue Sep 27, 2024
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](https://github.com/orgs/TheBevyFlock/projects/3/views/1?sliceBy%5Bvalue%5D=Restriction)
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`.

```rust
#![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:

```bash
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 closed this as completed in #95 Sep 27, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in `bevy_lint` Lints Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints C-Feature Make something new possible X-Blessed
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants