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

10 files changed, 330 insertions, 171 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 0282489405..6e06ef1f91 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -176,7 +176,7 @@ public final class RuleContext extends TargetContext
     parseFeatures(getConfiguration().getDefaultFeatures(), globallyEnabled, globallyDisabled);
     for (ImmutableMap.Entry<Class<? extends Fragment>, Fragment> entry :
         getConfiguration().getAllFragments().entrySet()) {
-      if (configurationFragmentPolicy.isLegalConfigurationFragment(entry.getKey())) {
+      if (isLegalFragment(entry.getKey())) {
         globallyEnabled.addAll(entry.getValue().configurationEnabledFeatures(this));
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 8ef66797ea..7e8c5b3486 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -56,7 +56,6 @@ import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.rules.test.TestActionBuilder;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
-import com.google.devtools.build.lib.syntax.SkylarkModuleNameResolver;
 import com.google.devtools.build.lib.util.CPU;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.OS;
@@ -1194,7 +1193,7 @@ public final class BuildConfiguration {
     this.actionsEnabled = !actionsDisabled;
     this.fragments = ImmutableSortedMap.copyOf(fragmentsMap, lexicalFragmentSorter);
 
-    this.skylarkVisibleFragments = buildIndexOfVisibleFragments();
+    this.skylarkVisibleFragments = buildIndexOfSkylarkVisibleFragments();
     
     this.buildOptions = buildOptions;
     this.options = buildOptions.get(Options.class);
@@ -1306,12 +1305,11 @@ public final class BuildConfiguration {
 
 
 
-  private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfVisibleFragments() {
+  private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfSkylarkVisibleFragments() {
     ImmutableMap.Builder<String, Class<? extends Fragment>> builder = ImmutableMap.builder();
-    SkylarkModuleNameResolver resolver = new SkylarkModuleNameResolver();
 
     for (Class<? extends Fragment> fragmentClass : fragments.keySet()) {
-      String name = resolver.resolveName(fragmentClass);
+      String name = SkylarkModule.Resolver.resolveName(fragmentClass);
       if (name != null) {
         builder.put(name, fragmentClass);
       }
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;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index cc5898e721..11268945c9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.rules;
 
 import static com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition.DATA;
 import static com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition.HOST;
-import static com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition.NONE;
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
@@ -33,7 +32,6 @@ import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.Constants;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
@@ -87,7 +85,6 @@ import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkCallbackFunction;
 import com.google.devtools.build.lib.syntax.SkylarkList;
-import com.google.devtools.build.lib.syntax.SkylarkModuleNameResolver;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
 import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor;
 import com.google.devtools.build.lib.syntax.Type;
@@ -95,7 +92,6 @@ import com.google.devtools.build.lib.syntax.Type.ConversionException;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 
-import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
@@ -308,31 +304,14 @@ public class SkylarkRuleClassFunctions {
         builder.setOutputToGenfiles();
       }
 
-      registerRequiredFragments(fragments, hostFragments, builder);
+      builder.requiresConfigurationFragmentsBySkylarkModuleName(
+          fragments.getContents(String.class, "fragments"));
+      builder.requiresHostConfigurationFragmentsBySkylarkModuleName(
+          hostFragments.getContents(String.class, "host_fragments"));
       builder.setConfiguredTargetFunction(implementation);
       builder.setRuleDefinitionEnvironment(funcallEnv);
       return new RuleFunction(builder, type, attributes, ast.getLocation());
     }
-
-      private void registerRequiredFragments(
-          SkylarkList fragments, SkylarkList hostFragments, RuleClass.Builder builder)
-          throws EvalException {
-        Map<ConfigurationTransition, ImmutableSet<String>> map = new HashMap<>();
-        addFragmentsToMap(map, fragments, NONE); // NONE represents target configuration
-        addFragmentsToMap(map, hostFragments, HOST);
-
-        builder.requiresConfigurationFragments(new SkylarkModuleNameResolver(), map);
-      }
-
-      private void addFragmentsToMap(
-          Map<ConfigurationTransition, ImmutableSet<String>> map,
-          SkylarkList fragments,
-          ConfigurationTransition config)
-          throws EvalException {
-        if (!fragments.isEmpty()) {
-          map.put(config, ImmutableSet.copyOf(fragments.getContents(String.class, "fragments")));
-        }
-      }
     };
 
   protected static ImmutableList<Pair<String, Descriptor>> attrObjectToAttributesList(
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java
index 92be3ba72b..0d9d63d1ac 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java
@@ -18,6 +18,8 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
+import javax.annotation.Nullable;
+
 /**
  * An annotation to mark Skylark modules or Skylark accessible Java data types.
  * A Skylark modules always corresponds to exactly one Java class.
@@ -33,4 +35,20 @@ public @interface SkylarkModule {
   boolean documented() default true;
 
   boolean namespace() default false;
+
+  /** Helper method to quickly get the SkylarkModule name of a class (if present). */
+  public static final class Resolver {
+    /**
+     * Returns the Skylark name of the given class or null, if the SkylarkModule annotation is not
+     * present.
+     */
+    @Nullable
+    public static String resolveName(Class<?> clazz) {
+      SkylarkModule annotation = clazz.getAnnotation(SkylarkModule.class);
+      return (annotation == null) ? null : annotation.name();
+    }
+
+    /** Utility method only. */
+    private Resolver() {}
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FragmentClassNameResolver.java b/src/main/java/com/google/devtools/build/lib/syntax/FragmentClassNameResolver.java
deleted file mode 100644
index d3be61080f..0000000000
--- a/src/main/java/com/google/devtools/build/lib/syntax/FragmentClassNameResolver.java
+++ /dev/null
@@ -1,33 +0,0 @@
-// Copyright 2015 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.syntax;
-
-import javax.annotation.Nullable;
-
-/**
- * Interface for retrieving the name of a {@link
- * com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment} based on its
- * class.
- * 
- * <p>Classes implementing this specific interface are required when doing look up operations and
- * comparisons of configuration fragments.
- */
-public interface FragmentClassNameResolver {
-  /**
-   * Returns the name of the configuration fragment specified by the given class or null, if this is
-   * not possible.
-   */
-  @Nullable
-  String resolveName(Class<?> clazz);
-}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModuleNameResolver.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModuleNameResolver.java
deleted file mode 100644
index 0f7f23a101..0000000000
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModuleNameResolver.java
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright 2015 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.syntax;
-
-import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
-
-/**
- * Helper class that resolves the name of classes with SkylarkModule annotation.
- */
-public final class SkylarkModuleNameResolver implements FragmentClassNameResolver {
-  /**
-   * Returns the Skylark name of the given class or null, if the SkylarkModule annotation is not
-   * present.
-   */
-  @Override
-  public String resolveName(Class<?> clazz) {
-    SkylarkModule annotation = clazz.getAnnotation(SkylarkModule.class);
-    return (annotation == null) ? null : annotation.name();
-  }
-}
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index d2366813e9..4b622f3860 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -574,6 +574,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:events",
         "//src/main/java/com/google/devtools/build/lib:packages",
+        "//src/main/java/com/google/devtools/build/lib:skylarkinterface",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib:vfs",
         "//src/main/java/com/google/devtools/build/skyframe",
diff --git a/src/test/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicyTest.java b/src/test/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicyTest.java
new file mode 100644
index 0000000000..a3ea359781
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicyTest.java
@@ -0,0 +1,174 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.packages;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
+import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for the ConfigurationFragmentPolicy builder and methods.
+ */
+@RunWith(JUnit4.class)
+public final class ConfigurationFragmentPolicyTest {
+
+  @SkylarkModule(name = "test_fragment", doc = "first fragment")
+  private static final class TestFragment {}
+
+  @SkylarkModule(name = "other_fragment", doc = "second fragment")
+  private static final class OtherFragment {}
+
+  @SkylarkModule(name = "unknown_fragment", doc = "useless waste of permgen")
+  private static final class UnknownFragment {}
+
+  @Test
+  public void testMissingFragmentPolicy() throws Exception {
+    ConfigurationFragmentPolicy policy =
+        new ConfigurationFragmentPolicy.Builder()
+            .setMissingFragmentPolicy(MissingFragmentPolicy.IGNORE)
+            .build();
+
+    assertThat(policy.getMissingFragmentPolicy()).isEqualTo(MissingFragmentPolicy.IGNORE);
+
+    ConfigurationFragmentPolicy otherPolicy =
+        new ConfigurationFragmentPolicy.Builder()
+            .setMissingFragmentPolicy(MissingFragmentPolicy.CREATE_FAIL_ACTIONS)
+            .build();
+
+    assertThat(otherPolicy.getMissingFragmentPolicy())
+        .isEqualTo(MissingFragmentPolicy.CREATE_FAIL_ACTIONS);
+  }
+
+  @Test
+  public void testRequiresConfigurationFragments_AddsToRequiredSet() throws Exception {
+    // Although these aren't configuration fragments, there are no requirements as to what the class
+    // has to be, so...
+    ConfigurationFragmentPolicy policy =
+        new ConfigurationFragmentPolicy.Builder()
+            .requiresConfigurationFragments(ImmutableSet.<Class<?>>of(Integer.class, String.class))
+            .requiresConfigurationFragments(ImmutableSet.<Class<?>>of(String.class, Long.class))
+            .build();
+
+    assertThat(policy.getRequiredConfigurationFragments())
+        .containsExactly(Integer.class, String.class, Long.class);
+  }
+
+  @Test
+  public void testRequiresConfigurationFragments_RequiredAndLegalForSpecifiedConfiguration()
+      throws Exception {
+    ConfigurationFragmentPolicy policy =
+        new ConfigurationFragmentPolicy.Builder()
+            .requiresConfigurationFragments(ImmutableSet.<Class<?>>of(Integer.class))
+            .requiresHostConfigurationFragments(ImmutableSet.<Class<?>>of(Long.class))
+            .build();
+
+    assertThat(policy.getRequiredConfigurationFragments()).containsAllOf(Integer.class, Long.class);
+
+    assertThat(policy.isLegalConfigurationFragment(Integer.class)).isTrue();
+    assertThat(policy.isLegalConfigurationFragment(Integer.class, ConfigurationTransition.NONE))
+        .isTrue();
+    // TODO(mstaib): .isFalse() when dynamic configurations care which configuration a fragment was
+    // specified for
+    assertThat(policy.isLegalConfigurationFragment(Integer.class, ConfigurationTransition.HOST))
+        .isTrue();
+
+    assertThat(policy.isLegalConfigurationFragment(Long.class)).isTrue();
+    // TODO(mstaib): .isFalse() when dynamic configurations care which configuration a fragment was
+    // specified for
+    assertThat(policy.isLegalConfigurationFragment(Long.class, ConfigurationTransition.NONE))
+        .isTrue();
+    assertThat(policy.isLegalConfigurationFragment(Long.class, ConfigurationTransition.HOST))
+        .isTrue();
+
+    assertThat(policy.isLegalConfigurationFragment(String.class)).isFalse();
+    assertThat(policy.isLegalConfigurationFragment(String.class, ConfigurationTransition.NONE))
+        .isFalse();
+    assertThat(policy.isLegalConfigurationFragment(String.class, ConfigurationTransition.HOST))
+        .isFalse();
+  }
+
+  @Test
+  public void testRequiresConfigurationFragments_MapSetsLegalityBySkylarkModuleName_NoRequires()
+      throws Exception {
+    ConfigurationFragmentPolicy policy =
+        new ConfigurationFragmentPolicy.Builder()
+            .requiresConfigurationFragmentsBySkylarkModuleName(ImmutableSet.of("test_fragment"))
+            .requiresHostConfigurationFragmentsBySkylarkModuleName(
+                ImmutableSet.of("other_fragment"))
+            .build();
+
+    assertThat(policy.getRequiredConfigurationFragments()).isEmpty();
+
+    assertThat(policy.isLegalConfigurationFragment(TestFragment.class)).isTrue();
+    assertThat(
+            policy.isLegalConfigurationFragment(TestFragment.class, ConfigurationTransition.NONE))
+        .isTrue();
+    assertThat(
+            policy.isLegalConfigurationFragment(TestFragment.class, ConfigurationTransition.HOST))
+        .isFalse();
+
+    assertThat(policy.isLegalConfigurationFragment(OtherFragment.class)).isTrue();
+    assertThat(
+            policy.isLegalConfigurationFragment(OtherFragment.class, ConfigurationTransition.NONE))
+        .isFalse();
+    assertThat(
+            policy.isLegalConfigurationFragment(OtherFragment.class, ConfigurationTransition.HOST))
+        .isTrue();
+
+    assertThat(policy.isLegalConfigurationFragment(UnknownFragment.class)).isFalse();
+    assertThat(
+            policy.isLegalConfigurationFragment(
+                UnknownFragment.class, ConfigurationTransition.NONE))
+        .isFalse();
+    assertThat(
+            policy.isLegalConfigurationFragment(
+                UnknownFragment.class, ConfigurationTransition.HOST))
+        .isFalse();
+  }
+
+  @Test
+  public void testIncludeConfigurationFragmentsFrom_MergesWithExistingFragmentSet()
+      throws Exception {
+    ConfigurationFragmentPolicy basePolicy =
+        new ConfigurationFragmentPolicy.Builder()
+            .requiresConfigurationFragmentsBySkylarkModuleName(ImmutableSet.of("test_fragment"))
+            .requiresConfigurationFragments(ImmutableSet.<Class<?>>of(Integer.class, Double.class))
+            .build();
+    ConfigurationFragmentPolicy addedPolicy =
+        new ConfigurationFragmentPolicy.Builder()
+            .requiresConfigurationFragmentsBySkylarkModuleName(ImmutableSet.of("other_fragment"))
+            .requiresHostConfigurationFragmentsBySkylarkModuleName(
+                ImmutableSet.of("other_fragment"))
+            .requiresConfigurationFragments(ImmutableSet.<Class<?>>of(Boolean.class))
+            .requiresHostConfigurationFragments(ImmutableSet.<Class<?>>of(Character.class))
+            .build();
+    ConfigurationFragmentPolicy combinedPolicy =
+        new ConfigurationFragmentPolicy.Builder()
+            .includeConfigurationFragmentsFrom(basePolicy)
+            .includeConfigurationFragmentsFrom(addedPolicy)
+            .build();
+
+    assertThat(combinedPolicy.getRequiredConfigurationFragments())
+        .containsExactly(Integer.class, Double.class, Boolean.class, Character.class);
+    assertThat(combinedPolicy.isLegalConfigurationFragment(TestFragment.class)).isTrue();
+    assertThat(combinedPolicy.isLegalConfigurationFragment(OtherFragment.class)).isTrue();
+  }
+}