Merge pull request #267381 from tweag/fileset.fileFilter-path

`fileset.fileFilter`: Don't run predicate unnecessarily

3 files changed, 54 insertions, 16 deletions

diff --git a/lib/fileset/default.nix b/lib/fileset/default.nix
index 7d5bbeee3ba..d90c770633d 100644
--- a/lib/fileset/default.nix
+++ b/lib/fileset/default.nix
@@ -380,7 +380,7 @@ in {
       fileFilter (file: hasPrefix "." file.name) ./.
 
       # Include all regular files (not symlinks or others) in the current directory
-      fileFilter (file: file.type == "regular")
+      fileFilter (file: file.type == "regular") ./.
   */
   fileFilter =
     /*
@@ -401,7 +401,7 @@ in {
     fileset:
     if ! isFunction predicate then
       throw ''
-        lib.fileset.fileFilter: First argument is of type ${typeOf predicate}, but it should be a function.''
+        lib.fileset.fileFilter: First argument is of type ${typeOf predicate}, but it should be a function instead.''
     else
       _fileFilter predicate
         (_coerce "lib.fileset.fileFilter: Second argument" fileset);
diff --git a/lib/fileset/internal.nix b/lib/fileset/internal.nix
index 717253f4571..b245caade1f 100644
--- a/lib/fileset/internal.nix
+++ b/lib/fileset/internal.nix
@@ -786,29 +786,40 @@ rec {
         _differenceTree (path + "/${name}") lhsValue (rhs.${name} or null)
       ) (_directoryEntries path lhs);
 
+  # Filters all files in a file set based on a predicate
+  # Type: ({ name, type, ... } -> Bool) -> FileSet -> FileSet
   _fileFilter = predicate: fileset:
     let
-      recurse = path: tree:
+      # Check the predicate for a single file
+      # Type: String -> String -> filesetTree
+      fromFile = name: type:
+        if
+          predicate {
+            inherit name type;
+            # To ensure forwards compatibility with more arguments being added in the future,
+            # adding an attribute which can't be deconstructed :)
+            "lib.fileset.fileFilter: The predicate function passed as the first argument must be able to handle extra attributes for future compatibility. If you're using `{ name, file }:`, use `{ name, file, ... }:` instead." = null;
+          }
+        then
+          type
+        else
+          null;
+
+      # Check the predicate for all files in a directory
+      # Type: Path -> filesetTree
+      fromDir = path: tree:
         mapAttrs (name: subtree:
           if isAttrs subtree || subtree == "directory" then
-            recurse (path + "/${name}") subtree
-          else if
-            predicate {
-              inherit name;
-              type = subtree;
-              # To ensure forwards compatibility with more arguments being added in the future,
-              # adding an attribute which can't be deconstructed :)
-              "lib.fileset.fileFilter: The predicate function passed as the first argument must be able to handle extra attributes for future compatibility. If you're using `{ name, file }:`, use `{ name, file, ... }:` instead." = null;
-            }
-          then
-            subtree
-          else
+            fromDir (path + "/${name}") subtree
+          else if subtree == null then
             null
+          else
+            fromFile name subtree
         ) (_directoryEntries path tree);
     in
     if fileset._internalIsEmptyWithoutBase then
       _emptyWithoutBase
     else
       _create fileset._internalBase
-        (recurse fileset._internalBase fileset._internalTree);
+        (fromDir fileset._internalBase fileset._internalTree);
 }
diff --git a/lib/fileset/tests.sh b/lib/fileset/tests.sh
index 796a03b52f0..5ef155d25a8 100755
--- a/lib/fileset/tests.sh
+++ b/lib/fileset/tests.sh
@@ -810,6 +810,13 @@ checkFileset 'difference ./. ./b'
 
 ## File filter
 
+# The first argument needs to be a function
+expectFailure 'fileFilter null (abort "this is not needed")' 'lib.fileset.fileFilter: First argument is of type null, but it should be a function instead.'
+
+# The second argument can be a file set or an existing path
+expectFailure 'fileFilter (file: abort "this is not needed") null' 'lib.fileset.fileFilter: Second argument is of type null, but it should be a file set or a path instead.'
+expectFailure 'fileFilter (file: abort "this is not needed") ./a' 'lib.fileset.fileFilter: Second argument \('"$work"'/a\) is a path that does not exist.'
+
 # The predicate is not called when there's no files
 tree=()
 checkFileset 'fileFilter (file: abort "this is not needed") ./.'
@@ -875,6 +882,26 @@ 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'
 
+# Also lazy, the filter isn't called on a filtered out path
+tree=(
+    [a]=1
+    [b]=0
+    [c]=0
+)
+checkFileset 'fileFilter (file: assert file.name != "c"; file.name == "a") (difference ./. ./c)'
+
+# Make sure single files are filtered correctly
+tree=(
+    [a]=1
+    [b]=0
+)
+checkFileset 'fileFilter (file: assert file.name == "a"; true) ./a'
+tree=(
+    [a]=0
+    [b]=0
+)
+checkFileset 'fileFilter (file: assert file.name == "a"; false) ./a'
+
 ## Tracing
 
 # The second trace argument is returned