Fix aliases for users of label-keyed string dicts.

Aliases mess with the assumption that attributeValue.containsKey(target.getLabel()) for every target in the prerequisites of a LABEL_KEYED_STRING_DICT attribute. The solution is to use AliasProvider.getDependencyLabel(target) instead. This fixes it for all current users, including SkylarkRuleContext. This also adjusts config_setting flag_values and Android feature_flags to do intelligent things with aliases in their respective attributes. RELNOTES: None. PiperOrigin-RevId: 157594095
bazelbuild · Jun 1, 2017 · fd2c682 · fd2c682
1 parent 65ceda9
commit fd2c682
Show file tree

Hide file tree

Showing 6 changed files with 290 additions and 54 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
@@ -404,7 +404,7 @@ private static SkylarkRuleAttributesCollection buildAttributesCollection(
         List<? extends TransitiveInfoCollection> allPrereq =
             ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK);
         for (TransitiveInfoCollection prereq : allPrereq) {
-          builder.put(prereq, original.get(prereq.getLabel()));
+          builder.put(prereq, original.get(AliasProvider.getDependencyLabel(prereq)));
         }
         attrBuilder.put(skyname, SkylarkType.convertToSkylark(builder.build(), null));
       } else if (type == BuildType.LABEL_DICT_UNARY) {

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java
@@ -25,6 +25,7 @@
 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.rules.AliasProvider;
 import com.google.devtools.build.lib.rules.config.ConfigFeatureFlagProvider;
 import java.util.Map;
 
@@ -53,7 +54,7 @@ public static AndroidFeatureFlagSetProvider create(Map<Label, String> flags) {
   /**
    * Builds a map which can be used with create, confirming that the desired flag values were
    * actually received, and producing an error if they were not (because dynamic configurations are
-   * not enabled).
+   * not enabled, or because aliases were used).
    */
   public static ImmutableMap<Label, String> getAndValidateFlagMapFromRuleContext(
       RuleContext ruleContext) throws RuleErrorException {
@@ -62,8 +63,18 @@ public static ImmutableMap<Label, String> getAndValidateFlagMapFromRuleContext(
             .get(FEATURE_FLAG_ATTR, BuildType.LABEL_KEYED_STRING_DICT);
     Iterable<? extends TransitiveInfoCollection> actualTargets =
         ruleContext.getPrerequisites(FEATURE_FLAG_ATTR, Mode.TARGET);
+    boolean aliasFound = false;
     for (TransitiveInfoCollection target : actualTargets) {
-      Label label = target.getLabel();
+      Label label = AliasProvider.getDependencyLabel(target);
+      if (!label.equals(target.getLabel())) {
+        ruleContext.attributeError(
+            FEATURE_FLAG_ATTR,
+            String.format(
+                "Feature flags must be named directly, not through aliases; use '%s', not '%s'",
+                target.getLabel(), label));
+        aliasFound = true;
+      }
+
       String expectedValue = expectedValues.get(label);
       String actualValue = ConfigFeatureFlagProvider.fromTarget(target).getValue();
 
@@ -75,6 +86,9 @@ public static ImmutableMap<Label, String> getAndValidateFlagMapFromRuleContext(
         throw new RuleErrorException();
       }
     }
+    if (aliasFound) {
+      throw new RuleErrorException();
+    }
     return ImmutableMap.copyOf(expectedValues);
   }
 

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
@@ -14,8 +14,11 @@
 
 package com.google.devtools.build.lib.rules.config;
 
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
@@ -34,13 +37,15 @@
 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.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.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -69,9 +74,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
         ruleContext.getPrerequisites(
             ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET);
 
-    ImmutableMap<Label, ConfigFeatureFlagProvider> configProviders =
-        buildConfigFeatureFlagMap(flagValues);
-
     if (settings.isEmpty() && flagSettings.isEmpty()) {
       ruleContext.ruleError(
           String.format(
@@ -81,7 +83,9 @@ public ConfiguredTarget create(RuleContext ruleContext)
       return null;
     }
 
-    boolean flagSettingsMatch = matchesUserConfig(flagSettings, configProviders, ruleContext);
+    ConfigFeatureFlagMatch featureFlags =
+        ConfigFeatureFlagMatch.fromAttributeValueAndPrerequisites(
+            flagSettings, flagValues, (RuleErrorConsumer) ruleContext);
 
     boolean settingsMatch =
         matchesConfig(
@@ -95,7 +99,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
 
     ConfigMatchingProvider configMatcher =
         new ConfigMatchingProvider(
-            ruleContext.getLabel(), settings, flagSettings, settingsMatch && flagSettingsMatch);
+            ruleContext.getLabel(),
+            settings,
+            featureFlags.getSpecifiedFlagValues(),
+            settingsMatch && featureFlags.matches());
 
     return new RuleConfiguredTargetBuilder(ruleContext)
         .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY)
@@ -106,52 +113,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
         .build();
   }
 
-  /** Maps the labels of the given prerequisites to their {@link ConfigFeatureFlagProvider}s. */
-  private static ImmutableMap<Label, ConfigFeatureFlagProvider> buildConfigFeatureFlagMap(
-      Iterable<? extends TransitiveInfoCollection> prerequisites) {
-    ImmutableMap.Builder<Label, ConfigFeatureFlagProvider> output = new ImmutableMap.Builder<>();
-
-    for (TransitiveInfoCollection target : prerequisites) {
-      ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.fromTarget(target);
-      // We know the provider exists because only labels with ConfigFeatureFlagProvider can be added
-      // to this attribute.
-      assert provider != null;
-      output.put(target.getLabel(), provider);
-    }
-
-    return output.build();
-  }
-
-  /** Returns whether the actual user-defined flags are set to the specified values. */
-  private static boolean matchesUserConfig(
-      Map<Label, String> specifiedFlags,
-      Map<Label, ConfigFeatureFlagProvider> actualFlags,
-      RuleErrorConsumer errors) {
-    boolean foundMismatch = false;
-    for (Map.Entry<Label, String> specifiedFlag : specifiedFlags.entrySet()) {
-      Label label = specifiedFlag.getKey();
-      String specifiedValue = specifiedFlag.getValue();
-      ConfigFeatureFlagProvider provider = actualFlags.get(label);
-      // Both specifiedFlags and actualFlags are built from the same set of keys; therefore, the
-      // provider will always be present.
-      assert provider != null;
-      if (!provider.isValidValue(specifiedValue)) {
-        errors.attributeError(
-            ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
-            String.format(
-                "error while parsing user-defined configuration values: "
-                    + "'%s' is not a valid value for '%s'",
-                specifiedValue, label));
-        foundMismatch = true;
-        continue;
-      }
-      if (!provider.getValue().equals(specifiedValue)) {
-        foundMismatch = true;
-      }
-    }
-    return !foundMismatch;
-  }
-
   /**
    * Given a list of [flagName, flagValue] pairs, returns true if flagName == flagValue for every
    * item in the list under this configuration, false otherwise.
@@ -258,4 +219,98 @@ private static boolean optionMatches(
     // Multi-value list:
     return actualList.contains(expectedSingleValue);
   }
+
+  private static final class ConfigFeatureFlagMatch {
+    private final boolean matches;
+    private final ImmutableMap<Label, String> specifiedFlagValues;
+
+    private static final Joiner QUOTED_COMMA_JOINER = Joiner.on("', '");
+
+    private ConfigFeatureFlagMatch(
+        boolean matches, ImmutableMap<Label, String> specifiedFlagValues) {
+      this.matches = matches;
+      this.specifiedFlagValues = specifiedFlagValues;
+    }
+
+    /** Returns whether the specified flag values matched the actual flag values. */
+    public boolean matches() {
+      return matches;
+    }
+
+    /** Gets the specified flag values, with aliases converted to their original targets' labels. */
+    public ImmutableMap<Label, String> getSpecifiedFlagValues() {
+      return specifiedFlagValues;
+    }
+
+    /** Groups aliases in the list of prerequisites by the target they point to. */
+    private static ListMultimap<Label, Label> collectAliases(
+        Iterable<? extends TransitiveInfoCollection> prerequisites) {
+      ImmutableListMultimap.Builder<Label, Label> targetsToAliases =
+          new ImmutableListMultimap.Builder<>();
+      for (TransitiveInfoCollection target : prerequisites) {
+        targetsToAliases.put(target.getLabel(), AliasProvider.getDependencyLabel(target));
+      }
+      return targetsToAliases.build();
+    }
+
+    public static ConfigFeatureFlagMatch fromAttributeValueAndPrerequisites(
+        Map<Label, String> attributeValue,
+        Iterable<? extends TransitiveInfoCollection> prerequisites,
+        RuleErrorConsumer errors) {
+      Map<Label, String> specifiedFlagValues = new LinkedHashMap<>();
+      boolean matches = true;
+      boolean foundDuplicate = false;
+
+      for (TransitiveInfoCollection target : prerequisites) {
+        ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.fromTarget(target);
+        // We know the provider exists because only labels with ConfigFeatureFlagProvider can be
+        // added to this attribute.
+        assert provider != null;
+
+        Label actualLabel = target.getLabel();
+        Label specifiedLabel = AliasProvider.getDependencyLabel(target);
+        String specifiedValue = attributeValue.get(specifiedLabel);
+        if (specifiedFlagValues.containsKey(actualLabel)) {
+          foundDuplicate = true;
+        }
+        specifiedFlagValues.put(actualLabel, specifiedValue);
+
+        if (!provider.isValidValue(specifiedValue)) {
+          errors.attributeError(
+              ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
+              String.format(
+                  "error while parsing user-defined configuration values: "
+                      + "'%s' is not a valid value for '%s'",
+                  specifiedValue, specifiedLabel));
+          matches = false;
+          continue;
+        }
+        if (!provider.getValue().equals(specifiedValue)) {
+          matches = false;
+        }
+      }
+
+      // attributeValue is the source of the prerequisites in prerequisites, so the final map built
+      // from iterating over prerequisites should always be the same size, barring duplicates.
+      assert foundDuplicate || attributeValue.size() == specifiedFlagValues.size();
+
+      if (foundDuplicate) {
+        ListMultimap<Label, Label> aliases = collectAliases(prerequisites);
+        for (Label actualLabel : aliases.keySet()) {
+          List<Label> aliasList = aliases.get(actualLabel);
+          if (aliasList.size() > 1) {
+            errors.attributeError(
+                ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
+                String.format(
+                    "flag '%s' referenced multiple times as ['%s']",
+                    actualLabel, QUOTED_COMMA_JOINER.join(aliasList)));
+          }
+        }
+
+        matches = false;
+      }
+
+      return new ConfigFeatureFlagMatch(matches, ImmutableMap.copyOf(specifiedFlagValues));
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -2761,6 +2761,36 @@ public void testFeatureFlagsAttributeFailsAnalysisIfFlagValueIsInvalidEvenIfNotU
             + "value must be one of ['off', 'on'], but was 'invalid'");
   }
 
+  @Test
+  public void testFeatureFlagsAttributeFailsAnalysisIfFlagIsAliased()
+      throws Exception {
+    reporter.removeHandler(failFastHandler);
+    useConfiguration("--experimental_dynamic_configs=on");
+    scratch.file(
+        "java/com/foo/BUILD",
+        "config_feature_flag(",
+        "  name = 'flag1',",
+        "  allowed_values = ['on', 'off'],",
+        "  default_value = 'off',",
+        ")",
+        "alias(",
+        "  name = 'alias',",
+        "  actual = 'flag1',",
+        ")",
+        "android_binary(",
+        "  name = 'foo',",
+        "  manifest = 'AndroidManifest.xml',",
+        "  feature_flags = {",
+        "    'alias': 'on',",
+        "  }",
+        ")");
+    assertThat(getConfiguredTarget("//java/com/foo")).isNull();
+    assertContainsEvent(
+        "in feature_flags attribute of android_binary rule //java/com/foo:foo: "
+            + "Feature flags must be named directly, not through aliases; "
+            + "use '//java/com/foo:flag1', not '//java/com/foo:alias'");
+  }
+
   @Test
   public void testFeatureFlagsAttributeSetsFeatureFlagProviderValues() throws Exception {
     useConfiguration("--experimental_dynamic_configs=on");