tests.nixpkgs-check-by-name: Refactor eval code and improve comments

Does a bunch of cleanups to the eval.{rs,nix} code to make future
changes easier, no functionality is changed.

2 files changed, 152 insertions, 119 deletions

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix
index bf9f19d8e460..847fab1085d1 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix
@@ -1,11 +1,7 @@
 # Takes a path to nixpkgs and a path to the json-encoded list of attributes to check.
-# Returns an attribute set containing information on each requested attribute.
-# If the attribute is missing from Nixpkgs it's also missing from the result.
-#
-# The returned information is an attribute set with:
-# - call_package_path: The <path> from `<attr> = callPackage <path> { ... }`,
-#   or null if it's not defined as with callPackage, or if the <path> is not a path
-# - is_derivation: The result of `lib.isDerivation <attr>`
+# Returns an value containing information on each requested attribute,
+# which is decoded on the Rust side.
+# See ./eval.rs for the meaning of the returned values
 {
   attrsPath,
   nixpkgsPath,
@@ -13,70 +9,78 @@
 let
   attrs = builtins.fromJSON (builtins.readFile attrsPath);
 
-  # This overlay mocks callPackage to persist the path of the first argument
-  callPackageOverlay = self: super: {
+  # We need access to the `callPackage` arguments of each attribute.
+  # The only way to do so is to override `callPackage` with our own version that adds this information to the result,
+  # and then try to access this information.
+  overlay = final: prev: {
+
+    # Information for attributes defined using `callPackage`
     callPackage = fn: args:
-      let
-        result = super.callPackage fn args;
-        variantInfo._attributeVariant = {
-          # These names are used by the deserializer on the Rust side
-          CallPackage.path =
+      addVariantInfo (prev.callPackage fn args) {
+        Manual = {
+          path =
             if builtins.isPath fn then
               toString fn
             else
               null;
-          CallPackage.empty_arg =
+          empty_arg =
             args == { };
         };
-      in
-      if builtins.isAttrs result then
-        # If this was the last overlay to be applied, we could just only return the `_callPackagePath`,
-        # but that's not the case because stdenv has another overlays on top of user-provided ones.
-        # So to not break the stdenv build we need to return the mostly proper result here
-        result // variantInfo
-      else
-        # It's very rare that callPackage doesn't return an attribute set, but it can occur.
-        variantInfo;
+      };
 
+    # Information for attributes that are auto-called from pkgs/by-name.
+    # This internal attribute is only used by pkgs/by-name
     _internalCallByNamePackageFile = file:
-      let
-        result = super._internalCallByNamePackageFile file;
-        variantInfo._attributeVariant = {
-          # This name is used by the deserializer on the Rust side
-          AutoCalled = null;
-        };
-      in
-      if builtins.isAttrs result then
-        # If this was the last overlay to be applied, we could just only return the `_callPackagePath`,
-        # but that's not the case because stdenv has another overlays on top of user-provided ones.
-        # So to not break the stdenv build we need to return the mostly proper result here
-        result // variantInfo
-      else
-        # It's very rare that callPackage doesn't return an attribute set, but it can occur.
-        variantInfo;
+      addVariantInfo (prev._internalCallByNamePackageFile file) {
+        Auto = null;
+      };
+
   };
 
+  # We can't just replace attribute values with their info in the overlay,
+  # because attributes can depend on other attributes, so this would break evaluation.
+  addVariantInfo = value: variant:
+    if builtins.isAttrs value then
+      value // {
+        _callPackageVariant = variant;
+      }
+    else
+      # It's very rare that callPackage doesn't return an attribute set, but it can occur.
+      # In such a case we can't really return anything sensible that would include the info,
+      # so just don't return the info and let the consumer handle it.
+      value;
+
   pkgs = import nixpkgsPath {
     # Don't let the users home directory influence this result
     config = { };
-    overlays = [ callPackageOverlay ];
+    overlays = [ overlay ];
   };
 
-  attrInfo = attr:
-    let
-      value = pkgs.${attr};
-    in
-    {
-    # These names are used by the deserializer on the Rust side
-    variant = value._attributeVariant or { Other = null; };
-    is_derivation = pkgs.lib.isDerivation value;
-  };
+  attrInfo = name: value:
+    if ! builtins.isAttrs value then
+      {
+        NonAttributeSet = null;
+      }
+    else if ! value ? _callPackageVariant then
+      {
+        NonCallPackage = null;
+      }
+    else
+      {
+        CallPackage = {
+          call_package_variant = value._callPackageVariant;
+          is_derivation = pkgs.lib.isDerivation value;
+        };
+      };
 
   attrInfos = builtins.listToAttrs (map (name: {
     inherit name;
-    value = attrInfo name;
+    value =
+      if ! pkgs ? ${name} then
+        { Missing = null; }
+      else
+        { Existing = attrInfo name pkgs.${name}; };
   }) attrs);
 
 in
-# Filter out attributes not in Nixpkgs
-builtins.intersectAttrs pkgs attrInfos
+attrInfos
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index cd8c70472cf2..4e8fba549386 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -13,26 +13,42 @@ use tempfile::NamedTempFile;
 
 /// Attribute set of this structure is returned by eval.nix
 #[derive(Deserialize)]
-struct AttributeInfo {
-    variant: AttributeVariant,
+enum ByNameAttribute {
+    /// The attribute doesn't exist at all
+    Missing,
+    Existing(AttributeInfo),
+}
+
+#[derive(Deserialize)]
+enum AttributeInfo {
+    /// The attribute exists, but its value isn't an attribute set
+    NonAttributeSet,
+    /// The attribute exists, but its value isn't defined using callPackage
+    NonCallPackage,
+    /// The attribute exists and its value is an attribute set
+    CallPackage(CallPackageInfo),
+}
+
+#[derive(Deserialize)]
+struct CallPackageInfo {
+    call_package_variant: CallPackageVariant,
+    /// Whether the attribute is a derivation (`lib.isDerivation`)
     is_derivation: bool,
 }
 
 #[derive(Deserialize)]
-enum AttributeVariant {
+enum CallPackageVariant {
     /// The attribute is auto-called as pkgs.callPackage using pkgs/by-name,
     /// and it is not overridden by a definition in all-packages.nix
-    AutoCalled,
+    Auto,
     /// The attribute is defined as a pkgs.callPackage <path> <args>,
     /// and overridden by all-packages.nix
-    CallPackage {
+    Manual {
         /// The <path> argument or None if it's not a path
         path: Option<PathBuf>,
         /// true if <args> is { }
         empty_arg: bool,
     },
-    /// The attribute is not defined as pkgs.callPackage
-    Other,
 }
 
 /// Check that the Nixpkgs attribute values corresponding to the packages in pkgs/by-name are
@@ -103,74 +119,87 @@ pub fn check_values(
         anyhow::bail!("Failed to run command {command:?}");
     }
     // Parse the resulting JSON value
-    let actual_files: HashMap<String, AttributeInfo> = serde_json::from_slice(&result.stdout)
+    let attributes: HashMap<String, ByNameAttribute> = serde_json::from_slice(&result.stdout)
         .context(format!(
             "Failed to deserialise {}",
             String::from_utf8_lossy(&result.stdout)
         ))?;
 
-    Ok(
-        validation::sequence(package_names.into_iter().map(|package_name| {
-            let relative_package_file = structure::relative_file_for_package(&package_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);
             let absolute_package_file = nixpkgs_path.join(&relative_package_file);
 
-            if let Some(attribute_info) = actual_files.get(&package_name) {
-                let check_result = if !attribute_info.is_derivation {
-                    NixpkgsProblem::NonDerivation {
-                        relative_package_file: relative_package_file.clone(),
-                        package_name: package_name.clone(),
-                    }
-                    .into()
-                } else {
-                    Success(())
-                };
-
-                let check_result = check_result.and(match &attribute_info.variant {
-                    AttributeVariant::AutoCalled => Success(ratchet::Package {
-                        empty_non_auto_called: ratchet::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
-                        };
-
-                        if correct_file {
-                            Success(ratchet::Package {
-                                // Empty arguments for non-auto-called packages are not allowed anymore.
-                                empty_non_auto_called: if *empty_arg {
-                                    ratchet::EmptyNonAutoCalled::Invalid
-                                } else {
-                                    ratchet::EmptyNonAutoCalled::Valid
-                                },
-                            })
-                        } else {
-                            NixpkgsProblem::WrongCallPackage {
-                                relative_package_file: relative_package_file.clone(),
-                                package_name: package_name.clone(),
+            use AttributeInfo::*;
+            use ByNameAttribute::*;
+            use CallPackageVariant::*;
+
+            let check_result = match attribute_value {
+                Missing => NixpkgsProblem::UndefinedAttr {
+                    relative_package_file: relative_package_file.clone(),
+                    package_name: attribute_name.clone(),
+                }
+                .into(),
+                Existing(NonAttributeSet) => NixpkgsProblem::NonDerivation {
+                    relative_package_file: relative_package_file.clone(),
+                    package_name: attribute_name.clone(),
+                }
+                .into(),
+                Existing(NonCallPackage) => NixpkgsProblem::WrongCallPackage {
+                    relative_package_file: relative_package_file.clone(),
+                    package_name: attribute_name.clone(),
+                }
+                .into(),
+                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 {
+                            empty_non_auto_called: ratchet::EmptyNonAutoCalled::Valid,
+                        }),
+                        Manual { path, empty_arg } => {
+                            let correct_file = if let Some(call_package_path) = path {
+                                absolute_package_file == *call_package_path
+                            } else {
+                                false
+                            };
+
+                            if correct_file {
+                                Success(ratchet::Package {
+                                    // Empty arguments for non-auto-called packages are not allowed anymore.
+                                    empty_non_auto_called: if *empty_arg {
+                                        ratchet::EmptyNonAutoCalled::Invalid
+                                    } else {
+                                        ratchet::EmptyNonAutoCalled::Valid
+                                    },
+                                })
+                            } else {
+                                NixpkgsProblem::WrongCallPackage {
+                                    relative_package_file: relative_package_file.clone(),
+                                    package_name: attribute_name.clone(),
+                                }
+                                .into()
                             }
-                            .into()
                         }
-                    }
-                    AttributeVariant::Other => NixpkgsProblem::WrongCallPackage {
-                        relative_package_file: relative_package_file.clone(),
-                        package_name: package_name.clone(),
-                    }
-                    .into(),
-                });
-
-                check_result.map(|value| (package_name.clone(), value))
-            } else {
-                NixpkgsProblem::UndefinedAttr {
-                    relative_package_file: relative_package_file.clone(),
-                    package_name: package_name.clone(),
+                    })
                 }
-                .into()
-            }
-        }))
-        .map(|elems| ratchet::Nixpkgs {
-            packages: elems.into_iter().collect(),
-        }),
-    )
+            };
+            check_result.map(|value| (attribute_name.clone(), value))
+        },
+    ));
+
+    Ok(check_result.map(|elems| ratchet::Nixpkgs {
+        packages: elems.into_iter().collect(),
+    }))
 }