-
Notifications
You must be signed in to change notification settings - Fork 9
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
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
79c8ade
feat: setup infrastructure
BD103 32bc6c4
feat: first working iteration
BD103 e2876af
feat: add further comments
BD103 1d587af
feat: do not lint on references and pointers
BD103 d86fe38
feat: add suggestion in diagnostic
BD103 d2c9d51
feat: begin lint documentation
BD103 639cb46
feat: finish documentation
BD103 da93d8a
fix: lint can be tricked with `use Foo as FooPlugin`
BD103 ebb5079
feat: add section on known issues
BD103 20ac53a
fix: do not lint on generic parameters
BD103 a5b9657
Merge branch 'main' into plugin-not-ending-in-plugin
BD103 a15c994
feat: clarify what is being linted
BD103 5616535
Merge branch 'main' into plugin-not-ending-in-plugin
BD103 65d5b3d
Merge branch 'main' into plugin-not-ending-in-plugin
BD103 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
//! Checks for types who implement `Plugin` but whose names does not end in "Plugin". | ||
//! | ||
//! This does _not_ check function-style plugins (`fn plugin(app: &mut App)`), only structures with | ||
//! `Plugin` explicitly implemented with `impl Plugin for T`. | ||
//! | ||
//! # Motivation | ||
//! | ||
//! 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. | ||
//! | ||
//! # Known issues | ||
//! | ||
//! Due to technical reasons, if you wish to silence this lint you need to annotate the | ||
//! `impl Plugin for T` line with `#[allow(bevy::plugin_not_ending_in_plugin)]`, not the `struct T` | ||
//! line. | ||
//! | ||
//! # Example | ||
//! | ||
//! ``` | ||
//! # use bevy::prelude::*; | ||
//! # | ||
//! struct Physics; | ||
//! | ||
//! impl Plugin for Physics { | ||
//! fn build(&self, app: &mut App) { | ||
//! // ... | ||
//! } | ||
//! } | ||
//! ``` | ||
//! | ||
//! Use instead: | ||
//! | ||
//! ``` | ||
//! # use bevy::prelude::*; | ||
//! # | ||
//! struct PhysicsPlugin; | ||
//! | ||
//! impl Plugin for PhysicsPlugin { | ||
//! fn build(&self, app: &mut App) { | ||
//! // ... | ||
//! } | ||
//! } | ||
//! ``` | ||
|
||
use crate::declare_bevy_lint; | ||
use clippy_utils::{diagnostics::span_lint_and_then, match_def_path, path_res}; | ||
use rustc_errors::Applicability; | ||
use rustc_hir::{def::Res, Item, ItemKind}; | ||
use rustc_lint::{LateContext, LateLintPass}; | ||
use rustc_session::declare_lint_pass; | ||
use rustc_span::symbol::Ident; | ||
|
||
declare_bevy_lint! { | ||
pub PLUGIN_NOT_ENDING_IN_PLUGIN, | ||
STYLE, | ||
"implemented `Plugin` for a structure whose name does not end in \"Plugin\"", | ||
} | ||
|
||
declare_lint_pass! { | ||
PluginNotEndingInPlugin => [PLUGIN_NOT_ENDING_IN_PLUGIN.lint] | ||
} | ||
|
||
impl<'tcx> LateLintPass<'tcx> for PluginNotEndingInPlugin { | ||
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &Item<'tcx>) { | ||
// Find `impl` items... | ||
if let ItemKind::Impl(impl_) = item.kind | ||
// ...that implement a trait... | ||
&& let Some(of_trait) = impl_.of_trait | ||
// ...where the trait is a path to user code... (I don't believe this will ever be | ||
// false, since the alternatives are primitives, `Self`, and others that wouldn't make | ||
// since in this scenario.) | ||
&& let Res::Def(_, def_id) = of_trait.path.res | ||
// ...where the trait being implemented is Bevy's `Plugin`... | ||
&& match_def_path(cx, def_id, &crate::paths::PLUGIN) | ||
{ | ||
// Try to resolve where this type was originally defined. This will result in a `DefId` | ||
// pointing to the original `struct Foo` definition, or `impl <T>` if it's a generic | ||
// parameter. | ||
let Some(def_id) = path_res(cx, impl_.self_ty).opt_def_id() else { | ||
return; | ||
}; | ||
|
||
// If this type is a generic parameter, exit. Their names, such as `T`, cannot be | ||
// referenced by others. | ||
if impl_ | ||
.generics | ||
.params | ||
.iter() | ||
.any(|param| param.def_id.to_def_id() == def_id) | ||
{ | ||
return; | ||
} | ||
|
||
// Find the original name and span of the type. (We don't use the name from the path, | ||
// since that can be spoofed through `use Foo as FooPlugin`.) | ||
let Some(Ident { | ||
name: self_name, | ||
span: self_span, | ||
}) = cx.tcx.opt_item_ident(def_id) | ||
else { | ||
return; | ||
}; | ||
|
||
// If the type's name ends in "Plugin", exit. | ||
if self_name.as_str().ends_with("Plugin") { | ||
return; | ||
} | ||
|
||
span_lint_and_then( | ||
cx, | ||
PLUGIN_NOT_ENDING_IN_PLUGIN.lint, | ||
item.span, | ||
PLUGIN_NOT_ENDING_IN_PLUGIN.lint.desc, | ||
|diag| { | ||
diag.span_suggestion( | ||
self_span, | ||
"rename the plugin", | ||
format!("{self_name}Plugin"), | ||
// There may be other references that also need to be renamed. | ||
Applicability::MaybeIncorrect, | ||
); | ||
}, | ||
); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
Plugin
s?