diff --git a/src/eval.rs b/src/eval.rs index 094508f..37a1cb1 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -1,8 +1,9 @@ use crate::nix_file::CallPackageArgumentInfo; -use crate::nixpkgs_problem::NixpkgsProblem; -use crate::ratchet; -use crate::ratchet::RatchetState::Loose; -use crate::ratchet::RatchetState::Tight; +use crate::nixpkgs_problem::{ + ByNameError, ByNameErrorKind, ByNameOverrideError, ByNameOverrideErrorKind, NixpkgsProblem, +}; +use crate::ratchet::RatchetState::{Loose, Tight}; +use crate::ratchet::{self, ManualDefinition, RatchetState}; use crate::structure; use crate::utils; use crate::validation::ResultIteratorExt as _; @@ -213,7 +214,13 @@ fn by_name( use ratchet::RatchetState::*; use ByNameAttribute::*; - let relative_package_file = structure::relative_file_for_package(attribute_name); + 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. // This match decides whether the attribute `foo` is defined accordingly @@ -223,11 +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` - NixpkgsProblem::UndefinedAttr { - relative_package_file: relative_package_file.to_owned(), - package_name: attribute_name.to_owned(), - } - .into() + to_validation(ByNameErrorKind::UndefinedAttr) } // The attribute exists Existing(AttributeInfo { @@ -241,11 +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. - NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.to_owned(), - package_name: attribute_name.to_owned(), - } - .into() + to_validation(ByNameErrorKind::NonDerivation) } // The attribute exists Existing(AttributeInfo { @@ -261,11 +260,7 @@ fn by_name( let is_derivation_result = if is_derivation { Success(()) } else { - NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.to_owned(), - package_name: attribute_name.to_owned(), - } - .into() + to_validation(ByNameErrorKind::NonDerivation).map(|_| ()) }; // If the definition looks correct @@ -277,10 +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`, - NixpkgsProblem::InternalCallPackageUsed { - attr_name: attribute_name.to_owned(), - } - .into() + to_validation(ByNameErrorKind::InternalCallPackageUsed) } else { Success(Tight) } @@ -312,7 +304,6 @@ fn by_name( by_name_override( attribute_name, - relative_package_file, is_semantic_call_package, optional_syntactic_call_package, definition, @@ -323,10 +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 - NixpkgsProblem::CannotDetermineAttributeLocation { - attr_name: attribute_name.to_owned(), - } - .into() + to_validation(ByNameErrorKind::CannotDetermineAttributeLocation) } } }; @@ -350,59 +338,48 @@ 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, 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::ByNameOverride(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 { + let expected_package_file = structure::relative_file_for_package(attribute_name); + 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 @@ -412,14 +389,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 7e257c0..3f1e942 100644 --- a/src/nixpkgs_problem.rs +++ b/src/nixpkgs_problem.rs @@ -11,363 +11,367 @@ use std::fmt; /// location #[derive(Clone)] pub enum NixpkgsProblem { - ShardNonDir { - relative_shard_path: RelativePathBuf, - }, - InvalidShardName { - relative_shard_path: RelativePathBuf, - shard_name: String, - }, + Shard(ShardError), + Package(PackageError), + ByName(ByNameError), + ByNameOverride(ByNameOverrideError), + Path(PathError), + NixFile(NixFileError), + 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, + pub kind: ShardErrorKind, +} + +#[derive(Clone)] +pub enum ShardErrorKind { + ShardNonDir, + InvalidShardName, + 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, + pub kind: PackageErrorKind, +} + +#[derive(Clone)] +pub enum PackageErrorKind { PackageNonDir { - relative_package_dir: RelativePathBuf, - }, - CaseSensitiveDuplicate { - relative_shard_path: RelativePathBuf, - first: OsString, - second: OsString, + package_name: String, }, InvalidPackageName { - relative_package_dir: RelativePathBuf, - package_name: String, + invalid_package_name: String, }, IncorrectShard { - relative_package_dir: RelativePathBuf, correct_relative_package_dir: RelativePathBuf, }, - PackageNixNonExistent { - relative_package_dir: RelativePathBuf, - }, - PackageNixDir { - relative_package_dir: RelativePathBuf, - }, - UndefinedAttr { - relative_package_file: RelativePathBuf, - 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, - }, + PackageNixNonExistent, + 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, + pub kind: ByNameErrorKind, +} + +#[derive(Clone)] +pub enum ByNameErrorKind { + UndefinedAttr, + NonDerivation, + InternalCallPackageUsed, + 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, + pub file: RelativePathBuf, + pub line: usize, + pub column: usize, + pub definition: String, + pub kind: ByNameOverrideErrorKind, +} + +#[derive(Clone)] +pub enum ByNameOverrideErrorKind { + NonSyntacticCallPackage, + NonToplevelCallPackage, 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, - }, - NonDerivation { - relative_package_file: RelativePathBuf, - package_name: String, - }, - OutsideSymlink { - relative_package_dir: RelativePathBuf, - subpath: RelativePathBuf, - }, - UnresolvableSymlink { - relative_package_dir: RelativePathBuf, - 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, - }, - 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, - }, - InternalCallPackageUsed { - attr_name: String, - }, - CannotDetermineAttributeLocation { - attr_name: String, - }, + EmptyArgument, + 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, + pub subpath: RelativePathBuf, + pub kind: PathErrorKind, +} + +#[derive(Clone)] +pub enum PathErrorKind { + OutsideSymlink, + 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, + pub subpath: RelativePathBuf, + pub line: usize, + pub text: String, + pub kind: NixFileErrorKind, +} + +#[derive(Clone)] +pub enum NixFileErrorKind { + PathInterpolation, + SearchPath, + OutsidePathReference, + 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 TopLevelPackageError { + pub package_name: String, + pub call_package_path: Option, + pub file: RelativePathBuf, + pub is_new: bool, + pub is_empty: bool, } 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::PackageNonDir { relative_package_dir } => - 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, - "{relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.", - ), - NixpkgsProblem::InvalidPackageName { relative_package_dir, 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 \"_\".", - ), - 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::UndefinedAttr { relative_package_file, 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 } => { + NixpkgsProblem::Shard(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::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::ByName(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::ByNameOverride(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} - {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; + 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 - However, in this PR, it isn't defined that way. See the definition in {file}:{line} + {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }}; - {indented_definition} - ", - ) - } - NixpkgsProblem::NonDerivation { relative_package_file, 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, - "{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::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::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`. - ", - ) + However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}: + + {indented_definition} + ", + ), + } }, - NixpkgsProblem::MovedOutOfByNameNonEmptyArg { 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); - // 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::Path(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::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::NixFile(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::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => { + NixpkgsProblem::TopLevelPackage(TopLevelPackageError { + package_name, + call_package_path, + file, + is_new, + is_empty, + }) => { let call_package_arg = if let Some(path) = &call_package_path { format!("./{}", path) @@ -375,26 +379,48 @@ impl fmt::Display for NixpkgsProblem { "...".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 (is_new, is_empty) { + (false, true) => + 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`. + ", + ), + (false, false) => + // 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. + ", + ), + (true, true) => + 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. + ", + ), + (true, false) => + 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!( - 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`.", - ), } } } diff --git a/src/ratchet.rs b/src/ratchet.rs index 8136d64..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; +use crate::nixpkgs_problem::{NixpkgsProblem, TopLevelPackageError}; use crate::validation::{self, Validation, Validation::Success}; use relative_path::RelativePathBuf; use std::collections::HashMap; @@ -153,32 +153,12 @@ 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::TopLevelPackage(TopLevelPackageError { + package_name: name.to_owned(), + call_package_path: to.relative_path.clone(), + file: file.to_owned(), + is_new: optional_from.is_none(), + is_empty: to.empty_arg, + }) } } diff --git a/src/references.rs b/src/references.rs index e231916..9a50d19 100644 --- a/src/references.rs +++ b/src/references.rs @@ -1,4 +1,6 @@ -use crate::nixpkgs_problem::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,14 @@ fn check_path( subpath: &RelativePath, ) -> validation::Result<()> { let path = subpath.to_path(absolute_package_dir); + 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() { // Check whether the symlink resolves to outside the package directory @@ -55,21 +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) { - NixpkgsProblem::OutsideSymlink { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), - } - .into() + to_validation(PathErrorKind::OutsideSymlink) } else { Success(()) } } - Err(io_error) => NixpkgsProblem::UnresolvableSymlink { - relative_package_dir: relative_package_dir.to_owned(), - subpath: subpath.to_owned(), + Err(io_error) => to_validation(PathErrorKind::UnresolvableSymlink { io_error: io_error.to_string(), - } - .into(), + }), } } else if path.is_dir() { // Recursively check each entry @@ -125,46 +128,36 @@ 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()); + let text = node.text().to_string(); // We're only interested in Path expressions let Some(path) = rnix::ast::Path::cast(node) 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 { + let to_validation = |kind| -> validation::Validation<()> { + NixpkgsProblem::NixFile(NixFileError { relative_package_dir: relative_package_dir.to_owned(), subpath: subpath.to_owned(), line, text, + kind, + }) + .into() + }; + + use crate::nix_file::ResolvedPath; + + match nix_file.static_resolve_path(path, absolute_package_dir) { + ResolvedPath::Interpolated => to_validation(NixFileErrorKind::PathInterpolation), + ResolvedPath::SearchPath => to_validation(NixFileErrorKind::SearchPath), + ResolvedPath::Outside => to_validation(NixFileErrorKind::OutsidePathReference), + ResolvedPath::Unresolvable(e) => { + to_validation(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 diff --git a/src/structure.rs b/src/structure.rs index 09806bc..fea8a85 100644 --- a/src/structure.rs +++ b/src/structure.rs @@ -1,4 +1,6 @@ -use crate::nixpkgs_problem::NixpkgsProblem; +use crate::nixpkgs_problem::{ + NixpkgsProblem, PackageError, PackageErrorKind, ShardError, 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::Shard(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::Shard(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::Shard(ShardError { + shard_name: shard_name.clone(), + kind: ShardErrorKind::CaseSensitiveDuplicate { + first: l.file_name(), + second: r.file_name(), + }, + }) .into() }); @@ -124,19 +128,25 @@ fn check_package( let relative_package_dir = RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}")); - Ok(if !package_path.is_dir() { - NixpkgsProblem::PackageNonDir { + 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_validation(PackageErrorKind::PackageNonDir { + package_name: package_name.clone(), + }) + .map(|_| package_name) } 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(), - package_name: package_name.clone(), - } - .into() + to_validation(PackageErrorKind::InvalidPackageName { + invalid_package_name: package_name.clone(), + }) } else { Success(()) }; @@ -146,11 +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 { - NixpkgsProblem::IncorrectShard { - relative_package_dir: relative_package_dir.clone(), + to_validation(PackageErrorKind::IncorrectShard { correct_relative_package_dir: correct_relative_package_dir.clone(), - } - .into() + }) } else { Success(()) } @@ -160,15 +168,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_validation(PackageErrorKind::PackageNixNonExistent) } else if package_nix_path.is_dir() { - NixpkgsProblem::PackageNixDir { - relative_package_dir: relative_package_dir.clone(), - } - .into() + to_validation(PackageErrorKind::PackageNixDir) } else { Success(()) }); @@ -179,6 +181,6 @@ fn check_package( &relative_package_dir.to_path(path), )?); - result.map(|_| package_name.clone()) + result.map(|_| package_name) }) }