diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7a005d7b..14329c9d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -17,11 +17,18 @@ The most important tools and commands in this environment are: ```bash cargo test ``` -- Linting and formatting: +- Formatting: ```bash - cargo clippy --all-targets treefmt ``` +- Linting: + ```bash + cargo clippy --all-targets + ``` + Or optionally: + ```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 diff --git a/src/eval.rs b/src/eval.rs index c4f73bab..e469aa09 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, @@ -314,14 +315,15 @@ fn by_name( // 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) - } + location.map_or_else( + || Success(Tight), + |_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() + }, + ) } // The attribute is manually defined, e.g. in `all-packages.nix`. // This means we need to enforce it to look like this: @@ -352,8 +354,7 @@ fn by_name( ) .with_context(|| { format!( - "Failed to get the definition info for attribute {}", - attribute_name + "Failed to get the definition info for attribute {attribute_name}" ) })?; @@ -454,8 +455,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`. // @@ -527,10 +528,14 @@ fn handle_non_by_name_attribute( nixpkgs_path ) .with_context(|| { - format!("Failed to get the definition info for attribute {}", attribute_name) + format!("Failed to get the definition info for attribute {attribute_name}") })?; // At this point, we completed two different checks for whether it's a `callPackage`. + + // Allow pedantic lint: https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns + // fixing it hurts readability of code due to comment restructuring + #[allow(clippy::unnested_or_patterns)] match (is_semantic_call_package, optional_syntactic_call_package) { // Something like ` = { }` (false, None) diff --git a/src/location.rs b/src/location.rs index 0d9d99e4..5418e308 100644 --- a/src/location.rs +++ b/src/location.rs @@ -28,7 +28,7 @@ pub struct LineIndex { } impl LineIndex { - pub fn new(s: &str) -> LineIndex { + pub fn new(s: &str) -> Self { let mut newlines = vec![]; let mut index = 0; // Iterates over all newline-split parts of the string, adding the index of the newline to @@ -37,7 +37,7 @@ impl LineIndex { index += split.len(); newlines.push(index - 1); } - LineIndex { newlines } + Self { newlines } } /// Returns the line number for a string index. diff --git a/src/main.rs b/src/main.rs index d810d53a..1976500a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,10 +1,3 @@ -// #![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)] mod eval; mod location; mod nix_file; @@ -54,7 +47,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 +57,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 +81,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 +240,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 765608d9..0a796003 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -48,8 +48,8 @@ pub struct NixFile { } impl NixFile { - /// Creates a new NixFile, failing for I/O or parse errors. - fn new(path: impl AsRef) -> anyhow::Result { + /// 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()) }; @@ -67,7 +67,7 @@ impl NixFile { // rnix's `rnix::Parse::ok` returns `Result<_, _>`, so no error is thrown away like it // would be with `std::result::Result::ok`. .ok() - .map(|syntax_root| NixFile { + .map(|syntax_root| Self { parent_dir: parent_dir.to_path_buf(), path: path.as_ref().to_owned(), syntax_root, @@ -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, @@ -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. @@ -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) } }) @@ -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: @@ -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; @@ -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, _)| { ( diff --git a/src/problem/npv_160.rs b/src/problem/npv_160.rs index 9bf9571c..0e56e2ac 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 36f5dbc5..f384c016 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 0e3dae33..f0c3ddb6 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 dde218a1..e9f26918 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, " diff --git a/src/ratchet.rs b/src/ratchet.rs index 205db93a..0367f762 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, @@ -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 @@ -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, } @@ -92,12 +92,12 @@ impl RatchetState { fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { match (optional_from, to) { // Loosening a ratchet is not allowed. - (Some(RatchetState::Tight), RatchetState::Loose(loose_context)) => { + (Some(Self::Tight), Self::Loose(loose_context)) => { Context::to_problem(name, Some(()), loose_context).into() } // Introducing a loose ratchet is also not allowed. - (None, RatchetState::Loose(loose_context)) => { + (None, Self::Loose(loose_context)) => { Context::to_problem(name, None, loose_context).into() } diff --git a/src/references.rs b/src/references.rs index 39947185..c429da31 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}; @@ -29,10 +30,7 @@ pub fn check_references( subpath, ) .with_context(|| { - format!( - "While checking the references in package directory {}", - relative_package_dir - ) + format!("While checking the references in package directory {relative_package_dir}") }) } @@ -85,7 +83,7 @@ fn check_path( ) }) .collect_vec() - .with_context(|| format!("Error while recursing into {}", subpath))?, + .with_context(|| format!("Error while recursing into {subpath}"))?, ) } else if path.is_file() { // Only check Nix files @@ -97,7 +95,7 @@ fn check_path( absolute_package_dir, subpath, ) - .with_context(|| format!("Error while checking Nix file {}", subpath))? + .with_context(|| format!("Error while checking Nix file {subpath}"))? } else { Success(()) } @@ -132,9 +130,7 @@ fn check_nix_file( return Success(()); }; - use crate::nix_file::ResolvedPath; - - 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/status.rs b/src/status.rs index 5a706337..80c42c39 100644 --- a/src/status.rs +++ b/src/status.rs @@ -29,7 +29,7 @@ pub enum Status { } impl Status { - fn errors(&self) -> Option<&Vec> { + const fn errors(&self) -> Option<&Vec> { match self { Self::ValidatedSuccessfully | Self::BranchHealed | Self::Error(..) => None, Self::BranchStillBroken(errors) @@ -86,18 +86,18 @@ impl From for Status { impl From for ExitCode { fn from(status: Status) -> Self { match status { - Status::ValidatedSuccessfully | Status::BranchHealed => ExitCode::SUCCESS, + Status::ValidatedSuccessfully | Status::BranchHealed => Self::SUCCESS, Status::BranchStillBroken(..) | Status::ProblemsIntroduced(..) - | Status::DiscouragedPatternedIntroduced(..) => ExitCode::from(1), - Status::Error(..) => ExitCode::from(2), + | Status::DiscouragedPatternedIntroduced(..) => Self::from(1), + Status::Error(..) => Self::from(2), } } } impl fmt::Display for Status { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - Status::fmt(self, f, /* use_color */ false) + Self::fmt(self, f, /* use_color */ false) } } diff --git a/src/structure.rs b/src/structure.rs index 93b5975e..2f3544ac 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -73,10 +73,10 @@ pub fn check_structure( npv_109::ByNameShardIsNotDirectory::new(shard_name).into() } else { let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); - let result = if !shard_name_valid { - npv_110::ByNameShardIsInvalid::new(shard_name.clone()).into() - } else { + let result = if shard_name_valid { Success(()) + } else { + npv_110::ByNameShardIsInvalid::new(shard_name.clone()).into() }; let entries = read_dir_sorted(&shard_path)?; @@ -106,7 +106,7 @@ pub fn check_structure( path, &shard_name, shard_name_valid, - package_entry, + &package_entry, ) }) .collect_vec()?; @@ -125,29 +125,29 @@ 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(); let relative_package_dir = RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); - Ok(if !package_path.is_dir() { - npv_140::PackageDirectoryIsNotDirectory::new(package_name).into() - } else { + Ok(if package_path.is_dir() { let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); - let result = if !package_name_valid { + let result = if package_name_valid { + Success(()) + } else { npv_141::InvalidPackageDirectoryName::new( package_name.clone(), relative_package_dir.clone(), ) .into() - } else { - Success(()) }; let correct_relative_package_dir = relative_dir_for_package(&package_name); - let result = result.and(if relative_package_dir != correct_relative_package_dir { + let result = result.and(if relative_package_dir == correct_relative_package_dir { + Success(()) + } else { // Only show this error if we have a valid shard and package name. // If one of those is wrong, you should fix that first. if shard_name_valid && package_name_valid { @@ -159,8 +159,6 @@ fn check_package( } else { Success(()) } - } else { - Success(()) }); let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); @@ -178,6 +176,8 @@ fn check_package( &relative_package_dir.to_path(path), )?); - result.map(|_| package_name) + result.map(|()| package_name) + } else { + npv_140::PackageDirectoryIsNotDirectory::new(package_name).into() }) } diff --git a/src/validation.rs b/src/validation.rs index f76e04b4..be2daa59 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 @@ -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. /// @@ -77,9 +77,9 @@ impl Validation<()> { /// successful. The `Problem`s of both sides are returned concatenated. pub fn and(self, other: Validation) -> Validation { match (self, other) { - (Success(_), Success(right_value)) => Success(right_value), + (Success(()), Success(right_value)) => Success(right_value), (Failure(errors_l), Failure(errors_r)) => Failure(concat([errors_l, errors_r])), - (Failure(errors), Success(_)) | (Success(_), Failure(errors)) => Failure(errors), + (Failure(errors), Success(_)) | (Success(()), Failure(errors)) => Failure(errors), } } }