Remove implicit iteration from Crosstool

Up until now we allowed implicit iteration, e.g.:

flag_group { flag: '%{some_sequence_variable}' }

From now on, snippet above will raise an error. We require explicit
'iterate_over' message, e.g.:

flag_group {
  iterate_over: 'some_sequence_variable'
  flag: '%{some_sequence_variable}'
}

RELNOTES: Implicit iteration in the CROSSTOOL has been removed, use explicit 'iterate_over' message.
PiperOrigin-RevId: 160871888

3 files changed, 144 insertions, 103 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
index 00ff1ca4ea..bd0bf1dfe0 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
@@ -76,9 +76,7 @@ public abstract class MockCcSupport {
           + "  }"
           + "}";
 
-  /**
-   * A feature configuration snippet useful for testing the layering check.
-   */
+  /** A feature configuration snippet useful for testing the layering check. */
   public static final String LAYERING_CHECK_FEATURE_CONFIGURATION =
       ""
           + "feature {"
@@ -90,6 +88,7 @@ public abstract class MockCcSupport {
           + "    action: 'c++-header-preprocessing'"
           + "    action: 'c++-module-compile'"
           + "    flag_group {"
+          + "      iterate_over: 'dependent_module_map_files'"
           + "      flag: 'dependent_module_map_file:%{dependent_module_map_files}'"
           + "    }"
           + "  }"
@@ -146,6 +145,7 @@ public abstract class MockCcSupport {
           + "    action: 'c++-header-preprocessing'"
           + "    action: 'c++-modules-compile'"
           + "    flag_group {"
+          + "      iterate_over: 'module_files'"
           + "      flag: 'module_file:%{module_files}'"
           + "    }"
           + "  }"
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
index 3beb3a6d13..9e804340d5 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
@@ -272,8 +272,8 @@ public class CcToolchainFeaturesTest {
     assertThat(getFlagParsingError("%{")).contains("expected variable name");
     assertThat(getFlagParsingError("%{}")).contains("expected variable name");
     assertThat(
-            getCommandLineForFlag(
-                "%{v}",
+            getCommandLineForFlagGroups(
+                "flag_group{ iterate_over: 'v' flag: '%{v}' }",
                 new Variables.Builder()
                     .addStringSequenceVariable("v", ImmutableList.<String>of())
                     .build()))
@@ -422,19 +422,6 @@ public class CcToolchainFeaturesTest {
         .containsExactly("-Afirst -Bfoo", "-Afirst -Bbar", "-Asecond -Bfoo", "-Asecond -Bbar");
   }
 
-  // <p>TODO(b/32655571): Get rid of this test once implicit iteration is not supported.
-  // It's there only to document a known limitation of the system.
-  @Test
-  public void testVariableLookupIsBrokenForImplicitStructFieldIteration() throws Exception {
-    assertThat(
-            getCommandLineForFlagGroups(
-                "flag_group { flag: '-A%{struct.sequence}' }",
-                createStructureVariables(
-                    "struct",
-                    new StructureBuilder().addField("sequence", ImmutableList.of("foo", "bar")))))
-        .containsExactly("-Afoo", "-Abar");
-  }
-
   @Test
   public void testExpandIfAllAvailableWithStructsExpandsIfPresent() throws Exception {
     assertThat(
@@ -660,18 +647,6 @@ public class CcToolchainFeaturesTest {
   }
 
   @Test
-  public void testLegacyListVariableExpansion() throws Exception {
-    assertThat(getCommandLineForFlag("%{v}", createVariables("v", "1", "v", "2")))
-        .containsExactly("1", "2");
-    assertThat(getCommandLineForFlag("%{v1} %{v2}",
-        createVariables("v1", "a1", "v1", "a2", "v2", "b")))
-        .containsExactly("a1 b", "a2 b");
-    assertThat(getFlagExpansionError("%{v1} %{v2}",
-        createVariables("v1", "a1", "v1", "a2", "v2", "b1", "v2", "b2")))
-        .contains("'v1' and 'v2'");
-  }
-
-  @Test
   public void testListVariableExpansion() throws Exception {
     assertThat(
             getCommandLineForFlagGroups(
@@ -715,42 +690,28 @@ public class CcToolchainFeaturesTest {
   }
 
   @Test
-  public void testListVariableExpansionMixedWithImplicitlyAccessedListVariableWithinFlagGroupWorks()
-      throws Exception {
+  public void testFlagGroupVariableExpansion() throws Exception {
     assertThat(
             getCommandLineForFlagGroups(
-                "flag_group {"
-                    + "  iterate_over: 'v1'"
-                    + "  flag_group {"
-                    + "    flag: '-A%{v1} -B%{v2}'"
-                    + "  }"
-                    + "}",
-                createVariables("v1", "a1", "v1", "a2", "v2", "b1", "v2", "b2")))
-        .containsExactly("-Aa1 -Bb1", "-Aa1 -Bb2", "-Aa2 -Bb1", "-Aa2 -Bb2");
-  }
-
-  @Test
-  public void testFlagGroupVariableExpansion() throws Exception {
-    assertThat(getCommandLineForFlagGroups(
-        "flag_group { flag: '-f' flag: '%{v}' } flag_group { flag: '-end' }",
-        createVariables("v", "1", "v", "2")))
+                ""
+                    + "flag_group { iterate_over: 'v' flag: '-f' flag: '%{v}' }"
+                    + "flag_group { flag: '-end' }",
+                createVariables("v", "1", "v", "2")))
         .containsExactly("-f", "1", "-f", "2", "-end");
-    assertThat(getCommandLineForFlagGroups(
-        "flag_group { flag: '-f' flag: '%{v}' } flag_group { flag: '%{v}' }",
-        createVariables("v", "1", "v", "2")))
+    assertThat(
+            getCommandLineForFlagGroups(
+                ""
+                    + "flag_group { iterate_over: 'v' flag: '-f' flag: '%{v}' }"
+                    + "flag_group { iterate_over: 'v' flag: '%{v}' }",
+                createVariables("v", "1", "v", "2")))
         .containsExactly("-f", "1", "-f", "2", "1", "2");
-    assertThat(getCommandLineForFlagGroups(
-        "flag_group { flag: '-f' flag: '%{v}' } flag_group { flag: '%{v}' }",
-        createVariables("v", "1", "v", "2")))
+    assertThat(
+            getCommandLineForFlagGroups(
+                ""
+                    + "flag_group { iterate_over: 'v' flag: '-f' flag: '%{v}' } "
+                    + "flag_group { iterate_over: 'v' flag: '%{v}' }",
+                createVariables("v", "1", "v", "2")))
         .containsExactly("-f", "1", "-f", "2", "1", "2");
-    try {
-      getCommandLineForFlagGroups(
-          "flag_group { flag: '%{v1}' flag: '%{v2}' }",
-          createVariables("v1", "1", "v1", "2", "v2", "1", "v2", "2"));
-      fail("Expected ExpansionException");
-    } catch (ExpansionException e) {
-      assertThat(e).hasMessageThat().contains("'v1' and 'v2'");
-    }
   }
 
   private VariableValueBuilder createNestedSequence(int depth, int count, String prefix) {
@@ -780,11 +741,11 @@ public class CcToolchainFeaturesTest {
   @Test
   public void testFlagTreeVariableExpansion() throws Exception {
     String nestedGroup =
-        "flag_group {"
+        ""
+            + "flag_group {"
+            + "  iterate_over: 'v'"
             + "  flag_group { flag: '-a' }"
-            + "  flag_group {"
-            + "    flag: '%{v}'"
-            + "  }"
+            + "  flag_group { iterate_over: 'v' flag: '%{v}' }"
             + "  flag_group { flag: '-b' }"
             + "}";
     assertThat(getCommandLineForFlagGroups(nestedGroup, createNestedVariables("v", 1, 3)))
@@ -792,9 +753,6 @@ public class CcToolchainFeaturesTest {
             "-a", "00", "01", "02", "-b", "-a", "10", "11", "12", "-b", "-a", "20", "21", "22",
             "-b");
 
-    assertThat(getCommandLineForFlagGroups(nestedGroup, createNestedVariables("v", 0, 3)))
-        .containsExactly("-a", "0", "-b", "-a", "1", "-b", "-a", "2", "-b");
-
     try {
       getCommandLineForFlagGroups(nestedGroup, createNestedVariables("v", 2, 3));
       fail("Expected ExpansionException");
@@ -993,7 +951,29 @@ public class CcToolchainFeaturesTest {
   }
 
   @Test
-  public void testSuppressionViaMissingBuildVariable() throws Exception {
+  public void testFlagSetWithMissingVariableIsNotExpanded() throws Exception {
+    FeatureConfiguration configuration =
+        buildFeatures(
+                "feature {",
+                "  name: 'a'",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     expand_if_all_available: 'v'",
+                "     flag_group { flag: '%{v}' }",
+                "  }",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     flag_group { flag: 'unconditional' }",
+                "  }",
+                "}")
+            .getFeatureConfiguration(assumptionsFor("a"));
+
+    assertThat(configuration.getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
+        .containsExactly("unconditional");
+  }
+
+  @Test
+  public void testOnlyFlagSetsWithAllVariablesPresentAreExpanded() throws Exception {
     FeatureConfiguration configuration =
         buildFeatures(
                 "feature {",
@@ -1016,16 +996,66 @@ public class CcToolchainFeaturesTest {
                 "}")
             .getFeatureConfiguration(assumptionsFor("a"));
 
-    assertThat(configuration.getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
-        .containsExactly("unconditional");
     assertThat(
             configuration.getCommandLine(CppCompileAction.CPP_COMPILE, createVariables("v", "1")))
         .containsExactly("1", "unconditional");
+  }
+
+  @Test
+  public void testOnlyInnerFlagSetIsIteratedWithSequenceVariable() throws Exception {
+    FeatureConfiguration configuration =
+        buildFeatures(
+                "feature {",
+                "  name: 'a'",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     expand_if_all_available: 'v'",
+                "     flag_group { iterate_over: 'v' flag: '%{v}' }",
+                "  }",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     expand_if_all_available: 'v'",
+                "     expand_if_all_available: 'w'",
+                "     flag_group { iterate_over: 'v' flag: '%{v}%{w}' }",
+                "  }",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     flag_group { flag: 'unconditional' }",
+                "  }",
+                "}")
+            .getFeatureConfiguration(assumptionsFor("a"));
+
     assertThat(
             configuration.getCommandLine(
                 CppCompileAction.CPP_COMPILE, createVariables("v", "1", "v", "2")))
         .containsExactly("1", "2", "unconditional")
         .inOrder();
+  }
+
+  @Test
+  public void testFlagSetsAreIteratedIndividuallyForSequenceVariables() throws Exception {
+    FeatureConfiguration configuration =
+        buildFeatures(
+                "feature {",
+                "  name: 'a'",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     expand_if_all_available: 'v'",
+                "     flag_group { iterate_over: 'v' flag: '%{v}' }",
+                "  }",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     expand_if_all_available: 'v'",
+                "     expand_if_all_available: 'w'",
+                "     flag_group { iterate_over: 'v' flag: '%{v}%{w}' }",
+                "  }",
+                "  flag_set {",
+                "     action: 'c++-compile'",
+                "     flag_group { flag: 'unconditional' }",
+                "  }",
+                "}")
+            .getFeatureConfiguration(assumptionsFor("a"));
+
     assertThat(
             configuration.getCommandLine(
                 CppCompileAction.CPP_COMPILE, createVariables("v", "1", "v", "2", "w", "3")))
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
index 7b28ff2a1f..4f48d0d6cf 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
@@ -66,24 +66,44 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
     return getCppLinkAction(target, type).getLinkCommandLine().getBuildVariables();
   }
 
-  /** Returns the value of a given variable in context of the given Variables instance. */
-  protected List<String> getVariableValue(Variables variables, String variable) throws Exception {
+  /** Returns the value of a given sequence variable in context of the given Variables instance. */
+  protected List<String> getSequenceVariableValue(Variables variables, String variable)
+      throws Exception {
     FeatureConfiguration mockFeatureConfiguration =
         CcToolchainFeaturesTest.buildFeatures(
                 "feature {",
-                "   name: 'a'",
-                "   flag_set {",
-                "   action: 'foo'",
-                "      flag_group {",
-                "         flag: '%{" + variable + "}'",
-                "      }",
-                "   }",
+                "  name: 'a'",
+                "  flag_set {",
+                "  action: 'foo'",
+                "    flag_group {",
+                "      iterate_over: '" + variable + "'",
+                "      flag: '%{" + variable + "}'",
+                "    }",
+                "  }",
                 "}")
             .getFeatureConfiguration(
                 FeatureSpecification.create(ImmutableSet.of("a"), ImmutableSet.<String>of()));
     return mockFeatureConfiguration.getCommandLine("foo", variables);
   }
 
+  /** Returns the value of a given string variable in context of the given Variables instance. */
+  protected String getVariableValue(Variables variables, String variable) throws Exception {
+    FeatureConfiguration mockFeatureConfiguration =
+        CcToolchainFeaturesTest.buildFeatures(
+                "feature {",
+                "  name: 'a'",
+                "  flag_set {",
+                "  action: 'foo'",
+                "    flag_group {",
+                "      flag: '%{" + variable + "}'",
+                "    }",
+                "  }",
+                "}")
+            .getFeatureConfiguration(
+                FeatureSpecification.create(ImmutableSet.of("a"), ImmutableSet.<String>of()));
+    return Iterables.getOnlyElement(mockFeatureConfiguration.getCommandLine("foo", variables));
+  }
+
   @Test
   public void testLinkstampBuildVariable() throws Exception {
     scratch.file(
@@ -105,7 +125,7 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
     ConfiguredTarget target = getConfiguredTarget("//x:bin");
     Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
     List<String> variableValue =
-        getVariableValue(variables, CppLinkActionBuilder.LINKSTAMP_PATHS_VARIABLE);
+        getSequenceVariableValue(variables, CppLinkActionBuilder.LINKSTAMP_PATHS_VARIABLE);
     assertThat(Iterables.getOnlyElement(variableValue)).contains("c.o");
   }
 
@@ -117,8 +137,7 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
 
     ConfiguredTarget target = getConfiguredTarget("//x:bin");
     Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
-    List<String> variableValue =
-        getVariableValue(variables, CppLinkActionBuilder.FORCE_PIC_VARIABLE);
+    String variableValue = getVariableValue(variables, CppLinkActionBuilder.FORCE_PIC_VARIABLE);
     assertThat(variableValue).contains("");
   }
 
@@ -161,7 +180,8 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
     ConfiguredTarget target = getConfiguredTarget("//x:bin");
     Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
     List<String> variableValue =
-        getVariableValue(variables, CppLinkActionBuilder.LIBRARY_SEARCH_DIRECTORIES_VARIABLE);
+        getSequenceVariableValue(
+            variables, CppLinkActionBuilder.LIBRARY_SEARCH_DIRECTORIES_VARIABLE);
     assertThat(Iterables.getOnlyElement(variableValue)).contains("some-dir");
   }
 
@@ -175,10 +195,9 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
 
     ConfiguredTarget target = getConfiguredTarget("//x:bin");
     Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
-    List<String> variableValue =
+    String variableValue =
         getVariableValue(variables, CppLinkActionBuilder.LINKER_PARAM_FILE_VARIABLE);
-    assertThat(Iterables.getOnlyElement(variableValue)).matches(".*bin/x/bin"
-        + OsUtils.executableExtension() + "-2.params$");
+    assertThat(variableValue).matches(".*bin/x/bin" + OsUtils.executableExtension() + "-2.params$");
   }
 
   @Test
@@ -197,17 +216,13 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
     Variables variables = getLinkBuildVariables(target, LinkTargetType.DYNAMIC_LIBRARY);
 
     String interfaceLibraryBuilder =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_BUILDER_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_BUILDER_VARIABLE);
     String interfaceLibraryInput =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_INPUT_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_INPUT_VARIABLE);
     String interfaceLibraryOutput =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_OUTPUT_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_OUTPUT_VARIABLE);
     String generateInterfaceLibrary =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.GENERATE_INTERFACE_LIBRARY_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.GENERATE_INTERFACE_LIBRARY_VARIABLE);
 
     assertThat(generateInterfaceLibrary).isEqualTo("yes");
     assertThat(interfaceLibraryInput).endsWith("libfoo.so");
@@ -231,17 +246,13 @@ public class LinkBuildVariablesTest extends BuildViewTestCase {
     Variables variables = getLinkBuildVariables(target, LinkTargetType.STATIC_LIBRARY);
 
     String interfaceLibraryBuilder =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_BUILDER_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_BUILDER_VARIABLE);
     String interfaceLibraryInput =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_INPUT_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_INPUT_VARIABLE);
     String interfaceLibraryOutput =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_OUTPUT_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.INTERFACE_LIBRARY_OUTPUT_VARIABLE);
     String generateInterfaceLibrary =
-        Iterables.getOnlyElement(
-            getVariableValue(variables, CppLinkActionBuilder.GENERATE_INTERFACE_LIBRARY_VARIABLE));
+        getVariableValue(variables, CppLinkActionBuilder.GENERATE_INTERFACE_LIBRARY_VARIABLE);
 
     assertThat(generateInterfaceLibrary).isEqualTo("no");
     assertThat(interfaceLibraryInput).endsWith("ignored");