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

@@ -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"

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<String>,
     keep_nix_path: bool,
 ) -> validation::Result<ratchet::Nixpkgs> {
@@ -155,8 +155,10 @@ pub fn check_values(
             )
         })?;
 
-    let check_result = validation::sequence(attributes.into_iter().map(
+    let check_result = validation::sequence(
-        |(attribute_name, attribute_value)| {
+        attributes
+            .into_iter()
+            .map(|(attribute_name, attribute_value)| {
                 let relative_package_file = structure::relative_file_for_package(&attribute_name);
 
                 use ratchet::RatchetState::*;
@@ -209,23 +211,21 @@ pub fn check_values(
                                 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) =>
+                                match nix_file_store
-                                // This is a bad sign for rnix, because it means cpp Nix could parse
+                                    .get(&location.file)?
-                                // something that rnix couldn't
+                                    .call_package_argument_info_at(
-                                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,
                                     )? {
+                                    // If the definition is not of the form `<attr> = callPackage <arg1> <arg2>;`,
+                                    // 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,9 +244,9 @@ pub fn check_values(
                                         } else {
                                             Success(Loose(call_package_argument_info))
                                         }
-                                    },
+                                    }
-                                },
+                                }
-                        },
+                            }
                         };
                         uses_by_name.map(|x| ratchet::Package {
                             manual_definition: Tight,
@@ -321,7 +321,11 @@ pub fn check_values(
                                 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 },
+                                        manual_definition: if *empty_arg {
+                                            Loose(())
+                                        } else {
+                                            Tight
+                                        },
                                         uses_by_name: Tight,
                                     })
                                 } else {

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<W: io::Write>(
     keep_nix_path: bool,
     error_writer: &mut W,
 ) -> validation::Result<ratchet::Nixpkgs> {
-    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<W: io::Write>(
             )?;
             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))?
         }
     })
 }

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 {
+#[derive(Default)]
-    entries: HashMap<PathBuf, NixFileResult>,
+pub struct NixFileStore {
+    entries: HashMap<PathBuf, NixFile>,
 }
 
-impl NixFileCache {
+impl NixFileStore {
-    /// Creates a new empty NixFileCache
+    /// Get the store entry for a Nix file if it exists, otherwise parse the file, insert it into
-    pub fn new() -> NixFileCache {
+    /// the store, and return the value
-        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> {
+    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<NixFile, rnix::parser::ParseError>;
-
 /// 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<Path>) -> anyhow::Result<NixFileResult> {
+    fn new(path: impl AsRef<Path>) -> anyhow::Result<NixFile> {
         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 `<attr> = callPackage <arg1> <arg2>;`, `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 <fun2> is an identifier, or an attribute path with an identifier at the end
         let ident = match function2 {
-            Expr::Ident(ident) =>
+            Expr::Ident(ident) => {
                 // This means it's something like `foo = callPackage <arg2> <arg1>`
-            {
                 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) =>
+            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),

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,

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
+    // The first subpath to check is the package directory itself, which we can represent as an
-    // An empty one means the package directory itself
+    // 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)? {
+    let nix_file = nix_file_store.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())
-        }
-    };
 
     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 =>
+                ResolvedPath::Interpolated => NixpkgsProblem::PathInterpolation {
-                // 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()
-                }
-                SearchPath =>
-                // Filters out search paths like <nixpkgs>
-                {
-                    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(),
-                Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference {
+                ResolvedPath::SearchPath => NixpkgsProblem::SearchPath {
+                    relative_package_dir: relative_package_dir.to_path_buf(),
+                    subpath: subpath.to_path_buf(),
+                    line,
+                    text,
+                }
+                .into(),
+                ResolvedPath::Outside => NixpkgsProblem::OutsidePathReference {
+                    relative_package_dir: relative_package_dir.to_path_buf(),
+                    subpath: subpath.to_path_buf(),
+                    line,
+                    text,
+                }
+                .into(),
+                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) =>
+                ResolvedPath::Within(..) => {
                     // No need to handle the case of it being inside the directory, since we scan through the
                     // entire directory recursively anyways
-                {
                     Success(())
                 }
             }

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<Vec<String>> {
     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),
         )?);