Skip to content

Create lint groups#98

Merged
BD103 merged 5 commits intomainfrom
lint-groups
Sep 25, 2024
Merged

Create lint groups#98
BD103 merged 5 commits intomainfrom
lint-groups

Conversation

@BD103
Copy link
Copy Markdown
Member

@BD103 BD103 commented Sep 23, 2024

Closes #90.

This PR implements lint groups! These, just like Clippy's groups, allow enabling and disable multiple lints at a time based on their categories. I, stealing Clippy's groups, have added:

  • Correctness: Zero-false-positives deny-by-default "if you hit these, you're wrong" lints
  • Suspicious: Like Correctness, but the behavior causing the lint may actually be intended. These are warnings, not hard errors.
  • Complexity: Hey, you! That while you wrote there could be a for loop! (This category checks for overly-complex code with good alternatives.)
  • Performance: Anything that can be changed to improve performance. (No, this doesn't warn if you call std::thread::sleep(), that's your own fault.)
  • Style: By far the most controversial of them all, this category enforces "idiomatic Bevy code". What does that mean? Fantastic question, let's bikeshed this into oblivion!
    • In all serious, because this is on-by-default, I'm going to be very conservative adding anything to this category. Serious thought is required!
  • Pedantic: Similar to Style, but disabled by default. This category is the nit-picky math teacher you had that one time who hated shortcuts, calculators, and mental math.
  • Restriction: Opt-in lints that you may want for specific purposes, such as requiring all components to derive Reflect.
  • Nursery: A category for unstable and flaky lints that may be knighted one day into full-on lints. For now, this category should not be used, since bevy_lint hasn't been released yet!

As part of this PR, I've changed a few things about how lints are defined.

  1. Lints are declared with the declare_bevy_lint! macro. This is very similar to the declare_tool_lint! that we've used before.
  2. BevyLint struct: contains the Lint struct and a reference to the lint group this is a member of.
  3. LintGroup struct: represents the name and default level (deny, warn, allow) of a lint.
Old comment about an error I fixed

As of yet I'm unable to actually deny / allow a lint group. I've tried:

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

#![deny(bevy::style)]

use bevy::prelude::*;

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

But I'm getting:

warning: unknown lint: `bevy::style`
 --> bevy_lint/examples/lint_test.rs:4:9
  |
4 | #![deny(bevy::style)]
  |         ^^^^^^^^^^^
  |
  = note: `#[warn(unknown_lints)]` on by default

warning: an entrypoint that calls `App::run()` does not return `AppExit`
 --> bevy_lint/examples/lint_test.rs:9:16
  |
8 | fn main() {
  |          - help: try: `-> AppExit`
9 |     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

warning: `bevy_lint` (example "lint_test") generated 2 warnings

main_return_without_appexit should have been denied as an error, but it was a warning instead. This requires a bit more testing before it can be merged.

@BD103 BD103 added A-Linter Related to the linter and custom lints C-Usability An improvement that makes the API more pleasant labels Sep 23, 2024
@BD103

This comment was marked as resolved.

@BD103 BD103 marked this pull request as ready for review September 23, 2024 13:21
@BD103
Copy link
Copy Markdown
Member Author

BD103 commented Sep 23, 2024

After testing (#![register_tool(bevy)] needed at the root of the crate):

  • #![deny(bevy::...)] works
  • [lints.bevy] table works in Cargo.toml
  • RUSTFLAGS="-Dbevy::..." does not work because it applies to dependencies, which have not registered bevy as a tool.

Copy link
Copy Markdown
Collaborator

@bas-ie bas-ie left a comment

Choose a reason for hiding this comment

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

Nice! Only some nits.

Comment on lines +54 to +56
/// These are designed for scenarios where you want to increase the consistency of your code-base
/// and reject certain patterns. They should not all be enabled at once, but instead specific lints
/// should be individually enabled.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I feel like the obvious question is, "if they shouldn't all be enabled at once, why are they in a group"? Or are we saying, this group is for categorising but not for actual use 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I want every lint to be within a group so that it's clear what its general purpose is. These lint groups are just as important for documentation as they are for bulk-toggling.

So yes, this group is a category that's never intended to be named directly. (Clippy has the same things!)

Co-authored-by: Rich Churcher <rich.churcher@gmail.com>
Copy link
Copy Markdown
Collaborator

@bas-ie bas-ie left a comment

Choose a reason for hiding this comment

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

Somehow failed to approve first time around.

@bas-ie
Copy link
Copy Markdown
Collaborator

bas-ie commented Sep 24, 2024

Oh, I'm not on the repo so doesn't matter! 😁

@BD103 BD103 enabled auto-merge (squash) September 24, 2024 11:22
@BD103 BD103 disabled auto-merge September 25, 2024 00:31
@BD103 BD103 merged commit 23bb966 into main Sep 25, 2024
@BD103 BD103 deleted the lint-groups branch September 25, 2024 00:45
BD103 added a commit that referenced this pull request Sep 25, 2024
Looks like I forgot to register the lint groups introduced in #98.
Whoops!

While I'm at it, I also changed the visibility of some items so that
less is public that before.
@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-Usability An improvement that makes the API more pleasant

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create lint groups

2 participants