Support multiple --define's in config_setting.

This fixes the problem that

config_setting(
  name = "a_and_b",
  values = {
    "define": "a=c",
    "define": "b=d"
  })

doesn't work as expected because BUILD parsing removes duplicate dictionary keys in accordance with Pythonic behavior. Even worse, Skylark will soon enforce this more aggressively by making this an outright error.

This change introduces the define_values attribute:

config_setting(
  name = "a_and_b",
  values = {
      "normal_flag": "normal_value",
  },
  define_values = {
    "a: "c",
    "b": "d"
  })

This is equivalent to "$ bazel build ... --normal_flag=normal_value --define a=c --define b=d" at the command line.

Also tried to clean up some ConfigSetting naming for clarity around the different kind of flags.

PiperOrigin-RevId: 162627180

3 files changed, 120 insertions, 38 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java
index 586c46257a..c1a28bca67 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java
@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.analysis.config;
 
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -33,7 +35,7 @@ public final class ConfigMatchingProvider implements TransitiveInfoProvider {
 
   private final Label label;
   private final boolean matches;
-  private final Map<String, String> settingsMap;
+  private final Multimap<String, String> settingsMap;
   private final Map<Label, String> flagSettingsMap;
 
   /**
@@ -45,7 +47,7 @@ public final class ConfigMatchingProvider implements TransitiveInfoProvider {
    */
   public ConfigMatchingProvider(
       Label label,
-      Map<String, String> settingsMap,
+      Multimap<String, String> settingsMap,
       Map<Label, String> flagSettingsMap,
       boolean matches) {
     this.label = label;
@@ -74,8 +76,8 @@ public final class ConfigMatchingProvider implements TransitiveInfoProvider {
    * conditions, i.e. if this matcher is a specialization of the other one.
    */
   public boolean refines(ConfigMatchingProvider other) {
-    Set<Map.Entry<String, String>> settings = settingsMap.entrySet();
-    Set<Map.Entry<String, String>> otherSettings = other.settingsMap.entrySet();
+    Set<Map.Entry<String, String>> settings = ImmutableSet.copyOf(settingsMap.entries());
+    Set<Map.Entry<String, String>> otherSettings = ImmutableSet.copyOf(other.settingsMap.entries());
     Set<Map.Entry<Label, String>> flagSettings = flagSettingsMap.entrySet();
     Set<Map.Entry<Label, String>> otherFlagSettings = other.flagSettingsMap.entrySet();
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
index 29ecacea1a..a6b30a4baa 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
@@ -102,10 +102,10 @@ public class ConfigRuleClasses {
      */
     public static final String RULE_NAME = "config_setting";
 
-    /**
-     * The name of the attribute that declares flag bindings.
-     */
+    /** The name of the attribute that declares flag bindings. */
     public static final String SETTINGS_ATTRIBUTE = "values";
+    /** The name of the attribute that declares "--define foo=bar" flag bindings.*/
+    public static final String DEFINE_SETTINGS_ATTRIBUTE = "define_values";
     /** The name of the attribute that declares user-defined flag bindings. */
     public static final String FLAG_SETTINGS_ATTRIBUTE = "flag_values";
 
@@ -143,12 +143,58 @@ public class ConfigRuleClasses {
              <i>any</i> of those settings match.
           <p>
 
-          <p>This attribute cannot be empty.
+          <p>This and <a href="${link config_setting.define_values}"><code>values</code></a> cannot
+             both be empty.
           </p>
           <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .add(
               attr(SETTINGS_ATTRIBUTE, STRING_DICT)
                   .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON))
+          /* <!-- #BLAZE_RULE(config_setting).ATTRIBUTE(define_values) -->
+          The same as <a href="${link config_setting.values}"><code>values</code></a> but
+          specifically for the <code>--define</code> flag.
+
+          <p><code>--define</code> is special for two reasons:
+
+          <ol>
+            <li>It's the primary interface Blaze has today for declaring user-definable settings.
+            </li>
+            <li>Its syntax (<code>--define KEY=VAL</code>) means <code>KEY=VAL</code> is
+            a <i>value</i> from a Blaze flag perspective.</li>
+          </ol>
+
+          <p>That means:
+
+          <pre class="code">
+            config_setting(
+                name = "a_and_b",
+                values = {
+                    "define": "a=1",
+                    "define": "b=2",
+                })
+          </pre>
+
+          <p>doesn't work because the same key (<code>define</code>) appears twice in the
+          dictionary. This attribute solves that problem:
+
+          <pre class="code">
+            config_setting(
+                name = "a_and_b",
+                define_values = {
+                    "a": "1",
+                    "b": "2",
+                })
+          </pre>
+
+          <p>corrrectly matches <code>blaze build //foo --define a=1 --define b=2</code>.
+
+          <p><code>--define</code> can still appear in
+          <a href="${link config_setting.values}"><code>values</code></a> with normal flag syntax,
+          and can be mixed freely with this attribute as long as dictionary keys remain distinct.
+          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+          .add(
+              attr(DEFINE_SETTINGS_ATTRIBUTE, STRING_DICT)
+                  .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON))
           .add(
               attr(FLAG_SETTINGS_ATTRIBUTE, LABEL_KEYED_STRING_DICT)
                   .undocumented("the feature flag feature has not yet been launched")
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
index 8215f4ad78..4b66c1f2a4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
@@ -15,11 +15,16 @@
 package com.google.devtools.build.lib.rules.config;
 
 import com.google.common.base.Joiner;
+import com.google.common.collect.HashMultiset;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Multiset;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
@@ -34,17 +39,20 @@ import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.TransitiveOptionDetails;
 import com.google.devtools.build.lib.analysis.featurecontrol.FeaturePolicyConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
-import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.packages.RuleErrorConsumer;
 import com.google.devtools.build.lib.rules.AliasProvider;
 import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
+import com.google.devtools.build.lib.rules.config.ConfigRuleClasses.ConfigSettingRule;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
+
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -62,41 +70,53 @@ public class ConfigSetting implements RuleConfiguredTargetFactory {
   @Override
   public ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException {
-    // Get the required flag=value settings for this rule.
-    Map<String, String> settings = NonconfigurableAttributeMapper.of(ruleContext.getRule())
-        .get(ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE, Type.STRING_DICT);
-    Map<Label, String> flagSettings =
+    AttributeMap attributes = NonconfigurableAttributeMapper.of(ruleContext.getRule());
+    // Get the built-in Blaze flag settings that match this rule.
+    ImmutableMultimap<String, String> nativeFlagSettings =
+        ImmutableMultimap.<String, String>builder()
+            .putAll(attributes.get(ConfigSettingRule.SETTINGS_ATTRIBUTE, Type.STRING_DICT)
+                .entrySet())
+            .putAll(attributes.get(ConfigSettingRule.DEFINE_SETTINGS_ATTRIBUTE, Type.STRING_DICT)
+                .entrySet()
+                .stream()
+                .map(in -> Maps.immutableEntry("define", in.getKey() + "=" + in.getValue()))
+                .collect(ImmutableList.toImmutableList()))
+            .build();
+
+    // Get the user-defined flag settings that match this rule.
+    Map<Label, String> userDefinedFlagSettings =
         NonconfigurableAttributeMapper.of(ruleContext.getRule())
             .get(
-                ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
+                ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
                 BuildType.LABEL_KEYED_STRING_DICT);
-    if (!flagSettings.isEmpty()) {
+
+    if (!userDefinedFlagSettings.isEmpty()) {
       FeaturePolicyConfiguration.checkAvailable(
           ruleContext,
           ConfigFeatureFlag.POLICY_NAME,
-          "the " + ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE + " attribute");
+          "the " + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE + " attribute");
     }
 
     List<? extends TransitiveInfoCollection> flagValues =
         ruleContext.getPrerequisites(
-            ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET);
+            ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET);
 
-    if (settings.isEmpty() && flagSettings.isEmpty()) {
+    if (nativeFlagSettings.size() == 0 && userDefinedFlagSettings.isEmpty()) {
       ruleContext.ruleError(
           String.format(
               "Either %s or %s must be specified and non-empty",
-              ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE,
-              ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE));
+              ConfigSettingRule.SETTINGS_ATTRIBUTE,
+              ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE));
       return null;
     }
 
     ConfigFeatureFlagMatch featureFlags =
         ConfigFeatureFlagMatch.fromAttributeValueAndPrerequisites(
-            flagSettings, flagValues, (RuleErrorConsumer) ruleContext);
+            userDefinedFlagSettings, flagValues, ruleContext);
 
-    boolean settingsMatch =
+    boolean nativeFlagsMatch =
         matchesConfig(
-            settings,
+            nativeFlagSettings.entries(),
             BuildConfigurationOptionDetails.get(ruleContext.getConfiguration()),
             ruleContext);
 
@@ -107,9 +127,9 @@ public class ConfigSetting implements RuleConfiguredTargetFactory {
     ConfigMatchingProvider configMatcher =
         new ConfigMatchingProvider(
             ruleContext.getLabel(),
-            settings,
+            nativeFlagSettings,
             featureFlags.getSpecifiedFlagValues(),
-            settingsMatch && featureFlags.matches());
+            nativeFlagsMatch && featureFlags.matches());
 
     return new RuleConfiguredTargetBuilder(ruleContext)
         .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY)
@@ -121,30 +141,40 @@ public class ConfigSetting implements RuleConfiguredTargetFactory {
   }
 
   /**
-   * Given a list of [flagName, flagValue] pairs, returns true if flagName == flagValue for every
-   * item in the list under this configuration, false otherwise.
+   * User error when value settings can't be properly parsed.
+   */
+  private static final String PARSE_ERROR_MESSAGE = "error while parsing configuration settings: ";
+
+  /**
+   * Given a list of [flagName, flagValue] pairs for native Blaze flags, returns true if
+   * flagName == flagValue for every item in the list under this configuration, false otherwise.
    */
   private boolean matchesConfig(
-      Map<String, String> expectedSettings,
+      Collection<Map.Entry<String, String>> expectedSettings,
       TransitiveOptionDetails options,
       RuleErrorConsumer errors) {
     // Rather than returning fast when we find a mismatch, continue looking at the other flags
-    // to check that they're indeed valid flag specifications.
+    // to check they're indeed valid flag specifications.
     boolean foundMismatch = false;
 
+    // Flags that appear multiple times are known as "multi-value options". Each time the options
+    // parser parses one of their values it adds it to an existing list. In those cases we need to
+    // make sure to examine only the value we just parsed: not the entire list.
+    Multiset<String> optionsCount = HashMultiset.create();
+
     // Since OptionsParser instantiation involves reflection, let's try to minimize that happening.
     Map<Class<? extends OptionsBase>, OptionsParser> parserCache = new HashMap<>();
 
-    for (Map.Entry<String, String> setting : expectedSettings.entrySet()) {
+    for (Map.Entry<String, String> setting : expectedSettings) {
       String optionName = setting.getKey();
       String expectedRawValue = setting.getValue();
+      int previousOptionCount = optionsCount.add(optionName, 1);
 
       Class<? extends OptionsBase> optionClass = options.getOptionClass(optionName);
       if (optionClass == null) {
         errors.attributeError(
-            ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE,
-            String.format(
-                "error while parsing configuration settings: unknown option: '%s'", optionName));
+            ConfigSettingRule.SETTINGS_ATTRIBUTE,
+            String.format(PARSE_ERROR_MESSAGE + "unknown option: '%s'", optionName));
         foundMismatch = true;
         continue;
       }
@@ -156,14 +186,18 @@ public class ConfigSetting implements RuleConfiguredTargetFactory {
         parser.parse("--" + optionName + "=" + expectedRawValue);
       } catch (OptionsParsingException ex) {
         errors.attributeError(
-            ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE,
-            "error while parsing configuration settings: " + ex.getMessage());
+            ConfigSettingRule.SETTINGS_ATTRIBUTE,
+            PARSE_ERROR_MESSAGE + ex.getMessage());
         foundMismatch = true;
         continue;
       }
 
       Object expectedParsedValue = parser.getOptions(optionClass).asMap().get(optionName);
-
+      if (previousOptionCount > 0) {
+        // We've seen this option before, so it's a multi-value option with multiple entries.
+        int listLength = ((List<?>) expectedParsedValue).size();
+        expectedParsedValue = ((List<?>) expectedParsedValue).subList(listLength - 1, listLength);
+      }
       if (!optionMatches(options, optionName, expectedParsedValue)) {
         foundMismatch = true;
       }
@@ -281,7 +315,7 @@ public class ConfigSetting implements RuleConfiguredTargetFactory {
 
         if (!provider.isValidValue(specifiedValue)) {
           errors.attributeError(
-              ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
+              ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
               String.format(
                   "error while parsing user-defined configuration values: "
                       + "'%s' is not a valid value for '%s'",
@@ -304,7 +338,7 @@ public class ConfigSetting implements RuleConfiguredTargetFactory {
           List<Label> aliasList = aliases.get(actualLabel);
           if (aliasList.size() > 1) {
             errors.attributeError(
-                ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
+                ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
                 String.format(
                     "flag '%s' referenced multiple times as ['%s']",
                     actualLabel, QUOTED_COMMA_JOINER.join(aliasList)));