ConfigurationFragmentPolicy: assume Skylark names, allow merging.

In preparation for allowing aspects to have their own configuration fragments
specified, allow ConfigurationFragmentPolicy.Builder to merge with built policies
more easily, setting up SetMultimaps in place of maps of sets. This changes how
named (Skylark) fragments are declared in the RuleContext builder, hopefully to
be a bit easier to write.

In order to do this, make SkylarkModuleNameResolver the only name resolver in use
(because it is the only name resolver which exists) so as to not worry about
collisions of different name resolvers.

This also changes isLegalConfigurationFragment's one-argument form to mean
"legal in ANY configuration" rather than "legal in the target (NONE)
configuration", as that is how it's used by TransitiveTargetFunction. Uses of it to
mean the latter have been revised to be more explicit.

Also in this CL:
* refactor ConfigurationFragmentPolicy to enforce its contracts about which
ConfigurationTransitions are legal
* use containsEntry or containsValue rather than looking in get(key) or values()
for the configuration fragment multimaps
* add tests for ConfigurationFragmentPolicy
* make SkylarkModuleNameResolver a static method

--
MOS_MIGRATED_REVID=112191439

2 files changed, 129 insertions, 76 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicy.java b/src/main/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicy.java
index 929ec0ee69..c75c01b088 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicy.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicy.java
@@ -13,19 +13,14 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages;
 
-import com.google.common.collect.ArrayListMultimap;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Multimap;
+import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.SetMultimap;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
-import com.google.devtools.build.lib.syntax.FragmentClassNameResolver;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 
-import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
-import java.util.LinkedHashMap;
-import java.util.Map;
 import java.util.Set;
 
 /**
@@ -65,35 +60,40 @@ public final class ConfigurationFragmentPolicy {
    * Builder to construct a new ConfigurationFragmentPolicy.
    */
   public static final class Builder {
-    private final Multimap<ConfigurationTransition, Class<?>> requiredConfigurationFragments
-        = ArrayListMultimap.create();
-    private final Map<ConfigurationTransition, ImmutableSet<String>>
-        requiredConfigurationFragmentNames = new LinkedHashMap<>();
+    /**
+     * Sets of configuration fragment classes required by this rule, a set for each configuration.
+     * Duplicate entries will automatically be ignored by the SetMultimap.
+     */
+    private final SetMultimap<ConfigurationTransition, Class<?>> requiredConfigurationFragments
+        = LinkedHashMultimap.create();
+    /**
+     * Sets of configuration fragment names required by this rule, a set for each configuration.
+     * Duplicate entries will automatically be ignored by the SetMultimap.
+     */
+    private final SetMultimap<ConfigurationTransition, String> requiredConfigurationFragmentNames
+        = LinkedHashMultimap.create();
     private MissingFragmentPolicy missingFragmentPolicy = MissingFragmentPolicy.FAIL_ANALYSIS;
-    private FragmentClassNameResolver fragmentNameResolver;
 
     /**
      * Declares that the implementation of the associated rule class requires the given
-     * fragments to be present in this rule's host and target configurations.
+     * fragments to be present in this rule's target configuration only.
      *
      * <p>The value is inherited by subclasses.
      */
     public Builder requiresConfigurationFragments(Collection<Class<?>> configurationFragments) {
-      requiredConfigurationFragments.putAll(ConfigurationTransition.HOST, configurationFragments);
-      requiredConfigurationFragments.putAll(ConfigurationTransition.NONE, configurationFragments);
+      requiresConfigurationFragments(ConfigurationTransition.NONE, configurationFragments);
       return this;
     }
 
     /**
      * Declares that the implementation of the associated rule class requires the given
-     * fragments to be present in this rule's host and target configurations.
+     * fragments to be present in this rule's host configuration only.
      *
      * <p>The value is inherited by subclasses.
      */
-    public Builder requiresConfigurationFragments(Class<?>... configurationFragments) {
-      Collection<Class<?>> asList = new ArrayList();
-      Collections.addAll(asList, configurationFragments);
-      return requiresConfigurationFragments(asList);
+    public Builder requiresHostConfigurationFragments(Collection<Class<?>> configurationFragments) {
+      requiresConfigurationFragments(ConfigurationTransition.HOST, configurationFragments);
+      return this;
     }
 
     /**
@@ -103,11 +103,42 @@ public final class ConfigurationFragmentPolicy {
      *
      * <p>The value is inherited by subclasses.
      */
-    public Builder requiresConfigurationFragments(ConfigurationTransition transition,
-        Class<?>... configurationFragments) {
-      for (Class<?> fragment : configurationFragments) {
-        requiredConfigurationFragments.put(transition, fragment);
-      }
+    private void requiresConfigurationFragments(ConfigurationTransition transition,
+        Collection<Class<?>> configurationFragments) {
+      requiredConfigurationFragments.putAll(transition, configurationFragments);
+    }
+
+    /**
+     * Declares that the implementation of the associated rule class requires the given
+     * fragments to be present in this rule's target configuration only.
+     *
+     * <p>In contrast to {@link #requiresConfigurationFragments(Collection)}, this method takes the
+     * names of fragments (as determined by {@link SkylarkModule.Resolver}) instead of their
+     * classes.
+     *
+     * <p>The value is inherited by subclasses.
+     */
+    public Builder requiresConfigurationFragmentsBySkylarkModuleName(
+        Collection<String> configurationFragmentNames) {
+      requiresConfigurationFragmentsBySkylarkModuleName(
+          ConfigurationTransition.NONE, configurationFragmentNames);
+      return this;
+    }
+
+    /**
+     * Declares that the implementation of the associated rule class requires the given
+     * fragments to be present in this rule's host configuration only.
+     *
+     * <p>In contrast to {@link #requiresHostConfigurationFragments(Collection)}, this method takes
+     * the names of fragments (as determined by {@link SkylarkModule.Resolver}) instead of their
+     * classes.
+     *
+     * <p>The value is inherited by subclasses.
+     */
+    public Builder requiresHostConfigurationFragmentsBySkylarkModuleName(
+        Collection<String> configurationFragmentNames) {
+      requiresConfigurationFragmentsBySkylarkModuleName(
+          ConfigurationTransition.HOST, configurationFragmentNames);
       return this;
     }
 
@@ -116,14 +147,23 @@ public final class ConfigurationFragmentPolicy {
      * configuration. Valid transition values are HOST for the host configuration and NONE for
      * the target configuration.
      *
-     * <p>In contrast to {@link #requiresConfigurationFragments(Class...)}, this method takes the
-     * names of fragments instead of their classes.
+     * <p>In contrast to {@link #requiresConfigurationFragments(Collection)}, this method takes the
+     * names of fragments (as determined by {@link SkylarkModule.Resolver}) instead of their
+     * classes.
+     */
+    private void requiresConfigurationFragmentsBySkylarkModuleName(
+        ConfigurationTransition transition, Collection<String> configurationFragmentNames) {
+      requiredConfigurationFragmentNames.putAll(transition, configurationFragmentNames);
+    }
+
+    /**
+     * Adds the configuration fragments from the {@code other} policy to this Builder.
+     *
+     * <p>Does not change the missing fragment policy.
      */
-    public Builder requiresConfigurationFragments(
-        FragmentClassNameResolver fragmentNameResolver,
-        Map<ConfigurationTransition, ImmutableSet<String>> configurationFragmentNames) {
-      requiredConfigurationFragmentNames.putAll(configurationFragmentNames);
-      this.fragmentNameResolver = fragmentNameResolver;
+    public Builder includeConfigurationFragmentsFrom(ConfigurationFragmentPolicy other) {
+      requiredConfigurationFragments.putAll(other.requiredConfigurationFragments);
+      requiredConfigurationFragmentNames.putAll(other.requiredConfigurationFragmentNames);
       return this;
     }
 
@@ -138,9 +178,8 @@ public final class ConfigurationFragmentPolicy {
 
     public ConfigurationFragmentPolicy build() {
       return new ConfigurationFragmentPolicy(
-          ImmutableMultimap.copyOf(requiredConfigurationFragments),
-          ImmutableMap.copyOf(requiredConfigurationFragmentNames),
-          fragmentNameResolver,
+          ImmutableSetMultimap.copyOf(requiredConfigurationFragments),
+          ImmutableSetMultimap.copyOf(requiredConfigurationFragmentNames),
           missingFragmentPolicy);
     }
   }
@@ -149,35 +188,27 @@ public final class ConfigurationFragmentPolicy {
    * A dictionary that maps configurations (NONE for target configuration, HOST for host
    * configuration) to required configuration fragments.
    */
-  private final ImmutableMultimap<ConfigurationTransition, Class<?>> requiredConfigurationFragments;
+  private final ImmutableSetMultimap<ConfigurationTransition, Class<?>>
+      requiredConfigurationFragments;
 
   /**
    * A dictionary that maps configurations (NONE for target configuration, HOST for host
-   * configuration) to lists of names of required configuration fragments.
+   * configuration) to lists of Skylark module names of required configuration fragments.
    */
-  private final ImmutableMap<ConfigurationTransition, ImmutableSet<String>>
+  private final ImmutableSetMultimap<ConfigurationTransition, String>
       requiredConfigurationFragmentNames;
 
   /**
-   * Used to resolve the names of fragments in order to compare them to values in {@link
-   * #requiredConfigurationFragmentNames}
-   */
-  private final FragmentClassNameResolver fragmentNameResolver;
-
-  /**
    * What to do during analysis if a configuration fragment is missing.
    */
   private final MissingFragmentPolicy missingFragmentPolicy;
 
   private ConfigurationFragmentPolicy(
-      ImmutableMultimap<ConfigurationTransition, Class<?>> requiredConfigurationFragments,
-      ImmutableMap<ConfigurationTransition, ImmutableSet<String>>
-          requiredConfigurationFragmentNames,
-      FragmentClassNameResolver fragmentNameResolver,
+      ImmutableSetMultimap<ConfigurationTransition, Class<?>> requiredConfigurationFragments,
+      ImmutableSetMultimap<ConfigurationTransition, String> requiredConfigurationFragmentNames,
       MissingFragmentPolicy missingFragmentPolicy) {
     this.requiredConfigurationFragments = requiredConfigurationFragments;
     this.requiredConfigurationFragmentNames = requiredConfigurationFragmentNames;
-    this.fragmentNameResolver = fragmentNameResolver;
     this.missingFragmentPolicy = missingFragmentPolicy;
   }
 
@@ -192,16 +223,23 @@ public final class ConfigurationFragmentPolicy {
   /**
    * Checks if the configuration fragment may be accessed (i.e., if it's declared) in the specified
    * configuration (target or host).
+   *
+   * <p>Note that, currently, all native fragments are included regardless of whether they were
+   * specified in the same configuration that was passed.
    */
   public boolean isLegalConfigurationFragment(
       Class<?> configurationFragment, ConfigurationTransition config) {
-    return getRequiredConfigurationFragments().contains(configurationFragment)
+    return requiredConfigurationFragments.containsValue(configurationFragment)
         || hasLegalFragmentName(configurationFragment, config);
   }
 
+  /**
+   * Checks if the configuration fragment may be accessed (i.e., if it's declared) in any
+   * configuration.
+   */
   public boolean isLegalConfigurationFragment(Class<?> configurationFragment) {
-    // NONE means target configuration.
-    return isLegalConfigurationFragment(configurationFragment, ConfigurationTransition.NONE);
+    return requiredConfigurationFragments.containsValue(configurationFragment)
+        || hasLegalFragmentName(configurationFragment);
   }
 
   /**
@@ -210,13 +248,17 @@ public final class ConfigurationFragmentPolicy {
    */
   private boolean hasLegalFragmentName(
       Class<?> configurationFragment, ConfigurationTransition config) {
-    if (fragmentNameResolver == null) {
-      return false;
-    }
+    return requiredConfigurationFragmentNames.containsEntry(
+        config, SkylarkModule.Resolver.resolveName(configurationFragment));
+  }
 
-    String name = fragmentNameResolver.resolveName(configurationFragment);
-    ImmutableSet<String> fragmentNames = requiredConfigurationFragmentNames.get(config);
-    return (name != null && fragmentNames != null && fragmentNames.contains(name));
+  /**
+   * Checks whether the name of the given fragment class was declared as required in any
+   * configuration.
+   */
+  private boolean hasLegalFragmentName(Class<?> configurationFragment) {
+    return requiredConfigurationFragmentNames.containsValue(
+        SkylarkModule.Resolver.resolveName(configurationFragment));
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 9ce41f6cd4..92ea112216 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -32,7 +32,6 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.BuildType.SelectorList;
 import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
@@ -40,7 +39,6 @@ import com.google.devtools.build.lib.packages.RuleFactory.AttributeValuesMap;
 import com.google.devtools.build.lib.syntax.Argument;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.Environment;
-import com.google.devtools.build.lib.syntax.FragmentClassNameResolver;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.GlobList;
 import com.google.devtools.build.lib.syntax.Runtime;
@@ -508,8 +506,8 @@ public final class RuleClass {
         if (parent.preferredDependencyPredicate != Predicates.<String>alwaysFalse()) {
           setPreferredDependencyPredicate(parent.preferredDependencyPredicate);
         }
-        configurationFragmentPolicy.requiresConfigurationFragments(
-            parent.getConfigurationFragmentPolicy().getRequiredConfigurationFragments());
+        configurationFragmentPolicy
+            .includeConfigurationFragmentsFrom(parent.getConfigurationFragmentPolicy());
         configurationFragmentPolicy.setMissingFragmentPolicy(
             parent.getConfigurationFragmentPolicy().getMissingFragmentPolicy());
         supportsConstraintChecking = parent.supportsConstraintChecking;
@@ -580,7 +578,8 @@ public final class RuleClass {
      * <p>The value is inherited by subclasses.
      */
     public Builder requiresConfigurationFragments(Class<?>... configurationFragments) {
-      configurationFragmentPolicy.requiresConfigurationFragments(configurationFragments);
+      configurationFragmentPolicy.requiresConfigurationFragments(
+          ImmutableSet.<Class<?>>copyOf(configurationFragments));
       return this;
     }
 
@@ -591,24 +590,36 @@ public final class RuleClass {
      * <p>The value is inherited by subclasses.
      */
     public Builder requiresHostConfigurationFragments(Class<?>... configurationFragments) {
-      configurationFragmentPolicy
-          .requiresConfigurationFragments(ConfigurationTransition.HOST, configurationFragments);
+      configurationFragmentPolicy.requiresHostConfigurationFragments(
+          ImmutableSet.<Class<?>>copyOf(configurationFragments));
       return this;
     }
 
     /**
-     * Declares the configuration fragments that are required by this rule for the specified
-     * configuration. Valid transition values are HOST for the host configuration and NONE for
-     * the target configuration.
+     * Declares the configuration fragments that are required by this rule for the target
+     * configuration.
      *
      * <p>In contrast to {@link #requiresConfigurationFragments(Class...)}, this method takes the
-     * names of fragments instead of their classes.
+     * Skylark module names of fragments instead of their classes.
      */
-    public Builder requiresConfigurationFragments(
-        FragmentClassNameResolver fragmentNameResolver,
-        Map<ConfigurationTransition, ImmutableSet<String>> configurationFragmentNames) {
-      configurationFragmentPolicy.requiresConfigurationFragments(
-          fragmentNameResolver, configurationFragmentNames);
+    public Builder requiresConfigurationFragmentsBySkylarkModuleName(
+        Collection<String> configurationFragmentNames) {
+      configurationFragmentPolicy
+          .requiresConfigurationFragmentsBySkylarkModuleName(configurationFragmentNames);
+      return this;
+    }
+
+    /**
+     * Declares the configuration fragments that are required by this rule for the host
+     * configuration.
+     *
+     * <p>In contrast to {@link #requiresHostConfigurationFragments(Class...)}, this method takes
+     * Skylark module names of fragments instead of their classes.
+     */
+    public Builder requiresHostConfigurationFragmentsBySkylarkModuleName(
+        Collection<String> configurationFragmentNames) {
+      configurationFragmentPolicy
+          .requiresHostConfigurationFragmentsBySkylarkModuleName(configurationFragmentNames);
       return this;
     }