Better pkgs/by-name errors, minor fixes and improvements

pkgs/test/nixpkgs-check-by-name/Cargo.toml

@@ -15,7 +15,9 @@ lazy_static = "1.4.0"
 colored = "2.0.4"
 itertools = "0.11.0"
 rowan = "0.15.11"
+indoc = "2.0.4"
+relative-path = "1.9.2"
+textwrap = "0.16.1"
 
 [dev-dependencies]
 temp-env = "0.3.5"
-indoc = "2.0.4"

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

@@ -51,6 +51,23 @@ struct Location {
     pub column: usize,
 }
 
+impl Location {
+    // Returns the [file] field, but relative to Nixpkgs
+    fn relative_file(&self, nixpkgs_path: &Path) -> anyhow::Result<PathBuf> {
+        Ok(self
+            .file
+            .strip_prefix(nixpkgs_path)
+            .with_context(|| {
+                format!(
+                    "The file ({}) is outside Nixpkgs ({})",
+                    self.file.display(),
+                    nixpkgs_path.display()
+                )
+            })?
+            .to_path_buf())
+    }
+}
+
 #[derive(Deserialize)]
 pub enum AttributeVariant {
     /// The attribute is not an attribute set, we're limited in the amount of information we can get
@@ -195,7 +212,6 @@ fn by_name(
     use ByNameAttribute::*;
 
     let relative_package_file = structure::relative_file_for_package(attribute_name);
-    let absolute_package_file = nixpkgs_path.join(&relative_package_file);
 
     // 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
@@ -276,51 +292,86 @@ fn by_name(
                     // 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>`,
+                        // Parse the Nix file in the location
+                        let nix_file = nix_file_store.get(&location.file)?;
+
+                        // The relative path of the Nix file, for error messages
+                        let relative_file =
+                            location.relative_file(nixpkgs_path).with_context(|| {
+                                format!(
+                                    "Failed to resolve location file of attribute {attribute_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_store
-                            .get(&location.file)?
-                            .call_package_argument_info_at(
+                        let (optional_syntactic_call_package, definition) = nix_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,
-                        )?;
+                            nixpkgs_path,
+                        ).with_context(|| format!("Failed to get the definition info for attribute {attribute_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> = { ... }`
-                            // 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> = foo`
+                            (_, None) => NixpkgsProblem::NonSyntacticCallPackage {
+                                package_name: attribute_name.to_owned(),
+                                file: relative_file,
+                                line: location.line,
+                                column: location.column,
+                                definition,
+                            }
+                            .into(),
+                            // Something like `<attr> = pythonPackages.callPackage ...`
+                            (false, Some(_)) => NixpkgsProblem::NonToplevelCallPackage {
+                                package_name: attribute_name.to_owned(),
+                                file: relative_file,
+                                line: location.line,
+                                column: location.column,
+                                definition,
                             }
-                            // Something like `<attr> = pkgs.callPackage ./pkgs/by-name/...`,
-                            // with the correct file
+                            .into(),
+                            // Something like `<attr> = pkgs.callPackage ...`
                             (true, Some(syntactic_call_package)) => {
-                                Success(
-                                    // Manual definitions with empty arguments are not allowed
-                                    // anymore
-                                    if syntactic_call_package.empty_arg {
-                                        Loose(())
+                                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(NixpkgsProblem::EmptyArgument {
+                                                package_name: attribute_name.to_owned(),
+                                                file: relative_file,
+                                                line: location.line,
+                                                column: location.column,
+                                                definition,
+                                            })
+                                        } else {
+                                            Tight
+                                        })
                                     } else {
-                                        Tight
+                                        // Wrong path
+                                        NixpkgsProblem::WrongCallPackagePath {
+                                            package_name: attribute_name.to_owned(),
+                                            file: relative_file,
+                                            line: location.line,
+                                            column: location.column,
+                                            actual_path: path,
+                                            expected_path: relative_package_file,
+                                        }
+                                        .into()
                                     }
-                                )
+                                } else {
+                                    // No path
+                                    NixpkgsProblem::NonPath {
+                                        package_name: attribute_name.to_owned(),
+                                        file: relative_file,
+                                        line: location.line,
+                                        column: location.column,
+                                        definition,
+                                    }
+                                    .into()
+                                }
                             }
                         }
                     } else {
@@ -408,7 +459,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,
@@ -453,7 +504,7 @@ fn handle_non_by_name_attribute(
                     _ => {
                         // Otherwise, the path is outside `pkgs/by-name`, which means it can be
                         // migrated
-                        Loose(syntactic_call_package)
+                        Loose((syntactic_call_package, location.file))
                     }
                 }
             }