From ed892915b8a7cf156d49c07ac1ef36321981d722 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 30 Jan 2024 20:36:35 +0100 Subject: [PATCH 1/7] tests.nixpkgs-check-by-name: LineIndex functionality, semantics, tests - `fromlinecolumn` is added to be almost the reverse of `line`. This is needed in the future to get from `builtins.unsafeGetAttrPos` locations to the index for rnix - Previously the semantics for newline indices were not really defined, now they are, and make more sense. - Now there's a unit test for these functions --- pkgs/test/nixpkgs-check-by-name/src/utils.rs | 48 +++++++++++++++++++- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/utils.rs b/pkgs/test/nixpkgs-check-by-name/src/utils.rs index 7e0198dede424..9a5d12748918a 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/utils.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/utils.rs @@ -35,12 +35,13 @@ impl LineIndex { // the vec for split in s.split_inclusive('\n') { index += split.len(); - newlines.push(index); + newlines.push(index - 1); } LineIndex { newlines } } - /// Returns the line number for a string index + /// Returns the line number for a string index. + /// If the index points to a newline, returns the line number before the newline pub fn line(&self, index: usize) -> usize { match self.newlines.binary_search(&index) { // +1 because lines are 1-indexed @@ -48,4 +49,47 @@ impl LineIndex { Err(x) => x + 1, } } + + /// Returns the string index for a line and column. + pub fn fromlinecolumn(&self, line: usize, column: usize) -> usize { + // If it's the 1th line, the column is the index + if line == 1 { + // But columns are 1-indexed + column - 1 + } else { + // For the nth line, we add the index of the (n-1)st newline to the column, + // and remove one more from the index since arrays are 0-indexed. + // Then add the 1-indexed column to get not the newline index itself, + // but rather the index of the position on the next line + self.newlines[line - 2] + column + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn line_index() { + let line_index = LineIndex::new("a\nbc\n\ndef\n"); + + let pairs = [ + (0, 1, 1), + (1, 1, 2), + (2, 2, 1), + (3, 2, 2), + (4, 2, 3), + (5, 3, 1), + (6, 4, 1), + (7, 4, 2), + (8, 4, 3), + (9, 4, 4), + ]; + + for (index, line, column) in pairs { + assert_eq!(line_index.line(index), line); + assert_eq!(line_index.fromlinecolumn(line, column), index); + } + } } From 0bcb05228423463ff80ad1419111ff4df31b9ee4 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 30 Jan 2024 21:24:56 +0100 Subject: [PATCH 2/7] tests.nixpkgs-check-by-name: Syntactic callPackage detection This makes the callPackage detection stronger by syntactically detecting whether an attribute is using callPackage See the added test case for why this is needed. --- pkgs/test/nixpkgs-check-by-name/Cargo.lock | 8 + pkgs/test/nixpkgs-check-by-name/Cargo.toml | 2 + pkgs/test/nixpkgs-check-by-name/src/eval.nix | 1 + pkgs/test/nixpkgs-check-by-name/src/eval.rs | 53 ++- pkgs/test/nixpkgs-check-by-name/src/main.rs | 8 +- .../nixpkgs-check-by-name/src/nix_file.rs | 443 ++++++++++++++++++ .../test/nixpkgs-check-by-name/src/ratchet.rs | 16 +- .../tests/callPackage-syntax/all-packages.nix | 7 + .../tests/callPackage-syntax/default.nix | 1 + .../callPackage-syntax/pkgs/by-name/README.md | 0 10 files changed, 516 insertions(+), 23 deletions(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/src/nix_file.rs create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.lock b/pkgs/test/nixpkgs-check-by-name/Cargo.lock index fc3aeb9fd79b4..904a9cff0e789 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.lock +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.lock @@ -213,6 +213,12 @@ version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "443144c8cdadd93ebf52ddb4056d257f5b52c04d3c804e657d19eb73fc33668b" +[[package]] +name = "indoc" +version = "2.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e186cfbae8084e513daff4240b4797e342f988cecda4fb6c939150f96315fd8" + [[package]] name = "is-terminal" version = "0.4.9" @@ -289,10 +295,12 @@ dependencies = [ "anyhow", "clap", "colored", + "indoc", "itertools", "lazy_static", "regex", "rnix", + "rowan", "serde", "serde_json", "temp-env", diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml index 1e6eaa1106d54..be15ac5f2453b 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml @@ -14,6 +14,8 @@ anyhow = "1.0" lazy_static = "1.4.0" colored = "2.0.4" itertools = "0.11.0" +rowan = "0.15.11" +indoc = "2.0.4" [dev-dependencies] temp-env = "0.3.5" diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix index 87c54b6444ee3..7179951d41cf0 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix @@ -76,6 +76,7 @@ let CallPackage = { call_package_variant = value._callPackageVariant; is_derivation = pkgs.lib.isDerivation value; + location = builtins.unsafeGetAttrPos name pkgs; }; }; diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index dd30cb9045e53..85c1f2812a68e 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,7 +1,9 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::ratchet; use crate::structure; +use crate::validation::ResultIteratorExt; use crate::validation::{self, Validation::Success}; +use crate::NixFileCache; use std::path::Path; use anyhow::Context; @@ -48,6 +50,15 @@ struct CallPackageInfo { call_package_variant: CallPackageVariant, /// Whether the attribute is a derivation (`lib.isDerivation`) is_derivation: bool, + location: Option, +} + +/// The structure returned by `builtins.unsafeGetAttrPos` +#[derive(Deserialize, Clone, Debug)] +struct Location { + pub file: PathBuf, + pub line: usize, + pub column: usize, } #[derive(Deserialize)] @@ -70,6 +81,7 @@ enum CallPackageVariant { /// See the `eval.nix` file for how this is achieved on the Nix side pub fn check_values( nixpkgs_path: &Path, + nix_file_cache: &mut NixFileCache, package_names: Vec, keep_nix_path: bool, ) -> validation::Result { @@ -184,17 +196,36 @@ pub fn check_values( is_derivation: false, .. }) => Success(NonApplicable), - + // A location of None indicates something weird, we can't really know where + // this attribute is defined, probably an alias + CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight), // The case of an attribute that qualifies: // - Uses callPackage // - Is a derivation CallPackage(CallPackageInfo { is_derivation: true, - call_package_variant: Manual { path, empty_arg }, - }) => Success(Loose(ratchet::CouldUseByName { - call_package_path: path, - empty_arg, - })), + call_package_variant: Manual { .. }, + location: Some(location), + }) => + // We'll use the attribute's location to parse the file that defines it + match nix_file_cache.get(&location.file)? { + Err(error) => + // This is a bad sign for rnix, because it means cpp Nix could parse + // something that rnix couldn't + anyhow::bail!( + "Could not parse file {} with rnix, even though it parsed with cpp Nix: {error}", + location.file.display() + ), + Ok(nix_file) => + match nix_file.call_package_argument_info_at( + location.line, + location.column, + nixpkgs_path, + )? { + None => Success(NonApplicable), + Some(call_package_argument_info) => Success(Loose(call_package_argument_info)) + }, + }, }; uses_by_name.map(|x| ratchet::Package { manual_definition: Tight, @@ -240,6 +271,7 @@ pub fn check_values( ByName(Existing(CallPackage(CallPackageInfo { is_derivation, call_package_variant, + .. }))) => { let check_result = if !is_derivation { NixpkgsProblem::NonDerivation { @@ -256,6 +288,8 @@ pub fn check_values( manual_definition: Tight, uses_by_name: Tight, }), + // TODO: Use the call_package_argument_info_at instead/additionally and + // simplify the eval.nix code Manual { path, empty_arg } => { let correct_file = if let Some(call_package_path) = path { relative_package_file == *call_package_path @@ -280,9 +314,10 @@ pub fn check_values( }) } }; - check_result.map(|value| (attribute_name.clone(), value)) - }, - )); + Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) + }) + .collect_vec()?, + ); Ok(check_result.map(|elems| ratchet::Nixpkgs { package_names: elems.iter().map(|(name, _)| name.to_owned()).collect(), diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 8179ec8ded74b..9efff7a8b2bd1 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,4 +1,6 @@ +use crate::nix_file::NixFileCache; mod eval; +mod nix_file; mod nixpkgs_problem; mod ratchet; mod references; @@ -116,6 +118,8 @@ pub fn check_nixpkgs( keep_nix_path: bool, error_writer: &mut W, ) -> validation::Result { + let mut nix_file_cache = NixFileCache::new(); + Ok({ let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| { format!( @@ -134,7 +138,7 @@ pub fn check_nixpkgs( } else { check_structure(&nixpkgs_path)?.result_map(|package_names| // Only if we could successfully parse the structure, we do the evaluation checks - eval::check_values(&nixpkgs_path, package_names, keep_nix_path))? + eval::check_values(&nixpkgs_path, &mut nix_file_cache, package_names, keep_nix_path))? } }) } @@ -169,7 +173,7 @@ mod tests { // tempfile::tempdir needs to be wrapped in temp_env lock // because it accesses TMPDIR environment variable. - fn tempdir() -> anyhow::Result { + pub fn tempdir() -> anyhow::Result { let empty_list: [(&str, Option<&str>); 0] = []; Ok(temp_env::with_vars(empty_list, tempfile::tempdir)?) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs new file mode 100644 index 0000000000000..3e4f2668f3eaa --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs @@ -0,0 +1,443 @@ +//! This is a utility module for interacting with the syntax of Nix files + +use rnix::SyntaxKind; +use crate::utils::LineIndex; +use anyhow::Context; +use rnix::ast; +use rnix::ast::Expr; +use rnix::ast::HasEntry; +use rowan::ast::AstNode; +use rowan::TextSize; +use rowan::TokenAtOffset; +use std::collections::hash_map::Entry; +use std::collections::HashMap; +use std::fs::read_to_string; +use std::path::Path; +use std::path::PathBuf; + +/// A structure to indefinitely cache parse results of Nix files in memory, +/// making sure that the same file never has to be parsed twice +pub struct NixFileCache { + entries: HashMap, +} + +impl NixFileCache { + /// Creates a new empty NixFileCache + pub fn new() -> NixFileCache { + NixFileCache { + entries: HashMap::new(), + } + } + + /// Get the cache entry for a Nix file if it exists, otherwise parse the file, insert it into + /// the cache, and return the value + /// + /// Note that this function only gives an anyhow::Result::Err for I/O errors. + /// A parse error is anyhow::Result::Ok(Result::Err(error)) + pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFileResult> { + match self.entries.entry(path.to_owned()) { + Entry::Occupied(entry) => Ok(entry.into_mut()), + Entry::Vacant(entry) => Ok(entry.insert(NixFile::new(path)?)), + } + } +} + +/// A type to represent the result when trying to initialise a `NixFile`. +type NixFileResult = Result; + +/// A structure for storing a successfully parsed Nix file +pub struct NixFile { + /// The parent directory of the Nix file, for more convenient error handling + pub parent_dir: PathBuf, + /// The path to the file itself, for errors + pub path: PathBuf, + pub syntax_root: rnix::Root, + pub line_index: LineIndex, +} + +impl NixFile { + /// Creates a new NixFile, failing for I/O or parse errors + fn new(path: impl AsRef) -> anyhow::Result { + let Some(parent_dir) = path.as_ref().parent() else { + anyhow::bail!("Could not get parent of path {}", path.as_ref().display()) + }; + + let contents = read_to_string(&path) + .with_context(|| format!("Could not read file {}", path.as_ref().display()))?; + let line_index = LineIndex::new(&contents); + + Ok(rnix::Root::parse(&contents).ok().map(|syntax_root| NixFile { + parent_dir: parent_dir.to_path_buf(), + path: path.as_ref().to_owned(), + syntax_root, + line_index, + })) + } +} + +/// Information about callPackage arguments +#[derive(Debug, PartialEq)] +pub struct CallPackageArgumentInfo { + /// The relative path of the first argument, or `None` if it's not a path. + pub relative_path: Option, + /// Whether the second argument is an empty attribute set + pub empty_arg: bool, +} + +impl NixFile { + /// Returns information about callPackage arguments for an attribute at a specific line/column + /// index. If there's no guaranteed `callPackage` at that location, `None` is returned. + /// + /// This is meant to be used with the location returned from `builtins.unsafeGetAttrPos`, e.g.: + /// - Create file `default.nix` with contents + /// ```nix + /// self: { + /// foo = self.callPackage ./default.nix { }; + /// } + /// ``` + /// - Evaluate + /// ```nix + /// builtins.unsafeGetAttrPos "foo" (import ./default.nix { }) + /// ``` + /// results in `{ file = ./default.nix; line = 2; column = 3; }` + /// - Get the NixFile for `.file` from a `NixFileCache` + /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current directory + /// + /// You'll get back + /// ```rust + /// Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }) + /// ``` + /// + /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an + /// attempt at distinguishing this. + pub fn call_package_argument_info_at(&self, line: usize, column: usize, relative_to: &Path) -> anyhow::Result> { + let Some(attrpath_value) = self.attrpath_value_at(line, column)? else { + return Ok(None) + }; + self.attrpath_value_call_package_argument_info(attrpath_value, relative_to) + } + + // Internal function mainly to make it independently testable + fn attrpath_value_at(&self, line: usize, column: usize) -> anyhow::Result> { + let index = self.line_index.fromlinecolumn(line, column); + + let token_at_offset = self.syntax_root.syntax().token_at_offset(TextSize::from(index as u32)); + + // The token_at_offset function takes indices to mean a location _between_ characters, + // which in this case is some spacing followed by the attribute name: + // + // foo = 10; + // /\ + // This is the token offset, we get both the (newline + indentation) on the left side, + // and the attribute name on the right side. + let TokenAtOffset::Between(_space, token) = token_at_offset else { + anyhow::bail!("Line {line} column {column} in {} is not the start of a token, but rather {token_at_offset:?}", self.path.display()) + }; + + // token looks like "foo" + let Some(node) = token.parent() else { + anyhow::bail!("Token on line {line} column {column} in {} does not have a parent node: {token:?}", self.path.display()) + }; + + // node looks like "foo" + let Some(attrpath_node) = node.parent() else { + anyhow::bail!("Node in {} does not have a parent node: {node:?}", self.path.display()) + }; + + if attrpath_node.kind() != SyntaxKind::NODE_ATTRPATH { + return Ok(None) + } + // attrpath_node looks like "foo.bar" + let Some(attrpath_value_node) = attrpath_node.parent() else { + anyhow::bail!("Attribute path node in {} does not have a parent node: {attrpath_node:?}", self.path.display()) + }; + + if ! ast::AttrpathValue::can_cast(attrpath_value_node.kind()) { + anyhow::bail!("Node in {} is not an attribute path value node: {attrpath_value_node:?}", self.path.display()) + } + // attrpath_value_node looks like "foo.bar = 10;" + + // unwrap is fine because we confirmed that we can cast with the above check + Ok(Some(ast::AttrpathValue::cast(attrpath_value_node).unwrap())) + } + + // Internal function mainly to make attrpath_value_at independently testable + fn attrpath_value_call_package_argument_info( + &self, + attrpath_value: ast::AttrpathValue, + relative_to: &Path, + ) -> anyhow::Result> { + let Some(attrpath) = attrpath_value.attrpath() else { + anyhow::bail!("attrpath value node doesn't have an attrpath: {attrpath_value:?}") + }; + + // At this point we know it's something like `foo...bar = ...` + + if attrpath.attrs().count() > 1 { + // If the attribute path has multiple entries, the left-most entry is an attribute and + // can't be a `callPackage`. + // + // FIXME: `builtins.unsafeGetAttrPos` will return the same position for all attribute + // paths and we can't really know which one it is. We could have a case like + // `foo.bar = callPackage ... { }` and trying to determine if `bar` is a `callPackage`, + // where this is not correct. + // However, this case typically doesn't occur anyways, + // because top-level packages wouldn't be nested under an attribute set. + return Ok(None); + } + let Some(value) = attrpath_value.value() else { + anyhow::bail!("attrpath value node doesn't have a value: {attrpath_value:?}") + }; + + // At this point we know it's something like `foo = ...` + + let Expr::Apply(apply1) = value else { + // Not even a function call, instead something like `foo = null` + return Ok(None); + }; + let Some(function1) = apply1.lambda() else { + anyhow::bail!("apply node doesn't have a lambda: {apply1:?}") + }; + let Some(arg1) = apply1.argument() else { + anyhow::bail!("apply node doesn't have an argument: {apply1:?}") + }; + + // At this point we know it's something like `foo = `. + // For a callPackage, `` would be `callPackage ./file` and `` would be `{ }` + + let empty_arg = if let Expr::AttrSet(attrset) = arg1 { + // We can only statically determine whether the argument is empty if it's an attribute + // set _expression_, even though other kind of expressions could evaluate to an attribute + // set _value_. But this is what we want anyways + attrset.entries().next().is_none() + } else { + false + }; + + // Because callPackage takes two curried arguments, the first function needs to be a + // function call itself + let Expr::Apply(apply2) = function1 else { + // Not a callPackage, instead something like `foo = import ./foo` + return Ok(None); + }; + let Some(function2) = apply2.lambda() else { + anyhow::bail!("apply node doesn't have a lambda: {apply2:?}") + }; + let Some(arg2) = apply2.argument() else { + anyhow::bail!("apply node doesn't have an argument: {apply2:?}") + }; + + // At this point we know it's something like `foo = `. + // For a callPackage, `` would be `callPackage`, `` would be `./file` + + // Check that is a path expression + let path = if let Expr::Path(actual_path) = arg2 { + // Try to statically resolve the path and turn it into a nixpkgs-relative path + if let ResolvedPath::Within(p) = self.static_resolve_path(actual_path, relative_to) { + Some(p) + } else { + // We can't statically know an existing path inside Nixpkgs used as + None + } + } else { + // is not a path, but rather e.g. an inline expression + None + }; + + // Check that is an identifier, or an attribute path with an identifier at the end + let ident = match function2 { + Expr::Ident(ident) => + // This means it's something like `foo = callPackage ` + ident, + Expr::Select(select) => { + // This means it's something like `foo = self.callPackage `. + // We also end up here for e.g. `pythonPackages.callPackage`, but the + // callPackage-mocking method will take care of not triggering for this case. + + if select.default_expr().is_some() { + // Very odd case, but this would be `foo = self.callPackage or true ./test.nix {} + // (yes this is valid Nix code) + return Ok(None) + } + let Some(attrpath) = select.attrpath() else { + anyhow::bail!("select node doesn't have an attrpath: {select:?}") + }; + let Some(last) = attrpath.attrs().last() else { + // This case shouldn't be possible, it would be `foo = self. ./test.nix {}`, + // which shouldn't parse + anyhow::bail!("select node has an empty attrpath: {select:?}") + }; + if let ast::Attr::Ident(ident) = last { + ident + } else { + // Here it's something like `foo = self."callPackage" /test.nix {}` + // which we're not gonna bother with + return Ok(None) + } + }, + // Any other expression we're not gonna treat as callPackage + _ => return Ok(None), + }; + + let Some(token) = ident.ident_token() else { + anyhow::bail!("ident node doesn't have a token: {ident:?}") + }; + + if token.text() == "callPackage" { + Ok(Some(CallPackageArgumentInfo { relative_path: path, empty_arg })) + } else { + Ok(None) + } + } +} + +/// The result of trying to statically resolve a Nix path expression +pub enum ResolvedPath { + /// Something like `./foo/${bar}/baz`, can't be known statically + Interpolated, + /// Something like ``, can't be known statically + SearchPath, + /// Path couldn't be resolved due to an IO error, + /// e.g. if the path doesn't exist or you don't have the right permissions + Unresolvable(std::io::Error), + /// The path is outside the given absolute path + Outside, + /// The path is within the given absolute path. + /// The `PathBuf` is the relative path under the given absolute path. + Within(PathBuf), +} + +impl NixFile { + /// Statically resolves a Nix path expression and checks that it's within an absolute path + /// path + /// + /// E.g. for the path expression `./bar.nix` in `./foo.nix` and an absolute path of the + /// current directory, the function returns `ResolvedPath::Within(./bar.nix)` + pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath { + if node.parts().count() != 1 { + // Only if there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`. + return ResolvedPath::Interpolated; + } + + let text = node.to_string(); + + if text.starts_with('<') { + // A search path like ``. There doesn't appear to be better way to detect + // these in rnix + return ResolvedPath::SearchPath; + } + + // Join the file's parent directory and the path expression, then resolve it + // FIXME: Expressions like `../../../../foo/bar/baz/qux` or absolute paths + // may resolve close to the original file, but may have left the relative_to. + // That should be checked more strictly + match self.parent_dir.join(Path::new(&text)).canonicalize() { + Err(resolution_error) => ResolvedPath::Unresolvable(resolution_error), + Ok(resolved) => + // Check if it's within relative_to + match resolved.strip_prefix(relative_to) { + Err(_prefix_error) => ResolvedPath::Outside, + Ok(suffix) => ResolvedPath::Within(suffix.to_path_buf()), + } + } + } +} + +#[cfg(test)] +mod tests { + use crate::tests; + use super::*; + use indoc::indoc; + + #[test] + fn detects_attributes() -> anyhow::Result<()> { + let temp_dir = tests::tempdir()?; + let file = temp_dir.path().join("file.nix"); + let contents = indoc! {r#" + toInherit: { + foo = 1; + "bar" = 2; + ${"baz"} = 3; + "${"qux"}" = 4; + + # A + quux + # B + = + # C + 5 + # D + ; + # E + + /**/quuux/**/=/**/5/**/;/*E*/ + + inherit toInherit; + } + "#}; + + std::fs::write(&file, contents)?; + + let nix_file = NixFile::new(&file)??; + + // These are builtins.unsafeGetAttrPos locations for the attributes + let cases = [ + (2, 3, Some("foo = 1;")), + (3, 3, Some(r#""bar" = 2;"#)), + (4, 3, Some(r#"${"baz"} = 3;"#)), + (5, 3, Some(r#""${"qux"}" = 4;"#)), + (8, 3, Some("quux\n # B\n =\n # C\n 5\n # D\n ;")), + (17, 7, Some("quuux/**/=/**/5/**/;")), + (19, 10, None), + ]; + + for (line, column, expected_result) in cases { + let actual_result = nix_file.attrpath_value_at(line, column)?.map(|node| node.to_string()); + assert_eq!(actual_result.as_deref(), expected_result); + } + + Ok(()) + } + + #[test] + fn detects_call_package() -> anyhow::Result<()> { + let temp_dir = tests::tempdir()?; + let file = temp_dir.path().join("file.nix"); + let contents = indoc! {r#" + self: with self; { + a.sub = null; + b = null; + c = import ./file.nix; + d = import ./file.nix { }; + e = pythonPackages.callPackage ./file.nix { }; + f = callPackage ./file.nix { }; + g = callPackage ({ }: { }) { }; + h = callPackage ./file.nix { x = 0; }; + i = callPackage ({ }: { }) (let in { }); + } + "#}; + + std::fs::write(&file, contents)?; + + let nix_file = NixFile::new(&file)??; + + let cases = [ + (2, None), + (3, None), + (4, None), + (5, None), + (6, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true })), + (7, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true })), + (8, Some(CallPackageArgumentInfo { relative_path: None, empty_arg: true })), + (9, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: false })), + (10, Some(CallPackageArgumentInfo { relative_path: None, empty_arg: false })), + ]; + + for (line, expected_result) in cases { + let actual_result = nix_file.call_package_argument_info_at(line, 3, temp_dir.path())?; + assert_eq!(actual_result, expected_result); + } + + Ok(()) + } +} diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs index 10ecc01d3580c..200bf92c516a1 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -2,11 +2,11 @@ //! //! 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::structure; use crate::validation::{self, Validation, Validation::Success}; use std::collections::HashMap; -use std::path::PathBuf; /// The ratchet value for the entirety of Nixpkgs. #[derive(Default)] @@ -148,16 +148,8 @@ impl ToNixpkgsProblem for ManualDefinition { /// It also checks that once a package uses pkgs/by-name, it can't switch back to all-packages.nix pub enum UsesByName {} -#[derive(Clone)] -pub struct CouldUseByName { - /// The first callPackage argument, used for better errors - pub call_package_path: Option, - /// Whether the second callPackage argument is empty, used for better errors - pub empty_arg: bool, -} - impl ToNixpkgsProblem for UsesByName { - type ToContext = CouldUseByName; + type ToContext = CallPackageArgumentInfo; fn to_nixpkgs_problem( name: &str, @@ -167,13 +159,13 @@ impl ToNixpkgsProblem for UsesByName { if let Some(()) = optional_from { NixpkgsProblem::MovedOutOfByName { package_name: name.to_owned(), - call_package_path: to.call_package_path.clone(), + call_package_path: to.relative_path.clone(), empty_arg: to.empty_arg, } } else { NixpkgsProblem::NewPackageNotUsingByName { package_name: name.to_owned(), - call_package_path: to.call_package_path.clone(), + call_package_path: to.relative_path.clone(), empty_arg: to.empty_arg, } } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix new file mode 100644 index 0000000000000..306d719c9e9d6 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix @@ -0,0 +1,7 @@ +self: super: { + set = self.callPackages ({ callPackage }: { + foo = callPackage ({ someDrv }: someDrv) { }; + }) { }; + + inherit (self.set) foo; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix new file mode 100644 index 0000000000000..861260cdca4b2 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md new file mode 100644 index 0000000000000..e69de29bb2d1d From db435ae516e548b278c156cc5e015017bb8495f6 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 30 Jan 2024 21:26:39 +0100 Subject: [PATCH 3/7] tests.nixpkgs-check-by-name: Re-use nixFileCache more Makes the reference check use the nixFileCache instead of separately parsing and resolving paths --- pkgs/test/nixpkgs-check-by-name/src/main.rs | 2 +- .../nixpkgs-check-by-name/src/references.rs | 80 ++++++++----------- .../nixpkgs-check-by-name/src/structure.rs | 7 +- 3 files changed, 41 insertions(+), 48 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 9efff7a8b2bd1..9a8a898a21be1 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -136,7 +136,7 @@ pub fn check_nixpkgs( )?; Success(ratchet::Nixpkgs::default()) } else { - check_structure(&nixpkgs_path)?.result_map(|package_names| + check_structure(&nixpkgs_path, &mut nix_file_cache)?.result_map(|package_names| // Only if we could successfully parse the structure, we do the evaluation checks eval::check_values(&nixpkgs_path, &mut nix_file_cache, package_names, keep_nix_path))? } diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index ce7403afb32d6..5da5097952ac3 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,23 +1,23 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::utils; -use crate::utils::LineIndex; use crate::validation::{self, ResultIteratorExt, Validation::Success}; +use crate::NixFileCache; +use rowan::ast::AstNode; use anyhow::Context; -use rnix::{Root, SyntaxKind::NODE_PATH}; use std::ffi::OsStr; -use std::fs::read_to_string; use std::path::Path; /// Check that every package directory in pkgs/by-name doesn't link to outside that directory. /// Both symlinks and Nix path expressions are checked. pub fn check_references( + nix_file_cache: &mut NixFileCache, relative_package_dir: &Path, absolute_package_dir: &Path, ) -> validation::Result<()> { // The empty argument here is the subpath under the package directory to check // An empty one means the package directory itself - check_path(relative_package_dir, absolute_package_dir, Path::new("")).with_context(|| { + check_path(nix_file_cache, relative_package_dir, absolute_package_dir, Path::new("")).with_context(|| { format!( "While checking the references in package directory {}", relative_package_dir.display() @@ -27,6 +27,7 @@ pub fn check_references( /// Checks for a specific path to not have references outside fn check_path( + nix_file_cache: &mut NixFileCache, relative_package_dir: &Path, absolute_package_dir: &Path, subpath: &Path, @@ -63,7 +64,7 @@ fn check_path( .into_iter() .map(|entry| { let entry_subpath = subpath.join(entry.file_name()); - check_path(relative_package_dir, absolute_package_dir, &entry_subpath) + check_path(nix_file_cache, relative_package_dir, absolute_package_dir, &entry_subpath) .with_context(|| { format!("Error while recursing into {}", subpath.display()) }) @@ -74,7 +75,7 @@ fn check_path( // Only check Nix files if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { - check_nix_file(relative_package_dir, absolute_package_dir, subpath).with_context( + check_nix_file(nix_file_cache, relative_package_dir, absolute_package_dir, subpath).with_context( || format!("Error while checking Nix file {}", subpath.display()), )? } else { @@ -92,20 +93,16 @@ fn check_path( /// Check whether a nix file contains path expression references pointing outside the package /// directory fn check_nix_file( + nix_file_cache: &mut NixFileCache, relative_package_dir: &Path, absolute_package_dir: &Path, subpath: &Path, ) -> validation::Result<()> { let path = absolute_package_dir.join(subpath); - let parent_dir = path - .parent() - .with_context(|| format!("Could not get parent of path {}", subpath.display()))?; - let contents = read_to_string(&path) - .with_context(|| format!("Could not read file {}", subpath.display()))?; - - let root = Root::parse(&contents); - if let Some(error) = root.errors().first() { + let nix_file = match nix_file_cache.get(&path)? { + Ok(nix_file) => nix_file, + Err(error) => // NOTE: There's now another Nixpkgs CI check to make sure all changed Nix files parse // correctly, though that uses mainline Nix instead of rnix, so it doesn't give the same // errors. In the future we should unify these two checks, ideally moving the other CI @@ -115,20 +112,23 @@ fn check_nix_file( subpath: subpath.to_path_buf(), error: error.clone(), } - .into()); - } - - let line_index = LineIndex::new(&contents); + .into()) + }; - Ok(validation::sequence_(root.syntax().descendants().map( + Ok(validation::sequence_(nix_file.syntax_root.syntax().descendants().map( |node| { let text = node.text().to_string(); - let line = line_index.line(node.text_range().start().into()); + let line = nix_file.line_index.line(node.text_range().start().into()); - if node.kind() != NODE_PATH { // We're only interested in Path expressions - Success(()) - } else if node.children().count() != 0 { + 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) { + Interpolated => // Filters out ./foo/${bar}/baz // TODO: We can just check ./foo NixpkgsProblem::PathInterpolation { @@ -137,8 +137,8 @@ fn check_nix_file( line, text, } - .into() - } else if text.starts_with('<') { + .into(), + SearchPath => // Filters out search paths like NixpkgsProblem::SearchPath { relative_package_dir: relative_package_dir.to_path_buf(), @@ -146,28 +146,15 @@ fn check_nix_file( line, text, } - .into() - } else { - // Resolves the reference of the Nix path - // turning `../baz` inside `/foo/bar/default.nix` to `/foo/baz` - match parent_dir.join(Path::new(&text)).canonicalize() { - Ok(target) => { - // Then checking if it's still in the package directory - // 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::OutsidePathReference { + .into(), + Outside => NixpkgsProblem::OutsidePathReference { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, } - .into() - } else { - Success(()) - } - } - Err(e) => NixpkgsProblem::UnresolvablePathReference { + .into(), + Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, @@ -175,8 +162,11 @@ fn check_nix_file( io_error: e, } .into(), + Within(_p) => + // No need to handle the case of it being inside the directory, since we scan through the + // entire directory recursively anyways + Success(()) } - } - }, - ))) + }), + )) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 4051ca037c9a3..084b0185563cd 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -3,6 +3,7 @@ use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use crate::validation::{self, ResultIteratorExt, Validation::Success}; +use crate::NixFileCache; use itertools::concat; use lazy_static::lazy_static; use regex::Regex; @@ -34,7 +35,7 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf { /// Check the structure of Nixpkgs, returning the attribute names that are defined in /// `pkgs/by-name` -pub fn check_structure(path: &Path) -> validation::Result> { +pub fn check_structure(path: &Path, nix_file_cache: &mut NixFileCache) -> validation::Result> { let base_dir = path.join(BASE_SUBPATH); let shard_results = utils::read_dir_sorted(&base_dir)? @@ -88,7 +89,7 @@ pub fn check_structure(path: &Path) -> validation::Result> { let package_results = entries .into_iter() .map(|package_entry| { - check_package(path, &shard_name, shard_name_valid, package_entry) + check_package(nix_file_cache, path, &shard_name, shard_name_valid, package_entry) }) .collect_vec()?; @@ -102,6 +103,7 @@ pub fn check_structure(path: &Path) -> validation::Result> { } fn check_package( + nix_file_cache: &mut NixFileCache, path: &Path, shard_name: &str, shard_name_valid: bool, @@ -161,6 +163,7 @@ fn check_package( }); let result = result.and(references::check_references( + nix_file_cache, &relative_package_dir, &path.join(&relative_package_dir), )?); From 46da6c5c1f8daee0be796f27f1307b39e45fb5cf Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 30 Jan 2024 21:28:01 +0100 Subject: [PATCH 4/7] tests.nixpkgs-check-by-name: Format With `cargo fmt` Explicitly didn't do that for previous commits in order to keep the diff more easily reviewable --- .../nixpkgs-check-by-name/src/nix_file.rs | 127 +++++++++++++----- .../nixpkgs-check-by-name/src/references.rs | 109 +++++++++------ .../nixpkgs-check-by-name/src/structure.rs | 13 +- 3 files changed, 172 insertions(+), 77 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs index 3e4f2668f3eaa..e550f5c459f54 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs @@ -1,11 +1,11 @@ //! This is a utility module for interacting with the syntax of Nix files -use rnix::SyntaxKind; use crate::utils::LineIndex; use anyhow::Context; use rnix::ast; use rnix::ast::Expr; use rnix::ast::HasEntry; +use rnix::SyntaxKind; use rowan::ast::AstNode; use rowan::TextSize; use rowan::TokenAtOffset; @@ -66,12 +66,14 @@ impl NixFile { .with_context(|| format!("Could not read file {}", path.as_ref().display()))?; let line_index = LineIndex::new(&contents); - Ok(rnix::Root::parse(&contents).ok().map(|syntax_root| NixFile { - parent_dir: parent_dir.to_path_buf(), - path: path.as_ref().to_owned(), - syntax_root, - line_index, - })) + Ok(rnix::Root::parse(&contents) + .ok() + .map(|syntax_root| NixFile { + parent_dir: parent_dir.to_path_buf(), + path: path.as_ref().to_owned(), + syntax_root, + line_index, + })) } } @@ -110,18 +112,30 @@ impl NixFile { /// /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an /// attempt at distinguishing this. - pub fn call_package_argument_info_at(&self, line: usize, column: usize, relative_to: &Path) -> anyhow::Result> { + pub fn call_package_argument_info_at( + &self, + line: usize, + column: usize, + relative_to: &Path, + ) -> anyhow::Result> { let Some(attrpath_value) = self.attrpath_value_at(line, column)? else { - return Ok(None) + return Ok(None); }; self.attrpath_value_call_package_argument_info(attrpath_value, relative_to) } // Internal function mainly to make it independently testable - fn attrpath_value_at(&self, line: usize, column: usize) -> anyhow::Result> { + fn attrpath_value_at( + &self, + line: usize, + column: usize, + ) -> anyhow::Result> { let index = self.line_index.fromlinecolumn(line, column); - let token_at_offset = self.syntax_root.syntax().token_at_offset(TextSize::from(index as u32)); + let token_at_offset = self + .syntax_root + .syntax() + .token_at_offset(TextSize::from(index as u32)); // The token_at_offset function takes indices to mean a location _between_ characters, // which in this case is some spacing followed by the attribute name: @@ -136,24 +150,36 @@ impl NixFile { // token looks like "foo" let Some(node) = token.parent() else { - anyhow::bail!("Token on line {line} column {column} in {} does not have a parent node: {token:?}", self.path.display()) + anyhow::bail!( + "Token on line {line} column {column} in {} does not have a parent node: {token:?}", + self.path.display() + ) }; // node looks like "foo" let Some(attrpath_node) = node.parent() else { - anyhow::bail!("Node in {} does not have a parent node: {node:?}", self.path.display()) + anyhow::bail!( + "Node in {} does not have a parent node: {node:?}", + self.path.display() + ) }; if attrpath_node.kind() != SyntaxKind::NODE_ATTRPATH { - return Ok(None) + return Ok(None); } // attrpath_node looks like "foo.bar" let Some(attrpath_value_node) = attrpath_node.parent() else { - anyhow::bail!("Attribute path node in {} does not have a parent node: {attrpath_node:?}", self.path.display()) + anyhow::bail!( + "Attribute path node in {} does not have a parent node: {attrpath_node:?}", + self.path.display() + ) }; - if ! ast::AttrpathValue::can_cast(attrpath_value_node.kind()) { - anyhow::bail!("Node in {} is not an attribute path value node: {attrpath_value_node:?}", self.path.display()) + if !ast::AttrpathValue::can_cast(attrpath_value_node.kind()) { + anyhow::bail!( + "Node in {} is not an attribute path value node: {attrpath_value_node:?}", + self.path.display() + ) } // attrpath_value_node looks like "foo.bar = 10;" @@ -247,8 +273,10 @@ impl NixFile { // Check that is an identifier, or an attribute path with an identifier at the end let ident = match function2 { Expr::Ident(ident) => - // This means it's something like `foo = callPackage ` - ident, + // This means it's something like `foo = callPackage ` + { + ident + } Expr::Select(select) => { // This means it's something like `foo = self.callPackage `. // We also end up here for e.g. `pythonPackages.callPackage`, but the @@ -257,7 +285,7 @@ impl NixFile { if select.default_expr().is_some() { // Very odd case, but this would be `foo = self.callPackage or true ./test.nix {} // (yes this is valid Nix code) - return Ok(None) + return Ok(None); } let Some(attrpath) = select.attrpath() else { anyhow::bail!("select node doesn't have an attrpath: {select:?}") @@ -272,9 +300,9 @@ impl NixFile { } else { // Here it's something like `foo = self."callPackage" /test.nix {}` // which we're not gonna bother with - return Ok(None) + return Ok(None); } - }, + } // Any other expression we're not gonna treat as callPackage _ => return Ok(None), }; @@ -284,7 +312,10 @@ impl NixFile { }; if token.text() == "callPackage" { - Ok(Some(CallPackageArgumentInfo { relative_path: path, empty_arg })) + Ok(Some(CallPackageArgumentInfo { + relative_path: path, + empty_arg, + })) } else { Ok(None) } @@ -334,19 +365,21 @@ impl NixFile { match self.parent_dir.join(Path::new(&text)).canonicalize() { Err(resolution_error) => ResolvedPath::Unresolvable(resolution_error), Ok(resolved) => - // Check if it's within relative_to + // Check if it's within relative_to + { match resolved.strip_prefix(relative_to) { Err(_prefix_error) => ResolvedPath::Outside, Ok(suffix) => ResolvedPath::Within(suffix.to_path_buf()), } + } } } } #[cfg(test)] mod tests { - use crate::tests; use super::*; + use crate::tests; use indoc::indoc; #[test] @@ -392,7 +425,9 @@ mod tests { ]; for (line, column, expected_result) in cases { - let actual_result = nix_file.attrpath_value_at(line, column)?.map(|node| node.to_string()); + let actual_result = nix_file + .attrpath_value_at(line, column)? + .map(|node| node.to_string()); assert_eq!(actual_result.as_deref(), expected_result); } @@ -426,11 +461,41 @@ mod tests { (3, None), (4, None), (5, None), - (6, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true })), - (7, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true })), - (8, Some(CallPackageArgumentInfo { relative_path: None, empty_arg: true })), - (9, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: false })), - (10, Some(CallPackageArgumentInfo { relative_path: None, empty_arg: false })), + ( + 6, + Some(CallPackageArgumentInfo { + relative_path: Some(PathBuf::from("file.nix")), + empty_arg: true, + }), + ), + ( + 7, + Some(CallPackageArgumentInfo { + relative_path: Some(PathBuf::from("file.nix")), + empty_arg: true, + }), + ), + ( + 8, + Some(CallPackageArgumentInfo { + relative_path: None, + empty_arg: true, + }), + ), + ( + 9, + Some(CallPackageArgumentInfo { + relative_path: Some(PathBuf::from("file.nix")), + empty_arg: false, + }), + ), + ( + 10, + Some(CallPackageArgumentInfo { + relative_path: None, + empty_arg: false, + }), + ), ]; for (line, expected_result) in cases { diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 5da5097952ac3..5ff8cf5168826 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -3,8 +3,8 @@ use crate::utils; use crate::validation::{self, ResultIteratorExt, Validation::Success}; use crate::NixFileCache; -use rowan::ast::AstNode; use anyhow::Context; +use rowan::ast::AstNode; use std::ffi::OsStr; use std::path::Path; @@ -17,7 +17,13 @@ pub fn check_references( ) -> validation::Result<()> { // The empty argument here is the subpath under the package directory to check // An empty one means the package directory itself - check_path(nix_file_cache, relative_package_dir, absolute_package_dir, Path::new("")).with_context(|| { + check_path( + nix_file_cache, + relative_package_dir, + absolute_package_dir, + Path::new(""), + ) + .with_context(|| { format!( "While checking the references in package directory {}", relative_package_dir.display() @@ -64,10 +70,13 @@ fn check_path( .into_iter() .map(|entry| { let entry_subpath = subpath.join(entry.file_name()); - check_path(nix_file_cache, relative_package_dir, absolute_package_dir, &entry_subpath) - .with_context(|| { - format!("Error while recursing into {}", subpath.display()) - }) + check_path( + nix_file_cache, + relative_package_dir, + absolute_package_dir, + &entry_subpath, + ) + .with_context(|| format!("Error while recursing into {}", subpath.display())) }) .collect_vec()?, ) @@ -75,9 +84,13 @@ fn check_path( // Only check Nix files if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { - check_nix_file(nix_file_cache, relative_package_dir, absolute_package_dir, subpath).with_context( - || format!("Error while checking Nix file {}", subpath.display()), - )? + check_nix_file( + nix_file_cache, + relative_package_dir, + absolute_package_dir, + subpath, + ) + .with_context(|| format!("Error while checking Nix file {}", subpath.display()))? } else { Success(()) } @@ -107,66 +120,74 @@ fn check_nix_file( // correctly, though that uses mainline Nix instead of rnix, so it doesn't give the same // errors. In the future we should unify these two checks, ideally moving the other CI // check into this tool as well and checking for both mainline Nix and rnix. - return Ok(NixpkgsProblem::CouldNotParseNix { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - error: error.clone(), + { + return Ok(NixpkgsProblem::CouldNotParseNix { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + error: error.clone(), + } + .into()) } - .into()) }; - Ok(validation::sequence_(nix_file.syntax_root.syntax().descendants().map( - |node| { + 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 { - return Success(()) - }; + // We're only interested in Path expressions + let Some(path) = rnix::ast::Path::cast(node) else { + return Success(()); + }; - use crate::nix_file::ResolvedPath::*; + use crate::nix_file::ResolvedPath::*; - match nix_file.static_resolve_path(path, absolute_package_dir) { + match nix_file.static_resolve_path(path, absolute_package_dir) { Interpolated => // Filters out ./foo/${bar}/baz // TODO: We can just check ./foo - NixpkgsProblem::PathInterpolation { + { + NixpkgsProblem::PathInterpolation { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into() + } + SearchPath => + // Filters out search paths like + { + NixpkgsProblem::SearchPath { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, + } + .into() + } + Outside => NixpkgsProblem::OutsidePathReference { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, } .into(), - SearchPath => - // Filters out search paths like - NixpkgsProblem::SearchPath { + Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, + io_error: e, } .into(), - Outside => NixpkgsProblem::OutsidePathReference { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into(), - Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - line, - text, - io_error: e, - } - .into(), - Within(_p) => - // No need to handle the case of it being inside the directory, since we scan through the - // entire directory recursively anyways + Within(_p) => + // No need to handle the case of it being inside the directory, since we scan through the + // entire directory recursively anyways + { Success(()) } + } }), )) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 084b0185563cd..7f9e7d4f717f7 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -35,7 +35,10 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf { /// Check the structure of Nixpkgs, returning the attribute names that are defined in /// `pkgs/by-name` -pub fn check_structure(path: &Path, nix_file_cache: &mut NixFileCache) -> validation::Result> { +pub fn check_structure( + path: &Path, + nix_file_cache: &mut NixFileCache, +) -> validation::Result> { let base_dir = path.join(BASE_SUBPATH); let shard_results = utils::read_dir_sorted(&base_dir)? @@ -89,7 +92,13 @@ pub fn check_structure(path: &Path, nix_file_cache: &mut NixFileCache) -> valida let package_results = entries .into_iter() .map(|package_entry| { - check_package(nix_file_cache, path, &shard_name, shard_name_valid, package_entry) + check_package( + nix_file_cache, + path, + &shard_name, + shard_name_valid, + package_entry, + ) }) .collect_vec()?; From 7d1f0a4cd4ac9b224ace12222107882d869adce0 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 30 Jan 2024 22:35:04 +0100 Subject: [PATCH 5/7] tests.nixpkgs-check-by-name: Allow new package variants --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 23 ++++++++++++++++++- .../tests/package-variants/all-packages.nix | 5 ++++ .../package-variants/base/all-packages.nix | 3 +++ .../tests/package-variants/base/default.nix | 1 + .../base/pkgs/by-name/fo/foo/package.nix | 1 + .../tests/package-variants/default.nix | 1 + .../tests/package-variants/package.nix | 1 + .../pkgs/by-name/fo/foo/package.nix | 1 + 8 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/all-packages.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/all-packages.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/pkgs/by-name/fo/foo/package.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/default.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/package.nix create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/pkgs/by-name/fo/foo/package.nix diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 85c1f2812a68e..53714886fa873 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,6 +1,7 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::ratchet; use crate::structure; +use crate::utils; use crate::validation::ResultIteratorExt; use crate::validation::{self, Validation::Success}; use crate::NixFileCache; @@ -223,7 +224,27 @@ pub fn check_values( nixpkgs_path, )? { None => Success(NonApplicable), - Some(call_package_argument_info) => Success(Loose(call_package_argument_info)) + Some(call_package_argument_info) => { + if let Some(ref rel_path) = call_package_argument_info.relative_path { + if rel_path.starts_with(utils::BASE_SUBPATH) { + // Package variants of by-name packages are explicitly allowed according to RFC 140 + // https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants: + // + // foo-variant = callPackage ../by-name/fo/foo/package.nix { + // someFlag = true; + // } + // + // While such definitions could be moved to `pkgs/by-name` by using + // `.override { someFlag = true; }` instead, this changes the semantics in + // relation with overlays. + Success(NonApplicable) + } else { + Success(Loose(call_package_argument_info)) + } + } else { + Success(Loose(call_package_argument_info)) + } + }, }, }, }; diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/all-packages.nix new file mode 100644 index 0000000000000..85f8c6138c5c2 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/all-packages.nix @@ -0,0 +1,5 @@ +self: super: { + foo-variant-unvarianted = self.callPackage ./package.nix { }; + + foo-variant-new = self.callPackage ./pkgs/by-name/fo/foo/package.nix { }; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/all-packages.nix new file mode 100644 index 0000000000000..734604360073b --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/all-packages.nix @@ -0,0 +1,3 @@ +self: super: { + foo-variant-unvarianted = self.callPackage ./pkgs/by-name/fo/foo/package.nix { }; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/default.nix new file mode 100644 index 0000000000000..861260cdca4b2 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/pkgs/by-name/fo/foo/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000000000..a1b92efbbadb9 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/base/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/default.nix new file mode 100644 index 0000000000000..861260cdca4b2 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/default.nix @@ -0,0 +1 @@ +import { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/package.nix new file mode 100644 index 0000000000000..a1b92efbbadb9 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/pkgs/by-name/fo/foo/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/pkgs/by-name/fo/foo/package.nix new file mode 100644 index 0000000000000..a1b92efbbadb9 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/pkgs/by-name/fo/foo/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv From 37a54e3ad58bbcc7791fa683bbd79453d8e988ac Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 5 Feb 2024 01:18:12 +0100 Subject: [PATCH 6/7] tests.nixpkgs-check-by-name: Apply feedback from review https://github.com/NixOS/nixpkgs/pull/285089#pullrequestreview-1861099233 Many thanks, Philip Taron! --- pkgs/test/nixpkgs-check-by-name/Cargo.toml | 2 +- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 322 +++++++++--------- pkgs/test/nixpkgs-check-by-name/src/main.rs | 8 +- .../nixpkgs-check-by-name/src/nix_file.rs | 68 ++-- .../src/nixpkgs_problem.rs | 14 - .../nixpkgs-check-by-name/src/references.rs | 97 +++--- .../nixpkgs-check-by-name/src/structure.rs | 10 +- 7 files changed, 247 insertions(+), 274 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml index be15ac5f2453b..5240cd69f996e 100644 --- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml +++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml @@ -15,7 +15,7 @@ lazy_static = "1.4.0" colored = "2.0.4" itertools = "0.11.0" rowan = "0.15.11" -indoc = "2.0.4" [dev-dependencies] temp-env = "0.3.5" +indoc = "2.0.4" diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 53714886fa873..bdde0e560057f 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -2,9 +2,9 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::ratchet; use crate::structure; use crate::utils; -use crate::validation::ResultIteratorExt; +use crate::validation::ResultIteratorExt as _; use crate::validation::{self, Validation::Success}; -use crate::NixFileCache; +use crate::NixFileStore; use std::path::Path; use anyhow::Context; @@ -82,7 +82,7 @@ enum CallPackageVariant { /// See the `eval.nix` file for how this is achieved on the Nix side pub fn check_values( nixpkgs_path: &Path, - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, package_names: Vec, keep_nix_path: bool, ) -> validation::Result { @@ -155,77 +155,77 @@ pub fn check_values( ) })?; - let check_result = validation::sequence(attributes.into_iter().map( - |(attribute_name, attribute_value)| { - let relative_package_file = structure::relative_file_for_package(&attribute_name); + let check_result = validation::sequence( + attributes + .into_iter() + .map(|(attribute_name, attribute_value)| { + let relative_package_file = structure::relative_file_for_package(&attribute_name); - use ratchet::RatchetState::*; - use Attribute::*; - use AttributeInfo::*; - use ByNameAttribute::*; - use CallPackageVariant::*; - use NonByNameAttribute::*; + use ratchet::RatchetState::*; + use Attribute::*; + use AttributeInfo::*; + use ByNameAttribute::*; + use CallPackageVariant::*; + use NonByNameAttribute::*; - let check_result = match attribute_value { - // The attribute succeeds evaluation and is NOT defined in pkgs/by-name - NonByName(EvalSuccess(attribute_info)) => { - let uses_by_name = match attribute_info { - // In these cases the package doesn't qualify for being in pkgs/by-name, - // so the UsesByName ratchet is already as tight as it can be - NonAttributeSet => Success(NonApplicable), - NonCallPackage => Success(NonApplicable), - // This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile - // is used for a package outside `pkgs/by-name` - CallPackage(CallPackageInfo { - call_package_variant: Auto, - .. - }) => { - // With the current detection mechanism, this also triggers for aliases - // to pkgs/by-name packages, and there's no good method of - // distinguishing alias vs non-alias. - // Using `config.allowAliases = false` at least currently doesn't work - // because there's nothing preventing people from defining aliases that - // are present even with that disabled. - // In the future we could kind of abuse this behavior to have better - // enforcement of conditional aliases, but for now we just need to not - // give an error. - Success(NonApplicable) - } - // Only derivations can be in pkgs/by-name, - // so this attribute doesn't qualify - CallPackage(CallPackageInfo { - is_derivation: false, - .. - }) => Success(NonApplicable), - // A location of None indicates something weird, we can't really know where - // this attribute is defined, probably an alias - CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight), - // The case of an attribute that qualifies: - // - Uses callPackage - // - Is a derivation - CallPackage(CallPackageInfo { - is_derivation: true, - call_package_variant: Manual { .. }, - location: Some(location), - }) => - // We'll use the attribute's location to parse the file that defines it - match nix_file_cache.get(&location.file)? { - Err(error) => - // This is a bad sign for rnix, because it means cpp Nix could parse - // something that rnix couldn't - anyhow::bail!( - "Could not parse file {} with rnix, even though it parsed with cpp Nix: {error}", - location.file.display() - ), - Ok(nix_file) => - match nix_file.call_package_argument_info_at( - location.line, - location.column, - nixpkgs_path, - )? { + let check_result = match attribute_value { + // The attribute succeeds evaluation and is NOT defined in pkgs/by-name + NonByName(EvalSuccess(attribute_info)) => { + let uses_by_name = match attribute_info { + // In these cases the package doesn't qualify for being in pkgs/by-name, + // so the UsesByName ratchet is already as tight as it can be + NonAttributeSet => Success(NonApplicable), + NonCallPackage => Success(NonApplicable), + // This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile + // is used for a package outside `pkgs/by-name` + CallPackage(CallPackageInfo { + call_package_variant: Auto, + .. + }) => { + // With the current detection mechanism, this also triggers for aliases + // to pkgs/by-name packages, and there's no good method of + // distinguishing alias vs non-alias. + // Using `config.allowAliases = false` at least currently doesn't work + // because there's nothing preventing people from defining aliases that + // are present even with that disabled. + // In the future we could kind of abuse this behavior to have better + // enforcement of conditional aliases, but for now we just need to not + // give an error. + Success(NonApplicable) + } + // Only derivations can be in pkgs/by-name, + // so this attribute doesn't qualify + CallPackage(CallPackageInfo { + is_derivation: false, + .. + }) => Success(NonApplicable), + // A location of None indicates something weird, we can't really know where + // this attribute is defined, probably an alias + CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight), + // The case of an attribute that qualifies: + // - Uses callPackage + // - Is a derivation + CallPackage(CallPackageInfo { + is_derivation: true, + call_package_variant: Manual { .. }, + location: Some(location), + }) => + // We'll use the attribute's location to parse the file that defines it + { + match nix_file_store + .get(&location.file)? + .call_package_argument_info_at( + location.line, + location.column, + nixpkgs_path, + )? { + // If the definition is not of the form ` = callPackage ;`, + // it's generally not possible to migrate to `pkgs/by-name` None => Success(NonApplicable), Some(call_package_argument_info) => { - if let Some(ref rel_path) = call_package_argument_info.relative_path { + if let Some(ref rel_path) = + call_package_argument_info.relative_path + { if rel_path.starts_with(utils::BASE_SUBPATH) { // Package variants of by-name packages are explicitly allowed according to RFC 140 // https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants: @@ -244,100 +244,104 @@ pub fn check_values( } else { Success(Loose(call_package_argument_info)) } - }, - }, - }, - }; - uses_by_name.map(|x| ratchet::Package { - manual_definition: Tight, - uses_by_name: x, - }) - } - NonByName(EvalFailure) => { - // We don't know anything about this attribute really - Success(ratchet::Package { - // We'll assume that we can't remove any manual definitions, which has the - // minimal drawback that if there was a manual definition that could've - // been removed, fixing the package requires removing the definition, no - // big deal, that's a minor edit. - manual_definition: Tight, + } + } + } + }; + uses_by_name.map(|x| ratchet::Package { + manual_definition: Tight, + uses_by_name: x, + }) + } + NonByName(EvalFailure) => { + // We don't know anything about this attribute really + Success(ratchet::Package { + // We'll assume that we can't remove any manual definitions, which has the + // minimal drawback that if there was a manual definition that could've + // been removed, fixing the package requires removing the definition, no + // big deal, that's a minor edit. + manual_definition: Tight, - // Regarding whether this attribute could `pkgs/by-name`, we don't really - // know, so return NonApplicable, which has the effect that if a - // package evaluation gets broken temporarily, the fix can remove it from - // pkgs/by-name again. For now this isn't our problem, but in the future we - // might have another check to enforce that evaluation must not be broken. - // The alternative of assuming that it's using `pkgs/by-name` already - // has the problem that if a package evaluation gets broken temporarily, - // fixing it requires a move to pkgs/by-name, which could happen more - // often and isn't really justified. - uses_by_name: NonApplicable, - }) - } - ByName(Missing) => NixpkgsProblem::UndefinedAttr { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(NonAttributeSet)) => NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(NonCallPackage)) => NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(CallPackage(CallPackageInfo { - is_derivation, - call_package_variant, - .. - }))) => { - let check_result = if !is_derivation { - NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into() - } else { - Success(()) - }; + // Regarding whether this attribute could `pkgs/by-name`, we don't really + // know, so return NonApplicable, which has the effect that if a + // package evaluation gets broken temporarily, the fix can remove it from + // pkgs/by-name again. For now this isn't our problem, but in the future we + // might have another check to enforce that evaluation must not be broken. + // The alternative of assuming that it's using `pkgs/by-name` already + // has the problem that if a package evaluation gets broken temporarily, + // fixing it requires a move to pkgs/by-name, which could happen more + // often and isn't really justified. + uses_by_name: NonApplicable, + }) + } + ByName(Missing) => NixpkgsProblem::UndefinedAttr { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + ByName(Existing(NonAttributeSet)) => NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + ByName(Existing(NonCallPackage)) => NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into(), + ByName(Existing(CallPackage(CallPackageInfo { + is_derivation, + call_package_variant, + .. + }))) => { + let check_result = if !is_derivation { + NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into() + } else { + Success(()) + }; - check_result.and(match &call_package_variant { - Auto => Success(ratchet::Package { - manual_definition: Tight, - uses_by_name: Tight, - }), - // TODO: Use the call_package_argument_info_at instead/additionally and - // simplify the eval.nix code - Manual { path, empty_arg } => { - let correct_file = if let Some(call_package_path) = path { - relative_package_file == *call_package_path - } else { - false - }; + check_result.and(match &call_package_variant { + Auto => Success(ratchet::Package { + manual_definition: Tight, + uses_by_name: Tight, + }), + // TODO: Use the call_package_argument_info_at instead/additionally and + // simplify the eval.nix code + Manual { path, empty_arg } => { + let correct_file = if let Some(call_package_path) = path { + relative_package_file == *call_package_path + } else { + false + }; - if correct_file { - Success(ratchet::Package { - // Empty arguments for non-auto-called packages are not allowed anymore. - manual_definition: if *empty_arg { Loose(()) } else { Tight }, - uses_by_name: Tight, - }) - } else { - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), + if correct_file { + Success(ratchet::Package { + // Empty arguments for non-auto-called packages are not allowed anymore. + manual_definition: if *empty_arg { + Loose(()) + } else { + Tight + }, + uses_by_name: Tight, + }) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: attribute_name.clone(), + } + .into() } - .into() } - } - }) - } - }; - Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) - }) - .collect_vec()?, + }) + } + }; + Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) + }) + .collect_vec()?, ); Ok(check_result.map(|elems| ratchet::Nixpkgs { diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 9a8a898a21be1..0d0ddcd7e6327 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,4 +1,4 @@ -use crate::nix_file::NixFileCache; +use crate::nix_file::NixFileStore; mod eval; mod nix_file; mod nixpkgs_problem; @@ -118,7 +118,7 @@ pub fn check_nixpkgs( keep_nix_path: bool, error_writer: &mut W, ) -> validation::Result { - let mut nix_file_cache = NixFileCache::new(); + let mut nix_file_store = NixFileStore::default(); Ok({ let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| { @@ -136,9 +136,9 @@ pub fn check_nixpkgs( )?; Success(ratchet::Nixpkgs::default()) } else { - check_structure(&nixpkgs_path, &mut nix_file_cache)?.result_map(|package_names| + check_structure(&nixpkgs_path, &mut nix_file_store)?.result_map(|package_names| // Only if we could successfully parse the structure, we do the evaluation checks - eval::check_values(&nixpkgs_path, &mut nix_file_cache, package_names, keep_nix_path))? + eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names, keep_nix_path))? } }) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs index e550f5c459f54..836c5e2dcdda5 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs @@ -15,26 +15,20 @@ use std::fs::read_to_string; use std::path::Path; use std::path::PathBuf; -/// A structure to indefinitely cache parse results of Nix files in memory, +/// A structure to store parse results of Nix files in memory, /// making sure that the same file never has to be parsed twice -pub struct NixFileCache { - entries: HashMap, +#[derive(Default)] +pub struct NixFileStore { + entries: HashMap, } -impl NixFileCache { - /// Creates a new empty NixFileCache - pub fn new() -> NixFileCache { - NixFileCache { - entries: HashMap::new(), - } - } - - /// Get the cache entry for a Nix file if it exists, otherwise parse the file, insert it into - /// the cache, and return the value +impl NixFileStore { + /// Get the store entry for a Nix file if it exists, otherwise parse the file, insert it into + /// the store, and return the value /// /// Note that this function only gives an anyhow::Result::Err for I/O errors. /// A parse error is anyhow::Result::Ok(Result::Err(error)) - pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFileResult> { + pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFile> { match self.entries.entry(path.to_owned()) { Entry::Occupied(entry) => Ok(entry.into_mut()), Entry::Vacant(entry) => Ok(entry.insert(NixFile::new(path)?)), @@ -42,9 +36,6 @@ impl NixFileCache { } } -/// A type to represent the result when trying to initialise a `NixFile`. -type NixFileResult = Result; - /// A structure for storing a successfully parsed Nix file pub struct NixFile { /// The parent directory of the Nix file, for more convenient error handling @@ -57,7 +48,7 @@ pub struct NixFile { impl NixFile { /// Creates a new NixFile, failing for I/O or parse errors - fn new(path: impl AsRef) -> anyhow::Result { + fn new(path: impl AsRef) -> anyhow::Result { let Some(parent_dir) = path.as_ref().parent() else { anyhow::bail!("Could not get parent of path {}", path.as_ref().display()) }; @@ -66,14 +57,21 @@ impl NixFile { .with_context(|| format!("Could not read file {}", path.as_ref().display()))?; let line_index = LineIndex::new(&contents); - Ok(rnix::Root::parse(&contents) + // NOTE: There's now another Nixpkgs CI check to make sure all changed Nix files parse + // correctly, though that uses mainline Nix instead of rnix, so it doesn't give the same + // errors. In the future we should unify these two checks, ideally moving the other CI + // check into this tool as well and checking for both mainline Nix and rnix. + rnix::Root::parse(&contents) + // rnix's ::ok returns Result<_, _> , so no error is thrown away like it would be with + // std::result's ::ok .ok() .map(|syntax_root| NixFile { parent_dir: parent_dir.to_path_buf(), path: path.as_ref().to_owned(), syntax_root, line_index, - })) + }) + .with_context(|| format!("Could not parse file {} with rnix", path.as_ref().display())) } } @@ -88,7 +86,11 @@ pub struct CallPackageArgumentInfo { impl NixFile { /// Returns information about callPackage arguments for an attribute at a specific line/column - /// index. If there's no guaranteed `callPackage` at that location, `None` is returned. + /// index. + /// If the location is not of the form ` = callPackage ;`, `None` is + /// returned. + /// This function only returns `Err` for problems that can't be caused by the Nix contents, + /// but rather problems in this programs code itself. /// /// This is meant to be used with the location returned from `builtins.unsafeGetAttrPos`, e.g.: /// - Create file `default.nix` with contents @@ -102,7 +104,7 @@ impl NixFile { /// builtins.unsafeGetAttrPos "foo" (import ./default.nix { }) /// ``` /// results in `{ file = ./default.nix; line = 2; column = 3; }` - /// - Get the NixFile for `.file` from a `NixFileCache` + /// - Get the NixFile for `.file` from a `NixFileStore` /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current directory /// /// You'll get back @@ -165,6 +167,7 @@ impl NixFile { }; if attrpath_node.kind() != SyntaxKind::NODE_ATTRPATH { + // This can happen for e.g. `inherit foo`, so definitely not a syntactic `callPackage` return Ok(None); } // attrpath_node looks like "foo.bar" @@ -183,7 +186,9 @@ impl NixFile { } // attrpath_value_node looks like "foo.bar = 10;" - // unwrap is fine because we confirmed that we can cast with the above check + // unwrap is fine because we confirmed that we can cast with the above check. + // We could avoid this `unwrap` for a `clone`, since `cast` consumes the argument, + // but we still need it for the error message when the cast fails. Ok(Some(ast::AttrpathValue::cast(attrpath_value_node).unwrap())) } @@ -272,9 +277,8 @@ impl NixFile { // Check that is an identifier, or an attribute path with an identifier at the end let ident = match function2 { - Expr::Ident(ident) => - // This means it's something like `foo = callPackage ` - { + Expr::Ident(ident) => { + // This means it's something like `foo = callPackage ` ident } Expr::Select(select) => { @@ -340,13 +344,12 @@ pub enum ResolvedPath { impl NixFile { /// Statically resolves a Nix path expression and checks that it's within an absolute path - /// path /// /// E.g. for the path expression `./bar.nix` in `./foo.nix` and an absolute path of the /// current directory, the function returns `ResolvedPath::Within(./bar.nix)` pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath { if node.parts().count() != 1 { - // Only if there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`. + // If there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`. return ResolvedPath::Interpolated; } @@ -364,9 +367,8 @@ impl NixFile { // That should be checked more strictly match self.parent_dir.join(Path::new(&text)).canonicalize() { Err(resolution_error) => ResolvedPath::Unresolvable(resolution_error), - Ok(resolved) => - // Check if it's within relative_to - { + Ok(resolved) => { + // Check if it's within relative_to match resolved.strip_prefix(relative_to) { Err(_prefix_error) => ResolvedPath::Outside, Ok(suffix) => ResolvedPath::Within(suffix.to_path_buf()), @@ -411,7 +413,7 @@ mod tests { std::fs::write(&file, contents)?; - let nix_file = NixFile::new(&file)??; + let nix_file = NixFile::new(&file)?; // These are builtins.unsafeGetAttrPos locations for the attributes let cases = [ @@ -454,7 +456,7 @@ mod tests { std::fs::write(&file, contents)?; - let nix_file = NixFile::new(&file)??; + let nix_file = NixFile::new(&file)?; let cases = [ (2, None), diff --git a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs index 16ea65deebfce..25e3ef4863e41 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs @@ -1,6 +1,5 @@ use crate::structure; use crate::utils::PACKAGE_NIX_FILENAME; -use rnix::parser::ParseError; use std::ffi::OsString; use std::fmt; use std::io; @@ -58,11 +57,6 @@ pub enum NixpkgsProblem { subpath: PathBuf, io_error: io::Error, }, - CouldNotParseNix { - relative_package_dir: PathBuf, - subpath: PathBuf, - error: ParseError, - }, PathInterpolation { relative_package_dir: PathBuf, subpath: PathBuf, @@ -184,14 +178,6 @@ impl fmt::Display for NixpkgsProblem { relative_package_dir.display(), subpath.display(), ), - NixpkgsProblem::CouldNotParseNix { relative_package_dir, subpath, error } => - write!( - f, - "{}: File {} could not be parsed by rnix: {}", - relative_package_dir.display(), - subpath.display(), - error, - ), NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } => write!( f, diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs index 5ff8cf5168826..169e996300baa 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/references.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs @@ -1,7 +1,7 @@ use crate::nixpkgs_problem::NixpkgsProblem; use crate::utils; use crate::validation::{self, ResultIteratorExt, Validation::Success}; -use crate::NixFileCache; +use crate::NixFileStore; use anyhow::Context; use rowan::ast::AstNode; @@ -11,17 +11,20 @@ use std::path::Path; /// Check that every package directory in pkgs/by-name doesn't link to outside that directory. /// Both symlinks and Nix path expressions are checked. pub fn check_references( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, relative_package_dir: &Path, absolute_package_dir: &Path, ) -> validation::Result<()> { - // The empty argument here is the subpath under the package directory to check - // An empty one means the package directory itself + // The first subpath to check is the package directory itself, which we can represent as an + // empty path, since the absolute package directory gets prepended to this. + // We don't use `./.` to keep the error messages cleaner + // (there's no canonicalisation going on underneath) + let subpath = Path::new(""); check_path( - nix_file_cache, + nix_file_store, relative_package_dir, absolute_package_dir, - Path::new(""), + subpath, ) .with_context(|| { format!( @@ -32,8 +35,12 @@ pub fn check_references( } /// Checks for a specific path to not have references outside +/// +/// The subpath is the relative path within the package directory we're currently checking. +/// A relative path so that the error messages don't get absolute paths (which are messy in CI). +/// The absolute package directory gets prepended before doing anything with it though. fn check_path( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, relative_package_dir: &Path, absolute_package_dir: &Path, subpath: &Path, @@ -69,23 +76,22 @@ fn check_path( utils::read_dir_sorted(&path)? .into_iter() .map(|entry| { - let entry_subpath = subpath.join(entry.file_name()); check_path( - nix_file_cache, + nix_file_store, relative_package_dir, absolute_package_dir, - &entry_subpath, + &subpath.join(entry.file_name()), ) - .with_context(|| format!("Error while recursing into {}", subpath.display())) }) - .collect_vec()?, + .collect_vec() + .with_context(|| format!("Error while recursing into {}", subpath.display()))?, ) } else if path.is_file() { // Only check Nix files if let Some(ext) = path.extension() { if ext == OsStr::new("nix") { check_nix_file( - nix_file_cache, + nix_file_store, relative_package_dir, absolute_package_dir, subpath, @@ -106,29 +112,14 @@ fn check_path( /// Check whether a nix file contains path expression references pointing outside the package /// directory fn check_nix_file( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, relative_package_dir: &Path, absolute_package_dir: &Path, subpath: &Path, ) -> validation::Result<()> { let path = absolute_package_dir.join(subpath); - let nix_file = match nix_file_cache.get(&path)? { - Ok(nix_file) => nix_file, - Err(error) => - // NOTE: There's now another Nixpkgs CI check to make sure all changed Nix files parse - // correctly, though that uses mainline Nix instead of rnix, so it doesn't give the same - // errors. In the future we should unify these two checks, ideally moving the other CI - // check into this tool as well and checking for both mainline Nix and rnix. - { - return Ok(NixpkgsProblem::CouldNotParseNix { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - error: error.clone(), - } - .into()) - } - }; + let nix_file = nix_file_store.get(&path)?; Ok(validation::sequence_( nix_file.syntax_root.syntax().descendants().map(|node| { @@ -140,40 +131,31 @@ fn check_nix_file( return Success(()); }; - use crate::nix_file::ResolvedPath::*; + use crate::nix_file::ResolvedPath; match nix_file.static_resolve_path(path, absolute_package_dir) { - Interpolated => - // Filters out ./foo/${bar}/baz - // TODO: We can just check ./foo - { - NixpkgsProblem::PathInterpolation { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into() + ResolvedPath::Interpolated => NixpkgsProblem::PathInterpolation { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, } - SearchPath => - // Filters out search paths like - { - NixpkgsProblem::SearchPath { - relative_package_dir: relative_package_dir.to_path_buf(), - subpath: subpath.to_path_buf(), - line, - text, - } - .into() + .into(), + ResolvedPath::SearchPath => NixpkgsProblem::SearchPath { + relative_package_dir: relative_package_dir.to_path_buf(), + subpath: subpath.to_path_buf(), + line, + text, } - Outside => NixpkgsProblem::OutsidePathReference { + .into(), + ResolvedPath::Outside => NixpkgsProblem::OutsidePathReference { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, text, } .into(), - Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { + ResolvedPath::Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference { relative_package_dir: relative_package_dir.to_path_buf(), subpath: subpath.to_path_buf(), line, @@ -181,10 +163,9 @@ fn check_nix_file( io_error: e, } .into(), - Within(_p) => - // No need to handle the case of it being inside the directory, since we scan through the - // entire directory recursively anyways - { + ResolvedPath::Within(..) => { + // No need to handle the case of it being inside the directory, since we scan through the + // entire directory recursively anyways Success(()) } } diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs index 7f9e7d4f717f7..9b615dd9969ac 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs @@ -3,7 +3,7 @@ use crate::references; use crate::utils; use crate::utils::{BASE_SUBPATH, PACKAGE_NIX_FILENAME}; use crate::validation::{self, ResultIteratorExt, Validation::Success}; -use crate::NixFileCache; +use crate::NixFileStore; use itertools::concat; use lazy_static::lazy_static; use regex::Regex; @@ -37,7 +37,7 @@ pub fn relative_file_for_package(package_name: &str) -> PathBuf { /// `pkgs/by-name` pub fn check_structure( path: &Path, - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, ) -> validation::Result> { let base_dir = path.join(BASE_SUBPATH); @@ -93,7 +93,7 @@ pub fn check_structure( .into_iter() .map(|package_entry| { check_package( - nix_file_cache, + nix_file_store, path, &shard_name, shard_name_valid, @@ -112,7 +112,7 @@ pub fn check_structure( } fn check_package( - nix_file_cache: &mut NixFileCache, + nix_file_store: &mut NixFileStore, path: &Path, shard_name: &str, shard_name_valid: bool, @@ -172,7 +172,7 @@ fn check_package( }); let result = result.and(references::check_references( - nix_file_cache, + nix_file_store, &relative_package_dir, &path.join(&relative_package_dir), )?); From 474e7e7040125eb0768057ef149e50915de63371 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 5 Feb 2024 01:35:23 +0100 Subject: [PATCH 7/7] tests.nixpkgs-check-by-name: Refactor eval.rs There was too much indentation! --- pkgs/test/nixpkgs-check-by-name/src/eval.rs | 378 +++++++++++--------- 1 file changed, 202 insertions(+), 176 deletions(-) diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index bdde0e560057f..cb49c3acbef30 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -159,193 +159,219 @@ pub fn check_values( attributes .into_iter() .map(|(attribute_name, attribute_value)| { - let relative_package_file = structure::relative_file_for_package(&attribute_name); - - use ratchet::RatchetState::*; - use Attribute::*; - use AttributeInfo::*; - use ByNameAttribute::*; - use CallPackageVariant::*; - use NonByNameAttribute::*; - let check_result = match attribute_value { - // The attribute succeeds evaluation and is NOT defined in pkgs/by-name - NonByName(EvalSuccess(attribute_info)) => { - let uses_by_name = match attribute_info { - // In these cases the package doesn't qualify for being in pkgs/by-name, - // so the UsesByName ratchet is already as tight as it can be - NonAttributeSet => Success(NonApplicable), - NonCallPackage => Success(NonApplicable), - // This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile - // is used for a package outside `pkgs/by-name` - CallPackage(CallPackageInfo { - call_package_variant: Auto, - .. - }) => { - // With the current detection mechanism, this also triggers for aliases - // to pkgs/by-name packages, and there's no good method of - // distinguishing alias vs non-alias. - // Using `config.allowAliases = false` at least currently doesn't work - // because there's nothing preventing people from defining aliases that - // are present even with that disabled. - // In the future we could kind of abuse this behavior to have better - // enforcement of conditional aliases, but for now we just need to not - // give an error. - Success(NonApplicable) - } - // Only derivations can be in pkgs/by-name, - // so this attribute doesn't qualify - CallPackage(CallPackageInfo { - is_derivation: false, - .. - }) => Success(NonApplicable), - // A location of None indicates something weird, we can't really know where - // this attribute is defined, probably an alias - CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight), - // The case of an attribute that qualifies: - // - Uses callPackage - // - Is a derivation - CallPackage(CallPackageInfo { - is_derivation: true, - call_package_variant: Manual { .. }, - location: Some(location), - }) => - // We'll use the attribute's location to parse the file that defines it - { - match nix_file_store - .get(&location.file)? - .call_package_argument_info_at( - location.line, - location.column, - nixpkgs_path, - )? { - // If the definition is not of the form ` = callPackage ;`, - // it's generally not possible to migrate to `pkgs/by-name` - None => Success(NonApplicable), - Some(call_package_argument_info) => { - if let Some(ref rel_path) = - call_package_argument_info.relative_path - { - if rel_path.starts_with(utils::BASE_SUBPATH) { - // Package variants of by-name packages are explicitly allowed according to RFC 140 - // https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants: - // - // foo-variant = callPackage ../by-name/fo/foo/package.nix { - // someFlag = true; - // } - // - // While such definitions could be moved to `pkgs/by-name` by using - // `.override { someFlag = true; }` instead, this changes the semantics in - // relation with overlays. - Success(NonApplicable) - } else { - Success(Loose(call_package_argument_info)) - } - } else { - Success(Loose(call_package_argument_info)) - } - } - } - } - }; - uses_by_name.map(|x| ratchet::Package { - manual_definition: Tight, - uses_by_name: x, - }) + Attribute::NonByName(non_by_name_attribute) => handle_non_by_name_attribute( + nixpkgs_path, + nix_file_store, + non_by_name_attribute, + )?, + Attribute::ByName(by_name_attribute) => { + by_name(&attribute_name, by_name_attribute) } - NonByName(EvalFailure) => { - // We don't know anything about this attribute really - Success(ratchet::Package { - // We'll assume that we can't remove any manual definitions, which has the - // minimal drawback that if there was a manual definition that could've - // been removed, fixing the package requires removing the definition, no - // big deal, that's a minor edit. - manual_definition: Tight, + }; + Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) + }) + .collect_vec()?, + ); + + Ok(check_result.map(|elems| ratchet::Nixpkgs { + package_names: elems.iter().map(|(name, _)| name.to_owned()).collect(), + package_map: elems.into_iter().collect(), + })) +} + +/// Handles the evaluation result for an attribute in `pkgs/by-name`, +/// turning it into a validation result. +fn by_name( + attribute_name: &str, + by_name_attribute: ByNameAttribute, +) -> validation::Validation { + use ratchet::RatchetState::*; + use AttributeInfo::*; + use ByNameAttribute::*; + use CallPackageVariant::*; + + let relative_package_file = structure::relative_file_for_package(attribute_name); + + match by_name_attribute { + Missing => NixpkgsProblem::UndefinedAttr { + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into(), + Existing(NonAttributeSet) => NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into(), + Existing(NonCallPackage) => NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into(), + Existing(CallPackage(CallPackageInfo { + is_derivation, + call_package_variant, + .. + })) => { + let check_result = if !is_derivation { + NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into() + } else { + Success(()) + }; + + check_result.and(match &call_package_variant { + Auto => Success(ratchet::Package { + manual_definition: Tight, + uses_by_name: Tight, + }), + // TODO: Use the call_package_argument_info_at instead/additionally and + // simplify the eval.nix code + Manual { path, empty_arg } => { + let correct_file = if let Some(call_package_path) = path { + relative_package_file == *call_package_path + } else { + false + }; - // Regarding whether this attribute could `pkgs/by-name`, we don't really - // know, so return NonApplicable, which has the effect that if a - // package evaluation gets broken temporarily, the fix can remove it from - // pkgs/by-name again. For now this isn't our problem, but in the future we - // might have another check to enforce that evaluation must not be broken. - // The alternative of assuming that it's using `pkgs/by-name` already - // has the problem that if a package evaluation gets broken temporarily, - // fixing it requires a move to pkgs/by-name, which could happen more - // often and isn't really justified. - uses_by_name: NonApplicable, + if correct_file { + Success(ratchet::Package { + // Empty arguments for non-auto-called packages are not allowed anymore. + manual_definition: if *empty_arg { Loose(()) } else { Tight }, + uses_by_name: Tight, }) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.to_owned(), + package_name: attribute_name.to_owned(), + } + .into() } - ByName(Missing) => NixpkgsProblem::UndefinedAttr { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(NonAttributeSet)) => NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(NonCallPackage)) => NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into(), - ByName(Existing(CallPackage(CallPackageInfo { - is_derivation, - call_package_variant, - .. - }))) => { - let check_result = if !is_derivation { - NixpkgsProblem::NonDerivation { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into() - } else { - Success(()) - }; + } + }) + } + } +} - check_result.and(match &call_package_variant { - Auto => Success(ratchet::Package { - manual_definition: Tight, - uses_by_name: Tight, - }), - // TODO: Use the call_package_argument_info_at instead/additionally and - // simplify the eval.nix code - Manual { path, empty_arg } => { - let correct_file = if let Some(call_package_path) = path { - relative_package_file == *call_package_path - } else { - false - }; +/// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`, +/// turning it into a validation result. +fn handle_non_by_name_attribute( + nixpkgs_path: &Path, + nix_file_store: &mut NixFileStore, + non_by_name_attribute: NonByNameAttribute, +) -> validation::Result { + use ratchet::RatchetState::*; + use AttributeInfo::*; + use CallPackageVariant::*; + use NonByNameAttribute::*; - if correct_file { - Success(ratchet::Package { - // Empty arguments for non-auto-called packages are not allowed anymore. - manual_definition: if *empty_arg { - Loose(()) - } else { - Tight - }, - uses_by_name: Tight, - }) + Ok(match non_by_name_attribute { + // The attribute succeeds evaluation and is NOT defined in pkgs/by-name + EvalSuccess(attribute_info) => { + let uses_by_name = match attribute_info { + // In these cases the package doesn't qualify for being in pkgs/by-name, + // so the UsesByName ratchet is already as tight as it can be + NonAttributeSet => Success(NonApplicable), + NonCallPackage => Success(NonApplicable), + // This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile + // is used for a package outside `pkgs/by-name` + CallPackage(CallPackageInfo { + call_package_variant: Auto, + .. + }) => { + // With the current detection mechanism, this also triggers for aliases + // to pkgs/by-name packages, and there's no good method of + // distinguishing alias vs non-alias. + // Using `config.allowAliases = false` at least currently doesn't work + // because there's nothing preventing people from defining aliases that + // are present even with that disabled. + // In the future we could kind of abuse this behavior to have better + // enforcement of conditional aliases, but for now we just need to not + // give an error. + Success(NonApplicable) + } + // Only derivations can be in pkgs/by-name, + // so this attribute doesn't qualify + CallPackage(CallPackageInfo { + is_derivation: false, + .. + }) => Success(NonApplicable), + // A location of None indicates something weird, we can't really know where + // this attribute is defined, probably an alias + CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight), + // The case of an attribute that qualifies: + // - Uses callPackage + // - Is a derivation + CallPackage(CallPackageInfo { + is_derivation: true, + call_package_variant: Manual { .. }, + location: Some(location), + }) => + // We'll use the attribute's location to parse the file that defines it + { + match nix_file_store + .get(&location.file)? + .call_package_argument_info_at( + location.line, + location.column, + nixpkgs_path, + )? { + // If the definition is not of the form ` = callPackage ;`, + // it's generally not possible to migrate to `pkgs/by-name` + None => Success(NonApplicable), + Some(call_package_argument_info) => { + if let Some(ref rel_path) = call_package_argument_info.relative_path { + if rel_path.starts_with(utils::BASE_SUBPATH) { + // Package variants of by-name packages are explicitly allowed according to RFC 140 + // https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants: + // + // foo-variant = callPackage ../by-name/fo/foo/package.nix { + // someFlag = true; + // } + // + // While such definitions could be moved to `pkgs/by-name` by using + // `.override { someFlag = true; }` instead, this changes the semantics in + // relation with overlays. + Success(NonApplicable) } else { - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: attribute_name.clone(), - } - .into() + Success(Loose(call_package_argument_info)) } + } else { + Success(Loose(call_package_argument_info)) } - }) + } } - }; - Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value))) + } + }; + uses_by_name.map(|x| ratchet::Package { + manual_definition: Tight, + uses_by_name: x, }) - .collect_vec()?, - ); + } + EvalFailure => { + // We don't know anything about this attribute really + Success(ratchet::Package { + // We'll assume that we can't remove any manual definitions, which has the + // minimal drawback that if there was a manual definition that could've + // been removed, fixing the package requires removing the definition, no + // big deal, that's a minor edit. + manual_definition: Tight, - Ok(check_result.map(|elems| ratchet::Nixpkgs { - package_names: elems.iter().map(|(name, _)| name.to_owned()).collect(), - package_map: elems.into_iter().collect(), - })) + // Regarding whether this attribute could `pkgs/by-name`, we don't really + // know, so return NonApplicable, which has the effect that if a + // package evaluation gets broken temporarily, the fix can remove it from + // pkgs/by-name again. For now this isn't our problem, but in the future we + // might have another check to enforce that evaluation must not be broken. + // The alternative of assuming that it's using `pkgs/by-name` already + // has the problem that if a package evaluation gets broken temporarily, + // fixing it requires a move to pkgs/by-name, which could happen more + // often and isn't really justified. + uses_by_name: NonApplicable, + }) + } + }) }