Add an explicit error when trying to expand a TreeArtifact outside …

…of inputs list in `Args.add_{all,joined}`

This is an analogue of b7590a0 for `TreeArtifact`s.

Previously, when users forgot to add a `TreeArtifact` (aka a "directory") to the inputs of an action, `Args` would silently expand the contents to the empty set and thus (usually) not add anything to the command line. This made the mistake difficult to track down.

Fixes bazelbuild#6487
Fixes bazelbuild#11811
Fixes bazelbuild#20635

Closes bazelbuild#23136.

PiperOrigin-RevId: 665926996
Change-Id: Id80262fc401da80aab05aadc978886e89cd1420e

src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java

@@ -139,7 +139,7 @@ public static List<ActionInput> expandArtifacts(
         }
       } else if (artifact.isTreeArtifact()) {
         ImmutableSortedSet<TreeFileArtifact> children =
-            artifactExpander.expandTreeArtifact(artifact);
+            artifactExpander.tryExpandTreeArtifact(artifact);
         if (children.isEmpty()) {
           emptyTreeArtifacts.add(artifact);
         } else {

src/main/java/com/google/devtools/build/lib/actions/ArtifactExpander.java

@@ -24,12 +24,27 @@
 /** Expands tree artifacts and filesets. */
 public interface ArtifactExpander {
 
+  /**
+   * Returns the expansion of the given {@linkplain SpecialArtifactType#TREE tree artifact}.
+   *
+   * @throws MissingExpansionException if this expander does not have data for the given tree
+   *     artifact
+   */
+  ImmutableSortedSet<TreeFileArtifact> expandTreeArtifact(Artifact treeArtifact)
+      throws MissingExpansionException;
+
   /**
    * Returns the expansion of the given {@linkplain SpecialArtifactType#TREE tree artifact}.
    *
    * <p>If this expander does not have data for the given tree artifact, returns an empty set.
    */
-  ImmutableSortedSet<TreeFileArtifact> expandTreeArtifact(Artifact treeArtifact);
+  default ImmutableSortedSet<TreeFileArtifact> tryExpandTreeArtifact(Artifact treeArtifact) {
+    try {
+      return expandTreeArtifact(treeArtifact);
+    } catch (MissingExpansionException e) {
+      return ImmutableSortedSet.of();
+    }
+  }
 
   /**
    * Returns the expansion of the given {@linkplain SpecialArtifactType#FILESET fileset artifact}.

src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java

@@ -692,7 +692,7 @@ private static final class ExpandedTreeArtifactArg extends TreeArtifactExpansion
 
     @Override
     void eval(ImmutableList.Builder<String> builder, ArtifactExpander artifactExpander) {
-      for (TreeFileArtifact child : artifactExpander.expandTreeArtifact(treeArtifact)) {
+      for (TreeFileArtifact child : artifactExpander.tryExpandTreeArtifact(treeArtifact)) {
         builder.add(child.getExecPathString());
       }
     }

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java

@@ -406,7 +406,7 @@ private static boolean hasDirectory(List<Object> originalValues) {
     }
 
     private static boolean isDirectory(Object object) {
-      return object instanceof Artifact && ((Artifact) object).isDirectory();
+      return object instanceof Artifact artifact && artifact.isDirectory();
     }
 
     private static List<Object> expandDirectories(
@@ -417,7 +417,18 @@ private static List<Object> expandDirectories(
         if (isDirectory(object)) {
           Artifact artifact = (Artifact) object;
           if (artifact.isTreeArtifact()) {
-            expandedValues.addAll(artifactExpander.expandTreeArtifact(artifact));
+            try {
+              expandedValues.addAll(artifactExpander.expandTreeArtifact(artifact));
+            } catch (MissingExpansionException e) {
+              throw new CommandLineExpansionException(
+                  String.format(
+                      "Failed to expand directory %s. Either add the directory as an input of the"
+                          + " action or set 'expand_directories = False' in the 'add_all' or"
+                          + " 'add_joined' call to have the path of the directory added to the"
+                          + " command line instead of its contents.",
+                      Starlark.repr(artifact)),
+                  e);
+            }
           } else if (artifact.isFileset()) {
             expandFileset(artifactExpander, artifact, expandedValues, pathMapper);
           } else {
@@ -1024,10 +1035,17 @@ private static final class FullExpander implements DirectoryExpander {
     }
 
     @Override
-    public ImmutableList<FileApi> list(FileApi file) {
+    public ImmutableList<FileApi> list(FileApi file) throws EvalException {
       Artifact artifact = (Artifact) file;
       if (artifact.isTreeArtifact()) {
-        return ImmutableList.copyOf(expander.expandTreeArtifact(artifact));
+        try {
+          return ImmutableList.copyOf(expander.expandTreeArtifact(artifact));
+        } catch (MissingExpansionException unused) {
+          throw Starlark.errorf(
+              "Failed to expand directory %s. Only directories that are action inputs can be"
+                  + " expanded.",
+              Starlark.repr(artifact));
+        }
       } else {
         return ImmutableList.of(file);
       }
@@ -1069,7 +1087,7 @@ private static void applyMapEach(
       }
       for (int i = 0; i < count; ++i) {
         args.set(0, originalValues.get(i));
-        Object ret = Starlark.call(thread, mapFn, args, /*kwargs=*/ ImmutableMap.of());
+        Object ret = Starlark.call(thread, mapFn, args, /* kwargs= */ ImmutableMap.of());
         if (ret instanceof String string) {
           consumer.accept(string);
         } else if (ret instanceof Sequence<?> sequence) {
@@ -1102,6 +1120,7 @@ private static class CommandLineItemMapEachAdaptor
     private final StarlarkCallable mapFn;
     private final Location location;
     private final StarlarkSemantics starlarkSemantics;
+
     /**
      * Indicates whether artifactExpander was provided on construction. This is used to distinguish
      * the case where it's not provided from the case where it was provided but subsequently

src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java

@@ -926,7 +926,7 @@ public VariableValue getFieldValue(
             if (objectFile.isTreeArtifact() && expander != null) {
               expandedObjectFiles.addAll(
                   Collections2.transform(
-                      expander.expandTreeArtifact(objectFile), Artifact::getExecPathString));
+                      expander.tryExpandTreeArtifact(objectFile), Artifact::getExecPathString));
             } else {
               expandedObjectFiles.add(objectFile.getExecPathString());
             }

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -1026,7 +1026,7 @@ public void validateInclusions(
       for (Artifact input : set.toList()) {
         if (input.isTreeArtifact()) {
           allowedIncludes.addAll(
-              actionExecutionContext.getArtifactExpander().expandTreeArtifact(input));
+              actionExecutionContext.getArtifactExpander().tryExpandTreeArtifact(input));
         }
         allowedIncludes.add(input);
       }

src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java

@@ -224,7 +224,7 @@ private static Iterable<Artifact> expandedHeaders(ArtifactExpander artifactExpan
     List<Artifact> expandedHeaders = new ArrayList<>();
     for (Artifact unexpandedHeader : unexpandedHeaders) {
       if (unexpandedHeader.isTreeArtifact()) {
-        expandedHeaders.addAll(artifactExpander.expandTreeArtifact(unexpandedHeader));
+        expandedHeaders.addAll(artifactExpander.tryExpandTreeArtifact(unexpandedHeader));
       } else {
         expandedHeaders.add(unexpandedHeader);
       }

src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java

@@ -83,7 +83,7 @@ private static Iterable<Artifact> expandedHeaders(ArtifactExpander artifactExpan
     List<Artifact> expandedHeaders = new ArrayList<>();
     for (Artifact unexpandedHeader : unexpandedHeaders) {
       if (unexpandedHeader.isTreeArtifact()) {
-        expandedHeaders.addAll(artifactExpander.expandTreeArtifact(unexpandedHeader));
+        expandedHeaders.addAll(artifactExpander.tryExpandTreeArtifact(unexpandedHeader));
       } else {
         expandedHeaders.add(unexpandedHeader);
       }

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

@@ -970,10 +970,14 @@ private static final class ActionInputArtifactExpander implements ArtifactExpand
     }
 
     @Override
-    public ImmutableSortedSet<TreeFileArtifact> expandTreeArtifact(Artifact treeArtifact) {
+    public ImmutableSortedSet<TreeFileArtifact> expandTreeArtifact(Artifact treeArtifact)
+        throws MissingExpansionException {
       checkArgument(treeArtifact.isTreeArtifact(), treeArtifact);
       TreeArtifactValue tree = inputArtifactData.getTreeMetadata(treeArtifact.getExecPath());
-      return tree == null ? ImmutableSortedSet.of() : tree.getChildren();
+      if (tree == null) {
+        throw new MissingExpansionException("Missing expansion for tree artifact: " + treeArtifact);
+      }
+      return tree.getChildren();
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/DirectoryExpander.java

@@ -19,6 +19,7 @@
 import net.starlark.java.annot.Param;
 import net.starlark.java.annot.StarlarkBuiltin;
 import net.starlark.java.annot.StarlarkMethod;
+import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.StarlarkValue;
 
 /** A class that can be used to expand directories at execution time. */
@@ -44,5 +45,5 @@ public interface DirectoryExpander extends StarlarkValue {
             named = false,
             doc = "The directory or file to expand."),
       })
-  ImmutableList<FileApi> list(FileApi artifact);
+  ImmutableList<FileApi> list(FileApi artifact) throws EvalException;
 }

src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java

@@ -37,15 +37,24 @@
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine.VectorArg;
+import com.google.devtools.build.lib.cmdline.BazelModuleContext;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
+import net.starlark.java.eval.Module;
+import net.starlark.java.eval.Mutability;
+import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkFunction;
 import net.starlark.java.eval.StarlarkSemantics;
+import net.starlark.java.eval.StarlarkThread;
 import net.starlark.java.eval.Tuple;
+import net.starlark.java.syntax.FileOptions;
 import net.starlark.java.syntax.Location;
+import net.starlark.java.syntax.ParserInput;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -54,8 +63,10 @@
 /** Tests for {@link StarlarkCustomCommandLine}. */
 @RunWith(JUnit4.class)
 public final class StarlarkCustomCommandLineTest {
-
-  private static final ArtifactExpander EMPTY_EXPANDER = artifact -> ImmutableSortedSet.of();
+  private static final ArtifactExpander EMPTY_EXPANDER =
+      artifact -> {
+        throw new ArtifactExpander.MissingExpansionException("Missing expansion for " + artifact);
+      };
 
   private final Scratch scratch = new Scratch();
   private Path execRoot;
@@ -197,21 +208,18 @@ public void flagPerLine() throws Exception {
   }
 
   @Test
-  public void vectorArgAddToFingerprint_treeArtifactMissingExpansion_returnsDigest()
-      throws Exception {
+  public void vectorArgAddToFingerprint_treeArtifactMissingExpansion_fails() {
     SpecialArtifact tree = createTreeArtifact("tree");
     CommandLine commandLine =
         builder
             .add(vectorArg(tree).setExpandDirectories(true))
             .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK);
-    ActionKeyContext actionKeyContext = new ActionKeyContext();
-    Fingerprint fingerprint = new Fingerprint();
-
-    // TODO(b/167696101): Fail arguments computation when we are missing the directory from inputs.
-    commandLine.addToFingerprint(
-        actionKeyContext, EMPTY_EXPANDER, CoreOptions.OutputPathsMode.OFF, fingerprint);
 
-    assertThat(fingerprint.digestAndReset()).isNotEmpty();
+    CommandLineExpansionException e =
+        assertThrows(
+            CommandLineExpansionException.class,
+            () -> commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP));
+    assertThat(e).hasMessageThat().contains("Failed to expand directory <generated file tree>");
   }
 
   @Test
@@ -227,7 +235,7 @@ public void vectorArgAddToFingerprint_expandFileset_includesInDigest() throws Ex
     Fingerprint fingerprint = new Fingerprint();
     ArtifactExpander artifactExpander =
         createArtifactExpander(
-            /*treeExpansions=*/ ImmutableMap.of(),
+            /* treeExpansions= */ ImmutableMap.of(),
             ImmutableMap.of(fileset, ImmutableList.of(symlink1, symlink2)));
 
     commandLine.addToFingerprint(
@@ -304,7 +312,7 @@ public void vectorArgArguments_expandsFileset() throws Exception {
     FilesetOutputSymlink symlink2 = createFilesetSymlink("file2");
     ArtifactExpander artifactExpander =
         createArtifactExpander(
-            /*treeExpansions=*/ ImmutableMap.of(),
+            /* treeExpansions= */ ImmutableMap.of(),
             ImmutableMap.of(fileset, ImmutableList.of(symlink1, symlink2)));
 
     Iterable<String> arguments = commandLine.arguments(artifactExpander, PathMapper.NOOP);
@@ -313,15 +321,42 @@ public void vectorArgArguments_expandsFileset() throws Exception {
   }
 
   @Test
-  public void vectorArgArguments_treeArtifactMissingExpansion_returnsEmptyList() throws Exception {
+  public void vectorArgArguments_treeArtifactMissingExpansion_fails() {
     SpecialArtifact tree = createTreeArtifact("tree");
     CommandLine commandLine =
         builder
             .add(vectorArg(tree).setExpandDirectories(true))
             .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK);
 
-    // TODO(b/167696101): Fail arguments computation when we are missing the directory from inputs.
-    assertThat(commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP)).isEmpty();
+    assertThrows(
+        CommandLineExpansionException.class,
+        () -> commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP));
+  }
+
+  @Test
+  public void vectorArgArguments_manuallyExpandedTreeArtifactMissingExpansion_fails()
+      throws Exception {
+    SpecialArtifact tree = createTreeArtifact("tree");
+    CommandLine commandLine =
+        builder
+            .add(
+                vectorArg(tree)
+                    .setExpandDirectories(false)
+                    .setMapEach(
+                        (StarlarkFunction)
+                            execStarlark(
+                                """
+                                def map_each(x, expander):
+                                  expander.expand(x)
+                                map_each
+                                """)))
+            .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK);
+
+    CommandLineExpansionException e =
+        assertThrows(
+            CommandLineExpansionException.class,
+            () -> commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP));
+    assertThat(e).hasMessageThat().contains("Failed to expand directory <generated file tree>");
   }
 
   @Test
@@ -379,9 +414,14 @@ private static ArtifactExpander createArtifactExpander(
       ImmutableMap<SpecialArtifact, ImmutableList<FilesetOutputSymlink>> filesetExpansions) {
     return new ArtifactExpander() {
       @Override
-      public ImmutableSortedSet<TreeFileArtifact> expandTreeArtifact(Artifact treeArtifact) {
+      public ImmutableSortedSet<TreeFileArtifact> expandTreeArtifact(Artifact treeArtifact)
+          throws MissingExpansionException {
         //noinspection SuspiciousMethodCalls
-        return treeExpansions.getOrDefault(treeArtifact, ImmutableSortedSet.of());
+        ImmutableSortedSet<TreeFileArtifact> expansion = treeExpansions.get(treeArtifact);
+        if (expansion == null) {
+          throw new MissingExpansionException("Cannot expand " + treeArtifact);
+        }
+        return expansion;
       }
 
       @Override
@@ -396,4 +436,23 @@ public ImmutableList<FilesetOutputSymlink> expandFileset(Artifact artifact)
       }
     };
   }
+
+  private static Object execStarlark(String code) throws Exception {
+    try (Mutability mutability = Mutability.create("test")) {
+      StarlarkThread thread = StarlarkThread.createTransient(mutability, StarlarkSemantics.DEFAULT);
+      return Starlark.execFile(
+          ParserInput.fromString(code, "test/label.bzl"),
+          FileOptions.DEFAULT,
+          Module.withPredeclaredAndData(
+              StarlarkSemantics.DEFAULT,
+              ImmutableMap.of(),
+              BazelModuleContext.create(
+                  Label.parseCanonicalUnchecked("//test:label"),
+                  RepositoryMapping.ALWAYS_FALLBACK,
+                  "test/label.bzl",
+                  /* loads= */ ImmutableList.of(),
+                  /* bzlTransitiveDigest= */ new byte[0])),
+          thread);
+    }
+  }
 }

src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java

@@ -1020,7 +1020,8 @@ private static final class CopyTreeAction extends SimpleTestAction {
 
     @Override
     void run(ActionExecutionContext context) throws IOException {
-      for (Artifact child : context.getArtifactExpander().expandTreeArtifact(getPrimaryInput())) {
+      for (Artifact child :
+          context.getArtifactExpander().tryExpandTreeArtifact(getPrimaryInput())) {
         Path newOutput = getPrimaryOutput().getPath().getRelative(child.getParentRelativePath());
         newOutput.createDirectoryAndParents();
         FileSystemUtils.copyFile(child.getPath(), newOutput);

src/test/java/com/google/devtools/build/lib/skyframe/rewinding/LostImportantOutputHandlerModule.java

@@ -163,7 +163,7 @@ private ImmutableList<OutputAndOwner> expand(
 
     private Stream<OutputAndOwner> expand(Artifact output, ArtifactExpander expander) {
       if (output.isTreeArtifact()) {
-        var children = expander.expandTreeArtifact(output).stream();
+        var children = expander.tryExpandTreeArtifact(output).stream();
         var archivedTreeArtifact = expander.getArchivedTreeArtifact(output);
         var expansion =
             archivedTreeArtifact == null

src/test/java/com/google/devtools/build/lib/testutil/SpawnInputUtils.java

@@ -99,7 +99,7 @@ public static SpecialArtifact getTreeArtifactWithName(Spawn spawn, String name)
 
   public static Artifact getExpandedToArtifact(
       String name, Artifact expandableArtifact, Spawn spawn, ActionExecutionContext context) {
-    return context.getArtifactExpander().expandTreeArtifact(expandableArtifact).stream()
+    return context.getArtifactExpander().tryExpandTreeArtifact(expandableArtifact).stream()
         .filter(artifact -> artifact.getExecPathString().contains(name))
         .findFirst()
         .orElseThrow(