tests.nixpkgs-check-by-name: Improve more errors

10 files changed, 134 insertions, 54 deletions

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index fa06e38c357a..ddeac2a445b3 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -305,7 +305,7 @@ fn by_name(
 
                         // 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.call_package_argument_info_at(
+                        let (optional_syntactic_call_package, definition) = nix_file.call_package_argument_info_at(
                             location.line,
                             location.column,
                             nixpkgs_path,
@@ -315,7 +315,7 @@ fn by_name(
                         // `callPackage`
                         match (is_semantic_call_package, optional_syntactic_call_package) {
                             // Something like `<attr> = foo`
-                            (_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage {
+                            (_, None) => NixpkgsProblem::NonSyntacticCallPackage {
                                 package_name: attribute_name.to_owned(),
                                 file: relative_file,
                                 line: location.line,
@@ -324,17 +324,16 @@ fn by_name(
                             }
                             .into(),
                             // Something like `<attr> = pythonPackages.callPackage ...`
-                            (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()
+                            (false, Some(_)) => NixpkgsProblem::NonToplevelCallPackage {
+                                package_name: attribute_name.to_owned(),
+                                file: relative_file,
+                                line: location.line,
+                                column: location.column,
+                                definition,
                             }
+                            .into(),
                             // Something like `<attr> = pkgs.callPackage ...`
-                            (true, Ok(syntactic_call_package)) => {
+                            (true, Some(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
@@ -358,9 +357,12 @@ fn by_name(
                                     }
                                 } else {
                                     // No path
-                                    NixpkgsProblem::WrongCallPackage {
-                                        relative_package_file: relative_package_file.to_owned(),
+                                    NixpkgsProblem::NonPath {
                                         package_name: attribute_name.to_owned(),
+                                        file: relative_file,
+                                        line: location.line,
+                                        column: location.column,
+                                        definition,
                                     }
                                     .into()
                                 }
@@ -451,7 +453,7 @@ fn handle_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
+        let (optional_syntactic_call_package, _definition) = nix_file_store
             .get(&location.file)?
             .call_package_argument_info_at(
                 location.line,
@@ -466,16 +468,16 @@ fn handle_non_by_name_attribute(
         // `callPackage`
         match (is_semantic_call_package, optional_syntactic_call_package) {
             // Something like `<attr> = { }`
-            (false, Err(_))
+            (false, None)
             // Something like `<attr> = pythonPackages.callPackage ...`
-            | (false, Ok(_))
+            | (false, Some(_))
             // Something like `<attr> = bar` where `bar = pkgs.callPackage ...`
-            | (true, Err(_)) => {
+            | (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, Ok(syntactic_call_package)) => {
+            (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) => {
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 3840f1dc479d..1382f389e2da 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -119,13 +119,14 @@ impl NixFile {
         line: usize,
         column: usize,
         relative_to: &Path,
-    ) -> anyhow::Result<Result<CallPackageArgumentInfo, String>> {
+    ) -> anyhow::Result<(Option<CallPackageArgumentInfo>, String)> {
         Ok(match self.attrpath_value_at(line, column)? {
-            Err(definition) => Err(definition),
+            Err(definition) => (None, definition),
             Ok(attrpath_value) => {
                 let definition = attrpath_value.to_string();
-                self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?
-                    .ok_or(definition)
+                let attrpath_value =
+                    self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?;
+                (attrpath_value, definition)
             }
         })
     }
@@ -492,41 +493,41 @@ mod tests {
         let nix_file = NixFile::new(&file)?;
 
         let cases = [
-            (2, Err("a.sub = null;")),
-            (3, Err("b = null;")),
-            (4, Err("c = import ./file.nix;")),
-            (5, Err("d = import ./file.nix { };")),
+            (2, None),
+            (3, None),
+            (4, None),
+            (5, None),
             (
                 6,
-                Ok(CallPackageArgumentInfo {
+                Some(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 7,
-                Ok(CallPackageArgumentInfo {
+                Some(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 8,
-                Ok(CallPackageArgumentInfo {
+                Some(CallPackageArgumentInfo {
                     relative_path: None,
                     empty_arg: true,
                 }),
             ),
             (
                 9,
-                Ok(CallPackageArgumentInfo {
+                Some(CallPackageArgumentInfo {
                     relative_path: Some(PathBuf::from("file.nix")),
                     empty_arg: false,
                 }),
             ),
             (
                 10,
-                Ok(CallPackageArgumentInfo {
+                Some(CallPackageArgumentInfo {
                     relative_path: None,
                     empty_arg: false,
                 }),
@@ -534,12 +535,10 @@ mod tests {
         ];
 
         for (line, expected_result) in cases {
-            let actual_result = nix_file
+            let (actual_result, _definition) = nix_file
                 .call_package_argument_info_at(line, 3, temp_dir.path())
-                .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}");
+                .context(format!("line {line}"))?;
+            assert_eq!(actual_result, 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 d8ae2db4dc22..54976306df32 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -47,6 +47,20 @@ pub enum NixpkgsProblem {
         relative_package_file: PathBuf,
         package_name: String,
     },
+    NonToplevelCallPackage {
+        package_name: String,
+        file: PathBuf,
+        line: usize,
+        column: usize,
+        definition: String,
+    },
+    NonPath {
+        package_name: String,
+        file: PathBuf,
+        line: usize,
+        column: usize,
+        definition: String,
+    },
     WrongCallPackagePath {
         package_name: String,
         file: PathBuf,
@@ -182,6 +196,42 @@ 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::NonToplevelCallPackage { package_name, file, line, column, definition } =>
+                writedoc!(
+                    f,
+                    "
+                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+
+                        {package_name} = callPackage {} {{ /* ... */ }};
+
+                      This is however not the case: A different `callPackage` is used.
+                      It is defined in {}:{} as
+
+                    {}",
+                    structure::relative_dir_for_package(package_name).display(),
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                    line,
+                    indent_definition(*column, definition.clone()),
+                ),
+            NixpkgsProblem::NonPath { package_name, file, line, column, definition } =>
+                writedoc!(
+                    f,
+                    "
+                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+
+                        {package_name} = callPackage {} {{ /* ... */ }};
+
+                      This is however not the case: The first callPackage argument is not right:
+                      It is defined in {}:{} as
+
+                    {}",
+                    structure::relative_dir_for_package(package_name).display(),
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                    line,
+                    indent_definition(*column, definition.clone()),
+                ),
             NixpkgsProblem::WrongCallPackagePath { package_name, file, line, column, actual_path, expected_path } =>
                 writedoc! {
                     f,
@@ -200,20 +250,6 @@ impl fmt::Display for NixpkgsProblem {
                     create_path_expr(file, actual_path),
                 },
             NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => {
-                // This is needed such multi-line definitions don't look odd
-                // A bit round-about, but it works and we might not need anything more complicated
-                let definition_indented =
-                    // The entire code should be indented 4 spaces
-                    textwrap::indent(
-                        // But first we want to strip the code's natural indentation
-                        &textwrap::dedent(
-                            // The definition _doesn't_ include the leading spaces, but we can
-                            // recover those from the column
-                            &format!("{}{definition}",
-                            " ".repeat(column - 1)),
-                        ),
-                        "    ",
-                    );
                 writedoc!(
                     f,
                     "
@@ -229,7 +265,7 @@ impl fmt::Display for NixpkgsProblem {
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
                     line,
-                    definition_indented,
+                    indent_definition(*column, definition.clone()),
                 )
             }
             NixpkgsProblem::NonDerivation { relative_package_file, package_name } =>
@@ -340,6 +376,19 @@ impl fmt::Display for NixpkgsProblem {
     }
 }
 
+fn indent_definition(column: usize, definition: String) -> String {
+    // The entire code should be indented 4 spaces
+    textwrap::indent(
+        // But first we want to strip the code's natural indentation
+        &textwrap::dedent(
+            // The definition _doesn't_ include the leading spaces, but we can
+            // recover those from the column
+            &format!("{}{definition}", " ".repeat(column - 1)),
+        ),
+        "    ",
+    )
+}
+
 /// Creates a Nix path expression that when put into Nix file `from_file`, would point to the `to_file`.
 fn create_path_expr(from_file: impl AsRef<Path>, to_file: impl AsRef<Path>) -> String {
     // FIXME: Clean these unwrap's up
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix
new file mode 100644
index 000000000000..399f8eee0a18
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix
@@ -0,0 +1,7 @@
+self: super: {
+
+  alt.callPackage = self.lib.callPackageWith {};
+
+  foo = self.alt.callPackage ({ }: self.someDrv) { };
+
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix
new file mode 100644
index 000000000000..861260cdca4b
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
new file mode 100644
index 000000000000..25d2e6c4fa6a
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -0,0 +1,8 @@
+- Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
+
+    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+
+  This is however not the case: A different `callPackage` is used.
+  It is defined in all-packages.nix:5 as
+
+    foo = self.alt.callPackage ({ }: self.someDrv) { };
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix
new file mode 100644
index 000000000000..a1b92efbbadb
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: someDrv
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix
index 3e0ea20c2281..e31aa831b814 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/all-packages.nix
@@ -2,5 +2,4 @@ self: super: {
 
   bar = (x: x) self.callPackage ./pkgs/by-name/fo/foo/package.nix { someFlag = true; };
   foo = self.bar;
-
 }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected
index 51479e22d26f..0f8a4cb65eef 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/expected
@@ -1 +1,8 @@
-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 like
+
+    nonDerivation = callPackage pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+
+  This is however not the case: The first callPackage argument is not right:
+  It is defined in all-packages.nix:2 as
+
+    nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected
index 9df788191ece..41e40a9b3c3f 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected
@@ -1 +1,8 @@
-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 like
+
+    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+
+  This is however not the case: The first callPackage argument is not right:
+  It is defined in all-packages.nix:3 as
+
+    foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };