From 6729186908bc8f962caede8f7bf548ebccd93f43 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 12 Jun 2024 12:43:49 +0200 Subject: [PATCH 1/4] store the lint levels in the clippy structs themselves --- src/bootstrap/src/core/build_steps/clippy.rs | 60 +++++++++++++------- 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index 6fb37e8cfc4a4..de96648d59d68 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -20,14 +20,12 @@ const IGNORED_RULES_FOR_STD_AND_RUSTC: &[&str] = &[ "wrong_self_convention", ]; -fn lint_args(builder: &Builder<'_>, ignored_rules: &[&str]) -> Vec { +fn lint_args(builder: &Builder<'_>, config: &LintConfig, ignored_rules: &[&str]) -> Vec { fn strings<'a>(arr: &'a [&str]) -> impl Iterator + 'a { arr.iter().copied().map(String::from) } - let Subcommand::Clippy { fix, allow_dirty, allow_staged, allow, deny, warn, forbid } = - &builder.config.cmd - else { + let Subcommand::Clippy { fix, allow_dirty, allow_staged, .. } = &builder.config.cmd else { unreachable!("clippy::lint_args can only be called from `clippy` subcommands."); }; @@ -53,12 +51,12 @@ fn lint_args(builder: &Builder<'_>, ignored_rules: &[&str]) -> Vec { args.extend(strings(&["--"])); - if deny.is_empty() && forbid.is_empty() { + if config.deny.is_empty() && config.forbid.is_empty() { args.extend(strings(&["--cap-lints", "warn"])); } let all_args = std::env::args().collect::>(); - args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid)); + args.extend(get_clippy_rules_in_order(&all_args, config)); args.extend(ignored_rules.iter().map(|lint| format!("-Aclippy::{}", lint))); args.extend(builder.config.free_args.clone()); @@ -68,21 +66,17 @@ fn lint_args(builder: &Builder<'_>, ignored_rules: &[&str]) -> Vec { /// We need to keep the order of the given clippy lint rules before passing them. /// Since clap doesn't offer any useful interface for this purpose out of the box, /// we have to handle it manually. -pub(crate) fn get_clippy_rules_in_order( - all_args: &[String], - allow_rules: &[String], - deny_rules: &[String], - warn_rules: &[String], - forbid_rules: &[String], -) -> Vec { +fn get_clippy_rules_in_order(all_args: &[String], config: &LintConfig) -> Vec { let mut result = vec![]; for (prefix, item) in - [("-A", allow_rules), ("-D", deny_rules), ("-W", warn_rules), ("-F", forbid_rules)] + [("-A", &config.allow), ("-D", &config.deny), ("-W", &config.warn), ("-F", &config.forbid)] { item.iter().for_each(|v| { let rule = format!("{prefix}{v}"); - let position = all_args.iter().position(|t| t == &rule || t == v).unwrap(); + // Arguments added by bootstrap in LintConfig won't show up in the all_args list, so + // put them at the end of the command line. + let position = all_args.iter().position(|t| t == &rule || t == v).unwrap_or(usize::MAX); result.push((position, rule)); }); } @@ -91,9 +85,29 @@ pub(crate) fn get_clippy_rules_in_order( result.into_iter().map(|v| v.1).collect() } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct LintConfig { + allow: Vec, + warn: Vec, + deny: Vec, + forbid: Vec, +} + +impl LintConfig { + fn new(builder: &Builder<'_>) -> Self { + match builder.config.cmd.clone() { + Subcommand::Clippy { allow, deny, warn, forbid, .. } => { + Self { allow, warn, deny, forbid } + } + _ => unreachable!("LintConfig can only be called from `clippy` subcommands."), + } + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Std { pub target: TargetSelection, + config: LintConfig, /// Whether to lint only a subset of crates. crates: Vec, } @@ -108,7 +122,8 @@ impl Step for Std { fn make_run(run: RunConfig<'_>) { let crates = std_crates_for_run_make(&run); - run.builder.ensure(Std { target: run.target, crates }); + let config = LintConfig::new(run.builder); + run.builder.ensure(Std { target: run.target, config, crates }); } fn run(self, builder: &Builder<'_>) { @@ -138,7 +153,7 @@ impl Step for Std { run_cargo( builder, cargo, - lint_args(builder, IGNORED_RULES_FOR_STD_AND_RUSTC), + lint_args(builder, &self.config, IGNORED_RULES_FOR_STD_AND_RUSTC), &libstd_stamp(builder, compiler, target), vec![], true, @@ -150,6 +165,7 @@ impl Step for Std { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Rustc { pub target: TargetSelection, + config: LintConfig, /// Whether to lint only a subset of crates. crates: Vec, } @@ -165,7 +181,8 @@ impl Step for Rustc { fn make_run(run: RunConfig<'_>) { let crates = run.make_run_crates(Alias::Compiler); - run.builder.ensure(Rustc { target: run.target, crates }); + let config = LintConfig::new(run.builder); + run.builder.ensure(Rustc { target: run.target, config, crates }); } /// Lints the compiler. @@ -212,7 +229,7 @@ impl Step for Rustc { run_cargo( builder, cargo, - lint_args(builder, IGNORED_RULES_FOR_STD_AND_RUSTC), + lint_args(builder, &self.config, IGNORED_RULES_FOR_STD_AND_RUSTC), &librustc_stamp(builder, compiler, target), vec![], true, @@ -232,6 +249,7 @@ macro_rules! lint_any { #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct $name { pub target: TargetSelection, + config: LintConfig, } impl Step for $name { @@ -243,8 +261,10 @@ macro_rules! lint_any { } fn make_run(run: RunConfig<'_>) { + let config = LintConfig::new(run.builder); run.builder.ensure($name { target: run.target, + config, }); } @@ -281,7 +301,7 @@ macro_rules! lint_any { run_cargo( builder, cargo, - lint_args(builder, &[]), + lint_args(builder, &self.config, &[]), &stamp, vec![], true, From 3c6841725c05c3e75a8321b114b2132eb1699fb8 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 12 Jun 2024 13:01:21 +0200 Subject: [PATCH 2/4] define all the clippy lints we check in CI in a step --- src/bootstrap/src/core/build_steps/clippy.rs | 68 +++++++++++++++++++ src/bootstrap/src/core/builder/mod.rs | 1 + .../docker/host-x86_64/mingw-check/Dockerfile | 4 +- 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index de96648d59d68..ad2b349bab670 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -102,6 +102,19 @@ impl LintConfig { _ => unreachable!("LintConfig can only be called from `clippy` subcommands."), } } + + fn merge(&self, other: &Self) -> Self { + let merged = |self_attr: &[String], other_attr: &[String]| -> Vec { + self_attr.iter().cloned().chain(other_attr.iter().cloned()).collect() + }; + // This is written this way to ensure we get a compiler error if we add a new field. + Self { + allow: merged(&self.allow, &other.allow), + warn: merged(&self.warn, &other.warn), + deny: merged(&self.deny, &other.deny), + forbid: merged(&self.forbid, &other.forbid), + } + } } #[derive(Debug, Clone, PartialEq, Eq, Hash)] @@ -339,3 +352,58 @@ lint_any!( Tidy, "src/tools/tidy", "tidy"; TestFloatParse, "src/etc/test-float-parse", "test-float-parse"; ); + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct CI { + target: TargetSelection, + config: LintConfig, +} + +impl Step for CI { + type Output = (); + const DEFAULT: bool = false; + + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + run.alias("ci") + } + + fn make_run(run: RunConfig<'_>) { + let config = LintConfig::new(run.builder); + run.builder.ensure(CI { target: run.target, config }); + } + + fn run(self, builder: &Builder<'_>) -> Self::Output { + builder.ensure(Bootstrap { + target: self.target, + config: self.config.merge(&LintConfig { + allow: vec![], + warn: vec![], + deny: vec!["warnings".into()], + forbid: vec![], + }), + }); + let library_clippy_cfg = LintConfig { + allow: vec!["clippy::all".into()], + warn: vec![], + deny: vec!["clippy::correctness".into()], + forbid: vec![], + }; + let compiler_clippy_cfg = LintConfig { + allow: vec!["clippy::all".into()], + warn: vec![], + deny: vec!["clippy::correctness".into(), "clippy::clone_on_ref_ptr".into()], + forbid: vec![], + }; + + builder.ensure(Std { + target: self.target, + config: self.config.merge(&library_clippy_cfg), + crates: vec![], + }); + builder.ensure(Rustc { + target: self.target, + config: self.config.merge(&compiler_clippy_cfg), + crates: vec![], + }); + } +} diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index f1b3cf6da13ec..d59e0fa728807 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -839,6 +839,7 @@ impl<'a> Builder<'a> { clippy::RustInstaller, clippy::TestFloatParse, clippy::Tidy, + clippy::CI, ), Kind::Check | Kind::Fix => describe!( check::Std, diff --git a/src/ci/docker/host-x86_64/mingw-check/Dockerfile b/src/ci/docker/host-x86_64/mingw-check/Dockerfile index fdc8a7310c8fe..f0afb570cc4a0 100644 --- a/src/ci/docker/host-x86_64/mingw-check/Dockerfile +++ b/src/ci/docker/host-x86_64/mingw-check/Dockerfile @@ -50,9 +50,7 @@ ENV SCRIPT \ python3 ../x.py check --stage 0 --set build.optimized-compiler-builtins=false core alloc std --target=aarch64-unknown-linux-gnu,i686-pc-windows-msvc,i686-unknown-linux-gnu,x86_64-apple-darwin,x86_64-pc-windows-gnu,x86_64-pc-windows-msvc && \ /scripts/check-default-config-profiles.sh && \ python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu && \ - python3 ../x.py clippy bootstrap -Dwarnings && \ - python3 ../x.py clippy library -Aclippy::all -Dclippy::correctness && \ - python3 ../x.py clippy compiler -Aclippy::all -Dclippy::correctness -Dclippy::clone_on_ref_ptr && \ + python3 ../x.py clippy ci && \ python3 ../x.py build --stage 0 src/tools/build-manifest && \ python3 ../x.py test --stage 0 src/tools/compiletest && \ python3 ../x.py test --stage 0 core alloc std test proc_macro && \ From dad667b6ac5a4d0ed806d53932f5491df418bfaa Mon Sep 17 00:00:00 2001 From: klensy Date: Tue, 5 Nov 2024 21:19:28 +0300 Subject: [PATCH 3/4] fix tests --- src/bootstrap/src/core/build_steps/clippy.rs | 12 ++++++------ src/bootstrap/src/core/config/tests.rs | 12 +++++++----- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clippy.rs b/src/bootstrap/src/core/build_steps/clippy.rs index ad2b349bab670..0884d86cc6d5f 100644 --- a/src/bootstrap/src/core/build_steps/clippy.rs +++ b/src/bootstrap/src/core/build_steps/clippy.rs @@ -66,7 +66,7 @@ fn lint_args(builder: &Builder<'_>, config: &LintConfig, ignored_rules: &[&str]) /// We need to keep the order of the given clippy lint rules before passing them. /// Since clap doesn't offer any useful interface for this purpose out of the box, /// we have to handle it manually. -fn get_clippy_rules_in_order(all_args: &[String], config: &LintConfig) -> Vec { +pub fn get_clippy_rules_in_order(all_args: &[String], config: &LintConfig) -> Vec { let mut result = vec![]; for (prefix, item) in @@ -86,11 +86,11 @@ fn get_clippy_rules_in_order(all_args: &[String], config: &LintConfig) -> Vec, - warn: Vec, - deny: Vec, - forbid: Vec, +pub struct LintConfig { + pub allow: Vec, + pub warn: Vec, + pub deny: Vec, + pub forbid: Vec, } impl LintConfig { diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index f911581b7eece..24f932a172432 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -9,7 +9,7 @@ use serde::Deserialize; use super::flags::Flags; use super::{ChangeIdWrapper, Config, RUSTC_IF_UNCHANGED_ALLOWED_PATHS}; -use crate::core::build_steps::clippy::get_clippy_rules_in_order; +use crate::core::build_steps::clippy::{LintConfig, get_clippy_rules_in_order}; use crate::core::build_steps::llvm; use crate::core::config::{LldMode, Target, TargetSelection, TomlConfig}; @@ -309,9 +309,10 @@ fn order_of_clippy_rules() { ]; let config = Config::parse(Flags::parse(&args)); - let actual = match &config.cmd { + let actual = match config.cmd.clone() { crate::Subcommand::Clippy { allow, deny, warn, forbid, .. } => { - get_clippy_rules_in_order(&args, &allow, &deny, &warn, &forbid) + let cfg = LintConfig { allow, deny, warn, forbid }; + get_clippy_rules_in_order(&args, &cfg) } _ => panic!("invalid subcommand"), }; @@ -332,9 +333,10 @@ fn clippy_rule_separate_prefix() { vec!["clippy".to_string(), "-A clippy:all".to_string(), "-W clippy::style".to_string()]; let config = Config::parse(Flags::parse(&args)); - let actual = match &config.cmd { + let actual = match config.cmd.clone() { crate::Subcommand::Clippy { allow, deny, warn, forbid, .. } => { - get_clippy_rules_in_order(&args, &allow, &deny, &warn, &forbid) + let cfg = LintConfig { allow, deny, warn, forbid }; + get_clippy_rules_in_order(&args, &cfg) } _ => panic!("invalid subcommand"), }; From cdd948cbc05f00bcf072808c8659b5fc6e0ab196 Mon Sep 17 00:00:00 2001 From: klensy Date: Tue, 12 Nov 2024 21:43:51 +0300 Subject: [PATCH 4/4] fix clippy warns on windows (not checked by CI) --- src/bootstrap/src/core/build_steps/clean.rs | 2 ++ src/bootstrap/src/utils/helpers.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/clean.rs b/src/bootstrap/src/core/build_steps/clean.rs index 040690623a108..d857de96cce73 100644 --- a/src/bootstrap/src/core/build_steps/clean.rs +++ b/src/bootstrap/src/core/build_steps/clean.rs @@ -226,6 +226,8 @@ where Err(ref e) if e.kind() == ErrorKind::PermissionDenied => { let m = t!(path.symlink_metadata()); let mut p = m.permissions(); + // this os not unix, so clippy gives FP + #[expect(clippy::permissions_set_readonly_false)] p.set_readonly(false); t!(fs::set_permissions(path, p)); f(path).unwrap_or_else(|e| { diff --git a/src/bootstrap/src/utils/helpers.rs b/src/bootstrap/src/utils/helpers.rs index 7162007e9f0f7..3fb2330469aef 100644 --- a/src/bootstrap/src/utils/helpers.rs +++ b/src/bootstrap/src/utils/helpers.rs @@ -145,7 +145,7 @@ pub fn symlink_dir(config: &Config, original: &Path, link: &Path) -> io::Result< #[cfg(windows)] fn symlink_dir_inner(target: &Path, junction: &Path) -> io::Result<()> { - junction::create(&target, &junction) + junction::create(target, junction) } }