tests.nixpkgs-check-by-name: Better error for non-syntactic callPackage

5 files changed, 111 insertions, 53 deletions

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 1322f0faaf917..122c4b7348971 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -51,6 +51,26 @@ struct Location {
     pub column: usize,
 }
 
+impl Location {
+    // Creates a [String] from a [Location], in a format like `{file}:{line}:{column}`,
+    // where `file` is relative to the given [Path].
+    fn to_relative_string(&self, nixpkgs_path: &Path) -> anyhow::Result<String> {
+        let relative = self.file.strip_prefix(nixpkgs_path).with_context(|| {
+            format!(
+                "An attributes location ({}) is outside Nixpkgs ({})",
+                self.file.display(),
+                nixpkgs_path.display()
+            )
+        })?;
+        Ok(format!(
+            "{}:{}:{}",
+            relative.display(),
+            self.line,
+            self.column
+        ))
+    }
+}
+
 #[derive(Deserialize)]
 pub enum AttributeVariant {
     /// The attribute is not an attribute set, we're limited in the amount of information we can get
@@ -289,37 +309,47 @@ fn by_name(
                         // 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> = foo`
+                            (_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage {
+                                package_name: attribute_name.to_owned(),
+                                location: location.to_relative_string(nixpkgs_path)?,
+                                definition,
+                            }
+                            .into(),
                             // Something like `<attr> = pythonPackages.callPackage ...`
-                            | (false, Some(_))
-                            // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
-                            | (true, None) => {
+                            (false, Ok(_)) => {
                                 // 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()
+                                    relative_package_file: relative_package_file.to_owned(),
+                                    package_name: attribute_name.to_owned(),
+                                }
+                                .into()
                             }
                             // Something like `<attr> = pkgs.callPackage ...`
-                            (true, Some(syntactic_call_package)) => {
+                            (true, Ok(syntactic_call_package)) => {
                                 if let Some(path) = syntactic_call_package.relative_path {
                                     if path == relative_package_file {
                                         // Manual definitions with empty arguments are not allowed
                                         // anymore
-                                        Success(if syntactic_call_package.empty_arg { Loose(()) } else { Tight })
+                                        Success(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(),
-                                        }.into()
+                                            relative_package_file: relative_package_file.to_owned(),
+                                            package_name: attribute_name.to_owned(),
+                                        }
+                                        .into()
                                     }
                                 } else {
                                     NixpkgsProblem::WrongCallPackage {
-                                          relative_package_file: relative_package_file.to_owned(),
-                                          package_name: attribute_name.to_owned(),
-                                    }.into()
+                                        relative_package_file: relative_package_file.to_owned(),
+                                        package_name: attribute_name.to_owned(),
+                                    }
+                                    .into()
                                 }
                             }
                         }
@@ -423,16 +453,16 @@ fn handle_non_by_name_attribute(
         // `callPackage`
         match (is_semantic_call_package, optional_syntactic_call_package) {
             // Something like `<attr> = { }`
-            (false, None)
+            (false, Err(_))
             // Something like `<attr> = pythonPackages.callPackage ...`
-            | (false, Some(_))
+            | (false, Ok(_))
             // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
-            | (true, None) => {
+            | (true, Err(_)) => {
                 // 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)) => {
+            (true, Ok(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) => {
diff --git a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
index ea0833792f3c2..3840f1dc479d6 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -86,8 +86,9 @@ pub struct CallPackageArgumentInfo {
 impl NixFile {
     /// Returns information about callPackage arguments for an attribute at a specific line/column
     /// index.
-    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `None` is
-    /// returned.
+    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `Ok(Err(String))` is
+    /// returned, with String being how the definition looks like.
+    ///
     /// This function only returns `Err` for problems that can't be caused by the Nix contents,
     /// but rather problems in this programs code itself.
     ///
@@ -108,7 +109,7 @@ impl NixFile {
     ///
     /// You'll get back
     /// ```rust
-    /// Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true })
+    /// Ok(Ok(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }))
     /// ```
     ///
     /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an
@@ -118,11 +119,15 @@ impl NixFile {
         line: usize,
         column: usize,
         relative_to: &Path,
-    ) -> anyhow::Result<Option<CallPackageArgumentInfo>> {
-        let Some(attrpath_value) = self.attrpath_value_at(line, column)? else {
-            return Ok(None);
-        };
-        self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)
+    ) -> anyhow::Result<Result<CallPackageArgumentInfo, String>> {
+        Ok(match self.attrpath_value_at(line, column)? {
+            Err(definition) => Err(definition),
+            Ok(attrpath_value) => {
+                let definition = attrpath_value.to_string();
+                self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?
+                    .ok_or(definition)
+            }
+        })
     }
 
     // Internal function mainly to make it independently testable
@@ -130,7 +135,7 @@ impl NixFile {
         &self,
         line: usize,
         column: usize,
-    ) -> anyhow::Result<Option<ast::AttrpathValue>> {
+    ) -> anyhow::Result<Result<ast::AttrpathValue, String>> {
         let index = self.line_index.fromlinecolumn(line, column);
 
         let token_at_offset = self
@@ -164,7 +169,7 @@ impl NixFile {
             // This is the only other way how `builtins.unsafeGetAttrPos` can return
             // attribute positions, but we only look for ones like `<attr-path> = <value>`, so
             // ignore this
-            return Ok(None);
+            return Ok(Err(node.to_string()));
         } else {
             // However, anything else is not expected and smells like a bug
             anyhow::bail!(
@@ -208,7 +213,7 @@ impl NixFile {
         // 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()))
+        Ok(Ok(ast::AttrpathValue::cast(attrpath_value_node).unwrap()))
     }
 
     // Internal function mainly to make attrpath_value_at independently testable
@@ -437,14 +442,14 @@ mod tests {
 
         // These are builtins.unsafeGetAttrPos locations for the attributes
         let cases = [
-            (2, 3, Some("foo = 1;")),
-            (3, 3, Some(r#""bar" = 2;"#)),
-            (4, 3, Some(r#"${"baz"} = 3;"#)),
-            (5, 3, Some(r#""${"qux"}" = 4;"#)),
-            (8, 3, Some("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
-            (17, 7, Some("quuux/**/=/**/5/**/;")),
-            (19, 10, None),
-            (20, 22, None),
+            (2, 3, Ok("foo = 1;")),
+            (3, 3, Ok(r#""bar" = 2;"#)),
+            (4, 3, Ok(r#"${"baz"} = 3;"#)),
+            (5, 3, Ok(r#""${"qux"}" = 4;"#)),
+            (8, 3, Ok("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
+            (17, 7, Ok("quuux/**/=/**/5/**/;")),
+            (19, 10, Err("inherit toInherit;")),
+            (20, 22, Err("inherit (toInherit) toInherit;")),
         ];
 
         for (line, column, expected_result) in cases {
@@ -452,7 +457,13 @@ mod tests {
                 .attrpath_value_at(line, column)
                 .context(format!("line {line}, column {column}"))?
                 .map(|node| node.to_string());
-            assert_eq!(actual_result.as_deref(), expected_result);
+            let owned_expected_result = expected_result
+                .map(|x| x.to_string())
+                .map_err(|x| x.to_string());
+            assert_eq!(
+                actual_result, owned_expected_result,
+                "line {line}, column {column}"
+            );
         }
 
         Ok(())
@@ -481,41 +492,41 @@ mod tests {
         let nix_file = NixFile::new(&file)?;
 
         let cases = [
-            (2, None),
-            (3, None),
-            (4, None),
-            (5, None),
+            (2, Err("a.sub = null;")),
+            (3, Err("b = null;")),
+            (4, Err("c = import ./file.nix;")),
+            (5, Err("d = import ./file.nix { };")),
             (
                 6,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 7,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 8,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: None,
                     empty_arg: true,
                 }),
             ),
             (
                 9,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: false,
                 }),
             ),
             (
                 10,
-                Some(CallPackageArgumentInfo {
+                Ok(CallPackageArgumentInfo {
                     relative_path: None,
                     empty_arg: false,
                 }),
@@ -525,8 +536,10 @@ mod tests {
         for (line, expected_result) in cases {
             let actual_result = nix_file
                 .call_package_argument_info_at(line, 3, temp_dir.path())
-                .context(format!("line {line}"))?;
-            assert_eq!(actual_result, expected_result);
+                .context(format!("line {line}"))?
+                .map_err(|x| x);
+            let owned_expected_result = expected_result.map_err(|x| x.to_string());
+            assert_eq!(actual_result, owned_expected_result, "line {line}");
         }
 
         Ok(())
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 e13869adaa419..18001eb44ecd5 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -44,6 +44,11 @@ pub enum NixpkgsProblem {
         relative_package_file: PathBuf,
         package_name: String,
     },
+    NonSyntacticCallPackage {
+        package_name: String,
+        location: String,
+        definition: String,
+    },
     NonDerivation {
         relative_package_file: PathBuf,
         package_name: String,
@@ -164,6 +169,14 @@ impl fmt::Display for NixpkgsProblem {
                     "pkgs.{package_name}: 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 {} {{ ... }}` with a non-empty second argument.",
                     relative_package_file.display()
                 ),
+            NixpkgsProblem::NonSyntacticCallPackage { package_name, location, definition } =>
+                write!(
+                    f,
+                    "Because {} exists, the attribute `pkgs.{package_name}` must be defined as `callPackage {} {{ ... }}`. This is however not the case: The attribute is defined in {location} as\n\t{}",
+                    structure::relative_dir_for_package(package_name).display(),
+                    structure::relative_file_for_package(package_name).display(),
+                    definition,
+                ),
             NixpkgsProblem::NonDerivation { relative_package_file, package_name } =>
                 write!(
                     f,
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index 9df788191ece0..f590ef4ff9fad 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
@@ -1 +1,2 @@
-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.
+Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined as `callPackage pkgs/by-name/fo/foo/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:4:3 as
+	foo = self.bar;
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
index 51479e22d26fe..25fc867b04fe6 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/expected
@@ -1 +1,2 @@
-pkgs.nonDerivation: 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/no/nonDerivation/package.nix { ... }` with a non-empty second argument.
+Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined as `callPackage pkgs/by-name/no/nonDerivation/package.nix { ... }`. This is however not the case: The attribute is defined in all-packages.nix:2:3 as
+	nonDerivation = self.someDrv;