From c2c335e458312365948d80f19ebf80c9a77b5aaa Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 05:42:41 -0600 Subject: [PATCH 01/11] lint: fix clippy::doc_markdown --- src/nix_file.rs | 4 ++-- src/ratchet.rs | 4 ++-- src/validation.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/nix_file.rs b/src/nix_file.rs index 765608d..81e6aed 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -48,7 +48,7 @@ pub struct NixFile { } impl NixFile { - /// Creates a new NixFile, failing for I/O or parse errors. + /// Creates a new `NixFile`, failing for I/O or parse errors. fn new(path: impl AsRef) -> anyhow::Result { let Some(parent_dir) = path.as_ref().parent() else { anyhow::bail!("Could not get parent of path {}", path.as_ref().display()) @@ -116,7 +116,7 @@ impl NixFile { /// /// results in `{ file = ./default.nix; line = 2; column = 3; }` /// - /// - Get the NixFile for `.file` from a `NixFileStore` + /// - Get the `NixFile` for `.file` from a `NixFileStore` /// /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current /// directory. diff --git a/src/ratchet.rs b/src/ratchet.rs index 205db93..32fed4c 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -13,7 +13,7 @@ use crate::validation::{self, Validation, Validation::Success}; /// The ratchet value for the entirety of Nixpkgs. #[derive(Default)] pub struct Nixpkgs { - /// Sorted list of packages in package_map + /// Sorted list of packages in `package_map` pub package_names: Vec, /// The ratchet values for all packages pub package_map: HashMap, @@ -73,7 +73,7 @@ pub enum RatchetState { /// use the latest state, or because the ratchet isn't relevant. Tight, - /// This ratchet can't be applied. State transitions from/to NonApplicable are always allowed. + /// This ratchet can't be applied. State transitions from/to `NonApplicable` are always allowed. NonApplicable, } diff --git a/src/validation.rs b/src/validation.rs index f76e04b..2ff4c96 100644 --- a/src/validation.rs +++ b/src/validation.rs @@ -25,7 +25,7 @@ impl> From

for Validation { /// A type alias representing the result of a check, either: /// -/// - Err(anyhow::Error): A fatal failure, typically I/O errors. +/// - `Err(anyhow::Error)`: A fatal failure, typically I/O errors. /// Such failures are not caused by the files in Nixpkgs. /// This hints at a bug in the code or a problem with the deployment. /// From 670f4edf35a533a11eb9974d0b2f52d4551c878e Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 05:51:54 -0600 Subject: [PATCH 02/11] Error instead of silent truncation of usize to u32 Unlikely this will ever be an issue, but probably would perfer it to be an explicit error since it would indicate another more serious issue is going on. --- src/nix_file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix_file.rs b/src/nix_file.rs index 81e6aed..4ad22ca 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -160,7 +160,7 @@ impl NixFile { let token_at_offset = self .syntax_root .syntax() - .token_at_offset(TextSize::from(index as u32)); + .token_at_offset(TextSize::from(u32::try_from(index)?)); // The token_at_offset function takes indices to mean a location _between_ characters, // which in this case is some spacing followed by the attribute name: From bd701464b0df0fc3fb231e91f05c3f40f18c3c32 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:06:02 -0600 Subject: [PATCH 03/11] Remove unnecessary map_right and to_string --- src/nix_file.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/nix_file.rs b/src/nix_file.rs index 4ad22ca..0a14a9d 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -563,12 +563,7 @@ mod tests { (24, 11, Left("testL")), ]; let expected = BTreeMap::from_iter(cases.map(|(line, column, result)| { - ( - Position { line, column }, - result - .map(ToString::to_string) - .map_right(|node| node.to_string()), - ) + (Position { line, column }, result.map(ToString::to_string)) })); let actual = BTreeMap::from_iter(cases.map(|(line, column, _)| { ( From b32893e7e0bb6c44ba5ca967b523f313d62ed07b Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:11:31 -0600 Subject: [PATCH 04/11] lint: fix clippy::items_after_statements --- src/references.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/references.rs b/src/references.rs index 3994718..9e8b6d1 100644 --- a/src/references.rs +++ b/src/references.rs @@ -5,6 +5,7 @@ use anyhow::Context; use relative_path::RelativePath; use rowan::ast::AstNode; +use crate::nix_file::ResolvedPath; use crate::problem::{npv_121, npv_122, npv_123, npv_124, npv_125, npv_126}; use crate::structure::read_dir_sorted; use crate::validation::{self, ResultIteratorExt, Validation::Success}; @@ -132,8 +133,6 @@ fn check_nix_file( return Success(()); }; - use crate::nix_file::ResolvedPath; - match nix_file.static_resolve_path(path, absolute_package_dir) { ResolvedPath::Interpolated => npv_121::NixFileContainsPathInterpolation::new( relative_package_dir, From 419de9292c24bb3f60c9883ca5167245ab144bf1 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:12:50 -0600 Subject: [PATCH 05/11] lint: fix clippy::derive_partial_eq_without_eq --- src/nix_file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix_file.rs b/src/nix_file.rs index 0a14a9d..20c302e 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -78,7 +78,7 @@ impl NixFile { } /// Information about `callPackage` arguments. -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Eq)] pub struct CallPackageArgumentInfo { /// The relative path of the first argument, or `None` if it's not a path. pub relative_path: Option, From 8aec35a74de2f348e8a3632ad27e42ab01cd54f4 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:14:28 -0600 Subject: [PATCH 06/11] lint: fix clippy::enum_glob_use --- src/eval.rs | 4 ++-- src/validation.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index c4f73ba..d4a94d0 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -454,8 +454,8 @@ fn handle_non_by_name_attribute( attribute_name: &str, non_by_name_attribute: NonByNameAttribute, ) -> validation::Result { - use ratchet::RatchetState::*; - use NonByNameAttribute::*; + use ratchet::RatchetState::{Loose, NonApplicable, Tight}; + use NonByNameAttribute::EvalSuccess; // The ratchet state whether this attribute uses `pkgs/by-name`. // diff --git a/src/validation.rs b/src/validation.rs index 2ff4c96..8264f4a 100644 --- a/src/validation.rs +++ b/src/validation.rs @@ -4,7 +4,7 @@ use itertools::{ Either::{Left, Right}, Itertools, }; -use Validation::*; +use Validation::{Failure, Success}; /// The validation result of a check. Instead of exiting at the first failure, this type can /// accumulate multiple failures. This can be achieved using the functions `and`, `sequence` and From 2e5eb34f0661fa6c4a40661a6aee189c1e0db437 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:22:34 -0600 Subject: [PATCH 07/11] lint: fix clippy::needless_pass_by_value --- src/main.rs | 10 +++++----- src/nix_file.rs | 8 ++++---- src/ratchet.rs | 2 +- src/references.rs | 2 +- src/structure.rs | 4 ++-- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/main.rs b/src/main.rs index d810d53..a07e2d4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -54,7 +54,7 @@ pub struct Args { fn main() -> ExitCode { let args = Args::parse(); - let status: ColoredStatus = process(args.base, args.nixpkgs).into(); + let status: ColoredStatus = process(args.base, &args.nixpkgs).into(); eprintln!("{status}"); status.into() } @@ -64,10 +64,10 @@ fn main() -> ExitCode { /// # Arguments /// - `base_nixpkgs`: Path to the base Nixpkgs to run ratchet checks against. /// - `main_nixpkgs`: Path to the main Nixpkgs to check. -fn process(base_nixpkgs: PathBuf, main_nixpkgs: PathBuf) -> Status { +fn process(base_nixpkgs: PathBuf, main_nixpkgs: &Path) -> Status { // Very easy to parallelise this, since both operations are totally independent of each other. let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs)); - let main_result = match check_nixpkgs(&main_nixpkgs) { + let main_result = match check_nixpkgs(main_nixpkgs) { Ok(result) => result, Err(error) => { return error.into(); @@ -88,7 +88,7 @@ fn process(base_nixpkgs: PathBuf, main_nixpkgs: PathBuf) -> Status { (Failure(..), Success(..)) => Status::BranchHealed, (Success(base), Success(main)) => { // Both base and main branch succeed. Check ratchet state between them... - match ratchet::Nixpkgs::compare(base, main) { + match ratchet::Nixpkgs::compare(&base, main) { Failure(errors) => Status::DiscouragedPatternedIntroduced(errors), Success(..) => Status::ValidatedSuccessfully, } @@ -247,7 +247,7 @@ mod tests { let nix_conf_dir = nix_conf_dir.path().as_os_str(); let status = temp_env::with_var("NIX_CONF_DIR", Some(nix_conf_dir), || { - process(base_nixpkgs, path) + process(base_nixpkgs, &path) }); let actual_errors = format!("{status}\n"); diff --git a/src/nix_file.rs b/src/nix_file.rs index 20c302e..c560506 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -143,7 +143,7 @@ impl NixFile { Right(attrpath_value) => { let definition = attrpath_value.to_string(); let attrpath_value = - self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?; + self.attrpath_value_call_package_argument_info(&attrpath_value, relative_to)?; (attrpath_value, definition) } }) @@ -252,7 +252,7 @@ impl NixFile { // Internal function mainly to make `attrpath_value_at` independently testable. fn attrpath_value_call_package_argument_info( &self, - attrpath_value: ast::AttrpathValue, + attrpath_value: &ast::AttrpathValue, relative_to: &Path, ) -> anyhow::Result> { let Some(attrpath) = attrpath_value.attrpath() else { @@ -326,7 +326,7 @@ impl NixFile { // Check that is a path expression. let path = if let Expr::Path(actual_path) = arg2 { // Try to statically resolve the path and turn it into a nixpkgs-relative path. - if let ResolvedPath::Within(p) = self.static_resolve_path(actual_path, relative_to) { + if let ResolvedPath::Within(p) = self.static_resolve_path(&actual_path, relative_to) { Some(p) } else { // We can't statically know an existing path inside Nixpkgs used as . @@ -417,7 +417,7 @@ impl NixFile { /// /// Given the path expression `./bar.nix` in `./foo.nix` and an absolute path of the /// current directory, the function returns `ResolvedPath::Within(./bar.nix)`. - pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath { + pub fn static_resolve_path(&self, node: &ast::Path, relative_to: &Path) -> ResolvedPath { if node.parts().count() != 1 { // If there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`. return ResolvedPath::Interpolated; diff --git a/src/ratchet.rs b/src/ratchet.rs index 32fed4c..4e21033 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -21,7 +21,7 @@ pub struct Nixpkgs { impl Nixpkgs { /// Validates the ratchet checks for Nixpkgs - pub fn compare(from: Self, to: Self) -> Validation<()> { + pub fn compare(from: &Self, to: Self) -> Validation<()> { validation::sequence_( // We only loop over the current attributes, // we don't need to check ones that were removed diff --git a/src/references.rs b/src/references.rs index 9e8b6d1..dff8191 100644 --- a/src/references.rs +++ b/src/references.rs @@ -133,7 +133,7 @@ fn check_nix_file( return Success(()); }; - match nix_file.static_resolve_path(path, absolute_package_dir) { + match nix_file.static_resolve_path(&path, absolute_package_dir) { ResolvedPath::Interpolated => npv_121::NixFileContainsPathInterpolation::new( relative_package_dir, subpath, diff --git a/src/structure.rs b/src/structure.rs index 93b5975..062ab9b 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -106,7 +106,7 @@ pub fn check_structure( path, &shard_name, shard_name_valid, - package_entry, + &package_entry, ) }) .collect_vec()?; @@ -125,7 +125,7 @@ fn check_package( path: &Path, shard_name: &str, shard_name_valid: bool, - package_entry: DirEntry, + package_entry: &DirEntry, ) -> validation::Result { let package_path = package_entry.path(); let package_name = package_entry.file_name().to_string_lossy().into_owned(); From 929a268b8c8f0c84d67de953497fc99baa79762d Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:26:32 -0600 Subject: [PATCH 08/11] lint: Allow clippy::unnecessary_wraps in one place --- src/eval.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/eval.rs b/src/eval.rs index d4a94d0..49ca339 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -115,6 +115,7 @@ fn pass_through_environment_variables_for_nix_eval_in_nix_build(command: &mut pr } #[cfg(not(test))] +#[allow(clippy::unnecessary_wraps)] fn mutate_nix_instatiate_arguments_based_on_cfg( _work_dir_path: &Path, command: &mut process::Command, From ff1d86a5910ad126fe93d3ada967bb5e35d4c3aa Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:30:54 -0600 Subject: [PATCH 09/11] lint: fix clippy::option_if_let_else --- src/eval.rs | 17 +++++++---------- src/problem/npv_160.rs | 8 +++----- src/problem/npv_161.rs | 8 +++----- src/problem/npv_162.rs | 8 +++----- src/problem/npv_163.rs | 8 +++----- 5 files changed, 19 insertions(+), 30 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 49ca339..fe25c4b 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -314,16 +314,13 @@ fn by_name( // An automatic `callPackage` by the `pkgs/by-name` overlay. // Though this gets detected by checking whether the internal // `_internalCallByNamePackageFile` was used - DefinitionVariant::AutoDefinition => { - if let Some(_location) = location { - // Such an automatic definition should definitely not have a location. - // Having one indicates that somebody is using - // `_internalCallByNamePackageFile`, - npv_102::ByNameInternalCallPackageUsed::new(attribute_name).into() - } else { - Success(Tight) - } - } + DefinitionVariant::AutoDefinition => location.map_or_else( + || Success(Tight), + // Such an automatic definition should definitely not have a location. + // Having one indicates that somebody is using + // `_internalCallByNamePackageFile`, + |_location| npv_102::ByNameInternalCallPackageUsed::new(attribute_name).into(), + ), // The attribute is manually defined, e.g. in `all-packages.nix`. // This means we need to enforce it to look like this: // callPackage ../pkgs/by-name/fo/foo/package.nix { ... } diff --git a/src/problem/npv_160.rs b/src/problem/npv_160.rs index 9bf9571..5c8b7f6 100644 --- a/src/problem/npv_160.rs +++ b/src/problem/npv_160.rs @@ -24,11 +24,9 @@ impl fmt::Display for TopLevelPackageMovedOutOfByName { file, } = self; let relative_package_file = structure::relative_file_for_package(package_name); - let call_package_arg = if let Some(path) = call_package_path { - format!("./{}", path) - } else { - "...".into() - }; + let call_package_arg = call_package_path + .as_ref() + .map_or_else(|| "...".into(), |path| format!("./{}", path)); writedoc!( f, " diff --git a/src/problem/npv_161.rs b/src/problem/npv_161.rs index 36f5dbc..6dc7a66 100644 --- a/src/problem/npv_161.rs +++ b/src/problem/npv_161.rs @@ -24,11 +24,9 @@ impl fmt::Display for TopLevelPackageMovedOutOfByNameWithCustomArguments { file, } = self; let relative_package_file = structure::relative_file_for_package(package_name); - let call_package_arg = if let Some(path) = call_package_path { - format!("./{}", path) - } else { - "...".into() - }; + let call_package_arg = call_package_path + .as_ref() + .map_or_else(|| "...".into(), |path| format!("./{}", path)); writedoc!( f, " diff --git a/src/problem/npv_162.rs b/src/problem/npv_162.rs index 0e3dae3..cbc4708 100644 --- a/src/problem/npv_162.rs +++ b/src/problem/npv_162.rs @@ -24,11 +24,9 @@ impl fmt::Display for NewTopLevelPackageShouldBeByName { file, } = self; let relative_package_file = structure::relative_file_for_package(package_name); - let call_package_arg = if let Some(path) = call_package_path { - format!("./{}", path) - } else { - "...".into() - }; + let call_package_arg = call_package_path + .as_ref() + .map_or_else(|| "...".into(), |path| format!("./{}", path)); writedoc!( f, " diff --git a/src/problem/npv_163.rs b/src/problem/npv_163.rs index dde218a..81278e6 100644 --- a/src/problem/npv_163.rs +++ b/src/problem/npv_163.rs @@ -24,11 +24,9 @@ impl fmt::Display for NewTopLevelPackageShouldBeByNameWithCustomArgument { file, } = self; let relative_package_file = structure::relative_file_for_package(package_name); - let call_package_arg = if let Some(path) = call_package_path { - format!("./{}", path) - } else { - "...".into() - }; + let call_package_arg = call_package_path + .as_ref() + .map_or_else(|| "...".into(), |path| format!("./{}", path)); writedoc!( f, " From 118d596398a5abb6dd228f143b6e3077deca1cc7 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:34:35 -0600 Subject: [PATCH 10/11] CONTRIBUTING: Mention how to run pedantic lints from CLI --- CONTRIBUTING.md | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7a005d7..5298847 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -12,16 +12,33 @@ nix-shell ``` The most important tools and commands in this environment are: + - [rust-analyzer](https://rust-analyzer.github.io/) to have an IDE-like experience for your own editor. + - Running tests: + ```bash cargo test ``` -- Linting and formatting: + +- Formatting: + ```bash - cargo clippy --all-targets treefmt ``` + +- Linting: + + ```bash + cargo clippy --all-targets + ``` + + Optionally, check out extra lints or uncomment them at the top of `main.rs`. + + ```bash + cargo clippy --all-targets -- -W clippy::nursery -W clippy::pedantic + ``` + - Running the [main CI checks](./.github/workflows/main.yml) locally: ```bash nix-build -A ci From 13e4a617244fa911420da1f2e2e691e0d4da8bd4 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 8 Nov 2024 06:35:06 -0600 Subject: [PATCH 11/11] main.rs: update commented out pedantic lints --- src/main.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/main.rs b/src/main.rs index a07e2d4..e46e708 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,10 +1,14 @@ +// Temporarily uncomment to view more pedantic or new nursery lints. // #![warn(clippy::pedantic)] -// #![allow(clippy::uninlined_format_args)] -// #![allow(clippy::enum_glob_use)] -// #![allow(clippy::module_name_repetitions)] -// #![allow(clippy::doc_markdown)] // #![allow(clippy::if_not_else)] // #![allow(clippy::ignored_unit_patterns)] +// #![allow(clippy::module_name_repetitions)] +// #![allow(clippy::uninlined_format_args)] +// #![allow(clippy::unnested_or_patterns)] +// #![warn(clippy::nursery)] +// #![allow(clippy::use_self)] +// #![allow(clippy::missing_const_for_fn)] + mod eval; mod location; mod nix_file;