From ead18f43f817dee6737137c68e8f6b3126efa98b Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sat, 24 Feb 2024 13:48:48 +0300 Subject: [PATCH 1/5] reorder clippy rules to their original order before passing them 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. Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/check.rs | 35 +++++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index a90139a070ac2..f582d5ada3e99 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -61,14 +61,11 @@ fn args(builder: &Builder<'_>) -> Vec { } } + let all_args = std::env::args().collect::>(); + args.extend(strings(&["--", "--cap-lints", "warn"])); args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint))); - let mut clippy_lint_levels: Vec = Vec::new(); - allow.iter().for_each(|v| clippy_lint_levels.push(format!("-A{}", v))); - deny.iter().for_each(|v| clippy_lint_levels.push(format!("-D{}", v))); - warn.iter().for_each(|v| clippy_lint_levels.push(format!("-W{}", v))); - forbid.iter().for_each(|v| clippy_lint_levels.push(format!("-F{}", v))); - args.extend(clippy_lint_levels); + args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid)); args.extend(builder.config.free_args.clone()); args } else { @@ -76,6 +73,32 @@ fn args(builder: &Builder<'_>) -> 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 { + let mut result = vec![]; + + for (prefix, item) in + [("-A", allow_rules), ("-D", deny_rules), ("-W", warn_rules), ("-F", forbid_rules)] + { + item.iter().for_each(|v| { + let rule = format!("{prefix}{v}"); + let position = all_args.iter().position(|t| t == &rule).unwrap(); + result.push((position, rule)); + }); + } + + result.sort_by_key(|&(position, _)| position); + result.into_iter().map(|v| v.1).collect() +} + fn cargo_subcommand(kind: Kind) -> &'static str { match kind { Kind::Check => "check", From b43dc065dd65c1c459957559882b1f87896e0208 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sat, 24 Feb 2024 15:08:11 +0300 Subject: [PATCH 2/5] add unit test: `order_of_clippy_rules` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/tests.rs | 54 +++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index 93ba5f4120ac1..ee8581ed50917 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -1,5 +1,6 @@ use super::{flags::Flags, ChangeIdWrapper, Config}; use crate::core::config::{LldMode, TomlConfig}; +use crate::core::build_steps::check::get_clippy_rules_in_order; use clap::CommandFactory; use serde::Deserialize; @@ -11,12 +12,13 @@ use std::{ }; fn parse(config: &str) -> Config { - let config = format!("{config} \r\n build.rustc = \"/does-not-exists\" "); Config::parse_inner( &[ - "check".to_owned(), - "--config=/does/not/exist".to_owned(), - "--skip-stage0-validation".to_owned(), + "check".to_string(), + "--set=build.rustc=/does/not/exist".to_string(), + "--set=build.cargo=/does/not/exist".to_string(), + "--config=/does/not/exist".to_string(), + "--skip-stage0-validation".to_string(), ], |&_| toml::from_str(&config).unwrap(), ) @@ -169,7 +171,10 @@ fn override_toml_duplicate() { Config::parse_inner( &[ "check".to_owned(), + "--set=build.rustc=/does/not/exist".to_string(), + "--set=build.cargo=/does/not/exist".to_string(), "--config=/does/not/exist".to_owned(), + "--skip-stage0-validation".to_owned(), "--set=change-id=1".to_owned(), "--set=change-id=2".to_owned(), ], @@ -192,7 +197,15 @@ fn profile_user_dist() { .and_then(|table: toml::Value| TomlConfig::deserialize(table)) .unwrap() } - Config::parse_inner(&["check".to_owned()], get_toml); + Config::parse_inner( + &[ + "check".to_owned(), + "--set=build.rustc=/does/not/exist".to_string(), + "--set=build.cargo=/does/not/exist".to_string(), + "--skip-stage0-validation".to_string(), + ], + get_toml, + ); } #[test] @@ -254,3 +267,34 @@ fn parse_change_id_with_unknown_field() { let change_id_wrapper: ChangeIdWrapper = toml::from_str(config).unwrap(); assert_eq!(change_id_wrapper.inner, Some(3461)); } + +#[test] +fn order_of_clippy_rules() { + let args = vec![ + "clippy".to_string(), + "--fix".to_string(), + "--allow-dirty".to_string(), + "--allow-staged".to_string(), + "-Aclippy:all".to_string(), + "-Wclippy::style".to_string(), + "-Aclippy::foo1".to_string(), + "-Aclippy::foo2".to_string(), + ]; + let config = Config::parse(&args); + + let actual = match &config.cmd { + crate::Subcommand::Clippy { allow, deny, warn, forbid, .. } => { + get_clippy_rules_in_order(&args, &allow, &deny, &warn, &forbid) + } + _ => panic!("invalid subcommand"), + }; + + let expected = vec![ + "-Aclippy:all".to_string(), + "-Wclippy::style".to_string(), + "-Aclippy::foo1".to_string(), + "-Aclippy::foo2".to_string(), + ]; + + assert_eq!(expected, actual); +} From 1945e8f1f6e54dc53ace8cf2314ed76301ff51ee Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sat, 24 Feb 2024 16:02:13 +0300 Subject: [PATCH 3/5] pass ignored lints after manual ones Previously, when passing lint rules manually using `x clippy ..`, ignored lints would override manual ones. This change corrects the order by passing ignored lints after the manual ones. Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index f582d5ada3e99..4d13cf94d918e 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -64,8 +64,8 @@ fn args(builder: &Builder<'_>) -> Vec { let all_args = std::env::args().collect::>(); args.extend(strings(&["--", "--cap-lints", "warn"])); - args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint))); args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid)); + args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint))); args.extend(builder.config.free_args.clone()); args } else { From a61bf3093eb57d6b63522d3ec1f4acceede0363b Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sat, 24 Feb 2024 16:08:28 +0300 Subject: [PATCH 4/5] use `--cap-lints` only when deny and forbid rules are not specified Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/check.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 4d13cf94d918e..55180a82885bb 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -61,10 +61,15 @@ fn args(builder: &Builder<'_>) -> Vec { } } - let all_args = std::env::args().collect::>(); + args.extend(strings(&["--"])); + + if deny.is_empty() && forbid.is_empty() { + args.extend(strings(&["--cap-lints", "warn"])); + } - 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(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint))); args.extend(builder.config.free_args.clone()); args From 81d7d7aabd5cdfb1e574a7ebae0b884e3aad8dea Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sun, 25 Feb 2024 01:11:09 +0300 Subject: [PATCH 5/5] resolve clippy errors Signed-off-by: onur-ozkan --- compiler/rustc_ast/src/token.rs | 1 + compiler/rustc_codegen_llvm/src/context.rs | 1 + .../rustc_const_eval/src/interpret/intern.rs | 4 +++- compiler/rustc_metadata/src/rmeta/encoder.rs | 2 +- compiler/rustc_middle/src/hir/map/mod.rs | 20 ++++++++----------- .../rustc_middle/src/mir/interpret/mod.rs | 4 ++-- .../rustc_mir_build/src/build/matches/mod.rs | 1 - compiler/stable_mir/src/mir/alloc.rs | 8 ++++---- library/std/src/io/buffered/bufreader.rs | 7 +++---- src/bootstrap/src/core/config/tests.rs | 2 +- 10 files changed, 24 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_ast/src/token.rs b/compiler/rustc_ast/src/token.rs index 5ccc7d51066d9..c17020ed6632e 100644 --- a/compiler/rustc_ast/src/token.rs +++ b/compiler/rustc_ast/src/token.rs @@ -11,6 +11,7 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_data_structures::sync::Lrc; use rustc_macros::HashStable_Generic; use rustc_span::symbol::{kw, sym}; +#[allow(clippy::useless_attribute)] // FIXME: following use of `hidden_glob_reexports` incorrectly triggers `useless_attribute` lint. #[allow(hidden_glob_reexports)] use rustc_span::symbol::{Ident, Symbol}; use rustc_span::{edition::Edition, ErrorGuaranteed, Span, DUMMY_SP}; diff --git a/compiler/rustc_codegen_llvm/src/context.rs b/compiler/rustc_codegen_llvm/src/context.rs index f89c8c9f836bf..f9f7e0d4e3754 100644 --- a/compiler/rustc_codegen_llvm/src/context.rs +++ b/compiler/rustc_codegen_llvm/src/context.rs @@ -326,6 +326,7 @@ pub unsafe fn create_module<'ll>( // // On the wasm targets it will get hooked up to the "producer" sections // `processed-by` information. + #[allow(clippy::option_env_unwrap)] let rustc_producer = format!("rustc version {}", option_env!("CFG_VERSION").expect("CFG_VERSION")); let name_metadata = llvm::LLVMMDStringInContext( diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index 17bb59aae8f17..6775a17987206 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -291,7 +291,9 @@ pub fn intern_const_alloc_for_constprop< return Ok(()); } // Move allocation to `tcx`. - for _ in intern_shallow(ecx, alloc_id, Mutability::Not).map_err(|()| err_ub!(DeadLocal))? { + if let Some(_) = + (intern_shallow(ecx, alloc_id, Mutability::Not).map_err(|()| err_ub!(DeadLocal))?).next() + { // We are not doing recursive interning, so we don't currently support provenance. // (If this assertion ever triggers, we should just implement a // proper recursive interning loop -- or just call `intern_const_alloc_recursive`. diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs index d8cfceab460a0..9db8bddfae5f9 100644 --- a/compiler/rustc_metadata/src/rmeta/encoder.rs +++ b/compiler/rustc_metadata/src/rmeta/encoder.rs @@ -2201,7 +2201,7 @@ impl Decodable for EncodedMetadata { let mmap = if len > 0 { let mut mmap = MmapMut::map_anon(len).unwrap(); for _ in 0..len { - (&mut mmap[..]).write(&[d.read_u8()]).unwrap(); + (&mut mmap[..]).write_all(&[d.read_u8()]).unwrap(); } mmap.flush().unwrap(); Some(mmap.make_read_only().unwrap()) diff --git a/compiler/rustc_middle/src/hir/map/mod.rs b/compiler/rustc_middle/src/hir/map/mod.rs index 5043bd855ccb9..b56bdb86129bd 100644 --- a/compiler/rustc_middle/src/hir/map/mod.rs +++ b/compiler/rustc_middle/src/hir/map/mod.rs @@ -114,20 +114,16 @@ impl<'hir> Iterator for ParentOwnerIterator<'hir> { if self.current_id == CRATE_HIR_ID { return None; } - loop { - // There are nodes that do not have entries, so we need to skip them. - let parent_id = self.map.def_key(self.current_id.owner.def_id).parent; - let parent_id = parent_id.map_or(CRATE_OWNER_ID, |local_def_index| { - let def_id = LocalDefId { local_def_index }; - self.map.tcx.local_def_id_to_hir_id(def_id).owner - }); - self.current_id = HirId::make_owner(parent_id.def_id); + let parent_id = self.map.def_key(self.current_id.owner.def_id).parent; + let parent_id = parent_id.map_or(CRATE_OWNER_ID, |local_def_index| { + let def_id = LocalDefId { local_def_index }; + self.map.tcx.local_def_id_to_hir_id(def_id).owner + }); + self.current_id = HirId::make_owner(parent_id.def_id); - // If this `HirId` doesn't have an entry, skip it and look for its `parent_id`. - let node = self.map.tcx.hir_owner_node(self.current_id.owner); - return Some((self.current_id.owner, node)); - } + let node = self.map.tcx.hir_owner_node(self.current_id.owner); + return Some((self.current_id.owner, node)); } } diff --git a/compiler/rustc_middle/src/mir/interpret/mod.rs b/compiler/rustc_middle/src/mir/interpret/mod.rs index f9edbb3c5ae21..6275942bafe65 100644 --- a/compiler/rustc_middle/src/mir/interpret/mod.rs +++ b/compiler/rustc_middle/src/mir/interpret/mod.rs @@ -671,11 +671,11 @@ pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result { - source.read(&mut buf)?; + source.read_exact(&mut buf[..source.len()])?; Ok(u128::from_le_bytes(buf)) } Endian::Big => { - source.read(&mut buf[16 - source.len()..])?; + source.read_exact(&mut buf[16 - source.len()..])?; Ok(u128::from_be_bytes(buf)) } }; diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index e7808ff880b57..d2cbbf9be32c3 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -229,7 +229,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { span: Span, scrutinee_span: Span, ) -> BlockAnd<()> { - let scrutinee_span = scrutinee_span; let scrutinee_place = unpack!(block = self.lower_scrutinee(block, scrutinee_id, scrutinee_span)); diff --git a/compiler/stable_mir/src/mir/alloc.rs b/compiler/stable_mir/src/mir/alloc.rs index c780042ff261c..6645793343875 100644 --- a/compiler/stable_mir/src/mir/alloc.rs +++ b/compiler/stable_mir/src/mir/alloc.rs @@ -57,11 +57,11 @@ pub(crate) fn read_target_uint(mut bytes: &[u8]) -> Result { let mut buf = [0u8; std::mem::size_of::()]; match MachineInfo::target_endianess() { Endian::Little => { - bytes.read(&mut buf)?; + bytes.read_exact(&mut buf[..bytes.len()])?; Ok(u128::from_le_bytes(buf)) } Endian::Big => { - bytes.read(&mut buf[16 - bytes.len()..])?; + bytes.read_exact(&mut buf[16 - bytes.len()..])?; Ok(u128::from_be_bytes(buf)) } } @@ -72,11 +72,11 @@ pub(crate) fn read_target_int(mut bytes: &[u8]) -> Result { let mut buf = [0u8; std::mem::size_of::()]; match MachineInfo::target_endianess() { Endian::Little => { - bytes.read(&mut buf)?; + bytes.read_exact(&mut buf[..bytes.len()])?; Ok(i128::from_le_bytes(buf)) } Endian::Big => { - bytes.read(&mut buf[16 - bytes.len()..])?; + bytes.read_exact(&mut buf[16 - bytes.len()..])?; Ok(i128::from_be_bytes(buf)) } } diff --git a/library/std/src/io/buffered/bufreader.rs b/library/std/src/io/buffered/bufreader.rs index 83db332ee2558..acaa7e9228ecc 100644 --- a/library/std/src/io/buffered/bufreader.rs +++ b/library/std/src/io/buffered/bufreader.rs @@ -328,10 +328,9 @@ impl Read for BufReader { self.discard_buffer(); return self.inner.read_vectored(bufs); } - let nread = { - let mut rem = self.fill_buf()?; - rem.read_vectored(bufs)? - }; + let mut rem = self.fill_buf()?; + let nread = rem.read_vectored(bufs)?; + self.consume(nread); Ok(nread) } diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index ee8581ed50917..8cd538953c56d 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -1,6 +1,6 @@ use super::{flags::Flags, ChangeIdWrapper, Config}; -use crate::core::config::{LldMode, TomlConfig}; use crate::core::build_steps::check::get_clippy_rules_in_order; +use crate::core::config::{LldMode, TomlConfig}; use clap::CommandFactory; use serde::Deserialize;