Refactoring: Types report what class of labels they contain.

Currently label-type attributes are detected in many places across the
codebase by simply reference-comparing against each of the label types.
This CL aims to generalize most of these cases, moving the encoding of
this logic into a single place (Type/BuildType itself). Not all of these
cases can be made general without further refactoring, and some perhaps
shouldn't be - serialization and Skylark rule context, for example, need
to do exotic things based on the type. But most sites can avoid having to
enumerate all the types they work with explicitly.

This causes LABEL_DICT_UNARY to start being treated like the other label
types, which means that CcToolchainSuiteRule and JavaRuntimeSuiteRule
need to include a set of allowed file types (none, in their case).
Skylark will continue treating it as a dictionary from String to Label
in its rule context, however, to avoid visible behavior changes.

--
PiperOrigin-RevId: 147471542
MOS_MIGRATED_REVID=147471542

13 files changed, 199 insertions, 81 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 937cf74c45..656ca2595f 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
@@ -81,6 +81,7 @@ import com.google.devtools.build.lib.rules.fileset.FilesetProvider;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -1083,8 +1084,7 @@ public final class RuleContext extends TargetContext
       throw new IllegalStateException(getRule().getLocation() + ": " + getRuleClassNameForLogging()
         + " attribute " + attributeName + " is not defined");
     }
-    if (!(attributeDefinition.getType() == BuildType.LABEL
-        || attributeDefinition.getType() == BuildType.LABEL_LIST)) {
+    if (attributeDefinition.getType().getLabelClass() != LabelClass.DEPENDENCY) {
       throw new IllegalStateException(getRuleClassNameForLogging() + " attribute " + attributeName
         + " is not a label type attribute");
     }
@@ -1125,8 +1125,7 @@ public final class RuleContext extends TargetContext
       throw new IllegalStateException(getRule().getLocation() + ": " + getRuleClassNameForLogging()
         + " attribute " + attributeName + " is not defined");
     }
-    if (!(attributeDefinition.getType() == BuildType.LABEL
-        || attributeDefinition.getType() == BuildType.LABEL_LIST)) {
+    if (attributeDefinition.getType().getLabelClass() != LabelClass.DEPENDENCY) {
       throw new IllegalStateException(getRuleClassNameForLogging() + " attribute " + attributeName
         + " is not a label type attribute");
     }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
index 86d736d60a..33f7d348f6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
@@ -36,6 +36,8 @@ import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
+import com.google.devtools.build.lib.syntax.Type.LabelVisitor;
 import com.google.devtools.build.lib.util.Preconditions;
 
 import java.util.Collection;
@@ -739,7 +741,7 @@ public class ConstraintSemantics {
     AttributeMap attributes = ruleContext.attributes();
     for (String attr : attributes.getAttributeNames()) {
       Attribute attrDef = attributes.getAttributeDefinition(attr);
-      if ((attrDef.getType() != BuildType.LABEL && attrDef.getType() != BuildType.LABEL_LIST)
+      if (attrDef.getType().getLabelClass() != LabelClass.DEPENDENCY
           || attrDef.skipConstraintsOverride()) {
         continue;
       }
@@ -819,21 +821,22 @@ public class ConstraintSemantics {
    * Adds all label values from the given select to the given set. Automatically handles different
    * value types (e.g. labels vs. label lists).
    */
-  private static void addSelectValuesToSet(BuildType.Selector<?> select, Set<Label> set) {
+  private static void addSelectValuesToSet(BuildType.Selector<?> select, final Set<Label> set) {
     Type<?> type = select.getOriginalType();
-    if (type == BuildType.LABEL || type == BuildType.NODEP_LABEL) {
-      set.addAll(((BuildType.Selector<Label>) select).getEntries().values());
-    } else if (type == BuildType.LABEL_LIST ||  type == BuildType.NODEP_LABEL_LIST) {
-      for (List<Label> labels : ((BuildType.Selector<List<Label>>) select).getEntries().values()) {
-        set.addAll(labels);
+    LabelVisitor visitor = new LabelVisitor() {
+      @Override
+      public void visit(Label label) {
+        set.add(label);
       }
-    } else if (type == BuildType.LABEL_DICT_UNARY) {
-      for (Map<String, Label> mapEntry :
-          ((BuildType.Selector<Map<String, Label>>) select).getEntries().values()) {
-        set.addAll(mapEntry.values());
+    };
+    for (Object value : select.getEntries().values()) {
+      try {
+        type.visitLabels(visitor, value);
+      } catch (InterruptedException ex) {
+        // Because the LabelVisitor does not throw InterruptedException, it should not be thrown
+        // by visitLabels here.
+        throw new AssertionError(ex);
       }
-    } else {
-      throw new IllegalStateException("Expected a label-based type for this select");
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
index 338d4bfce8..a305e515d2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -25,11 +25,12 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
+import com.google.devtools.build.lib.syntax.Type.LabelVisitor;
 import com.google.devtools.build.lib.util.Preconditions;
 import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -190,32 +191,36 @@ public final class AspectDefinition {
    * Collects all attribute labels from the specified aspectDefinition.
    */
   public static void addAllAttributesOfAspect(
-      Rule from,
-      Multimap<Attribute, Label> labelBuilder,
+      final Rule from,
+      final Multimap<Attribute, Label> labelBuilder,
       Aspect aspect,
       DependencyFilter dependencyFilter) {
     ImmutableMap<String, Attribute> attributes = aspect.getDefinition().getAttributes();
-    for (Attribute aspectAttribute : attributes.values()) {
+    for (final Attribute aspectAttribute : attributes.values()) {
       if (!dependencyFilter.apply(aspect, aspectAttribute)) {
         continue;
       }
-      if (aspectAttribute.getType() == BuildType.LABEL) {
-        Label label = maybeGetRepositoryRelativeLabel(
-            from, BuildType.LABEL.cast(aspectAttribute.getDefaultValue(from)));
-        if (label != null) {
-          labelBuilder.put(aspectAttribute, label);
-        }
-      } else if (aspectAttribute.getType() == BuildType.LABEL_LIST) {
-        List<Label> defaultLabels = BuildType.LABEL_LIST.cast(
+      Type type = aspectAttribute.getType();
+      if (type.getLabelClass() != LabelClass.DEPENDENCY) {
+        continue;
+      }
+      try {
+        type.visitLabels(
+            new LabelVisitor() {
+              @Override
+              public void visit(Label label) {
+                Label repositoryRelative = maybeGetRepositoryRelativeLabel(from, label);
+                if (repositoryRelative == null) {
+                  return;
+                }
+                labelBuilder.put(aspectAttribute, repositoryRelative);
+              }
+            },
             aspectAttribute.getDefaultValue(from));
-        if (defaultLabels != null) {
-          for (Label defaultLabel : defaultLabels) {
-            Label label = maybeGetRepositoryRelativeLabel(from, defaultLabel);
-            if (label != null) {
-              labelBuilder.put(aspectAttribute, label);
-            }
-          }
-        }
+      } catch (InterruptedException ex) {
+        // Because the LabelVisitor does not throw InterruptedException, it should not be thrown
+        // by visitLabels here.
+        throw new AssertionError(ex);
       }
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index 2288a76210..f5e9b95eb9 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -36,6 +36,7 @@ import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkCallbackFunction;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -507,7 +508,7 @@ public final class Attribute implements Comparable<Attribute> {
      * Makes the built attribute producing a single artifact.
      */
     public Builder<TYPE> singleArtifact() {
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "attribute '%s' must be a label-valued type", name);
       return setPropertyFlag(PropertyFlag.SINGLE_ARTIFACT, "single_artifact");
     }
@@ -517,7 +518,7 @@ public final class Attribute implements Comparable<Attribute> {
      * This flag is introduced to handle plugins, do not use it in other cases.
      */
     public Builder<TYPE> silentRuleClassFilter() {
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "must be a label-valued type");
       return setPropertyFlag(PropertyFlag.SILENT_RULECLASS_FILTER, "silent_ruleclass_filter");
     }
@@ -526,7 +527,7 @@ public final class Attribute implements Comparable<Attribute> {
      * Skip analysis time filetype check. Don't use it if avoidable.
      */
     public Builder<TYPE> skipAnalysisTimeFileTypeCheck() {
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "must be a label-valued type");
       return setPropertyFlag(PropertyFlag.SKIP_ANALYSIS_TIME_FILETYPE_CHECK,
           "skip_analysis_time_filetype_check");
@@ -801,7 +802,7 @@ public final class Attribute implements Comparable<Attribute> {
      * other words, it works for 'deps' attributes, but not 'srcs' attributes.
      */
     public Builder<TYPE> allowedRuleClasses(Predicate<RuleClass> allowedRuleClasses) {
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "must be a label-valued type");
       propertyFlags.add(PropertyFlag.STRICT_LABEL_CHECKING);
       allowedRuleClassesForLabels = allowedRuleClasses;
@@ -831,7 +832,7 @@ public final class Attribute implements Comparable<Attribute> {
      * other words, it works for 'deps' attributes, but not 'srcs' attributes.
      */
     public Builder<TYPE> allowedFileTypes(FileTypeSet allowedFileTypes) {
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "must be a label-valued type");
       propertyFlags.add(PropertyFlag.STRICT_LABEL_CHECKING);
       allowedFileTypesForLabels = Preconditions.checkNotNull(allowedFileTypes);
@@ -886,7 +887,7 @@ public final class Attribute implements Comparable<Attribute> {
      * other words, it works for 'deps' attributes, but not 'srcs' attributes.
      */
     public Builder<TYPE> allowedRuleClassesWithWarning(Predicate<RuleClass> allowedRuleClasses) {
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "must be a label-valued type");
       propertyFlags.add(PropertyFlag.STRICT_LABEL_CHECKING);
       allowedRuleClassesForLabelsWarning = allowedRuleClasses;
@@ -914,7 +915,7 @@ public final class Attribute implements Comparable<Attribute> {
      */
     public final Builder<TYPE> mandatoryNativeProvidersList(
         Iterable<? extends Iterable<Class<? extends TransitiveInfoProvider>>> providersList) {
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "must be a label-valued type");
       ImmutableList.Builder<ImmutableList<Class<? extends TransitiveInfoProvider>>> listBuilder
           = ImmutableList.builder();
@@ -941,7 +942,7 @@ public final class Attribute implements Comparable<Attribute> {
      */
     public Builder<TYPE> mandatoryProvidersList(
         Iterable<? extends Iterable<SkylarkProviderIdentifier>> providersList){
-      Preconditions.checkState((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST),
+      Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
           "must be a label-valued type");
       ImmutableList.Builder<ImmutableSet<SkylarkProviderIdentifier>> listBuilder
           = ImmutableList.builder();
@@ -1086,13 +1087,13 @@ public final class Attribute implements Comparable<Attribute> {
 
       // do not modify this.allowedFileTypesForLabels, instead create a copy.
       FileTypeSet allowedFileTypesForLabels = this.allowedFileTypesForLabels;
-      if ((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST)) {
+      if (type.getLabelClass() == LabelClass.DEPENDENCY) {
         if (isPrivateAttribute(name) && allowedFileTypesForLabels == null) {
           allowedFileTypesForLabels = FileTypeSet.ANY_FILE;
         }
         Preconditions.checkNotNull(
             allowedFileTypesForLabels, "allowedFileTypesForLabels not set for %s", name);
-      } else if ((type == BuildType.OUTPUT) || (type == BuildType.OUTPUT_LIST)) {
+      } else if (type.getLabelClass() == LabelClass.OUTPUT) {
         // TODO(bazel-team): Set the default to no file type and make explicit calls instead.
         if (allowedFileTypesForLabels == null) {
           allowedFileTypesForLabels = FileTypeSet.ANY_FILE;
@@ -1769,8 +1770,8 @@ public final class Attribute implements Comparable<Attribute> {
     Preconditions.checkNotNull(configTransition);
     Preconditions.checkArgument(
         (configTransition == ConfigurationTransition.NONE && configurator == null)
-        || type == BuildType.LABEL || type == BuildType.LABEL_LIST
-        || type == BuildType.NODEP_LABEL || type == BuildType.NODEP_LABEL_LIST,
+        || type.getLabelClass() == LabelClass.DEPENDENCY
+        || type.getLabelClass() == LabelClass.NONDEP_REFERENCE,
         "Configuration transitions can only be specified for label or label list attributes");
     Preconditions.checkArgument(
         isLateBound(name) == (defaultValue instanceof LateBoundDefault),
@@ -1975,8 +1976,8 @@ public final class Attribute implements Comparable<Attribute> {
    * Returns true if this attribute's value can be influenced by the build configuration.
    */
   public boolean isConfigurable() {
-    return !(type == BuildType.OUTPUT      // Excluded because of Rule#populateExplicitOutputFiles.
-        || type == BuildType.OUTPUT_LIST
+    // Output types are excluded because of Rule#populateExplicitOutputFiles.
+    return !(type.getLabelClass() == LabelClass.OUTPUT
         || getPropertyFlag(PropertyFlag.NONCONFIGURABLE));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
index ae3b1214ed..051b7e033f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.syntax.SelectorValue;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
 import com.google.devtools.build.lib.syntax.Type.DictType;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
 import com.google.devtools.build.lib.syntax.Type.ListType;
 import java.util.Collections;
 import java.util.LinkedHashMap;
@@ -47,7 +48,7 @@ public final class BuildType {
    * attributes that it's worth treating them specially (and providing support
    * for resolution of relative-labels in the <code>convert()</code> method).
    */
-  public static final Type<Label> LABEL = new LabelType();
+  public static final Type<Label> LABEL = new LabelType(LabelClass.DEPENDENCY);
   /**
    * The type of a dictionary of {@linkplain #LABEL labels}.
    */
@@ -62,7 +63,7 @@ public final class BuildType {
    * certain rules want to verify the type of a target referenced by one of their attributes, but
    * if there was a dependency edge there, it would be a circular dependency.
    */
-  public static final Type<Label> NODEP_LABEL = new LabelType();
+  public static final Type<Label> NODEP_LABEL = new LabelType(LabelClass.NONDEP_REFERENCE);
   /**
    *  The type of a list of {@linkplain #NODEP_LABEL labels} that do not cause
    *  dependencies.
@@ -136,8 +137,7 @@ public final class BuildType {
    * Returns whether the specified type is a label type or not.
    */
   public static boolean isLabelType(Type<?> type) {
-    return type == LABEL || type == LABEL_LIST || type == LABEL_DICT_UNARY
-        || type == NODEP_LABEL || type == NODEP_LABEL_LIST || type == FILESET_ENTRY_LIST;
+    return type.getLabelClass() != LabelClass.NONE;
   }
 
   /**
@@ -182,6 +182,11 @@ public final class BuildType {
     }
 
     @Override
+    public LabelClass getLabelClass() {
+      return LabelClass.FILESET_ENTRY;
+    }
+
+    @Override
     public FilesetEntry getDefaultValue() {
       return null;
     }
@@ -195,6 +200,12 @@ public final class BuildType {
   }
 
   private static class LabelType extends Type<Label> {
+    private final LabelClass labelClass;
+
+    LabelType(LabelClass labelClass) {
+      this.labelClass = labelClass;
+    }
+
     @Override
     public Label cast(Object value) {
       return (Label) value;
@@ -216,6 +227,11 @@ public final class BuildType {
     }
 
     @Override
+    public LabelClass getLabelClass() {
+      return labelClass;
+    }
+
+    @Override
     public Label convert(Object x, Object what, Object context)
         throws ConversionException {
       if (x instanceof Label) {
@@ -327,6 +343,11 @@ public final class BuildType {
     }
 
     @Override
+    public LabelClass getLabelClass() {
+      return LabelClass.OUTPUT;
+    }
+
+    @Override
     public String toString() {
       return "output";
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java b/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java
index a5d41ee631..88fe6d4157 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.packages;
 
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.DependencyFilter.AttributeInfoProvider;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
 import com.google.devtools.build.lib.util.BinaryPredicate;
 
 /**
@@ -38,8 +39,8 @@ public abstract class DependencyFilter
       new DependencyFilter() {
     @Override
     public boolean apply(AttributeInfoProvider infoProvider, Attribute attribute) {
-      // isHostConfiguration() is only defined for labels and label lists.
-      if (attribute.getType() != BuildType.LABEL && attribute.getType() != BuildType.LABEL_LIST) {
+      // getConfigurationTransition() is only defined for labels which introduce a dependency.
+      if (attribute.getType().getLabelClass() != LabelClass.DEPENDENCY) {
         return true;
       }
 
@@ -62,8 +63,7 @@ public abstract class DependencyFilter
       new DependencyFilter() {
     @Override
     public boolean apply(AttributeInfoProvider infoProvider, Attribute attribute) {
-      return attribute.getType() != BuildType.NODEP_LABEL
-          && attribute.getType() != BuildType.NODEP_LABEL_LIST;
+      return attribute.getType().getLabelClass() != LabelClass.NONDEP_REFERENCE;
     }
   };
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
index abec031476..08113b6f20 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
@@ -46,6 +46,7 @@ import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor;
 import com.google.devtools.build.lib.syntax.SkylarkType;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
 import com.google.devtools.build.lib.syntax.UserDefinedFunction;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
@@ -235,7 +236,7 @@ public final class SkylarkAttr {
       Object fileTypesObj = arguments.get(ALLOW_SINGLE_FILE_ARG);
       setAllowedFileTypes(ALLOW_SINGLE_FILE_ARG, fileTypesObj, ast, builder);
       builder.setPropertyFlag("SINGLE_ARTIFACT");
-    } else if (type.equals(BuildType.LABEL) || type.equals(BuildType.LABEL_LIST)) {
+    } else if (type.getLabelClass() == LabelClass.DEPENDENCY) {
       builder.allowedFileTypes(FileTypeSet.NO_FILE);
     }
 
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
index 7e7b69cda1..8dfc1579d8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
@@ -64,6 +64,7 @@ import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkType;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
@@ -206,7 +207,7 @@ public final class SkylarkRuleContext {
       for (Attribute a : attributes) {
         String attrName = a.getName();
         Type<?> type = a.getType();
-        if (type != BuildType.OUTPUT && type != BuildType.OUTPUT_LIST) {
+        if (type.getLabelClass() != LabelClass.OUTPUT) {
           continue;
         }
         ImmutableList.Builder<Artifact> artifactsBuilder = ImmutableList.builder();
@@ -290,7 +291,10 @@ public final class SkylarkRuleContext {
     for (Attribute a : attributes) {
       Type<?> type = a.getType();
       Object val = attributeValueExtractor.apply(a);
-      if (type != BuildType.LABEL && type != BuildType.LABEL_LIST) {
+      // TODO(mstaib): Remove the LABEL_DICT_UNARY special case of this conditional
+      // LABEL_DICT_UNARY was previously not treated as a dependency-bearing type, and was put into
+      // Skylark as a Map<String, Label>; this special case preserves that behavior temporarily.
+      if (type.getLabelClass() != LabelClass.DEPENDENCY || type == BuildType.LABEL_DICT_UNARY) {
         attrBuilder.put(a.getPublicName(), val == null ? Runtime.NONE
             // Attribute values should be type safe
             : SkylarkType.convertToSkylark(val, null));
@@ -336,10 +340,26 @@ public final class SkylarkRuleContext {
           prereq = Runtime.NONE;
         }
         attrBuilder.put(skyname, prereq);
-      } else {
-        // Type.LABEL_LIST
+      } else if (type == BuildType.LABEL_LIST
+          || (type == BuildType.LABEL && a.hasSplitConfigurationTransition())) {
         List<?> allPrereq = ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK);
         attrBuilder.put(skyname, SkylarkList.createImmutable(allPrereq));
+      } else if (type == BuildType.LABEL_DICT_UNARY) {
+        Map<Label, TransitiveInfoCollection> prereqsByLabel = new LinkedHashMap<>();
+        for (TransitiveInfoCollection target
+              : ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK)) {
+          prereqsByLabel.put(target.getLabel(), target);
+        }
+        ImmutableMap.Builder<String, TransitiveInfoCollection> attrValue =
+            new ImmutableMap.Builder<>();
+        for (Map.Entry<String, Label> entry : ((Map<String, Label>) val).entrySet()) {
+          attrValue.put(entry.getKey(), prereqsByLabel.get(entry.getValue()));
+        }
+        attrBuilder.put(skyname, attrValue.build());
+      } else {
+        throw new IllegalArgumentException(
+            "Can't transform attribute " + a.getName() + " of type " + type
+            + " to a Skylark object");
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuiteRule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuiteRule.java
index a08e6e0930..55244b470a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuiteRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuiteRule.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.util.FileTypeSet;
 
 /**
  * Definition of the {@code cc_toolchain_suite} rule.
@@ -34,6 +35,7 @@ public final class CcToolchainSuiteRule implements RuleDefinition {
         .setUndocumented()
         .add(attr("toolchains", BuildType.LABEL_DICT_UNARY)
             .mandatory()
+            .allowedFileTypes(FileTypeSet.NO_FILE)
             .nonconfigurable("Used during configuration creation"))
         .add(attr("proto", Type.STRING)
             .nonconfigurable("Used during configuration creation"))
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeSuiteRule.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeSuiteRule.java
index 8b28e91c29..22de4df4a3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeSuiteRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeSuiteRule.java
@@ -35,7 +35,7 @@ public final class JavaRuntimeSuiteRule implements RuleDefinition {
         /* <!-- #BLAZE_RULE(java_runtime_suite).ATTRIBUTE(runtimes) -->
         A map from each supported architecture to the corresponding <code>java_runtime</code>.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("runtimes", BuildType.LABEL_DICT_UNARY))
+        .add(attr("runtimes", BuildType.LABEL_DICT_UNARY).allowedFileTypes(FileTypeSet.NO_FILE))
         /* <!-- #BLAZE_RULE(java_runtime_suite).ATTRIBUTE(default) -->
         The default <code>java_runtime</code>, used if
         <a href="${link java_runtime_suite.runtimes}"><code>runtimes</code></a>
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Type.java b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
index dbbfc79c42..387794b40f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -139,6 +140,31 @@ public abstract class Type<T> {
    */
   public abstract void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException;
 
+  /** Classifications of labels by their usage. */
+  public enum LabelClass {
+    /** Used for types which are not labels. */
+    NONE,
+    /** Used for types which use labels to declare a dependency. */
+    DEPENDENCY,
+    /**
+     * Used for types which use labels to reference another target but do not declare a dependency,
+     * in cases where doing so would cause a dependency cycle.
+     */
+    NONDEP_REFERENCE,
+    /** Used for types which use labels to declare an output path. */
+    OUTPUT,
+    /**
+     * Used for types which contain Fileset entries, which contain labels but do not produce
+     * normal dependencies.
+     */
+    FILESET_ENTRY
+  }
+
+  /** Returns the class of labels contained by this type, if any. */
+  public LabelClass getLabelClass() {
+    return LabelClass.NONE;
+  }
+
   /**
    * Implementation of concatenation for this type (e.g. "val1 + val2"). Returns null to
    * indicate concatenation isn't supported.
@@ -423,6 +449,8 @@ public abstract class Type<T> {
 
     private final Map<KeyT, ValueT> empty = ImmutableMap.of();
 
+    private final LabelClass labelClass;
+
     @Override
     public void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException {
       for (Entry<KeyT, ValueT> entry : cast(value).entrySet()) {
@@ -433,12 +461,24 @@ public abstract class Type<T> {
 
     public static <KEY, VALUE> DictType<KEY, VALUE> create(
         Type<KEY> keyType, Type<VALUE> valueType) {
-      return new DictType<>(keyType, valueType);
+      LabelClass keyLabelClass = keyType.getLabelClass();
+      LabelClass valueLabelClass = valueType.getLabelClass();
+      Preconditions.checkArgument(
+          keyLabelClass == LabelClass.NONE
+              || valueLabelClass == LabelClass.NONE
+              || keyLabelClass == valueLabelClass,
+          "A DictType's keys and values must be the same class of label if both contain labels, "
+          + "but the key type " + keyType + " contains " + keyLabelClass + " labels, while "
+          + "the value type " + valueType + " contains " + valueLabelClass + " labels.");
+      LabelClass labelClass = (keyLabelClass != LabelClass.NONE) ? keyLabelClass : valueLabelClass;
+
+      return new DictType<>(keyType, valueType, labelClass);
     }
 
-    private DictType(Type<KeyT> keyType, Type<ValueT> valueType) {
+    private DictType(Type<KeyT> keyType, Type<ValueT> valueType, LabelClass labelClass) {
       this.keyType = keyType;
       this.valueType = valueType;
+      this.labelClass = labelClass;
     }
 
     public Type<KeyT> getKeyType() {
@@ -449,6 +489,11 @@ public abstract class Type<T> {
       return valueType;
     }
 
+    @Override
+    public LabelClass getLabelClass() {
+      return labelClass;
+    }
+
     @SuppressWarnings("unchecked")
     @Override
     public Map<KeyT, ValueT> cast(Object value) {
@@ -511,6 +556,11 @@ public abstract class Type<T> {
     }
 
     @Override
+    public LabelClass getLabelClass() {
+      return elemType.getLabelClass();
+    }
+
+    @Override
     public List<ElemT> getDefaultValue() {
       return empty;
     }
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 078d6af669..578db1b98f 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -1383,4 +1383,25 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
         "ruleContext.coverage_instrumented(ruleContext.attr.deps[0])");
     assertThat((Boolean) result).isTrue();
   }
+
+  @Test
+  public void testStringKeyedLabelDictAttributeInSkylarkRuleContext() throws Exception {
+    scratch.file("jvm/BUILD",
+        "java_runtime(name='runtime', srcs=[], java_home='')",
+        "java_runtime_suite(",
+        "  name = 'suite',",
+        "  runtimes = {'x86': ':runtime'},",
+        "  default = ':runtime',",
+        ")");
+
+    invalidatePackages();
+    SkylarkRuleContext ruleContext = createRuleContext("//jvm:suite");
+    assertNoEvents();
+    String keyString =
+        (String) evalRuleContextCode(ruleContext, "ruleContext.attr.runtimes.keys()[0]");
+    assertThat(keyString).isEqualTo("x86");
+    Label valueLabel =
+        (Label) evalRuleContextCode(ruleContext, "ruleContext.attr.runtimes.values()[0]");
+    assertThat(valueLabel).isEqualTo(Label.parseAbsolute("//jvm:runtime"));
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/BuildRuleWithDefaultsBuilder.java b/src/test/java/com/google/devtools/build/lib/testutil/BuildRuleWithDefaultsBuilder.java
index 7db103eba3..e0e6b77407 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/BuildRuleWithDefaultsBuilder.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/BuildRuleWithDefaultsBuilder.java
@@ -20,6 +20,8 @@ import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.LabelClass;
+import com.google.devtools.build.lib.syntax.Type.ListType;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Preconditions;
 
@@ -57,8 +59,8 @@ public class BuildRuleWithDefaultsBuilder extends BuildRuleBuilder {
    */
   private String getDummyFileLabel(String rulePkg, String filePkg, String extension,
       Type<?> attrType) {
-    boolean isInput = (attrType == BuildType.LABEL || attrType == BuildType.LABEL_LIST);
-    String fileName = (isInput ? "dummy_input" : "dummy_output") + extension;
+    boolean isOutput = attrType.getLabelClass() == LabelClass.OUTPUT;
+    String fileName = (isOutput ? "dummy_output" : "dummy_input") + extension;
     generateFiles.add(filePkg + "/" + fileName);
     if (rulePkg.equals(filePkg)) {
       return ":" + fileName;
@@ -122,7 +124,7 @@ public class BuildRuleWithDefaultsBuilder extends BuildRuleBuilder {
       }
     }
     if (label != null) {
-      if (attrType == BuildType.LABEL_LIST || attrType == BuildType.OUTPUT_LIST) {
+      if (attrType instanceof ListType<?>) {
         addMultiValueAttributes(attribute.getName(), label);
       } else {
         setSingleValueAttribute(attribute.getName(), label);
@@ -175,17 +177,10 @@ public class BuildRuleWithDefaultsBuilder extends BuildRuleBuilder {
   public BuildRuleWithDefaultsBuilder populateAttributes(String rulePkg, boolean heuristics) {
     for (Attribute attribute : ruleClass.getAttributes()) {
       if (attribute.isMandatory()) {
-        if (attribute.getType() == BuildType.LABEL_LIST
-            || attribute.getType() == BuildType.OUTPUT_LIST) {
-          if (attribute.isNonEmpty()) {
-            populateLabelAttribute(rulePkg, attribute);
-          } else {
-            // TODO(bazel-team): actually here an empty list would be fine, but BuildRuleBuilder
-            // doesn't support that, and it makes little sense anyway
-            populateLabelAttribute(rulePkg, attribute);
-          }
-        } else if (attribute.getType() == BuildType.LABEL
-            || attribute.getType() == BuildType.OUTPUT) {
+        if (BuildType.isLabelType(attribute.getType())) {
+          // TODO(bazel-team): actually an empty list would be fine in the case where
+          // attribute instanceof ListType && !attribute.isNonEmpty(), but BuildRuleBuilder
+          // doesn't support that, and it makes little sense anyway
           populateLabelAttribute(rulePkg, attribute);
         } else {
           // Non label type attributes