From e6d82ca10d946ff2c6702427798e1b41a44a8df4 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Fri, 1 Nov 2024 15:16:28 -0700 Subject: [PATCH 1/6] Added `zst_query` lint --- bevy_lint/src/lib.rs | 2 + bevy_lint/src/lints/mod.rs | 3 + bevy_lint/src/lints/zst_query.rs | 179 ++++++++++++++++++++++ bevy_lint/tests/ui/zst_query/query.rs | 66 ++++++++ bevy_lint/tests/ui/zst_query/query.stderr | 39 +++++ 5 files changed, 289 insertions(+) create mode 100644 bevy_lint/src/lints/zst_query.rs create mode 100644 bevy_lint/tests/ui/zst_query/query.rs create mode 100644 bevy_lint/tests/ui/zst_query/query.stderr diff --git a/bevy_lint/src/lib.rs b/bevy_lint/src/lib.rs index 9758eda..3b68811 100644 --- a/bevy_lint/src/lib.rs +++ b/bevy_lint/src/lib.rs @@ -12,9 +12,11 @@ // This is a list of every single `rustc` crate used within this library. If you need another, add // it here! +extern crate rustc_abi; extern crate rustc_driver; extern crate rustc_errors; extern crate rustc_hir; +extern crate rustc_hir_analysis; extern crate rustc_interface; extern crate rustc_lint; extern crate rustc_middle; diff --git a/bevy_lint/src/lints/mod.rs b/bevy_lint/src/lints/mod.rs index 01246f3..d57368c 100644 --- a/bevy_lint/src/lints/mod.rs +++ b/bevy_lint/src/lints/mod.rs @@ -10,6 +10,7 @@ pub mod main_return_without_appexit; pub mod missing_reflect; pub mod panicking_methods; pub mod plugin_not_ending_in_plugin; +pub mod zst_query; pub(crate) static LINTS: &[&BevyLint] = &[ insert_event_resource::INSERT_EVENT_RESOURCE, @@ -18,6 +19,7 @@ pub(crate) static LINTS: &[&BevyLint] = &[ missing_reflect::MISSING_REFLECT, panicking_methods::PANICKING_WORLD_METHODS, plugin_not_ending_in_plugin::PLUGIN_NOT_ENDING_IN_PLUGIN, + zst_query::ZST_QUERY, ]; pub(crate) fn register_lints(store: &mut LintStore) { @@ -31,4 +33,5 @@ pub(crate) fn register_passes(store: &mut LintStore) { store.register_late_pass(|_| Box::new(missing_reflect::MissingReflect)); store.register_late_pass(|_| Box::new(panicking_methods::PanickingMethods)); store.register_late_pass(|_| Box::new(plugin_not_ending_in_plugin::PluginNotEndingInPlugin)); + store.register_late_pass(|_| Box::new(zst_query::ZstQuery)); } diff --git a/bevy_lint/src/lints/zst_query.rs b/bevy_lint/src/lints/zst_query.rs new file mode 100644 index 0000000..954d197 --- /dev/null +++ b/bevy_lint/src/lints/zst_query.rs @@ -0,0 +1,179 @@ +//! Checks for queries that query for a zero-sized type. +//! +//! # Motivation +//! +//! Zero-sized types (ZSTs) are types that have no size as a result of containing no runtime data. +//! In Bevy, such types are often used as marker components and are best used as filters. +//! +//! # Example +//! +//! ``` +//! # use bevy::prelude::*; +//! +//! #[derive(Component)] +//! struct Player; +//! +//! fn move_player(mut query: Query<(&mut Transform, &Player)>) { +//! // ... +//! } +//! ``` +//! +//! Use instead: +//! +//! ``` +//! # use bevy::prelude::*; +//! +//! #[derive(Component)] +//! struct Player; +//! +//! fn move_player(query: Query<&mut Transform, With>) { +//! // ... +//! } +//! ``` + +use crate::declare_bevy_lint; +use clippy_utils::{ + diagnostics::span_lint_and_help, + ty::{is_normalizable, match_type}, +}; +use rustc_abi::Size; +use rustc_hir::{Node, QPath}; +use rustc_hir_analysis::collect::ItemCtxt; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{ + layout::{LayoutOf, TyAndLayout}, + Ty, +}; +use rustc_session::declare_lint_pass; + +declare_bevy_lint! { + pub ZST_QUERY, + RESTRICTION, + "query for a zero-sized type" +} + +declare_lint_pass! { + ZstQuery => [ZST_QUERY.lint] +} + +impl<'tcx> LateLintPass<'tcx> for ZstQuery { + fn check_ty(&mut self, cx: &LateContext<'tcx>, hir_ty: &'tcx rustc_hir::Ty<'tcx>) { + let item_cx = ItemCtxt::new(cx.tcx, hir_ty.hir_id.owner.def_id); + let ty = item_cx.lower_ty(hir_ty); + + let Some(query_kind) = QueryKind::try_from_ty(cx, ty) else { + return; + }; + + let Some(query_data_ty) = generic_type_at(cx, hir_ty, 2) else { + return; + }; + + for hir_ty in detuple(*query_data_ty) { + let ty = item_cx.lower_ty(&hir_ty); + + // We want to make sure we're evaluating `Foo` and not `&Foo`/`&mut Foo` + let peeled = ty.peel_refs(); + + if !is_zero_sized(cx, peeled).unwrap_or_default() { + continue; + } + + // TODO: We can also special case `Option<&Foo>`/`Option<&mut Foo>` to + // instead suggest `Has` + span_lint_and_help( + cx, + ZST_QUERY.lint, + hir_ty.span, + ZST_QUERY.lint.desc, + None, + query_kind.help(&peeled), + ); + } + } +} + +enum QueryKind { + Query, +} + +impl QueryKind { + fn try_from_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option { + if match_type(cx, ty, &crate::paths::QUERY) { + Some(Self::Query) + } else { + None + } + } + + fn help(&self, ty: &Ty<'_>) -> String { + // It should be noted that `With` is not always the best filter to suggest. + // While it's most often going to be what users want, there's also `Added` + // and `Changed` which might be more appropriate in some cases + // (i.e. users are calling `foo.is_added()` or `foo.is_changed()` in the body of + // the system). + // In the future, we might want to span the usage site of `is_added()`/`is_changed()` + // and suggest to use `Added`/`Changed` instead. + match self { + Self::Query => format!("consider using a filter instead: `With<{ty}>`"), + } + } +} + +/// Checks if a type is zero-sized. +/// +/// Returns: +/// - `Some(true)` if the type is most likely a ZST +/// - `Some(false)` if the type is most likely not a ZST +/// - `None` if we cannot determine the size (e.g., type is not normalizable) +fn is_zero_sized<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option { + if !is_normalizable(cx, cx.param_env, ty) { + return None; + } + + let Ok(TyAndLayout { layout, .. }) = cx.layout_of(ty) else { + return None; + }; + + Some(layout.size() == Size::ZERO) +} + +/// Returns the list of types inside a tuple type. +/// +/// If the type is not a tuple, returns a list containing the type itself. +fn detuple<'hir>(ty: rustc_hir::Ty<'hir>) -> Vec> { + if let rustc_hir::TyKind::Tup(items) = ty.peel_refs().kind { + items.to_vec() + } else { + vec![ty] + } +} + +/// Gets the [`rustc_hir::Ty`] for a generic argument at the specified index. +/// +/// If the generic argument is not a type, returns `None`. +fn generic_type_at<'tcx>( + cx: &LateContext<'tcx>, + hir_ty: &'tcx rustc_hir::Ty<'tcx>, + index: usize, +) -> Option<&'tcx rustc_hir::Ty<'tcx>> { + let generic_arg = generic_at(hir_ty, index)?; + let generic_node = cx.tcx.hir_node(generic_arg.hir_id()); + + if let Node::Ty(ty) = generic_node { + Some(ty) + } else { + None + } +} +/// Returns the [`rustc_hir::GenericArg`] at the given index. +fn generic_at<'hir>( + hir_ty: &'hir rustc_hir::Ty<'hir>, + index: usize, +) -> Option<&'hir rustc_hir::GenericArg<'hir>> { + let rustc_hir::TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind else { + return None; + }; + + path.segments.last()?.args().args.get(index) +} diff --git a/bevy_lint/tests/ui/zst_query/query.rs b/bevy_lint/tests/ui/zst_query/query.rs new file mode 100644 index 0000000..a14d35f --- /dev/null +++ b/bevy_lint/tests/ui/zst_query/query.rs @@ -0,0 +1,66 @@ +//! This tests the `zst_query` lint, specifically when triggered on the `Query` type. + +#![feature(register_tool)] +#![register_tool(bevy)] +#![deny(bevy::zst_query)] + +use bevy::prelude::*; + +#[derive(Component)] +struct Foo; + +#[derive(Component)] +#[allow(dead_code)] +struct Bar(u32); + +#[derive(Component)] +#[allow(dead_code)] +struct Baz(T); + +fn main() { + App::new() + .add_systems( + Startup, + ( + immutable_zst, + mutable_zst, + immutable_zst_tuple, + mutable_zst_tuple, + immutable_query, + mutable_query, + generic_immutable_query::, + generic_mutable_query::, + immutable_query_tuple, + mutable_query_tuple, + ), + ) + .run(); +} + +//~| HELP: consider using a filter instead: `With` +//~v ERROR: query for a zero-sized type +fn immutable_zst(_query: Query<&Foo>) {} + +//~| HELP: consider using a filter instead: `With` +//~v ERROR: query for a zero-sized type +fn mutable_zst(_query: Query<&mut Foo>) {} + +//~| HELP: consider using a filter instead: `With` +//~v ERROR: query for a zero-sized type +fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} + +//~| HELP: consider using a filter instead: `With` +//~v ERROR: query for a zero-sized type +fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} + +fn immutable_query(_query: Query<&Bar>) {} + +fn mutable_query(_query: Query<&mut Bar>) {} + +fn generic_immutable_query(_query: Query<&Baz>) {} + +fn generic_mutable_query(_query: Query<&mut Baz>) {} + +fn immutable_query_tuple(_query: Query<(Entity, &Bar)>) {} + +fn mutable_query_tuple(_query: Query<(Entity, &mut Bar)>) {} diff --git a/bevy_lint/tests/ui/zst_query/query.stderr b/bevy_lint/tests/ui/zst_query/query.stderr new file mode 100644 index 0000000..e8705c6 --- /dev/null +++ b/bevy_lint/tests/ui/zst_query/query.stderr @@ -0,0 +1,39 @@ +error: query for a zero-sized type + --> tests/ui/zst_query/query.rs:42:32 + | +42 | fn immutable_zst(_query: Query<&Foo>) {} + | ^^^^ + | + = help: consider using a filter instead: `With` +note: the lint level is defined here + --> tests/ui/zst_query/query.rs:5:9 + | +5 | #![deny(bevy::zst_query)] + | ^^^^^^^^^^^^^^^ + +error: query for a zero-sized type + --> tests/ui/zst_query/query.rs:46:30 + | +46 | fn mutable_zst(_query: Query<&mut Foo>) {} + | ^^^^^^^^ + | + = help: consider using a filter instead: `With` + +error: query for a zero-sized type + --> tests/ui/zst_query/query.rs:50:47 + | +50 | fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} + | ^^^^ + | + = help: consider using a filter instead: `With` + +error: query for a zero-sized type + --> tests/ui/zst_query/query.rs:54:45 + | +54 | fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} + | ^^^^^^^^ + | + = help: consider using a filter instead: `With` + +error: aborting due to 4 previous errors + From 8860f3bdea008b62bbcbda2492165387d5a5dec6 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Fri, 1 Nov 2024 15:59:32 -0700 Subject: [PATCH 2/6] Moved helpers to `utils::hir_parse` module --- bevy_lint/src/lib.rs | 1 + bevy_lint/src/lints/zst_query.rs | 46 +++----------------------------- bevy_lint/src/utils/hir_parse.rs | 44 ++++++++++++++++++++++++++++++ bevy_lint/src/utils/mod.rs | 1 + 4 files changed, 50 insertions(+), 42 deletions(-) create mode 100644 bevy_lint/src/utils/hir_parse.rs create mode 100644 bevy_lint/src/utils/mod.rs diff --git a/bevy_lint/src/lib.rs b/bevy_lint/src/lib.rs index 3b68811..0ec702e 100644 --- a/bevy_lint/src/lib.rs +++ b/bevy_lint/src/lib.rs @@ -28,5 +28,6 @@ pub mod groups; mod lint; pub mod lints; mod paths; +mod utils; pub use self::callback::BevyLintCallback; diff --git a/bevy_lint/src/lints/zst_query.rs b/bevy_lint/src/lints/zst_query.rs index 954d197..267ffd8 100644 --- a/bevy_lint/src/lints/zst_query.rs +++ b/bevy_lint/src/lints/zst_query.rs @@ -31,13 +31,15 @@ //! } //! ``` -use crate::declare_bevy_lint; +use crate::{ + declare_bevy_lint, + utils::hir_parse::{detuple, generic_type_at}, +}; use clippy_utils::{ diagnostics::span_lint_and_help, ty::{is_normalizable, match_type}, }; use rustc_abi::Size; -use rustc_hir::{Node, QPath}; use rustc_hir_analysis::collect::ItemCtxt; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{ @@ -137,43 +139,3 @@ fn is_zero_sized<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option { Some(layout.size() == Size::ZERO) } - -/// Returns the list of types inside a tuple type. -/// -/// If the type is not a tuple, returns a list containing the type itself. -fn detuple<'hir>(ty: rustc_hir::Ty<'hir>) -> Vec> { - if let rustc_hir::TyKind::Tup(items) = ty.peel_refs().kind { - items.to_vec() - } else { - vec![ty] - } -} - -/// Gets the [`rustc_hir::Ty`] for a generic argument at the specified index. -/// -/// If the generic argument is not a type, returns `None`. -fn generic_type_at<'tcx>( - cx: &LateContext<'tcx>, - hir_ty: &'tcx rustc_hir::Ty<'tcx>, - index: usize, -) -> Option<&'tcx rustc_hir::Ty<'tcx>> { - let generic_arg = generic_at(hir_ty, index)?; - let generic_node = cx.tcx.hir_node(generic_arg.hir_id()); - - if let Node::Ty(ty) = generic_node { - Some(ty) - } else { - None - } -} -/// Returns the [`rustc_hir::GenericArg`] at the given index. -fn generic_at<'hir>( - hir_ty: &'hir rustc_hir::Ty<'hir>, - index: usize, -) -> Option<&'hir rustc_hir::GenericArg<'hir>> { - let rustc_hir::TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind else { - return None; - }; - - path.segments.last()?.args().args.get(index) -} diff --git a/bevy_lint/src/utils/hir_parse.rs b/bevy_lint/src/utils/hir_parse.rs new file mode 100644 index 0000000..c18620a --- /dev/null +++ b/bevy_lint/src/utils/hir_parse.rs @@ -0,0 +1,44 @@ +//! Utility functions for parsing HIR types. + +use rustc_hir::{GenericArg, Node, QPath, Ty, TyKind}; +use rustc_lint::LateContext; + +/// Returns the list of types inside a tuple type. +/// +/// If the type is not a tuple, returns a list containing the type itself. +pub(crate) fn detuple<'hir>(ty: Ty<'hir>) -> Vec> { + if let TyKind::Tup(items) = ty.peel_refs().kind { + items.to_vec() + } else { + vec![ty] + } +} + +/// Gets the [`Ty`] for a generic argument at the specified index. +/// +/// If the generic argument is not a type, returns `None`. +pub(crate) fn generic_type_at<'tcx>( + cx: &LateContext<'tcx>, + hir_ty: &'tcx Ty<'tcx>, + index: usize, +) -> Option<&'tcx Ty<'tcx>> { + let generic_arg = generic_at(hir_ty, index)?; + let generic_node = cx.tcx.hir_node(generic_arg.hir_id()); + + if let Node::Ty(ty) = generic_node { + Some(ty) + } else { + None + } +} +/// Returns the [`GenericArg`] at the given index. +pub(crate) fn generic_at<'hir>( + hir_ty: &'hir Ty<'hir>, + index: usize, +) -> Option<&'hir GenericArg<'hir>> { + let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind else { + return None; + }; + + path.segments.last()?.args().args.get(index) +} diff --git a/bevy_lint/src/utils/mod.rs b/bevy_lint/src/utils/mod.rs new file mode 100644 index 0000000..8a76bd1 --- /dev/null +++ b/bevy_lint/src/utils/mod.rs @@ -0,0 +1 @@ +pub(crate) mod hir_parse; From c1e886b7d45ce760eb0a5cc1b43ecf533e9a7f47 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Fri, 1 Nov 2024 22:36:05 -0700 Subject: [PATCH 3/6] Fix clippy --- bevy_lint/src/utils/hir_parse.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bevy_lint/src/utils/hir_parse.rs b/bevy_lint/src/utils/hir_parse.rs index c18620a..2d1fbd6 100644 --- a/bevy_lint/src/utils/hir_parse.rs +++ b/bevy_lint/src/utils/hir_parse.rs @@ -6,7 +6,7 @@ use rustc_lint::LateContext; /// Returns the list of types inside a tuple type. /// /// If the type is not a tuple, returns a list containing the type itself. -pub(crate) fn detuple<'hir>(ty: Ty<'hir>) -> Vec> { +pub(crate) fn detuple(ty: Ty<'_>) -> Vec> { if let TyKind::Tup(items) = ty.peel_refs().kind { items.to_vec() } else { From 43e59d9640b0d5b3c15762fa4766068a3fa3a2c0 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Sat, 2 Nov 2024 10:08:22 -0700 Subject: [PATCH 4/6] Add test for unit type --- bevy_lint/tests/ui/zst_query/query.rs | 3 +++ bevy_lint/tests/ui/zst_query/query.stderr | 16 ++++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/bevy_lint/tests/ui/zst_query/query.rs b/bevy_lint/tests/ui/zst_query/query.rs index a14d35f..6e911fc 100644 --- a/bevy_lint/tests/ui/zst_query/query.rs +++ b/bevy_lint/tests/ui/zst_query/query.rs @@ -22,6 +22,7 @@ fn main() { .add_systems( Startup, ( + unit_query, immutable_zst, mutable_zst, immutable_zst_tuple, @@ -37,6 +38,8 @@ fn main() { .run(); } +fn unit_query(_query: Query<()>) {} + //~| HELP: consider using a filter instead: `With` //~v ERROR: query for a zero-sized type fn immutable_zst(_query: Query<&Foo>) {} diff --git a/bevy_lint/tests/ui/zst_query/query.stderr b/bevy_lint/tests/ui/zst_query/query.stderr index e8705c6..39212ab 100644 --- a/bevy_lint/tests/ui/zst_query/query.stderr +++ b/bevy_lint/tests/ui/zst_query/query.stderr @@ -1,7 +1,7 @@ error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:42:32 + --> tests/ui/zst_query/query.rs:45:32 | -42 | fn immutable_zst(_query: Query<&Foo>) {} +45 | fn immutable_zst(_query: Query<&Foo>) {} | ^^^^ | = help: consider using a filter instead: `With` @@ -12,25 +12,25 @@ note: the lint level is defined here | ^^^^^^^^^^^^^^^ error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:46:30 + --> tests/ui/zst_query/query.rs:49:30 | -46 | fn mutable_zst(_query: Query<&mut Foo>) {} +49 | fn mutable_zst(_query: Query<&mut Foo>) {} | ^^^^^^^^ | = help: consider using a filter instead: `With` error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:50:47 + --> tests/ui/zst_query/query.rs:53:47 | -50 | fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} +53 | fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} | ^^^^ | = help: consider using a filter instead: `With` error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:54:45 + --> tests/ui/zst_query/query.rs:57:45 | -54 | fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} +57 | fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} | ^^^^^^^^ | = help: consider using a filter instead: `With` From 7463962c6960730c3dc2ae72059f6501d94c4d6b Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Sat, 2 Nov 2024 10:13:39 -0700 Subject: [PATCH 5/6] Added test for PhantomData components --- bevy_lint/tests/ui/zst_query/query.rs | 10 +++++++++ bevy_lint/tests/ui/zst_query/query.stderr | 26 +++++++++++++++-------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/bevy_lint/tests/ui/zst_query/query.rs b/bevy_lint/tests/ui/zst_query/query.rs index 6e911fc..7b244ae 100644 --- a/bevy_lint/tests/ui/zst_query/query.rs +++ b/bevy_lint/tests/ui/zst_query/query.rs @@ -5,6 +5,7 @@ #![deny(bevy::zst_query)] use bevy::prelude::*; +use std::marker::PhantomData; #[derive(Component)] struct Foo; @@ -17,6 +18,10 @@ struct Bar(u32); #[allow(dead_code)] struct Baz(T); +#[derive(Component)] +#[allow(dead_code)] +struct Phantom(PhantomData); + fn main() { App::new() .add_systems( @@ -33,6 +38,7 @@ fn main() { generic_mutable_query::, immutable_query_tuple, mutable_query_tuple, + phantom_data_query, ), ) .run(); @@ -67,3 +73,7 @@ fn generic_mutable_query(_query: Query<&mut Ba fn immutable_query_tuple(_query: Query<(Entity, &Bar)>) {} fn mutable_query_tuple(_query: Query<(Entity, &mut Bar)>) {} + +//~| HELP: consider using a filter instead: `With>` +//~v ERROR: query for a zero-sized type +fn phantom_data_query(_query: Query<&Phantom>) {} diff --git a/bevy_lint/tests/ui/zst_query/query.stderr b/bevy_lint/tests/ui/zst_query/query.stderr index 39212ab..bf0515d 100644 --- a/bevy_lint/tests/ui/zst_query/query.stderr +++ b/bevy_lint/tests/ui/zst_query/query.stderr @@ -1,7 +1,7 @@ error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:45:32 + --> tests/ui/zst_query/query.rs:51:32 | -45 | fn immutable_zst(_query: Query<&Foo>) {} +51 | fn immutable_zst(_query: Query<&Foo>) {} | ^^^^ | = help: consider using a filter instead: `With` @@ -12,28 +12,36 @@ note: the lint level is defined here | ^^^^^^^^^^^^^^^ error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:49:30 + --> tests/ui/zst_query/query.rs:55:30 | -49 | fn mutable_zst(_query: Query<&mut Foo>) {} +55 | fn mutable_zst(_query: Query<&mut Foo>) {} | ^^^^^^^^ | = help: consider using a filter instead: `With` error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:53:47 + --> tests/ui/zst_query/query.rs:59:47 | -53 | fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} +59 | fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} | ^^^^ | = help: consider using a filter instead: `With` error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:57:45 + --> tests/ui/zst_query/query.rs:63:45 | -57 | fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} +63 | fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} | ^^^^^^^^ | = help: consider using a filter instead: `With` -error: aborting due to 4 previous errors +error: query for a zero-sized type + --> tests/ui/zst_query/query.rs:79:37 + | +79 | fn phantom_data_query(_query: Query<&Phantom>) {} + | ^^^^^^^^^^^^^ + | + = help: consider using a filter instead: `With>` + +error: aborting due to 5 previous errors From e4a80fa2d99b9e84b843a0db1823ec3582b4640b Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Sat, 9 Nov 2024 17:38:45 -0700 Subject: [PATCH 6/6] Address code review feedback --- bevy_lint/src/lints/zst_query.rs | 14 ++++--- bevy_lint/src/utils/hir_parse.rs | 12 +++--- bevy_lint/src/utils/mod.rs | 2 +- bevy_lint/tests/ui/zst_query/query.rs | 48 +++++++++++++--------- bevy_lint/tests/ui/zst_query/query.stderr | 50 +++++++++++++++-------- 5 files changed, 78 insertions(+), 48 deletions(-) diff --git a/bevy_lint/src/lints/zst_query.rs b/bevy_lint/src/lints/zst_query.rs index 267ffd8..4741f37 100644 --- a/bevy_lint/src/lints/zst_query.rs +++ b/bevy_lint/src/lints/zst_query.rs @@ -129,13 +129,17 @@ impl QueryKind { /// - `Some(false)` if the type is most likely not a ZST /// - `None` if we cannot determine the size (e.g., type is not normalizable) fn is_zero_sized<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option { + // `cx.layout_of()` panics if the type is not normalizable. if !is_normalizable(cx, cx.param_env, ty) { return None; } - let Ok(TyAndLayout { layout, .. }) = cx.layout_of(ty) else { - return None; - }; - - Some(layout.size() == Size::ZERO) + // Note: we don't use `approx_ty_size` from `clippy_utils` here + // because it will return `0` as the default value if the type is not + // normalizable, which will put us at risk of emitting more false positives. + if let Ok(TyAndLayout { layout, .. }) = cx.layout_of(ty) { + Some(layout.size() == Size::ZERO) + } else { + None + } } diff --git a/bevy_lint/src/utils/hir_parse.rs b/bevy_lint/src/utils/hir_parse.rs index 2d1fbd6..b93aaa2 100644 --- a/bevy_lint/src/utils/hir_parse.rs +++ b/bevy_lint/src/utils/hir_parse.rs @@ -6,7 +6,10 @@ use rustc_lint::LateContext; /// Returns the list of types inside a tuple type. /// /// If the type is not a tuple, returns a list containing the type itself. -pub(crate) fn detuple(ty: Ty<'_>) -> Vec> { +/// +/// This function will work for both tuples and references to tuples, +/// such as `(f32, &str)` and `&(f32, &str)`. +pub fn detuple(ty: Ty<'_>) -> Vec> { if let TyKind::Tup(items) = ty.peel_refs().kind { items.to_vec() } else { @@ -17,7 +20,7 @@ pub(crate) fn detuple(ty: Ty<'_>) -> Vec> { /// Gets the [`Ty`] for a generic argument at the specified index. /// /// If the generic argument is not a type, returns `None`. -pub(crate) fn generic_type_at<'tcx>( +pub fn generic_type_at<'tcx>( cx: &LateContext<'tcx>, hir_ty: &'tcx Ty<'tcx>, index: usize, @@ -32,10 +35,7 @@ pub(crate) fn generic_type_at<'tcx>( } } /// Returns the [`GenericArg`] at the given index. -pub(crate) fn generic_at<'hir>( - hir_ty: &'hir Ty<'hir>, - index: usize, -) -> Option<&'hir GenericArg<'hir>> { +pub fn generic_at<'hir>(hir_ty: &'hir Ty<'hir>, index: usize) -> Option<&'hir GenericArg<'hir>> { let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind else { return None; }; diff --git a/bevy_lint/src/utils/mod.rs b/bevy_lint/src/utils/mod.rs index 8a76bd1..94a682b 100644 --- a/bevy_lint/src/utils/mod.rs +++ b/bevy_lint/src/utils/mod.rs @@ -1 +1 @@ -pub(crate) mod hir_parse; +pub mod hir_parse; diff --git a/bevy_lint/tests/ui/zst_query/query.rs b/bevy_lint/tests/ui/zst_query/query.rs index 7b244ae..f056726 100644 --- a/bevy_lint/tests/ui/zst_query/query.rs +++ b/bevy_lint/tests/ui/zst_query/query.rs @@ -8,15 +8,15 @@ use bevy::prelude::*; use std::marker::PhantomData; #[derive(Component)] -struct Foo; +struct ZST; #[derive(Component)] #[allow(dead_code)] -struct Bar(u32); +struct NonZST(u32); #[derive(Component)] #[allow(dead_code)] -struct Baz(T); +struct Generic(T); #[derive(Component)] #[allow(dead_code)] @@ -35,6 +35,8 @@ fn main() { immutable_query, mutable_query, generic_immutable_query::, + generic_immutable_zst, + generic_mutable_zst, generic_mutable_query::, immutable_query_tuple, mutable_query_tuple, @@ -46,34 +48,42 @@ fn main() { fn unit_query(_query: Query<()>) {} -//~| HELP: consider using a filter instead: `With` +//~| HELP: consider using a filter instead: `With` //~v ERROR: query for a zero-sized type -fn immutable_zst(_query: Query<&Foo>) {} +fn immutable_zst(_query: Query<&ZST>) {} -//~| HELP: consider using a filter instead: `With` +//~| HELP: consider using a filter instead: `With` //~v ERROR: query for a zero-sized type -fn mutable_zst(_query: Query<&mut Foo>) {} +fn mutable_zst(_query: Query<&mut ZST>) {} -//~| HELP: consider using a filter instead: `With` +//~| HELP: consider using a filter instead: `With` //~v ERROR: query for a zero-sized type -fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} +fn immutable_zst_tuple(_query: Query<(Entity, &ZST)>) {} -//~| HELP: consider using a filter instead: `With` +//~| HELP: consider using a filter instead: `With` //~v ERROR: query for a zero-sized type -fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} +fn mutable_zst_tuple(_query: Query<(Entity, &mut ZST)>) {} -fn immutable_query(_query: Query<&Bar>) {} +fn immutable_query(_query: Query<&NonZST>) {} -fn mutable_query(_query: Query<&mut Bar>) {} +fn mutable_query(_query: Query<&mut NonZST>) {} -fn generic_immutable_query(_query: Query<&Baz>) {} +fn generic_immutable_query(_query: Query<&Generic>) {} -fn generic_mutable_query(_query: Query<&mut Baz>) {} +fn generic_mutable_query(_query: Query<&mut Generic>) {} -fn immutable_query_tuple(_query: Query<(Entity, &Bar)>) {} +//~| HELP: consider using a filter instead: `With>` +//~v ERROR: query for a zero-sized type +fn generic_immutable_zst(_query: Query<&Generic>) {} + +//~| HELP: consider using a filter instead: `With>` +//~v ERROR: query for a zero-sized type +fn generic_mutable_zst(_query: Query<&mut Generic>) {} + +fn immutable_query_tuple(_query: Query<(Entity, &NonZST)>) {} -fn mutable_query_tuple(_query: Query<(Entity, &mut Bar)>) {} +fn mutable_query_tuple(_query: Query<(Entity, &mut NonZST)>) {} -//~| HELP: consider using a filter instead: `With>` +//~| HELP: consider using a filter instead: `With>` //~v ERROR: query for a zero-sized type -fn phantom_data_query(_query: Query<&Phantom>) {} +fn phantom_data_query(_query: Query<&Phantom>) {} diff --git a/bevy_lint/tests/ui/zst_query/query.stderr b/bevy_lint/tests/ui/zst_query/query.stderr index bf0515d..b3291dd 100644 --- a/bevy_lint/tests/ui/zst_query/query.stderr +++ b/bevy_lint/tests/ui/zst_query/query.stderr @@ -1,10 +1,10 @@ error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:51:32 + --> tests/ui/zst_query/query.rs:53:32 | -51 | fn immutable_zst(_query: Query<&Foo>) {} +53 | fn immutable_zst(_query: Query<&ZST>) {} | ^^^^ | - = help: consider using a filter instead: `With` + = help: consider using a filter instead: `With` note: the lint level is defined here --> tests/ui/zst_query/query.rs:5:9 | @@ -12,36 +12,52 @@ note: the lint level is defined here | ^^^^^^^^^^^^^^^ error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:55:30 + --> tests/ui/zst_query/query.rs:57:30 | -55 | fn mutable_zst(_query: Query<&mut Foo>) {} +57 | fn mutable_zst(_query: Query<&mut ZST>) {} | ^^^^^^^^ | - = help: consider using a filter instead: `With` + = help: consider using a filter instead: `With` error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:59:47 + --> tests/ui/zst_query/query.rs:61:47 | -59 | fn immutable_zst_tuple(_query: Query<(Entity, &Foo)>) {} +61 | fn immutable_zst_tuple(_query: Query<(Entity, &ZST)>) {} | ^^^^ | - = help: consider using a filter instead: `With` + = help: consider using a filter instead: `With` error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:63:45 + --> tests/ui/zst_query/query.rs:65:45 | -63 | fn mutable_zst_tuple(_query: Query<(Entity, &mut Foo)>) {} +65 | fn mutable_zst_tuple(_query: Query<(Entity, &mut ZST)>) {} | ^^^^^^^^ | - = help: consider using a filter instead: `With` + = help: consider using a filter instead: `With` error: query for a zero-sized type - --> tests/ui/zst_query/query.rs:79:37 + --> tests/ui/zst_query/query.rs:77:40 | -79 | fn phantom_data_query(_query: Query<&Phantom>) {} - | ^^^^^^^^^^^^^ +77 | fn generic_immutable_zst(_query: Query<&Generic>) {} + | ^^^^^^^^^^^^^ | - = help: consider using a filter instead: `With>` + = help: consider using a filter instead: `With>` -error: aborting due to 5 previous errors +error: query for a zero-sized type + --> tests/ui/zst_query/query.rs:81:38 + | +81 | fn generic_mutable_zst(_query: Query<&mut Generic>) {} + | ^^^^^^^^^^^^^^^^^ + | + = help: consider using a filter instead: `With>` + +error: query for a zero-sized type + --> tests/ui/zst_query/query.rs:89:37 + | +89 | fn phantom_data_query(_query: Query<&Phantom>) {} + | ^^^^^^^^^^^^^^^^ + | + = help: consider using a filter instead: `With>` + +error: aborting due to 7 previous errors