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

5 files changed, 335 insertions, 221 deletions

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix
index 7179951d41cf0..ab1c41e0b1458 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix
+++ b/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,30 +57,33 @@ 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);
 
@@ -93,6 +91,8 @@ let
   # 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
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index cb49c3acbef30..e90a95533144c 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -37,24 +37,13 @@ enum ByNameAttribute {
 }
 
 #[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,
+struct AttributeInfo {
+    /// The location of the attribute as returned by `builtins.unsafeGetAttrPos`
     location: Option<Location>,
+    attribute_variant: AttributeVariant,
 }
 
-/// The structure returned by `builtins.unsafeGetAttrPos`
+/// The structure returned by a successful `builtins.unsafeGetAttrPos`
 #[derive(Deserialize, Clone, Debug)]
 struct Location {
     pub file: PathBuf,
@@ -63,17 +52,28 @@ struct Location {
 }
 
 #[derive(Deserialize)]
-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
-    Auto,
-    /// The attribute is defined as a pkgs.callPackage <path> <args>,
-    /// and overridden by all-packages.nix
-    Manual {
-        /// The <path> argument or None if it's not a path
-        path: Option<PathBuf>,
-        /// true if <args> is { }
-        empty_arg: bool,
+pub enum AttributeVariant {
+    /// The attribute is not an attribute set, we're limited in the amount of information we can get
+    /// from it (though it's obviously not a derivation)
+    NonAttributeSet,
+    AttributeSet {
+        /// Whether the attribute is a derivation (`lib.isDerivation`)
+        is_derivation: bool,
+        /// The type of callPackage
+        definition_variant: DefinitionVariant,
+    },
+}
+
+#[derive(Deserialize)]
+pub enum DefinitionVariant {
+    /// An automatic definition by the `pkgs/by-name` overlay
+    /// Though it's detected using the internal _internalCallByNamePackageFile attribute,
+    /// which can in theory also be used by other code
+    AutoDefinition,
+    /// A manual definition of the attribute, typically in `all-packages.nix`
+    ManualDefinition {
+        /// Whether the attribute is defined as `pkgs.callPackage ...` or something else.
+        is_semantic_call_package: bool,
     },
 }
 
@@ -165,9 +165,12 @@ pub fn check_values(
                         nix_file_store,
                         non_by_name_attribute,
                     )?,
-                    Attribute::ByName(by_name_attribute) => {
-                        by_name(&attribute_name, by_name_attribute)
-                    }
+                    Attribute::ByName(by_name_attribute) => by_name(
+                        nix_file_store,
+                        nixpkgs_path,
+                        &attribute_name,
+                        by_name_attribute,
+                    )?,
                 };
                 Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value)))
             })
@@ -183,78 +186,168 @@ pub fn check_values(
 /// Handles the evaluation result for an attribute in `pkgs/by-name`,
 /// turning it into a validation result.
 fn by_name(
+    nix_file_store: &mut NixFileStore,
+    nixpkgs_path: &Path,
     attribute_name: &str,
     by_name_attribute: ByNameAttribute,
-) -> validation::Validation<ratchet::Package> {
+) -> validation::Result<ratchet::Package> {
     use ratchet::RatchetState::*;
-    use AttributeInfo::*;
     use ByNameAttribute::*;
-    use CallPackageVariant::*;
 
     let relative_package_file = structure::relative_file_for_package(attribute_name);
+    let absolute_package_file = nixpkgs_path.join(&relative_package_file);
 
-    match by_name_attribute {
-        Missing => NixpkgsProblem::UndefinedAttr {
-            relative_package_file: relative_package_file.to_owned(),
-            package_name: attribute_name.to_owned(),
+    // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists.
+    // This match decides whether the attribute `foo` is defined accordingly
+    // and whether a legacy manual definition could be removed
+    let manual_definition_result = match by_name_attribute {
+        // The attribute is missing
+        Missing => {
+            // This indicates a bug in the `pkgs/by-name` overlay, because it's supposed to
+            // automatically defined attributes in `pkgs/by-name`
+            NixpkgsProblem::UndefinedAttr {
+                relative_package_file: relative_package_file.to_owned(),
+                package_name: attribute_name.to_owned(),
+            }
+            .into()
         }
-        .into(),
-        Existing(NonAttributeSet) => NixpkgsProblem::NonDerivation {
-            relative_package_file: relative_package_file.to_owned(),
-            package_name: attribute_name.to_owned(),
+        // The attribute exists
+        Existing(AttributeInfo {
+            // But it's not an attribute set, which limits the amount of information we can get
+            // about this attribute (see ./eval.nix)
+            attribute_variant: AttributeVariant::NonAttributeSet,
+            location: _location,
+        }) => {
+            // The only thing we know is that it's definitely not a derivation, since those are
+            // always attribute sets.
+            //
+            // We can't know whether the attribute is automatically or manually defined for sure,
+            // and while we could check the location, the error seems clear enough as is.
+            NixpkgsProblem::NonDerivation {
+                relative_package_file: relative_package_file.to_owned(),
+                package_name: attribute_name.to_owned(),
+            }
+            .into()
         }
-        .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 {
+        // The attribute exists
+        Existing(AttributeInfo {
+            // And it's an attribute set, which allows us to get more information about it
+            attribute_variant:
+                AttributeVariant::AttributeSet {
+                    is_derivation,
+                    definition_variant,
+                },
+            location,
+        }) => {
+            // Only derivations are allowed in `pkgs/by-name`
+            let is_derivation_result = if is_derivation {
+                Success(())
+            } else {
                 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
+            // If the definition looks correct
+            let variant_result = match definition_variant {
+                // An automatic `callPackage` by the `pkgs/by-name` overlay.
+                // Though this gets detected by checking whether the internal
+                // `_internalCallByNamePackageFile` was used
+                DefinitionVariant::AutoDefinition => {
+                    if let Some(_location) = location {
+                        // Such an automatic definition should definitely not have a location
+                        // Having one indicates that somebody is using `_internalCallByNamePackageFile`,
+                        NixpkgsProblem::InternalCallPackageUsed {
+                            attr_name: attribute_name.to_owned(),
+                        }
+                        .into()
                     } else {
-                        false
-                    };
+                        Success(Tight)
+                    }
+                }
+                // The attribute is manually defined, e.g. in `all-packages.nix`.
+                // This means we need to enforce it to look like this:
+                //   callPackage ../pkgs/by-name/fo/foo/package.nix { ... }
+                DefinitionVariant::ManualDefinition {
+                    is_semantic_call_package,
+                } => {
+                    // We should expect manual definitions to have a location, otherwise we can't
+                    // enforce the expected format
+                    if let Some(location) = location {
+                        // Parse the Nix file in the location and figure out whether it's an
+                        // attribute definition of the form `= callPackage <arg1> <arg2>`,
+                        // returning the arguments if so.
+                        let optional_syntactic_call_package = nix_file_store
+                            .get(&location.file)?
+                            .call_package_argument_info_at(
+                            location.line,
+                            location.column,
+                            // We're passing `pkgs/by-name/fo/foo/package.nix` here, which causes
+                            // the function to verify that `<arg1>` is the same path,
+                            // making `syntactic_call_package.relative_path` end up as `""`
+                            // TODO: This is confusing and should be improved
+                            &absolute_package_file,
+                        )?;
 
-                    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,
-                        })
+                        // At this point, we completed two different checks for whether it's a
+                        // `callPackage`
+                        match (is_semantic_call_package, optional_syntactic_call_package) {
+                            // Something like `<attr> = { ... }`
+                            // or a `pkgs.callPackage` but with the wrong file
+                            (false, None)
+                            // Something like `<attr> = pythonPackages.callPackage ./pkgs/by-name/...`
+                            | (false, Some(_))
+                            // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
+                            // or a `callPackage` but with the wrong file
+                            | (true, None) => {
+                                // All of these are not of the expected form, so error out
+                                // TODO: Make error more specific, don't lump everything together
+                                NixpkgsProblem::WrongCallPackage {
+                                      relative_package_file: relative_package_file.to_owned(),
+                                      package_name: attribute_name.to_owned(),
+                                }.into()
+                            }
+                            // Something like `<attr> = pkgs.callPackage ./pkgs/by-name/...`,
+                            // with the correct file
+                            (true, Some(syntactic_call_package)) => {
+                                Success(
+                                    // Manual definitions with empty arguments are not allowed
+                                    // anymore
+                                    if syntactic_call_package.empty_arg {
+                                        Loose(())
+                                    } else {
+                                        Tight
+                                    }
+                                )
+                            }
+                        }
                     } else {
-                        NixpkgsProblem::WrongCallPackage {
-                            relative_package_file: relative_package_file.to_owned(),
-                            package_name: attribute_name.to_owned(),
+                        // If manual definitions don't have a location, it's likely `mapAttrs`'d
+                        // over, e.g. if it's defined in aliases.nix.
+                        // We can't verify whether its of the expected `callPackage`, so error out
+                        NixpkgsProblem::CannotDetermineAttributeLocation {
+                            attr_name: attribute_name.to_owned(),
                         }
                         .into()
                     }
                 }
-            })
+            };
+
+            // Independently report problems about whether it's a derivation and the callPackage variant
+            is_derivation_result.and(variant_result)
         }
-    }
+    };
+    Ok(
+        // Packages being checked in this function are _always_ already defined in `pkgs/by-name`,
+        // so instead of repeating ourselves all the time to define `uses_by_name`, just set it
+        // once at the end with a map
+        manual_definition_result.map(|manual_definition| ratchet::Package {
+            manual_definition,
+            uses_by_name: Tight,
+        }),
+    )
 }
 
 /// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`,
@@ -265,113 +358,117 @@ fn handle_non_by_name_attribute(
     non_by_name_attribute: NonByNameAttribute,
 ) -> validation::Result<ratchet::Package> {
     use ratchet::RatchetState::*;
-    use AttributeInfo::*;
-    use CallPackageVariant::*;
     use NonByNameAttribute::*;
 
-    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 `<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 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))
-                            }
-                        }
+    // The ratchet state whether this attribute uses `pkgs/by-name`.
+    // This is never `Tight`, because we only either:
+    // - Know that the attribute _could_ be migrated to `pkgs/by-name`, which is `Loose`
+    // - Or we're unsure, in which case we use NonApplicable
+    let uses_by_name =
+        // This is a big ol' match on various properties of the attribute
+
+        // First, it needs to succeed evaluation. We can't know whether an attribute could be
+        // migrated to `pkgs/by-name` if it doesn't evaluate, since we need to check that it's a
+        // derivation.
+        //
+        // This only has the minor negative effect that if a PR that breaks evaluation
+        // gets merged, fixing those failures won't force anything into `pkgs/by-name`.
+        //
+        // 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 failing attributes would have been fit for `pkgs/by-name`
+        // 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.
+        if let EvalSuccess(AttributeInfo {
+            // We're only interested in attributes that are attribute sets (which includes
+            // derivations). Anything else can't be in `pkgs/by-name`.
+            attribute_variant: AttributeVariant::AttributeSet {
+                // Indeed, we only care about derivations, non-derivation attribute sets can't be
+                // in `pkgs/by-name`
+                is_derivation: true,
+                // Of the two definition variants, really only the manual one makes sense here.
+                // Special cases are:
+                // - Manual aliases to auto-called packages are not treated as manual definitions,
+                //   due to limitations in the semantic callPackage detection. So those should be
+                //   ignored.
+                // - Manual definitions using the internal _internalCallByNamePackageFile are
+                //   not treated as manual definitions, since _internalCallByNamePackageFile is
+                //   used to detect automatic ones. We can't distinguish from the above case, so we
+                //   just need to ignore this one too, even if that internal attribute should never
+                //   be called manually.
+                definition_variant: DefinitionVariant::ManualDefinition { is_semantic_call_package }
+            },
+            // We need the location of the manual definition, because otherwise
+            // we can't figure out whether it's a syntactic callPackage
+            location: Some(location),
+        }) = non_by_name_attribute {
+
+        // Parse the Nix file in the location and figure out whether it's an
+        // attribute definition of the form `= callPackage <arg1> <arg2>`,
+        // returning the arguments if so.
+        let optional_syntactic_call_package = nix_file_store
+            .get(&location.file)?
+            .call_package_argument_info_at(
+                location.line,
+                location.column,
+                // Passing the Nixpkgs path here both checks that the <arg1> is within Nixpkgs, and
+                // strips the absolute Nixpkgs path from it, such that
+                // syntactic_call_package.relative_path is relative to Nixpkgs
+                nixpkgs_path
+            )?;
+
+        // At this point, we completed two different checks for whether it's a
+        // `callPackage`
+        match (is_semantic_call_package, optional_syntactic_call_package) {
+            // Something like `<attr> = { }`
+            (false, None)
+            // Something like `<attr> = pythonPackages.callPackage ...`
+            | (false, Some(_))
+            // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
+            | (true, None) => {
+                // In all of these cases, it's not possible to migrate the package to `pkgs/by-name`
+                NonApplicable
+            }
+            // Something like `<attr> = pkgs.callPackage ...`
+            (true, Some(syntactic_call_package)) => {
+                // It's only possible to migrate such a definitions if..
+                match syntactic_call_package.relative_path {
+                    Some(ref rel_path) if rel_path.starts_with(utils::BASE_SUBPATH) => {
+                        // ..the path is not already within `pkgs/by-name` like
+                        //
+                        //   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, so migration is generally not possible.
+                        //
+                        // See also "package variants" in RFC 140:
+                        // https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md#package-variants
+                        NonApplicable
+                    }
+                    _ => {
+                        // Otherwise, the path is outside `pkgs/by-name`, which means it can be
+                        // migrated
+                        Loose(syntactic_call_package)
                     }
                 }
-            };
-            uses_by_name.map(|x| ratchet::Package {
-                manual_definition: Tight,
-                uses_by_name: x,
-            })
-        }
-        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,
-            })
+            }
         }
-    })
+    } else {
+        // This catches all the cases not matched by the above `if let`, falling back to not being
+        // able to migrate such attributes
+        NonApplicable
+    };
+    Ok(Success(ratchet::Package {
+        // Packages being checked in this function _always_ need a manual definition, because
+        // they're not using `pkgs/by-name` which would allow avoiding it.
+        // so instead of repeating ourselves all the time to define `manual_definition`,
+        // just set it once at the end here
+        manual_definition: Tight,
+        uses_by_name,
+    }))
 }
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 25e3ef4863e41..e13869adaa419 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -92,6 +92,12 @@ pub enum NixpkgsProblem {
         call_package_path: Option<PathBuf>,
         empty_arg: bool,
     },
+    InternalCallPackageUsed {
+        attr_name: String,
+    },
+    CannotDetermineAttributeLocation {
+        attr_name: String,
+    },
 }
 
 impl fmt::Display for NixpkgsProblem {
@@ -252,6 +258,16 @@ impl fmt::Display for NixpkgsProblem {
                     structure::relative_file_for_package(package_name).display(),
                 )
             },
-        }
+            NixpkgsProblem::InternalCallPackageUsed { attr_name } =>
+                write!(
+                    f,
+                    "pkgs.{attr_name}: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.",
+                ),
+            NixpkgsProblem::CannotDetermineAttributeLocation { attr_name } =>
+                write!(
+                    f,
+                    "pkgs.{attr_name}: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.",
+                ),
+       }
     }
 }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
new file mode 100644
index 0000000000000..404795ee5c79a
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
@@ -0,0 +1 @@
+pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected b/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
index 9df788191ece0..2a248c23ab50c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
@@ -1 +1 @@
-pkgs.foo: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/fo/foo/package.nix { ... }` with a non-empty second argument.
+pkgs.foo: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.