Make ExecutionInfoModifier have a useful equals by making it AutoValue.

All objects which are to be part of the build options must parse equal on repeated
invocations, or the analysis cache will be discarded unnecessarily.

It's also now possible to create an empty ExecutionInfoModifier. This is used
instead of null as the default value. This allows for more easily turning
--modify_execution_info off, even when it's in the RC file. (It's already
possible to do this by creating a silly regex like "(?!.*)=-nonsense-value",
so this isn't a new capability, it's just a more discoverable one.)

Also cleans up the parsing code to use Guava string-handling methods and
fixes some uses of plain non-linked HashMap surrounding the use of this.
Most users of this method were already using LinkedHashMaps, so this makes
callers more consistent.

RELNOTES: None.
PiperOrigin-RevId: 214622440

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java

@@ -911,7 +911,7 @@ public ConfigsModeConverter() {
           OptionEffectTag.AFFECTS_OUTPUTS,
           OptionEffectTag.LOADING_AND_ANALYSIS,
         },
-        defaultValue = "null",
+        defaultValue = "",
         help =
             "Add or remove keys from an action's execution info based on action mnemonic.  "
                 + "Applies only to actions which support execution info. Many common actions "
@@ -1841,19 +1841,17 @@ public boolean runfilesEnabled() {
    */
   public ImmutableMap<String, String> modifiedExecutionInfo(
       ImmutableMap<String, String> executionInfo, String mnemonic) {
-    if (options.executionInfoModifier == null || !options.executionInfoModifier.matches(mnemonic)) {
+    if (!options.executionInfoModifier.matches(mnemonic)) {
       return executionInfo;
     }
-    HashMap<String, String> mutableCopy = new HashMap<>(executionInfo);
+    LinkedHashMap<String, String> mutableCopy = new LinkedHashMap<>(executionInfo);
     modifyExecutionInfo(mutableCopy, mnemonic);
     return ImmutableMap.copyOf(mutableCopy);
   }
 
   /** Applies {@code executionInfoModifiers} to the given {@code executionInfo}. */
   public void modifyExecutionInfo(Map<String, String> executionInfo, String mnemonic) {
-    if (options.executionInfoModifier != null) {
-      options.executionInfoModifier.apply(mnemonic, executionInfo);
-    }
+    options.executionInfoModifier.apply(mnemonic, executionInfo);
   }
 
   /** @return the list of default features used for all packages. */

src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java

@@ -15,7 +15,11 @@
 package com.google.devtools.build.lib.analysis.config;
 
 import com.google.auto.value.AutoValue;
+import com.google.auto.value.extension.memoized.Memoized;
+import com.google.common.base.Splitter;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableList.Builder;
 import com.google.devtools.common.options.Converters.RegexPatternConverter;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.Map;
@@ -26,52 +30,58 @@
  * Represents a list of regexes over mnemonics and changes to add or remove keys, parsed from a
  * {@code --modify_execution_info} option.
  */
-public final class ExecutionInfoModifier {
+@AutoValue
+public abstract class ExecutionInfoModifier {
 
   private static final Pattern MODIFIER_PATTERN =
       Pattern.compile("^(?<pattern>.+)=(?<sign>[+-])(?<key>.+)$");
 
-  private final ImmutableList<Expression> expressions;
-  private final String option;
+  private static final ExecutionInfoModifier EMPTY = create("", ImmutableList.of());
+
+  abstract String option();
+  abstract ImmutableList<Expression> expressions();
 
   @AutoValue
   abstract static class Expression {
-    abstract Pattern pattern();
+    // Patterns do not have a useful equals(), so compare by the regex and memoize the derived
+    // Pattern.
+    @Memoized
+    Pattern pattern() {
+      return Pattern.compile(regex());
+    }
+
+    abstract String regex();
 
     abstract boolean remove();
 
     abstract String key();
   }
 
-  private ExecutionInfoModifier(String option, ImmutableList<Expression> expressions) {
-    this.expressions = expressions;
-    this.option = option;
-  }
-
   /** Constructs an instance of ExecutionInfoModifier by parsing an option string. */
   public static class Converter
       implements com.google.devtools.common.options.Converter<ExecutionInfoModifier> {
     @Override
     public ExecutionInfoModifier convert(String input) throws OptionsParsingException {
-
-      String[] expressionSpecs = input.split(",");
-      if (expressionSpecs.length == 0) {
-        throw new OptionsParsingException("expected 'regex=[+-]key,regex=[+-]key,...'", input);
+      if (Strings.isNullOrEmpty(input)) {
+        return EMPTY;
       }
+
       ImmutableList.Builder<Expression> expressionBuilder = ImmutableList.builder();
-      for (String spec : expressionSpecs) {
+      for (String spec : Splitter.on(",").split(input)) {
         Matcher specMatcher = MODIFIER_PATTERN.matcher(spec);
         if (!specMatcher.matches()) {
           throw new OptionsParsingException(
               String.format("malformed expression '%s'", spec), input);
         }
         expressionBuilder.add(
             new AutoValue_ExecutionInfoModifier_Expression(
-                new RegexPatternConverter().convert(specMatcher.group("pattern")),
+                // Convert to get a useful exception if it's not a valid pattern, but use the regex
+                // (see comment in Expression)
+                new RegexPatternConverter().convert(specMatcher.group("pattern")).pattern(),
                 specMatcher.group("sign").equals("-"),
                 specMatcher.group("key")));
       }
-      return new ExecutionInfoModifier(input, expressionBuilder.build());
+      return ExecutionInfoModifier.create(input, expressionBuilder.build());
     }
 
     @Override
@@ -80,16 +90,20 @@ public String getTypeDescription() {
     }
   }
 
+  private static ExecutionInfoModifier create(String input, ImmutableList<Expression> expressions) {
+    return new AutoValue_ExecutionInfoModifier(input, expressions);
+  }
+
   /**
    * Determines whether the given {@code mnemonic} (e.g. "CppCompile") matches any of the patterns.
    */
   public boolean matches(String mnemonic) {
-    return expressions.stream().anyMatch(expr -> expr.pattern().matcher(mnemonic).matches());
+    return expressions().stream().anyMatch(expr -> expr.pattern().matcher(mnemonic).matches());
   }
 
   /** Modifies the given map of {@code executionInfo} to add or remove the keys for this option. */
   void apply(String mnemonic, Map<String, String> executionInfo) {
-    for (Expression expr : expressions) {
+    for (Expression expr : expressions()) {
       if (expr.pattern().matcher(mnemonic).matches()) {
         if (expr.remove()) {
           executionInfo.remove(expr.key());
@@ -99,9 +113,4 @@ void apply(String mnemonic, Map<String, String> executionInfo) {
       }
     }
   }
-
-  @Override
-  public String toString() {
-    return option;
-  }
 }

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

@@ -58,6 +58,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -1077,7 +1078,7 @@ public CppLinkAction build() throws InterruptedException {
     // If the crosstool uses action_configs to configure cc compilation, collect execution info
     // from there, otherwise, use no execution info.
     // TODO(b/27903698): Assert that the crosstool has an action_config for this action.
-    Map<String, String> executionRequirements = new HashMap<>();
+    Map<String, String> executionRequirements = new LinkedHashMap<>();
 
     if (featureConfiguration.actionIsConfigured(getActionName())) {
       for (String req : featureConfiguration.getToolRequirementsForAction(getActionName())) {

src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifierTest.java

@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.analysis.config.ExecutionInfoModifier.Converter;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.HashMap;
@@ -36,6 +37,12 @@ public class ExecutionInfoModifierTest {
 
   private final ExecutionInfoModifier.Converter converter = new Converter();
 
+  @Test
+  public void executionInfoModifier_empty() throws Exception {
+    ExecutionInfoModifier modifier = converter.convert("");
+    assertThat(modifier.matches("Anything")).isFalse();
+  }
+
   @Test
   public void executionInfoModifier_singleAdd() throws Exception {
     ExecutionInfoModifier modifier = converter.convert("Genrule=+x");
@@ -65,7 +72,7 @@ public void executionInfoModifier_multipleExpressions() throws Exception {
   @Test
   public void executionInfoModifier_invalidFormat_throws() throws Exception {
     List<String> invalidModifiers =
-        ImmutableList.of("", "A", "=", "A=", "A=+", "=+", "A=-B,A", "A=B", "A", ",");
+        ImmutableList.of("A", "=", "A=", "A=+", "=+", "A=-B,A", "A=B", "A", ",");
     for (String invalidModifer : invalidModifiers) {
       assertThrows(OptionsParsingException.class, () -> converter.convert(invalidModifer));
     }
@@ -79,6 +86,28 @@ public void executionInfoModifier_invalidFormat_exceptionShowsOffender() throws
     assertThat(thrown).hasMessageThat().contains("'B=2'");
   }
 
+  @Test
+  public void executionInfoModifier_EqualsTester() throws Exception {
+    new EqualsTester()
+        // base empty
+        .addEqualityGroup(converter.convert(""), converter.convert(""))
+        // base non-empty
+        .addEqualityGroup(converter.convert("A=+B"), converter.convert("A=+B"))
+        // different pattern and key
+        .addEqualityGroup(converter.convert("C=+D"))
+        // different key
+        .addEqualityGroup(converter.convert("A=+D"))
+        // different pattern
+        .addEqualityGroup(converter.convert("C=+B"))
+        // different operation
+        .addEqualityGroup(converter.convert("A=-B"))
+        // more items
+        .addEqualityGroup(converter.convert("A=+B,C=-D"), converter.convert("A=+B,C=-D"))
+        // different order
+        .addEqualityGroup(converter.convert("C=-D,A=+B"))
+        .testEquals();
+  }
+
   private void assertModifierMatchesAndResults(
       ExecutionInfoModifier modifier, String mnemonic, Set<String> expectedKeys) {
     Map<String, String> copy = new HashMap<>();