tests.nixpkgs-check-by-name: Syntactic callPackage detection

This makes the callPackage detection stronger by syntactically detecting
whether an attribute is using callPackage

See the added test case for why this is needed.

10 files changed, 516 insertions, 23 deletions

diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.lock b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
index fc3aeb9fd79b..904a9cff0e78 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.lock
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.lock
@@ -214,6 +214,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "443144c8cdadd93ebf52ddb4056d257f5b52c04d3c804e657d19eb73fc33668b"
 
 [[package]]
+name = "indoc"
+version = "2.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1e186cfbae8084e513daff4240b4797e342f988cecda4fb6c939150f96315fd8"
+
+[[package]]
 name = "is-terminal"
 version = "0.4.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -289,10 +295,12 @@ dependencies = [
  "anyhow",
  "clap",
  "colored",
+ "indoc",
  "itertools",
  "lazy_static",
  "regex",
  "rnix",
+ "rowan",
  "serde",
  "serde_json",
  "temp-env",
diff --git a/pkgs/test/nixpkgs-check-by-name/Cargo.toml b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
index 1e6eaa1106d5..be15ac5f2453 100644
--- a/pkgs/test/nixpkgs-check-by-name/Cargo.toml
+++ b/pkgs/test/nixpkgs-check-by-name/Cargo.toml
@@ -14,6 +14,8 @@ anyhow = "1.0"
 lazy_static = "1.4.0"
 colored = "2.0.4"
 itertools = "0.11.0"
+rowan = "0.15.11"
+indoc = "2.0.4"
 
 [dev-dependencies]
 temp-env = "0.3.5"
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.nix b/pkgs/test/nixpkgs-check-by-name/src/eval.nix
index 87c54b6444ee..7179951d41cf 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.nix
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.nix
@@ -76,6 +76,7 @@ let
         CallPackage = {
           call_package_variant = value._callPackageVariant;
           is_derivation = pkgs.lib.isDerivation value;
+          location = builtins.unsafeGetAttrPos name pkgs;
         };
       };
 
diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
index dd30cb9045e5..85c1f2812a68 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs
@@ -1,7 +1,9 @@
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::ratchet;
 use crate::structure;
+use crate::validation::ResultIteratorExt;
 use crate::validation::{self, Validation::Success};
+use crate::NixFileCache;
 use std::path::Path;
 
 use anyhow::Context;
@@ -48,6 +50,15 @@ struct CallPackageInfo {
     call_package_variant: CallPackageVariant,
     /// Whether the attribute is a derivation (`lib.isDerivation`)
     is_derivation: bool,
+    location: Option<Location>,
+}
+
+/// The structure returned by `builtins.unsafeGetAttrPos`
+#[derive(Deserialize, Clone, Debug)]
+struct Location {
+    pub file: PathBuf,
+    pub line: usize,
+    pub column: usize,
 }
 
 #[derive(Deserialize)]
@@ -70,6 +81,7 @@ enum CallPackageVariant {
 /// See the `eval.nix` file for how this is achieved on the Nix side
 pub fn check_values(
     nixpkgs_path: &Path,
+    nix_file_cache: &mut NixFileCache,
     package_names: Vec<String>,
     keep_nix_path: bool,
 ) -> validation::Result<ratchet::Nixpkgs> {
@@ -184,17 +196,36 @@ pub fn check_values(
                             is_derivation: false,
                             ..
                         }) => Success(NonApplicable),
-
+                        // A location of None indicates something weird, we can't really know where
+                        // this attribute is defined, probably an alias
+                        CallPackage(CallPackageInfo { location: None, .. }) => Success(Tight),
                         // The case of an attribute that qualifies:
                         // - Uses callPackage
                         // - Is a derivation
                         CallPackage(CallPackageInfo {
                             is_derivation: true,
-                            call_package_variant: Manual { path, empty_arg },
-                        }) => Success(Loose(ratchet::CouldUseByName {
-                            call_package_path: path,
-                            empty_arg,
-                        })),
+                            call_package_variant: Manual { .. },
+                            location: Some(location),
+                        }) =>
+                        // We'll use the attribute's location to parse the file that defines it
+                        match nix_file_cache.get(&location.file)? {
+                            Err(error) =>
+                                // This is a bad sign for rnix, because it means cpp Nix could parse
+                                // something that rnix couldn't
+                                anyhow::bail!(
+                                    "Could not parse file {} with rnix, even though it parsed with cpp Nix: {error}",
+                                    location.file.display()
+                                ),
+                            Ok(nix_file) =>
+                                match nix_file.call_package_argument_info_at(
+                                    location.line,
+                                    location.column,
+                                    nixpkgs_path,
+                                )? {
+                                    None => Success(NonApplicable),
+                                    Some(call_package_argument_info) => Success(Loose(call_package_argument_info))
+                                },
+                        },
                     };
                     uses_by_name.map(|x| ratchet::Package {
                         manual_definition: Tight,
@@ -240,6 +271,7 @@ pub fn check_values(
                 ByName(Existing(CallPackage(CallPackageInfo {
                     is_derivation,
                     call_package_variant,
+                    ..
                 }))) => {
                     let check_result = if !is_derivation {
                         NixpkgsProblem::NonDerivation {
@@ -256,6 +288,8 @@ pub fn check_values(
                             manual_definition: Tight,
                             uses_by_name: Tight,
                         }),
+                        // TODO: Use the call_package_argument_info_at instead/additionally and
+                        // simplify the eval.nix code
                         Manual { path, empty_arg } => {
                             let correct_file = if let Some(call_package_path) = path {
                                 relative_package_file == *call_package_path
@@ -280,9 +314,10 @@ pub fn check_values(
                     })
                 }
             };
-            check_result.map(|value| (attribute_name.clone(), value))
-        },
-    ));
+            Ok::<_, anyhow::Error>(check_result.map(|value| (attribute_name.clone(), value)))
+        })
+        .collect_vec()?,
+    );
 
     Ok(check_result.map(|elems| ratchet::Nixpkgs {
         package_names: elems.iter().map(|(name, _)| name.to_owned()).collect(),
diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs
index 8179ec8ded74..9efff7a8b2bd 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/main.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs
@@ -1,4 +1,6 @@
+use crate::nix_file::NixFileCache;
 mod eval;
+mod nix_file;
 mod nixpkgs_problem;
 mod ratchet;
 mod references;
@@ -116,6 +118,8 @@ pub fn check_nixpkgs<W: io::Write>(
     keep_nix_path: bool,
     error_writer: &mut W,
 ) -> validation::Result<ratchet::Nixpkgs> {
+    let mut nix_file_cache = NixFileCache::new();
+
     Ok({
         let nixpkgs_path = nixpkgs_path.canonicalize().with_context(|| {
             format!(
@@ -134,7 +138,7 @@ pub fn check_nixpkgs<W: io::Write>(
         } else {
             check_structure(&nixpkgs_path)?.result_map(|package_names|
                 // Only if we could successfully parse the structure, we do the evaluation checks
-                eval::check_values(&nixpkgs_path, package_names, keep_nix_path))?
+                eval::check_values(&nixpkgs_path, &mut nix_file_cache, package_names, keep_nix_path))?
         }
     })
 }
@@ -169,7 +173,7 @@ mod tests {
 
     // tempfile::tempdir needs to be wrapped in temp_env lock
     // because it accesses TMPDIR environment variable.
-    fn tempdir() -> anyhow::Result<TempDir> {
+    pub fn tempdir() -> anyhow::Result<TempDir> {
         let empty_list: [(&str, Option<&str>); 0] = [];
         Ok(temp_env::with_vars(empty_list, tempfile::tempdir)?)
     }
diff --git a/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
new file mode 100644
index 000000000000..3e4f2668f3ea
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
@@ -0,0 +1,443 @@
+//! This is a utility module for interacting with the syntax of Nix files
+
+use rnix::SyntaxKind;
+use crate::utils::LineIndex;
+use anyhow::Context;
+use rnix::ast;
+use rnix::ast::Expr;
+use rnix::ast::HasEntry;
+use rowan::ast::AstNode;
+use rowan::TextSize;
+use rowan::TokenAtOffset;
+use std::collections::hash_map::Entry;
+use std::collections::HashMap;
+use std::fs::read_to_string;
+use std::path::Path;
+use std::path::PathBuf;
+
+/// A structure to indefinitely cache parse results of Nix files in memory,
+/// making sure that the same file never has to be parsed twice
+pub struct NixFileCache {
+    entries: HashMap<PathBuf, NixFileResult>,
+}
+
+impl NixFileCache {
+    /// Creates a new empty NixFileCache
+    pub fn new() -> NixFileCache {
+        NixFileCache {
+            entries: HashMap::new(),
+        }
+    }
+
+    /// Get the cache entry for a Nix file if it exists, otherwise parse the file, insert it into
+    /// the cache, and return the value
+    ///
+    /// Note that this function only gives an anyhow::Result::Err for I/O errors.
+    /// A parse error is anyhow::Result::Ok(Result::Err(error))
+    pub fn get(&mut self, path: &Path) -> anyhow::Result<&NixFileResult> {
+        match self.entries.entry(path.to_owned()) {
+            Entry::Occupied(entry) => Ok(entry.into_mut()),
+            Entry::Vacant(entry) => Ok(entry.insert(NixFile::new(path)?)),
+        }
+    }
+}
+
+/// A type to represent the result when trying to initialise a `NixFile`.
+type NixFileResult = Result<NixFile, rnix::parser::ParseError>;
+
+/// A structure for storing a successfully parsed Nix file
+pub struct NixFile {
+    /// The parent directory of the Nix file, for more convenient error handling
+    pub parent_dir: PathBuf,
+    /// The path to the file itself, for errors
+    pub path: PathBuf,
+    pub syntax_root: rnix::Root,
+    pub line_index: LineIndex,
+}
+
+impl NixFile {
+    /// Creates a new NixFile, failing for I/O or parse errors
+    fn new(path: impl AsRef<Path>) -> anyhow::Result<NixFileResult> {
+        let Some(parent_dir) = path.as_ref().parent() else {
+            anyhow::bail!("Could not get parent of path {}", path.as_ref().display())
+        };
+
+        let contents = read_to_string(&path)
+            .with_context(|| format!("Could not read file {}", path.as_ref().display()))?;
+        let line_index = LineIndex::new(&contents);
+
+        Ok(rnix::Root::parse(&contents).ok().map(|syntax_root| NixFile {
+            parent_dir: parent_dir.to_path_buf(),
+            path: path.as_ref().to_owned(),
+            syntax_root,
+            line_index,
+        }))
+    }
+}
+
+/// Information about callPackage arguments
+#[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>,
+    /// Whether the second argument is an empty attribute set
+    pub empty_arg: bool,
+}
+
+impl NixFile {
+    /// Returns information about callPackage arguments for an attribute at a specific line/column
+    /// index. If there's no guaranteed `callPackage` at that location, `None` is returned.
+    ///
+    /// This is meant to be used with the location returned from `builtins.unsafeGetAttrPos`, e.g.:
+    /// - Create file `default.nix` with contents
+    ///   ```nix
+    ///   self: {
+    ///     foo = self.callPackage ./default.nix { };
+    ///   }
+    ///   ```
+    /// - Evaluate
+    ///   ```nix
+    ///   builtins.unsafeGetAttrPos "foo" (import ./default.nix { })
+    ///   ```
+    ///   results in `{ file = ./default.nix; line = 2; column = 3; }`
+    /// - Get the NixFile for `.file` from a `NixFileCache`
+    /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current directory
+    ///
+    /// You'll get back
+    /// ```rust
+    /// Some(CallPackageArgumentInfo { path = Some("default.nix"), empty_arg: true })
+    /// ```
+    ///
+    /// Note that this also returns the same for `pythonPackages.callPackage`. It doesn't make an
+    /// attempt at distinguishing this.
+    pub fn call_package_argument_info_at(&self, 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)
+    }
+
+    // Internal function mainly to make it independently testable
+    fn attrpath_value_at(&self, line: usize, column: usize) -> anyhow::Result<Option<ast::AttrpathValue>> {
+        let index = self.line_index.fromlinecolumn(line, column);
+
+        let token_at_offset = self.syntax_root.syntax().token_at_offset(TextSize::from(index as u32));
+
+        // The token_at_offset function takes indices to mean a location _between_ characters,
+        // which in this case is some spacing followed by the attribute name:
+        //
+        //   foo = 10;
+        //  /\
+        //  This is the token offset, we get both the (newline + indentation) on the left side,
+        //  and the attribute name on the right side.
+        let TokenAtOffset::Between(_space, token) = token_at_offset else {
+            anyhow::bail!("Line {line} column {column} in {} is not the start of a token, but rather {token_at_offset:?}", self.path.display())
+        };
+
+        // token looks like "foo"
+        let Some(node) = token.parent() else {
+            anyhow::bail!("Token on line {line} column {column} in {} does not have a parent node: {token:?}", self.path.display())
+        };
+
+        // node looks like "foo"
+        let Some(attrpath_node) = node.parent() else {
+            anyhow::bail!("Node in {} does not have a parent node: {node:?}", self.path.display())
+        };
+
+        if attrpath_node.kind() != SyntaxKind::NODE_ATTRPATH {
+            return Ok(None)
+        }
+        // attrpath_node looks like "foo.bar"
+        let Some(attrpath_value_node) = attrpath_node.parent() else {
+            anyhow::bail!("Attribute path node in {} does not have a parent node: {attrpath_node:?}", self.path.display())
+        };
+
+        if ! ast::AttrpathValue::can_cast(attrpath_value_node.kind()) {
+            anyhow::bail!("Node in {} is not an attribute path value node: {attrpath_value_node:?}", self.path.display())
+        }
+        // attrpath_value_node looks like "foo.bar = 10;"
+
+        // unwrap is fine because we confirmed that we can cast with the above check
+        Ok(Some(ast::AttrpathValue::cast(attrpath_value_node).unwrap()))
+    }
+
+    // Internal function mainly to make attrpath_value_at independently testable
+    fn attrpath_value_call_package_argument_info(
+        &self,
+        attrpath_value: ast::AttrpathValue,
+        relative_to: &Path,
+    ) -> anyhow::Result<Option<CallPackageArgumentInfo>> {
+        let Some(attrpath) = attrpath_value.attrpath() else {
+            anyhow::bail!("attrpath value node doesn't have an attrpath: {attrpath_value:?}")
+        };
+
+        // At this point we know it's something like `foo...bar = ...`
+
+        if attrpath.attrs().count() > 1 {
+            // If the attribute path has multiple entries, the left-most entry is an attribute and
+            // can't be a `callPackage`.
+            //
+            // FIXME: `builtins.unsafeGetAttrPos` will return the same position for all attribute
+            // paths and we can't really know which one it is. We could have a case like
+            // `foo.bar = callPackage ... { }` and trying to determine if `bar` is a `callPackage`,
+            // where this is not correct.
+            // However, this case typically doesn't occur anyways,
+            // because top-level packages wouldn't be nested under an attribute set.
+            return Ok(None);
+        }
+        let Some(value) = attrpath_value.value() else {
+            anyhow::bail!("attrpath value node doesn't have a value: {attrpath_value:?}")
+        };
+
+        // At this point we know it's something like `foo = ...`
+
+        let Expr::Apply(apply1) = value else {
+            // Not even a function call, instead something like `foo = null`
+            return Ok(None);
+        };
+        let Some(function1) = apply1.lambda() else {
+            anyhow::bail!("apply node doesn't have a lambda: {apply1:?}")
+        };
+        let Some(arg1) = apply1.argument() else {
+            anyhow::bail!("apply node doesn't have an argument: {apply1:?}")
+        };
+
+        // At this point we know it's something like `foo = <fun> <arg>`.
+        // For a callPackage, `<fun>` would be `callPackage ./file` and `<arg>` would be `{ }`
+
+        let empty_arg = if let Expr::AttrSet(attrset) = arg1 {
+            // We can only statically determine whether the argument is empty if it's an attribute
+            // set _expression_, even though other kind of expressions could evaluate to an attribute
+            // set _value_. But this is what we want anyways
+            attrset.entries().next().is_none()
+        } else {
+            false
+        };
+
+        // Because callPackage takes two curried arguments, the first function needs to be a
+        // function call itself
+        let Expr::Apply(apply2) = function1 else {
+            // Not a callPackage, instead something like `foo = import ./foo`
+            return Ok(None);
+        };
+        let Some(function2) = apply2.lambda() else {
+            anyhow::bail!("apply node doesn't have a lambda: {apply2:?}")
+        };
+        let Some(arg2) = apply2.argument() else {
+            anyhow::bail!("apply node doesn't have an argument: {apply2:?}")
+        };
+
+        // At this point we know it's something like `foo = <fun2> <arg2> <arg1>`.
+        // For a callPackage, `<fun2>` would be `callPackage`, `<arg2>` would be `./file`
+
+        // Check that <arg2> is a path expression
+        let path = if let Expr::Path(actual_path) = arg2 {
+            // Try to statically resolve the path and turn it into a nixpkgs-relative path
+            if let ResolvedPath::Within(p) = self.static_resolve_path(actual_path, relative_to) {
+                Some(p)
+            } else {
+                // We can't statically know an existing path inside Nixpkgs used as <arg2>
+                None
+            }
+        } else {
+            // <arg2> is not a path, but rather e.g. an inline expression
+            None
+        };
+
+        // Check that <fun2> is an identifier, or an attribute path with an identifier at the end
+        let ident = match function2 {
+            Expr::Ident(ident) =>
+                // This means it's something like `foo = callPackage <arg2> <arg1>`
+                ident,
+            Expr::Select(select) => {
+                // This means it's something like `foo = self.callPackage <arg2> <arg1>`.
+                // We also end up here for e.g. `pythonPackages.callPackage`, but the
+                // callPackage-mocking method will take care of not triggering for this case.
+
+                if select.default_expr().is_some() {
+                    // Very odd case, but this would be `foo = self.callPackage or true ./test.nix {}
+                    // (yes this is valid Nix code)
+                    return Ok(None)
+                }
+                let Some(attrpath) = select.attrpath() else {
+                    anyhow::bail!("select node doesn't have an attrpath: {select:?}")
+                };
+                let Some(last) = attrpath.attrs().last() else {
+                    // This case shouldn't be possible, it would be `foo = self. ./test.nix {}`,
+                    // which shouldn't parse
+                    anyhow::bail!("select node has an empty attrpath: {select:?}")
+                };
+                if let ast::Attr::Ident(ident) = last {
+                    ident
+                } else {
+                    // Here it's something like `foo = self."callPackage" /test.nix {}`
+                    // which we're not gonna bother with
+                    return Ok(None)
+                }
+            },
+            // Any other expression we're not gonna treat as callPackage
+            _ => return Ok(None),
+        };
+
+        let Some(token) = ident.ident_token() else {
+            anyhow::bail!("ident node doesn't have a token: {ident:?}")
+        };
+
+        if token.text() == "callPackage" {
+            Ok(Some(CallPackageArgumentInfo { relative_path: path, empty_arg }))
+        } else {
+            Ok(None)
+        }
+    }
+}
+
+/// The result of trying to statically resolve a Nix path expression
+pub enum ResolvedPath {
+    /// Something like `./foo/${bar}/baz`, can't be known statically
+    Interpolated,
+    /// Something like `<nixpkgs>`, can't be known statically
+    SearchPath,
+    /// Path couldn't be resolved due to an IO error,
+    /// e.g. if the path doesn't exist or you don't have the right permissions
+    Unresolvable(std::io::Error),
+    /// 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),
+}
+
+impl NixFile {
+    /// Statically resolves a Nix path expression and checks that it's within an absolute path
+    /// path
+    ///
+    /// E.g. for the path expression `./bar.nix` in `./foo.nix` and an absolute path of the
+    /// current directory, the function returns `ResolvedPath::Within(./bar.nix)`
+    pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath {
+        if node.parts().count() != 1 {
+            // Only if there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`.
+            return ResolvedPath::Interpolated;
+        }
+
+        let text = node.to_string();
+
+        if text.starts_with('<') {
+            // A search path like `<nixpkgs>`. There doesn't appear to be better way to detect
+            // these in rnix
+            return ResolvedPath::SearchPath;
+        }
+
+        // Join the file's parent directory and the path expression, then resolve it
+        // FIXME: Expressions like `../../../../foo/bar/baz/qux` or absolute paths
+        // may resolve close to the original file, but may have left the relative_to.
+        // That should be checked more strictly
+        match self.parent_dir.join(Path::new(&text)).canonicalize() {
+            Err(resolution_error) => ResolvedPath::Unresolvable(resolution_error),
+            Ok(resolved) =>
+                // 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()),
+                }
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::tests;
+    use super::*;
+    use indoc::indoc;
+
+    #[test]
+    fn detects_attributes() -> anyhow::Result<()> {
+        let temp_dir = tests::tempdir()?;
+        let file = temp_dir.path().join("file.nix");
+        let contents = indoc! {r#"
+            toInherit: {
+              foo = 1;
+              "bar" = 2;
+              ${"baz"} = 3;
+              "${"qux"}" = 4;
+
+              # A
+              quux
+              # B
+              =
+              # C
+              5
+              # D
+              ;
+              # E
+
+              /**/quuux/**/=/**/5/**/;/*E*/
+
+              inherit toInherit;
+            }
+        "#};
+
+        std::fs::write(&file, contents)?;
+
+        let nix_file = NixFile::new(&file)??;
+
+        // 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),
+        ];
+
+        for (line, column, expected_result) in cases {
+            let actual_result = nix_file.attrpath_value_at(line, column)?.map(|node| node.to_string());
+            assert_eq!(actual_result.as_deref(), expected_result);
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn detects_call_package() -> anyhow::Result<()> {
+        let temp_dir = tests::tempdir()?;
+        let file = temp_dir.path().join("file.nix");
+        let contents = indoc! {r#"
+            self: with self; {
+              a.sub = null;
+              b = null;
+              c = import ./file.nix;
+              d = import ./file.nix { };
+              e = pythonPackages.callPackage ./file.nix { };
+              f = callPackage ./file.nix { };
+              g = callPackage ({ }: { }) { };
+              h = callPackage ./file.nix { x = 0; };
+              i = callPackage ({ }: { }) (let in { });
+            }
+        "#};
+
+        std::fs::write(&file, contents)?;
+
+        let nix_file = NixFile::new(&file)??;
+
+        let cases = [
+            (2, None),
+            (3, None),
+            (4, None),
+            (5, None),
+            (6, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true })),
+            (7, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: true })),
+            (8, Some(CallPackageArgumentInfo { relative_path: None, empty_arg: true })),
+            (9, Some(CallPackageArgumentInfo { relative_path: Some(PathBuf::from("file.nix")), empty_arg: false })),
+            (10, Some(CallPackageArgumentInfo { relative_path: None, empty_arg: false })),
+        ];
+
+        for (line, expected_result) in cases {
+            let actual_result = nix_file.call_package_argument_info_at(line, 3, temp_dir.path())?;
+            assert_eq!(actual_result, expected_result);
+        }
+
+        Ok(())
+    }
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index 10ecc01d3580..200bf92c516a 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -2,11 +2,11 @@
 //!
 //! Each type has a `compare` method that validates the ratchet checks for that item.
 
+use crate::nix_file::CallPackageArgumentInfo;
 use crate::nixpkgs_problem::NixpkgsProblem;
 use crate::structure;
 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)]
@@ -148,16 +148,8 @@ impl ToNixpkgsProblem for ManualDefinition {
 /// It also checks that once a package uses pkgs/by-name, it can't switch back to all-packages.nix
 pub enum UsesByName {}
 
-#[derive(Clone)]
-pub struct CouldUseByName {
-    /// The first callPackage argument, used for better errors
-    pub call_package_path: Option<PathBuf>,
-    /// Whether the second callPackage argument is empty, used for better errors
-    pub empty_arg: bool,
-}
-
 impl ToNixpkgsProblem for UsesByName {
-    type ToContext = CouldUseByName;
+    type ToContext = CallPackageArgumentInfo;
 
     fn to_nixpkgs_problem(
         name: &str,
@@ -167,13 +159,13 @@ impl ToNixpkgsProblem for UsesByName {
         if let Some(()) = optional_from {
             NixpkgsProblem::MovedOutOfByName {
                 package_name: name.to_owned(),
-                call_package_path: to.call_package_path.clone(),
+                call_package_path: to.relative_path.clone(),
                 empty_arg: to.empty_arg,
             }
         } else {
             NixpkgsProblem::NewPackageNotUsingByName {
                 package_name: name.to_owned(),
-                call_package_path: to.call_package_path.clone(),
+                call_package_path: to.relative_path.clone(),
                 empty_arg: to.empty_arg,
             }
         }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix
new file mode 100644
index 000000000000..306d719c9e9d
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix
@@ -0,0 +1,7 @@
+self: super: {
+  set = self.callPackages ({ callPackage }: {
+    foo = callPackage ({ someDrv }: someDrv) { };
+  }) { };
+
+  inherit (self.set) foo;
+}
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix
new file mode 100644
index 000000000000..861260cdca4b
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md
new file mode 100644
index 000000000000..e69de29bb2d1
--- /dev/null
+++ b/pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md