lib.fileset: Lazier path existence check

doc/functions/fileset.section.md

@@ -15,7 +15,8 @@ Such path arguments are implicitly coerced to file sets containing all files und
 - A path to a file turns into a file set containing that single file.
 - A path to a directory turns into a file set containing all files _recursively_ in that directory.
 
-If the path points to a non-existent location, an error is thrown.
+If the path points to a non-existent location, an error is thrown
+only once/if either the file's type or contents are needed.
 
 ::: {.note}
 Just like in Git, file sets cannot represent empty directories.

lib/fileset/README.md

@@ -58,7 +58,8 @@ An attribute set with these values:
 
 - `_internalBase` (path):
   Any files outside of this path cannot influence the set of files.
-  This is always a directory and should be as long as possible.
+  This path should be as long as possible.
+  This may be a directory or a file.
   This is used by `lib.fileset.toSource` to check that all files are under the `root` argument
 
 - `_internalBaseRoot` (path):

lib/fileset/default.nix

@@ -3,7 +3,7 @@ let
 
   inherit (import ./internal.nix { inherit lib; })
     _coerce
-    _singleton
+    _create
     _coerceMany
     _toSourceFilter
     _fromSourceFilter
@@ -257,12 +257,12 @@ in {
           lib.fileset.fromSource: The source origin of the argument is of type ${typeOf path}, but it should be a path instead.''
     else if ! pathExists path then
       throw ''
-        lib.fileset.fromSource: The source origin (${toString path}) of the argument does not exist.''
+        lib.fileset.fromSource: The source origin (${toString path}) of the argument is a path that does not exist.''
     else if isFiltered then
       _fromSourceFilter path source.filter
     else
       # If there's no filter, no need to run the expensive conversion, all subpaths will be included
-      _singleton path;
+      _create path (pathType path);
 
   /*
     The file set containing all files that are in either of two given file sets.

lib/fileset/internal.nix

@@ -65,7 +65,7 @@ rec {
   # - Increment this version
   # - Add an additional migration function below
   # - Update the description of the internal representation in ./README.md
-  _currentVersion = 3;
+  _currentVersion = 4;
 
   # Migrations between versions. The 0th element converts from v0 to v1, and so on
   migrations = [
@@ -100,6 +100,16 @@ rec {
         _internalVersion = 3;
       }
     )
+
+    # Convert v3 into v4: filesetTree's can now have an _internalBase that's a file
+    (
+      filesetV3:
+      filesetV3 // {
+        # This change is backwards compatible but not forwards compatible,
+        # old code can't handle files in _internalBase
+        _internalVersion = 4;
+      }
+    )
   ];
 
   _noEvalMessage = ''
@@ -179,11 +189,15 @@ rec {
       else
         throw ''
           ${context} is of type ${typeOf value}, but it should be a file set or a path instead.''
-    else if ! pathExists value then
-      throw ''
-        ${context} (${toString value}) is a path that does not exist.''
     else
-      _singleton value;
+      _create value (
+        # Only check for file existence if we need to know the file type
+        if ! pathExists value then
+          throw ''
+            ${context} (${toString value}) is a path that does not exist.''
+        else
+          pathType value
+      );
 
   # Coerce many values to filesets, erroring when any value cannot be coerced,
   # or if the filesystem root of the values doesn't match.
@@ -216,26 +230,6 @@ rec {
     else
       filesets;
 
-  # Create a file set from a path.
-  # Type: Path -> fileset
-  _singleton = path:
-    let
-      type = pathType path;
-    in
-    if type == "directory" then
-      _create path type
-    else
-      # This turns a file path ./default.nix into a fileset with
-      # - _internalBase: ./.
-      # - _internalTree: {
-      #     "default.nix" = <type>;
-      #   }
-      # See ./README.md#single-files
-      _create (dirOf path)
-        {
-          ${baseNameOf path} = type;
-        };
-
   # Expand a directory representation to an equivalent one in attribute set form.
   # All directory entries are included in the result.
   # Type: Path -> filesetTree -> { <name> = filesetTree; }
@@ -375,13 +369,20 @@ rec {
   _printFileset = fileset:
     if fileset._internalIsEmptyWithoutBase then
       trace "(empty)" null
-    else
+    else if isAttrs fileset._internalTree || fileset._internalTree == "directory" then
       _printMinimalTree fileset._internalBase
-        (_normaliseTreeMinimal fileset._internalBase fileset._internalTree);
+        (_normaliseTreeMinimal fileset._internalBase fileset._internalTree)
+    else
+      # Pretty-printed file sets should always use a directory as the base path,
+      # since that's what toSource would require and how e.g. fileFilter works
+      _printMinimalTree (dirOf fileset._internalBase)
+        (_normaliseTreeMinimal (dirOf fileset._internalBase) {
+          ${baseNameOf fileset._internalBase} = fileset._internalTree;
+        });
 
   # Turn a fileset into a source filter function suitable for `builtins.path`
   # Only directories recursively containing at least one files are recursed into
-  # Type: Path -> fileset -> (String -> String -> Bool)
+  # Type: fileset -> (String -> String -> Bool)
   _toSourceFilter = fileset:
     let
       # Simplify the tree, necessary to make sure all empty directories are null
@@ -753,9 +754,9 @@ rec {
 
       resultingTree =
         _differenceTree
-        positive._internalBase
-        positive._internalTree
-        negativeTreeWithPositiveBase;
+          positive._internalBase
+          positive._internalTree
+          negativeTreeWithPositiveBase;
     in
     # If the first file set is empty, we can never have any files in the result
     if positive._internalIsEmptyWithoutBase then
@@ -808,7 +809,14 @@ rec {
     in
     if fileset._internalIsEmptyWithoutBase then
       _emptyWithoutBase
-    else
+    else if isAttrs fileset._internalTree || fileset._internalTree == "directory" then
       _create fileset._internalBase
-        (recurse fileset._internalBase fileset._internalTree);
+        (recurse fileset._internalBase fileset._internalTree)
+    else
+      # The recurse function can only handle directories
+      # Single files in the _internalBase need to be turned into a directory with only that file
+      _create (dirOf fileset._internalBase)
+        (recurse (dirOf fileset._internalBase) {
+          ${baseNameOf fileset._internalBase} = fileset._internalTree;
+        });
 }

lib/fileset/tests.sh

@@ -404,23 +404,25 @@ expectFailure '_emptyWithoutBase' 'lib.fileset: Directly evaluating a file set i
 
 # Past versions of the internal representation are supported
 expectEqual '_coerce "<tests>: value" { _type = "fileset"; _internalVersion = 0; _internalBase = ./.; }' \
-    '{ _internalBase = ./.; _internalBaseComponents = path.subpath.components (path.splitRoot ./.).subpath; _internalBaseRoot = /.; _internalIsEmptyWithoutBase = false; _internalVersion = 3; _type = "fileset"; }'
+    '{ _internalBase = ./.; _internalBaseComponents = path.subpath.components (path.splitRoot ./.).subpath; _internalBaseRoot = /.; _internalIsEmptyWithoutBase = false; _internalVersion = 4; _type = "fileset"; }'
 expectEqual '_coerce "<tests>: value" { _type = "fileset"; _internalVersion = 1; }' \
-    '{ _type = "fileset"; _internalIsEmptyWithoutBase = false; _internalVersion = 3; }'
+    '{ _type = "fileset"; _internalIsEmptyWithoutBase = false; _internalVersion = 4; }'
 expectEqual '_coerce "<tests>: value" { _type = "fileset"; _internalVersion = 2; }' \
-    '{ _type = "fileset"; _internalIsEmptyWithoutBase = false; _internalVersion = 3; }'
+    '{ _type = "fileset"; _internalIsEmptyWithoutBase = false; _internalVersion = 4; }'
+expectEqual '_coerce "<tests>: value" { _type = "fileset"; _internalVersion = 3; }' \
+    '{ _type = "fileset"; _internalVersion = 4; }'
 
 # Future versions of the internal representation are unsupported
-expectFailure '_coerce "<tests>: value" { _type = "fileset"; _internalVersion = 4; }' '<tests>: value is a file set created from a future version of the file set library with a different internal representation:
-\s*- Internal version of the file set: 4
-\s*- Internal version of the library: 3
+expectFailure '_coerce "<tests>: value" { _type = "fileset"; _internalVersion = 5; }' '<tests>: value is a file set created from a future version of the file set library with a different internal representation:
+\s*- Internal version of the file set: 5
+\s*- Internal version of the library: 4
 \s*Make sure to update your Nixpkgs to have a newer version of `lib.fileset`.'
 
 # _create followed by _coerce should give the inputs back without any validation
 expectEqual '{
   inherit (_coerce "<test>" (_create ./. "directory"))
     _internalVersion _internalBase _internalTree;
-}' '{ _internalBase = ./.; _internalTree = "directory"; _internalVersion = 3; }'
+}' '{ _internalBase = ./.; _internalTree = "directory"; _internalVersion = 4; }'
 
 #### Resulting store path ####
 
@@ -528,10 +530,12 @@ expectFailure 'with ((import <nixpkgs/lib>).extend (import <nixpkgs/lib/fileset/
 rm -rf -- *
 
 # Coercion errors show the correct context
-expectFailure 'toSource { root = ./.; fileset = union ./a ./.; }' 'lib.fileset.union: First argument \('"$work"'/a\) is a path that does not exist.'
-expectFailure 'toSource { root = ./.; fileset = union ./. ./b; }' 'lib.fileset.union: Second argument \('"$work"'/b\) is a path that does not exist.'
-expectFailure 'toSource { root = ./.; fileset = unions [ ./a ./. ]; }' 'lib.fileset.unions: Element 0 \('"$work"'/a\) is a path that does not exist.'
-expectFailure 'toSource { root = ./.; fileset = unions [ ./. ./b ]; }' 'lib.fileset.unions: Element 1 \('"$work"'/b\) is a path that does not exist.'
+touch e
+expectFailure 'toSource { root = ./.; fileset = union ./a ./e; }' 'lib.fileset.union: First argument \('"$work"'/a\) is a path that does not exist.'
+expectFailure 'toSource { root = ./.; fileset = union ./e ./b; }' 'lib.fileset.union: Second argument \('"$work"'/b\) is a path that does not exist.'
+expectFailure 'toSource { root = ./.; fileset = unions [ ./a ./e ]; }' 'lib.fileset.unions: Element 0 \('"$work"'/a\) is a path that does not exist.'
+expectFailure 'toSource { root = ./.; fileset = unions [ ./e ./b ]; }' 'lib.fileset.unions: Element 1 \('"$work"'/b\) is a path that does not exist.'
+rm -rf -- *
 
 # unions needs a list
 expectFailure 'toSource { root = ./.; fileset = unions null; }' 'lib.fileset.unions: Argument is of type null, but it should be a list instead.'
@@ -637,6 +641,12 @@ rm -rf -- *
 expectFailure 'toSource { root = ./.; fileset = intersection ./a ./.; }' 'lib.fileset.intersection: First argument \('"$work"'/a\) is a path that does not exist.'
 expectFailure 'toSource { root = ./.; fileset = intersection ./. ./b; }' 'lib.fileset.intersection: Second argument \('"$work"'/b\) is a path that does not exist.'
 
+# Files that wouldn't be included anyways don't need to exist
+tree=(
+    [a]=1
+)
+checkFileset 'intersection (union ./a ./b) (union ./a ./c)'
+
 # The tree of later arguments should not be evaluated if a former argument already excludes all files
 tree=(
     [a]=0
@@ -721,6 +731,30 @@ checkFileset 'difference ./. ./.'
 checkFileset 'difference _emptyWithoutBase (_create ./. (abort "This should not be used!"))'
 checkFileset 'difference (_create ./. null) (_create ./. (abort "This should not be used!"))'
 
+# We can try to remove non-existent files
+tree=(
+    [a]=1
+)
+checkFileset 'difference ./. ./b'
+checkFileset 'difference ./. (difference ./b ./b/d)'
+
+# A double-negative is currently not checked for existence
+# TODO: This behavior is not great, this should give an error instead.
+# Generally all files that would be included in the final result should be checked for existence
+tree=(
+    [a]=1
+    [b/c]=0
+)
+checkFileset 'difference ./. (difference ./b ./b/d)'
+
+# If a double-negative exists, it's included
+tree=(
+    [a]=1
+    [b/c]=0
+    [b/d]=1
+)
+checkFileset 'difference ./. (difference ./b ./b/d)'
+
 # Subtracting nothing gives the same thing back
 tree=(
     [a]=1
@@ -875,6 +909,16 @@ checkFileset 'union ./c/a (fileFilter (file: assert file.name != "a"; true) ./.)
 # but here we need to use ./c
 checkFileset 'union (fileFilter (file: assert file.name != "a"; true) ./.) ./c'
 
+# Make sure single files are filtered correctly
+tree=(
+    [a]=1
+)
+checkFileset 'fileFilter (file: assert file.name == "a"; true) ./a'
+tree=(
+    [a]=0
+)
+checkFileset 'fileFilter (file: assert file.name == "a"; false) ./a'
+
 ## Tracing
 
 # The second trace argument is returned
@@ -977,6 +1021,15 @@ expectTrace 'unions [
 - foo (regular)'
 rm -rf -- *
 
+# Trying to trace a non-existent file as a file set doesn't work
+expectFailure 'trace ./a null' 'lib.fileset.trace: Argument \('"$work"'/a\) is a path that does not exist.'
+
+# Tracing a single existing file shows the parent directory
+# Mirroring how it would have to be added into the store
+touch a
+expectTrace './a' "$work (all files in directory)"
+rm  -rf -- *
+
 # For recursively included directories,
 # `(all files in directory)` should only be used if there's at least one file (otherwise it would be `(empty)`)
 # and this should be determined without doing a full search
@@ -1023,13 +1076,18 @@ rm -rf -- *
 ## lib.fileset.fromSource
 
 # Check error messages
-expectFailure 'fromSource null' 'lib.fileset.fromSource: The source origin of the argument is of type null, but it should be a path instead.'
 
+# String-like values are not supported
 expectFailure 'fromSource (lib.cleanSource "")' 'lib.fileset.fromSource: The source origin of the argument is a string-like value \(""\), but it should be a path instead.
 \s*Sources created from paths in strings cannot be turned into file sets, use `lib.sources` or derivations instead.'
 
+# Wrong type
+expectFailure 'fromSource null' 'lib.fileset.fromSource: The source origin of the argument is of type null, but it should be a path instead.'
 expectFailure 'fromSource (lib.cleanSource null)' 'lib.fileset.fromSource: The source origin of the argument is of type null, but it should be a path instead.'
 
+# fromSource on non-existent paths gives an error
+expectFailure 'fromSource ./a' 'lib.fileset.fromSource: The source origin \('"$work"'/a\) of the argument is a path that does not exist.'
+
 # fromSource on a path works and is the same as coercing that path
 mkdir a
 touch a/b c