tests.nixpkgs-check-by-name: Intermediate refactor

This prepares the code base for the removal of the `--version` flag, to
be replaced with a flag that can specify a base version to compare the
main Nixpkgs against, in order to have gradual transitions to stricter
checks.

This refactoring does:
- Introduce the `version` module that can house the logic to increase
  strictness, with a `version::Nixpkgs` struct that contains the
  strictness conformity of a single Nixpkgs version
- Make the check return `version::Nixpkgs`
- Handle the behavior of the still-existing `--version` flag with `version::Nixpkgs`
- Introduce an intermediate `process` function to handle the top-level
  logic, especially useful in the next commit

3 files changed, 139 insertions, 40 deletions

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 08dc243359d52..face1117f6435 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -1,7 +1,7 @@
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::structure;
 use crate::validation::{self, Validation::Success};
-use crate::Version;
+use crate::version;
 use std::path::Path;
 
 use anyhow::Context;
@@ -39,11 +39,10 @@ enum AttributeVariant {
 /// of the form `callPackage <package_file> { ... }`.
 /// See the `eval.nix` file for how this is achieved on the Nix side
 pub fn check_values(
-    version: Version,
     nixpkgs_path: &Path,
     package_names: Vec<String>,
     eval_accessible_paths: Vec<&Path>,
-) -> validation::Result<()> {
+) -> validation::Result<version::Nixpkgs> {
     // Write the list of packages we need to check into a temporary JSON file.
     // This can then get read by the Nix evaluation.
     let attrs_file = NamedTempFile::new().context("Failed to create a temporary file")?;
@@ -110,8 +109,8 @@ pub fn check_values(
             String::from_utf8_lossy(&result.stdout)
         ))?;
 
-    Ok(validation::sequence_(package_names.iter().map(
-        |package_name| {
+    Ok(
+        validation::sequence(package_names.iter().map(|package_name| {
             let relative_package_file = structure::relative_file_for_package(package_name);
             let absolute_package_file = nixpkgs_path.join(&relative_package_file);
 
@@ -126,23 +125,25 @@ pub fn check_values(
                     Success(())
                 };
 
-                check_result.and(match &attribute_info.variant {
-                    AttributeVariant::AutoCalled => Success(()),
+                let check_result = check_result.and(match &attribute_info.variant {
+                    AttributeVariant::AutoCalled => Success(version::Attribute {
+                        empty_non_auto_called: version::EmptyNonAutoCalled::Valid,
+                    }),
                     AttributeVariant::CallPackage { path, empty_arg } => {
                         let correct_file = if let Some(call_package_path) = path {
                             absolute_package_file == *call_package_path
                         } else {
                             false
                         };
-                        // Only check for the argument to be non-empty if the version is V1 or
-                        // higher
-                        let non_empty = if version >= Version::V1 {
-                            !empty_arg
-                        } else {
-                            true
-                        };
-                        if correct_file && non_empty {
-                            Success(())
+
+                        if correct_file {
+                            Success(version::Attribute {
+                                empty_non_auto_called: if *empty_arg {
+                                    version::EmptyNonAutoCalled::Invalid
+                                } else {
+                                    version::EmptyNonAutoCalled::Valid
+                                },
+                            })
                         } else {
                             NixpkgsProblem::WrongCallPackage {
                                 relative_package_file: relative_package_file.clone(),
@@ -156,7 +157,9 @@ pub fn check_values(
                         package_name: package_name.clone(),
                     }
                     .into(),
-                })
+                });
+
+                check_result.map(|value| (package_name.clone(), value))
             } else {
                 NixpkgsProblem::UndefinedAttr {
                     relative_package_file: relative_package_file.clone(),
@@ -164,6 +167,9 @@ pub fn check_values(
                 }
                 .into()
             }
-        },
-    )))
+        }))
+        .map(|elems| version::Nixpkgs {
+            attributes: elems.into_iter().collect(),
+        }),
+    )
 }
diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index 567da00333e6c..981d1134c85a7 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -4,6 +4,7 @@ mod references;
 mod structure;
 mod utils;
 mod validation;
+mod version;
 
 use crate::structure::check_structure;
 use crate::validation::Validation::Failure;
@@ -39,7 +40,7 @@ pub enum Version {
 
 fn main() -> ExitCode {
     let args = Args::parse();
-    match check_nixpkgs(&args.nixpkgs, args.version, vec![], &mut io::stderr()) {
+    match process(&args.nixpkgs, args.version, vec![], &mut io::stderr()) {
         Ok(true) => {
             eprintln!("{}", "Validated successfully".green());
             ExitCode::SUCCESS
@@ -55,7 +56,7 @@ fn main() -> ExitCode {
     }
 }
 
-/// Checks whether the pkgs/by-name structure in Nixpkgs is valid.
+/// Does the actual work. This is the abstraction used both by `main` and the tests.
 ///
 /// # Arguments
 /// - `nixpkgs_path`: The path to the Nixpkgs to check
@@ -68,28 +69,23 @@ fn main() -> ExitCode {
 /// - `Err(e)` if an I/O-related error `e` occurred.
 /// - `Ok(false)` if there are problems, all of which will be written to `error_writer`.
 /// - `Ok(true)` if there are no problems
-pub fn check_nixpkgs<W: io::Write>(
+pub fn process<W: io::Write>(
     nixpkgs_path: &Path,
     version: Version,
     eval_accessible_paths: Vec<&Path>,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
-    let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
-        "Nixpkgs path {} could not be resolved",
-        nixpkgs_path.display()
-    ))?;
-
-    let check_result = if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
-        eprintln!(
-            "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
-            utils::BASE_SUBPATH
-        );
-        Success(())
-    } 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(version, &nixpkgs_path, package_names, eval_accessible_paths))?
-    };
+    let nixpkgs_result = check_nixpkgs(nixpkgs_path, eval_accessible_paths)?;
+    let check_result = nixpkgs_result.result_map(|nixpkgs_version| {
+        let empty_non_auto_called_base = match version {
+            Version::V0 => version::EmptyNonAutoCalled::Invalid,
+            Version::V1 => version::EmptyNonAutoCalled::Valid,
+        };
+        Ok(version::Nixpkgs::compare(
+            &empty_non_auto_called_base,
+            nixpkgs_version,
+        ))
+    })?;
 
     match check_result {
         Failure(errors) => {
@@ -102,9 +98,35 @@ pub fn check_nixpkgs<W: io::Write>(
     }
 }
 
+/// Checks whether the pkgs/by-name structure in Nixpkgs is valid,
+/// and returns to which degree it's valid for checks with increased strictness.
+pub fn check_nixpkgs(
+    nixpkgs_path: &Path,
+    eval_accessible_paths: Vec<&Path>,
+) -> validation::Result<version::Nixpkgs> {
+    Ok({
+        let nixpkgs_path = nixpkgs_path.canonicalize().context(format!(
+            "Nixpkgs path {} could not be resolved",
+            nixpkgs_path.display()
+        ))?;
+
+        if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
+            eprintln!(
+                "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
+                utils::BASE_SUBPATH
+            );
+            Success(version::Nixpkgs::default())
+        } 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, eval_accessible_paths))?
+        }
+    })
+}
+
 #[cfg(test)]
 mod tests {
-    use crate::check_nixpkgs;
+    use crate::process;
     use crate::utils;
     use crate::Version;
     use anyhow::Context;
@@ -195,7 +217,7 @@ mod tests {
         // We don't want coloring to mess up the tests
         let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> {
             let mut writer = vec![];
-            check_nixpkgs(&path, Version::V1, vec![&extra_nix_path], &mut writer)
+            process(&path, Version::V1, vec![&extra_nix_path], &mut writer)
                 .context(format!("Failed test case {name}"))?;
             Ok(writer)
         })?;
diff --git a/pkgs/test/nixpkgs-check-by-name/src/version.rs b/pkgs/test/nixpkgs-check-by-name/src/version.rs
new file mode 100644
index 0000000000000..ab146270241ef
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/src/version.rs
@@ -0,0 +1,71 @@
+use crate::nixpkgs_problem::NixpkgsProblem;
+use crate::structure;
+use crate::validation::{self, Validation, Validation::Success};
+use std::collections::HashMap;
+
+/// The check version conformity of a Nixpkgs path:
+/// When the strictness of the check increases, this structure should be extended to distinguish
+/// between parts that are still valid, and ones that aren't valid anymore.
+#[derive(Default)]
+pub struct Nixpkgs {
+    /// The package attributes tracked in `pkgs/by-name`
+    pub attributes: HashMap<String, Attribute>,
+}
+
+impl Nixpkgs {
+    /// Compares two Nixpkgs versions against each other, returning validation errors only if the
+    /// `from` version satisfied the stricter checks, while the `to` version doesn't satisfy them
+    /// anymore.
+    pub fn compare(empty_non_auto_called_from: &EmptyNonAutoCalled, to: Self) -> Validation<()> {
+        validation::sequence_(
+            // We only loop over the current attributes,
+            // we don't need to check ones that were removed
+            to.attributes.into_iter().map(|(name, attr_to)| {
+                Attribute::compare(&name, empty_non_auto_called_from, &attr_to)
+            }),
+        )
+    }
+}
+
+/// The check version conformity of an attribute defined by `pkgs/by-name`
+pub struct Attribute {
+    pub empty_non_auto_called: EmptyNonAutoCalled,
+}
+
+impl Attribute {
+    pub fn compare(
+        name: &str,
+        empty_non_auto_called_from: &EmptyNonAutoCalled,
+        to: &Self,
+    ) -> Validation<()> {
+        EmptyNonAutoCalled::compare(name, empty_non_auto_called_from, &to.empty_non_auto_called)
+    }
+}
+
+/// Whether an attribute conforms to the new strictness check that
+/// `callPackage ... {}` is not allowed anymore in `all-package.nix`
+#[derive(PartialEq, PartialOrd)]
+pub enum EmptyNonAutoCalled {
+    /// The attribute is not valid anymore with the new check
+    Invalid,
+    /// The attribute is still valid with the new check
+    Valid,
+}
+
+impl EmptyNonAutoCalled {
+    fn compare(
+        name: &str,
+        empty_non_auto_called_from: &EmptyNonAutoCalled,
+        to: &Self,
+    ) -> Validation<()> {
+        if to >= empty_non_auto_called_from {
+            Success(())
+        } else {
+            NixpkgsProblem::WrongCallPackage {
+                relative_package_file: structure::relative_file_for_package(name),
+                package_name: name.to_owned(),
+            }
+            .into()
+        }
+    }
+}