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, 22 insertions, 96 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
index 1eee0802c4..db8774d118 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
@@ -360,7 +360,6 @@ public class CcToolchainFeatures implements Serializable {
   @Immutable
   private static class FlagGroup implements Serializable, Expandable {
     private final ImmutableList<Expandable> expandables;
-    private final ImmutableSet<String> usedVariables;
     private String iterateOverVariable;
     private final ImmutableSet<String> expandIfAllAvailable;
     private final ImmutableSet<String> expandIfNoneAvailable;
@@ -368,15 +367,8 @@ public class CcToolchainFeatures implements Serializable {
     private final String expandIfFalse;
     private final VariableWithValue expandIfEqual;
 
-    /**
-     * TODO(b/32655571): Cleanup and get rid of usedVariables field once implicit iteration is not
-     * needed.
-     *
-     * @throws InvalidConfigurationException
-     */
     private FlagGroup(CToolchain.FlagGroup flagGroup) throws InvalidConfigurationException {
       ImmutableList.Builder<Expandable> expandables = ImmutableList.builder();
-      ImmutableSet.Builder<String> usedVariables = ImmutableSet.builder();
       Collection<String> flags = flagGroup.getFlagList();
       Collection<CToolchain.FlagGroup> groups = flagGroup.getFlagGroupList();
       if (!flags.isEmpty() && !groups.isEmpty()) {
@@ -388,18 +380,14 @@ public class CcToolchainFeatures implements Serializable {
       for (String flag : flags) {
         StringValueParser parser = new StringValueParser(flag);
         expandables.add(new Flag(parser.getChunks()));
-        usedVariables.addAll(parser.getUsedVariables());
       }
       for (CToolchain.FlagGroup group : groups) {
         FlagGroup subgroup = new FlagGroup(group);
         expandables.add(subgroup);
-        usedVariables.addAll(subgroup.getUsedVariables());
       }
       if (flagGroup.hasIterateOver()) {
         this.iterateOverVariable = flagGroup.getIterateOver();
-        usedVariables.add(this.iterateOverVariable);
       }
-      this.usedVariables = usedVariables.build();
       this.expandables = expandables.build();
       this.expandIfAllAvailable = ImmutableSet.copyOf(flagGroup.getExpandIfAllAvailableList());
       this.expandIfNoneAvailable = ImmutableSet.copyOf(flagGroup.getExpandIfNoneAvailableList());
@@ -419,10 +407,6 @@ public class CcToolchainFeatures implements Serializable {
       if (!canBeExpanded(variables)) {
         return;
       }
-      if (iterateOverVariable == null) {
-        // TODO(b/32655571): Remove branch once implicit iteration is not needed anymore.
-        iterateOverVariable = variables.guessIteratedOverVariable(usedVariables);
-      }
       if (iterateOverVariable != null) {
         for (VariableValue variableValue : variables.getSequenceVariable(iterateOverVariable)) {
           Variables nestedVariables = new Variables(variables, iterateOverVariable, variableValue);
@@ -469,10 +453,6 @@ public class CcToolchainFeatures implements Serializable {
       return true;
     }
 
-    private Set<String> getUsedVariables() {
-      return usedVariables;
-    }
-
     /**
      * Expands all flags in this group and adds them to {@code commandLine}.
      *
@@ -870,9 +850,6 @@ public class CcToolchainFeatures implements Serializable {
        */
       Iterable<? extends VariableValue> getSequenceValue(String variableName);
 
-      // TODO(b/32655571): Remove once implicit iteration is not needed
-      boolean isSequence();
-
       /**
        * Return value of the field, if the variable is of struct type or throw exception if it is
        * not or no such field exists.
@@ -1084,11 +1061,6 @@ public class CcToolchainFeatures implements Serializable {
       }
 
       @Override
-      public boolean isSequence() {
-        return false;
-      }
-
-      @Override
       public VariableValue getFieldValue(String variableName, String field) {
         Preconditions.checkNotNull(field);
         if (NAME_FIELD_NAME.equals(field) && !type.equals(Type.OBJECT_FILE_GROUP)) {
@@ -1143,11 +1115,6 @@ public class CcToolchainFeatures implements Serializable {
       }
 
       @Override
-      public boolean isSequence() {
-        return true;
-      }
-
-      @Override
       public VariableValue getFieldValue(String variableName, String field) {
         throw new ExpansionException(
             String.format(
@@ -1196,11 +1163,6 @@ public class CcToolchainFeatures implements Serializable {
       }
 
       @Override
-      public boolean isSequence() {
-        return true;
-      }
-
-      @Override
       public VariableValue getFieldValue(String variableName, String field) {
         throw new ExpansionException(
             String.format(
@@ -1240,11 +1202,6 @@ public class CcToolchainFeatures implements Serializable {
       }
 
       @Override
-      public boolean isSequence() {
-        return true;
-      }
-
-      @Override
       public VariableValue getFieldValue(String variableName, String field) {
         throw new ExpansionException(
             String.format(
@@ -1309,11 +1266,6 @@ public class CcToolchainFeatures implements Serializable {
       }
 
       @Override
-      public boolean isSequence() {
-        return false;
-      }
-
-      @Override
       public boolean isTruthy() {
         return !value.isEmpty();
       }
@@ -1357,11 +1309,6 @@ public class CcToolchainFeatures implements Serializable {
       }
 
       @Override
-      public boolean isSequence() {
-        return false;
-      }
-
-      @Override
       public boolean isTruthy() {
         return !value.isEmpty();
       }
@@ -1405,11 +1352,6 @@ public class CcToolchainFeatures implements Serializable {
       }
 
       @Override
-      public boolean isSequence() {
-        return false;
-      }
-
-      @Override
       public boolean isTruthy() {
         return value != 0;
       }
@@ -1685,27 +1627,6 @@ public class CcToolchainFeatures implements Serializable {
       return getVariable(variableName).getSequenceValue(variableName);
     }
 
-    private String guessIteratedOverVariable(ImmutableSet<String> usedVariables) {
-      String sequenceName = null;
-      for (String usedVariable : usedVariables) {
-        VariableValue variableValue = lookupVariable(usedVariable, false);
-        if (variableValue != null && variableValue.isSequence()) {
-          if (sequenceName != null) {
-            throw new ExpansionException(
-                "Invalid toolchain configuration: trying to expand two variable list in one "
-                    + "flag group: '"
-                    + sequenceName
-                    + "' and '"
-                    + usedVariable
-                    + "'");
-          } else {
-            sequenceName = usedVariable;
-          }
-        }
-      }
-      return sequenceName;
-    }
-
     /** Returns whether {@code variable} is set. */
     boolean isAvailable(String variable) {
       return lookupVariable(variable, false) != null;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
index 36dfc9eb93..5229ddf19f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
@@ -18,15 +18,15 @@ import com.google.common.collect.ImmutableList;
 import java.util.Set;
 
 /**
- * A helper class for creating action_configs for the c++ link action.
+ * A helper class for creating action_configs for the c++ actions.
  *
  * <p>TODO(b/30109612): Replace this with action_configs in the crosstool instead of putting it in
  * legacy features.
  */
 public class CppActionConfigs {
 
-  /** A platform for linker invocations. */
-  public static enum CppPlatform {
+  /** A platform for C++ tool invocations. */
+  public enum CppPlatform {
     LINUX,
     MAC
   }
@@ -44,15 +44,15 @@ public class CppActionConfigs {
       cppDynamicLibraryLinkerTool =
           ""
               + "feature {"
-              + "   name: 'dynamic_library_linker_tool'"
-              + "   flag_set {"
-              + "       action: 'c++-link-dynamic-library'"
-              + "       flag_group {"
-              + "           flag: '"
+              + "  name: 'dynamic_library_linker_tool'"
+              + "  flag_set {"
+              + "    action: 'c++-link-dynamic-library'"
+              + "    flag_group {"
+              + "      flag: '"
               + cppLinkDynamicLibraryToolPath
               + "'"
-              + "       }"
-              + "   }"
+              + "    }"
+              + "  }"
               + "}";
     }
 
@@ -240,6 +240,7 @@ public class CppActionConfigs {
                 "    action: 'c++-link-dynamic-library'",
                 "    expand_if_all_available: 'linkstamp_paths'",
                 "    flag_group {",
+                "      iterate_over: 'linkstamp_paths'",
                 "      flag: '%{linkstamp_paths}'",
                 "    }",
                 "  }",
@@ -328,7 +329,10 @@ public class CppActionConfigs {
                     "    action: 'c++-link-pic-static-library'",
                     "    action: 'c++-link-alwayslink-pic-static-library'",
                     "    flag_group {",
-                    ifLinux(platform, "  flag: 'rcsD'", "  flag: '%{output_execpath}'"),
+                    ifLinux(
+                        platform,
+                        "  flag: 'rcsD'",
+                        "  flag: '%{output_execpath}'"),
                     ifMac(
                         platform,
                         "  flag: '-static'",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index a8e6233d02..3e0c7446c1 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -773,6 +773,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
                   + "    action: 'c++-module-compile'"
                   + "    action: 'clif-match'"
                   + "    flag_group {"
+                  + "      iterate_over: 'preprocessor_defines'"
                   + "      flag: '-D%{preprocessor_defines}'"
                   + "    }"
                   + "  }"
@@ -795,13 +796,16 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
                   + "    action: 'objc-compile'"
                   + "    action: 'objc++-compile'"
                   + "    flag_group {"
+                  + "      iterate_over: 'quote_include_paths'"
                   + "      flag: '-iquote'"
                   + "      flag: '%{quote_include_paths}'"
                   + "    }"
                   + "    flag_group {"
+                  + "      iterate_over: 'include_paths'"
                   + "      flag: '-I%{include_paths}'"
                   + "    }"
                   + "    flag_group {"
+                  + "      iterate_over: 'system_include_paths'"
                   + "      flag: '-isystem'"
                   + "      flag: '%{system_include_paths}'"
                   + "    }"
@@ -890,11 +894,8 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
           String linkerFlags;
           if (useLLVMCoverageMap) {
             compileFlags =
-                "flag_group {"
-                    + " flag: '-fprofile-instr-generate'"
-                    + " flag: '-fcoverage-mapping'"
-                    + "}";
-            linkerFlags = "  flag_group {" + "  flag: '-fprofile-instr-generate'" + "}";
+                "flag_group { flag: '-fprofile-instr-generate' flag: '-fcoverage-mapping' }";
+            linkerFlags = "flag_group { flag: '-fprofile-instr-generate' }";
           } else {
             compileFlags =
                 "  expand_if_all_available: 'gcov_gcno_file'"
@@ -902,7 +903,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
                     + "  flag: '-fprofile-arcs'"
                     + "  flag: '-ftest-coverage'"
                     + "}";
-            linkerFlags = "  flag_group {" + "  flag: '-lgcov'" + "}";
+            linkerFlags = "  flag_group { flag: '-lgcov' }";
           }
           TextFormat.merge(
               ""