From 45617bcb4e34a4a09a28d8cca06bcb8e80ddbc3f Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Sun, 1 Aug 2021 20:06:25 +0200 Subject: [PATCH 1/9] Add a Bevy Linter --- .github/workflows/ci.yml | 33 +++ Cargo.toml | 7 +- crates/bevy_dylint/.cargo/config.toml | 8 + crates/bevy_dylint/Cargo.toml | 26 ++ crates/bevy_dylint/rust-toolchain | 3 + crates/bevy_dylint/src/bevy_paths.rs | 5 + crates/bevy_dylint/src/lib.rs | 39 +++ crates/bevy_dylint/src/unnecessary_with.rs | 223 ++++++++++++++++++ crates/bevy_dylint/ui/unnecessary_with.rs | 37 +++ crates/bevy_dylint/ui/unnecessary_with.stderr | 148 ++++++++++++ 10 files changed, 528 insertions(+), 1 deletion(-) create mode 100644 crates/bevy_dylint/.cargo/config.toml create mode 100644 crates/bevy_dylint/Cargo.toml create mode 100644 crates/bevy_dylint/rust-toolchain create mode 100644 crates/bevy_dylint/src/bevy_paths.rs create mode 100644 crates/bevy_dylint/src/lib.rs create mode 100644 crates/bevy_dylint/src/unnecessary_with.rs create mode 100644 crates/bevy_dylint/ui/unnecessary_with.rs create mode 100644 crates/bevy_dylint/ui/unnecessary_with.stderr diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e8876a38cebdd..69b26de05be09 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,6 +70,39 @@ jobs: # See tools/ci/src/main.rs for the commands this runs run: cargo run -p ci + dylint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions/cache@v2 + id: cache + with: + path: | + ~/.cargo/bin/ + ~/.cargo/registry/index/ + ~/.cargo/registry/cache/ + ~/.cargo/git/db/ + ~/.dylint_drivers + target/ + crates/bevy_dylint/target + key: ${{ runner.os }}-cargo-dylint-${{ hashFiles('**/Cargo.toml') }} + - name: Install alsa and udev + run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev + - name: Install dylint + run: cargo install cargo-dylint; cargo install dylint-link + if: steps.cache.outputs.cache-hit != 'true' + - name: Run dylint + run: cargo dylint --all + - name: Check Format + run: cd crates/bevy_dylint; cargo fmt --all -- --check + - name: Check Clippy + run: cd crates/bevy_dylint; cargo clippy --all-targets --all-features -- -D warnings -A clippy::type_complexity + - name: Run Tests + run: cd crates/bevy_dylint; cargo test + env: + # Can be removed once https://github.com/trailofbits/dylint/pull/48 is merged and released + CARGO_TERM_COLOR: never + build-wasm: strategy: matrix: diff --git a/Cargo.toml b/Cargo.toml index f95b76f75a9ee..b41372f0e456d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,9 +16,14 @@ readme = "README.md" repository = "https://github.com/bevyengine/bevy" [workspace] -exclude = ["benches"] +exclude = ["benches", "crates/bevy_dylint"] members = ["crates/*", "examples/ios", "tools/ci"] +[workspace.metadata.dylint] +libraries = [ + { path = "crates/bevy_dylint" }, +] + [features] default = [ "bevy_audio", diff --git a/crates/bevy_dylint/.cargo/config.toml b/crates/bevy_dylint/.cargo/config.toml new file mode 100644 index 0000000000000..d624ca4c74caf --- /dev/null +++ b/crates/bevy_dylint/.cargo/config.toml @@ -0,0 +1,8 @@ +[target.x86_64-apple-darwin] +linker = "dylint-link" + +[target.x86_64-unknown-linux-gnu] +linker = "dylint-link" + +[target.x86_64-pc-windows-msvc] +linker = "dylint-link" diff --git a/crates/bevy_dylint/Cargo.toml b/crates/bevy_dylint/Cargo.toml new file mode 100644 index 0000000000000..76644f3bb96c2 --- /dev/null +++ b/crates/bevy_dylint/Cargo.toml @@ -0,0 +1,26 @@ +[package] +name = "bevy_dylint" +version = "0.1.0" +authors = ["Bevy Contributors "] +description = "decription goes here" +edition = "2018" +publish = false + +[package.metadata.rust-analyzer] +rustc_private=true + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", tag = "rust-1.54.0"} +dylint_linting = "1.0.1" +if_chain = "1.0.1" + +[dev-dependencies] +dylint_testing = "1.0.1" +bevy_ecs = { path = "../bevy_ecs"} + +[[example]] +name = "unnecessary_with" +path = "ui/unnecessary_with.rs" diff --git a/crates/bevy_dylint/rust-toolchain b/crates/bevy_dylint/rust-toolchain new file mode 100644 index 0000000000000..8bd33e1ddd848 --- /dev/null +++ b/crates/bevy_dylint/rust-toolchain @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2021-06-03" +components = ["llvm-tools-preview", "rustc-dev", "rustfmt", "clippy"] diff --git a/crates/bevy_dylint/src/bevy_paths.rs b/crates/bevy_dylint/src/bevy_paths.rs new file mode 100644 index 0000000000000..cb643714853cc --- /dev/null +++ b/crates/bevy_dylint/src/bevy_paths.rs @@ -0,0 +1,5 @@ +pub const ADDED: &[&str] = &["bevy_ecs", "query", "filter", "Added"]; +pub const CHANGED: &[&str] = &["bevy_ecs", "query", "filter", "Changed"]; +pub const OR: &[&str] = &["bevy_ecs", "query", "filter", "Or"]; +pub const WITH: &[&str] = &["bevy_ecs", "query", "filter", "With"]; +pub const QUERY: &[&str] = &["bevy_ecs", "system", "query", "Query"]; diff --git a/crates/bevy_dylint/src/lib.rs b/crates/bevy_dylint/src/lib.rs new file mode 100644 index 0000000000000..6f8b684f6408c --- /dev/null +++ b/crates/bevy_dylint/src/lib.rs @@ -0,0 +1,39 @@ +#![feature(rustc_private)] +//#![warn(unused_extern_crates)] + +dylint_linting::dylint_library!(); + +extern crate rustc_ast; +extern crate rustc_ast_pretty; +extern crate rustc_attr; +extern crate rustc_data_structures; +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_hir_pretty; +extern crate rustc_index; +extern crate rustc_infer; +extern crate rustc_lexer; +extern crate rustc_lint; +extern crate rustc_middle; +extern crate rustc_mir; +extern crate rustc_parse; +extern crate rustc_parse_format; +extern crate rustc_session; +extern crate rustc_span; +extern crate rustc_target; +extern crate rustc_trait_selection; +extern crate rustc_typeck; + +mod bevy_paths; +mod unnecessary_with; + +#[no_mangle] +pub fn register_lints(_sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore) { + lint_store.register_lints(&[unnecessary_with::UNNECESSARY_WITH]); + lint_store.register_late_pass(|| Box::new(unnecessary_with::UnnecessaryWith)); +} + +#[test] +fn ui() { + dylint_testing::ui_test_examples(env!("CARGO_PKG_NAME")); +} diff --git a/crates/bevy_dylint/src/unnecessary_with.rs b/crates/bevy_dylint/src/unnecessary_with.rs new file mode 100644 index 0000000000000..a66c057047e94 --- /dev/null +++ b/crates/bevy_dylint/src/unnecessary_with.rs @@ -0,0 +1,223 @@ +use clippy_utils::diagnostics::span_lint; +use if_chain::if_chain; +use rustc_hir::{ + def::Res, hir_id::HirId, intravisit::FnKind, Body, FnDecl, GenericArg, MutTy, Path, QPath, Ty, + TyKind, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint, declare_lint_pass}; +use rustc_span::{def_id::DefId, symbol::Symbol, Span}; + +use crate::bevy_paths; + +declare_lint! { + /// **What it does:** + /// Detectes unnecessary instances of the `With` + /// **Why is this bad?** + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// # use bevy_ecs::system::Query; + /// # use bevy_ecs::query::With; + /// fn system(query: Query<&A, With>) {} + /// ``` + /// Use instead: + /// ```rust + /// # use bevy_ecs::system::Query; + /// fn system(query: Query<&A>) {} + /// ``` + pub UNNECESSARY_WITH, + Warn, + "description goes here" +} + +declare_lint_pass!(UnnecessaryWith => [UNNECESSARY_WITH]); + +impl<'hir> LateLintPass<'hir> for UnnecessaryWith { + // A list of things you might check can be found here: + // https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html + + fn check_fn( + &mut self, + ctx: &LateContext<'hir>, + _: FnKind<'hir>, + decl: &'hir FnDecl<'hir>, + _: &'hir Body<'hir>, + _: Span, + _: HirId, + ) { + for typ in decl.inputs { + if_chain! { + if let TyKind::Path(QPath::Resolved(_, path)) = &typ.kind; + if path_matches_symbol_path(ctx, path, bevy_paths::QUERY); + if let Some(segment) = path.segments.iter().last(); + if let Some(generic_args) = segment.args; + if let Some(GenericArg::Type(world)) = &generic_args.args.get(1); + if let Some(GenericArg::Type(filter)) = &generic_args.args.get(2); + then { + check_for_overlap(ctx, world, filter); + } + } + } + } +} + +fn check_for_overlap<'hir>(ctx: &LateContext<'hir>, world: &Ty, filter: &Ty) { + let mut required_types = Vec::new(); + let mut with_types = Vec::new(); + + match &world.kind { + TyKind::Rptr(_, mut_type) => { + if let Some(def_id) = get_def_id_of_reference(&mut_type) { + required_types.push(def_id); + } + } + TyKind::Tup(types) => { + for typ in *types { + if let TyKind::Rptr(_, mut_type) = &typ.kind { + if let Some(def_id) = get_def_id_of_reference(&mut_type) { + required_types.push(def_id); + } + } + } + } + _ => (), + } + + match &filter.kind { + TyKind::Path(QPath::Resolved(_, path)) => { + if path_matches_symbol_path(ctx, path, bevy_paths::OR) { + with_types.extend(check_or_filter(ctx, path)); + } + if path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + with_types.push((def_id, filter.span)); + } + } + } + TyKind::Tup(types) => { + for typ in *types { + if let TyKind::Path(QPath::Resolved(_, path)) = typ.kind { + if path_matches_symbol_path(ctx, path, bevy_paths::OR) { + with_types.extend(check_or_filter(ctx, path)); + } + if path_matches_symbol_path(ctx, path, bevy_paths::ADDED) + || path_matches_symbol_path(ctx, path, bevy_paths::CHANGED) + { + if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + required_types.push(def_id); + } + } + if path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + with_types.push((def_id, typ.span)); + } + } + } + } + } + _ => (), + } + + for with_type in with_types { + if required_types.contains(&with_type.0) { + span_lint( + ctx, + UNNECESSARY_WITH, + with_type.1, + "Unnecessary `With` Filter", + ); + } + } +} + +fn get_def_id_of_reference(reference: &MutTy) -> Option { + if let TyKind::Path(QPath::Resolved(_, path)) = reference.ty.kind { + if let Res::Def(_, def_id) = path.res { + return Some(def_id); + } + } + + None +} + +fn path_matches_symbol_path<'hir>( + ctx: &LateContext<'hir>, + path: &Path, + symbol_path: &[&str], +) -> bool { + if let Res::Def(_, def_id) = path.res { + return ctx.match_def_path( + def_id, + symbol_path + .iter() + .map(|str| Symbol::intern(str)) + .collect::>() + .as_slice(), + ); + }; + + false +} + +fn get_def_id_of_first_generic_arg(path: &Path) -> Option { + if let Some(segment) = path.segments.iter().last() { + if let Some(generic_args) = segment.args { + if let Some(GenericArg::Type(component)) = &generic_args.args.get(0) { + if let TyKind::Path(QPath::Resolved(_, path)) = component.kind { + if let Res::Def(_, def_id) = path.res { + return Some(def_id); + } + } + } + } + } + + None +} + +fn check_or_filter<'hir>(ctx: &LateContext<'hir>, path: &Path) -> Vec<(DefId, Span)> { + let mut local_required_types = Vec::new(); + let mut local_with_types = Vec::new(); + + if let Some(segment) = path.segments.iter().last() { + if let Some(generic_args) = segment.args { + if let GenericArg::Type(tuple) = &generic_args.args[0] { + if let TyKind::Tup(types) = tuple.kind { + for typ in types { + if let TyKind::Path(QPath::Resolved(_, path)) = typ.kind { + if path_matches_symbol_path(ctx, path, bevy_paths::ADDED) + || path_matches_symbol_path(ctx, path, bevy_paths::CHANGED) + { + if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + local_required_types.push(def_id); + } + } + if path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + local_with_types.push((def_id, typ.span)); + } + } + } + } + + for with_type in &local_with_types { + if local_required_types.contains(&with_type.0) { + span_lint( + ctx, + UNNECESSARY_WITH, + with_type.1, + "Unnecessary `With` Filter", + ); + } + } + } + } + } + } + + local_with_types +} diff --git a/crates/bevy_dylint/ui/unnecessary_with.rs b/crates/bevy_dylint/ui/unnecessary_with.rs new file mode 100644 index 0000000000000..e5b21c73ca60c --- /dev/null +++ b/crates/bevy_dylint/ui/unnecessary_with.rs @@ -0,0 +1,37 @@ +#![allow(dead_code)] +use bevy_ecs::prelude::*; + +fn main() {} + +struct A; +struct B; +struct C; + +fn test_query1(_query: Query<&A, With>) {} +fn test_query2(_query: Query<(&A, &B), With>) {} +fn test_query3(_query: Query<(&A, &B), With>) {} +fn test_query4(_query: Query<(&A, &B), (With, With)>) {} +fn test_query5(_query: Query<(&A, &B), (With, With)>) {} +fn test_query6(_query: Query<&A, (With, With)>) {} + +fn test_query7(_query: Query<&mut A, With>) {} +fn test_query8(_query: Query<(&mut A, &B), With>) {} +fn test_query9(_query: Query<(&mut A, &B), With>) {} +fn test_query10(_query: Query<(&mut A, &B), (With, With)>) {} +fn test_query11(_query: Query<(&mut A, &B), (With, With)>) {} +fn test_query12(_query: Query<&mut A, (With, With)>) {} + +fn test_query13(_query: Query<(), (Added, With)>) {} +fn test_query14(_query: Query<(), (Changed, With)>) {} + +fn test_query15(_query: Query<&A, Or<(With, With)>>) {} +fn test_query16(_query: Query<&B, Or<(With, With)>>) {} + +fn test_query17(_query: Query<&mut A, Or<(With, With)>>) {} +fn test_query18(_query: Query<&mut B, Or<(With, With)>>) {} + +fn test_query19(_query: Query<(), Or<(Added, With)>>) {} +fn test_query20(_query: Query<(), Or<(Changed, With)>>) {} + +fn test_query21(_query: Query<&A, (Or<(With, With)>, With)>) {} +fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) {} diff --git a/crates/bevy_dylint/ui/unnecessary_with.stderr b/crates/bevy_dylint/ui/unnecessary_with.stderr new file mode 100644 index 0000000000000..486da2e62d78a --- /dev/null +++ b/crates/bevy_dylint/ui/unnecessary_with.stderr @@ -0,0 +1,148 @@ +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:10:34 + | +LL | fn test_query1(_query: Query<&A, With>) {} + | ^^^^^^^ + | + = note: `-D unnecessary-with` implied by `-D warnings` + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:11:40 + | +LL | fn test_query2(_query: Query<(&A, &B), With>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:12:40 + | +LL | fn test_query3(_query: Query<(&A, &B), With>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:13:41 + | +LL | fn test_query4(_query: Query<(&A, &B), (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:13:50 + | +LL | fn test_query4(_query: Query<(&A, &B), (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:14:41 + | +LL | fn test_query5(_query: Query<(&A, &B), (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:15:35 + | +LL | fn test_query6(_query: Query<&A, (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:17:38 + | +LL | fn test_query7(_query: Query<&mut A, With>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:18:44 + | +LL | fn test_query8(_query: Query<(&mut A, &B), With>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:19:44 + | +LL | fn test_query9(_query: Query<(&mut A, &B), With>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:20:46 + | +LL | fn test_query10(_query: Query<(&mut A, &B), (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:20:55 + | +LL | fn test_query10(_query: Query<(&mut A, &B), (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:21:46 + | +LL | fn test_query11(_query: Query<(&mut A, &B), (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:22:40 + | +LL | fn test_query12(_query: Query<&mut A, (With, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:24:46 + | +LL | fn test_query13(_query: Query<(), (Added, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:25:48 + | +LL | fn test_query14(_query: Query<(), (Changed, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:27:39 + | +LL | fn test_query15(_query: Query<&A, Or<(With, With)>>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:28:48 + | +LL | fn test_query16(_query: Query<&B, Or<(With, With)>>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:30:43 + | +LL | fn test_query17(_query: Query<&mut A, Or<(With, With)>>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:31:52 + | +LL | fn test_query18(_query: Query<&mut B, Or<(With, With)>>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:33:49 + | +LL | fn test_query19(_query: Query<(), Or<(Added, With)>>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:34:51 + | +LL | fn test_query20(_query: Query<(), Or<(Changed, With)>>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:36:40 + | +LL | fn test_query21(_query: Query<&A, (Or<(With, With)>, With)>) {} + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:37:44 + | +LL | fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) {} + | ^^^^^^^ + +error: aborting due to 24 previous errors + From d99fb81b7db3b5ee4f5a2778919292920beaeb22 Mon Sep 17 00:00:00 2001 From: MinerSebas <66798382+MinerSebas@users.noreply.github.com> Date: Sun, 1 Aug 2021 22:47:38 +0200 Subject: [PATCH 2/9] Update crates/bevy_dylint/src/unnecessary_with.rs Co-authored-by: Alice Cecile --- crates/bevy_dylint/src/unnecessary_with.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_dylint/src/unnecessary_with.rs b/crates/bevy_dylint/src/unnecessary_with.rs index a66c057047e94..8b80b45d40468 100644 --- a/crates/bevy_dylint/src/unnecessary_with.rs +++ b/crates/bevy_dylint/src/unnecessary_with.rs @@ -31,7 +31,7 @@ declare_lint! { /// ``` pub UNNECESSARY_WITH, Warn, - "description goes here" + "Detects unnecessary `With` query filters in Bevy query parameters." } declare_lint_pass!(UnnecessaryWith => [UNNECESSARY_WITH]); From 627b5f7b5cba2e4ffd260f24b33007ff3c37ef69 Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Sun, 1 Aug 2021 22:56:13 +0200 Subject: [PATCH 3/9] Check whole workspace --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69b26de05be09..4c28ea49f2953 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,7 +92,7 @@ jobs: run: cargo install cargo-dylint; cargo install dylint-link if: steps.cache.outputs.cache-hit != 'true' - name: Run dylint - run: cargo dylint --all + run: cargo dylint --all --workspace - name: Check Format run: cd crates/bevy_dylint; cargo fmt --all -- --check - name: Check Clippy From f75bd60d799c9233f6a85e52eed95fabf82f30ec Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Mon, 2 Aug 2021 14:26:19 +0200 Subject: [PATCH 4/9] Rename crate to bevy_lint --- .github/workflows/ci.yml | 10 +++++----- Cargo.toml | 4 ++-- crates/{bevy_dylint => bevy_lint}/.cargo/config.toml | 0 crates/{bevy_dylint => bevy_lint}/Cargo.toml | 2 +- crates/{bevy_dylint => bevy_lint}/rust-toolchain | 0 crates/{bevy_dylint => bevy_lint}/src/bevy_paths.rs | 0 crates/{bevy_dylint => bevy_lint}/src/lib.rs | 0 .../{bevy_dylint => bevy_lint}/src/unnecessary_with.rs | 0 .../{bevy_dylint => bevy_lint}/ui/unnecessary_with.rs | 0 .../ui/unnecessary_with.stderr | 0 10 files changed, 8 insertions(+), 8 deletions(-) rename crates/{bevy_dylint => bevy_lint}/.cargo/config.toml (100%) rename crates/{bevy_dylint => bevy_lint}/Cargo.toml (96%) rename crates/{bevy_dylint => bevy_lint}/rust-toolchain (100%) rename crates/{bevy_dylint => bevy_lint}/src/bevy_paths.rs (100%) rename crates/{bevy_dylint => bevy_lint}/src/lib.rs (100%) rename crates/{bevy_dylint => bevy_lint}/src/unnecessary_with.rs (100%) rename crates/{bevy_dylint => bevy_lint}/ui/unnecessary_with.rs (100%) rename crates/{bevy_dylint => bevy_lint}/ui/unnecessary_with.stderr (100%) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4c28ea49f2953..4f3c588125ef8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,7 +70,7 @@ jobs: # See tools/ci/src/main.rs for the commands this runs run: cargo run -p ci - dylint: + bevy_lint: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 @@ -84,7 +84,7 @@ jobs: ~/.cargo/git/db/ ~/.dylint_drivers target/ - crates/bevy_dylint/target + crates/bevy_lint/target key: ${{ runner.os }}-cargo-dylint-${{ hashFiles('**/Cargo.toml') }} - name: Install alsa and udev run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev @@ -94,11 +94,11 @@ jobs: - name: Run dylint run: cargo dylint --all --workspace - name: Check Format - run: cd crates/bevy_dylint; cargo fmt --all -- --check + run: cd crates/bevy_lint; cargo fmt --all -- --check - name: Check Clippy - run: cd crates/bevy_dylint; cargo clippy --all-targets --all-features -- -D warnings -A clippy::type_complexity + run: cd crates/bevy_lint; cargo clippy --all-targets --all-features -- -D warnings -A clippy::type_complexity - name: Run Tests - run: cd crates/bevy_dylint; cargo test + run: cd crates/bevy_lint; cargo test env: # Can be removed once https://github.com/trailofbits/dylint/pull/48 is merged and released CARGO_TERM_COLOR: never diff --git a/Cargo.toml b/Cargo.toml index b41372f0e456d..ac97267c9b0e3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,12 +16,12 @@ readme = "README.md" repository = "https://github.com/bevyengine/bevy" [workspace] -exclude = ["benches", "crates/bevy_dylint"] +exclude = ["benches", "crates/bevy_lint"] members = ["crates/*", "examples/ios", "tools/ci"] [workspace.metadata.dylint] libraries = [ - { path = "crates/bevy_dylint" }, + { path = "crates/bevy_lint" }, ] [features] diff --git a/crates/bevy_dylint/.cargo/config.toml b/crates/bevy_lint/.cargo/config.toml similarity index 100% rename from crates/bevy_dylint/.cargo/config.toml rename to crates/bevy_lint/.cargo/config.toml diff --git a/crates/bevy_dylint/Cargo.toml b/crates/bevy_lint/Cargo.toml similarity index 96% rename from crates/bevy_dylint/Cargo.toml rename to crates/bevy_lint/Cargo.toml index 76644f3bb96c2..3efea114644f6 100644 --- a/crates/bevy_dylint/Cargo.toml +++ b/crates/bevy_lint/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "bevy_dylint" +name = "bevy_lint" version = "0.1.0" authors = ["Bevy Contributors "] description = "decription goes here" diff --git a/crates/bevy_dylint/rust-toolchain b/crates/bevy_lint/rust-toolchain similarity index 100% rename from crates/bevy_dylint/rust-toolchain rename to crates/bevy_lint/rust-toolchain diff --git a/crates/bevy_dylint/src/bevy_paths.rs b/crates/bevy_lint/src/bevy_paths.rs similarity index 100% rename from crates/bevy_dylint/src/bevy_paths.rs rename to crates/bevy_lint/src/bevy_paths.rs diff --git a/crates/bevy_dylint/src/lib.rs b/crates/bevy_lint/src/lib.rs similarity index 100% rename from crates/bevy_dylint/src/lib.rs rename to crates/bevy_lint/src/lib.rs diff --git a/crates/bevy_dylint/src/unnecessary_with.rs b/crates/bevy_lint/src/unnecessary_with.rs similarity index 100% rename from crates/bevy_dylint/src/unnecessary_with.rs rename to crates/bevy_lint/src/unnecessary_with.rs diff --git a/crates/bevy_dylint/ui/unnecessary_with.rs b/crates/bevy_lint/ui/unnecessary_with.rs similarity index 100% rename from crates/bevy_dylint/ui/unnecessary_with.rs rename to crates/bevy_lint/ui/unnecessary_with.rs diff --git a/crates/bevy_dylint/ui/unnecessary_with.stderr b/crates/bevy_lint/ui/unnecessary_with.stderr similarity index 100% rename from crates/bevy_dylint/ui/unnecessary_with.stderr rename to crates/bevy_lint/ui/unnecessary_with.stderr From a3b7171a91df0726ee3a3997344f908aa46bbf61 Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Mon, 2 Aug 2021 19:01:47 +0200 Subject: [PATCH 5/9] Add Readme --- .github/workflows/ci.yml | 2 +- crates/bevy_lint/Cargo.toml | 2 +- crates/bevy_lint/readme.md | 34 ++++++++++++++++++++++++ crates/bevy_lint/src/unnecessary_with.rs | 3 ++- 4 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 crates/bevy_lint/readme.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4f3c588125ef8..91ee034c2269b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,7 +92,7 @@ jobs: run: cargo install cargo-dylint; cargo install dylint-link if: steps.cache.outputs.cache-hit != 'true' - name: Run dylint - run: cargo dylint --all --workspace + run: cargo dylint --all --workspace -- --all-targets - name: Check Format run: cd crates/bevy_lint; cargo fmt --all -- --check - name: Check Clippy diff --git a/crates/bevy_lint/Cargo.toml b/crates/bevy_lint/Cargo.toml index 3efea114644f6..a235633c6a91b 100644 --- a/crates/bevy_lint/Cargo.toml +++ b/crates/bevy_lint/Cargo.toml @@ -2,7 +2,7 @@ name = "bevy_lint" version = "0.1.0" authors = ["Bevy Contributors "] -description = "decription goes here" +description = "Provides Lints for Bevy Code" edition = "2018" publish = false diff --git a/crates/bevy_lint/readme.md b/crates/bevy_lint/readme.md new file mode 100644 index 0000000000000..eb9a07f5c7d0c --- /dev/null +++ b/crates/bevy_lint/readme.md @@ -0,0 +1,34 @@ +# Bevy Lint + +## What is Bevy Lint? + +This crates provides Lints for Bevy Code using [dylint](https://github.com/trailofbits/dylint). + +## How to you run Lints + +Add this to your Cargo.toml: + +```toml +[workspace.metadata.dylint] +libraries = [ + { git = "https://github.com/bevyengine/bevy", tag = "v0.6.0", pattern = "crates/bevy_dylint" }, +] +``` + +Instead of a `tag`, you can also provide a `branch` or a `rev` (revision). + +Afterwards you need to run these commans: + +```sh +cargo install cargo-dylint dylint-link # Only neccesary once +cargo dylint bevy_dylint +``` + +## Lint Creation + +A Lint is created by implementing the [LateLintPass](https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html) trait and adding to the `register_lints` function. + +When creating a UI Test, add the Test as an Example to the [Cargo.toml](Cargo.toml). +Also make sure that your `.stderr` File uses `LF` Line-endings and not `CRLF`, as otherwise the Test will fail without any explanation. + +For more Resources you can take a look at the [dylint resources](https://github.com/trailofbits/dylint#resources). diff --git a/crates/bevy_lint/src/unnecessary_with.rs b/crates/bevy_lint/src/unnecessary_with.rs index 8b80b45d40468..8da19fb9425c2 100644 --- a/crates/bevy_lint/src/unnecessary_with.rs +++ b/crates/bevy_lint/src/unnecessary_with.rs @@ -12,8 +12,9 @@ use crate::bevy_paths; declare_lint! { /// **What it does:** - /// Detectes unnecessary instances of the `With` + /// Detects unnecessary `With` query filters in Bevy query parameters. /// **Why is this bad?** + /// The Filter does not effect the Results of a query, but still wasted space. /// /// **Known problems:** None. /// From 1be2fe183f45c98351c8f4423aa0238c63b1f022 Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Mon, 2 Aug 2021 23:34:00 +0200 Subject: [PATCH 6/9] Move helper functions to a separate Module --- crates/bevy_lint/src/bevy_helpers.rs | 73 +++++++++++++++ crates/bevy_lint/src/lib.rs | 1 + crates/bevy_lint/src/unnecessary_with.rs | 109 +++++++---------------- 3 files changed, 108 insertions(+), 75 deletions(-) create mode 100644 crates/bevy_lint/src/bevy_helpers.rs diff --git a/crates/bevy_lint/src/bevy_helpers.rs b/crates/bevy_lint/src/bevy_helpers.rs new file mode 100644 index 0000000000000..5a311c112c209 --- /dev/null +++ b/crates/bevy_lint/src/bevy_helpers.rs @@ -0,0 +1,73 @@ +use rustc_hir::{def::Res, GenericArg, MutTy, Path, QPath, Ty, TyKind}; +use rustc_lint::LateContext; +use rustc_span::{def_id::DefId, symbol::Symbol}; + +use crate::bevy_paths; + +pub fn path_matches_symbol_path<'hir>( + ctx: &LateContext<'hir>, + path: &Path, + symbol_path: &[&str], +) -> bool { + if let Res::Def(_, def_id) = path.res { + return ctx.match_def_path( + def_id, + symbol_path + .iter() + .map(|str| Symbol::intern(str)) + .collect::>() + .as_slice(), + ); + }; + + false +} + +pub fn get_def_id_of_referenced_type(reference: &MutTy) -> Option { + if let TyKind::Path(QPath::Resolved(_, path)) = reference.ty.kind { + if let Res::Def(_, def_id) = path.res { + return Some(def_id); + } + } + + None +} + +pub fn get_def_id_of_first_generic_arg(path: &Path) -> Option { + if let Some(segment) = path.segments.iter().last() { + if let Some(generic_args) = segment.args { + if let Some(GenericArg::Type(component)) = &generic_args.args.get(0) { + if let TyKind::Path(QPath::Resolved(_, path)) = component.kind { + if let Res::Def(_, def_id) = path.res { + return Some(def_id); + } + } + } + } + } + + None +} + +pub fn get_generics_of_query<'hir>( + ctx: &LateContext<'hir>, + query: &'hir Ty, +) -> Option<(&'hir Ty<'hir>, Option<&'hir Ty<'hir>>)> { + if let TyKind::Path(QPath::Resolved(_, path)) = query.kind { + if path_matches_symbol_path(ctx, path, bevy_paths::QUERY) { + if let Some(segment) = path.segments.iter().last() { + if let Some(generic_args) = segment.args { + if let Some(GenericArg::Type(world)) = &generic_args.args.get(1) { + if let Some(GenericArg::Type(filter)) = &generic_args.args.get(2) { + return Some((world, Some(filter))); + } + + return Some((world, None)); + } + } + } + } + } + + None +} diff --git a/crates/bevy_lint/src/lib.rs b/crates/bevy_lint/src/lib.rs index 6f8b684f6408c..fb96935b2d6da 100644 --- a/crates/bevy_lint/src/lib.rs +++ b/crates/bevy_lint/src/lib.rs @@ -24,6 +24,7 @@ extern crate rustc_target; extern crate rustc_trait_selection; extern crate rustc_typeck; +mod bevy_helpers; mod bevy_paths; mod unnecessary_with; diff --git a/crates/bevy_lint/src/unnecessary_with.rs b/crates/bevy_lint/src/unnecessary_with.rs index 8da19fb9425c2..3253550aaf2cc 100644 --- a/crates/bevy_lint/src/unnecessary_with.rs +++ b/crates/bevy_lint/src/unnecessary_with.rs @@ -1,14 +1,12 @@ use clippy_utils::diagnostics::span_lint; -use if_chain::if_chain; use rustc_hir::{ - def::Res, hir_id::HirId, intravisit::FnKind, Body, FnDecl, GenericArg, MutTy, Path, QPath, Ty, - TyKind, + hir_id::HirId, intravisit::FnKind, Body, FnDecl, GenericArg, Path, QPath, Ty, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint, declare_lint_pass}; -use rustc_span::{def_id::DefId, symbol::Symbol, Span}; +use rustc_span::{def_id::DefId, Span}; -use crate::bevy_paths; +use crate::{bevy_helpers, bevy_paths}; declare_lint! { /// **What it does:** @@ -51,15 +49,13 @@ impl<'hir> LateLintPass<'hir> for UnnecessaryWith { _: HirId, ) { for typ in decl.inputs { - if_chain! { - if let TyKind::Path(QPath::Resolved(_, path)) = &typ.kind; - if path_matches_symbol_path(ctx, path, bevy_paths::QUERY); - if let Some(segment) = path.segments.iter().last(); - if let Some(generic_args) = segment.args; - if let Some(GenericArg::Type(world)) = &generic_args.args.get(1); - if let Some(GenericArg::Type(filter)) = &generic_args.args.get(2); - then { - check_for_overlap(ctx, world, filter); + if let TyKind::Path(QPath::Resolved(_, path)) = &typ.kind { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::QUERY) { + if let Some((world, Some(filter))) = + bevy_helpers::get_generics_of_query(ctx, &typ) + { + check_for_overlap(ctx, world, filter); + } } } } @@ -72,14 +68,14 @@ fn check_for_overlap<'hir>(ctx: &LateContext<'hir>, world: &Ty, filter: &Ty) { match &world.kind { TyKind::Rptr(_, mut_type) => { - if let Some(def_id) = get_def_id_of_reference(&mut_type) { + if let Some(def_id) = bevy_helpers::get_def_id_of_referenced_type(&mut_type) { required_types.push(def_id); } } TyKind::Tup(types) => { for typ in *types { if let TyKind::Rptr(_, mut_type) = &typ.kind { - if let Some(def_id) = get_def_id_of_reference(&mut_type) { + if let Some(def_id) = bevy_helpers::get_def_id_of_referenced_type(&mut_type) { required_types.push(def_id); } } @@ -90,11 +86,11 @@ fn check_for_overlap<'hir>(ctx: &LateContext<'hir>, world: &Ty, filter: &Ty) { match &filter.kind { TyKind::Path(QPath::Resolved(_, path)) => { - if path_matches_symbol_path(ctx, path, bevy_paths::OR) { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::OR) { with_types.extend(check_or_filter(ctx, path)); } - if path_matches_symbol_path(ctx, path, bevy_paths::WITH) { - if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = bevy_helpers::get_def_id_of_first_generic_arg(path) { with_types.push((def_id, filter.span)); } } @@ -102,18 +98,18 @@ fn check_for_overlap<'hir>(ctx: &LateContext<'hir>, world: &Ty, filter: &Ty) { TyKind::Tup(types) => { for typ in *types { if let TyKind::Path(QPath::Resolved(_, path)) = typ.kind { - if path_matches_symbol_path(ctx, path, bevy_paths::OR) { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::OR) { with_types.extend(check_or_filter(ctx, path)); } - if path_matches_symbol_path(ctx, path, bevy_paths::ADDED) - || path_matches_symbol_path(ctx, path, bevy_paths::CHANGED) + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::ADDED) + || bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::CHANGED) { - if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + if let Some(def_id) = bevy_helpers::get_def_id_of_first_generic_arg(path) { required_types.push(def_id); } } - if path_matches_symbol_path(ctx, path, bevy_paths::WITH) { - if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = bevy_helpers::get_def_id_of_first_generic_arg(path) { with_types.push((def_id, typ.span)); } } @@ -135,51 +131,6 @@ fn check_for_overlap<'hir>(ctx: &LateContext<'hir>, world: &Ty, filter: &Ty) { } } -fn get_def_id_of_reference(reference: &MutTy) -> Option { - if let TyKind::Path(QPath::Resolved(_, path)) = reference.ty.kind { - if let Res::Def(_, def_id) = path.res { - return Some(def_id); - } - } - - None -} - -fn path_matches_symbol_path<'hir>( - ctx: &LateContext<'hir>, - path: &Path, - symbol_path: &[&str], -) -> bool { - if let Res::Def(_, def_id) = path.res { - return ctx.match_def_path( - def_id, - symbol_path - .iter() - .map(|str| Symbol::intern(str)) - .collect::>() - .as_slice(), - ); - }; - - false -} - -fn get_def_id_of_first_generic_arg(path: &Path) -> Option { - if let Some(segment) = path.segments.iter().last() { - if let Some(generic_args) = segment.args { - if let Some(GenericArg::Type(component)) = &generic_args.args.get(0) { - if let TyKind::Path(QPath::Resolved(_, path)) = component.kind { - if let Res::Def(_, def_id) = path.res { - return Some(def_id); - } - } - } - } - } - - None -} - fn check_or_filter<'hir>(ctx: &LateContext<'hir>, path: &Path) -> Vec<(DefId, Span)> { let mut local_required_types = Vec::new(); let mut local_with_types = Vec::new(); @@ -190,15 +141,23 @@ fn check_or_filter<'hir>(ctx: &LateContext<'hir>, path: &Path) -> Vec<(DefId, Sp if let TyKind::Tup(types) = tuple.kind { for typ in types { if let TyKind::Path(QPath::Resolved(_, path)) = typ.kind { - if path_matches_symbol_path(ctx, path, bevy_paths::ADDED) - || path_matches_symbol_path(ctx, path, bevy_paths::CHANGED) + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::ADDED) + || bevy_helpers::path_matches_symbol_path( + ctx, + path, + bevy_paths::CHANGED, + ) { - if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + if let Some(def_id) = + bevy_helpers::get_def_id_of_first_generic_arg(path) + { local_required_types.push(def_id); } } - if path_matches_symbol_path(ctx, path, bevy_paths::WITH) { - if let Some(def_id) = get_def_id_of_first_generic_arg(path) { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = + bevy_helpers::get_def_id_of_first_generic_arg(path) + { local_with_types.push((def_id, typ.span)); } } From 59d9aa370a0966943ddcc3c767a086d978f96dbc Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Tue, 3 Aug 2021 18:14:50 +0200 Subject: [PATCH 7/9] Check that test systems are actualy systems --- crates/bevy_lint/ui/unnecessary_with.rs | 100 ++++++++++++++------ crates/bevy_lint/ui/unnecessary_with.stderr | 94 +++++++++--------- 2 files changed, 119 insertions(+), 75 deletions(-) diff --git a/crates/bevy_lint/ui/unnecessary_with.rs b/crates/bevy_lint/ui/unnecessary_with.rs index e5b21c73ca60c..dc0a19b8dcd76 100644 --- a/crates/bevy_lint/ui/unnecessary_with.rs +++ b/crates/bevy_lint/ui/unnecessary_with.rs @@ -7,31 +7,75 @@ struct A; struct B; struct C; -fn test_query1(_query: Query<&A, With>) {} -fn test_query2(_query: Query<(&A, &B), With>) {} -fn test_query3(_query: Query<(&A, &B), With>) {} -fn test_query4(_query: Query<(&A, &B), (With, With)>) {} -fn test_query5(_query: Query<(&A, &B), (With, With)>) {} -fn test_query6(_query: Query<&A, (With, With)>) {} - -fn test_query7(_query: Query<&mut A, With>) {} -fn test_query8(_query: Query<(&mut A, &B), With>) {} -fn test_query9(_query: Query<(&mut A, &B), With>) {} -fn test_query10(_query: Query<(&mut A, &B), (With, With)>) {} -fn test_query11(_query: Query<(&mut A, &B), (With, With)>) {} -fn test_query12(_query: Query<&mut A, (With, With)>) {} - -fn test_query13(_query: Query<(), (Added, With)>) {} -fn test_query14(_query: Query<(), (Changed, With)>) {} - -fn test_query15(_query: Query<&A, Or<(With, With)>>) {} -fn test_query16(_query: Query<&B, Or<(With, With)>>) {} - -fn test_query17(_query: Query<&mut A, Or<(With, With)>>) {} -fn test_query18(_query: Query<&mut B, Or<(With, With)>>) {} - -fn test_query19(_query: Query<(), Or<(Added, With)>>) {} -fn test_query20(_query: Query<(), Or<(Changed, With)>>) {} - -fn test_query21(_query: Query<&A, (Or<(With, With)>, With)>) {} -fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) {} +fn test_query1(_query: Query<&A, With>) { + test_query1.system(); +} +fn test_query2(_query: Query<(&A, &B), With>) { + test_query2.system(); +} +fn test_query3(_query: Query<(&A, &B), With>) { + test_query3.system(); +} +fn test_query4(_query: Query<(&A, &B), (With, With)>) { + test_query4.system(); +} +fn test_query5(_query: Query<(&A, &B), (With, With)>) { + test_query5.system(); +} +fn test_query6(_query: Query<&A, (With, With)>) { + test_query6.system(); +} + +fn test_query7(_query: Query<&mut A, With>) { + test_query7.system(); +} +fn test_query8(_query: Query<(&mut A, &B), With>) { + test_query8.system(); +} +fn test_query9(_query: Query<(&mut A, &B), With>) { + test_query9.system(); +} +fn test_query10(_query: Query<(&mut A, &B), (With, With)>) { + test_query10.system(); +} +fn test_query11(_query: Query<(&mut A, &B), (With, With)>) { + test_query11.system(); +} +fn test_query12(_query: Query<&mut A, (With, With)>) { + test_query12.system(); +} + +fn test_query13(_query: Query<(), (Added, With)>) { + test_query13.system(); +} +fn test_query14(_query: Query<(), (Changed, With)>) { + test_query14.system(); +} + +fn test_query15(_query: Query<&A, Or<(With, With)>>) { + test_query15.system(); +} +fn test_query16(_query: Query<&B, Or<(With, With)>>) { + test_query16.system(); +} + +fn test_query17(_query: Query<&mut A, Or<(With, With)>>) { + test_query17.system(); +} +fn test_query18(_query: Query<&mut B, Or<(With, With)>>) { + test_query18.system(); +} + +fn test_query19(_query: Query<(), Or<(Added, With)>>) { + test_query19.system(); +} +fn test_query20(_query: Query<(), Or<(Changed, With)>>) { + test_query20.system(); +} + +fn test_query21(_query: Query<&A, (Or<(With, With)>, With)>) { + test_query21.system(); +} +fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) { + test_query22.system(); +} diff --git a/crates/bevy_lint/ui/unnecessary_with.stderr b/crates/bevy_lint/ui/unnecessary_with.stderr index 486da2e62d78a..c68678c1d83a0 100644 --- a/crates/bevy_lint/ui/unnecessary_with.stderr +++ b/crates/bevy_lint/ui/unnecessary_with.stderr @@ -1,147 +1,147 @@ error: Unnecessary `With` Filter --> $DIR/unnecessary_with.rs:10:34 | -LL | fn test_query1(_query: Query<&A, With>) {} +LL | fn test_query1(_query: Query<&A, With>) { | ^^^^^^^ | = note: `-D unnecessary-with` implied by `-D warnings` error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:11:40 + --> $DIR/unnecessary_with.rs:13:40 | -LL | fn test_query2(_query: Query<(&A, &B), With>) {} +LL | fn test_query2(_query: Query<(&A, &B), With>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:12:40 + --> $DIR/unnecessary_with.rs:16:40 | -LL | fn test_query3(_query: Query<(&A, &B), With>) {} +LL | fn test_query3(_query: Query<(&A, &B), With>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:13:41 + --> $DIR/unnecessary_with.rs:19:41 | -LL | fn test_query4(_query: Query<(&A, &B), (With, With)>) {} +LL | fn test_query4(_query: Query<(&A, &B), (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:13:50 + --> $DIR/unnecessary_with.rs:19:50 | -LL | fn test_query4(_query: Query<(&A, &B), (With, With)>) {} +LL | fn test_query4(_query: Query<(&A, &B), (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:14:41 + --> $DIR/unnecessary_with.rs:22:41 | -LL | fn test_query5(_query: Query<(&A, &B), (With, With)>) {} +LL | fn test_query5(_query: Query<(&A, &B), (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:15:35 + --> $DIR/unnecessary_with.rs:25:35 | -LL | fn test_query6(_query: Query<&A, (With, With)>) {} +LL | fn test_query6(_query: Query<&A, (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:17:38 + --> $DIR/unnecessary_with.rs:29:38 | -LL | fn test_query7(_query: Query<&mut A, With>) {} +LL | fn test_query7(_query: Query<&mut A, With>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:18:44 + --> $DIR/unnecessary_with.rs:32:44 | -LL | fn test_query8(_query: Query<(&mut A, &B), With>) {} +LL | fn test_query8(_query: Query<(&mut A, &B), With>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:19:44 + --> $DIR/unnecessary_with.rs:35:44 | -LL | fn test_query9(_query: Query<(&mut A, &B), With>) {} +LL | fn test_query9(_query: Query<(&mut A, &B), With>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:20:46 + --> $DIR/unnecessary_with.rs:38:46 | -LL | fn test_query10(_query: Query<(&mut A, &B), (With, With)>) {} +LL | fn test_query10(_query: Query<(&mut A, &B), (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:20:55 + --> $DIR/unnecessary_with.rs:38:55 | -LL | fn test_query10(_query: Query<(&mut A, &B), (With, With)>) {} +LL | fn test_query10(_query: Query<(&mut A, &B), (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:21:46 + --> $DIR/unnecessary_with.rs:41:46 | -LL | fn test_query11(_query: Query<(&mut A, &B), (With, With)>) {} +LL | fn test_query11(_query: Query<(&mut A, &B), (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:22:40 + --> $DIR/unnecessary_with.rs:44:40 | -LL | fn test_query12(_query: Query<&mut A, (With, With)>) {} +LL | fn test_query12(_query: Query<&mut A, (With, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:24:46 + --> $DIR/unnecessary_with.rs:48:46 | -LL | fn test_query13(_query: Query<(), (Added, With)>) {} +LL | fn test_query13(_query: Query<(), (Added, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:25:48 + --> $DIR/unnecessary_with.rs:51:48 | -LL | fn test_query14(_query: Query<(), (Changed, With)>) {} +LL | fn test_query14(_query: Query<(), (Changed, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:27:39 + --> $DIR/unnecessary_with.rs:55:39 | -LL | fn test_query15(_query: Query<&A, Or<(With, With)>>) {} +LL | fn test_query15(_query: Query<&A, Or<(With, With)>>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:28:48 + --> $DIR/unnecessary_with.rs:58:48 | -LL | fn test_query16(_query: Query<&B, Or<(With, With)>>) {} +LL | fn test_query16(_query: Query<&B, Or<(With, With)>>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:30:43 + --> $DIR/unnecessary_with.rs:62:43 | -LL | fn test_query17(_query: Query<&mut A, Or<(With, With)>>) {} +LL | fn test_query17(_query: Query<&mut A, Or<(With, With)>>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:31:52 + --> $DIR/unnecessary_with.rs:65:52 | -LL | fn test_query18(_query: Query<&mut B, Or<(With, With)>>) {} +LL | fn test_query18(_query: Query<&mut B, Or<(With, With)>>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:33:49 + --> $DIR/unnecessary_with.rs:69:49 | -LL | fn test_query19(_query: Query<(), Or<(Added, With)>>) {} +LL | fn test_query19(_query: Query<(), Or<(Added, With)>>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:34:51 + --> $DIR/unnecessary_with.rs:72:51 | -LL | fn test_query20(_query: Query<(), Or<(Changed, With)>>) {} +LL | fn test_query20(_query: Query<(), Or<(Changed, With)>>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:36:40 + --> $DIR/unnecessary_with.rs:76:40 | -LL | fn test_query21(_query: Query<&A, (Or<(With, With)>, With)>) {} +LL | fn test_query21(_query: Query<&A, (Or<(With, With)>, With)>) { | ^^^^^^^ error: Unnecessary `With` Filter - --> $DIR/unnecessary_with.rs:37:44 + --> $DIR/unnecessary_with.rs:79:44 | -LL | fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) {} +LL | fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) { | ^^^^^^^ error: aborting due to 24 previous errors From ba04cc2a0163f88e803f71c27365d6e3f7ab459e Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Tue, 3 Aug 2021 18:25:27 +0200 Subject: [PATCH 8/9] Refactor Further --- crates/bevy_lint/src/unnecessary_with.rs | 115 ++++++++++++----------- 1 file changed, 61 insertions(+), 54 deletions(-) diff --git a/crates/bevy_lint/src/unnecessary_with.rs b/crates/bevy_lint/src/unnecessary_with.rs index 3253550aaf2cc..11ebb29c2801f 100644 --- a/crates/bevy_lint/src/unnecessary_with.rs +++ b/crates/bevy_lint/src/unnecessary_with.rs @@ -51,82 +51,89 @@ impl<'hir> LateLintPass<'hir> for UnnecessaryWith { for typ in decl.inputs { if let TyKind::Path(QPath::Resolved(_, path)) = &typ.kind { if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::QUERY) { - if let Some((world, Some(filter))) = - bevy_helpers::get_generics_of_query(ctx, &typ) - { - check_for_overlap(ctx, world, filter); - } + check_for_unnecesarry_with(ctx, &typ); } } } } } -fn check_for_overlap<'hir>(ctx: &LateContext<'hir>, world: &Ty, filter: &Ty) { - let mut required_types = Vec::new(); - let mut with_types = Vec::new(); +fn check_for_unnecesarry_with<'hir>(ctx: &LateContext<'hir>, query: &'hir Ty) { + if let Some((world, Some(filter))) = bevy_helpers::get_generics_of_query(ctx, query) { + let mut required_types = Vec::new(); + let mut with_types = Vec::new(); - match &world.kind { - TyKind::Rptr(_, mut_type) => { - if let Some(def_id) = bevy_helpers::get_def_id_of_referenced_type(&mut_type) { - required_types.push(def_id); + match &world.kind { + TyKind::Rptr(_, mut_type) => { + if let Some(def_id) = bevy_helpers::get_def_id_of_referenced_type(&mut_type) { + required_types.push(def_id); + } } - } - TyKind::Tup(types) => { - for typ in *types { - if let TyKind::Rptr(_, mut_type) = &typ.kind { - if let Some(def_id) = bevy_helpers::get_def_id_of_referenced_type(&mut_type) { - required_types.push(def_id); + TyKind::Tup(types) => { + for typ in *types { + if let TyKind::Rptr(_, mut_type) = &typ.kind { + if let Some(def_id) = bevy_helpers::get_def_id_of_referenced_type(&mut_type) + { + required_types.push(def_id); + } } } } + _ => (), } - _ => (), - } - match &filter.kind { - TyKind::Path(QPath::Resolved(_, path)) => { - if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::OR) { - with_types.extend(check_or_filter(ctx, path)); - } - if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::WITH) { - if let Some(def_id) = bevy_helpers::get_def_id_of_first_generic_arg(path) { - with_types.push((def_id, filter.span)); + match &filter.kind { + TyKind::Path(QPath::Resolved(_, path)) => { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::OR) { + with_types.extend(check_or_filter(ctx, path)); } - } - } - TyKind::Tup(types) => { - for typ in *types { - if let TyKind::Path(QPath::Resolved(_, path)) = typ.kind { - if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::OR) { - with_types.extend(check_or_filter(ctx, path)); + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = bevy_helpers::get_def_id_of_first_generic_arg(path) { + with_types.push((def_id, filter.span)); } - if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::ADDED) - || bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::CHANGED) - { - if let Some(def_id) = bevy_helpers::get_def_id_of_first_generic_arg(path) { - required_types.push(def_id); + } + } + TyKind::Tup(types) => { + for typ in *types { + if let TyKind::Path(QPath::Resolved(_, path)) = typ.kind { + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::OR) { + with_types.extend(check_or_filter(ctx, path)); } - } - if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::WITH) { - if let Some(def_id) = bevy_helpers::get_def_id_of_first_generic_arg(path) { - with_types.push((def_id, typ.span)); + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::ADDED) + || bevy_helpers::path_matches_symbol_path( + ctx, + path, + bevy_paths::CHANGED, + ) + { + if let Some(def_id) = + bevy_helpers::get_def_id_of_first_generic_arg(path) + { + required_types.push(def_id); + } + } + if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::WITH) { + if let Some(def_id) = + bevy_helpers::get_def_id_of_first_generic_arg(path) + { + with_types.push((def_id, typ.span)); + } } } } } + _ => (), } - _ => (), - } - for with_type in with_types { - if required_types.contains(&with_type.0) { - span_lint( - ctx, - UNNECESSARY_WITH, - with_type.1, - "Unnecessary `With` Filter", - ); + for with_type in with_types { + if required_types.contains(&with_type.0) { + span_lint( + ctx, + UNNECESSARY_WITH, + with_type.1, + "Unnecessary `With` Filter", + ); + } } } } From edcbf3ae5f84a4a8d69752ad21a43a0bd5df455e Mon Sep 17 00:00:00 2001 From: MinerSebas Date: Tue, 3 Aug 2021 18:38:54 +0200 Subject: [PATCH 9/9] Also check Querys that are nested in Tuples --- crates/bevy_lint/src/unnecessary_with.rs | 30 +++++++++++++++++---- crates/bevy_lint/ui/unnecessary_with.rs | 22 +++++++++++++++ crates/bevy_lint/ui/unnecessary_with.stderr | 26 +++++++++++++++++- 3 files changed, 72 insertions(+), 6 deletions(-) diff --git a/crates/bevy_lint/src/unnecessary_with.rs b/crates/bevy_lint/src/unnecessary_with.rs index 11ebb29c2801f..15ca4ed48d189 100644 --- a/crates/bevy_lint/src/unnecessary_with.rs +++ b/crates/bevy_lint/src/unnecessary_with.rs @@ -49,16 +49,36 @@ impl<'hir> LateLintPass<'hir> for UnnecessaryWith { _: HirId, ) { for typ in decl.inputs { - if let TyKind::Path(QPath::Resolved(_, path)) = &typ.kind { - if bevy_helpers::path_matches_symbol_path(ctx, path, bevy_paths::QUERY) { - check_for_unnecesarry_with(ctx, &typ); - } + recursively_search_type(ctx, typ, bevy_paths::QUERY, &check_for_unnecesarry_with); + } + } +} + +fn recursively_search_type<'hir, T: Fn(&LateContext<'hir>, &'hir Ty<'hir>) -> ()>( + ctx: &LateContext<'hir>, + typ: &'hir Ty, + symbol_path: &[&str], + function: &T, +) { + match &typ.kind { + TyKind::Path(QPath::Resolved(_, path)) => { + if bevy_helpers::path_matches_symbol_path(ctx, path, symbol_path) { + (function)(ctx, &typ) + } + } + TyKind::Tup(types) => { + for tup_typ in *types { + // Todo: Filter out Types that dont implement SystemParam + // -> Is it possible to go from rustc_hir::Ty to rustc_middle::Ty? + // Required for using clippy_utils::ty::implements_trait. + recursively_search_type(ctx, tup_typ, symbol_path, function); } } + _ => (), } } -fn check_for_unnecesarry_with<'hir>(ctx: &LateContext<'hir>, query: &'hir Ty) { +fn check_for_unnecesarry_with<'hir>(ctx: &LateContext<'hir>, query: &'hir Ty<'hir>) { if let Some((world, Some(filter))) = bevy_helpers::get_generics_of_query(ctx, query) { let mut required_types = Vec::new(); let mut with_types = Vec::new(); diff --git a/crates/bevy_lint/ui/unnecessary_with.rs b/crates/bevy_lint/ui/unnecessary_with.rs index dc0a19b8dcd76..da914378b34b3 100644 --- a/crates/bevy_lint/ui/unnecessary_with.rs +++ b/crates/bevy_lint/ui/unnecessary_with.rs @@ -79,3 +79,25 @@ fn test_query21(_query: Query<&A, (Or<(With, With)>, With)>) { fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) { test_query22.system(); } + +fn test_query23(_query: (Query<&A, With>,)) { + test_query23.system(); +} +fn test_query24(_query: (((((((((Query<&A, With>,),),),),),),),),)) { + test_query24.system(); +} +fn test_query25( + _query: ( + (), + ((( + (), + ( + ((((Query<&A, With>,), ()),),), + (), + (Query<&A, With>, ()), + ), + ),),), + ), +) { + test_query25.system(); +} diff --git a/crates/bevy_lint/ui/unnecessary_with.stderr b/crates/bevy_lint/ui/unnecessary_with.stderr index c68678c1d83a0..f8aef83d74f37 100644 --- a/crates/bevy_lint/ui/unnecessary_with.stderr +++ b/crates/bevy_lint/ui/unnecessary_with.stderr @@ -144,5 +144,29 @@ error: Unnecessary `With` Filter LL | fn test_query22(_query: Query<&mut A, (Or<(With, With)>, With)>) { | ^^^^^^^ -error: aborting due to 24 previous errors +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:83:36 + | +LL | fn test_query23(_query: (Query<&A, With>,)) { + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:86:44 + | +LL | fn test_query24(_query: (((((((((Query<&A, With>,),),),),),),),),)) { + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:95:31 + | +LL | ((((Query<&A, With>,), ()),),), + | ^^^^^^^ + +error: Unnecessary `With` Filter + --> $DIR/unnecessary_with.rs:97:28 + | +LL | (Query<&A, With>, ()), + | ^^^^^^^ + +error: aborting due to 28 previous errors