diff --git a/Cargo.lock b/Cargo.lock index aeb3908..6231d75 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -146,6 +146,12 @@ version = "3.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7704b5fdd17b18ae31c4c1da5a2e0305a2bf17b5249300a9ee9ed7b72114c636" +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "either" version = "1.13.0" @@ -260,6 +266,7 @@ dependencies = [ "indoc", "itertools", "lazy_static", + "pretty_assertions", "regex", "relative-path", "rnix", @@ -294,6 +301,16 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "pretty_assertions" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af7cee1a6c8a5b9208b3cb1061f10c0cb689087b3d8ce85fb9d2dd7a29b6ba66" +dependencies = [ + "diff", + "yansi", +] + [[package]] name = "proc-macro2" version = "1.0.86" @@ -669,3 +686,9 @@ name = "windows_x86_64_msvc" version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" + +[[package]] +name = "yansi" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09041cd90cf85f7f8b2df60c646f853b7f535ce68f85244eb6731cf89fa498ec" diff --git a/Cargo.toml b/Cargo.toml index 95754be..44e7960 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,4 +20,5 @@ relative-path = "1.9.3" textwrap = "0.16.1" [dev-dependencies] +pretty_assertions = "1.4.0" temp-env = "0.3.6" diff --git a/default.nix b/default.nix index 8bb7bac..0c1592b 100644 --- a/default.nix +++ b/default.nix @@ -161,21 +161,20 @@ let ''; }; - # Tests the tool on the pinned Nixpkgs tree, this is a good sanity check - nixpkgsCheck = - pkgs.runCommand "test-nixpkgs-check-by-name" - { - nativeBuildInputs = [ - packages.build - pkgs.nix - ]; - nixpkgsPath = nixpkgs; - } - '' - ${initNix} - nixpkgs-check-by-name --base "$nixpkgsPath" "$nixpkgsPath" - touch $out - ''; + # Tests the tool on the pinned Nixpkgs tree with stable Nix. This is a good sanity check. + nixpkgsCheck = pkgs.callPackage ./nixpkgs-check.nix { + inherit initNix nixpkgs; + nixpkgs-check-by-name = packages.build; + nix = pkgs.nixVersions.stable; + }; + + # Tests the tool on the pinned Nixpkgs tree with various Nix and Lix versions. + # This allows exposure to changes in behavior from Nix and Nix-alikes. + nixpkgsCheckWithLatestNix = packages.nixpkgsCheck.nixVersions.latest; + nixpkgsCheckWithGitNix = packages.nixpkgsCheck.nixVersions.git; + nixpkgsCheckWithMinimumNix = packages.nixpkgsCheck.nixVersions.minimum; + nixpkgsCheckWithStableLix = packages.nixpkgsCheck.lixVersions.stable; + nixpkgsCheckWithLatestLix = packages.nixpkgsCheck.lixVersions.latest; }; in packages diff --git a/nixpkgs-check.nix b/nixpkgs-check.nix new file mode 100644 index 0000000..3ffe24b --- /dev/null +++ b/nixpkgs-check.nix @@ -0,0 +1,56 @@ +{ + lib, + runCommand, + nixpkgs-check-by-name, + initNix, + nixpkgs, + nix, + nixVersions, + lixVersions, +}: +let + # Given an attrset, return only the values that both eval and are derivations. + # + # We do this to avoid encoding information about which names are in present in the attrset. + # For instance, `nixVersions` contains `nix_2_10`, which throws, and `lixVersions` does not + # contain `minimum` or `git`, but `nixVersions` does. + # + # Let's just map over those attrsets and return what's useful from there. + derivationsFromAttrset = + attrset: + lib.filterAttrs ( + name: value: + let + eval = builtins.tryEval value; + in + eval.success && lib.isDerivation eval.value + ) attrset; + + mkNixpkgsCheck = + name: nix: + runCommand "test-nixpkgs-check-by-name-with-${nix.name}" + { + nativeBuildInputs = [ + nixpkgs-check-by-name + nix + ]; + + env.NIX_CHECK_BY_NAME_NIX_PACKAGE = lib.getBin nix; + + passthru = { + # Allow running against all other Nix versions. + nixVersions = lib.mapAttrs mkNixpkgsCheck (derivationsFromAttrset nixVersions); + + # Allow running against all other Lix versions. + lixVersions = lib.mapAttrs mkNixpkgsCheck (derivationsFromAttrset lixVersions); + }; + } + '' + ${initNix} + # This is what nixpkgs-check-by-name uses + export NIX_CHECK_BY_NAME_NIX_PACKAGE=${lib.getBin nix} + ${nixpkgs-check-by-name}/bin/.nixpkgs-check-by-name-wrapped --base "${nixpkgs}" "${nixpkgs}" + touch $out + ''; +in +mkNixpkgsCheck nix.name nix diff --git a/package.nix b/package.nix index a2e1da5..5b6dbda 100644 --- a/package.nix +++ b/package.nix @@ -3,6 +3,7 @@ rustPlatform, nix, nixVersions, + lixVersions, clippy, makeWrapper, @@ -10,6 +11,9 @@ nix nixVersions.stable nixVersions.minimum + nixVersions.latest + lixVersions.stable + lixVersions.latest ], nixpkgsLibPath, diff --git a/src/nix_file.rs b/src/nix_file.rs index 3b1695f..6a5c665 100644 --- a/src/nix_file.rs +++ b/src/nix_file.rs @@ -187,12 +187,12 @@ impl NixFile { }; if ast::Attr::can_cast(node.kind()) { - // Something like `foo`, `"foo"` or `${"foo"}` + // Something like `foo`, `"foo"` or `${"foo"}`. + // For Nix versions >= 2.21, this is also `foo` in `inherit (bar) foo`. } else if ast::Inherit::can_cast(node.kind()) { - // Something like `inherit ` or `inherit () `. - // This is the only other way how `builtins.unsafeGetAttrPos` can return - // attribute positions, but we only look for ones like ` = `, - // so ignore this. + // For Nix versions <= 2.20, this is something like `inherit ` or + // `inherit () `. Since we only look for expressions like + // ` = `, ignore this. return Ok(Left(node.to_string())); } else { // However, anything else is not expected and smells like a bug. @@ -210,6 +210,13 @@ impl NixFile { ) }; + // For Nix versions >= 2.21, an `ast::Attr` can have a parent of `ast::Inherit`. + if ast::Inherit::can_cast(attrpath_node.kind()) { + // Something like `inherit ` or `inherit () `. + // We only look for ones like ` = `, so ignore this. + return Ok(Left(node.to_string())); + }; + if !ast::Attrpath::can_cast(attrpath_node.kind()) { // We know that `node` is an attribute, so its parent should be an attribute path. anyhow::bail!( @@ -449,20 +456,28 @@ mod tests { use super::*; use crate::tests; use indoc::indoc; + use pretty_assertions::assert_eq; + use std::collections::BTreeMap; + + #[derive(Debug, PartialEq, Eq, PartialOrd, Ord)] + struct Position { + line: usize, + column: usize, + } #[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; + testG: { + testA = 1; + "testB" = 2; + ${"testC"} = 3; + "${"testD"}" = 4; # A - quux + testE # B = # C @@ -471,10 +486,14 @@ mod tests { ; # E - /**/quuux/**/=/**/5/**/;/*E*/ + /**/testF/**/=/**/5/**/;/*E*/ - inherit toInherit; - inherit (toInherit) toInherit; + inherit testG; + inherit ({}) testH; + + inherit ({}) + testI testJ + testK testL; } "#}; @@ -482,31 +501,86 @@ mod tests { let nix_file = NixFile::new(&file)?; - // These are `builtins.unsafeGetAttrPos` locations for the attributes + // These are `builtins.unsafeGetAttrPos` locations for the attributes, generated by saving + // the Nix file above as `test.nix`, then making this `driver.nix` file. + // + // ```nix + // let + // value = import ./test.nix null; + // info = + // name: + // let + // inherit (builtins.unsafeGetAttrPos name value) line column; + // in + // _: ''(${toString line}, ${toString column}, Left("fill me in")),''; + // in + // builtins.mapAttrs info value + // ``` + // + // Then, run this command to get the raw test data. + // + // ``` + // for v in Versions.{stable,minimum,latest} lixVersions.{stable,latest}; do + // nix-instantiate --strict --eval --json driver.nix | jq -r .[] + // done | sort -u + // ``` + // + // Fill in the expected data until the test passes. let cases = [ - (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;")), + // generated with all Nix versions + (2, 3, Right("testA = 1;")), + // generated with all Nix versions + (3, 3, Right("\"testB\" = 2;")), + // generated with all Nix versions + (4, 3, Right("${\"testC\"} = 3;")), + // generated with all Nix versions + (5, 3, Right("\"${\"testD\"}\" = 4;")), + // generated with all Nix versions + (8, 3, Right("testE\n # B\n =\n # C\n 5\n # D\n ;")), + // generated on all Nix versions + (17, 7, Right("testF/**/=/**/5/**/;")), + // generated with Nix <= 2.20 + (19, 10, Left("inherit testG;")), + // generated with Nix >= 2.21 + (19, 11, Left("testG")), + // generated with Nix <= 2.20 + (20, 15, Left("inherit ({}) testH;")), + // generated with Nix >= 2.21 + (20, 16, Left("testH")), + // generated with Nix <= 2.20 + ( + 22, + 15, + Left("inherit ({})\n testI testJ\n testK testL;"), + ), + // generated with Nix >= 2.21 + (23, 5, Left("testI")), + // generated with Nix >= 2.21 + (23, 11, Left("testJ")), + // generated with Nix >= 2.21 + (24, 5, Left("testK")), + // generated with Nix >= 2.21 + (24, 11, Left("testL")), ]; - - for (line, column, expected_result) in cases { - let actual_result = nix_file - .attrpath_value_at(line, column) - .context(format!("line {line}, column {column}"))? - .map_right(|node| node.to_string()); - let owned_expected_result = expected_result - .map(|x| x.to_string()) - .map_left(|x| x.to_string()); - assert_eq!( - actual_result, owned_expected_result, - "line {line}, column {column}" - ); - } + let expected = BTreeMap::from_iter(cases.map(|(line, column, result)| { + ( + Position { line, column }, + result + .map(|node| node.to_string()) + .map_right(|node| node.to_string()), + ) + })); + let actual = BTreeMap::from_iter(cases.map(|(line, column, _)| { + ( + Position { line, column }, + nix_file + .attrpath_value_at(line, column) + .context(format!("line {line}, column {column}")) + .expect("value") + .map_right(|node| node.to_string()), + ) + })); + assert_eq!(expected, actual); Ok(()) }