tests.nixpkgs-check-by-name: Improve code

- Detect manual use of _internalCallByNamePackageFile for packages in
  `pkgs/by-name` (can't be done for others though)
- Separate error message for when attribute locations can't be
  determined for `pkgs/by-name` attributes
- Much better structure of the code in eval.rs, representing more
  closely what is being checked
- Much more extensive comments

Author: infinisil

pkgs/test/nixpkgs-check-by-name/src/eval.nix

@@ -1,5 +1,5 @@
-# Takes a path to nixpkgs and a path to the json-encoded list of attributes to check.
-# Returns an value containing information on each requested attribute,
+# Takes a path to nixpkgs and a path to the json-encoded list of `pkgs/by-name` attributes.
+# Returns a value containing information on all Nixpkgs attributes
 # which is decoded on the Rust side.
 # See ./eval.rs for the meaning of the returned values
 {
@@ -9,33 +9,28 @@
 let
   attrs = builtins.fromJSON (builtins.readFile attrsPath);
 
-  nixpkgsPathLength = builtins.stringLength (toString nixpkgsPath) + 1;
-  removeNixpkgsPrefix = builtins.substring nixpkgsPathLength (-1);
-
-  # 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.
+  # We need to check whether attributes are defined manually e.g. in
+  # `all-packages.nix`, automatically by the `pkgs/by-name` overlay, or
+  # neither. The only way to do so is to override `callPackage` and
+  # `_internalCallByNamePackageFile` with our own version that adds this
+  # information to the result, and then try to access it.
   overlay = final: prev: {
 
-    # Information for attributes defined using `callPackage`
+    # Adds information to each attribute about whether it's manually defined using `callPackage`
     callPackage = fn: args:
       addVariantInfo (prev.callPackage fn args) {
-        Manual = {
-          path =
-            if builtins.isPath fn then
-              removeNixpkgsPrefix (toString fn)
-            else
-              null;
-          empty_arg =
-            args == { };
-        };
+        # This is a manual definition of the attribute, and it's a callPackage, specifically a semantic callPackage
+        ManualDefinition.is_semantic_call_package = true;
       };
 
-    # Information for attributes that are auto-called from pkgs/by-name.
-    # This internal attribute is only used by pkgs/by-name
+    # Adds information to each attribute about whether it's automatically
+    # defined by the `pkgs/by-name` overlay. This internal attribute is only
+    # used by that overlay.
+    # This overrides the above `callPackage` information (we don't need that
+    # one, since `pkgs/by-name` always uses `callPackage` underneath.
     _internalCallByNamePackageFile = file:
       addVariantInfo (prev._internalCallByNamePackageFile file) {
-        Auto = null;
+        AutoDefinition = null;
       };
 
   };
@@ -50,7 +45,7 @@ let
     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.
+      # so just don't return the value directly and treat it as if it wasn't a callPackage.
       value;
 
   pkgs = import nixpkgsPath {
@@ -62,37 +57,42 @@ let
     system = "x86_64-linux";
   };
 
-  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;
-          location = builtins.unsafeGetAttrPos name pkgs;
+  # See AttributeInfo in ./eval.rs for the meaning of this
+  attrInfo = name: value: {
+    location = builtins.unsafeGetAttrPos name pkgs;
+    attribute_variant =
+      if ! builtins.isAttrs value then
+        { NonAttributeSet = null; }
+      else
+        {
+          AttributeSet = {
+            is_derivation = pkgs.lib.isDerivation value;
+            definition_variant =
+              if ! value ? _callPackageVariant then
+                { ManualDefinition.is_semantic_call_package = false; }
+              else
+                value._callPackageVariant;
+          };
         };
-      };
+  };
 
+  # Information on all attributes that are in pkgs/by-name.
   byNameAttrs = builtins.listToAttrs (map (name: {
     inherit name;
     value.ByName =
       if ! pkgs ? ${name} then
         { Missing = null; }
       else
+        # Evaluation failures are not allowed, so don't try to catch them
         { Existing = attrInfo name pkgs.${name}; };
   }) attrs);
 
   # Information on all attributes that exist but are not in pkgs/by-name.
   # We need this to enforce pkgs/by-name for new packages
   nonByNameAttrs = builtins.mapAttrs (name: value:
     let
+      # Packages outside `pkgs/by-name` often fail evaluation,
+      # so we need to handle that
       output = attrInfo name value;
       result = builtins.tryEval (builtins.deepSeq output null);
     in