From bf666af3a2c44f4e131d71bf75cc99c2fa9f1c30 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Sat, 2 Mar 2024 15:51:01 -0500 Subject: [PATCH] Deny all Clippy warnings (#8720) This PR makes Clippy deny all warnings across the workspace. We now enumerate all of the rules that have violations and temporarily allow them, with the goal being to drive the list down over time. On Windows we don't yet use `--deny warnings`, as the Windows build still has some warnings. Release Notes: - N/A --- .github/workflows/ci.yml | 9 +- .github/workflows/deploy_collab.yml | 3 +- .github/workflows/release_nightly.yml | 3 +- crates/gpui/build.rs | 2 +- tooling/xtask/src/main.rs | 122 +++++++++++++++++++++++--- 5 files changed, 116 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f3ed4a18d..fac8924863 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -93,8 +93,7 @@ jobs: fi - name: cargo clippy - shell: bash -euxo pipefail {0} - run: script/clippy + run: cargo xtask clippy - name: Run tests uses: ./.github/actions/run_tests @@ -126,8 +125,7 @@ jobs: run: script/linux - name: cargo clippy - shell: bash -euxo pipefail {0} - run: script/clippy + run: cargo xtask clippy - name: Build Zed run: cargo build -p zed @@ -149,8 +147,7 @@ jobs: save-if: ${{ github.ref == 'refs/heads/main' }} - name: cargo clippy - shell: bash -euxo pipefail {0} - run: script/clippy + run: cargo xtask clippy - name: Build Zed run: cargo build -p zed diff --git a/.github/workflows/deploy_collab.yml b/.github/workflows/deploy_collab.yml index b866d9080c..220c4ca6f7 100644 --- a/.github/workflows/deploy_collab.yml +++ b/.github/workflows/deploy_collab.yml @@ -28,8 +28,7 @@ jobs: uses: ./.github/actions/check_style - name: Run clippy - shell: bash -euxo pipefail {0} - run: script/clippy + run: cargo xtask clippy tests: name: Run tests diff --git a/.github/workflows/release_nightly.yml b/.github/workflows/release_nightly.yml index 1a7a0f704d..6e34e859b5 100644 --- a/.github/workflows/release_nightly.yml +++ b/.github/workflows/release_nightly.yml @@ -32,8 +32,7 @@ jobs: uses: ./.github/actions/check_style - name: Run clippy - shell: bash -euxo pipefail {0} - run: script/clippy + run: cargo xtask clippy tests: name: Run tests if: github.repository_owner == 'zed-industries' diff --git a/crates/gpui/build.rs b/crates/gpui/build.rs index 6d6f384bc5..8f63787c8b 100644 --- a/crates/gpui/build.rs +++ b/crates/gpui/build.rs @@ -125,7 +125,7 @@ fn emit_stitched_shaders(header_path: &Path) { let shader_contents = std::fs::read_to_string(shader_path)?; let stitched_contents = format!("{header_contents}\n{shader_contents}"); let out_path = PathBuf::from(env::var("OUT_DIR").unwrap()).join("stitched_shaders.metal"); - let _ = std::fs::write(&out_path, stitched_contents)?; + std::fs::write(&out_path, stitched_contents)?; Ok(out_path) } let shader_source_path = "./src/platform/mac/shaders.metal"; diff --git a/tooling/xtask/src/main.rs b/tooling/xtask/src/main.rs index 1fed1574b3..6126978e57 100644 --- a/tooling/xtask/src/main.rs +++ b/tooling/xtask/src/main.rs @@ -26,31 +26,129 @@ fn main() -> Result<()> { #[derive(Parser)] struct ClippyArgs { - /// Whether to deny warnings (`clippy --deny warnings`). + /// Automatically apply lint suggestions (`clippy --fix`). #[arg(long)] - deny_warnings: bool, + fix: bool, + + /// The package to run Clippy against (`cargo -p clippy`). + #[arg(long, short)] + package: Option, } fn run_clippy(args: ClippyArgs) -> Result<()> { let cargo = std::env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()); let mut clippy_command = Command::new(&cargo); + clippy_command.arg("clippy"); + + if let Some(package) = args.package { + clippy_command.args(["--package", &package]); + } else { + clippy_command.arg("--workspace"); + } + clippy_command - .arg("clippy") - .arg("--workspace") .arg("--release") .arg("--all-targets") .arg("--all-features"); - clippy_command.arg("--"); - - if args.deny_warnings { - clippy_command.args(["--deny", "warnings"]); + if args.fix { + clippy_command.arg("--fix"); } - // Allow all Clippy lints by default, as we have a lot of violations at the moment. - // We can tighten things up once we have a better handle on them. - clippy_command.args(["--allow", "clippy::all"]); + clippy_command.arg("--"); + + // Deny all warnings. + // We don't do this yet on Windows, as it still has some warnings present. + #[cfg(not(target_os = "windows"))] + clippy_command.args(["--deny", "warnings"]); + + /// These are all of the rules that currently have violations in the Zed + /// codebase. + /// + /// We'll want to drive this list down by either: + /// 1. fixing violations of the rule and begin enforcing it + /// 2. deciding we want to allow the rule permanently, at which point + /// we should codify that separately in this script. + const MIGRATORY_RULES_TO_ALLOW: &[&str] = &[ + // There's a bunch of rules currently failing in the `style` group, so + // allow all of those, for now. + "clippy::style", + // Individual rules that have violations in the codebase: + "clippy::almost_complete_range", + "clippy::arc_with_non_send_sync", + "clippy::await_holding_lock", + "clippy::bind_instead_of_map", + "clippy::bool_comparison", + "clippy::borrow_deref_ref", + "clippy::borrowed_box", + "clippy::cast_abs_to_unsigned", + "clippy::clone_on_copy", + "clippy::cmp_owned", + "clippy::crate_in_macro_def", + "clippy::default_constructed_unit_structs", + "clippy::derivable_impls", + "clippy::derive_ord_xor_partial_ord", + "clippy::drain_collect", + "clippy::eq_op", + "clippy::expect_fun_call", + "clippy::explicit_auto_deref", + "clippy::explicit_counter_loop", + "clippy::extra_unused_lifetimes", + "clippy::filter_map_identity", + "clippy::identity_op", + "clippy::implied_bounds_in_impls", + "clippy::iter_kv_map", + "clippy::iter_overeager_cloned", + "clippy::let_underscore_future", + "clippy::manual_find", + "clippy::manual_flatten", + "clippy::map_entry", + "clippy::map_flatten", + "clippy::map_identity", + "clippy::needless_arbitrary_self_type", + "clippy::needless_borrowed_reference", + "clippy::needless_lifetimes", + "clippy::needless_option_as_deref", + "clippy::needless_question_mark", + "clippy::needless_update", + "clippy::never_loop", + "clippy::non_canonical_clone_impl", + "clippy::non_canonical_partial_ord_impl", + "clippy::nonminimal_bool", + "clippy::option_as_ref_deref", + "clippy::option_map_unit_fn", + "clippy::redundant_closure_call", + "clippy::redundant_guards", + "clippy::redundant_locals", + "clippy::reversed_empty_ranges", + "clippy::search_is_some", + "clippy::single_char_pattern", + "clippy::single_range_in_vec_init", + "clippy::suspicious_to_owned", + "clippy::to_string_in_format_args", + "clippy::too_many_arguments", + "clippy::type_complexity", + "clippy::unit_arg", + "clippy::unnecessary_cast", + "clippy::unnecessary_filter_map", + "clippy::unnecessary_find_map", + "clippy::unnecessary_operation", + "clippy::unnecessary_to_owned", + "clippy::unnecessary_unwrap", + "clippy::useless_conversion", + "clippy::useless_format", + "clippy::vec_init_then_push", + ]; + + // When fixing violations automatically we don't care about the + // rules we're already violating, since it may be possible to + // have them fixed automatically. + if !args.fix { + for rule in MIGRATORY_RULES_TO_ALLOW { + clippy_command.args(["--allow", rule]); + } + } // Deny `dbg!` and `todo!`s. clippy_command @@ -61,7 +159,7 @@ fn run_clippy(args: ClippyArgs) -> Result<()> { "running: {cargo} {}", clippy_command .get_args() - .map(|arg| format!("{}", arg.to_str().unwrap())) + .map(|arg| arg.to_str().unwrap()) .collect::>() .join(" ") );