Merge pull request #267384 from tweag/fileset.fileFilter-path2

`fileset.fileFilter`: Restrict second argument to paths

lib/fileset/README.md

@@ -238,6 +238,21 @@ Arguments:
   And it would be unclear how the library should behave if the one file wouldn't be added to the store:
   `toSource { root = ./file.nix; fileset = <empty>; }` has no reasonable result because returing an empty store path wouldn't match the file type, and there's no way to have an empty file store path, whatever that would mean.
 
+### `fileFilter` takes a path
+
+The `fileFilter` function takes a path, and not a file set, as its second argument.
+
+- (-) Makes it harder to compose functions, since the file set type, the return value, can't be passed to the function itself like `fileFilter predicate fileset`
+  - (+) It's still possible to use `intersection` to filter on file sets: `intersection fileset (fileFilter predicate ./.)`
+    - (-) This does need an extra `./.` argument that's not obvious
+      - (+) This could always be `/.` or the project directory, `intersection` will make it lazy
+- (+) In the future this will allow `fileFilter` to support a predicate property like `subpath` and/or `components` in a reproducible way.
+  This wouldn't be possible if it took a file set, because file sets don't have a predictable absolute path.
+  - (-) What about the base path?
+    - (+) That can change depending on which files are included, so if it's used for `fileFilter`
+      it would change the `subpath`/`components` value depending on which files are included.
+- (+) If necessary, this restriction can be relaxed later, the opposite wouldn't be possible
+
 ## To update in the future
 
 Here's a list of places in the library that need to be updated in the future:

lib/fileset/default.nix

@@ -366,7 +366,7 @@ in {
           type :: String,
           ...
         } -> Bool)
-        -> FileSet
+        -> Path
         -> FileSet
 
     Example:
@@ -397,14 +397,24 @@ in {
       Other attributes may be added in the future.
     */
     predicate:
-    # The file set to filter based on the predicate function
-    fileset:
+    # The path whose files to filter
+    path:
     if ! isFunction predicate then
       throw ''
         lib.fileset.fileFilter: First argument is of type ${typeOf predicate}, but it should be a function instead.''
+    else if ! isPath path then
+      if path._type or "" == "fileset" then
+        throw ''
+          lib.fileset.fileFilter: Second argument is a file set, but it should be a path instead.
+              If you need to filter files in a file set, use `intersection fileset (fileFilter pred ./.)` instead.''
+      else
+        throw ''
+          lib.fileset.fileFilter: Second argument is of type ${typeOf path}, but it should be a path instead.''
+    else if ! pathExists path then
+      throw ''
+        lib.fileset.fileFilter: Second argument (${toString path}) is a path that does not exist.''
     else
-      _fileFilter predicate
-        (_coerce "lib.fileset.fileFilter: Second argument" fileset);
+      _fileFilter predicate path;
 
   /*
     The file set containing all files that are in both of two given file sets.

lib/fileset/internal.nix

@@ -786,9 +786,9 @@ 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:
+  # Filters all files in a path based on a predicate
+  # Type: ({ name, type, ... } -> Bool) -> Path -> FileSet
+  _fileFilter = predicate: root:
     let
       # Check the predicate for a single file
       # Type: String -> String -> filesetTree
@@ -807,19 +807,22 @@ rec {
 
       # Check the predicate for all files in a directory
       # Type: Path -> filesetTree
-      fromDir = path: tree:
-        mapAttrs (name: subtree:
-          if isAttrs subtree || subtree == "directory" then
-            fromDir (path + "/${name}") subtree
-          else if subtree == null then
-            null
+      fromDir = path:
+        mapAttrs (name: type:
+          if type == "directory" then
+            fromDir (path + "/${name}")
           else
-            fromFile name subtree
-        ) (_directoryEntries path tree);
+            fromFile name type
+        ) (readDir path);
+
+      rootType = pathType root;
     in
-    if fileset._internalIsEmptyWithoutBase then
-      _emptyWithoutBase
+    if rootType == "directory" then
+      _create root (fromDir root)
     else
-      _create fileset._internalBase
-        (fromDir fileset._internalBase fileset._internalTree);
+      # Single files are turned into a directory containing that file or nothing.
+      _create (dirOf root) {
+        ${baseNameOf root} =
+          fromFile (baseNameOf root) rootType;
+      };
 }

lib/fileset/tests.sh

@@ -813,14 +813,15 @@ checkFileset 'difference ./. ./b'
 # 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.'
+# The second argument needs to be an existing path
+expectFailure 'fileFilter (file: abort "this is not needed") _emptyWithoutBase' 'lib.fileset.fileFilter: Second argument is a file set, but it should be a path instead.
+\s*If you need to filter files in a file set, use `intersection fileset \(fileFilter pred \./\.\)` instead.'
+expectFailure 'fileFilter (file: abort "this is not needed") null' 'lib.fileset.fileFilter: Second argument is of type null, but it should be 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") ./.'
-checkFileset 'fileFilter (file: abort "this is not needed") _emptyWithoutBase'
 
 # The predicate must be able to handle extra attributes
 touch a
@@ -882,14 +883,6 @@ 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