Merge pull request #79 from philiptaron/issue-78/fix-inherit-io-issue

Allow nixpkgs-check-by-name to work with recent Nix versions

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"

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

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

package.nix

@@ -3,13 +3,17 @@
   rustPlatform,
   nix,
   nixVersions,
+  lixVersions,
   clippy,
   makeWrapper,
 
   nixVersionsToTest ? [
     nix
     nixVersions.stable
     nixVersions.minimum
+    nixVersions.latest
+    lixVersions.stable
+    lixVersions.latest
   ],
 
   nixpkgsLibPath,

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 <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.
+            // For Nix versions <= 2.20, this is something like `inherit <attr>` or
+            // `inherit (<source>) <attr>`. Since we only look for expressions like
+            // `<attr-path> = <value>`, 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 <attr>` or `inherit (<source>) <attr>`.
+            // We only look for ones like `<attr-path> = <value>`, 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,42 +486,101 @@ mod tests {
               ;
               # E
 
-              /**/quuux/**/=/**/5/**/;/*E*/
+              /**/testF/**/=/**/5/**/;/*E*/
 
-              inherit toInherit;
-              inherit (toInherit) toInherit;
+              inherit testG;
+              inherit ({}) testH;
+
+              inherit ({})
+                testI testJ
+                testK testL;
             }
         "#};
 
         std::fs::write(&file, contents)?;
 
         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(())
     }