CustomCommandLine.Builder: clean up its interface

In this commit:
- remove unused methods and classes
- turn CustomCommandLine.ArgvFragment into an
  interface
- remove the
  CustomCommandLine.TreeFileArtifactArgvFragment
  abstract class; it only had one remaining
  subclass
- add @Nullable annotations where nulls are fine
- add Precondition checks for non-nullable args
- simplify the interface by removing add* methods
  that can be composed of other add* methods; this
  makes it easier to see what the callers do with
  the Builder
- remove add* methods that add a single argument
  followed by a list of other elements (or a
  joined string of them); these had a bug in that
  they didn't check if the collection was empty
  (only that it was not null), and if it was empty
  then the single argument was still added though
  it was not followed by any value
- fix call sites of add* methods where we
  previously could have added a flag with an empty
  collection
- audit every affected call site

RELNOTES: none
PiperOrigin-RevId: 161957521

2 files changed, 112 insertions, 66 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java
index ea061beb2d..f7995b42d3 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java
@@ -18,6 +18,7 @@ import static org.junit.Assert.fail;
 
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
+import com.google.common.testing.NullPointerTester;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
@@ -32,7 +33,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
-import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -71,15 +71,18 @@ public class CustomCommandLineTest {
 
   @Test
   public void testStringsArgs() {
-    CustomCommandLine cl = CustomCommandLine.builder().add("--arg",
-        ImmutableList.of("a", "b")).build();
+    CustomCommandLine cl =
+        CustomCommandLine.builder().add("--arg").add(ImmutableList.of("a", "b")).build();
     assertThat(cl.arguments()).isEqualTo(ImmutableList.of("--arg", "a", "b"));
   }
 
   @Test
   public void testArtifactJoinStringArgs() {
-    CustomCommandLine cl = CustomCommandLine.builder().addJoinStrings("--path", ":",
-        ImmutableList.of("foo", "bar")).build();
+    CustomCommandLine cl =
+        CustomCommandLine.builder()
+            .add("--path")
+            .addJoinStrings(":", ImmutableList.of("foo", "bar"))
+            .build();
     assertThat(cl.arguments()).isEqualTo(ImmutableList.of("--path", "foo:bar"));
   }
 
@@ -87,17 +90,8 @@ public class CustomCommandLineTest {
   public void testJoinValues() {
     CustomCommandLine cl =
         CustomCommandLine.builder()
-            .addJoinValues(
-                "--path",
-                ":",
-                ImmutableList.of("foo", "bar", "baz"),
-                new Function<String, String>() {
-                  @Nullable
-                  @Override
-                  public String apply(@Nullable String s) {
-                    return s.toUpperCase();
-                  }
-                })
+            .add("--path")
+            .addJoinValues(":", ImmutableList.of("foo", "bar", "baz"), String::toUpperCase)
             .build();
     assertThat(cl.arguments()).isEqualTo(ImmutableList.of("--path", "FOO:BAR:BAZ"));
   }
@@ -110,8 +104,11 @@ public class CustomCommandLineTest {
 
   @Test
   public void testArtifactExecPathsArgs() {
-    CustomCommandLine cl = CustomCommandLine.builder().addExecPaths("--path",
-        ImmutableList.of(artifact1, artifact2)).build();
+    CustomCommandLine cl =
+        CustomCommandLine.builder()
+            .add("--path")
+            .addExecPaths(ImmutableList.of(artifact1, artifact2))
+            .build();
     assertThat(cl.arguments())
         .isEqualTo(ImmutableList.of("--path", "dir/file1.txt", "dir/file2.txt"));
   }
@@ -125,8 +122,11 @@ public class CustomCommandLineTest {
 
   @Test
   public void testArtifactJoinExecPathArgs() {
-    CustomCommandLine cl = CustomCommandLine.builder().addJoinExecPaths("--path", ":",
-        ImmutableList.of(artifact1, artifact2)).build();
+    CustomCommandLine cl =
+        CustomCommandLine.builder()
+            .add("--path")
+            .addJoinExecPaths(":", ImmutableList.of(artifact1, artifact2))
+            .build();
     assertThat(cl.arguments()).isEqualTo(ImmutableList.of("--path", "dir/file1.txt:dir/file2.txt"));
   }
 
@@ -137,13 +137,6 @@ public class CustomCommandLineTest {
   }
 
   @Test
-  public void testJoinPathArgs() {
-    CustomCommandLine cl = CustomCommandLine.builder().addJoinPaths(":",
-        ImmutableList.of(artifact1.getExecPath(), artifact2.getExecPath())).build();
-    assertThat(cl.arguments()).isEqualTo(ImmutableList.of("dir/file1.txt:dir/file2.txt"));
-  }
-
-  @Test
   public void testPathsArgs() {
     CustomCommandLine cl = CustomCommandLine.builder().addPaths("%s:%s",
         artifact1.getExecPath(), artifact1.getRootRelativePath()).build();
@@ -174,12 +167,15 @@ public class CustomCommandLineTest {
 
   @Test
   public void testCombinedArgs() {
-    CustomCommandLine cl = CustomCommandLine.builder()
-        .add("--arg")
-        .add("--args", ImmutableList.of("abc"))
-        .addExecPaths("--path1", ImmutableList.of(artifact1))
-        .addExecPath("--path2", artifact2)
-        .build();
+    CustomCommandLine cl =
+        CustomCommandLine.builder()
+            .add("--arg")
+            .add("--args")
+            .add(ImmutableList.of("abc"))
+            .add("--path1")
+            .addExecPaths(ImmutableList.of(artifact1))
+            .addExecPath("--path2", artifact2)
+            .build();
     assertThat(cl.arguments())
         .isEqualTo(
             ImmutableList.of(
@@ -187,13 +183,73 @@ public class CustomCommandLineTest {
   }
 
   @Test
-  public void testAddNulls() {
-    CustomCommandLine cl = CustomCommandLine.builder()
-        .add("--args", null)
-        .addExecPaths(null, ImmutableList.of(artifact1))
-        .addExecPath(null, null)
-        .build();
+  public void testAddNulls() throws Exception {
+    Artifact treeArtifact = createTreeArtifact("myTreeArtifact");
+    assertThat(treeArtifact).isNotNull();
+
+    CustomCommandLine cl =
+        CustomCommandLine.builder()
+            .add((CharSequence) null)
+            .add((Label) null)
+            .add((Iterable<String>) null)
+            .add(ImmutableList.<String>of())
+            .addExecPaths(null)
+            .addExecPaths(ImmutableList.of())
+            .addExecPath("foo", null)
+            .addPlaceholderTreeArtifactExecPath(null)
+            .addJoinStrings("foo", null)
+            .addJoinStrings("foo", ImmutableList.of())
+            .addJoinValues("foo", null, String::toString)
+            .addJoinValues("foo", ImmutableList.of(), String::toString)
+            .addJoinExecPaths("foo", null)
+            .addJoinExecPaths("foo", ImmutableList.of())
+            .addPath(null)
+            .addPaths("foo")
+            .addPaths("foo", (PathFragment[]) null)
+            .addPaths("foo", new PathFragment[0])
+            .addBeforeEachPath("foo", null)
+            .addBeforeEachPath("foo", ImmutableList.of())
+            .addBeforeEach("foo", null)
+            .addBeforeEach("foo", ImmutableList.of())
+            .addBeforeEachExecPath("foo", null)
+            .addBeforeEachExecPath("foo", ImmutableList.of())
+            .addFormatEach("%s", null)
+            .addFormatEach("%s", ImmutableList.of())
+            .add((CustomArgv) null)
+            .add((CustomMultiArgv) null)
+            .build();
     assertThat(cl.arguments()).isEqualTo(ImmutableList.of());
+
+    CustomCommandLine.Builder obj = CustomCommandLine.builder();
+    Class<CustomCommandLine.Builder> clazz = CustomCommandLine.Builder.class;
+    NullPointerTester npt =
+        new NullPointerTester()
+            .setDefault(Artifact.class, artifact1)
+            .setDefault(String.class, "foo")
+            .setDefault(PathFragment[].class, new PathFragment[] {PathFragment.create("foo")});
+
+    npt.testMethod(obj, clazz.getMethod("addExecPath", String.class, Artifact.class));
+    npt.testMethod(obj, clazz.getMethod("addParamFile", String.class, Artifact.class));
+    npt.testMethod(obj, clazz.getMethod("addPaths", String.class, PathFragment[].class));
+
+    npt.setDefault(Iterable.class, ImmutableList.of("foo"));
+    npt.testMethod(obj, clazz.getMethod("addJoinStrings", String.class, Iterable.class));
+    npt.testMethod(
+        obj, clazz.getMethod("addJoinValues", String.class, Iterable.class, Function.class));
+    npt.testMethod(obj, clazz.getMethod("addBeforeEach", String.class, Iterable.class));
+    npt.testMethod(obj, clazz.getMethod("addFormatEach", String.class, Iterable.class));
+
+    npt.setDefault(Iterable.class, ImmutableList.of(artifact1));
+    npt.testMethod(obj, clazz.getMethod("addJoinExecPaths", String.class, Iterable.class));
+    npt.testMethod(obj, clazz.getMethod("addBeforeEachExecPath", String.class, Iterable.class));
+
+    npt.setDefault(Iterable.class, ImmutableList.of(PathFragment.create("foo")));
+    npt.testMethod(obj, clazz.getMethod("addBeforeEachPath", String.class, Iterable.class));
+
+    npt.setDefault(Artifact.class, treeArtifact);
+    npt.testMethod(
+        obj, clazz.getMethod("addJoinExpandedTreeArtifactExecPath", String.class, Artifact.class));
+    npt.testMethod(obj, clazz.getMethod("addExpandedTreeArtifactExecPaths", Artifact.class));
   }
 
   @Test
@@ -201,10 +257,13 @@ public class CustomCommandLineTest {
     Artifact treeArtifactOne = createTreeArtifact("myArtifact/treeArtifact1");
     Artifact treeArtifactTwo = createTreeArtifact("myArtifact/treeArtifact2");
 
-    CustomCommandLine commandLineTemplate = CustomCommandLine.builder()
-        .addPlaceholderTreeArtifactExecPath("--argOne", treeArtifactOne)
-        .addPlaceholderTreeArtifactExecPath("--argTwo", treeArtifactTwo)
-        .build();
+    CustomCommandLine commandLineTemplate =
+        CustomCommandLine.builder()
+            .add("--argOne")
+            .addPlaceholderTreeArtifactExecPath(treeArtifactOne)
+            .add("--argTwo")
+            .addPlaceholderTreeArtifactExecPath(treeArtifactTwo)
+            .build();
 
     TreeFileArtifact treeFileArtifactOne = createTreeFileArtifact(
         treeArtifactOne, "children/child1");
@@ -224,32 +283,17 @@ public class CustomCommandLineTest {
   }
 
   @Test
-  public void testTreeFileArtifactExecPathWithTemplateArgs() {
-    Artifact treeArtifact = createTreeArtifact("myArtifact/treeArtifact1");
-
-    CustomCommandLine commandLineTemplate = CustomCommandLine.builder()
-        .addPlaceholderTreeArtifactFormattedExecPath("path:%s", treeArtifact)
-        .build();
-
-    TreeFileArtifact treeFileArtifact = createTreeFileArtifact(
-        treeArtifact, "children/child1");
-
-    CustomCommandLine commandLine = commandLineTemplate.evaluateTreeFileArtifacts(
-        ImmutableList.of(treeFileArtifact));
-
-    assertThat(commandLine.arguments()).containsExactly(
-        "path:myArtifact/treeArtifact1/children/child1");
-  }
-
-  @Test
   public void testTreeFileArtifactArgThrowWithoutSubstitution() {
     Artifact treeArtifactOne = createTreeArtifact("myArtifact/treeArtifact1");
     Artifact treeArtifactTwo = createTreeArtifact("myArtifact/treeArtifact2");
 
-    CustomCommandLine commandLineTemplate = CustomCommandLine.builder()
-        .addPlaceholderTreeArtifactExecPath("--argOne", treeArtifactOne)
-        .addPlaceholderTreeArtifactExecPath("--argTwo", treeArtifactTwo)
-        .build();
+    CustomCommandLine commandLineTemplate =
+        CustomCommandLine.builder()
+            .add("--argOne")
+            .addPlaceholderTreeArtifactExecPath(treeArtifactOne)
+            .add("--argTwo")
+            .addPlaceholderTreeArtifactExecPath(treeArtifactTwo)
+            .build();
 
     try {
       commandLineTemplate.arguments();
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java
index 8711b47a4b..6e5de2fce5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java
@@ -132,7 +132,9 @@ public class ParamFileWriteActionTest extends BuildViewTestCase {
     return CustomCommandLine.builder()
         .add("--flag1")
         .add("--flag2")
-        .add("--flag3", ImmutableList.of("value1", "value2"))
+        .add("--flag3")
+        .add("value1")
+        .add("value2")
         .build();
   }