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: bevy::main_return_without_appexit #84

Merged
merged 24 commits into from
Sep 18, 2024
Merged

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Sep 17, 2024

This is the first lint! Closes #53. Please see the module documentation or this link for information about what the lint does.

Design decisions

  • Lint modules are there documentation, and as such are public.

    image

  • lints/mod.rs has the LINTS static for register_passes() function, so you don't have to remember to touch BevyLintCallback each time you add a lint.

  • For now, lints are going to be manually added as modules, to LINTS, and to register_passes(). I tried using a macro, but it was too complicated and not worth it.

  • Lints should be declared with the builtin declare_tool_lint! and declare_lint_pass! macros.

Current drawbacks

  • This lint does not detect if you emit AppExit through some other means, such as:

    fn main() {
      let app_exit = App::new().run();
      println!("{app_exit:?}");
    }

    I found this too complicated for a first lint, so I'm going to create a follow-up issue to tackle this.

  • Lints currently do not support be silenced / changing levels. #[allow(bevy::main_return_without_appexit)], -A bevy::main_return_without_appexit, and adding main_return_without_appexit = "allow" to Cargo.toml all fail. This is another follow-up issue.

  • I have not created the lint groups yet, so this is not within the bevy::style category yet. This is the final follow-up that I know of.

Testing!

Now for the fun part! Because there are no UI tests (yet), manually paste the following in bevy_lint/examples/lint_test.rs:

use bevy::prelude::*;

fn main() {
    App::new().run();
}

Then, run the following:

cd bevy_lint
cargo build
cargo run -- --example lint_test

The outputted warning should look like this:

warning: an entrypoint that calls `App::run()` does not return `AppExit`
 --> bevy_lint/examples/lint_test.rs:4:16
  |
3 | fn main() {
  |          - help: try: `-> AppExit`
4 |     App::new().run();
  |                ^^^^^
  |
  = note: `App::run()` returns `AppExit`, which can be used to determine whether the app exited successfully or not
  = note: `#[warn(bevy::main_return_without_appexit)]` on by default

@BD103 BD103 added A-Linter Related to the linter and custom lints C-Feature Make something new possible labels Sep 17, 2024
@bas-ie
Copy link
Collaborator

bas-ie commented Sep 18, 2024

Confirmed insofar it works, and goes away when I fix the problem 🙂

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Great stuff, tested it and it works very reliably. Left some minor suggestions, nothing blocking.

BD103 and others added 2 commits September 18, 2024 17:54
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
@BD103 BD103 enabled auto-merge (squash) September 18, 2024 21:58
@BD103 BD103 merged commit b12f311 into main Sep 18, 2024
6 checks passed
@BD103 BD103 deleted the main-return-without-appexit branch September 18, 2024 22:05
@BD103 BD103 mentioned this pull request Nov 9, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lint: Returning AppExit from fn main()
3 participants