-
Notifications
You must be signed in to change notification settings - Fork 32
Add Linit: missing_trait_impls #574
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
Open
DaAlbrecht
wants to merge
9
commits into
main
Choose a base branch
from
unit-components-impl-default-211
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4e111bf
feat: move utils to check if a type implements a trait to the utils m…
DaAlbrecht 8b4ee66
feat: missing default on unit component structs lint
DaAlbrecht 2f258bb
feat: skip if the type was defined in an external macro
DaAlbrecht ed066c2
feat: fix missing spce in lint description
DaAlbrecht 146dfc9
feat: rename lint from `missing_default` to `missing_trait_impls`
DaAlbrecht 2c69176
feat: support copy,clone
DaAlbrecht 8496f45
fix missing_default.fixed
DaAlbrecht 19f3b34
Merge branch 'main' into unit-components-impl-default-211
DaAlbrecht 6b862a6
Merge branch 'main' into unit-components-impl-default-211
DaAlbrecht 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 hidden or 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 hidden or 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,144 @@ | ||
use clippy_utils::{diagnostics::span_lint_hir_and_then, sugg::DiagExt, ty::implements_trait}; | ||
use rustc_errors::Applicability; | ||
use rustc_hir::{ItemKind, def_id::DefId}; | ||
use rustc_lint::{LateContext, LateLintPass, Lint}; | ||
|
||
use crate::{declare_bevy_lint, declare_bevy_lint_pass, utils::traits::TraitType}; | ||
|
||
declare_bevy_lint! { | ||
pub(crate) MISSING_DEFAULT, | ||
super::Restriction, | ||
"defined a unit component without a `Default` implementation", | ||
} | ||
|
||
declare_bevy_lint! { | ||
pub(crate) MISSING_CLONE, | ||
super::Restriction, | ||
"defined a unit component without a `Clone` implementation", | ||
} | ||
|
||
declare_bevy_lint! { | ||
pub(crate) MISSING_COPY, | ||
super::Restriction, | ||
"defined a unit component without a `Copy` implementation", | ||
} | ||
|
||
declare_bevy_lint_pass! { | ||
pub(crate) MissingTraitImpls => [MISSING_DEFAULT,MISSING_CLONE,MISSING_COPY], | ||
} | ||
|
||
impl<'tcx> LateLintPass<'tcx> for MissingTraitImpls { | ||
fn check_crate(&mut self, cx: &LateContext<'tcx>) { | ||
// Finds all types that implement `Component` in this crate. | ||
let components: Vec<TraitType> = | ||
TraitType::from_local_crate(cx, &crate::paths::COMPONENT).collect(); | ||
|
||
for component in components { | ||
// Skip if a types originates from a foreign crate's macro | ||
if component | ||
.item_span | ||
.in_external_macro(cx.tcx.sess.source_map()) | ||
{ | ||
continue; | ||
} | ||
|
||
let def_id = component.hir_id.owner.to_def_id(); | ||
|
||
// Skip binder as we are not interested in the generics | ||
let ty = cx.tcx.type_of(def_id).skip_binder(); | ||
|
||
for trait_to_implement in Trait::all() { | ||
// get the def_id of the trait that should be implement for unit structures. | ||
if let Some(trait_def_id) = trait_to_implement.get_def_id(cx) | ||
&& !implements_trait(cx, ty, trait_def_id, &[]) | ||
{ | ||
// Find the `Item` definition of the Component missing the trait derive. | ||
let missing_trait_imp_item = cx | ||
.tcx | ||
.hir_expect_item(component.hir_id.expect_owner().def_id); | ||
|
||
let fields = match missing_trait_imp_item.kind { | ||
ItemKind::Struct(_, _, data) => data.fields().to_vec(), | ||
// If this item is not a struct, continue | ||
_ => continue, | ||
}; | ||
|
||
// Check if the struct is a unit struct (contains no fields) | ||
if !fields.is_empty() { | ||
continue; | ||
} | ||
|
||
span_lint_hir_and_then( | ||
cx, | ||
trait_to_implement.lint(), | ||
// This tells `rustc` where to search for `#[allow(...)]` attributes. | ||
component.hir_id, | ||
component.item_span, | ||
format!( | ||
"defined a unit struct without a `{}` implementation", | ||
trait_to_implement.name() | ||
), | ||
|diag| { | ||
diag.span_note(component.impl_span, "`Component` implemented here") | ||
.suggest_item_with_attr( | ||
cx, | ||
component.item_span, | ||
format!( | ||
"`{}` can be automatically derived", | ||
trait_to_implement.name() | ||
) | ||
.as_str(), | ||
format!("#[derive({})]", trait_to_implement.name()).as_str(), | ||
// This lint is MachineApplicable, since we only lint structs | ||
// that do not have any | ||
// fields. This suggestion | ||
// may result in two consecutive | ||
// `#[derive(...)]` attributes, but `rustfmt` merges them | ||
// afterwards. | ||
Applicability::MachineApplicable, | ||
); | ||
}, | ||
); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
enum Trait { | ||
Copy, | ||
Clone, | ||
Default, | ||
} | ||
|
||
impl Trait { | ||
const fn all() -> [Trait; 3] { | ||
use Trait::*; | ||
|
||
[Copy, Clone, Default] | ||
} | ||
|
||
const fn name(&self) -> &'static str { | ||
match self { | ||
Trait::Copy => "Copy", | ||
Trait::Clone => "Clone", | ||
Trait::Default => "Default", | ||
} | ||
} | ||
|
||
const fn lint(&self) -> &'static Lint { | ||
match self { | ||
Trait::Copy => MISSING_COPY, | ||
Trait::Clone => MISSING_CLONE, | ||
Trait::Default => MISSING_DEFAULT, | ||
} | ||
} | ||
|
||
fn get_def_id(&self, cx: &LateContext) -> std::option::Option<DefId> { | ||
match self { | ||
Trait::Copy => cx.tcx.get_diagnostic_item(rustc_span::symbol::sym::Copy), | ||
Trait::Clone => cx.tcx.get_diagnostic_item(rustc_span::symbol::sym::Clone), | ||
Trait::Default => cx.tcx.get_diagnostic_item(rustc_span::symbol::sym::Default), | ||
} | ||
} | ||
} |
This file contains hidden or 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 hidden or 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 |
---|---|---|
|
@@ -4,3 +4,4 @@ pub mod cargo; | |
pub mod hir_parse; | ||
pub mod method_call; | ||
mod panic; | ||
pub mod traits; |
This file contains hidden or 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,100 @@ | ||
use clippy_utils::paths::PathLookup; | ||
use rustc_hir::{HirId, Item, ItemKind, Node, OwnerId, QPath, TyKind, def::DefKind}; | ||
use rustc_lint::LateContext; | ||
use rustc_span::Span; | ||
|
||
/// Represents a type that implements a specific trait. | ||
#[derive(Debug)] | ||
pub struct TraitType { | ||
/// The [`HirId`] pointing to the type item declaration. | ||
pub hir_id: HirId, | ||
/// The span where the type was declared. | ||
pub item_span: Span, | ||
/// The span where the trait was implemented. | ||
pub impl_span: Span, | ||
} | ||
|
||
impl TraitType { | ||
pub fn from_local_crate<'tcx, 'a>( | ||
cx: &'a LateContext<'tcx>, | ||
trait_path: &'a PathLookup, | ||
) -> impl Iterator<Item = Self> + use<'tcx, 'a> { | ||
// Find the `DefId` of the trait. There may be multiple if there are multiple versions of | ||
// the same crate. | ||
let trait_def_ids = trait_path | ||
.get(cx) | ||
.iter() | ||
.filter(|&def_id| cx.tcx.def_kind(def_id) == DefKind::Trait); | ||
|
||
// Find a map of all trait `impl` items within the current crate. The key is the `DefId` of | ||
// the trait, and the value is a `Vec<LocalDefId>` for all `impl` items. | ||
let all_trait_impls = cx.tcx.all_local_trait_impls(()); | ||
|
||
// Find all `impl` items for the specific trait. | ||
let trait_impls = trait_def_ids | ||
.filter_map(|def_id| all_trait_impls.get(def_id)) | ||
.flatten() | ||
.copied(); | ||
|
||
// Map the `DefId`s of `impl` items to `TraitType`s. Sometimes this conversion can fail, so | ||
// we use `filter_map()` to skip errors. | ||
trait_impls.filter_map(move |local_def_id| { | ||
// Retrieve the node of the `impl` item from its `DefId`. | ||
let node = cx.tcx.hir_node_by_def_id(local_def_id); | ||
|
||
// Verify that it's an `impl` item and not something else. | ||
let Node::Item(Item { | ||
kind: ItemKind::Impl(impl_), | ||
span: impl_span, | ||
.. | ||
}) = node | ||
else { | ||
return None; | ||
}; | ||
|
||
// Find where `T` in `impl T` was originally defined, after peeling away all references | ||
// `&`. This was adapted from `clippy_utils::path_res()` in order to avoid passing | ||
// `LateContext` to this function. | ||
let def_id = match impl_.self_ty.peel_refs().kind { | ||
TyKind::Path(QPath::Resolved(_, path)) => path.res.opt_def_id()?, | ||
_ => return None, | ||
}; | ||
|
||
// Tries to convert the `DefId` to a `LocalDefId`, exiting early if it cannot be done. | ||
// This will only work if `T` in `impl T` is defined within the same crate. | ||
// | ||
// In most cases this will succeed due to Rust's orphan rule, but it notably fails | ||
// within `bevy_reflect` itself, since that crate implements `Reflect` for `std` types | ||
// such as `String`. | ||
let local_def_id = def_id.as_local()?; | ||
|
||
// Find the `HirId` from the `LocalDefId`. This is like a `DefId`, but with further | ||
// constraints on what it can represent. | ||
let hir_id = OwnerId { | ||
def_id: local_def_id, | ||
} | ||
.into(); | ||
|
||
// Find the span where the type was declared. This is guaranteed to be an item, so we | ||
// can safely call `expect_item()` without it panicking. | ||
let item_span = cx.tcx.hir_node(hir_id).expect_item().span; | ||
|
||
Some(TraitType { | ||
hir_id, | ||
item_span, | ||
impl_span: *impl_span, | ||
}) | ||
}) | ||
} | ||
} | ||
|
||
/// A custom equality implementation that just checks the [`HirId`] of the [`TraitType`], and skips | ||
/// the other values. | ||
/// | ||
/// [`TraitType`]s with equal [`HirId`]s are guaranteed to be equal in all other fields, so this | ||
/// takes advantage of that fact. | ||
impl PartialEq for TraitType { | ||
fn eq(&self, other: &Self) -> bool { | ||
self.hir_id == other.hir_id | ||
} | ||
} |
Oops, something went wrong.
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 did not want to create our own proc macro for this / use an extra crate but it's rather sad that there is no built-in way to do this.