gador/nixpkgs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Silvan Mosberger
2024-01-30 21:53:59 +01:00
parent ed892915b8
commit 0bcb052284
10 changed files with 516 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkgs/test/nixpkgs-check-by-name/Cargo.lock
Generated
+8
View File
@@ -213,6 +213,12 @@ version = "0.3.2"
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"
@@ -289,10 +295,12 @@ dependencies = [
"anyhow",
"clap",
"colored",
"indoc",
"itertools",
"lazy_static",
"regex",
"rnix",
"rowan",
"serde",
"serde_json",
"temp-env",
pkgs/test/nixpkgs-check-by-name/Cargo.toml
+2
View File
@@ -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"
pkgs/test/nixpkgs-check-by-name/src/eval.nix
+1
View File
@@ -76,6 +76,7 @@ let
CallPackage = {
call_package_variant = value._callPackageVariant;
is_derivation = pkgs.lib.isDerivation value;
location = builtins.unsafeGetAttrPos name pkgs;
};
};
pkgs/test/nixpkgs-check-by-name/src/eval.rs
+44 -9
View File
@@ -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(),
pkgs/test/nixpkgs-check-by-name/src/main.rs
+6 -2
View File
@@ -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)?)
}
pkgs/test/nixpkgs-check-by-name/src/nix_file.rs
+443
View File
@@ -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(())
}
}
pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+4 -12
View File
@@ -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,
}
}
pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/all-packages.nix
+7
View File
@@ -0,0 +1,7 @@
self: super: {
set = self.callPackages ({ callPackage }: {
foo = callPackage ({ someDrv }: someDrv) { };
}) { };
inherit (self.set) foo;
}
pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/default.nix
+1
View File
@@ -0,0 +1 @@
import <test-nixpkgs> { root = ./.; }
pkgs/test/nixpkgs-check-by-name/tests/callPackage-syntax/pkgs/by-name/README.md
View File