Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Error Types #24

Merged
merged 19 commits into from
Mar 26, 2024
Merged
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a169008
Remove superfluous error field relative_package_file
willbush Mar 22, 2024
d12e15a
Rename package_name field in InvalidPackageName error
willbush Mar 22, 2024
0bb6405
Derive relative_package_dir from the package name for PackageNonDir
willbush Mar 22, 2024
f8bdeab
Refactor ratchet problems into a struct with common fields
willbush Mar 22, 2024
5c502a2
Remove unnecessary braces
willbush Mar 23, 2024
9391e85
Group shard problems into a struct for shared context
willbush Mar 23, 2024
686cbbc
Fix clippy warnings
willbush Mar 23, 2024
94244b0
Refactor nix file problems into a struct with common fields
willbush Mar 23, 2024
7eaa754
Refactor ByNameOveride problems into a struct with common fields
willbush Mar 24, 2024
d32d4cf
Refactor check_nix_file to use to_problem closure
willbush Mar 24, 2024
797f201
Refactor ByName problems into a struct with common fields
willbush Mar 24, 2024
e5aa63d
Refactor Path related problems into a struct with common fields
willbush Mar 24, 2024
b0d270e
Refactor Package related problems into a struct with common fields
willbush Mar 24, 2024
81e45fa
Refactor some nixpkgs_problem imports
willbush Mar 24, 2024
25adca4
Fix clone of `node` by moving variables above usage of `cast`
willbush Mar 26, 2024
2ed5984
Change most to_problem closures into to_validation
willbush Mar 26, 2024
c57206d
Replace RatchetErrorKind with is_new and is_empty bools
willbush Mar 26, 2024
bcc472d
Rename RatchetError to TopLevelPackageError
willbush Mar 26, 2024
591be8e
Add comments to NixpkgsProblem struct sub-types
willbush Mar 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 32 additions & 62 deletions src/eval.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use crate::nix_file::CallPackageArgumentInfo;
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;
@@ -213,7 +215,12 @@ fn by_name(
use ratchet::RatchetState::*;
use ByNameAttribute::*;

let relative_package_file = structure::relative_file_for_package(attribute_name);
let to_problem = |kind| {
NixpkgsProblem::ByName(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
@@ -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_problem(ByNameErrorKind::UndefinedAttr).into()
}
// 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_problem(ByNameErrorKind::NonDerivation).into()
}
// 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_problem(ByNameErrorKind::NonDerivation).into()
};

// 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_problem(ByNameErrorKind::InternalCallPackageUsed).into()
} 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_problem(ByNameErrorKind::CannotDetermineAttributeLocation).into()
}
}
};
@@ -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<CallPackageArgumentInfo>,
definition: String,
location: Location,
relative_location_file: RelativePathBuf,
) -> validation::Validation<ratchet::RatchetState<ratchet::ManualDefinition>> {
// 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 `<attr> = 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 `<attr> = foo`
(_, None) => to_problem(ByNameOverrideErrorKind::NonSyntacticCallPackage).into(),
// Something like `<attr> = 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 `<attr> = 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()
}
}
}
696 changes: 358 additions & 338 deletions src/nixpkgs_problem.rs

Large diffs are not rendered by default.

40 changes: 12 additions & 28 deletions src/ratchet.rs
Original file line number Diff line number Diff line change
@@ -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::Ratchet(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::MovedOutOfByName,
(None, true) => RatchetErrorKind::NewPackageNotUsingByNameEmptyArg,
(None, false) => RatchetErrorKind::NewPackageNotUsingByName,
},
})
}
}
74 changes: 33 additions & 41 deletions src/references.rs
Original file line number Diff line number Diff line change
@@ -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,13 @@ fn check_path(
subpath: &RelativePath,
) -> validation::Result<()> {
let path = subpath.to_path(absolute_package_dir);
let to_problem = |kind| {
NixpkgsProblem::Path(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() {
@@ -125,46 +128,35 @@ 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(());
};

let to_problem = |kind| {
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(),
kind,
})
};

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,
ResolvedPath::Interpolated => {
to_problem(NixFileErrorKind::PathInterpolation).into()
}
.into(),
ResolvedPath::SearchPath => NixpkgsProblem::SearchPath {
relative_package_dir: relative_package_dir.to_owned(),
subpath: subpath.to_owned(),
line,
text,
}
.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(),
ResolvedPath::SearchPath => to_problem(NixFileErrorKind::SearchPath).into(),
ResolvedPath::Outside => to_problem(NixFileErrorKind::OutsidePathReference).into(),
ResolvedPath::Unresolvable(e) => {
to_problem(NixFileErrorKind::UnresolvablePathReference {
io_error: e.to_string(),
})
.into()
}
.into(),
ResolvedPath::Within(..) => {
// No need to handle the case of it being inside the directory, since we scan through the
// entire directory recursively anyways
65 changes: 34 additions & 31 deletions src/structure.rs
Original file line number Diff line number Diff line change
@@ -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,18 +128,24 @@ 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_problem = |kind| {
NixpkgsProblem::Package(PackageError {
relative_package_dir: relative_package_dir.clone(),
}
kind,
})
};

Ok(if !package_path.is_dir() {
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(),
package_name: package_name.clone(),
}
to_problem(PackageErrorKind::InvalidPackageName {
invalid_package_name: package_name.clone(),
})
.into()
} else {
Success(())
@@ -146,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 {
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(())
@@ -160,15 +169,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(())
});