From 712ac796e5be1c6bcafad54d46b7cdcd7fdc7bcb Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Mon, 19 Feb 2024 22:02:27 +0100
Subject: [PATCH 01/18] tests.nixpkgs-check-by-name: Improve inherit detection

Detect inherit's better, such that a future commit can use this
information in an error message
---
 .../nixpkgs-check-by-name/src/nix_file.rs     | 36 +++++++++++++++----
 1 file changed, 30 insertions(+), 6 deletions(-)

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 836c5e2dcdda5..ea0833792f3c2 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -5,7 +5,6 @@ use anyhow::Context;
 use rnix::ast;
 use rnix::ast::Expr;
 use rnix::ast::HasEntry;
-use rnix::SyntaxKind;
 use rowan::ast::AstNode;
 use rowan::TextSize;
 use rowan::TokenAtOffset;
@@ -158,6 +157,22 @@ impl NixFile {
             )
         };
 
+        if ast::Attr::can_cast(node.kind()) {
+            // Something like `foo`, `"foo"` or `${"foo"}`
+        } else if ast::Inherit::can_cast(node.kind()) {
+            // Something like `inherit <attr>` or `inherit (<source>) <attr>`
+            // 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);
+        } else {
+            // However, anything else is not expected and smells like a bug
+            anyhow::bail!(
+                "Node in {} is neither an attribute node, nor an inherit node: {node:?}",
+                self.path.display()
+            )
+        }
+
         // node looks like "foo"
         let Some(attrpath_node) = node.parent() else {
             anyhow::bail!(
@@ -166,10 +181,14 @@ impl NixFile {
             )
         };
 
-        if attrpath_node.kind() != SyntaxKind::NODE_ATTRPATH {
-            // This can happen for e.g. `inherit foo`, so definitely not a syntactic `callPackage`
-            return Ok(None);
+        if !ast::Attrpath::can_cast(attrpath_node.kind()) {
+            // We know that `node` is an attribute, its parent should be an attribute path
+            anyhow::bail!(
+                "In {}, attribute parent node is not an attribute path node: {attrpath_node:?}",
+                self.path.display()
+            )
         }
+
         // attrpath_node looks like "foo.bar"
         let Some(attrpath_value_node) = attrpath_node.parent() else {
             anyhow::bail!(
@@ -408,6 +427,7 @@ mod tests {
               /**/quuux/**/=/**/5/**/;/*E*/
 
               inherit toInherit;
+              inherit (toInherit) toInherit;
             }
         "#};
 
@@ -424,11 +444,13 @@ mod tests {
             (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),
         ];
 
         for (line, column, expected_result) in cases {
             let actual_result = nix_file
-                .attrpath_value_at(line, column)?
+                .attrpath_value_at(line, column)
+                .context(format!("line {line}, column {column}"))?
                 .map(|node| node.to_string());
             assert_eq!(actual_result.as_deref(), expected_result);
         }
@@ -501,7 +523,9 @@ mod tests {
         ];
 
         for (line, expected_result) in cases {
-            let actual_result = nix_file.call_package_argument_info_at(line, 3, temp_dir.path())?;
+            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);
         }
 

From a61c8c9f5c19030102f62820db1ad037311702f7 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Mon, 19 Feb 2024 22:28:01 +0100
Subject: [PATCH 02/18] tests.nixpkgs-check-by-name: Fix allowing non-path
 overrides

An edge case was allowed when it shouldn't be: A package defined in
`pkgs/by-name` could be overridden in `all-packages.nix` if it was of
the form `callPackage (<expr>) { <non-empty> }`.

This is not right, it's required that the first argument be the path
matching the package to be overridden.
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 36 +++++++++----------
 .../tests/override-non-path/all-packages.nix  |  5 +++
 .../tests/override-non-path/default.nix       |  1 +
 .../tests/override-non-path/expected          |  1 +
 .../pkgs/by-name/fo/foo/package.nix           |  1 +
 5 files changed, 26 insertions(+), 18 deletions(-)
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-non-path/pkgs/by-name/fo/foo/package.nix

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index e90a95533144c..1322f0faaf917 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -195,7 +195,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
@@ -284,23 +283,17 @@ fn by_name(
                             .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,
                         )?;
 
                         // 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/...`
+                            // Something like `<attr> = pythonPackages.callPackage ...`
                             | (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
@@ -309,18 +302,25 @@ fn by_name(
                                       package_name: attribute_name.to_owned(),
                                 }.into()
                             }
-                            // Something like `<attr> = pkgs.callPackage ./pkgs/by-name/...`,
-                            // with the correct file
+                            // 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(()) } else { Tight })
                                     } else {
-                                        Tight
+                                        NixpkgsProblem::WrongCallPackage {
+                                              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()
+                                }
                             }
                         }
                     } else {
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix
new file mode 100644
index 0000000000000..f07e7a42744a4
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/all-packages.nix
@@ -0,0 +1,5 @@
+self: super: {
+
+  foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
+
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix
new file mode 100644
index 0000000000000..861260cdca4b2
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
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
new file mode 100644
index 0000000000000..9df788191ece0
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/expected
@@ -0,0 +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.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/pkgs/by-name/fo/foo/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/pkgs/by-name/fo/foo/package.nix
new file mode 100644
index 0000000000000..a1b92efbbadb9
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-non-path/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: someDrv

From 24069b982dcb3b64bb3de2fc3c7b3463f4b00723 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Mon, 19 Feb 2024 23:35:18 +0100
Subject: [PATCH 03/18] tests.nixpkgs-check-by-name: Better error for
 non-syntactic callPackage

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 70 ++++++++++++-----
 .../nixpkgs-check-by-name/src/nix_file.rs     | 75 +++++++++++--------
 .../src/nixpkgs_problem.rs                    | 13 ++++
 .../expected                                  |  3 +-
 .../tests/override-no-call-package/expected   |  3 +-
 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;

From a53b07e2523978518efc6ae161ef9ffba7933d6e Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Thu, 22 Feb 2024 23:04:40 +0100
Subject: [PATCH 04/18] tests.nixpkgs-check-by-name: Improve error for wrong
 package file

---
 pkgs/test/nixpkgs-check-by-name/Cargo.lock    |  7 +++
 pkgs/test/nixpkgs-check-by-name/Cargo.toml    |  3 +-
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 63 +++++++++++--------
 pkgs/test/nixpkgs-check-by-name/src/main.rs   |  2 +-
 .../src/nixpkgs_problem.rs                    | 53 +++++++++++++++-
 .../tests/override-different-file/expected    |  9 ++-
 6 files changed, 106 insertions(+), 31 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.lock b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
index 904a9cff0e789..b02f7075ab613 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.lock
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
@@ -299,6 +299,7 @@ dependencies = [
  "itertools",
  "lazy_static",
  "regex",
+ "relative-path",
  "rnix",
  "rowan",
  "serde",
@@ -392,6 +393,12 @@ version = "0.7.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "e5ea92a5b6195c6ef2a0295ea818b312502c6fc94dde986c5553242e18fd4ce2"
 
+[[package]]
+name = "relative-path"
+version = "1.9.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e898588f33fdd5b9420719948f9f2a32c922a246964576f71ba7f24f80610fbc"
+
 [[package]]
 name = "rnix"
 version = "0.11.0"
diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
index 5240cd69f996e..1b8f8e9085d4e 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
@@ -15,7 +15,8 @@ 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"
 
 [dev-dependencies]
 temp-env = "0.3.5"
-indoc = "2.0.4"
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 122c4b7348971..fa06e38c357ad 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -52,22 +52,19 @@ struct Location {
 }
 
 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
-        ))
+    // 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())
     }
 }
 
@@ -295,16 +292,24 @@ 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 = nix_file.call_package_argument_info_at(
                             location.line,
                             location.column,
                             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`
@@ -312,7 +317,9 @@ fn by_name(
                             // Something like `<attr> = foo`
                             (_, Err(definition)) => NixpkgsProblem::NonSyntacticCallPackage {
                                 package_name: attribute_name.to_owned(),
-                                location: location.to_relative_string(nixpkgs_path)?,
+                                file: relative_file,
+                                line: location.line,
+                                column: location.column,
                                 definition,
                             }
                             .into(),
@@ -338,13 +345,19 @@ fn by_name(
                                             Tight
                                         })
                                     } else {
-                                        NixpkgsProblem::WrongCallPackage {
-                                            relative_package_file: relative_package_file.to_owned(),
+                                        // 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::WrongCallPackage {
                                         relative_package_file: relative_package_file.to_owned(),
                                         package_name: attribute_name.to_owned(),
diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index 0d0ddcd7e6327..24415424914e1 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -249,7 +249,7 @@ mod tests {
 
         if actual_errors != expected_errors {
             panic!(
-                "Failed test case {name}, expected these errors:\n\n{}\n\nbut got these:\n\n{}",
+                "Failed test case {name}, expected these errors:\n=======\n{}\n=======\nbut got these:\n=======\n{}\n=======",
                 expected_errors, actual_errors
             );
         }
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 18001eb44ecd5..dda89895697f0 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -1,8 +1,11 @@
 use crate::structure;
 use crate::utils::PACKAGE_NIX_FILENAME;
+use indoc::writedoc;
+use relative_path::RelativePath;
 use std::ffi::OsString;
 use std::fmt;
 use std::io;
+use std::path::Path;
 use std::path::PathBuf;
 
 /// Any problem that can occur when checking Nixpkgs
@@ -44,9 +47,19 @@ pub enum NixpkgsProblem {
         relative_package_file: PathBuf,
         package_name: String,
     },
+    WrongCallPackagePath {
+        package_name: String,
+        file: PathBuf,
+        line: usize,
+        column: usize,
+        actual_path: PathBuf,
+        expected_path: PathBuf,
+    },
     NonSyntacticCallPackage {
         package_name: String,
-        location: String,
+        file: PathBuf,
+        line: usize,
+        column: usize,
         definition: String,
     },
     NonDerivation {
@@ -169,12 +182,32 @@ 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 } =>
+            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, column, actual_path, expected_path } =>
+                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 the wrong path.
+                      It is defined in {}:{}:{} as
+
+                        {package_name} = callPackage {} {{ /* ... */ }}",
+                    structure::relative_dir_for_package(package_name).display(),
+                    create_path_expr(file, expected_path),
+                    file.display(), line, column,
+                    create_path_expr(file, actual_path),
+                },
+            NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, 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{}",
+                    "Because {} exists, the attribute `pkgs.{package_name}` must be defined as `callPackage {} {{ ... }}`. This is however not the case: The attribute is defined in {}:{}:{} as\n\t{}",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                    line,
+                    column,
                     definition,
                 ),
             NixpkgsProblem::NonDerivation { relative_package_file, package_name } =>
@@ -284,3 +317,17 @@ impl fmt::Display for NixpkgsProblem {
        }
     }
 }
+
+/// 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
+    // But these should never trigger because we only call this function with relative paths, and only
+    // with `from_file` being an actual file (so there's always a parent)
+    let from = RelativePath::from_path(&from_file)
+        .unwrap()
+        .parent()
+        .unwrap();
+    let to = RelativePath::from_path(&to_file).unwrap();
+    let rel = from.relative(to);
+    format!("./{rel}")
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
index 51479e22d26fe..3566b15bdbc40 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-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 the wrong path.
+  It is defined in all-packages.nix:2:3 as
+
+    nonDerivation = callPackage ./someDrv.nix { /* ... */ }

From 7258d472bcd8886be8b1463bea6030d1afe93bbf Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Thu, 22 Feb 2024 23:43:02 +0100
Subject: [PATCH 05/18] tests.nixpkgs-check-by-name: Improve non-syntactic
 callPackage error more

---
 pkgs/test/nixpkgs-check-by-name/Cargo.lock    | 30 +++++++++++++++
 pkgs/test/nixpkgs-check-by-name/Cargo.toml    |  1 +
 .../src/nixpkgs_problem.rs                    | 38 +++++++++++++++----
 .../expected                                  | 10 ++++-
 .../tests/override-different-file/expected    |  4 +-
 .../tests/override-no-call-package/expected   | 10 ++++-
 6 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.lock b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
index b02f7075ab613..19435c2ab76e3 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.lock
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
@@ -306,6 +306,7 @@ dependencies = [
  "serde_json",
  "temp-env",
  "tempfile",
+ "textwrap",
 ]
 
 [[package]]
@@ -489,6 +490,12 @@ version = "1.11.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "942b4a808e05215192e39f4ab80813e599068285906cc91aa64f923db842bd5a"
 
+[[package]]
+name = "smawk"
+version = "0.3.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "b7c388c1b5e93756d0c740965c41e8822f866621d41acbdf6336a6a168f8840c"
+
 [[package]]
 name = "strsim"
 version = "0.10.0"
@@ -534,12 +541,35 @@ version = "1.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "f18aa187839b2bdb1ad2fa35ead8c4c2976b64e4363c386d45ac0f7ee85c9233"
 
+[[package]]
+name = "textwrap"
+version = "0.16.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "23d434d3f8967a09480fb04132ebe0a3e088c173e6d0ee7897abbdf4eab0f8b9"
+dependencies = [
+ "smawk",
+ "unicode-linebreak",
+ "unicode-width",
+]
+
 [[package]]
 name = "unicode-ident"
 version = "1.0.11"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "301abaae475aa91687eb82514b328ab47a211a533026cb25fc3e519b86adfc3c"
 
+[[package]]
+name = "unicode-linebreak"
+version = "0.1.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3b09c83c3c29d37506a3e260c08c03743a6bb66a9cd432c6934ab501a190571f"
+
+[[package]]
+name = "unicode-width"
+version = "0.1.11"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e51733f11c9c4f72aa0c160008246859e340b00807569a0da0e7a1079b27ba85"
+
 [[package]]
 name = "utf8parse"
 version = "0.2.1"
diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
index 1b8f8e9085d4e..50cdabb7e2dd3 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
@@ -17,6 +17,7 @@ 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"
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 dda89895697f0..d8ae2db4dc22d 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -188,28 +188,50 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }}
+                        {package_name} = callPackage {} {{ /* ... */ }};
 
                       This is however not the case: The first `callPackage` argument is the wrong path.
                       It is defined in {}:{}:{} as
 
-                        {package_name} = callPackage {} {{ /* ... */ }}",
+                        {package_name} = callPackage {} {{ /* ... */ }};",
                     structure::relative_dir_for_package(package_name).display(),
                     create_path_expr(file, expected_path),
                     file.display(), line, column,
                     create_path_expr(file, actual_path),
                 },
-            NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } =>
-                write!(
+            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,
-                    "Because {} exists, the attribute `pkgs.{package_name}` must be defined as `callPackage {} {{ ... }}`. This is however not the case: The attribute is defined in {}:{}:{} as\n\t{}",
+                    "
+                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+
+                        {package_name} = callPackage {} {{ /* ... */ }};
+
+                      This is however not the case.
+                      It is defined in {}:{} as
+
+                    {}",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
                     line,
-                    column,
-                    definition,
-                ),
+                    definition_indented,
+                )
+            }
             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 f590ef4ff9fad..e620096338855 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,2 +1,8 @@
-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;
+- 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.
+  It is defined in all-packages.nix:4 as
+
+    foo = self.bar;
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
index 3566b15bdbc40..18bf137364fe8 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
@@ -1,8 +1,8 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ }
+    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
   This is however not the case: The first `callPackage` argument is the wrong path.
   It is defined in all-packages.nix:2:3 as
 
-    nonDerivation = callPackage ./someDrv.nix { /* ... */ }
+    nonDerivation = callPackage ./someDrv.nix { /* ... */ };
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 25fc867b04fe6..e31f9eb6d5390 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,2 +1,8 @@
-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;
+- 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.
+  It is defined in all-packages.nix:2 as
+
+    nonDerivation = self.someDrv;

From 75cdccd6c4753efe4fa543a05e4d1741cfb63218 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 00:10:20 +0100
Subject: [PATCH 06/18] tests.nixpkgs-check-by-name: Improve more errors

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 38 ++++-----
 .../nixpkgs-check-by-name/src/nix_file.rs     | 35 ++++----
 .../src/nixpkgs_problem.rs                    | 79 +++++++++++++++----
 .../tests/alt-callPackage/all-packages.nix    |  7 ++
 .../tests/alt-callPackage/default.nix         |  1 +
 .../tests/alt-callPackage/expected            |  8 ++
 .../pkgs/by-name/fo/foo/package.nix           |  1 +
 .../all-packages.nix                          |  1 -
 .../tests/override-no-file/expected           |  9 ++-
 .../tests/override-non-path/expected          |  9 ++-
 10 files changed, 134 insertions(+), 54 deletions(-)
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/all-packages.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/pkgs/by-name/fo/foo/package.nix

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index fa06e38c357ad..ddeac2a445b3f 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 3840f1dc479d6..1382f389e2dab 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 d8ae2db4dc22d..54976306df32d 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 0000000000000..399f8eee0a186
--- /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 0000000000000..861260cdca4b2
--- /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 0000000000000..25d2e6c4fa6a4
--- /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 0000000000000..a1b92efbbadb9
--- /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 3e0ea20c22810..e31aa831b8142 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 51479e22d26fe..0f8a4cb65eef3 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 9df788191ece0..41e40a9b3c3f7 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; };

From db7562e886d07ab365e8c55c50cf93dd709121ab Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:01:57 +0100
Subject: [PATCH 07/18] tests.nixpkgs-check-by-name: Improve errors relating to
 the base branch

---
 pkgs/test/nixpkgs-check-by-name/src/main.rs   | 94 ++++++++++++-------
 .../tests/aliases/expected                    |  1 +
 .../tests/alt-callPackage/expected            |  1 +
 .../tests/base-fixed/base/default.nix         |  1 +
 .../tests/base-fixed/base/pkgs/by-name/foo    |  0
 .../tests/base-fixed/default.nix              |  1 +
 .../tests/base-fixed/expected                 |  1 +
 .../tests/base-fixed/pkgs/by-name/README.md   |  0
 .../tests/base-still-broken/base/default.nix  |  1 +
 .../base-still-broken/base/pkgs/by-name/foo   |  0
 .../tests/base-still-broken/default.nix       |  1 +
 .../tests/base-still-broken/expected          |  3 +
 .../tests/base-still-broken/pkgs/by-name/bar  |  0
 .../tests/broken-autocall/expected            |  1 +
 .../tests/callPackage-syntax/expected         |  1 +
 .../tests/empty-base/expected                 |  1 +
 .../tests/incorrect-shard/expected            |  1 +
 .../tests/internalCallPackage/expected        |  1 +
 .../tests/invalid-package-name/expected       |  1 +
 .../tests/invalid-shard-name/expected         |  1 +
 .../tests/manual-definition/expected          |  1 +
 .../tests/missing-package-nix/expected        |  1 +
 .../tests/move-to-non-by-name/expected        |  1 +
 .../tests/multiple-failures/expected          |  1 +
 .../tests/new-package-non-by-name/expected    |  1 +
 .../tests/no-by-name/expected                 |  2 +-
 .../tests/no-eval/expected                    |  1 +
 .../tests/non-attrs/expected                  |  1 +
 .../tests/non-derivation/expected             |  1 +
 .../expected                                  |  1 +
 .../tests/one-letter/expected                 |  1 +
 .../only-callPackage-derivations/expected     |  1 +
 .../tests/override-different-file/expected    |  1 +
 .../tests/override-empty-arg-gradual/expected |  1 +
 .../tests/override-empty-arg/expected         |  1 +
 .../tests/override-no-call-package/expected   |  1 +
 .../tests/override-no-file/expected           |  1 +
 .../tests/override-non-path/expected          |  1 +
 .../tests/override-success/expected           |  1 +
 .../tests/package-dir-is-file/expected        |  1 +
 .../tests/package-nix-dir/expected            |  1 +
 .../tests/package-variants/expected           |  1 +
 .../tests/ref-absolute/expected               |  1 +
 .../tests/ref-escape/expected                 |  1 +
 .../tests/ref-nix-path/expected               |  1 +
 .../tests/ref-path-subexpr/expected           |  1 +
 .../tests/ref-success/expected                |  1 +
 .../tests/shard-file/expected                 |  1 +
 .../tests/sorted-order/expected               |  1 +
 .../tests/success/expected                    |  1 +
 .../tests/symlink-escape/expected             |  1 +
 .../tests/symlink-invalid/expected            |  1 +
 .../tests/unknown-location/expected           |  1 +
 .../tests/uppercase/expected                  |  1 +
 .../tests/with-readme/expected                |  1 +
 55 files changed, 110 insertions(+), 37 deletions(-)
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/aliases/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/pkgs/by-name/foo
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-fixed/pkgs/by-name/README.md
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/pkgs/by-name/foo
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/pkgs/by-name/bar
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/override-success/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/success/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected
 create mode 100644 pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected

diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index 24415424914e1..d5ea2c67438be 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -47,14 +47,8 @@ pub struct Args {
 fn main() -> ExitCode {
     let args = Args::parse();
     match process(&args.base, &args.nixpkgs, false, &mut io::stderr()) {
-        Ok(true) => {
-            eprintln!("{}", "Validated successfully".green());
-            ExitCode::SUCCESS
-        }
-        Ok(false) => {
-            eprintln!("{}", "Validation failed, see above errors".yellow());
-            ExitCode::from(1)
-        }
+        Ok(true) => ExitCode::SUCCESS,
+        Ok(false) => ExitCode::from(1),
         Err(e) => {
             eprintln!("{} {:#}", "I/O error: ".yellow(), e);
             ExitCode::from(2)
@@ -82,29 +76,58 @@ pub fn process<W: io::Write>(
     keep_nix_path: bool,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
-    // Check the main Nixpkgs first
-    let main_result = check_nixpkgs(main_nixpkgs, keep_nix_path, error_writer)?;
-    let check_result = main_result.result_map(|nixpkgs_version| {
-        // If the main Nixpkgs doesn't have any problems, run the ratchet checks against the base
-        // Nixpkgs
-        check_nixpkgs(base_nixpkgs, keep_nix_path, error_writer)?.result_map(
-            |base_nixpkgs_version| {
-                Ok(ratchet::Nixpkgs::compare(
-                    base_nixpkgs_version,
-                    nixpkgs_version,
-                ))
-            },
-        )
-    })?;
 
-    match check_result {
-        Failure(errors) => {
+    // TODO: Run in parallel
+    let base_result = check_nixpkgs(base_nixpkgs, keep_nix_path)?;
+    let main_result = check_nixpkgs(main_nixpkgs, keep_nix_path)?;
+
+    match (base_result, main_result) {
+        (Failure(_), Failure(errors)) => {
+            // Base branch fails and the PR doesn't fix it and may also introduce additional problems
+            for error in errors {
+                writeln!(error_writer, "{}", error.to_string().red())?
+            }
+            writeln!(error_writer, "{}", "The base branch is broken and still has above problems with this PR, these need to be fixed first.\nConsider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.".yellow())?;
+            Ok(false)
+        }
+        (Failure(_), Success(_)) => {
+            // Base branch fails, but the PR fixes it
+            writeln!(
+                error_writer,
+                "{}",
+                "The base branch was broken, but this PR fixes it, nice job!".green()
+            )?;
+            Ok(true)
+        }
+        (Success(_), Failure(errors)) => {
+            // Base branch succeeds, the PR breaks it
             for error in errors {
                 writeln!(error_writer, "{}", error.to_string().red())?
             }
+            writeln!(
+                error_writer,
+                "{}",
+                "This PR introduces the above problems, merging would break the base branch"
+                    .yellow()
+            )?;
             Ok(false)
         }
-        Success(()) => Ok(true),
+        (Success(base), Success(main)) => {
+            // Both base and main branch succeed, check ratchet state
+            match ratchet::Nixpkgs::compare(base, main) {
+                Failure(errors) => {
+                    for error in errors {
+                        writeln!(error_writer, "{}", error.to_string().red())?
+                    }
+                    writeln!(error_writer, "{}", "This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch".yellow())?;
+                    Ok(false)
+                }
+                Success(()) => {
+                    writeln!(error_writer, "{}", "Validated successfully".green())?;
+                    Ok(true)
+                }
+            }
+        }
     }
 }
 
@@ -113,10 +136,9 @@ pub fn process<W: io::Write>(
 /// This does not include ratchet checks, see ../README.md#ratchet-checks
 /// Instead a `ratchet::Nixpkgs` value is returned, whose `compare` method allows performing the
 /// ratchet check against another result.
-pub fn check_nixpkgs<W: io::Write>(
+pub fn check_nixpkgs(
     nixpkgs_path: &Path,
     keep_nix_path: bool,
-    error_writer: &mut W,
 ) -> validation::Result<ratchet::Nixpkgs> {
     let mut nix_file_store = NixFileStore::default();
 
@@ -129,11 +151,7 @@ pub fn check_nixpkgs<W: io::Write>(
         })?;
 
         if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() {
-            writeln!(
-                error_writer,
-                "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.",
-                utils::BASE_SUBPATH
-            )?;
+            // No pkgs/by-name directory, always valid
             Success(ratchet::Nixpkgs::default())
         } else {
             check_structure(&nixpkgs_path, &mut nix_file_store)?.result_map(|package_names|
@@ -163,8 +181,8 @@ mod tests {
                 continue;
             }
 
-            let expected_errors =
-                fs::read_to_string(path.join("expected")).unwrap_or(String::new());
+            let expected_errors = fs::read_to_string(path.join("expected"))
+                .expect("No expected file for test {name}");
 
             test_nixpkgs(&name, &path, &expected_errors)?;
         }
@@ -201,7 +219,7 @@ mod tests {
         test_nixpkgs(
             "case_sensitive",
             &path,
-            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\n",
+            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\nThis PR introduces the above problems, merging would break the base branch\n",
         )?;
 
         Ok(())
@@ -225,7 +243,11 @@ mod tests {
         let tmpdir = temp_root.path().join("symlinked");
 
         temp_env::with_var("TMPDIR", Some(&tmpdir), || {
-            test_nixpkgs("symlinked_tmpdir", Path::new("tests/success"), "")
+            test_nixpkgs(
+                "symlinked_tmpdir",
+                Path::new("tests/success"),
+                "Validated successfully\n",
+            )
         })
     }
 
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/aliases/expected b/pkgs/test/nixpkgs-check-by-name/tests/aliases/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/aliases/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 25d2e6c4fa6a4..3abb613de096e 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:5 as
 
     foo = self.alt.callPackage ({ }: self.someDrv) { };
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix
new file mode 100644
index 0000000000000..861260cdca4b2
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/pkgs/by-name/foo b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/base/pkgs/by-name/foo
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix
new file mode 100644
index 0000000000000..861260cdca4b2
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
new file mode 100644
index 0000000000000..feb6cce2fd04c
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
@@ -0,0 +1 @@
+The base branch was broken, but this PR fixes it, nice job!
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/pkgs/by-name/README.md b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/pkgs/by-name/README.md
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix
new file mode 100644
index 0000000000000..861260cdca4b2
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/pkgs/by-name/foo b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/base/pkgs/by-name/foo
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix
new file mode 100644
index 0000000000000..861260cdca4b2
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
new file mode 100644
index 0000000000000..4f0d08357d1fa
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
@@ -0,0 +1,3 @@
+pkgs/by-name/bar: This is a file, but it should be a directory.
+The base branch is broken and still has above problems with this PR, these need to be fixed first.
+Consider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/pkgs/by-name/bar b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/pkgs/by-name/bar
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
index fff17c6c7cd5a..6b937106b8060 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
@@ -1 +1,2 @@
 pkgs.foo: This attribute is not defined but it should be defined automatically as pkgs/by-name/fo/foo/package.nix
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected b/pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/empty-base/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
index 3627368c0ef0e..ec1918cace8c5 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/FOO: Incorrect directory location, should be pkgs/by-name/fo/FOO instead.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
index 404795ee5c79a..9fef30d14f376 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
@@ -1 +1,2 @@
 pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
index 8c8eafdcb3d12..aac1b186ca491 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
index 248aa8877966e..ce7c2695424c2 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
@@ -1 +1,2 @@
 pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 29d33f7dbdc00..1cc970b7fd725 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -1,2 +1,3 @@
 pkgs.noEval: 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/noEval/package.nix { ... }` with a non-empty second argument.
 pkgs.onlyMove: 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/on/onlyMove/package.nix { ... }` with a non-empty second argument.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
index ce1afcbf2d34d..93c882b2bd3cb 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 96da50b524916..8a2116aebcb9c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -2,3 +2,4 @@ pkgs.foo1: This top-level package was previously defined in pkgs/by-name/fo/foo1
 pkgs.foo2: This top-level package was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
 pkgs.foo3: This top-level package was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
 pkgs.foo4: This top-level package was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
index ff5d18556ef03..303884cb7944e 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
@@ -11,3 +11,4 @@ pkgs/by-name/ba/foo: File package.nix at line 3 contains the path expression "..
 pkgs/by-name/ba/foo: File package.nix at line 4 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
 pkgs/by-name/ba/foo: File package.nix at line 5 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index 3f294f26dfd83..31eff28f3c889 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -2,3 +2,4 @@ pkgs.new1: This is a new top-level package of the form `callPackage ... { }`. Pl
 pkgs.new2: This is a new top-level package of the form `callPackage ./without-config.nix { }`. Please define it in pkgs/by-name/ne/new2/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
 pkgs.new3: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/ne/new3/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
 pkgs.new4: This is a new top-level package of the form `callPackage ./with-config.nix { }`. Please define it in pkgs/by-name/ne/new4/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected
index ddcb2df46e5fb..defae2634c343 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected
@@ -1 +1 @@
-Given Nixpkgs path does not contain a pkgs/by-name subdirectory, no check necessary.
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected b/pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/no-eval/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
index e6c9237901029..48acbec1210de 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
@@ -1 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
index e6c9237901029..48acbec1210de 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
@@ -1 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
+This PR introduces the above problems, merging would break the base branch
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 e620096338855..d1a31336504a4 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
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:4 as
 
     foo = self.bar;
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected b/pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/one-letter/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected b/pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/only-callPackage-derivations/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
index 18bf137364fe8..6f439dd4b7d0b 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:2:3 as
 
     nonDerivation = callPackage ./someDrv.nix { /* ... */ };
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected
index e69de29bb2d1d..defae2634c343 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index 51479e22d26fe..bf96b181d7fee 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/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.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
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 e31f9eb6d5390..54d1eb51be34f 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
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.someDrv;
+This PR introduces the above problems, merging would break the base branch
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 0f8a4cb65eef3..a1980fdc08da9 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
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
+This PR introduces the above problems, merging would break the base branch
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 41e40a9b3c3f7..c8cb5be6d8ee7 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
@@ -6,3 +6,4 @@
   It is defined in all-packages.nix:3 as
 
     foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-success/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-success/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-success/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
index 3ad4b8f820f5b..864978b9b731c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: This path is a file, but it should be a directory.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
index 67a0c69fe29e5..82ad2ce85bbfc 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: "package.nix" must be a file.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-variants/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
index 7d20c32aad683..0e0e876d5561c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "/foo" which cannot be resolved: No such file or directory (os error 2).
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
index 3d7fb64e80a36..f414e4145674d 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "../." which may point outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
index b0cdff4a4778c..7b47a786a8be5 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
index ad662af27a86e..1905841a63df8 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
@@ -1 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-success/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
index 447b38e6b6c13..4a02d99778a9d 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo: This is a file, but it should be a directory.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 349e9ad47c41f..b835348cc3cf8 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -2,3 +2,4 @@ pkgs.a: This attribute is manually defined (most likely in pkgs/top-level/all-pa
 pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/b/b/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
 pkgs.c: 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/c/c/package.nix { ... }` with a non-empty second argument.
 pkgs.d: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/d/d/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
+This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/success/expected b/pkgs/test/nixpkgs-check-by-name/tests/success/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/success/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
index 335c5d6b6e5dc..e6c183207e578 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: Path package.nix is a symlink pointing to a path outside the directory of that package.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
index c1e7a28205a77..01de2702f7d52 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
@@ -1 +1,2 @@
 pkgs/by-name/fo/foo: Path foo is a symlink which cannot be resolved: No such file or directory (os error 2).
+This PR introduces the above problems, merging would break the base branch
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 2a248c23ab50c..d66a3185e701a 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,2 @@
 pkgs.foo: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.
+This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected b/pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/uppercase/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected b/pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected
new file mode 100644
index 0000000000000..defae2634c343
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/with-readme/expected
@@ -0,0 +1 @@
+Validated successfully

From f5e9b88af41e5c11b766fa9846d5f65b68fb2351 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:15:22 +0100
Subject: [PATCH 08/18] tests.nixpkgs-check-by-name: Evaluate base and main
 branch in parallel

---
 pkgs/test/nixpkgs-check-by-name/src/main.rs | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index d5ea2c67438be..c92d0af649692 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -1,4 +1,5 @@
 use crate::nix_file::NixFileStore;
+use std::panic;
 mod eval;
 mod nix_file;
 mod nixpkgs_problem;
@@ -17,6 +18,7 @@ use colored::Colorize;
 use std::io;
 use std::path::{Path, PathBuf};
 use std::process::ExitCode;
+use std::thread;
 
 /// Program to check the validity of pkgs/by-name
 ///
@@ -46,7 +48,7 @@ pub struct Args {
 
 fn main() -> ExitCode {
     let args = Args::parse();
-    match process(&args.base, &args.nixpkgs, false, &mut io::stderr()) {
+    match process(args.base, args.nixpkgs, false, &mut io::stderr()) {
         Ok(true) => ExitCode::SUCCESS,
         Ok(false) => ExitCode::from(1),
         Err(e) => {
@@ -71,15 +73,19 @@ fn main() -> ExitCode {
 /// - `Ok(false)` if there are problems, all of which will be written to `error_writer`.
 /// - `Ok(true)` if there are no problems
 pub fn process<W: io::Write>(
-    base_nixpkgs: &Path,
-    main_nixpkgs: &Path,
+    base_nixpkgs: PathBuf,
+    main_nixpkgs: PathBuf,
     keep_nix_path: bool,
     error_writer: &mut W,
 ) -> anyhow::Result<bool> {
+    // Very easy to parallelise this, since it's totally independent
+    let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs, keep_nix_path));
+    let main_result = check_nixpkgs(&main_nixpkgs, keep_nix_path)?;
 
-    // TODO: Run in parallel
-    let base_result = check_nixpkgs(base_nixpkgs, keep_nix_path)?;
-    let main_result = check_nixpkgs(main_nixpkgs, keep_nix_path)?;
+    let base_result = match base_thread.join() {
+        Ok(res) => res?,
+        Err(e) => panic::resume_unwind(e),
+    };
 
     match (base_result, main_result) {
         (Failure(_), Failure(errors)) => {
@@ -262,7 +268,7 @@ mod tests {
         // We don't want coloring to mess up the tests
         let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> {
             let mut writer = vec![];
-            process(base_nixpkgs, &path, true, &mut writer)
+            process(base_nixpkgs.to_owned(), path.to_owned(), true, &mut writer)
                 .with_context(|| format!("Failed test case {name}"))?;
             Ok(writer)
         })?;

From eb1f01440af0f73fdc8e4e214a9a8b70358df971 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:53:45 +0100
Subject: [PATCH 09/18] tests.nixpkgs-check-by-name: More consistent errors

---
 pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs      | 6 +++---
 .../nixpkgs-check-by-name/tests/alt-callPackage/expected    | 2 +-
 .../tests/non-syntactical-callPackage-by-name/expected      | 2 +-
 .../tests/override-no-call-package/expected                 | 2 +-
 .../nixpkgs-check-by-name/tests/override-no-file/expected   | 2 +-
 .../nixpkgs-check-by-name/tests/override-non-path/expected  | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

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 54976306df32d..808cda47cc1aa 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -202,7 +202,7 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
 
                       This is however not the case: A different `callPackage` is used.
                       It is defined in {}:{} as
@@ -220,7 +220,7 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
 
                       This is however not the case: The first callPackage argument is not right:
                       It is defined in {}:{} as
@@ -255,7 +255,7 @@ impl fmt::Display for NixpkgsProblem {
                     "
                     - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
 
                       This is however not the case.
                       It is defined in {}:{} as
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 3abb613de096e..80d79ecaba12f 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -1,6 +1,6 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    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
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 d1a31336504a4..5f914d1fce962 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,6 +1,6 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    foo = callPackage ./pkgs/by-name/fo/foo/package.nix { /* ... */ };
 
   This is however not the case.
   It is defined in all-packages.nix:4 as
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 54d1eb51be34f..f1fc4892b2abc 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,6 +1,6 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
   This is however not the case.
   It is defined in all-packages.nix:2 as
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 a1980fdc08da9..040d7c635f7a7 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,6 +1,6 @@
 - Because pkgs/by-name/no/nonDerivation exists, the attribute `pkgs.nonDerivation` must be defined like
 
-    nonDerivation = callPackage pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
+    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
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 c8cb5be6d8ee7..1f15c6473b4a5 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,6 +1,6 @@
 - Because pkgs/by-name/fo/foo exists, the attribute `pkgs.foo` must be defined like
 
-    foo = callPackage pkgs/by-name/fo/foo/package.nix { /* ... */ };
+    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

From 1c4308c35211e91bb97dedaaa1f0cba6e5b8322c Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 01:54:05 +0100
Subject: [PATCH 10/18] tests.nixpkgs-check-by-name: Better error for empty
 second arg

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   |  8 ++++-
 .../src/nixpkgs_problem.rs                    | 33 ++++++++++++++-----
 .../test/nixpkgs-check-by-name/src/ratchet.rs | 12 +++----
 .../nixpkgs-check-by-name/src/references.rs   |  4 +--
 .../tests/manual-definition/expected          | 18 ++++++++--
 .../tests/override-empty-arg/expected         |  9 ++++-
 .../tests/sorted-order/expected               | 18 ++++++++--
 7 files changed, 77 insertions(+), 25 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index ddeac2a445b3f..4748e907db725 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -339,7 +339,13 @@ fn by_name(
                                         // Manual definitions with empty arguments are not allowed
                                         // anymore
                                         Success(if syntactic_call_package.empty_arg {
-                                            Loose(())
+                                            Loose(NixpkgsProblem::EmptyArgument {
+                                                package_name: attribute_name.to_owned(),
+                                                file: relative_file,
+                                                line: location.line,
+                                                column: location.column,
+                                                definition,
+                                            })
                                         } else {
                                             Tight
                                         })
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 808cda47cc1aa..cf40238f9cb6a 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -4,11 +4,11 @@ use indoc::writedoc;
 use relative_path::RelativePath;
 use std::ffi::OsString;
 use std::fmt;
-use std::io;
 use std::path::Path;
 use std::path::PathBuf;
 
 /// Any problem that can occur when checking Nixpkgs
+#[derive(Clone)]
 pub enum NixpkgsProblem {
     ShardNonDir {
         relative_shard_path: PathBuf,
@@ -43,9 +43,12 @@ pub enum NixpkgsProblem {
         relative_package_file: PathBuf,
         package_name: String,
     },
-    WrongCallPackage {
-        relative_package_file: PathBuf,
+    EmptyArgument {
         package_name: String,
+        file: PathBuf,
+        line: usize,
+        column: usize,
+        definition: String,
     },
     NonToplevelCallPackage {
         package_name: String,
@@ -87,7 +90,7 @@ pub enum NixpkgsProblem {
     UnresolvableSymlink {
         relative_package_dir: PathBuf,
         subpath: PathBuf,
-        io_error: io::Error,
+        io_error: String,
     },
     PathInterpolation {
         relative_package_dir: PathBuf,
@@ -112,7 +115,7 @@ pub enum NixpkgsProblem {
         subpath: PathBuf,
         line: usize,
         text: String,
-        io_error: io::Error,
+        io_error: String,
     },
     MovedOutOfByName {
         package_name: String,
@@ -190,11 +193,23 @@ impl fmt::Display for NixpkgsProblem {
                     "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}",
                     relative_package_file.display()
                 ),
-            NixpkgsProblem::WrongCallPackage { relative_package_file, package_name } =>
-                write!(
+            NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } =>
+                writedoc!(
                     f,
-                    "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()
+                    "
+                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+
+                        {package_name} = callPackage ./{} {{ /* ... */ }};
+
+                      Notably the second argument must not be empty, which is not the case.
+                      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::NonToplevelCallPackage { package_name, file, line, column, definition } =>
                 writedoc!(
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index 200bf92c516a1..bdc1fbec4a273 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -4,7 +4,6 @@
 
 use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
-use crate::structure;
 use crate::validation::{self, Validation, Validation::Success};
 use std::collections::HashMap;
 
@@ -127,17 +126,14 @@ impl<Context: ToNixpkgsProblem> RatchetState<Context> {
 pub enum ManualDefinition {}
 
 impl ToNixpkgsProblem for ManualDefinition {
-    type ToContext = ();
+    type ToContext = NixpkgsProblem;
 
     fn to_nixpkgs_problem(
-        name: &str,
+        _name: &str,
         _optional_from: Option<()>,
-        _to: &Self::ToContext,
+        to: &Self::ToContext,
     ) -> NixpkgsProblem {
-        NixpkgsProblem::WrongCallPackage {
-            relative_package_file: structure::relative_file_for_package(name),
-            package_name: name.to_owned(),
-        }
+        (*to).clone()
     }
 }
 
diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs
index 169e996300baa..b716b99099c49 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/references.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs
@@ -66,7 +66,7 @@ fn check_path(
             Err(io_error) => NixpkgsProblem::UnresolvableSymlink {
                 relative_package_dir: relative_package_dir.to_path_buf(),
                 subpath: subpath.to_path_buf(),
-                io_error,
+                io_error: io_error.to_string(),
             }
             .into(),
         }
@@ -160,7 +160,7 @@ fn check_nix_file(
                     subpath: subpath.to_path_buf(),
                     line,
                     text,
-                    io_error: e,
+                    io_error: e.to_string(),
                 }
                 .into(),
                 ResolvedPath::Within(..) => {
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 1cc970b7fd725..e619887012d0a 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -1,3 +1,17 @@
-pkgs.noEval: 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/noEval/package.nix { ... }` with a non-empty second argument.
-pkgs.onlyMove: 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/on/onlyMove/package.nix { ... }` with a non-empty second argument.
+- Because pkgs/by-name/no/noEval exists, the attribute `pkgs.noEval` must be defined like
+
+    noEval = callPackage ./pkgs/by-name/no/noEval/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:9 as
+
+    noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
+- Because pkgs/by-name/on/onlyMove exists, the attribute `pkgs.onlyMove` must be defined like
+
+    onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:7 as
+
+    onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index bf96b181d7fee..dc64698f4bfaa 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -1,2 +1,9 @@
-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 { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:2 as
+
+    nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index b835348cc3cf8..d96d0d5638e54 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -1,5 +1,19 @@
-pkgs.a: 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/a/a/package.nix { ... }` with a non-empty second argument.
+- Because pkgs/by-name/a/a exists, the attribute `pkgs.a` must be defined like
+
+    a = callPackage ./pkgs/by-name/a/a/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:2 as
+
+    a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
 pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/b/b/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
-pkgs.c: 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/c/c/package.nix { ... }` with a non-empty second argument.
+- Because pkgs/by-name/c/c exists, the attribute `pkgs.c` must be defined like
+
+    c = callPackage ./pkgs/by-name/c/c/package.nix { /* ... */ };
+
+  Notably the second argument must not be empty, which is not the case.
+  It is defined in all-packages.nix:4 as
+
+    c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
 pkgs.d: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/d/d/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch

From ce271786deaef526b9c99da85bf84ab4ff24a472 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 02:16:49 +0100
Subject: [PATCH 11/18] tests.nixpkgs-check-by-name: More spaces in errors

---
 .../nixpkgs-check-by-name/src/nixpkgs_problem.rs  | 15 ++++++++++-----
 .../tests/alt-callPackage/expected                |  1 +
 .../tests/manual-definition/expected              |  2 ++
 .../non-syntactical-callPackage-by-name/expected  |  1 +
 .../tests/override-different-file/expected        |  1 +
 .../tests/override-empty-arg/expected             |  1 +
 .../tests/override-no-call-package/expected       |  1 +
 .../tests/override-no-file/expected               |  1 +
 .../tests/override-non-path/expected              |  1 +
 9 files changed, 19 insertions(+), 5 deletions(-)

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 cf40238f9cb6a..762d0e9fb0e04 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -204,7 +204,8 @@ impl fmt::Display for NixpkgsProblem {
                       Notably the second argument must not be empty, which is not the case.
                       It is defined in {}:{} as
 
-                    {}",
+                    {}
+                    ",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
@@ -222,7 +223,8 @@ impl fmt::Display for NixpkgsProblem {
                       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(),
@@ -240,7 +242,8 @@ impl fmt::Display for NixpkgsProblem {
                       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(),
@@ -258,7 +261,8 @@ impl fmt::Display for NixpkgsProblem {
                       This is however not the case: The first `callPackage` argument is the wrong path.
                       It is defined in {}:{}:{} as
 
-                        {package_name} = callPackage {} {{ /* ... */ }};",
+                        {package_name} = callPackage {} {{ /* ... */ }};
+                    ",
                     structure::relative_dir_for_package(package_name).display(),
                     create_path_expr(file, expected_path),
                     file.display(), line, column,
@@ -275,7 +279,8 @@ impl fmt::Display for NixpkgsProblem {
                       This is however not the case.
                       It is defined in {}:{} as
 
-                    {}",
+                    {}
+                    ",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
                     file.display(),
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 80d79ecaba12f..2d05db0d46993 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:5 as
 
     foo = self.alt.callPackage ({ }: self.someDrv) { };
+
 This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index e619887012d0a..71d78019604b6 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -6,6 +6,7 @@
   It is defined in all-packages.nix:9 as
 
     noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
+
 - Because pkgs/by-name/on/onlyMove exists, the attribute `pkgs.onlyMove` must be defined like
 
     onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
@@ -14,4 +15,5 @@
   It is defined in all-packages.nix:7 as
 
     onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
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 5f914d1fce962..f56585ac46783 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
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:4 as
 
     foo = self.bar;
+
 This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
index 6f439dd4b7d0b..571ec8e09a5c2 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2:3 as
 
     nonDerivation = callPackage ./someDrv.nix { /* ... */ };
+
 This PR introduces the above problems, merging would break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index dc64698f4bfaa..257030e9a6c0e 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
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 f1fc4892b2abc..eb13c4df1e888 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
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.someDrv;
+
 This PR introduces the above problems, merging would break the base branch
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 040d7c635f7a7..31679e55b7d61 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
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:2 as
 
     nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
+
 This PR introduces the above problems, merging would break the base branch
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 1f15c6473b4a5..e7fd8242f5f47 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
@@ -6,4 +6,5 @@
   It is defined in all-packages.nix:3 as
 
     foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
+
 This PR introduces the above problems, merging would break the base branch

From d2fa5bafa9faff354c13c3117deb3025ca4a5795 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 02:17:47 +0100
Subject: [PATCH 12/18] tests.nixpkgs-check-by-name: Improve errors for new
 packages

Or rather, make it more consistent
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   |  2 +-
 .../src/nixpkgs_problem.rs                    | 35 +++++++++++++------
 .../test/nixpkgs-check-by-name/src/ratchet.rs |  7 ++--
 .../tests/move-to-non-by-name/expected        | 16 ++++++---
 .../tests/new-package-non-by-name/expected    | 24 ++++++++++---
 .../tests/sorted-order/expected               | 14 ++++++--
 6 files changed, 75 insertions(+), 23 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 4748e907db725..a8427a27ee43e 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -504,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))
                     }
                 }
             }
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 762d0e9fb0e04..fac789e3e03fb 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -121,11 +121,13 @@ pub enum NixpkgsProblem {
         package_name: String,
         call_package_path: Option<PathBuf>,
         empty_arg: bool,
+        file: PathBuf,
     },
     NewPackageNotUsingByName {
         package_name: String,
         call_package_path: Option<PathBuf>,
         empty_arg: bool,
+        file: PathBuf,
     },
     InternalCallPackageUsed {
         attr_name: String,
@@ -340,7 +342,7 @@ impl fmt::Display for NixpkgsProblem {
                     subpath.display(),
                     text,
                 ),
-            NixpkgsProblem::MovedOutOfByName { package_name, call_package_path, empty_arg } => {
+            NixpkgsProblem::MovedOutOfByName { package_name, call_package_path, empty_arg, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
@@ -348,22 +350,30 @@ impl fmt::Display for NixpkgsProblem {
                         "...".into()
                     };
                 if *empty_arg {
-                    write!(
+                    writedoc!(
                         f,
-                        "pkgs.{package_name}: This top-level package was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ }}` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.",
+                        "
+                        - Attribute `pkgs.{package_name} was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
+                          Please move the package back and remove the manual `callPackage`.
+                        ",
                         structure::relative_file_for_package(package_name).display(),
+                        file.display(),
                         )
                 } else {
                     // This can happen if users mistakenly assume that for custom arguments,
                     // pkgs/by-name can't be used.
-                    write!(
+                    writedoc!(
                         f,
-                        "pkgs.{package_name}: This top-level package was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.",
+                        "
+                        - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
+                          While the manual `callPackage` is still needed, it's not necessary to move the package files.
+                        ",
                         structure::relative_file_for_package(package_name).display(),
+                        file.display(),
                         )
                 }
             },
-            NixpkgsProblem::NewPackageNotUsingByName { package_name, call_package_path, empty_arg } => {
+            NixpkgsProblem::NewPackageNotUsingByName { package_name, call_package_path, empty_arg, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
@@ -372,13 +382,18 @@ impl fmt::Display for NixpkgsProblem {
                     };
                 let extra =
                     if *empty_arg {
-                        "Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore."
+                        format!("Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.", file.display())
                     } else {
-                        "Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed."
+                        format!("Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.", file.display())
                     };
-                write!(
+                writedoc!(
                     f,
-                    "pkgs.{package_name}: This is a new top-level package of the form `callPackage {call_package_arg} {{ }}`. Please define it in {} instead. See `pkgs/by-name/README.md` for more details. {extra}",
+                    "
+                    - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
+                      Please define it in {} instead.
+                      See `pkgs/by-name/README.md` for more details.
+                      {extra}
+                    ",
                     structure::relative_file_for_package(package_name).display(),
                 )
             },
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index bdc1fbec4a273..28036d3689f15 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -6,6 +6,7 @@ use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::validation::{self, Validation, Validation::Success};
 use std::collections::HashMap;
+use std::path::PathBuf;
 
 /// The ratchet value for the entirety of Nixpkgs.
 #[derive(Default)]
@@ -145,24 +146,26 @@ impl ToNixpkgsProblem for ManualDefinition {
 pub enum UsesByName {}
 
 impl ToNixpkgsProblem for UsesByName {
-    type ToContext = CallPackageArgumentInfo;
+    type ToContext = (CallPackageArgumentInfo, PathBuf);
 
     fn to_nixpkgs_problem(
         name: &str,
         optional_from: Option<()>,
-        to: &Self::ToContext,
+        (to, file): &Self::ToContext,
     ) -> NixpkgsProblem {
         if let Some(()) = optional_from {
             NixpkgsProblem::MovedOutOfByName {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
                 empty_arg: to.empty_arg,
+                file: file.to_owned(),
             }
         } else {
             NixpkgsProblem::NewPackageNotUsingByName {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
                 empty_arg: to.empty_arg,
+                file: file.to_owned(),
             }
         }
     }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 8a2116aebcb9c..3319cc271ac9f 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -1,5 +1,13 @@
-pkgs.foo1: This top-level package was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
-pkgs.foo2: This top-level package was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
-pkgs.foo3: This top-level package was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
-pkgs.foo4: This top-level package was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
+- Attribute `pkgs.foo1 was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  Please move the package back and remove the manual `callPackage`.
+
+- Attribute `pkgs.foo2 was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  Please move the package back and remove the manual `callPackage`.
+
+- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  While the manual `callPackage` is still needed, it's not necessary to move the package files.
+
+- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+  While the manual `callPackage` is still needed, it's not necessary to move the package files.
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index 31eff28f3c889..a7db19e5fd907 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -1,5 +1,21 @@
-pkgs.new1: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/ne/new1/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
-pkgs.new2: This is a new top-level package of the form `callPackage ./without-config.nix { }`. Please define it in pkgs/by-name/ne/new2/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
-pkgs.new3: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/ne/new3/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
-pkgs.new4: This is a new top-level package of the form `callPackage ./with-config.nix { }`. Please define it in pkgs/by-name/ne/new4/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is not `{ }`, the manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is still needed.
+- Attribute `pkgs.new1` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new1/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+
+- Attribute `pkgs.new2` is a new top-level package using `pkgs.callPackage ./without-config.nix { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new2/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+
+- Attribute `pkgs.new3` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new3/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+
+- Attribute `pkgs.new4` is a new top-level package using `pkgs.callPackage ./with-config.nix { /* ... */ }`.
+  Please define it in pkgs/by-name/ne/new4/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index d96d0d5638e54..2b3c2cdeee183 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -6,7 +6,12 @@
   It is defined in all-packages.nix:2 as
 
     a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
-pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/b/b/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
+
+- Attribute `pkgs.b` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/b/b/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+
 - Because pkgs/by-name/c/c exists, the attribute `pkgs.c` must be defined like
 
     c = callPackage ./pkgs/by-name/c/c/package.nix { /* ... */ };
@@ -15,5 +20,10 @@ pkgs.b: This is a new top-level package of the form `callPackage ... { }`. Pleas
   It is defined in all-packages.nix:4 as
 
     c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
-pkgs.d: This is a new top-level package of the form `callPackage ... { }`. Please define it in pkgs/by-name/d/d/package.nix instead. See `pkgs/by-name/README.md` for more details. Since the second `callPackage` argument is `{ }`, no manual `callPackage` (e.g. in `pkgs/top-level/all-packages.nix`) is needed anymore.
+
+- Attribute `pkgs.d` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
+  Please define it in pkgs/by-name/d/d/package.nix instead.
+  See `pkgs/by-name/README.md` for more details.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+
 This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch

From 64da6178bf10b92f57352d7d0cfca7eebbd527df Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 23 Feb 2024 02:32:27 +0100
Subject: [PATCH 13/18] tests.nixpkgs-check-by-name: Fix internal docs

---
 pkgs/test/nixpkgs-check-by-name/src/nix_file.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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 1382f389e2dab..0176164e2ceae 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -86,7 +86,7 @@ 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>;`, `Ok(Err(String))` is
+    /// If the location is not of the form `<attr> = callPackage <arg1> <arg2>;`, `Ok((None, 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,
@@ -109,7 +109,10 @@ impl NixFile {
     ///
     /// You'll get back
     /// ```rust
-    /// Ok(Ok(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }))
+    /// Ok((
+    ///   Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true }),
+    ///   "foo = self.callPackage ./default.nix { };",
+    /// ))
     /// ```
     ///
     /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an

From 2e8d778994b4e2a285bb599808a1b168077e5a66 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 01:12:11 +0100
Subject: [PATCH 14/18] tests.nixpkgs-check-by-name: Various minor improvements

Co-Authored-By: Philip Taron <philip.taron@gmail.com>
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 175 ++++++++++--------
 pkgs/test/nixpkgs-check-by-name/src/main.rs   |  13 +-
 .../nixpkgs-check-by-name/src/nix_file.rs     |  39 ++--
 .../src/nixpkgs_problem.rs                    | 126 +++++++------
 .../test/nixpkgs-check-by-name/src/ratchet.rs |  20 +-
 .../tests/alt-callPackage/expected            |   5 +-
 .../tests/base-fixed/expected                 |   2 +-
 .../tests/base-still-broken/expected          |   2 +-
 .../tests/broken-autocall/expected            |   2 +-
 .../tests/incorrect-shard/expected            |   2 +-
 .../tests/internalCallPackage/expected        |   2 +-
 .../tests/invalid-package-name/expected       |   2 +-
 .../tests/invalid-shard-name/expected         |   2 +-
 .../tests/manual-definition/expected          |   8 +-
 .../tests/missing-package-nix/expected        |   2 +-
 .../tests/move-to-non-by-name/expected        |   6 +-
 .../tests/multiple-failures/expected          |   2 +-
 .../tests/new-package-non-by-name/expected    |   2 +-
 .../tests/non-attrs/expected                  |   2 +-
 .../tests/non-derivation/expected             |   2 +-
 .../all-packages.nix                          |   1 +
 .../expected                                  |   5 +-
 .../tests/override-different-file/expected    |   5 +-
 .../tests/override-empty-arg/expected         |   5 +-
 .../tests/override-no-call-package/expected   |   5 +-
 .../tests/override-no-file/expected           |   5 +-
 .../tests/override-non-path/expected          |   5 +-
 .../tests/package-dir-is-file/expected        |   2 +-
 .../tests/package-nix-dir/expected            |   2 +-
 .../tests/ref-absolute/expected               |   2 +-
 .../tests/ref-escape/expected                 |   2 +-
 .../tests/ref-nix-path/expected               |   2 +-
 .../tests/ref-path-subexpr/expected           |   2 +-
 .../tests/shard-file/expected                 |   2 +-
 .../tests/sorted-order/expected               |   8 +-
 .../tests/symlink-escape/expected             |   2 +-
 .../tests/symlink-invalid/expected            |   2 +-
 .../tests/unknown-location/expected           |   2 +-
 38 files changed, 262 insertions(+), 213 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index a8427a27ee43e..a43a0cd743a73 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -1,5 +1,8 @@
+use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::ratchet;
+use crate::ratchet::RatchetState::Loose;
+use crate::ratchet::RatchetState::Tight;
 use crate::structure;
 use crate::utils;
 use crate::validation::ResultIteratorExt as _;
@@ -296,84 +299,27 @@ fn by_name(
                         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}"
-                                )
-                            })?;
+                        let relative_location_file = location.relative_file(nixpkgs_path).with_context(|| {
+                            format!("Failed to resolve the file where attribute {attribute_name} is defined")
+                        })?;
 
                         // Figure out whether it's an attribute definition of the form `= callPackage <arg1> <arg2>`,
                         // returning the arguments if so.
-                        let (optional_syntactic_call_package, definition) = nix_file.call_package_argument_info_at(
-                            location.line,
-                            location.column,
-                            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> = 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,
-                            }
-                            .into(),
-                            // Something like `<attr> = pkgs.callPackage ...`
-                            (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
-                                        // 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 {
-                                        // 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()
-                                }
-                            }
-                        }
+                        let (optional_syntactic_call_package, definition) = nix_file
+                            .call_package_argument_info_at(location.line, location.column, nixpkgs_path)
+                            .with_context(|| {
+                                format!("Failed to get the definition info for attribute {attribute_name}")
+                            })?;
+
+                        by_name_override(
+                            attribute_name,
+                            relative_package_file,
+                            is_semantic_call_package,
+                            optional_syntactic_call_package,
+                            definition,
+                            location,
+                            relative_location_file,
+                        )
                     } else {
                         // If manual definitions don't have a location, it's likely `mapAttrs`'d
                         // over, e.g. if it's defined in aliases.nix.
@@ -401,6 +347,85 @@ fn by_name(
     )
 }
 
+/// Handles the case for packages in `pkgs/by-name` that are manually overridden, e.g. in
+/// all-packages.nix
+fn by_name_override(
+    attribute_name: &str,
+    expected_package_file: PathBuf,
+    is_semantic_call_package: bool,
+    optional_syntactic_call_package: Option<CallPackageArgumentInfo>,
+    definition: String,
+    location: Location,
+    relative_location_file: PathBuf,
+) -> validation::Validation<ratchet::RatchetState<ratchet::ManualDefinition>> {
+    // 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> = foo`
+        (_, None) => NixpkgsProblem::NonSyntacticCallPackage {
+            package_name: attribute_name.to_owned(),
+            file: relative_location_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_location_file,
+            line: location.line,
+            column: location.column,
+            definition,
+        }
+        .into(),
+        // Something like `<attr> = pkgs.callPackage ...`
+        (true, Some(syntactic_call_package)) => {
+            if let Some(actual_package_file) = syntactic_call_package.relative_path {
+                if actual_package_file != expected_package_file {
+                    // Wrong path
+                    NixpkgsProblem::WrongCallPackagePath {
+                        package_name: attribute_name.to_owned(),
+                        file: relative_location_file,
+                        line: location.line,
+                        actual_path: actual_package_file,
+                        expected_path: expected_package_file,
+                    }
+                    .into()
+                } else {
+                    // Manual definitions with empty arguments are not allowed
+                    // anymore, but existing ones should continue to be allowed
+                    let manual_definition_ratchet = if syntactic_call_package.empty_arg {
+                        // This is the state to migrate away from
+                        Loose(NixpkgsProblem::EmptyArgument {
+                            package_name: attribute_name.to_owned(),
+                            file: relative_location_file,
+                            line: location.line,
+                            column: location.column,
+                            definition,
+                        })
+                    } else {
+                        // This is the state to migrate to
+                        Tight
+                    };
+
+                    Success(manual_definition_ratchet)
+                }
+            } else {
+                // No path
+                NixpkgsProblem::NonPath {
+                    package_name: attribute_name.to_owned(),
+                    file: relative_location_file,
+                    line: location.line,
+                    column: location.column,
+                    definition,
+                }
+                .into()
+            }
+        }
+    }
+}
+
 /// Handles the evaluation result for an attribute _not_ in `pkgs/by-name`,
 /// turning it into a validation result.
 fn handle_non_by_name_attribute(
diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index c92d0af649692..dcc9cb9e716d5 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -93,27 +93,25 @@ pub fn process<W: io::Write>(
             for error in errors {
                 writeln!(error_writer, "{}", error.to_string().red())?
             }
-            writeln!(error_writer, "{}", "The base branch is broken and still has above problems with this PR, these need to be fixed first.\nConsider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.".yellow())?;
+            writeln!(error_writer, "{}", "The base branch is broken and still has above problems with this PR, which need to be fixed first.\nConsider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.".yellow())?;
             Ok(false)
         }
         (Failure(_), Success(_)) => {
-            // Base branch fails, but the PR fixes it
             writeln!(
                 error_writer,
                 "{}",
-                "The base branch was broken, but this PR fixes it, nice job!".green()
+                "The base branch is broken, but this PR fixes it. Nice job!".green()
             )?;
             Ok(true)
         }
         (Success(_), Failure(errors)) => {
-            // Base branch succeeds, the PR breaks it
             for error in errors {
                 writeln!(error_writer, "{}", error.to_string().red())?
             }
             writeln!(
                 error_writer,
                 "{}",
-                "This PR introduces the above problems, merging would break the base branch"
+                "This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break."
                     .yellow()
             )?;
             Ok(false)
@@ -125,7 +123,8 @@ pub fn process<W: io::Write>(
                     for error in errors {
                         writeln!(error_writer, "{}", error.to_string().red())?
                     }
-                    writeln!(error_writer, "{}", "This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch".yellow())?;
+                    writeln!(error_writer, "{}", "This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.".yellow())?;
+
                     Ok(false)
                 }
                 Success(()) => {
@@ -225,7 +224,7 @@ mod tests {
         test_nixpkgs(
             "case_sensitive",
             &path,
-            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\nThis PR introduces the above problems, merging would break the base branch\n",
+            "pkgs/by-name/fo: Duplicate case-sensitive package directories \"foO\" and \"foo\".\nThis PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.\n",
         )?;
 
         Ok(())
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 0176164e2ceae..1cb5d1c0d08d3 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -2,6 +2,7 @@
 
 use crate::utils::LineIndex;
 use anyhow::Context;
+use itertools::Either::{self, Left, Right};
 use rnix::ast;
 use rnix::ast::Expr;
 use rnix::ast::HasEntry;
@@ -86,8 +87,8 @@ 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>;`, `Ok((None, String))` is
-    /// returned, with String being how the definition looks like.
+    /// If the definition at the given location is not of the form `<attr> = callPackage <arg1> <arg2>;`,
+    /// `Ok((None, String))` is returned, with `String` being the definition itself.
     ///
     /// This function only returns `Err` for problems that can't be caused by the Nix contents,
     /// but rather problems in this programs code itself.
@@ -124,8 +125,8 @@ impl NixFile {
         relative_to: &Path,
     ) -> anyhow::Result<(Option<CallPackageArgumentInfo>, String)> {
         Ok(match self.attrpath_value_at(line, column)? {
-            Err(definition) => (None, definition),
-            Ok(attrpath_value) => {
+            Left(definition) => (None, definition),
+            Right(attrpath_value) => {
                 let definition = attrpath_value.to_string();
                 let attrpath_value =
                     self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?;
@@ -139,7 +140,7 @@ impl NixFile {
         &self,
         line: usize,
         column: usize,
-    ) -> anyhow::Result<Result<ast::AttrpathValue, String>> {
+    ) -> anyhow::Result<Either<String, ast::AttrpathValue>> {
         let index = self.line_index.fromlinecolumn(line, column);
 
         let token_at_offset = self
@@ -173,11 +174,11 @@ 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(Err(node.to_string()));
+            return Ok(Left(node.to_string()));
         } else {
             // However, anything else is not expected and smells like a bug
             anyhow::bail!(
-                "Node in {} is neither an attribute node, nor an inherit node: {node:?}",
+                "Node in {} is neither an attribute node nor an inherit node: {node:?}",
                 self.path.display()
             )
         }
@@ -217,7 +218,9 @@ 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(Ok(ast::AttrpathValue::cast(attrpath_value_node).unwrap()))
+        Ok(Right(
+            ast::AttrpathValue::cast(attrpath_value_node).unwrap(),
+        ))
     }
 
     // Internal function mainly to make attrpath_value_at independently testable
@@ -446,24 +449,24 @@ mod tests {
 
         // These are builtins.unsafeGetAttrPos locations for the attributes
         let cases = [
-            (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;")),
+            (2, 3, Right("foo = 1;")),
+            (3, 3, Right(r#""bar" = 2;"#)),
+            (4, 3, Right(r#"${"baz"} = 3;"#)),
+            (5, 3, Right(r#""${"qux"}" = 4;"#)),
+            (8, 3, Right("quux\n  # B\n  =\n  # C\n  5\n  # D\n  ;")),
+            (17, 7, Right("quuux/**/=/**/5/**/;")),
+            (19, 10, Left("inherit toInherit;")),
+            (20, 22, Left("inherit (toInherit) toInherit;")),
         ];
 
         for (line, column, expected_result) in cases {
             let actual_result = nix_file
                 .attrpath_value_at(line, column)
                 .context(format!("line {line}, column {column}"))?
-                .map(|node| node.to_string());
+                .map_right(|node| node.to_string());
             let owned_expected_result = expected_result
                 .map(|x| x.to_string())
-                .map_err(|x| x.to_string());
+                .map_left(|x| x.to_string());
             assert_eq!(
                 actual_result, owned_expected_result,
                 "line {line}, column {column}"
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 fac789e3e03fb..03203073fd4a1 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -68,7 +68,6 @@ pub enum NixpkgsProblem {
         package_name: String,
         file: PathBuf,
         line: usize,
-        column: usize,
         actual_path: PathBuf,
         expected_path: PathBuf,
     },
@@ -117,16 +116,24 @@ pub enum NixpkgsProblem {
         text: String,
         io_error: String,
     },
-    MovedOutOfByName {
+    MovedOutOfByNameEmptyArg {
+        package_name: String,
+        call_package_path: Option<PathBuf>,
+        file: PathBuf,
+    },
+    MovedOutOfByNameNonEmptyArg {
         package_name: String,
         call_package_path: Option<PathBuf>,
-        empty_arg: bool,
         file: PathBuf,
     },
-    NewPackageNotUsingByName {
+    NewPackageNotUsingByNameEmptyArg {
+        package_name: String,
+        call_package_path: Option<PathBuf>,
+        file: PathBuf,
+    },
+    NewPackageNotUsingByNameNonEmptyArg {
         package_name: String,
         call_package_path: Option<PathBuf>,
-        empty_arg: bool,
         file: PathBuf,
     },
     InternalCallPackageUsed {
@@ -203,8 +210,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      Notably the second argument must not be empty, which is not the case.
-                      It is defined in {}:{} as
+                      However, in this PR, the second argument is empty. See the definition in {}:{}:
 
                     {}
                     ",
@@ -222,8 +228,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      This is however not the case: A different `callPackage` is used.
-                      It is defined in {}:{} as
+                      However, in this PR, a different `callPackage` is used. See the definition in {}:{}:
 
                     {}
                     ",
@@ -241,8 +246,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      This is however not the case: The first callPackage argument is not right:
-                      It is defined in {}:{} as
+                      However, in this PR, the first `callPackage` argument is not a path. See the definition in {}:{}:
 
                     {}
                     ",
@@ -252,7 +256,7 @@ impl fmt::Display for NixpkgsProblem {
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
-            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, column, actual_path, expected_path } =>
+            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, actual_path, expected_path } =>
                 writedoc! {
                     f,
                     "
@@ -260,14 +264,13 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage {} {{ /* ... */ }};
 
-                      This is however not the case: The first `callPackage` argument is the wrong path.
-                      It is defined in {}:{}:{} as
+                      However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {}:{}:
 
                         {package_name} = callPackage {} {{ /* ... */ }};
                     ",
                     structure::relative_dir_for_package(package_name).display(),
                     create_path_expr(file, expected_path),
-                    file.display(), line, column,
+                    file.display(), line,
                     create_path_expr(file, actual_path),
                 },
             NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => {
@@ -278,8 +281,7 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage ./{} {{ /* ... */ }};
 
-                      This is however not the case.
-                      It is defined in {}:{} as
+                      However, in this PR, it isn't defined that way. See the definition in {}:{}
 
                     {}
                     ",
@@ -342,49 +344,67 @@ impl fmt::Display for NixpkgsProblem {
                     subpath.display(),
                     text,
                 ),
-            NixpkgsProblem::MovedOutOfByName { package_name, call_package_path, empty_arg, file } => {
+            NixpkgsProblem::MovedOutOfByNameEmptyArg { package_name, call_package_path, file } => {
+                let call_package_arg =
+                    if let Some(path) = &call_package_path {
+                        format!("./{}", path.display())
+                    } else {
+                        "...".into()
+                    };
+                writedoc!(
+                    f,
+                    "
+                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
+                      Please move the package back and remove the manual `callPackage`.
+                    ",
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                    )
+            },
+            NixpkgsProblem::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
                     } else {
                         "...".into()
                     };
-                if *empty_arg {
-                    writedoc!(
-                        f,
-                        "
-                        - Attribute `pkgs.{package_name} was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
-                          Please move the package back and remove the manual `callPackage`.
-                        ",
-                        structure::relative_file_for_package(package_name).display(),
-                        file.display(),
-                        )
-                } else {
-                    // This can happen if users mistakenly assume that for custom arguments,
-                    // pkgs/by-name can't be used.
-                    writedoc!(
-                        f,
-                        "
-                        - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
-                          While the manual `callPackage` is still needed, it's not necessary to move the package files.
-                        ",
-                        structure::relative_file_for_package(package_name).display(),
-                        file.display(),
-                        )
-                }
+                // This can happen if users mistakenly assume that for custom arguments,
+                // pkgs/by-name can't be used.
+                writedoc!(
+                    f,
+                    "
+                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
+                      While the manual `callPackage` is still needed, it's not necessary to move the package files.
+                    ",
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                    )
             },
-            NixpkgsProblem::NewPackageNotUsingByName { package_name, call_package_path, empty_arg, file } => {
+            NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
                         format!("./{}", path.display())
                     } else {
                         "...".into()
                     };
-                let extra =
-                    if *empty_arg {
-                        format!("Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.", file.display())
+                writedoc!(
+                    f,
+                    "
+                    - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
+                      Please define it in {} instead.
+                      See `pkgs/by-name/README.md` for more details.
+                      Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.
+                    ",
+                    structure::relative_file_for_package(package_name).display(),
+                    file.display(),
+                )
+            },
+            NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => {
+                let call_package_arg =
+                    if let Some(path) = &call_package_path {
+                        format!("./{}", path.display())
                     } else {
-                        format!("Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.", file.display())
+                        "...".into()
                     };
                 writedoc!(
                     f,
@@ -392,9 +412,10 @@ impl fmt::Display for NixpkgsProblem {
                     - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
                       Please define it in {} instead.
                       See `pkgs/by-name/README.md` for more details.
-                      {extra}
+                      Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.
                     ",
                     structure::relative_file_for_package(package_name).display(),
+                    file.display(),
                 )
             },
             NixpkgsProblem::InternalCallPackageUsed { attr_name } =>
@@ -426,14 +447,13 @@ fn indent_definition(column: usize, definition: String) -> String {
 
 /// 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
-    // But these should never trigger because we only call this function with relative paths, and only
-    // with `from_file` being an actual file (so there's always a parent)
+    // These `expect` calls should never trigger because we only call this function with
+    // relative paths that have a parent. That's why we `expect` them!
     let from = RelativePath::from_path(&from_file)
-        .unwrap()
+        .expect("a relative path")
         .parent()
-        .unwrap();
-    let to = RelativePath::from_path(&to_file).unwrap();
+        .expect("a parent for this path");
+    let to = RelativePath::from_path(&to_file).expect("a path");
     let rel = from.relative(to);
     format!("./{rel}")
 }
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index 28036d3689f15..eed5071a5a2a7 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -154,17 +154,29 @@ impl ToNixpkgsProblem for UsesByName {
         (to, file): &Self::ToContext,
     ) -> NixpkgsProblem {
         if let Some(()) = optional_from {
-            NixpkgsProblem::MovedOutOfByName {
+            if to.empty_arg {
+                NixpkgsProblem::MovedOutOfByNameEmptyArg {
+                    package_name: name.to_owned(),
+                    call_package_path: to.relative_path.clone(),
+                    file: file.to_owned(),
+                }
+            } else {
+                NixpkgsProblem::MovedOutOfByNameNonEmptyArg {
+                    package_name: name.to_owned(),
+                    call_package_path: to.relative_path.clone(),
+                    file: file.to_owned(),
+                }
+            }
+        } else if to.empty_arg {
+            NixpkgsProblem::NewPackageNotUsingByNameEmptyArg {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
-                empty_arg: to.empty_arg,
                 file: file.to_owned(),
             }
         } else {
-            NixpkgsProblem::NewPackageNotUsingByName {
+            NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg {
                 package_name: name.to_owned(),
                 call_package_path: to.relative_path.clone(),
-                empty_arg: to.empty_arg,
                 file: file.to_owned(),
             }
         }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
index 2d05db0d46993..1d92e652200e4 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/alt-callPackage/expected
@@ -2,9 +2,8 @@
 
     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
+  However, in this PR, a different `callPackage` is used. See the definition in all-packages.nix:5:
 
     foo = self.alt.callPackage ({ }: self.someDrv) { };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
index feb6cce2fd04c..e209e18553147 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-fixed/expected
@@ -1 +1 @@
-The base branch was broken, but this PR fixes it, nice job!
+The base branch is broken, but this PR fixes it. Nice job!
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
index 4f0d08357d1fa..c25f06b4150ea 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/base-still-broken/expected
@@ -1,3 +1,3 @@
 pkgs/by-name/bar: This is a file, but it should be a directory.
-The base branch is broken and still has above problems with this PR, these need to be fixed first.
+The base branch is broken and still has above problems with this PR, which need to be fixed first.
 Consider reverting the PR that introduced these problems in order to prevent more failures of unrelated PRs.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
index 6b937106b8060..15b3e3ff6ede9 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/broken-autocall/expected
@@ -1,2 +1,2 @@
 pkgs.foo: This attribute is not defined but it should be defined automatically as pkgs/by-name/fo/foo/package.nix
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
index ec1918cace8c5..3b0146cdc146c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/incorrect-shard/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/FOO: Incorrect directory location, should be pkgs/by-name/fo/FOO instead.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
index 9fef30d14f376..b3d0c6fc1a401 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/internalCallPackage/expected
@@ -1,2 +1,2 @@
 pkgs.foo: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
index aac1b186ca491..80f6e7dd59983 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-package-name/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/fo@: Invalid package directory name "fo@", must be ASCII characters consisting of a-z, A-Z, 0-9, "-" or "_".
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
index ce7c2695424c2..7ca9ff8565bd2 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/invalid-shard-name/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/A: Invalid directory name "A", must be at most 2 ASCII characters consisting of a-z, 0-9, "-" or "_".
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 71d78019604b6..8822e0cc24d47 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -2,8 +2,7 @@
 
     noEval = callPackage ./pkgs/by-name/no/noEval/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:9 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:9:
 
     noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
 
@@ -11,9 +10,8 @@
 
     onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:7 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:7:
 
     onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
index 93c882b2bd3cb..1b67704817cfe 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/missing-package-nix/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 3319cc271ac9f..0e5c07cc04c66 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -1,7 +1,7 @@
-- Attribute `pkgs.foo1 was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo2 was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
 - Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
@@ -10,4 +10,4 @@
 - Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
index 303884cb7944e..0105b19078c75 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/multiple-failures/expected
@@ -11,4 +11,4 @@ pkgs/by-name/ba/foo: File package.nix at line 3 contains the path expression "..
 pkgs/by-name/ba/foo: File package.nix at line 4 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
 pkgs/by-name/ba/foo: File package.nix at line 5 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
 pkgs/by-name/fo/foo: Missing required "package.nix" file.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index a7db19e5fd907..9cfb996f381e9 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -18,4 +18,4 @@
   See `pkgs/by-name/README.md` for more details.
   Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
index 48acbec1210de..1705d22be7984 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-attrs/expected
@@ -1,2 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
index 48acbec1210de..1705d22be7984 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/non-derivation/expected
@@ -1,2 +1,2 @@
 pkgs.nonDerivation: This attribute defined by pkgs/by-name/no/nonDerivation/package.nix is not a derivation
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 e31aa831b8142..3e0ea20c22810 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,4 +2,5 @@ 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/non-syntactical-callPackage-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/non-syntactical-callPackage-by-name/expected
index f56585ac46783..e09e931bb658e 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
@@ -2,9 +2,8 @@
 
     foo = callPackage ./pkgs/by-name/fo/foo/package.nix { /* ... */ };
 
-  This is however not the case.
-  It is defined in all-packages.nix:4 as
+  However, in this PR, it isn't defined that way. See the definition in all-packages.nix:4
 
     foo = self.bar;
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
index 571ec8e09a5c2..16292c0c0eb14 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-different-file/expected
@@ -2,9 +2,8 @@
 
     nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  This is however not the case: The first `callPackage` argument is the wrong path.
-  It is defined in all-packages.nix:2:3 as
+  However, in this PR, the first `callPackage` argument is the wrong path. See the definition in all-packages.nix:2:
 
     nonDerivation = callPackage ./someDrv.nix { /* ... */ };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index 257030e9a6c0e..79f78e2437a02 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -2,9 +2,8 @@
 
     nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:2 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:2:
 
     nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
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 eb13c4df1e888..d91d58d629f2b 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
@@ -2,9 +2,8 @@
 
     nonDerivation = callPackage ./pkgs/by-name/no/nonDerivation/package.nix { /* ... */ };
 
-  This is however not the case.
-  It is defined in all-packages.nix:2 as
+  However, in this PR, it isn't defined that way. See the definition in all-packages.nix:2
 
     nonDerivation = self.someDrv;
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 31679e55b7d61..807c440dd3d26 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
@@ -2,9 +2,8 @@
 
     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
+  However, in this PR, the first `callPackage` argument is not a path. See the definition in all-packages.nix:2:
 
     nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 e7fd8242f5f47..4adbaf66edc0e 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
@@ -2,9 +2,8 @@
 
     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
+  However, in this PR, the first `callPackage` argument is not a path. See the definition in all-packages.nix:3:
 
     foo = self.callPackage ({ someDrv, someFlag }: someDrv) { someFlag = true; };
 
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
index 864978b9b731c..adee1d0137fab 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-dir-is-file/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: This path is a file, but it should be a directory.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
index 82ad2ce85bbfc..d03e1eceea266 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/package-nix-dir/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: "package.nix" must be a file.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
index 0e0e876d5561c..0bdb00f20cb99 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-absolute/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "/foo" which cannot be resolved: No such file or directory (os error 2).
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
index f414e4145674d..2e4338ccc7ae1 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-escape/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "../." which may point outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
index 7b47a786a8be5..30125570794ae 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-nix-path/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the nix search path expression "<nixpkgs>" which may point outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
index 1905841a63df8..6567439b77f8d 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/ref-path-subexpr/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/aa/aa: File package.nix at line 2 contains the path expression "./${"test"}", which is not yet supported and may point outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
index 4a02d99778a9d..689cee41f1e3b 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/shard-file/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo: This is a file, but it should be a directory.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 2b3c2cdeee183..8f574d9dddc3f 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -2,8 +2,7 @@
 
     a = callPackage ./pkgs/by-name/a/a/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:2 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:2:
 
     a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
 
@@ -16,8 +15,7 @@
 
     c = callPackage ./pkgs/by-name/c/c/package.nix { /* ... */ };
 
-  Notably the second argument must not be empty, which is not the case.
-  It is defined in all-packages.nix:4 as
+  However, in this PR, the second argument is empty. See the definition in all-packages.nix:4:
 
     c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
 
@@ -26,4 +24,4 @@
   See `pkgs/by-name/README.md` for more details.
   Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
 
-This PR introduces the above problems compared to the base branch, merging is discouraged, but would not break the base branch
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
index e6c183207e578..cd555c658a09c 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-escape/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: Path package.nix is a symlink pointing to a path outside the directory of that package.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
index 01de2702f7d52..1b06bcf4972be 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/symlink-invalid/expected
@@ -1,2 +1,2 @@
 pkgs/by-name/fo/foo: Path foo is a symlink which cannot be resolved: No such file or directory (os error 2).
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
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 d66a3185e701a..608843d939037 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/unknown-location/expected
@@ -1,2 +1,2 @@
 pkgs.foo: Cannot determine the location of this attribute using `builtins.unsafeGetAttrPos`.
-This PR introduces the above problems, merging would break the base branch
+This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

From f056449c462ff6c5f933f3c297d605ff02147a99 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 01:42:28 +0100
Subject: [PATCH 15/18] tests.nixpkgs-check-by-name: Fix absolute path in
 errors

---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   | 23 ++++++++++++++-----
 .../tests/mock-nixpkgs.nix                    | 23 +++++++++++++------
 .../tests/move-to-non-by-name/expected        |  8 +++----
 .../tests/new-package-non-by-name/expected    |  8 +++----
 .../tests/sorted-order/expected               |  4 ++--
 5 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index a43a0cd743a73..978fe9295336a 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -183,6 +183,7 @@ pub fn check_values(
                     Attribute::NonByName(non_by_name_attribute) => handle_non_by_name_attribute(
                         nixpkgs_path,
                         nix_file_store,
+                        &attribute_name,
                         non_by_name_attribute,
                     )?,
                     Attribute::ByName(by_name_attribute) => by_name(
@@ -431,6 +432,7 @@ fn by_name_override(
 fn handle_non_by_name_attribute(
     nixpkgs_path: &Path,
     nix_file_store: &mut NixFileStore,
+    attribute_name: &str,
     non_by_name_attribute: NonByNameAttribute,
 ) -> validation::Result<ratchet::Package> {
     use ratchet::RatchetState::*;
@@ -481,11 +483,17 @@ fn handle_non_by_name_attribute(
             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>`,
+        // 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_location_file = location.relative_file(nixpkgs_path).with_context(|| {
+            format!("Failed to resolve the file where attribute {attribute_name} is defined")
+        })?;
+
+        // Figure out whether it's an attribute definition of the form `= callPackage <arg1> <arg2>`,
         // returning the arguments if so.
-        let (optional_syntactic_call_package, _definition) = nix_file_store
-            .get(&location.file)?
+        let (optional_syntactic_call_package, _definition) = nix_file
             .call_package_argument_info_at(
                 location.line,
                 location.column,
@@ -493,7 +501,10 @@ fn handle_non_by_name_attribute(
                 // strips the absolute Nixpkgs path from it, such that
                 // syntactic_call_package.relative_path is relative to Nixpkgs
                 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`
@@ -529,7 +540,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, location.file))
+                        Loose((syntactic_call_package, relative_location_file))
                     }
                 }
             }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix b/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix
index 81a9c916ac2d9..fbd51f656138d 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix
+++ b/pkgs/test/nixpkgs-check-by-name/tests/mock-nixpkgs.nix
@@ -28,13 +28,22 @@ let
   lib = import <test-nixpkgs/lib>;
 
   # The base fixed-point function to populate the resulting attribute set
-  pkgsFun = self: {
-    inherit lib;
-    newScope = extra: lib.callPackageWith (self // extra);
-    callPackage = self.newScope { };
-    callPackages = lib.callPackagesWith self;
-    someDrv = { type = "derivation"; };
-  };
+  pkgsFun =
+    self: {
+      inherit lib;
+      newScope = extra: lib.callPackageWith (self // extra);
+      callPackage = self.newScope { };
+      callPackages = lib.callPackagesWith self;
+    }
+    # This mapAttrs is a very hacky workaround necessary because for all attributes defined in Nixpkgs,
+    # the files that define them are verified to be within Nixpkgs.
+    # This is usually a very safe assumption, but it fails in these tests for someDrv,
+    # because it's technically defined outside the Nixpkgs directories of each test case.
+    # By using `mapAttrs`, `builtins.unsafeGetAttrPos` just returns `null`,
+    # which then doesn't trigger this check
+    // lib.mapAttrs (name: value: value) {
+      someDrv = { type = "derivation"; };
+    };
 
   baseDirectory = root + "/pkgs/by-name";
 
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
index 0e5c07cc04c66..123e24daab8a4 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/expected
@@ -1,13 +1,13 @@
-- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo1` was previously defined in pkgs/by-name/fo/foo1/package.nix, but is now manually defined as `callPackage ... { /* ... */ }` in all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo2` was previously defined in pkgs/by-name/fo/foo2/package.nix, but is now manually defined as `callPackage ./without-config.nix { /* ... */ }` in all-packages.nix.
   Please move the package back and remove the manual `callPackage`.
 
-- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo3` was previously defined in pkgs/by-name/fo/foo3/package.nix, but is now manually defined as `callPackage ... { ... }` in all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
-- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/move-to-non-by-name/all-packages.nix.
+- Attribute `pkgs.foo4` was previously defined in pkgs/by-name/fo/foo4/package.nix, but is now manually defined as `callPackage ./with-config.nix { ... }` in all-packages.nix.
   While the manual `callPackage` is still needed, it's not necessary to move the package files.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
index 9cfb996f381e9..92668a231b48e 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected
@@ -1,21 +1,21 @@
 - Attribute `pkgs.new1` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new1/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 - Attribute `pkgs.new2` is a new top-level package using `pkgs.callPackage ./without-config.nix { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new2/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 - Attribute `pkgs.new3` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new3/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in all-packages.nix is still needed.
 
 - Attribute `pkgs.new4` is a new top-level package using `pkgs.callPackage ./with-config.nix { /* ... */ }`.
   Please define it in pkgs/by-name/ne/new4/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/all-packages.nix is still needed.
+  Since the second `callPackage` argument is not `{ }`, the manual `callPackage` in all-packages.nix is still needed.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 8f574d9dddc3f..9c7b0d22a6101 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -9,7 +9,7 @@
 - Attribute `pkgs.b` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/b/b/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 - Because pkgs/by-name/c/c exists, the attribute `pkgs.c` must be defined like
 
@@ -22,6 +22,6 @@
 - Attribute `pkgs.d` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/d/d/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
-  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in /home/tweagysil/src/nixpkgs/by-name-improv/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/all-packages.nix is needed anymore.
+  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in all-packages.nix is needed anymore.
 
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

From 7858f062884e2318dda212ab7cc1ceda871195b2 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 01:50:05 +0100
Subject: [PATCH 16/18] tests.nixpkgs-check-by-name: Better empty argument
 error

---
 pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs        | 2 ++
 .../nixpkgs-check-by-name/tests/manual-definition/expected    | 4 ++++
 .../nixpkgs-check-by-name/tests/override-empty-arg/expected   | 2 ++
 pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected   | 4 ++++
 4 files changed, 12 insertions(+)

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 03203073fd4a1..498284ac6f240 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -213,6 +213,8 @@ impl fmt::Display for NixpkgsProblem {
                       However, in this PR, the second argument is empty. See the definition in {}:{}:
 
                     {}
+
+                      Such a definition is provided automatically and therefore not necessary. Please remove it.
                     ",
                     structure::relative_dir_for_package(package_name).display(),
                     structure::relative_file_for_package(package_name).display(),
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
index 8822e0cc24d47..4d906ec0d086a 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected
@@ -6,6 +6,8 @@
 
     noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 - Because pkgs/by-name/on/onlyMove exists, the attribute `pkgs.onlyMove` must be defined like
 
     onlyMove = callPackage ./pkgs/by-name/on/onlyMove/package.nix { /* ... */ };
@@ -14,4 +16,6 @@
 
     onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
index 79f78e2437a02..f3306aadbbb77 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/expected
@@ -6,4 +6,6 @@
 
     nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
index 9c7b0d22a6101..8a8104b737209 100644
--- a/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
+++ b/pkgs/test/nixpkgs-check-by-name/tests/sorted-order/expected
@@ -6,6 +6,8 @@
 
     a = self.callPackage ./pkgs/by-name/a/a/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 - Attribute `pkgs.b` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/b/b/package.nix instead.
   See `pkgs/by-name/README.md` for more details.
@@ -19,6 +21,8 @@
 
     c = self.callPackage ./pkgs/by-name/c/c/package.nix { };
 
+  Such a definition is provided automatically and therefore not necessary. Please remove it.
+
 - Attribute `pkgs.d` is a new top-level package using `pkgs.callPackage ... { /* ... */ }`.
   Please define it in pkgs/by-name/d/d/package.nix instead.
   See `pkgs/by-name/README.md` for more details.

From 5981aff2125baba1430e5a96338a71e2e1fad18d Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 02:29:24 +0100
Subject: [PATCH 17/18] tests.nixpkgs-check-by-name: Use RelativePath for
 relative paths

Makes the code easier to understand and less error-prone
---
 pkgs/test/nixpkgs-check-by-name/src/eval.rs   |  26 ++-
 .../nixpkgs-check-by-name/src/nix_file.rs     |  17 +-
 .../src/nixpkgs_problem.rs                    | 196 +++++++++---------
 .../test/nixpkgs-check-by-name/src/ratchet.rs |   4 +-
 .../nixpkgs-check-by-name/src/references.rs   |  52 ++---
 .../nixpkgs-check-by-name/src/structure.rs    |  16 +-
 6 files changed, 158 insertions(+), 153 deletions(-)

diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index 978fe9295336a..094508f595d8f 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -8,6 +8,7 @@ use crate::utils;
 use crate::validation::ResultIteratorExt as _;
 use crate::validation::{self, Validation::Success};
 use crate::NixFileStore;
+use relative_path::RelativePathBuf;
 use std::path::Path;
 
 use anyhow::Context;
@@ -56,18 +57,15 @@ struct Location {
 
 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())
+    fn relative_file(&self, nixpkgs_path: &Path) -> anyhow::Result<RelativePathBuf> {
+        let path = self.file.strip_prefix(nixpkgs_path).with_context(|| {
+            format!(
+                "The file ({}) is outside Nixpkgs ({})",
+                self.file.display(),
+                nixpkgs_path.display()
+            )
+        })?;
+        Ok(RelativePathBuf::from_path(path).expect("relative path"))
     }
 }
 
@@ -352,12 +350,12 @@ fn by_name(
 /// all-packages.nix
 fn by_name_override(
     attribute_name: &str,
-    expected_package_file: PathBuf,
+    expected_package_file: RelativePathBuf,
     is_semantic_call_package: bool,
     optional_syntactic_call_package: Option<CallPackageArgumentInfo>,
     definition: String,
     location: Location,
-    relative_location_file: PathBuf,
+    relative_location_file: RelativePathBuf,
 ) -> validation::Validation<ratchet::RatchetState<ratchet::ManualDefinition>> {
     // At this point, we completed two different checks for whether it's a
     // `callPackage`
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 1cb5d1c0d08d3..e2dc1e1961414 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -3,6 +3,7 @@
 use crate::utils::LineIndex;
 use anyhow::Context;
 use itertools::Either::{self, Left, Right};
+use relative_path::RelativePathBuf;
 use rnix::ast;
 use rnix::ast::Expr;
 use rnix::ast::HasEntry;
@@ -79,7 +80,7 @@ impl NixFile {
 #[derive(Debug, PartialEq)]
 pub struct CallPackageArgumentInfo {
     /// The relative path of the first argument, or `None` if it's not a path.
-    pub relative_path: Option<PathBuf>,
+    pub relative_path: Option<RelativePathBuf>,
     /// Whether the second argument is an empty attribute set
     pub empty_arg: bool,
 }
@@ -369,8 +370,8 @@ pub enum ResolvedPath {
     /// The path is outside the given absolute path
     Outside,
     /// The path is within the given absolute path.
-    /// The `PathBuf` is the relative path under the given absolute path.
-    Within(PathBuf),
+    /// The `RelativePathBuf` is the relative path under the given absolute path.
+    Within(RelativePathBuf),
 }
 
 impl NixFile {
@@ -402,7 +403,9 @@ impl NixFile {
                 // Check if it's within relative_to
                 match resolved.strip_prefix(relative_to) {
                     Err(_prefix_error) => ResolvedPath::Outside,
-                    Ok(suffix) => ResolvedPath::Within(suffix.to_path_buf()),
+                    Ok(suffix) => ResolvedPath::Within(
+                        RelativePathBuf::from_path(suffix).expect("a relative path"),
+                    ),
                 }
             }
         }
@@ -506,14 +509,14 @@ mod tests {
             (
                 6,
                 Some(CallPackageArgumentInfo {
-                    relative_path: Some(PathBuf::from("file.nix")),
+                    relative_path: Some(RelativePathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
             (
                 7,
                 Some(CallPackageArgumentInfo {
-                    relative_path: Some(PathBuf::from("file.nix")),
+                    relative_path: Some(RelativePathBuf::from("file.nix")),
                     empty_arg: true,
                 }),
             ),
@@ -527,7 +530,7 @@ mod tests {
             (
                 9,
                 Some(CallPackageArgumentInfo {
-                    relative_path: Some(PathBuf::from("file.nix")),
+                    relative_path: Some(RelativePathBuf::from("file.nix")),
                     empty_arg: false,
                 }),
             ),
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 498284ac6f240..f9e232b939335 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -2,139 +2,140 @@ use crate::structure;
 use crate::utils::PACKAGE_NIX_FILENAME;
 use indoc::writedoc;
 use relative_path::RelativePath;
+use relative_path::RelativePathBuf;
 use std::ffi::OsString;
 use std::fmt;
-use std::path::Path;
-use std::path::PathBuf;
 
 /// Any problem that can occur when checking Nixpkgs
+/// All paths are relative to Nixpkgs such that the error messages can't be influenced by Nixpkgs absolute
+/// location
 #[derive(Clone)]
 pub enum NixpkgsProblem {
     ShardNonDir {
-        relative_shard_path: PathBuf,
+        relative_shard_path: RelativePathBuf,
     },
     InvalidShardName {
-        relative_shard_path: PathBuf,
+        relative_shard_path: RelativePathBuf,
         shard_name: String,
     },
     PackageNonDir {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
     },
     CaseSensitiveDuplicate {
-        relative_shard_path: PathBuf,
+        relative_shard_path: RelativePathBuf,
         first: OsString,
         second: OsString,
     },
     InvalidPackageName {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
         package_name: String,
     },
     IncorrectShard {
-        relative_package_dir: PathBuf,
-        correct_relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        correct_relative_package_dir: RelativePathBuf,
     },
     PackageNixNonExistent {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
     },
     PackageNixDir {
-        relative_package_dir: PathBuf,
+        relative_package_dir: RelativePathBuf,
     },
     UndefinedAttr {
-        relative_package_file: PathBuf,
+        relative_package_file: RelativePathBuf,
         package_name: String,
     },
     EmptyArgument {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     NonToplevelCallPackage {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     NonPath {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     WrongCallPackagePath {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
-        actual_path: PathBuf,
-        expected_path: PathBuf,
+        actual_path: RelativePathBuf,
+        expected_path: RelativePathBuf,
     },
     NonSyntacticCallPackage {
         package_name: String,
-        file: PathBuf,
+        file: RelativePathBuf,
         line: usize,
         column: usize,
         definition: String,
     },
     NonDerivation {
-        relative_package_file: PathBuf,
+        relative_package_file: RelativePathBuf,
         package_name: String,
     },
     OutsideSymlink {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
     },
     UnresolvableSymlink {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         io_error: String,
     },
     PathInterpolation {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
     },
     SearchPath {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
     },
     OutsidePathReference {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
     },
     UnresolvablePathReference {
-        relative_package_dir: PathBuf,
-        subpath: PathBuf,
+        relative_package_dir: RelativePathBuf,
+        subpath: RelativePathBuf,
         line: usize,
         text: String,
         io_error: String,
     },
     MovedOutOfByNameEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     MovedOutOfByNameNonEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     NewPackageNotUsingByNameEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     NewPackageNotUsingByNameNonEmptyArg {
         package_name: String,
-        call_package_path: Option<PathBuf>,
-        file: PathBuf,
+        call_package_path: Option<RelativePathBuf>,
+        file: RelativePathBuf,
     },
     InternalCallPackageUsed {
         attr_name: String,
@@ -151,56 +152,56 @@ impl fmt::Display for NixpkgsProblem {
                 write!(
                     f,
                     "{}: This is a file, but it should be a directory.",
-                    relative_shard_path.display(),
+                    relative_shard_path,
                 ),
             NixpkgsProblem::InvalidShardName { relative_shard_path, shard_name } =>
                 write!(
                     f,
                     "{}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
-                    relative_shard_path.display()
+                    relative_shard_path,
                 ),
             NixpkgsProblem::PackageNonDir { relative_package_dir } =>
                 write!(
                     f,
                     "{}: This path is a file, but it should be a directory.",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } =>
                 write!(
                     f,
                     "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
-                    relative_shard_path.display(),
+                    relative_shard_path,
                 ),
             NixpkgsProblem::InvalidPackageName { relative_package_dir, package_name } =>
                 write!(
                     f,
                     "{}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::IncorrectShard { relative_package_dir, correct_relative_package_dir } =>
                 write!(
                     f,
                     "{}: Incorrect directory location, should be {} instead.",
-                    relative_package_dir.display(),
-                    correct_relative_package_dir.display(),
+                    relative_package_dir,
+                    correct_relative_package_dir,
                 ),
             NixpkgsProblem::PackageNixNonExistent { relative_package_dir } =>
                 write!(
                     f,
                     "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::PackageNixDir { relative_package_dir } =>
                 write!(
                     f,
                     "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
-                    relative_package_dir.display(),
+                    relative_package_dir,
                 ),
             NixpkgsProblem::UndefinedAttr { relative_package_file, package_name } =>
                 write!(
                     f,
                     "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}",
-                    relative_package_file.display()
+                    relative_package_file,
                 ),
             NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } =>
                 writedoc!(
@@ -216,9 +217,9 @@ impl fmt::Display for NixpkgsProblem {
 
                       Such a definition is provided automatically and therefore not necessary. Please remove it.
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
@@ -234,9 +235,9 @@ impl fmt::Display for NixpkgsProblem {
 
                     {}
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
@@ -252,9 +253,9 @@ impl fmt::Display for NixpkgsProblem {
 
                     {}
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 ),
@@ -270,9 +271,9 @@ impl fmt::Display for NixpkgsProblem {
 
                         {package_name} = callPackage {} {{ /* ... */ }};
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
+                    structure::relative_dir_for_package(package_name),
                     create_path_expr(file, expected_path),
-                    file.display(), line,
+                    file, line,
                     create_path_expr(file, actual_path),
                 },
             NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => {
@@ -287,9 +288,9 @@ impl fmt::Display for NixpkgsProblem {
 
                     {}
                     ",
-                    structure::relative_dir_for_package(package_name).display(),
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_dir_for_package(package_name),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     line,
                     indent_definition(*column, definition.clone()),
                 )
@@ -298,58 +299,58 @@ impl fmt::Display for NixpkgsProblem {
                 write!(
                     f,
                     "pkgs.{package_name}: This attribute defined by {} is not a derivation",
-                    relative_package_file.display()
+                    relative_package_file,
                 ),
             NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } =>
                 write!(
                     f,
                     "{}: Path {} is a symlink pointing to a path outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                 ),
             NixpkgsProblem::UnresolvableSymlink { relative_package_dir, subpath, io_error } =>
                 write!(
                     f,
                     "{}: Path {} is a symlink which cannot be resolved: {io_error}.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                 ),
             NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the path expression \"{}\", which is not yet supported and may point outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text
                 ),
             NixpkgsProblem::SearchPath { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the nix search path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text
                 ),
             NixpkgsProblem::OutsidePathReference { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text,
                 ),
             NixpkgsProblem::UnresolvablePathReference { relative_package_dir, subpath, line, text, io_error } =>
                 write!(
                     f,
                     "{}: File {} at line {line} contains the path expression \"{}\" which cannot be resolved: {io_error}.",
-                    relative_package_dir.display(),
-                    subpath.display(),
+                    relative_package_dir,
+                    subpath,
                     text,
                 ),
             NixpkgsProblem::MovedOutOfByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -359,14 +360,14 @@ impl fmt::Display for NixpkgsProblem {
                     - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
                       Please move the package back and remove the manual `callPackage`.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     )
             },
             NixpkgsProblem::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -378,14 +379,14 @@ impl fmt::Display for NixpkgsProblem {
                     - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
                       While the manual `callPackage` is still needed, it's not necessary to move the package files.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                     )
             },
             NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -397,14 +398,14 @@ impl fmt::Display for NixpkgsProblem {
                       See `pkgs/by-name/README.md` for more details.
                       Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                 )
             },
             NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path.display())
+                        format!("./{}", path)
                     } else {
                         "...".into()
                     };
@@ -416,8 +417,8 @@ impl fmt::Display for NixpkgsProblem {
                       See `pkgs/by-name/README.md` for more details.
                       Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.
                     ",
-                    structure::relative_file_for_package(package_name).display(),
-                    file.display(),
+                    structure::relative_file_for_package(package_name),
+                    file,
                 )
             },
             NixpkgsProblem::InternalCallPackageUsed { attr_name } =>
@@ -448,14 +449,13 @@ fn indent_definition(column: usize, definition: String) -> String {
 }
 
 /// 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 {
-    // These `expect` calls should never trigger because we only call this function with
-    // relative paths that have a parent. That's why we `expect` them!
-    let from = RelativePath::from_path(&from_file)
-        .expect("a relative path")
-        .parent()
-        .expect("a parent for this path");
-    let to = RelativePath::from_path(&to_file).expect("a path");
-    let rel = from.relative(to);
+fn create_path_expr(
+    from_file: impl AsRef<RelativePath>,
+    to_file: impl AsRef<RelativePath>,
+) -> String {
+    // This `expect` calls should never trigger because we only call this function with files.
+    // That's why we `expect` them!
+    let from = from_file.as_ref().parent().expect("a parent for this path");
+    let rel = from.relative(to_file);
     format!("./{rel}")
 }
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index eed5071a5a2a7..8136d641c3515 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -5,8 +5,8 @@
 use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::validation::{self, Validation, Validation::Success};
+use relative_path::RelativePathBuf;
 use std::collections::HashMap;
-use std::path::PathBuf;
 
 /// The ratchet value for the entirety of Nixpkgs.
 #[derive(Default)]
@@ -146,7 +146,7 @@ impl ToNixpkgsProblem for ManualDefinition {
 pub enum UsesByName {}
 
 impl ToNixpkgsProblem for UsesByName {
-    type ToContext = (CallPackageArgumentInfo, PathBuf);
+    type ToContext = (CallPackageArgumentInfo, RelativePathBuf);
 
     fn to_nixpkgs_problem(
         name: &str,
diff --git a/pkgs/test/nixpkgs-check-by-name/src/references.rs b/pkgs/test/nixpkgs-check-by-name/src/references.rs
index b716b99099c49..e2319163ccc67 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/references.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/references.rs
@@ -2,6 +2,7 @@ use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::utils;
 use crate::validation::{self, ResultIteratorExt, Validation::Success};
 use crate::NixFileStore;
+use relative_path::RelativePath;
 
 use anyhow::Context;
 use rowan::ast::AstNode;
@@ -12,14 +13,14 @@ use std::path::Path;
 /// Both symlinks and Nix path expressions are checked.
 pub fn check_references(
     nix_file_store: &mut NixFileStore,
-    relative_package_dir: &Path,
+    relative_package_dir: &RelativePath,
     absolute_package_dir: &Path,
 ) -> validation::Result<()> {
     // The first subpath to check is the package directory itself, which we can represent as an
     // empty path, since the absolute package directory gets prepended to this.
     // We don't use `./.` to keep the error messages cleaner
     // (there's no canonicalisation going on underneath)
-    let subpath = Path::new("");
+    let subpath = RelativePath::new("");
     check_path(
         nix_file_store,
         relative_package_dir,
@@ -29,7 +30,7 @@ pub fn check_references(
     .with_context(|| {
         format!(
             "While checking the references in package directory {}",
-            relative_package_dir.display()
+            relative_package_dir
         )
     })
 }
@@ -41,11 +42,11 @@ pub fn check_references(
 /// The absolute package directory gets prepended before doing anything with it though.
 fn check_path(
     nix_file_store: &mut NixFileStore,
-    relative_package_dir: &Path,
+    relative_package_dir: &RelativePath,
     absolute_package_dir: &Path,
-    subpath: &Path,
+    subpath: &RelativePath,
 ) -> validation::Result<()> {
-    let path = absolute_package_dir.join(subpath);
+    let path = subpath.to_path(absolute_package_dir);
 
     Ok(if path.is_symlink() {
         // Check whether the symlink resolves to outside the package directory
@@ -55,8 +56,8 @@ fn check_path(
                 // entire directory recursively anyways
                 if let Err(_prefix_error) = target.strip_prefix(absolute_package_dir) {
                     NixpkgsProblem::OutsideSymlink {
-                        relative_package_dir: relative_package_dir.to_path_buf(),
-                        subpath: subpath.to_path_buf(),
+                        relative_package_dir: relative_package_dir.to_owned(),
+                        subpath: subpath.to_owned(),
                     }
                     .into()
                 } else {
@@ -64,8 +65,8 @@ fn check_path(
                 }
             }
             Err(io_error) => NixpkgsProblem::UnresolvableSymlink {
-                relative_package_dir: relative_package_dir.to_path_buf(),
-                subpath: subpath.to_path_buf(),
+                relative_package_dir: relative_package_dir.to_owned(),
+                subpath: subpath.to_owned(),
                 io_error: io_error.to_string(),
             }
             .into(),
@@ -80,11 +81,12 @@ fn check_path(
                         nix_file_store,
                         relative_package_dir,
                         absolute_package_dir,
-                        &subpath.join(entry.file_name()),
+                        // TODO: The relative_path crate doesn't seem to support OsStr
+                        &subpath.join(entry.file_name().to_string_lossy().to_string()),
                     )
                 })
                 .collect_vec()
-                .with_context(|| format!("Error while recursing into {}", subpath.display()))?,
+                .with_context(|| format!("Error while recursing into {}", subpath))?,
         )
     } else if path.is_file() {
         // Only check Nix files
@@ -96,7 +98,7 @@ fn check_path(
                     absolute_package_dir,
                     subpath,
                 )
-                .with_context(|| format!("Error while checking Nix file {}", subpath.display()))?
+                .with_context(|| format!("Error while checking Nix file {}", subpath))?
             } else {
                 Success(())
             }
@@ -105,7 +107,7 @@ fn check_path(
         }
     } else {
         // This should never happen, git doesn't support other file types
-        anyhow::bail!("Unsupported file type for path {}", subpath.display());
+        anyhow::bail!("Unsupported file type for path {}", subpath);
     })
 }
 
@@ -113,11 +115,11 @@ fn check_path(
 /// directory
 fn check_nix_file(
     nix_file_store: &mut NixFileStore,
-    relative_package_dir: &Path,
+    relative_package_dir: &RelativePath,
     absolute_package_dir: &Path,
-    subpath: &Path,
+    subpath: &RelativePath,
 ) -> validation::Result<()> {
-    let path = absolute_package_dir.join(subpath);
+    let path = subpath.to_path(absolute_package_dir);
 
     let nix_file = nix_file_store.get(&path)?;
 
@@ -135,29 +137,29 @@ fn check_nix_file(
 
             match nix_file.static_resolve_path(path, absolute_package_dir) {
                 ResolvedPath::Interpolated => NixpkgsProblem::PathInterpolation {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                 }
                 .into(),
                 ResolvedPath::SearchPath => NixpkgsProblem::SearchPath {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                 }
                 .into(),
                 ResolvedPath::Outside => NixpkgsProblem::OutsidePathReference {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                 }
                 .into(),
                 ResolvedPath::Unresolvable(e) => NixpkgsProblem::UnresolvablePathReference {
-                    relative_package_dir: relative_package_dir.to_path_buf(),
-                    subpath: subpath.to_path_buf(),
+                    relative_package_dir: relative_package_dir.to_owned(),
+                    subpath: subpath.to_owned(),
                     line,
                     text,
                     io_error: e.to_string(),
diff --git a/pkgs/test/nixpkgs-check-by-name/src/structure.rs b/pkgs/test/nixpkgs-check-by-name/src/structure.rs
index 9b615dd9969ac..09806bc3d4dc3 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/structure.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/structure.rs
@@ -7,8 +7,9 @@ use crate::NixFileStore;
 use itertools::concat;
 use lazy_static::lazy_static;
 use regex::Regex;
+use relative_path::RelativePathBuf;
 use std::fs::DirEntry;
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
 lazy_static! {
     static ref SHARD_NAME_REGEX: Regex = Regex::new(r"^[a-z0-9_-]{1,2}$").unwrap();
@@ -21,15 +22,15 @@ pub fn shard_for_package(package_name: &str) -> String {
     package_name.to_lowercase().chars().take(2).collect()
 }
 
-pub fn relative_dir_for_shard(shard_name: &str) -> PathBuf {
-    PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}"))
+pub fn relative_dir_for_shard(shard_name: &str) -> RelativePathBuf {
+    RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}"))
 }
 
-pub fn relative_dir_for_package(package_name: &str) -> PathBuf {
+pub fn relative_dir_for_package(package_name: &str) -> RelativePathBuf {
     relative_dir_for_shard(&shard_for_package(package_name)).join(package_name)
 }
 
-pub fn relative_file_for_package(package_name: &str) -> PathBuf {
+pub fn relative_file_for_package(package_name: &str) -> RelativePathBuf {
     relative_dir_for_package(package_name).join(PACKAGE_NIX_FILENAME)
 }
 
@@ -120,7 +121,8 @@ fn check_package(
 ) -> validation::Result<String> {
     let package_path = package_entry.path();
     let package_name = package_entry.file_name().to_string_lossy().into_owned();
-    let relative_package_dir = PathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}"));
+    let relative_package_dir =
+        RelativePathBuf::from(format!("{BASE_SUBPATH}/{shard_name}/{package_name}"));
 
     Ok(if !package_path.is_dir() {
         NixpkgsProblem::PackageNonDir {
@@ -174,7 +176,7 @@ fn check_package(
         let result = result.and(references::check_references(
             nix_file_store,
             &relative_package_dir,
-            &path.join(&relative_package_dir),
+            &relative_package_dir.to_path(path),
         )?);
 
         result.map(|_| package_name.clone())

From fb0a07229f81417b90cbb05d6522dbc4c654fb76 Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Fri, 1 Mar 2024 02:41:02 +0100
Subject: [PATCH 18/18] tests.nixpkgs-check-by-name: More inline format!
 arguments

Now that the previous commit removed all the .display()'s that were
previously necessary for PathBuf's, but now aren't for RelativePathBuf,
we can also inline the format! arguments
---
 .../src/nixpkgs_problem.rs                    | 188 +++++++-----------
 1 file changed, 76 insertions(+), 112 deletions(-)

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 f9e232b939335..7e257c0ed5d88 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs
@@ -151,218 +151,185 @@ impl fmt::Display for NixpkgsProblem {
             NixpkgsProblem::ShardNonDir { relative_shard_path } =>
                 write!(
                     f,
-                    "{}: This is a file, but it should be a directory.",
-                    relative_shard_path,
+                    "{relative_shard_path}: This is a file, but it should be a directory.",
                 ),
             NixpkgsProblem::InvalidShardName { relative_shard_path, shard_name } =>
                 write!(
                     f,
-                    "{}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
-                    relative_shard_path,
+                    "{relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
                 ),
             NixpkgsProblem::PackageNonDir { relative_package_dir } =>
                 write!(
                     f,
-                    "{}: This path is a file, but it should be a directory.",
-                    relative_package_dir,
+                    "{relative_package_dir}: This path is a file, but it should be a directory.",
                 ),
             NixpkgsProblem::CaseSensitiveDuplicate { relative_shard_path, first, second } =>
                 write!(
                     f,
-                    "{}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
-                    relative_shard_path,
+                    "{relative_shard_path}: Duplicate case-sensitive package directories {first:?} and {second:?}.",
                 ),
             NixpkgsProblem::InvalidPackageName { relative_package_dir, package_name } =>
                 write!(
                     f,
-                    "{}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
-                    relative_package_dir,
+                    "{relative_package_dir}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",
                 ),
             NixpkgsProblem::IncorrectShard { relative_package_dir, correct_relative_package_dir } =>
                 write!(
                     f,
-                    "{}: Incorrect directory location, should be {} instead.",
-                    relative_package_dir,
-                    correct_relative_package_dir,
+                    "{relative_package_dir}: Incorrect directory location, should be {correct_relative_package_dir} instead.",
                 ),
             NixpkgsProblem::PackageNixNonExistent { relative_package_dir } =>
                 write!(
                     f,
-                    "{}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
-                    relative_package_dir,
+                    "{relative_package_dir}: Missing required \"{PACKAGE_NIX_FILENAME}\" file.",
                 ),
             NixpkgsProblem::PackageNixDir { relative_package_dir } =>
                 write!(
                     f,
-                    "{}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
-                    relative_package_dir,
+                    "{relative_package_dir}: \"{PACKAGE_NIX_FILENAME}\" must be a file.",
                 ),
             NixpkgsProblem::UndefinedAttr { relative_package_file, package_name } =>
                 write!(
                     f,
-                    "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {}",
-                    relative_package_file,
+                    "pkgs.{package_name}: This attribute is not defined but it should be defined automatically as {relative_package_file}",
                 ),
-            NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } =>
+            NixpkgsProblem::EmptyArgument { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, the second argument is empty. See the definition in {}:{}:
+                      However, in this PR, the second argument is empty. See the definition in {file}:{line}:
 
-                    {}
+                    {indented_definition}
 
                       Such a definition is provided automatically and therefore not necessary. Please remove it.
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
-                ),
-            NixpkgsProblem::NonToplevelCallPackage { package_name, file, line, column, definition } =>
+                )
+            }
+            NixpkgsProblem::NonToplevelCallPackage { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, a different `callPackage` is used. See the definition in {}:{}:
+                      However, in this PR, a different `callPackage` is used. See the definition in {file}:{line}:
 
-                    {}
+                    {indented_definition}
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
-                ),
-            NixpkgsProblem::NonPath { package_name, file, line, column, definition } =>
+                )
+            }
+            NixpkgsProblem::NonPath { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, the first `callPackage` argument is not a path. See the definition in {}:{}:
+                      However, in this PR, the first `callPackage` argument is not a path. See the definition in {file}:{line}:
 
-                    {}
+                    {indented_definition}
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
-                ),
-            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, actual_path, expected_path } =>
+                )
+            }
+            NixpkgsProblem::WrongCallPackagePath { package_name, file, line, actual_path, expected_path } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let expected_path_expr = create_path_expr(file, expected_path);
+                let actual_path_expr = create_path_expr(file, actual_path);
                 writedoc! {
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage {expected_path_expr} {{ /* ... */ }};
 
-                      However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {}:{}:
+                      However, in this PR, the first `callPackage` argument is the wrong path. See the definition in {file}:{line}:
 
-                        {package_name} = callPackage {} {{ /* ... */ }};
+                        {package_name} = callPackage {actual_path_expr} {{ /* ... */ }};
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    create_path_expr(file, expected_path),
-                    file, line,
-                    create_path_expr(file, actual_path),
-                },
+                }
+            }
             NixpkgsProblem::NonSyntacticCallPackage { package_name, file, line, column, definition } => {
+                let relative_package_dir = structure::relative_dir_for_package(package_name);
+                let relative_package_file = structure::relative_file_for_package(package_name);
+                let indented_definition = indent_definition(*column, definition.clone());
                 writedoc!(
                     f,
                     "
-                    - Because {} exists, the attribute `pkgs.{package_name}` must be defined like
+                    - Because {relative_package_dir} exists, the attribute `pkgs.{package_name}` must be defined like
 
-                        {package_name} = callPackage ./{} {{ /* ... */ }};
+                        {package_name} = callPackage ./{relative_package_file} {{ /* ... */ }};
 
-                      However, in this PR, it isn't defined that way. See the definition in {}:{}
+                      However, in this PR, it isn't defined that way. See the definition in {file}:{line}
 
-                    {}
+                    {indented_definition}
                     ",
-                    structure::relative_dir_for_package(package_name),
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    line,
-                    indent_definition(*column, definition.clone()),
                 )
             }
             NixpkgsProblem::NonDerivation { relative_package_file, package_name } =>
                 write!(
                     f,
-                    "pkgs.{package_name}: This attribute defined by {} is not a derivation",
-                    relative_package_file,
+                    "pkgs.{package_name}: This attribute defined by {relative_package_file} is not a derivation",
                 ),
             NixpkgsProblem::OutsideSymlink { relative_package_dir, subpath } =>
                 write!(
                     f,
-                    "{}: Path {} is a symlink pointing to a path outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
+                    "{relative_package_dir}: Path {subpath} is a symlink pointing to a path outside the directory of that package.",
                 ),
             NixpkgsProblem::UnresolvableSymlink { relative_package_dir, subpath, io_error } =>
                 write!(
                     f,
-                    "{}: Path {} is a symlink which cannot be resolved: {io_error}.",
-                    relative_package_dir,
-                    subpath,
+                    "{relative_package_dir}: Path {subpath} is a symlink which cannot be resolved: {io_error}.",
                 ),
             NixpkgsProblem::PathInterpolation { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the path expression \"{}\", which is not yet supported and may point outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
-                    text
+                    "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\", which is not yet supported and may point outside the directory of that package.",
                 ),
             NixpkgsProblem::SearchPath { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the nix search path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
-                    text
+                    "{relative_package_dir}: File {subpath} at line {line} contains the nix search path expression \"{text}\" which may point outside the directory of that package.",
                 ),
             NixpkgsProblem::OutsidePathReference { relative_package_dir, subpath, line, text } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the path expression \"{}\" which may point outside the directory of that package.",
-                    relative_package_dir,
-                    subpath,
-                    text,
+                    "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which may point outside the directory of that package.",
                 ),
             NixpkgsProblem::UnresolvablePathReference { relative_package_dir, subpath, line, text, io_error } =>
                 write!(
                     f,
-                    "{}: File {} at line {line} contains the path expression \"{}\" which cannot be resolved: {io_error}.",
-                    relative_package_dir,
-                    subpath,
-                    text,
+                    "{relative_package_dir}: File {subpath} at line {line} contains the path expression \"{text}\" which cannot be resolved: {io_error}.",
                 ),
             NixpkgsProblem::MovedOutOfByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
                     if let Some(path) = &call_package_path {
-                        format!("./{}", path)
+                        format!("./{path}")
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 writedoc!(
                     f,
                     "
-                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {}.
+                    - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ /* ... */ }}` in {file}.
                       Please move the package back and remove the manual `callPackage`.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    )
+                )
             },
             NixpkgsProblem::MovedOutOfByNameNonEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
@@ -371,17 +338,16 @@ impl fmt::Display for NixpkgsProblem {
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 // This can happen if users mistakenly assume that for custom arguments,
                 // pkgs/by-name can't be used.
                 writedoc!(
                     f,
                     "
-                    - Attribute `pkgs.{package_name}` was previously defined in {}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {}.
+                    - Attribute `pkgs.{package_name}` was previously defined in {relative_package_file}, but is now manually defined as `callPackage {call_package_arg} {{ ... }}` in {file}.
                       While the manual `callPackage` is still needed, it's not necessary to move the package files.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
-                    )
+                )
             },
             NixpkgsProblem::NewPackageNotUsingByNameEmptyArg { package_name, call_package_path, file } => {
                 let call_package_arg =
@@ -390,16 +356,15 @@ impl fmt::Display for NixpkgsProblem {
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 writedoc!(
                     f,
                     "
                     - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
-                      Please define it in {} instead.
+                      Please define it in {relative_package_file} instead.
                       See `pkgs/by-name/README.md` for more details.
-                      Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {} is needed anymore.
+                      Since the second `callPackage` argument is `{{ }}`, no manual `callPackage` in {file} is needed anymore.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
                 )
             },
             NixpkgsProblem::NewPackageNotUsingByNameNonEmptyArg { package_name, call_package_path, file } => {
@@ -409,16 +374,15 @@ impl fmt::Display for NixpkgsProblem {
                     } else {
                         "...".into()
                     };
+                let relative_package_file = structure::relative_file_for_package(package_name);
                 writedoc!(
                     f,
                     "
                     - Attribute `pkgs.{package_name}` is a new top-level package using `pkgs.callPackage {call_package_arg} {{ /* ... */ }}`.
-                      Please define it in {} instead.
+                      Please define it in {relative_package_file} instead.
                       See `pkgs/by-name/README.md` for more details.
-                      Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {} is still needed.
+                      Since the second `callPackage` argument is not `{{ }}`, the manual `callPackage` in {file} is still needed.
                     ",
-                    structure::relative_file_for_package(package_name),
-                    file,
                 )
             },
             NixpkgsProblem::InternalCallPackageUsed { attr_name } =>