From a1690089e05b418fb3547038c6b3885f991d5345 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Thu, 21 Mar 2024 22:50:36 -0500 Subject: [PATCH 01/19] Remove superfluous error field relative_package_file It can be derived from the package_name --- src/eval.rs | 9 ++------- src/nixpkgs_problem.rs | 14 ++++++++------ 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 094508f..112d1cd 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -213,8 +213,6 @@ fn by_name( use ratchet::RatchetState::*; use ByNameAttribute::*; - let relative_package_file = structure::relative_file_for_package(attribute_name); - // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists. // This match decides whether the attribute `foo` is defined accordingly // and whether a legacy manual definition could be removed @@ -224,7 +222,6 @@ fn by_name( // This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to // automatically defined attributes in `pkgs/by-name` NixpkgsProblem::UndefinedAttr { - relative_package_file: relative_package_file.to_owned(), package_name: attribute_name.to_owned(), } .into() @@ -242,7 +239,6 @@ fn by_name( // We can't know whether the attribute is automatically or manually defined for sure, // and while we could check the location, the error seems clear enough as is. NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.to_owned(), package_name: attribute_name.to_owned(), } .into() @@ -262,7 +258,6 @@ fn by_name( Success(()) } else { NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.to_owned(), package_name: attribute_name.to_owned(), } .into() @@ -312,7 +307,6 @@ fn by_name( by_name_override( attribute_name, - relative_package_file, is_semantic_call_package, optional_syntactic_call_package, definition, @@ -350,7 +344,6 @@ fn by_name( /// all-packages.nix fn by_name_override( attribute_name: &str, - expected_package_file: RelativePathBuf, is_semantic_call_package: bool, optional_syntactic_call_package: Option, definition: String, @@ -381,6 +374,8 @@ fn by_name_override( // Something like ` = pkgs.callPackage ...` (true, Some(syntactic_call_package)) => { if let Some(actual_package_file) = syntactic_call_package.relative_path { + let expected_package_file = structure::relative_file_for_package(attribute_name); + if actual_package_file != expected_package_file { // Wrong path NixpkgsProblem::WrongCallPackagePath { diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 7e257c0..fb9f793 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -41,7 +41,6 @@ pub enum NixpkgsProblem { relative_package_dir: RelativePathBuf, }, UndefinedAttr { - relative_package_file: RelativePathBuf, package_name: String, }, EmptyArgument { @@ -80,7 +79,6 @@ pub enum NixpkgsProblem { definition: String, }, NonDerivation { - relative_package_file: RelativePathBuf, package_name: String, }, OutsideSymlink { @@ -188,11 +186,13 @@ impl fmt::Display for NixpkgsProblem { f, "{relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", ), - NixpkgsProblem::UndefinedAttr { relative_package_file, package_name } => + NixpkgsProblem::UndefinedAttr { package_name } => { + let relative_package_file = structure::relative_file_for_package(package_name); write!( f, "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}", - ), + ) + } NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } => { let relative_package_dir = structure::relative_dir_for_package(package_name); let relative_package_file = structure::relative_file_for_package(package_name); @@ -280,11 +280,13 @@ impl fmt::Display for NixpkgsProblem { ", ) } - NixpkgsProblem::NonDerivation { relative_package_file, package_name } => + NixpkgsProblem::NonDerivation { package_name } => { + let relative_package_file = structure::relative_file_for_package(package_name); write!( f, "pkgs.{package_name}: This attribute defined by {relative_package_file} is not a derivation", - ), + ) + } NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } => write!( f, From d12e15aaa57c59e52d6414b1b3c00a3296076684 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Thu, 21 Mar 2024 22:52:21 -0500 Subject: [PATCH 02/19] Rename package_name field in InvalidPackageName error to avoid confusing it with a valid package_name where relative_package_dir could be derived. --- src/nixpkgs_problem.rs | 6 +++--- src/structure.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index fb9f793..0bf6036 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -28,7 +28,7 @@ pub enum NixpkgsProblem { }, InvalidPackageName { relative_package_dir: RelativePathBuf, - package_name: String, + invalid_package_name: String, }, IncorrectShard { relative_package_dir: RelativePathBuf, @@ -166,10 +166,10 @@ impl fmt::Display for NixpkgsProblem { f, "{relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.", ), - NixpkgsProblem::InvalidPackageName { relative_package_dir, package_name } => + NixpkgsProblem::InvalidPackageName { relative_package_dir, invalid_package_name } => write!( f, - "{relative_package_dir}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", + "{relative_package_dir}: Invalid package directory name \"{invalid_package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", ), NixpkgsProblem::IncorrectShard { relative_package_dir, correct_relative_package_dir } => write!( diff --git a/src/structure.rs b/src/structure.rs index 09806bc..f24150c 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -134,7 +134,7 @@ fn check_package( let result = if !package_name_valid { NixpkgsProblem::InvalidPackageName { relative_package_dir: relative_package_dir.clone(), - package_name: package_name.clone(), + invalid_package_name: package_name.clone(), } .into() } else { From 0bb64059c45a07583e7c53a2fe703857c67be12c Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 22 Mar 2024 18:35:14 -0500 Subject: [PATCH 03/19] Derive relative_package_dir from the package name for PackageNonDir --- src/nixpkgs_problem.rs | 8 +++++--- src/structure.rs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 0bf6036..1da1ea1 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -19,7 +19,7 @@ pub enum NixpkgsProblem { shard_name: String, }, PackageNonDir { - relative_package_dir: RelativePathBuf, + package_name: String, }, CaseSensitiveDuplicate { relative_shard_path: RelativePathBuf, @@ -156,11 +156,13 @@ impl fmt::Display for NixpkgsProblem { f, "{relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".", ), - NixpkgsProblem::PackageNonDir { relative_package_dir } => + NixpkgsProblem::PackageNonDir { package_name } => { + let relative_package_dir = structure::relative_dir_for_package(package_name); write!( f, "{relative_package_dir}: This path is a file, but it should be a directory.", - ), + ) + } NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } => write!( f, diff --git a/src/structure.rs b/src/structure.rs index f24150c..d24d984 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -126,7 +126,7 @@ fn check_package( Ok(if !package_path.is_dir() { NixpkgsProblem::PackageNonDir { - relative_package_dir: relative_package_dir.clone(), + package_name: package_name.clone(), } .into() } else { From f8bdeab0116576056092110555859e3e168659ce Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 22 Mar 2024 18:36:11 -0500 Subject: [PATCH 04/19] Refactor ratchet problems into a struct with common fields --- src/nixpkgs_problem.rs | 150 +++++++++++++++++++---------------------- src/ratchet.rs | 40 ++++------- 2 files changed, 80 insertions(+), 110 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 1da1ea1..5ab24e6 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -115,26 +115,7 @@ pub enum NixpkgsProblem { text: String, io_error: String, }, - MovedOutOfByNameEmptyArg { - package_name: String, - call_package_path: Option, - file: RelativePathBuf, - }, - MovedOutOfByNameNonEmptyArg { - package_name: String, - call_package_path: Option, - file: RelativePathBuf, - }, - NewPackageNotUsingByNameEmptyArg { - package_name: String, - call_package_path: Option, - file: RelativePathBuf, - }, - NewPackageNotUsingByNameNonEmptyArg { - package_name: String, - call_package_path: Option, - file: RelativePathBuf, - }, + RatchetProblem(RatchetError), InternalCallPackageUsed { attr_name: String, }, @@ -143,6 +124,22 @@ pub enum NixpkgsProblem { }, } +#[derive(Clone)] +pub struct RatchetError { + pub package_name: String, + pub call_package_path: Option, + pub file: RelativePathBuf, + pub kind: RatchetErrorKind, +} + +#[derive(Clone)] +pub enum RatchetErrorKind { + MovedOutOfByNameEmptyArg, + MovedOutOfByNameNonEmptyArg, + NewPackageNotUsingByNameEmptyArg, + NewPackageNotUsingByNameNonEmptyArg, +} + impl fmt::Display for NixpkgsProblem { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { @@ -319,23 +316,12 @@ impl fmt::Display for NixpkgsProblem { f, "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.", ), - NixpkgsProblem::MovedOutOfByNameEmptyArg { package_name, call_package_path, file } => { - let call_package_arg = - if let Some(path) = &call_package_path { - format!("./{path}") - } else { - "...".into() - }; - let relative_package_file = structure::relative_file_for_package(package_name); - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {file}. - Please move the package back and remove the manual `callPackage`. - ", - ) - }, - NixpkgsProblem::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => { + NixpkgsProblem::RatchetProblem(RatchetError { + package_name, + call_package_path, + file, + kind, + }) => { let call_package_arg = if let Some(path) = &call_package_path { format!("./{}", path) @@ -343,51 +329,51 @@ impl fmt::Display for NixpkgsProblem { "...".into() }; let relative_package_file = structure::relative_file_for_package(package_name); - // This can happen if users mistakenly assume that for custom arguments, - // pkgs/by-name can't be used. - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {file}. - While the manual `callPackage` is still needed, it's not necessary to move the package files. - ", - ) - }, - NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { package_name, call_package_path, file } => { - let call_package_arg = - if let Some(path) = &call_package_path { - format!("./{}", path) - } else { - "...".into() - }; - let relative_package_file = structure::relative_file_for_package(package_name); - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. - Please define it in {relative_package_file} instead. - See `pkgs/by-name/README.md` for more details. - Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore. - ", - ) - }, - NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => { - let call_package_arg = - if let Some(path) = &call_package_path { - format!("./{}", path) - } else { - "...".into() - }; - let relative_package_file = structure::relative_file_for_package(package_name); - writedoc!( - f, - " - - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. - Please define it in {relative_package_file} instead. - See `pkgs/by-name/README.md` for more details. - Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {file} is still needed. - ", - ) + + match kind { + RatchetErrorKind::MovedOutOfByNameEmptyArg => { + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {file}. + Please move the package back and remove the manual `callPackage`. + ", + ) + }, + RatchetErrorKind::MovedOutOfByNameNonEmptyArg => { + // This can happen if users mistakenly assume that for custom arguments, + // pkgs/by-name can't be used. + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {file}. + While the manual `callPackage` is still needed, it's not necessary to move the package files. + ", + ) + }, + RatchetErrorKind::NewPackageNotUsingByNameEmptyArg => { + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. + Please define it in {relative_package_file} instead. + See `pkgs/by-name/README.md` for more details. + Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore. + ", + ) + }, + RatchetErrorKind::NewPackageNotUsingByNameNonEmptyArg => { + writedoc!( + f, + " + - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`. + Please define it in {relative_package_file} instead. + See `pkgs/by-name/README.md` for more details. + Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {file} is still needed. + ", + ) + }, + } }, NixpkgsProblem::InternalCallPackageUsed { attr_name } => write!( diff --git a/src/ratchet.rs b/src/ratchet.rs index 8136d64..9938051 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -3,7 +3,7 @@ //! Each type has a `compare` method that validates the ratchet checks for that item. use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::NixpkgsProblem; +use crate::nixpkgs_problem::{NixpkgsProblem, RatchetError, RatchetErrorKind}; use crate::validation::{self, Validation, Validation::Success}; use relative_path::RelativePathBuf; use std::collections::HashMap; @@ -153,32 +153,16 @@ impl ToNixpkgsProblem for UsesByName { optional_from: Option<()>, (to, file): &Self::ToContext, ) -> NixpkgsProblem { - if let Some(()) = optional_from { - if to.empty_arg { - NixpkgsProblem::MovedOutOfByNameEmptyArg { - package_name: name.to_owned(), - call_package_path: to.relative_path.clone(), - file: file.to_owned(), - } - } else { - NixpkgsProblem::MovedOutOfByNameNonEmptyArg { - package_name: name.to_owned(), - call_package_path: to.relative_path.clone(), - file: file.to_owned(), - } - } - } else if to.empty_arg { - NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { - package_name: name.to_owned(), - call_package_path: to.relative_path.clone(), - file: file.to_owned(), - } - } else { - NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { - package_name: name.to_owned(), - call_package_path: to.relative_path.clone(), - file: file.to_owned(), - } - } + NixpkgsProblem::RatchetProblem(RatchetError { + package_name: name.to_owned(), + call_package_path: to.relative_path.clone(), + file: file.to_owned(), + kind: match (optional_from, to.empty_arg) { + (Some(()), true) => RatchetErrorKind::MovedOutOfByNameEmptyArg, + (Some(()), false) => RatchetErrorKind::MovedOutOfByNameNonEmptyArg, + (None, true) => RatchetErrorKind::NewPackageNotUsingByNameEmptyArg, + (None, false) => RatchetErrorKind::NewPackageNotUsingByNameNonEmptyArg, + }, + }) } } From 5c502a2ef04b54562d3fdcb24be38aa024667d9b Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 22 Mar 2024 20:11:12 -0500 Subject: [PATCH 05/19] Remove unnecessary braces --- src/nixpkgs_problem.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 5ab24e6..fa508aa 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -331,16 +331,15 @@ impl fmt::Display for NixpkgsProblem { let relative_package_file = structure::relative_file_for_package(package_name); match kind { - RatchetErrorKind::MovedOutOfByNameEmptyArg => { + RatchetErrorKind::MovedOutOfByNameEmptyArg => writedoc!( f, " - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {file}. Please move the package back and remove the manual `callPackage`. ", - ) - }, - RatchetErrorKind::MovedOutOfByNameNonEmptyArg => { + ), + RatchetErrorKind::MovedOutOfByNameNonEmptyArg => // This can happen if users mistakenly assume that for custom arguments, // pkgs/by-name can't be used. writedoc!( @@ -349,9 +348,8 @@ impl fmt::Display for NixpkgsProblem { - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {file}. While the manual `callPackage` is still needed, it's not necessary to move the package files. ", - ) - }, - RatchetErrorKind::NewPackageNotUsingByNameEmptyArg => { + ), + RatchetErrorKind::NewPackageNotUsingByNameEmptyArg => writedoc!( f, " @@ -360,9 +358,8 @@ impl fmt::Display for NixpkgsProblem { See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore. ", - ) - }, - RatchetErrorKind::NewPackageNotUsingByNameNonEmptyArg => { + ), + RatchetErrorKind::NewPackageNotUsingByNameNonEmptyArg => writedoc!( f, " @@ -371,8 +368,7 @@ impl fmt::Display for NixpkgsProblem { See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {file} is still needed. ", - ) - }, + ), } }, NixpkgsProblem::InternalCallPackageUsed { attr_name } => From 9391e85bd3ea55cef1bbd3150a07b1a46e63730c Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 22 Mar 2024 20:47:52 -0500 Subject: [PATCH 06/19] Group shard problems into a struct for shared context --- src/nixpkgs_problem.rs | 64 ++++++++++++++++++++++++------------------ src/structure.rs | 28 ++++++++++-------- 2 files changed, 53 insertions(+), 39 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index fa508aa..c576e8b 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -11,21 +11,10 @@ use std::fmt; /// location #[derive(Clone)] pub enum NixpkgsProblem { - ShardNonDir { - relative_shard_path: RelativePathBuf, - }, - InvalidShardName { - relative_shard_path: RelativePathBuf, - shard_name: String, - }, + ShardProblem(ShardError), PackageNonDir { package_name: String, }, - CaseSensitiveDuplicate { - relative_shard_path: RelativePathBuf, - first: OsString, - second: OsString, - }, InvalidPackageName { relative_package_dir: RelativePathBuf, invalid_package_name: String, @@ -124,6 +113,19 @@ pub enum NixpkgsProblem { }, } +#[derive(Clone)] +pub struct ShardError { + pub shard_name: String, + pub kind: ShardErrorKind, +} + +#[derive(Clone)] +pub enum ShardErrorKind { + ShardNonDir, + InvalidShardName, + CaseSensitiveDuplicate { first: OsString, second: OsString }, +} + #[derive(Clone)] pub struct RatchetError { pub package_name: String, @@ -143,16 +145,29 @@ pub enum RatchetErrorKind { impl fmt::Display for NixpkgsProblem { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - NixpkgsProblem::ShardNonDir { relative_shard_path } => - write!( - f, - "{relative_shard_path}: This is a file, but it should be a directory.", - ), - NixpkgsProblem::InvalidShardName { relative_shard_path, shard_name } => - write!( - f, - "{relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".", - ), + NixpkgsProblem::ShardProblem(ShardError { + shard_name, + kind, + }) => { + let relative_shard_path = structure::relative_dir_for_shard(&shard_name); + match kind { + ShardErrorKind::ShardNonDir => + write!( + f, + "{relative_shard_path}: This is a file, but it should be a directory.", + ), + ShardErrorKind::InvalidShardName => + write!( + f, + "{relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".", + ), + ShardErrorKind::CaseSensitiveDuplicate { first, second } => + write!( + f, + "{relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.", + ), + } + } NixpkgsProblem::PackageNonDir { package_name } => { let relative_package_dir = structure::relative_dir_for_package(package_name); write!( @@ -160,11 +175,6 @@ impl fmt::Display for NixpkgsProblem { "{relative_package_dir}: This path is a file, but it should be a directory.", ) } - NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } => - write!( - f, - "{relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.", - ), NixpkgsProblem::InvalidPackageName { relative_package_dir, invalid_package_name } => write!( f, diff --git a/src/structure.rs b/src/structure.rs index d24d984..5198898 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -1,4 +1,6 @@ use crate::nixpkgs_problem::NixpkgsProblem; +use crate::nixpkgs_problem::ShardError; +use crate::nixpkgs_problem::ShardErrorKind; use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; @@ -47,25 +49,25 @@ pub fn check_structure( .map(|shard_entry| -> validation::Result<_> { let shard_path = shard_entry.path(); let shard_name = shard_entry.file_name().to_string_lossy().into_owned(); - let relative_shard_path = relative_dir_for_shard(&shard_name); Ok(if shard_name == "README.md" { // README.md is allowed to be a file and not checked Success(vec![]) } else if !shard_path.is_dir() { - NixpkgsProblem::ShardNonDir { - relative_shard_path: relative_shard_path.clone(), - } + NixpkgsProblem::ShardProblem(ShardError { + shard_name: shard_name.clone(), + kind: ShardErrorKind::ShardNonDir, + }) .into() // we can't check for any other errors if it's a file, since there's no subdirectories to check } else { let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); let result = if !shard_name_valid { - NixpkgsProblem::InvalidShardName { - relative_shard_path: relative_shard_path.clone(), + NixpkgsProblem::ShardProblem(ShardError { shard_name: shard_name.clone(), - } + kind: ShardErrorKind::InvalidShardName, + }) .into() } else { Success(()) @@ -80,11 +82,13 @@ pub fn check_structure( l.file_name().to_ascii_lowercase() == r.file_name().to_ascii_lowercase() }) .map(|(l, r)| { - NixpkgsProblem::CaseSensitiveDuplicate { - relative_shard_path: relative_shard_path.clone(), - first: l.file_name(), - second: r.file_name(), - } + NixpkgsProblem::ShardProblem(ShardError { + shard_name: shard_name.clone(), + kind: ShardErrorKind::CaseSensitiveDuplicate { + first: l.file_name(), + second: r.file_name(), + }, + }) .into() }); From 686cbbc1379590a4f685df4c01635d9675eef858 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 22 Mar 2024 21:48:37 -0500 Subject: [PATCH 07/19] Fix clippy warnings --- src/nixpkgs_problem.rs | 10 +++++----- src/ratchet.rs | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index c576e8b..08414a5 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -136,10 +136,10 @@ pub struct RatchetError { #[derive(Clone)] pub enum RatchetErrorKind { + MovedOutOfByName, MovedOutOfByNameEmptyArg, - MovedOutOfByNameNonEmptyArg, + NewPackageNotUsingByName, NewPackageNotUsingByNameEmptyArg, - NewPackageNotUsingByNameNonEmptyArg, } impl fmt::Display for NixpkgsProblem { @@ -149,7 +149,7 @@ impl fmt::Display for NixpkgsProblem { shard_name, kind, }) => { - let relative_shard_path = structure::relative_dir_for_shard(&shard_name); + let relative_shard_path = structure::relative_dir_for_shard(shard_name); match kind { ShardErrorKind::ShardNonDir => write!( @@ -349,7 +349,7 @@ impl fmt::Display for NixpkgsProblem { Please move the package back and remove the manual `callPackage`. ", ), - RatchetErrorKind::MovedOutOfByNameNonEmptyArg => + RatchetErrorKind::MovedOutOfByName => // This can happen if users mistakenly assume that for custom arguments, // pkgs/by-name can't be used. writedoc!( @@ -369,7 +369,7 @@ impl fmt::Display for NixpkgsProblem { Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore. ", ), - RatchetErrorKind::NewPackageNotUsingByNameNonEmptyArg => + RatchetErrorKind::NewPackageNotUsingByName => writedoc!( f, " diff --git a/src/ratchet.rs b/src/ratchet.rs index 9938051..d00fba4 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -159,9 +159,9 @@ impl ToNixpkgsProblem for UsesByName { file: file.to_owned(), kind: match (optional_from, to.empty_arg) { (Some(()), true) => RatchetErrorKind::MovedOutOfByNameEmptyArg, - (Some(()), false) => RatchetErrorKind::MovedOutOfByNameNonEmptyArg, + (Some(()), false) => RatchetErrorKind::MovedOutOfByName, (None, true) => RatchetErrorKind::NewPackageNotUsingByNameEmptyArg, - (None, false) => RatchetErrorKind::NewPackageNotUsingByNameNonEmptyArg, + (None, false) => RatchetErrorKind::NewPackageNotUsingByName, }, }) } From 94244b040cdf7c4364bcc81866a36d649be003da Mon Sep 17 00:00:00 2001 From: Will Bush Date: Fri, 22 Mar 2024 21:49:14 -0500 Subject: [PATCH 08/19] Refactor nix file problems into a struct with common fields --- src/nixpkgs_problem.rs | 93 ++++++++++++++++++++++-------------------- src/references.rs | 58 +++++++++++--------------- 2 files changed, 71 insertions(+), 80 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 08414a5..c5a6789 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -79,31 +79,7 @@ pub enum NixpkgsProblem { subpath: RelativePathBuf, io_error: String, }, - PathInterpolation { - relative_package_dir: RelativePathBuf, - subpath: RelativePathBuf, - line: usize, - text: String, - }, - SearchPath { - relative_package_dir: RelativePathBuf, - subpath: RelativePathBuf, - line: usize, - text: String, - }, - OutsidePathReference { - relative_package_dir: RelativePathBuf, - subpath: RelativePathBuf, - line: usize, - text: String, - }, - UnresolvablePathReference { - relative_package_dir: RelativePathBuf, - subpath: RelativePathBuf, - line: usize, - text: String, - io_error: String, - }, + NixFileProblem(NixFileError), RatchetProblem(RatchetError), InternalCallPackageUsed { attr_name: String, @@ -126,6 +102,23 @@ pub enum ShardErrorKind { CaseSensitiveDuplicate { first: OsString, second: OsString }, } +#[derive(Clone)] +pub struct NixFileError { + pub relative_package_dir: RelativePathBuf, + pub subpath: RelativePathBuf, + pub line: usize, + pub text: String, + pub kind: NixFileErrorKind, +} + +#[derive(Clone)] +pub enum NixFileErrorKind { + PathInterpolation, + SearchPath, + OutsidePathReference, + UnresolvablePathReference { io_error: String }, +} + #[derive(Clone)] pub struct RatchetError { pub package_name: String, @@ -306,26 +299,36 @@ impl fmt::Display for NixpkgsProblem { f, "{relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.", ), - NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } => - write!( - f, - "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.", - ), - NixpkgsProblem::SearchPath { relative_package_dir, subpath, line, text } => - write!( - f, - "{relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.", - ), - NixpkgsProblem::OutsidePathReference { relative_package_dir, subpath, line, text } => - write!( - f, - "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package.", - ), - NixpkgsProblem::UnresolvablePathReference { relative_package_dir, subpath, line, text, io_error } => - write!( - f, - "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.", - ), + NixpkgsProblem::NixFileProblem(NixFileError { + relative_package_dir, + subpath, + line, + text, + kind + }) => { + match kind { + NixFileErrorKind::PathInterpolation => + write!( + f, + "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.", + ), + NixFileErrorKind::SearchPath => + write!( + f, + "{relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.", + ), + NixFileErrorKind::OutsidePathReference => + write!( + f, + "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package.", + ), + NixFileErrorKind::UnresolvablePathReference { io_error } => + write!( + f, + "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.", + ), + } + }, NixpkgsProblem::RatchetProblem(RatchetError { package_name, call_package_path, diff --git a/src/references.rs b/src/references.rs index e231916..b702f14 100644 --- a/src/references.rs +++ b/src/references.rs @@ -1,4 +1,4 @@ -use crate::nixpkgs_problem::NixpkgsProblem; +use crate::nixpkgs_problem::{NixFileError, NixFileErrorKind, NixpkgsProblem}; use crate::utils; use crate::validation::{self, ResultIteratorExt, Validation::Success}; use crate::NixFileStore; @@ -125,51 +125,39 @@ fn check_nix_file( Ok(validation::sequence_( nix_file.syntax_root.syntax().descendants().map(|node| { - let text = node.text().to_string(); - let line = nix_file.line_index.line(node.text_range().start().into()); - // We're only interested in Path expressions - let Some(path) = rnix::ast::Path::cast(node) else { + let Some(path) = rnix::ast::Path::cast(node.clone()) else { return Success(()); }; use crate::nix_file::ResolvedPath; - match nix_file.static_resolve_path(path, absolute_package_dir) { - ResolvedPath::Interpolated => NixpkgsProblem::PathInterpolation { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - line, - text, - } - .into(), - ResolvedPath::SearchPath => NixpkgsProblem::SearchPath { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - line, - text, + let error_or_none = match nix_file.static_resolve_path(path, absolute_package_dir) { + ResolvedPath::Interpolated => Some(NixFileErrorKind::PathInterpolation), + ResolvedPath::SearchPath => Some(NixFileErrorKind::SearchPath), + ResolvedPath::Outside => Some(NixFileErrorKind::OutsidePathReference), + ResolvedPath::Unresolvable(e) => { + Some(NixFileErrorKind::UnresolvablePathReference { + io_error: e.to_string(), + }) } - .into(), - ResolvedPath::Outside => NixpkgsProblem::OutsidePathReference { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - line, - text, - } - .into(), - ResolvedPath::Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - line, - text, - io_error: e.to_string(), - } - .into(), ResolvedPath::Within(..) => { // No need to handle the case of it being inside the directory, since we scan through the // entire directory recursively anyways - Success(()) + None } + }; + if let Some(kind) = error_or_none { + NixpkgsProblem::NixFileProblem(NixFileError { + relative_package_dir: relative_package_dir.to_owned(), + subpath: subpath.to_owned(), + line: nix_file.line_index.line(node.text_range().start().into()), + text: node.text().to_string(), + kind, + }) + .into() + } else { + Success(()) } }), )) From 7eaa75464eecb4f96a4b01cfb786c630d681c671 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Sat, 23 Mar 2024 19:28:09 -0500 Subject: [PATCH 09/19] Refactor ByNameOveride problems into a struct with common fields --- src/eval.rs | 53 ++++-------- src/nixpkgs_problem.rs | 192 +++++++++++++++++++---------------------- 2 files changed, 107 insertions(+), 138 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 112d1cd..bf6e1bd 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -1,4 +1,6 @@ use crate::nix_file::CallPackageArgumentInfo; +use crate::nixpkgs_problem::ByNameOverrideError; +use crate::nixpkgs_problem::ByNameOverrideErrorKind; use crate::nixpkgs_problem::NixpkgsProblem; use crate::ratchet; use crate::ratchet::RatchetState::Loose; @@ -350,27 +352,24 @@ fn by_name_override( location: Location, relative_location_file: RelativePathBuf, ) -> validation::Validation> { - // At this point, we completed two different checks for whether it's a - // `callPackage` - match (is_semantic_call_package, optional_syntactic_call_package) { - // Something like ` = foo` - (_, None) => NixpkgsProblem::NonSyntacticCallPackage { + let to_problem = |kind| -> NixpkgsProblem { + NixpkgsProblem::ByNameOverrideProblem(ByNameOverrideError { package_name: attribute_name.to_owned(), file: relative_location_file, line: location.line, column: location.column, definition, - } - .into(), + kind, + }) + }; + + // At this point, we completed two different checks for whether it's a + // `callPackage` + match (is_semantic_call_package, optional_syntactic_call_package) { + // Something like ` = foo` + (_, None) => to_problem(ByNameOverrideErrorKind::NonSyntacticCallPackage).into(), // Something like ` = pythonPackages.callPackage ...` - (false, Some(_)) => NixpkgsProblem::NonToplevelCallPackage { - package_name: attribute_name.to_owned(), - file: relative_location_file, - line: location.line, - column: location.column, - definition, - } - .into(), + (false, Some(_)) => to_problem(ByNameOverrideErrorKind::NonToplevelCallPackage).into(), // Something like ` = pkgs.callPackage ...` (true, Some(syntactic_call_package)) => { if let Some(actual_package_file) = syntactic_call_package.relative_path { @@ -378,26 +377,17 @@ fn by_name_override( if actual_package_file != expected_package_file { // Wrong path - NixpkgsProblem::WrongCallPackagePath { - package_name: attribute_name.to_owned(), - file: relative_location_file, - line: location.line, + to_problem(ByNameOverrideErrorKind::WrongCallPackagePath { actual_path: actual_package_file, expected_path: expected_package_file, - } + }) .into() } else { // Manual definitions with empty arguments are not allowed // anymore, but existing ones should continue to be allowed let manual_definition_ratchet = if syntactic_call_package.empty_arg { // This is the state to migrate away from - Loose(NixpkgsProblem::EmptyArgument { - package_name: attribute_name.to_owned(), - file: relative_location_file, - line: location.line, - column: location.column, - definition, - }) + Loose(to_problem(ByNameOverrideErrorKind::EmptyArgument)) } else { // This is the state to migrate to Tight @@ -407,14 +397,7 @@ fn by_name_override( } } else { // No path - NixpkgsProblem::NonPath { - package_name: attribute_name.to_owned(), - file: relative_location_file, - line: location.line, - column: location.column, - definition, - } - .into() + to_problem(ByNameOverrideErrorKind::NonPath).into() } } } diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index c5a6789..46d94a6 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -32,41 +32,7 @@ pub enum NixpkgsProblem { UndefinedAttr { package_name: String, }, - EmptyArgument { - package_name: String, - file: RelativePathBuf, - line: usize, - column: usize, - definition: String, - }, - NonToplevelCallPackage { - package_name: String, - file: RelativePathBuf, - line: usize, - column: usize, - definition: String, - }, - NonPath { - package_name: String, - file: RelativePathBuf, - line: usize, - column: usize, - definition: String, - }, - WrongCallPackagePath { - package_name: String, - file: RelativePathBuf, - line: usize, - actual_path: RelativePathBuf, - expected_path: RelativePathBuf, - }, - NonSyntacticCallPackage { - package_name: String, - file: RelativePathBuf, - line: usize, - column: usize, - definition: String, - }, + ByNameOverrideProblem(ByNameOverrideError), NonDerivation { package_name: String, }, @@ -102,6 +68,28 @@ pub enum ShardErrorKind { CaseSensitiveDuplicate { first: OsString, second: OsString }, } +#[derive(Clone)] +pub struct ByNameOverrideError { + pub package_name: String, + pub file: RelativePathBuf, + pub line: usize, + pub column: usize, + pub definition: String, + pub kind: ByNameOverrideErrorKind, +} + +#[derive(Clone)] +pub enum ByNameOverrideErrorKind { + NonSyntacticCallPackage, + NonToplevelCallPackage, + WrongCallPackagePath { + actual_path: RelativePathBuf, + expected_path: RelativePathBuf, + }, + EmptyArgument, + NonPath, +} + #[derive(Clone)] pub struct NixFileError { pub relative_package_dir: RelativePathBuf, @@ -195,93 +183,91 @@ impl fmt::Display for NixpkgsProblem { "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}", ) } - NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } => { + NixpkgsProblem::ByNameOverrideProblem(ByNameOverrideError { + package_name, + file, + line, + column, + definition, + kind, + }) => { let relative_package_dir = structure::relative_dir_for_package(package_name); let relative_package_file = structure::relative_file_for_package(package_name); let indented_definition = indent_definition(*column, definition.clone()); - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; + match kind { + ByNameOverrideErrorKind::NonSyntacticCallPackage => + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - However, in this PR, the second argument is empty. See the definition in {file}:{line}: + {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; - {indented_definition} + However, in this PR, it isn't defined that way. See the definition in {file}:{line} - Such a definition is provided automatically and therefore not necessary. Please remove it. - ", - ) - } - NixpkgsProblem::NonToplevelCallPackage { package_name, file, line, column, definition } => { - let relative_package_dir = structure::relative_dir_for_package(package_name); - let relative_package_file = structure::relative_file_for_package(package_name); - let indented_definition = indent_definition(*column, definition.clone()); - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + {indented_definition} + ", + ), + ByNameOverrideErrorKind::NonToplevelCallPackage => + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; + {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; - However, in this PR, a different `callPackage` is used. See the definition in {file}:{line}: + However, in this PR, a different `callPackage` is used. See the definition in {file}:{line}: - {indented_definition} - ", - ) - } - NixpkgsProblem::NonPath { package_name, file, line, column, definition } => { - let relative_package_dir = structure::relative_dir_for_package(package_name); - let relative_package_file = structure::relative_file_for_package(package_name); - let indented_definition = indent_definition(*column, definition.clone()); - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + {indented_definition} + ", + ), + ByNameOverrideErrorKind::WrongCallPackagePath { actual_path, expected_path } => { + let expected_path_expr = create_path_expr(file, expected_path); + let actual_path_expr = create_path_expr(file, actual_path); + writedoc! { + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; + {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; - However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}: + However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {file}:{line}: - {indented_definition} - ", - ) - } - NixpkgsProblem::WrongCallPackagePath { package_name, file, line, actual_path, expected_path } => { - let relative_package_dir = structure::relative_dir_for_package(package_name); - let expected_path_expr = create_path_expr(file, expected_path); - let actual_path_expr = create_path_expr(file, actual_path); - writedoc! { - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + {package_name} = callPackage {actual_path_expr} {{ /* ... */ }}; + ", + } + } + ByNameOverrideErrorKind::EmptyArgument => + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - {package_name} = callPackage {expected_path_expr} {{ /* ... */ }}; + {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; - However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {file}:{line}: + However, in this PR, the second argument is empty. See the definition in {file}:{line}: - {package_name} = callPackage {actual_path_expr} {{ /* ... */ }}; - ", - } - } - NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => { - let relative_package_dir = structure::relative_dir_for_package(package_name); - let relative_package_file = structure::relative_file_for_package(package_name); - let indented_definition = indent_definition(*column, definition.clone()); - writedoc!( - f, - " - - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like + {indented_definition} + + Such a definition is provided automatically and therefore not necessary. Please remove it. + ", + ), + ByNameOverrideErrorKind::NonPath => + writedoc!( + f, + " + - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like - {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; + {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; - However, in this PR, it isn't defined that way. See the definition in {file}:{line} + However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}: - {indented_definition} - ", - ) - } + {indented_definition} + ", + ), + } + }, NixpkgsProblem::NonDerivation { package_name } => { let relative_package_file = structure::relative_file_for_package(package_name); write!( From d32d4cf43356033ba985986f306de1058c48e2ab Mon Sep 17 00:00:00 2001 From: Will Bush Date: Sat, 23 Mar 2024 19:38:39 -0500 Subject: [PATCH 10/19] Refactor check_nix_file to use to_problem closure --- src/eval.rs | 2 +- src/references.rs | 37 +++++++++++++++++++------------------ 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index bf6e1bd..2b02e71 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -352,7 +352,7 @@ fn by_name_override( location: Location, relative_location_file: RelativePathBuf, ) -> validation::Validation> { - let to_problem = |kind| -> NixpkgsProblem { + let to_problem = |kind| { NixpkgsProblem::ByNameOverrideProblem(ByNameOverrideError { package_name: attribute_name.to_owned(), file: relative_location_file, diff --git a/src/references.rs b/src/references.rs index b702f14..5ebaa8a 100644 --- a/src/references.rs +++ b/src/references.rs @@ -130,34 +130,35 @@ fn check_nix_file( return Success(()); }; + let to_problem = |kind| { + NixpkgsProblem::NixFileProblem(NixFileError { + relative_package_dir: relative_package_dir.to_owned(), + subpath: subpath.to_owned(), + line: nix_file.line_index.line(node.text_range().start().into()), + text: node.text().to_string(), + kind, + }) + }; + use crate::nix_file::ResolvedPath; - let error_or_none = match nix_file.static_resolve_path(path, absolute_package_dir) { - ResolvedPath::Interpolated => Some(NixFileErrorKind::PathInterpolation), - ResolvedPath::SearchPath => Some(NixFileErrorKind::SearchPath), - ResolvedPath::Outside => Some(NixFileErrorKind::OutsidePathReference), + match nix_file.static_resolve_path(path, absolute_package_dir) { + ResolvedPath::Interpolated => { + to_problem(NixFileErrorKind::PathInterpolation).into() + } + ResolvedPath::SearchPath => to_problem(NixFileErrorKind::SearchPath).into(), + ResolvedPath::Outside => to_problem(NixFileErrorKind::OutsidePathReference).into(), ResolvedPath::Unresolvable(e) => { - Some(NixFileErrorKind::UnresolvablePathReference { + to_problem(NixFileErrorKind::UnresolvablePathReference { io_error: e.to_string(), }) + .into() } ResolvedPath::Within(..) => { // No need to handle the case of it being inside the directory, since we scan through the // entire directory recursively anyways - None + Success(()) } - }; - if let Some(kind) = error_or_none { - NixpkgsProblem::NixFileProblem(NixFileError { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - line: nix_file.line_index.line(node.text_range().start().into()), - text: node.text().to_string(), - kind, - }) - .into() - } else { - Success(()) } }), )) From 797f201bdffae76d3a6dd71501ba94495da3227c Mon Sep 17 00:00:00 2001 From: Will Bush Date: Sat, 23 Mar 2024 20:02:50 -0500 Subject: [PATCH 11/19] Refactor ByName problems into a struct with common fields --- src/eval.rs | 34 ++++++++---------- src/nixpkgs_problem.rs | 82 ++++++++++++++++++++++++------------------ 2 files changed, 61 insertions(+), 55 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 2b02e71..1b63b65 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -1,4 +1,6 @@ use crate::nix_file::CallPackageArgumentInfo; +use crate::nixpkgs_problem::ByNameError; +use crate::nixpkgs_problem::ByNameErrorKind; use crate::nixpkgs_problem::ByNameOverrideError; use crate::nixpkgs_problem::ByNameOverrideErrorKind; use crate::nixpkgs_problem::NixpkgsProblem; @@ -215,6 +217,13 @@ fn by_name( use ratchet::RatchetState::*; use ByNameAttribute::*; + let to_problem = |kind| { + NixpkgsProblem::ByNameProblem(ByNameError { + attribute_name: attribute_name.to_owned(), + kind, + }) + }; + // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists. // This match decides whether the attribute `foo` is defined accordingly // and whether a legacy manual definition could be removed @@ -223,10 +232,7 @@ fn by_name( Missing => { // This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to // automatically defined attributes in `pkgs/by-name` - NixpkgsProblem::UndefinedAttr { - package_name: attribute_name.to_owned(), - } - .into() + to_problem(ByNameErrorKind::UndefinedAttr).into() } // The attribute exists Existing(AttributeInfo { @@ -240,10 +246,7 @@ fn by_name( // // We can't know whether the attribute is automatically or manually defined for sure, // and while we could check the location, the error seems clear enough as is. - NixpkgsProblem::NonDerivation { - package_name: attribute_name.to_owned(), - } - .into() + to_problem(ByNameErrorKind::NonDerivation).into() } // The attribute exists Existing(AttributeInfo { @@ -259,10 +262,7 @@ fn by_name( let is_derivation_result = if is_derivation { Success(()) } else { - NixpkgsProblem::NonDerivation { - package_name: attribute_name.to_owned(), - } - .into() + to_problem(ByNameErrorKind::NonDerivation).into() }; // If the definition looks correct @@ -274,10 +274,7 @@ fn by_name( if let Some(_location) = location { // Such an automatic definition should definitely not have a location // Having one indicates that somebody is using `_internalCallByNamePackageFile`, - NixpkgsProblem::InternalCallPackageUsed { - attr_name: attribute_name.to_owned(), - } - .into() + to_problem(ByNameErrorKind::InternalCallPackageUsed).into() } else { Success(Tight) } @@ -319,10 +316,7 @@ fn by_name( // If manual definitions don't have a location, it's likely `mapAttrs`'d // over, e.g. if it's defined in aliases.nix. // We can't verify whether its of the expected `callPackage`, so error out - NixpkgsProblem::CannotDetermineAttributeLocation { - attr_name: attribute_name.to_owned(), - } - .into() + to_problem(ByNameErrorKind::CannotDetermineAttributeLocation).into() } } }; diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 46d94a6..e5d6ec4 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -29,13 +29,10 @@ pub enum NixpkgsProblem { PackageNixDir { relative_package_dir: RelativePathBuf, }, - UndefinedAttr { - package_name: String, - }, + + ByNameProblem(ByNameError), + ByNameOverrideProblem(ByNameOverrideError), - NonDerivation { - package_name: String, - }, OutsideSymlink { relative_package_dir: RelativePathBuf, subpath: RelativePathBuf, @@ -47,12 +44,6 @@ pub enum NixpkgsProblem { }, NixFileProblem(NixFileError), RatchetProblem(RatchetError), - InternalCallPackageUsed { - attr_name: String, - }, - CannotDetermineAttributeLocation { - attr_name: String, - }, } #[derive(Clone)] @@ -68,6 +59,20 @@ pub enum ShardErrorKind { CaseSensitiveDuplicate { first: OsString, second: OsString }, } +#[derive(Clone)] +pub struct ByNameError { + pub attribute_name: String, + pub kind: ByNameErrorKind, +} + +#[derive(Clone)] +pub enum ByNameErrorKind { + UndefinedAttr, + NonDerivation, + InternalCallPackageUsed, + CannotDetermineAttributeLocation, +} + #[derive(Clone)] pub struct ByNameOverrideError { pub package_name: String, @@ -176,12 +181,36 @@ impl fmt::Display for NixpkgsProblem { f, "{relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", ), - NixpkgsProblem::UndefinedAttr { package_name } => { - let relative_package_file = structure::relative_file_for_package(package_name); - write!( - f, - "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}", - ) + NixpkgsProblem::ByNameProblem(ByNameError { + attribute_name, + kind, + }) => { + match kind { + ByNameErrorKind::UndefinedAttr => { + let relative_package_file = structure::relative_file_for_package(attribute_name); + write!( + f, + "pkgs.{attribute_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}", + ) + } + ByNameErrorKind::NonDerivation => { + let relative_package_file = structure::relative_file_for_package(attribute_name); + write!( + f, + "pkgs.{attribute_name}: This attribute defined by {relative_package_file} is not a derivation", + ) + } + ByNameErrorKind::InternalCallPackageUsed => + write!( + f, + "pkgs.{attribute_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.", + ), + ByNameErrorKind::CannotDetermineAttributeLocation => + write!( + f, + "pkgs.{attribute_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.", + ), + } } NixpkgsProblem::ByNameOverrideProblem(ByNameOverrideError { package_name, @@ -268,13 +297,6 @@ impl fmt::Display for NixpkgsProblem { ), } }, - NixpkgsProblem::NonDerivation { package_name } => { - let relative_package_file = structure::relative_file_for_package(package_name); - write!( - f, - "pkgs.{package_name}: This attribute defined by {relative_package_file} is not a derivation", - ) - } NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } => write!( f, @@ -370,16 +392,6 @@ impl fmt::Display for NixpkgsProblem { ), } }, - NixpkgsProblem::InternalCallPackageUsed { attr_name } => - write!( - f, - "pkgs.{attr_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.", - ), - NixpkgsProblem::CannotDetermineAttributeLocation { attr_name } => - write!( - f, - "pkgs.{attr_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.", - ), } } } From e5aa63dd5d2d8301662b7fdc2f873c0b760c7d63 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Sat, 23 Mar 2024 20:12:42 -0500 Subject: [PATCH 12/19] Refactor Path related problems into a struct with common fields --- src/nixpkgs_problem.rs | 51 ++++++++++++++++++++++++++---------------- src/references.rs | 23 ++++++++++--------- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index e5d6ec4..9e8553b 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -33,15 +33,7 @@ pub enum NixpkgsProblem { ByNameProblem(ByNameError), ByNameOverrideProblem(ByNameOverrideError), - OutsideSymlink { - relative_package_dir: RelativePathBuf, - subpath: RelativePathBuf, - }, - UnresolvableSymlink { - relative_package_dir: RelativePathBuf, - subpath: RelativePathBuf, - io_error: String, - }, + PathProblem(PathError), NixFileProblem(NixFileError), RatchetProblem(RatchetError), } @@ -95,6 +87,19 @@ pub enum ByNameOverrideErrorKind { NonPath, } +#[derive(Clone)] +pub struct PathError { + pub relative_package_dir: RelativePathBuf, + pub subpath: RelativePathBuf, + pub kind: PathErrorKind, +} + +#[derive(Clone)] +pub enum PathErrorKind { + OutsideSymlink, + UnresolvableSymlink { io_error: String }, +} + #[derive(Clone)] pub struct NixFileError { pub relative_package_dir: RelativePathBuf, @@ -297,16 +302,24 @@ impl fmt::Display for NixpkgsProblem { ), } }, - NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } => - write!( - f, - "{relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.", - ), - NixpkgsProblem::UnresolvableSymlink { relative_package_dir, subpath, io_error } => - write!( - f, - "{relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.", - ), + NixpkgsProblem::PathProblem(PathError { + relative_package_dir, + subpath, + kind, + }) => { + match kind { + PathErrorKind::OutsideSymlink => + write!( + f, + "{relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.", + ), + PathErrorKind::UnresolvableSymlink { io_error } => + write!( + f, + "{relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.", + ), + } + }, NixpkgsProblem::NixFileProblem(NixFileError { relative_package_dir, subpath, diff --git a/src/references.rs b/src/references.rs index 5ebaa8a..e3f3771 100644 --- a/src/references.rs +++ b/src/references.rs @@ -1,4 +1,6 @@ -use crate::nixpkgs_problem::{NixFileError, NixFileErrorKind, NixpkgsProblem}; +use crate::nixpkgs_problem::{ + NixFileError, NixFileErrorKind, NixpkgsProblem, PathError, PathErrorKind, +}; use crate::utils; use crate::validation::{self, ResultIteratorExt, Validation::Success}; use crate::NixFileStore; @@ -47,6 +49,13 @@ fn check_path( subpath: &RelativePath, ) -> validation::Result<()> { let path = subpath.to_path(absolute_package_dir); + let to_problem = |kind| { + NixpkgsProblem::PathProblem(PathError { + relative_package_dir: relative_package_dir.to_owned(), + subpath: subpath.to_owned(), + kind, + }) + }; Ok(if path.is_symlink() { // Check whether the symlink resolves to outside the package directory @@ -55,20 +64,14 @@ fn check_path( // No need to handle the case of it being inside the directory, since we scan through the // entire directory recursively anyways if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { - NixpkgsProblem::OutsideSymlink { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - } - .into() + to_problem(PathErrorKind::OutsideSymlink).into() } else { Success(()) } } - Err(io_error) => NixpkgsProblem::UnresolvableSymlink { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), + Err(io_error) => to_problem(PathErrorKind::UnresolvableSymlink { io_error: io_error.to_string(), - } + }) .into(), } } else if path.is_dir() { From b0d270ee2a30baa5695eb81cced695189131f792 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Sat, 23 Mar 2024 20:45:37 -0500 Subject: [PATCH 13/19] Refactor Package related problems into a struct with common fields Fix clippy warning about same postfix "Problem" in struct names on NixpkgsProblem enum --- src/eval.rs | 4 +- src/nixpkgs_problem.rs | 124 ++++++++++++++++++++++------------------- src/ratchet.rs | 2 +- src/references.rs | 4 +- src/structure.rs | 39 ++++++------- 5 files changed, 92 insertions(+), 81 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 1b63b65..af75da0 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -218,7 +218,7 @@ fn by_name( use ByNameAttribute::*; let to_problem = |kind| { - NixpkgsProblem::ByNameProblem(ByNameError { + NixpkgsProblem::ByName(ByNameError { attribute_name: attribute_name.to_owned(), kind, }) @@ -347,7 +347,7 @@ fn by_name_override( relative_location_file: RelativePathBuf, ) -> validation::Validation> { let to_problem = |kind| { - NixpkgsProblem::ByNameOverrideProblem(ByNameOverrideError { + NixpkgsProblem::ByNameOverride(ByNameOverrideError { package_name: attribute_name.to_owned(), file: relative_location_file, line: location.line, diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 9e8553b..9a3160b 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -11,31 +11,13 @@ use std::fmt; /// location #[derive(Clone)] pub enum NixpkgsProblem { - ShardProblem(ShardError), - PackageNonDir { - package_name: String, - }, - InvalidPackageName { - relative_package_dir: RelativePathBuf, - invalid_package_name: String, - }, - IncorrectShard { - relative_package_dir: RelativePathBuf, - correct_relative_package_dir: RelativePathBuf, - }, - PackageNixNonExistent { - relative_package_dir: RelativePathBuf, - }, - PackageNixDir { - relative_package_dir: RelativePathBuf, - }, - - ByNameProblem(ByNameError), - - ByNameOverrideProblem(ByNameOverrideError), - PathProblem(PathError), - NixFileProblem(NixFileError), - RatchetProblem(RatchetError), + Shard(ShardError), + Package(PackageError), + ByName(ByNameError), + ByNameOverride(ByNameOverrideError), + Path(PathError), + NixFile(NixFileError), + Ratchet(RatchetError), } #[derive(Clone)] @@ -51,6 +33,27 @@ pub enum ShardErrorKind { CaseSensitiveDuplicate { first: OsString, second: OsString }, } +#[derive(Clone)] +pub struct PackageError { + pub relative_package_dir: RelativePathBuf, + pub kind: PackageErrorKind, +} + +#[derive(Clone)] +pub enum PackageErrorKind { + PackageNonDir { + package_name: String, + }, + InvalidPackageName { + invalid_package_name: String, + }, + IncorrectShard { + correct_relative_package_dir: RelativePathBuf, + }, + PackageNixNonExistent, + PackageNixDir, +} + #[derive(Clone)] pub struct ByNameError { pub attribute_name: String, @@ -136,7 +139,7 @@ pub enum RatchetErrorKind { impl fmt::Display for NixpkgsProblem { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - NixpkgsProblem::ShardProblem(ShardError { + NixpkgsProblem::Shard(ShardError { shard_name, kind, }) => { @@ -159,34 +162,41 @@ impl fmt::Display for NixpkgsProblem { ), } } - NixpkgsProblem::PackageNonDir { package_name } => { - let relative_package_dir = structure::relative_dir_for_package(package_name); - write!( - f, - "{relative_package_dir}: This path is a file, but it should be a directory.", - ) + NixpkgsProblem::Package(PackageError { + relative_package_dir, + kind, + }) => { + match kind { + PackageErrorKind::PackageNonDir { package_name } => { + let relative_package_dir = structure::relative_dir_for_package(package_name); + write!( + f, + "{relative_package_dir}: This path is a file, but it should be a directory.", + ) + } + PackageErrorKind::InvalidPackageName { invalid_package_name } => + write!( + f, + "{relative_package_dir}: Invalid package directory name \"{invalid_package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", + ), + PackageErrorKind::IncorrectShard { correct_relative_package_dir } => + write!( + f, + "{relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.", + ), + PackageErrorKind::PackageNixNonExistent => + write!( + f, + "{relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", + ), + PackageErrorKind::PackageNixDir => + write!( + f, + "{relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", + ), + } } - NixpkgsProblem::InvalidPackageName { relative_package_dir, invalid_package_name } => - write!( - f, - "{relative_package_dir}: Invalid package directory name \"{invalid_package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".", - ), - NixpkgsProblem::IncorrectShard { relative_package_dir, correct_relative_package_dir } => - write!( - f, - "{relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.", - ), - NixpkgsProblem::PackageNixNonExistent { relative_package_dir } => - write!( - f, - "{relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.", - ), - NixpkgsProblem::PackageNixDir { relative_package_dir } => - write!( - f, - "{relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.", - ), - NixpkgsProblem::ByNameProblem(ByNameError { + NixpkgsProblem::ByName(ByNameError { attribute_name, kind, }) => { @@ -217,7 +227,7 @@ impl fmt::Display for NixpkgsProblem { ), } } - NixpkgsProblem::ByNameOverrideProblem(ByNameOverrideError { + NixpkgsProblem::ByNameOverride(ByNameOverrideError { package_name, file, line, @@ -302,7 +312,7 @@ impl fmt::Display for NixpkgsProblem { ), } }, - NixpkgsProblem::PathProblem(PathError { + NixpkgsProblem::Path(PathError { relative_package_dir, subpath, kind, @@ -320,7 +330,7 @@ impl fmt::Display for NixpkgsProblem { ), } }, - NixpkgsProblem::NixFileProblem(NixFileError { + NixpkgsProblem::NixFile(NixFileError { relative_package_dir, subpath, line, @@ -350,7 +360,7 @@ impl fmt::Display for NixpkgsProblem { ), } }, - NixpkgsProblem::RatchetProblem(RatchetError { + NixpkgsProblem::Ratchet(RatchetError { package_name, call_package_path, file, diff --git a/src/ratchet.rs b/src/ratchet.rs index d00fba4..64f5e46 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -153,7 +153,7 @@ impl ToNixpkgsProblem for UsesByName { optional_from: Option<()>, (to, file): &Self::ToContext, ) -> NixpkgsProblem { - NixpkgsProblem::RatchetProblem(RatchetError { + NixpkgsProblem::Ratchet(RatchetError { package_name: name.to_owned(), call_package_path: to.relative_path.clone(), file: file.to_owned(), diff --git a/src/references.rs b/src/references.rs index e3f3771..2ffaa34 100644 --- a/src/references.rs +++ b/src/references.rs @@ -50,7 +50,7 @@ fn check_path( ) -> validation::Result<()> { let path = subpath.to_path(absolute_package_dir); let to_problem = |kind| { - NixpkgsProblem::PathProblem(PathError { + NixpkgsProblem::Path(PathError { relative_package_dir: relative_package_dir.to_owned(), subpath: subpath.to_owned(), kind, @@ -134,7 +134,7 @@ fn check_nix_file( }; let to_problem = |kind| { - NixpkgsProblem::NixFileProblem(NixFileError { + NixpkgsProblem::NixFile(NixFileError { relative_package_dir: relative_package_dir.to_owned(), subpath: subpath.to_owned(), line: nix_file.line_index.line(node.text_range().start().into()), diff --git a/src/structure.rs b/src/structure.rs index 5198898..a1bd43e 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -1,4 +1,6 @@ use crate::nixpkgs_problem::NixpkgsProblem; +use crate::nixpkgs_problem::PackageError; +use crate::nixpkgs_problem::PackageErrorKind; use crate::nixpkgs_problem::ShardError; use crate::nixpkgs_problem::ShardErrorKind; use crate::references; @@ -55,7 +57,7 @@ pub fn check_structure( Success(vec![]) } else if !shard_path.is_dir() { - NixpkgsProblem::ShardProblem(ShardError { + NixpkgsProblem::Shard(ShardError { shard_name: shard_name.clone(), kind: ShardErrorKind::ShardNonDir, }) @@ -64,7 +66,7 @@ pub fn check_structure( } else { let shard_name_valid = SHARD_NAME_REGEX.is_match(&shard_name); let result = if !shard_name_valid { - NixpkgsProblem::ShardProblem(ShardError { + NixpkgsProblem::Shard(ShardError { shard_name: shard_name.clone(), kind: ShardErrorKind::InvalidShardName, }) @@ -82,7 +84,7 @@ pub fn check_structure( l.file_name().to_ascii_lowercase() == r.file_name().to_ascii_lowercase() }) .map(|(l, r)| { - NixpkgsProblem::ShardProblem(ShardError { + NixpkgsProblem::Shard(ShardError { shard_name: shard_name.clone(), kind: ShardErrorKind::CaseSensitiveDuplicate { first: l.file_name(), @@ -128,18 +130,24 @@ fn check_package( let relative_package_dir = RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); + let to_problem = |kind| { + NixpkgsProblem::Package(PackageError { + relative_package_dir: relative_package_dir.clone(), + kind, + }) + }; + Ok(if !package_path.is_dir() { - NixpkgsProblem::PackageNonDir { + to_problem(PackageErrorKind::PackageNonDir { package_name: package_name.clone(), - } + }) .into() } else { let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); let result = if !package_name_valid { - NixpkgsProblem::InvalidPackageName { - relative_package_dir: relative_package_dir.clone(), + to_problem(PackageErrorKind::InvalidPackageName { invalid_package_name: package_name.clone(), - } + }) .into() } else { Success(()) @@ -150,10 +158,9 @@ fn check_package( // Only show this error if we have a valid shard and package name // Because if one of those is wrong, you should fix that first if shard_name_valid && package_name_valid { - NixpkgsProblem::IncorrectShard { - relative_package_dir: relative_package_dir.clone(), + to_problem(PackageErrorKind::IncorrectShard { correct_relative_package_dir: correct_relative_package_dir.clone(), - } + }) .into() } else { Success(()) @@ -164,15 +171,9 @@ fn check_package( let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); let result = result.and(if !package_nix_path.exists() { - NixpkgsProblem::PackageNixNonExistent { - relative_package_dir: relative_package_dir.clone(), - } - .into() + to_problem(PackageErrorKind::PackageNixNonExistent).into() } else if package_nix_path.is_dir() { - NixpkgsProblem::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into() + to_problem(PackageErrorKind::PackageNixDir).into() } else { Success(()) }); From 81e45fa837d02815577bb3eb1bd2e73e76ad5428 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Sat, 23 Mar 2024 21:13:48 -0500 Subject: [PATCH 14/19] Refactor some nixpkgs_problem imports --- src/eval.rs | 8 +++----- src/structure.rs | 8 +++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index af75da0..a8e4835 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -1,9 +1,7 @@ use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::ByNameError; -use crate::nixpkgs_problem::ByNameErrorKind; -use crate::nixpkgs_problem::ByNameOverrideError; -use crate::nixpkgs_problem::ByNameOverrideErrorKind; -use crate::nixpkgs_problem::NixpkgsProblem; +use crate::nixpkgs_problem::{ + ByNameError, ByNameErrorKind, ByNameOverrideError, ByNameOverrideErrorKind, NixpkgsProblem, +}; use crate::ratchet; use crate::ratchet::RatchetState::Loose; use crate::ratchet::RatchetState::Tight; diff --git a/src/structure.rs b/src/structure.rs index a1bd43e..c0da10f 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -1,8 +1,6 @@ -use crate::nixpkgs_problem::NixpkgsProblem; -use crate::nixpkgs_problem::PackageError; -use crate::nixpkgs_problem::PackageErrorKind; -use crate::nixpkgs_problem::ShardError; -use crate::nixpkgs_problem::ShardErrorKind; +use crate::nixpkgs_problem::{ + NixpkgsProblem, PackageError, PackageErrorKind, ShardError, ShardErrorKind, +}; use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; From 25adca4ec059eed89b5e0064ec68566565278ddb Mon Sep 17 00:00:00 2001 From: Will Bush Date: Mon, 25 Mar 2024 19:45:20 -0500 Subject: [PATCH 15/19] Fix clone of `node` by moving variables above usage of `cast` --- src/references.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/references.rs b/src/references.rs index 2ffaa34..03af531 100644 --- a/src/references.rs +++ b/src/references.rs @@ -128,8 +128,11 @@ fn check_nix_file( Ok(validation::sequence_( nix_file.syntax_root.syntax().descendants().map(|node| { + let line = nix_file.line_index.line(node.text_range().start().into()); + let text = node.text().to_string(); + // We're only interested in Path expressions - let Some(path) = rnix::ast::Path::cast(node.clone()) else { + let Some(path) = rnix::ast::Path::cast(node) else { return Success(()); }; @@ -137,8 +140,8 @@ fn check_nix_file( NixpkgsProblem::NixFile(NixFileError { relative_package_dir: relative_package_dir.to_owned(), subpath: subpath.to_owned(), - line: nix_file.line_index.line(node.text_range().start().into()), - text: node.text().to_string(), + line, + text, kind, }) }; From 2ed5984984b956a9562848f3117427f4ca2aec81 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Mon, 25 Mar 2024 20:39:29 -0500 Subject: [PATCH 16/19] Change most to_problem closures into to_validation To remove duplicate `.into()` calls --- src/eval.rs | 18 +++++++++--------- src/references.rs | 24 +++++++++++------------- src/structure.rs | 19 +++++++++---------- 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index a8e4835..37a1cb1 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -2,9 +2,8 @@ use crate::nix_file::CallPackageArgumentInfo; use crate::nixpkgs_problem::{ ByNameError, ByNameErrorKind, ByNameOverrideError, ByNameOverrideErrorKind, NixpkgsProblem, }; -use crate::ratchet; -use crate::ratchet::RatchetState::Loose; -use crate::ratchet::RatchetState::Tight; +use crate::ratchet::RatchetState::{Loose, Tight}; +use crate::ratchet::{self, ManualDefinition, RatchetState}; use crate::structure; use crate::utils; use crate::validation::ResultIteratorExt as _; @@ -215,11 +214,12 @@ fn by_name( use ratchet::RatchetState::*; use ByNameAttribute::*; - let to_problem = |kind| { + let to_validation = |kind| -> validation::Validation> { NixpkgsProblem::ByName(ByNameError { attribute_name: attribute_name.to_owned(), kind, }) + .into() }; // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists. @@ -230,7 +230,7 @@ fn by_name( Missing => { // This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to // automatically defined attributes in `pkgs/by-name` - to_problem(ByNameErrorKind::UndefinedAttr).into() + to_validation(ByNameErrorKind::UndefinedAttr) } // The attribute exists Existing(AttributeInfo { @@ -244,7 +244,7 @@ fn by_name( // // We can't know whether the attribute is automatically or manually defined for sure, // and while we could check the location, the error seems clear enough as is. - to_problem(ByNameErrorKind::NonDerivation).into() + to_validation(ByNameErrorKind::NonDerivation) } // The attribute exists Existing(AttributeInfo { @@ -260,7 +260,7 @@ fn by_name( let is_derivation_result = if is_derivation { Success(()) } else { - to_problem(ByNameErrorKind::NonDerivation).into() + to_validation(ByNameErrorKind::NonDerivation).map(|_| ()) }; // If the definition looks correct @@ -272,7 +272,7 @@ fn by_name( if let Some(_location) = location { // Such an automatic definition should definitely not have a location // Having one indicates that somebody is using `_internalCallByNamePackageFile`, - to_problem(ByNameErrorKind::InternalCallPackageUsed).into() + to_validation(ByNameErrorKind::InternalCallPackageUsed) } else { Success(Tight) } @@ -314,7 +314,7 @@ fn by_name( // If manual definitions don't have a location, it's likely `mapAttrs`'d // over, e.g. if it's defined in aliases.nix. // We can't verify whether its of the expected `callPackage`, so error out - to_problem(ByNameErrorKind::CannotDetermineAttributeLocation).into() + to_validation(ByNameErrorKind::CannotDetermineAttributeLocation) } } }; diff --git a/src/references.rs b/src/references.rs index 03af531..9a50d19 100644 --- a/src/references.rs +++ b/src/references.rs @@ -49,12 +49,13 @@ fn check_path( subpath: &RelativePath, ) -> validation::Result<()> { let path = subpath.to_path(absolute_package_dir); - let to_problem = |kind| { + let to_validation = |kind| -> validation::Validation<()> { NixpkgsProblem::Path(PathError { relative_package_dir: relative_package_dir.to_owned(), subpath: subpath.to_owned(), kind, }) + .into() }; Ok(if path.is_symlink() { @@ -64,15 +65,14 @@ fn check_path( // No need to handle the case of it being inside the directory, since we scan through the // entire directory recursively anyways if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) { - to_problem(PathErrorKind::OutsideSymlink).into() + to_validation(PathErrorKind::OutsideSymlink) } else { Success(()) } } - Err(io_error) => to_problem(PathErrorKind::UnresolvableSymlink { + Err(io_error) => to_validation(PathErrorKind::UnresolvableSymlink { io_error: io_error.to_string(), - }) - .into(), + }), } } else if path.is_dir() { // Recursively check each entry @@ -136,7 +136,7 @@ fn check_nix_file( return Success(()); }; - let to_problem = |kind| { + let to_validation = |kind| -> validation::Validation<()> { NixpkgsProblem::NixFile(NixFileError { relative_package_dir: relative_package_dir.to_owned(), subpath: subpath.to_owned(), @@ -144,21 +144,19 @@ fn check_nix_file( text, kind, }) + .into() }; use crate::nix_file::ResolvedPath; match nix_file.static_resolve_path(path, absolute_package_dir) { - ResolvedPath::Interpolated => { - to_problem(NixFileErrorKind::PathInterpolation).into() - } - ResolvedPath::SearchPath => to_problem(NixFileErrorKind::SearchPath).into(), - ResolvedPath::Outside => to_problem(NixFileErrorKind::OutsidePathReference).into(), + ResolvedPath::Interpolated => to_validation(NixFileErrorKind::PathInterpolation), + ResolvedPath::SearchPath => to_validation(NixFileErrorKind::SearchPath), + ResolvedPath::Outside => to_validation(NixFileErrorKind::OutsidePathReference), ResolvedPath::Unresolvable(e) => { - to_problem(NixFileErrorKind::UnresolvablePathReference { + to_validation(NixFileErrorKind::UnresolvablePathReference { io_error: e.to_string(), }) - .into() } ResolvedPath::Within(..) => { // No need to handle the case of it being inside the directory, since we scan through the diff --git a/src/structure.rs b/src/structure.rs index c0da10f..fea8a85 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -128,25 +128,25 @@ fn check_package( let relative_package_dir = RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); - let to_problem = |kind| { + let to_validation = |kind| -> validation::Validation<()> { NixpkgsProblem::Package(PackageError { relative_package_dir: relative_package_dir.clone(), kind, }) + .into() }; Ok(if !package_path.is_dir() { - to_problem(PackageErrorKind::PackageNonDir { + to_validation(PackageErrorKind::PackageNonDir { package_name: package_name.clone(), }) - .into() + .map(|_| package_name) } else { let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name); let result = if !package_name_valid { - to_problem(PackageErrorKind::InvalidPackageName { + to_validation(PackageErrorKind::InvalidPackageName { invalid_package_name: package_name.clone(), }) - .into() } else { Success(()) }; @@ -156,10 +156,9 @@ fn check_package( // Only show this error if we have a valid shard and package name // Because if one of those is wrong, you should fix that first if shard_name_valid && package_name_valid { - to_problem(PackageErrorKind::IncorrectShard { + to_validation(PackageErrorKind::IncorrectShard { correct_relative_package_dir: correct_relative_package_dir.clone(), }) - .into() } else { Success(()) } @@ -169,9 +168,9 @@ fn check_package( let package_nix_path = package_path.join(PACKAGE_NIX_FILENAME); let result = result.and(if !package_nix_path.exists() { - to_problem(PackageErrorKind::PackageNixNonExistent).into() + to_validation(PackageErrorKind::PackageNixNonExistent) } else if package_nix_path.is_dir() { - to_problem(PackageErrorKind::PackageNixDir).into() + to_validation(PackageErrorKind::PackageNixDir) } else { Success(()) }); @@ -182,6 +181,6 @@ fn check_package( &relative_package_dir.to_path(path), )?); - result.map(|_| package_name.clone()) + result.map(|_| package_name) }) } From c57206d1f690264c8d871e00b8798be5cb5d7a20 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Mon, 25 Mar 2024 21:05:02 -0500 Subject: [PATCH 17/19] Replace RatchetErrorKind with is_new and is_empty bools --- src/nixpkgs_problem.rs | 24 +++++++++--------------- src/ratchet.rs | 10 +++------- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index 9a3160b..b7dcd5a 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -125,15 +125,8 @@ pub struct RatchetError { pub package_name: String, pub call_package_path: Option, pub file: RelativePathBuf, - pub kind: RatchetErrorKind, -} - -#[derive(Clone)] -pub enum RatchetErrorKind { - MovedOutOfByName, - MovedOutOfByNameEmptyArg, - NewPackageNotUsingByName, - NewPackageNotUsingByNameEmptyArg, + pub is_new: bool, + pub is_empty: bool, } impl fmt::Display for NixpkgsProblem { @@ -364,7 +357,8 @@ impl fmt::Display for NixpkgsProblem { package_name, call_package_path, file, - kind, + is_new, + is_empty, }) => { let call_package_arg = if let Some(path) = &call_package_path { @@ -374,8 +368,8 @@ impl fmt::Display for NixpkgsProblem { }; let relative_package_file = structure::relative_file_for_package(package_name); - match kind { - RatchetErrorKind::MovedOutOfByNameEmptyArg => + match (is_new, is_empty) { + (false, true) => writedoc!( f, " @@ -383,7 +377,7 @@ impl fmt::Display for NixpkgsProblem { Please move the package back and remove the manual `callPackage`. ", ), - RatchetErrorKind::MovedOutOfByName => + (false, false) => // This can happen if users mistakenly assume that for custom arguments, // pkgs/by-name can't be used. writedoc!( @@ -393,7 +387,7 @@ impl fmt::Display for NixpkgsProblem { While the manual `callPackage` is still needed, it's not necessary to move the package files. ", ), - RatchetErrorKind::NewPackageNotUsingByNameEmptyArg => + (true, true) => writedoc!( f, " @@ -403,7 +397,7 @@ impl fmt::Display for NixpkgsProblem { Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore. ", ), - RatchetErrorKind::NewPackageNotUsingByName => + (true, false) => writedoc!( f, " diff --git a/src/ratchet.rs b/src/ratchet.rs index 64f5e46..06c1c58 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -3,7 +3,7 @@ //! Each type has a `compare` method that validates the ratchet checks for that item. use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::{NixpkgsProblem, RatchetError, RatchetErrorKind}; +use crate::nixpkgs_problem::{NixpkgsProblem, RatchetError}; use crate::validation::{self, Validation, Validation::Success}; use relative_path::RelativePathBuf; use std::collections::HashMap; @@ -157,12 +157,8 @@ impl ToNixpkgsProblem for UsesByName { package_name: name.to_owned(), call_package_path: to.relative_path.clone(), file: file.to_owned(), - kind: match (optional_from, to.empty_arg) { - (Some(()), true) => RatchetErrorKind::MovedOutOfByNameEmptyArg, - (Some(()), false) => RatchetErrorKind::MovedOutOfByName, - (None, true) => RatchetErrorKind::NewPackageNotUsingByNameEmptyArg, - (None, false) => RatchetErrorKind::NewPackageNotUsingByName, - }, + is_new: optional_from.is_none(), + is_empty: to.empty_arg, }) } } From bcc472db5b641f2dd51f75dea52ceb7e1e2d0822 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Mon, 25 Mar 2024 21:09:20 -0500 Subject: [PATCH 18/19] Rename RatchetError to TopLevelPackageError --- src/nixpkgs_problem.rs | 7 ++++--- src/ratchet.rs | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index b7dcd5a..e0ee5b8 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -17,7 +17,7 @@ pub enum NixpkgsProblem { ByNameOverride(ByNameOverrideError), Path(PathError), NixFile(NixFileError), - Ratchet(RatchetError), + TopLevelPackage(TopLevelPackageError), } #[derive(Clone)] @@ -120,8 +120,9 @@ pub enum NixFileErrorKind { UnresolvablePathReference { io_error: String }, } +/// An error related to the introduction/move of a top-level package not using `pkgs/by-name`, but it should #[derive(Clone)] -pub struct RatchetError { +pub struct TopLevelPackageError { pub package_name: String, pub call_package_path: Option, pub file: RelativePathBuf, @@ -353,7 +354,7 @@ impl fmt::Display for NixpkgsProblem { ), } }, - NixpkgsProblem::Ratchet(RatchetError { + NixpkgsProblem::TopLevelPackage(TopLevelPackageError { package_name, call_package_path, file, diff --git a/src/ratchet.rs b/src/ratchet.rs index 06c1c58..e7cb57e 100644 --- a/src/ratchet.rs +++ b/src/ratchet.rs @@ -3,7 +3,7 @@ //! Each type has a `compare` method that validates the ratchet checks for that item. use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::{NixpkgsProblem, RatchetError}; +use crate::nixpkgs_problem::{NixpkgsProblem, TopLevelPackageError}; use crate::validation::{self, Validation, Validation::Success}; use relative_path::RelativePathBuf; use std::collections::HashMap; @@ -153,7 +153,7 @@ impl ToNixpkgsProblem for UsesByName { optional_from: Option<()>, (to, file): &Self::ToContext, ) -> NixpkgsProblem { - NixpkgsProblem::Ratchet(RatchetError { + NixpkgsProblem::TopLevelPackage(TopLevelPackageError { package_name: name.to_owned(), call_package_path: to.relative_path.clone(), file: file.to_owned(), From 591be8ed6c465539b37cf7c05e338479f88f34b4 Mon Sep 17 00:00:00 2001 From: Will Bush Date: Mon, 25 Mar 2024 23:09:42 -0500 Subject: [PATCH 19/19] Add comments to NixpkgsProblem struct sub-types --- src/nixpkgs_problem.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/nixpkgs_problem.rs b/src/nixpkgs_problem.rs index e0ee5b8..3f1e942 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -20,6 +20,7 @@ pub enum NixpkgsProblem { TopLevelPackage(TopLevelPackageError), } +/// A file structure error involving a shard (e.g. `fo` is the shard in the path `pkgs/by-name/fo/foo/package.nix`) #[derive(Clone)] pub struct ShardError { pub shard_name: String, @@ -33,6 +34,7 @@ pub enum ShardErrorKind { CaseSensitiveDuplicate { first: OsString, second: OsString }, } +/// A file structure error involving the package name and/or path. #[derive(Clone)] pub struct PackageError { pub relative_package_dir: RelativePathBuf, @@ -54,6 +56,8 @@ pub enum PackageErrorKind { PackageNixDir, } +/// An error related to checks involving by-name attributes. For example, attribute `foo` in +/// `pkgs/by-name/fo/foo/package.nix`. #[derive(Clone)] pub struct ByNameError { pub attribute_name: String, @@ -68,6 +72,8 @@ pub enum ByNameErrorKind { CannotDetermineAttributeLocation, } +/// An error related to packages in `pkgs/by-name` that are manually overridden, e.g. in +/// all-packages.nix #[derive(Clone)] pub struct ByNameOverrideError { pub package_name: String, @@ -90,6 +96,8 @@ pub enum ByNameOverrideErrorKind { NonPath, } +/// An error that results from checks that verify a specific path does not reference outside the +/// package directory. #[derive(Clone)] pub struct PathError { pub relative_package_dir: RelativePathBuf, @@ -103,6 +111,8 @@ pub enum PathErrorKind { UnresolvableSymlink { io_error: String }, } +/// An error that results from checks that verify a nix file that contains a path expression does +/// not reference outside the package. #[derive(Clone)] pub struct NixFileError { pub relative_package_dir: RelativePathBuf, @@ -120,7 +130,8 @@ pub enum NixFileErrorKind { UnresolvablePathReference { io_error: String }, } -/// An error related to the introduction/move of a top-level package not using `pkgs/by-name`, but it should +/// An error related to the introduction/move of a top-level package not using `pkgs/by-name`, but +/// it should. #[derive(Clone)] pub struct TopLevelPackageError { pub package_name: String,