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: Plugin not ending in "Plugin" #111

Merged
merged 14 commits into from
Oct 3, 2024
Merged

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Sep 27, 2024

Closes #55.

This adds a lint that verifies that all plugins have the "Plugin" suffix, as that pattern is pretty-much standardized across the ecosystem. This is our first "Style" lint, meaning it tries to suggest idiomatic code.

Testing

Paste the following 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)]

use bevy::prelude::*;

mod module {
    pub mod submodule {
        use std::marker::PhantomData;

        // Verify generics are not included in name.
        pub struct Foo<T>(PhantomData<T>);
    }
}

// Ensure you cannot spoof the name of the plugin.
use module::submodule::Foo as FooPlugin;

impl<T: Send + Sync + 'static> Plugin for FooPlugin<T> {
    fn build(&self, _: &mut App) {}
}

fn main() {}

@BD103 BD103 added A-Linter Related to the linter and custom lints C-Feature Make something new possible labels Sep 27, 2024
@BD103 BD103 marked this pull request as ready for review September 27, 2024 01:43
@BD103
Copy link
Member Author

BD103 commented Sep 27, 2024

Found two issues when running this on the engine itself:

  1. It warns against a plugin ending in "Plugins" (plural), specifically bevy_picking::DefaultPickingPlugins. Should this be handled by the lint or #[allow(...)]d in the definition?
  2. It should skip over generics, but it does not:
warning: implemented `Plugin` for a structure whose name does not end in "Plugin"
   --> crates/bevy_app/src/plugin.rs:97:6
    |
97  | impl<T: Fn(&mut App) + Send + Sync + 'static> Plugin for T {
    |      ^ help: rename the plugin: `TPlugin`
    |
help: `Plugin` implemented here
   --> crates/bevy_app/src/plugin.rs:97:1
    |
97  | / impl<T: Fn(&mut App) + Send + Sync + 'static> Plugin for T {
98  | |     fn build(&self, app: &mut App) {
99  | |         self(app);
100 | |     }
101 | | }
    | |_^
    = note: `#[warn(bevy::plugin_not_ending_in_plugin)]` on by default

@BD103
Copy link
Member Author

BD103 commented Sep 27, 2024

  1. It warns against a plugin ending in "Plugins" (plural), specifically bevy_picking::DefaultPickingPlugins. Should this be handled by the lint or #[allow(...)]d in the definition?

After further thought, I think this is not something the lint should handle. Downstream it should be #[allow(...)]'d, since it's technically mimicking PluginGroup.

  1. It should skip over generics, but it does not:

This has been fixed in 20ac53a.

Copy link
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.

A small comment on the motivation part, but not a blocking one.

Overall, nicely documented!

Comment on lines +8 to +10
//! Unlike traits like [`Clone`] or [`Debug`], the primary purpose of a type that implements
//! `Plugin` is to be a Bevy plugin. As such, it is common practice to suffix plugin names with
//! "Plugin" to signal how they should be used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we could expand on the motivation a bit. Currently it explains that this is a common practice, but not really why this is the case (or rather, why it is a good idea to follow it).

Digging into this a bit more will also help to resolve the question whether a Plugins suffix should be allowed or not.
If the goal is to clearly distinguish a plugin struct from other structs to ensure that it is used in the correct places, then Plugins would probably also accomplish that goal.
If it's more about strict consistency, then perhaps Plguins should not be allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've personally always used the "Plugin" suffix for consistency with the ecosystem, but I've always favored consistency in my code.

@alice-i-cecile or @janhohenheim, would you like to pitch in here? Why should we encourage users to use the "Plugin" suffix for all Plugins?

@BD103
Copy link
Member Author

BD103 commented Sep 28, 2024

Note to self: this should probably use implements_trait() and check the structure definition not the impl.

@BD103
Copy link
Member Author

BD103 commented Sep 30, 2024

Note to self: this should probably use implements_trait() and check the structure definition not the impl.

I'm going to make this a follow-up issue. I'd rather get this merged as-is.

This makes it more clear that the `impl` item is linted, not the original structure definition.
@BD103
Copy link
Member Author

BD103 commented Oct 1, 2024

Note to self: this should probably use implements_trait() and check the structure definition not the impl.

I'm going to make this a follow-up issue. I'd rather get this merged as-is.

I did some more experimentation on this. It is far easier to lint on the impl block because it does not require dealing with generics nearly as much as linting on structure definitions. implements_trait() requires a list of GenericArg, since traits can be conditionally implemented for certain generic types and not others.

While implements_trait() is handy, and I got to learn a lot more about compiler queries and binders, it's not worth it for this specific case.

@alice-i-cecile
Copy link
Member

@BD103 feel free to merge this when you're ready, this LGTM.

@BD103
Copy link
Member Author

BD103 commented Oct 3, 2024

I'm going to merge this as-is. There is still the open question (#111 (comment)) on why we use the Plugin suffix, but that can only really be answered by @cart. The current reasoning is short, but I'm going to leave it as-is unless someone brings it up again.

@BD103 BD103 enabled auto-merge (squash) October 3, 2024 17:54
@BD103 BD103 merged commit 80c81b6 into main Oct 3, 2024
7 checks passed
@BD103 BD103 deleted the plugin-not-ending-in-plugin branch October 3, 2024 17:56
@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: Plugin names should end in Plugin
3 participants