Add the LABEL_KEYED_STRING_DICT type for attributes.

This enables both native and Skylark rules to declare attributes which
have labels/Targets as keys, and have string values.

--
PiperOrigin-RevId: 148365033
MOS_MIGRATED_REVID=148365033

12 files changed, 828 insertions, 7 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeFormatter.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeFormatter.java
index 44623f7ef6..4a1c283a3a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AttributeFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeFormatter.java
@@ -17,6 +17,7 @@ import static com.google.devtools.build.lib.packages.BuildType.DISTRIBUTIONS;
 import static com.google.devtools.build.lib.packages.BuildType.FILESET_ENTRY_LIST;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_DICT_UNARY;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_KEYED_STRING_DICT;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static com.google.devtools.build.lib.packages.BuildType.LICENSE;
 import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
@@ -45,6 +46,7 @@ import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.Sele
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.SelectorEntry.Builder;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.Tristate;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.LabelDictUnaryEntry;
+import com.google.devtools.build.lib.query2.proto.proto2api.Build.LabelKeyedStringDictEntry;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.LabelListDictEntry;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.StringDictEntry;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.StringDictUnaryEntry;
@@ -61,7 +63,15 @@ public class AttributeFormatter {
 
   private static final ImmutableSet<Type<?>> depTypes =
       ImmutableSet.<Type<?>>of(
-          STRING, LABEL, OUTPUT, STRING_LIST, LABEL_LIST, OUTPUT_LIST, DISTRIBUTIONS);
+          STRING,
+          LABEL,
+          OUTPUT,
+          STRING_LIST,
+          LABEL_LIST,
+          LABEL_DICT_UNARY,
+          LABEL_KEYED_STRING_DICT,
+          OUTPUT_LIST,
+          DISTRIBUTIONS);
 
   private static final ImmutableSet<Type<?>> noDepTypes =
       ImmutableSet.<Type<?>>of(NODEP_LABEL_LIST, NODEP_LABEL);
@@ -230,6 +240,15 @@ public class AttributeFormatter {
                 .setValue(dictEntry.getValue().toString());
         builder.addLabelDictUnaryValue(entry);
       }
+    } else if (type == LABEL_KEYED_STRING_DICT) {
+      Map<Label, String> dict = (Map<Label, String>) value;
+      for (Map.Entry<Label, String> dictEntry : dict.entrySet()) {
+        LabelKeyedStringDictEntry.Builder entry =
+            LabelKeyedStringDictEntry.newBuilder()
+                .setKey(dictEntry.getKey().toString())
+                .setValue(dictEntry.getValue());
+        builder.addLabelKeyedStringDictValue(entry);
+      }
     } else if (type == FILESET_ENTRY_LIST) {
       List<FilesetEntry> filesetEntries = (List<FilesetEntry>) value;
       for (FilesetEntry filesetEntry : filesetEntries) {
@@ -302,6 +321,8 @@ public class AttributeFormatter {
 
     void addLabelDictUnaryValue(LabelDictUnaryEntry.Builder builder);
 
+    void addLabelKeyedStringDictValue(LabelKeyedStringDictEntry.Builder builder);
+
     void addLabelListDictValue(LabelListDictEntry.Builder builder);
 
     void addIntListValue(int i);
@@ -362,6 +383,11 @@ public class AttributeFormatter {
     }
 
     @Override
+    public void addLabelKeyedStringDictValue(LabelKeyedStringDictEntry.Builder builder) {
+      attributeBuilder.addLabelKeyedStringDictValue(builder);
+    }
+
+    @Override
     public void addLabelListDictValue(LabelListDictEntry.Builder builder) {
       attributeBuilder.addLabelListDictValue(builder);
     }
@@ -488,6 +514,11 @@ public class AttributeFormatter {
     }
 
     @Override
+    public void addLabelKeyedStringDictValue(LabelKeyedStringDictEntry.Builder builder) {
+      selectorEntryBuilder.addLabelKeyedStringDictValue(builder);
+    }
+
+    @Override
     public void addLabelListDictValue(LabelListDictEntry.Builder builder) {
       selectorEntryBuilder.addLabelListDictValue(builder);
     }
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 051b7e033f..263fe63e79 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
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -22,6 +23,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.packages.License.DistributionType;
 import com.google.devtools.build.lib.packages.License.LicenseParsingException;
+import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SelectorValue;
 import com.google.devtools.build.lib.syntax.Type;
@@ -29,6 +31,7 @@ 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.ArrayList;
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -55,6 +58,11 @@ public final class BuildType {
   public static final DictType<String, Label> LABEL_DICT_UNARY = DictType.create(
       Type.STRING, LABEL);
   /**
+   * The type of a dictionary keyed by {@linkplain #LABEL labels} with string values.
+   */
+  public static final DictType<Label, String> LABEL_KEYED_STRING_DICT =
+      LabelKeyedDictType.create(Type.STRING);
+  /**
    *  The type of a list of {@linkplain #LABEL labels}.
    */
   public static final ListType<Label> LABEL_LIST = ListType.create(LABEL);
@@ -247,6 +255,70 @@ public final class BuildType {
   }
 
   /**
+   * Dictionary type specialized for label keys, which is able to detect collisions caused by the
+   * fact that labels have multiple equivalent representations in Skylark code.
+   */
+  private static class LabelKeyedDictType<ValueT> extends DictType<Label, ValueT> {
+    private LabelKeyedDictType(Type<ValueT> valueType) {
+      super(LABEL, valueType, LabelClass.DEPENDENCY);
+    }
+
+    public static <ValueT> LabelKeyedDictType<ValueT> create(Type<ValueT> valueType) {
+      Preconditions.checkArgument(
+          valueType.getLabelClass() == LabelClass.NONE
+          || valueType.getLabelClass() == LabelClass.DEPENDENCY,
+          "Values associated with label keys must not be labels themselves.");
+      return new LabelKeyedDictType<>(valueType);
+    }
+
+    @Override
+    public Map<Label, ValueT> convert(Object x, Object what, Object context)
+        throws ConversionException {
+      Map<Label, ValueT> result = super.convert(x, what, context);
+      // The input is known to be a map because super.convert succeded; otherwise, a
+      // ConversionException would have been thrown.
+      Map<?, ?> input = (Map<?, ?>) x;
+
+      if (input.size() == result.size()) {
+        // No collisions found. Exit early.
+        return result;
+      }
+      // Look for collisions in order to produce a nicer error message.
+      Map<Label, List<Object>> convertedFrom = new LinkedHashMap<>();
+      for (Object original : input.keySet()) {
+        Label label = LABEL.convert(original, what, context);
+        if (!convertedFrom.containsKey(label)) {
+          convertedFrom.put(label, new ArrayList<Object>());
+        }
+        convertedFrom.get(label).add(original);
+      }
+      StringBuilder errorMessage = new StringBuilder();
+      errorMessage.append("duplicate labels");
+      if (what != null) {
+        errorMessage.append(" in ").append(what);
+      }
+      errorMessage.append(':');
+      boolean isFirstEntry = true;
+      for (Map.Entry<Label, List<Object>> entry : convertedFrom.entrySet()) {
+        if (entry.getValue().size() == 1) {
+          continue;
+        }
+        if (isFirstEntry) {
+          isFirstEntry = false;
+        } else {
+          errorMessage.append(',');
+        }
+        errorMessage.append(' ');
+        errorMessage.append(entry.getKey());
+        errorMessage.append(" (as ");
+        Printer.write(errorMessage, entry.getValue());
+        errorMessage.append(')');
+      }
+      throw new ConversionException(errorMessage.toString());
+    }
+  }
+
+  /**
    * Like Label, LicenseType is a derived type, which is declared specially
    * in order to allow syntax validation. It represents the licenses, as
    * described in {@ref License}.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java b/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java
index c34ad2e224..7c0335d406 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java
@@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.packages.BuildType.DISTRIBUTIONS;
 import static com.google.devtools.build.lib.packages.BuildType.FILESET_ENTRY_LIST;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_DICT_UNARY;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_KEYED_STRING_DICT;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static com.google.devtools.build.lib.packages.BuildType.LICENSE;
 import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
@@ -68,6 +69,7 @@ public class ProtoUtils {
           .put(TRISTATE, Discriminator.TRISTATE)
           .put(INTEGER_LIST, Discriminator.INTEGER_LIST)
           .put(STRING_DICT_UNARY, Discriminator.STRING_DICT_UNARY)
+          .put(LABEL_KEYED_STRING_DICT, Discriminator.LABEL_KEYED_STRING_DICT)
           .build();
 
   /** Returns the {@link Discriminator} value corresponding to the provided {@link Type}. */
diff --git a/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java
index 317888d073..66e1182c74 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java
@@ -432,7 +432,8 @@ public class ProtoOutputFormatter extends AbstractUnorderedFormatter {
     if (attrType == Type.STRING_DICT
         || attrType == Type.STRING_DICT_UNARY
         || attrType == Type.STRING_LIST_DICT
-        || attrType == BuildType.LABEL_DICT_UNARY) {
+        || attrType == BuildType.LABEL_DICT_UNARY
+        || attrType == BuildType.LABEL_KEYED_STRING_DICT) {
       Map<Object, Object> mergedDict = new HashMap<>();
       for (Object possibleValue : possibleValues) {
         Map<Object, Object> stringDict = (Map<Object, Object>) possibleValue;
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 08113b6f20..b28b08e07c 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
@@ -922,6 +922,158 @@ public final class SkylarkAttr {
       };
 
   @SkylarkSignature(
+    name = "label_keyed_string_dict",
+    doc =
+        "Creates an attribute which is a <a href=\"dict.html\">dict</a>. Its keys are type "
+            + "<a href=\"Target.html\">Target</a> and are specified by the label keys of the "
+            + "input dict. Its values are <a href=\"string.html\">strings</a>. See "
+            + "<a href=\"attr.html#label\">label</a> for more information.",
+    objectType = SkylarkAttr.class,
+    returnType = Descriptor.class,
+    parameters = {
+      @Param(
+        name = DEFAULT_ARG,
+        type = SkylarkDict.class,
+        callbackEnabled = true,
+        defaultValue = "{}",
+        named = true,
+        positional = false,
+        doc =
+            DEFAULT_DOC
+                + " Use the <a href=\"globals.html#Label\"><code>Label</code></a> function to "
+                + "specify default values ex:</p>"
+                + "<code>attr.label_keyed_string_dict(default = "
+                + "{ Label(\"//a:b\"): \"value\", Label(\"//a:c\"): \"string\" })</code>"
+      ),
+      @Param(
+        name = ALLOW_FILES_ARG, // bool or FileType filter
+        defaultValue = "None",
+        named = true,
+        positional = false,
+        doc = ALLOW_FILES_DOC
+      ),
+      @Param(
+        name = ALLOW_RULES_ARG,
+        type = SkylarkList.class,
+        generic1 = String.class,
+        noneable = true,
+        defaultValue = "None",
+        named = true,
+        positional = false,
+        doc = ALLOW_RULES_DOC
+      ),
+      @Param(
+        name = PROVIDERS_ARG,
+        type = SkylarkList.class,
+        defaultValue = "[]",
+        named = true,
+        positional = false,
+        doc = PROVIDERS_DOC
+      ),
+      @Param(
+        name = FLAGS_ARG,
+        type = SkylarkList.class,
+        generic1 = String.class,
+        defaultValue = "[]",
+        named = true,
+        positional = false,
+        doc = FLAGS_DOC
+      ),
+      @Param(
+        name = MANDATORY_ARG,
+        type = Boolean.class,
+        defaultValue = "False",
+        named = true,
+        positional = false,
+        doc = MANDATORY_DOC
+      ),
+      @Param(
+        name = NON_EMPTY_ARG,
+        type = Boolean.class,
+        defaultValue = "False",
+        named = true,
+        positional = false,
+        doc = NON_EMPTY_DOC
+      ),
+      @Param(
+        name = ALLOW_EMPTY_ARG,
+        type = Boolean.class,
+        defaultValue = "True",
+        doc = ALLOW_EMPTY_DOC
+      ),
+      @Param(
+        name = CONFIGURATION_ARG,
+        type = Object.class,
+        noneable = true,
+        defaultValue = "None",
+        named = true,
+        positional = false,
+        doc = CONFIGURATION_DOC
+      ),
+      @Param(
+        name = ASPECTS_ARG,
+        type = SkylarkList.class,
+        generic1 = SkylarkAspect.class,
+        defaultValue = "[]",
+        named = true,
+        positional = false,
+        doc = ASPECTS_ARG_DOC
+      )
+    },
+    useAst = true,
+    useEnvironment = true
+  )
+  private static BuiltinFunction labelKeyedStringDict =
+      new BuiltinFunction("label_keyed_string_dict") {
+        public Descriptor invoke(
+            Object defaultList,
+            Object allowFiles,
+            Object allowRules,
+            SkylarkList<?> providers,
+            SkylarkList<?> flags,
+            Boolean mandatory,
+            Boolean nonEmpty,
+            Boolean allowEmpty,
+            Object cfg,
+            SkylarkList<?> aspects,
+            FuncallExpression ast,
+            Environment env)
+            throws EvalException {
+          env.checkLoadingOrWorkspacePhase("attr.label_keyed_string_dict", ast.getLocation());
+          SkylarkDict<String, Object> kwargs =
+              EvalUtils.<String, Object>optionMap(
+                  env,
+                  DEFAULT_ARG,
+                  defaultList,
+                  ALLOW_FILES_ARG,
+                  allowFiles,
+                  ALLOW_RULES_ARG,
+                  allowRules,
+                  PROVIDERS_ARG,
+                  providers,
+                  FLAGS_ARG,
+                  flags,
+                  MANDATORY_ARG,
+                  mandatory,
+                  NON_EMPTY_ARG,
+                  nonEmpty,
+                  ALLOW_EMPTY_ARG,
+                  allowEmpty,
+                  CONFIGURATION_ARG,
+                  cfg);
+          try {
+            Attribute.Builder<?> attribute =
+                createAttribute(BuildType.LABEL_KEYED_STRING_DICT, kwargs, ast, env);
+            ImmutableList<SkylarkAspect> skylarkAspects =
+                ImmutableList.copyOf(aspects.getContents(SkylarkAspect.class, "aspects"));
+            return new Descriptor(attribute, skylarkAspects);
+          } catch (EvalException e) {
+            throw new EvalException(ast.getLocation(), e.getMessage(), e);
+          }
+        }
+      };
+
+  @SkylarkSignature(
     name = "bool",
     doc = "Creates an attribute of type bool.",
     objectType = SkylarkAttr.class,
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 5414bd993c..def045239b 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
@@ -344,6 +344,16 @@ public final class SkylarkRuleContext {
           || (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_KEYED_STRING_DICT) {
+        ImmutableMap.Builder<TransitiveInfoCollection, String> builder =
+            new ImmutableMap.Builder<>();
+        Map<Label, String> original = BuildType.LABEL_KEYED_STRING_DICT.cast(val);
+        List<? extends TransitiveInfoCollection> allPrereq =
+            ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK);
+        for (TransitiveInfoCollection prereq : allPrereq) {
+          builder.put(prereq, original.get(prereq.getLabel()));
+        }
+        attrBuilder.put(skyname, SkylarkType.convertToSkylark(builder.build(), null));
       } else if (type == BuildType.LABEL_DICT_UNARY) {
         Map<Label, TransitiveInfoCollection> prereqsByLabel = new LinkedHashMap<>();
         for (TransitiveInfoCollection target
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 387794b40f..fc89927c42 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
@@ -475,7 +475,7 @@ public abstract class Type<T> {
       return new DictType<>(keyType, valueType, labelClass);
     }
 
-    private DictType(Type<KeyT> keyType, Type<ValueT> valueType, LabelClass labelClass) {
+    protected DictType(Type<KeyT> keyType, Type<ValueT> valueType, LabelClass labelClass) {
       this.keyType = keyType;
       this.valueType = valueType;
       this.labelClass = labelClass;
@@ -509,8 +509,7 @@ public abstract class Type<T> {
     public Map<KeyT, ValueT> convert(Object x, Object what, Object context)
         throws ConversionException {
       if (!(x instanceof Map<?, ?>)) {
-        throw new ConversionException(String.format(
-            "Expected a map for dictionary but got a %s", x.getClass().getName()));
+        throw new ConversionException(this, x, what);
       }
       // Order the keys so the return value will be independent of insertion order.
       Map<KeyT, ValueT> result = new TreeMap<>();
diff --git a/src/main/protobuf/build.proto b/src/main/protobuf/build.proto
index f125ad2aa9..fe4534abb8 100644
--- a/src/main/protobuf/build.proto
+++ b/src/main/protobuf/build.proto
@@ -47,6 +47,11 @@ message LabelListDictEntry {
   repeated string value = 2;
 }
 
+message LabelKeyedStringDictEntry {
+  required string key = 1;
+  required string value = 2;
+}
+
 message StringListDictEntry {
   required string key = 1;
   repeated string value = 2;
@@ -132,6 +137,7 @@ message Attribute {
     UNKNOWN = 18;            // unknown type, use only for build extensions
     LABEL_DICT_UNARY = 19;   // label_dict_unary_value
     SELECTOR_LIST = 20;      // selector_list
+    LABEL_KEYED_STRING_DICT = 21; // label_keyed_string_dict
   }
 
   // Values for the TriState field type.
@@ -172,6 +178,7 @@ message Attribute {
     repeated int32 int_list_value = 13;
     repeated StringDictUnaryEntry string_dict_unary_value = 14;
     repeated LabelDictUnaryEntry label_dict_unary_value = 15;
+    repeated LabelKeyedStringDictEntry label_keyed_string_dict_value = 17;
   }
 
   message Selector {
@@ -267,6 +274,9 @@ message Attribute {
   // If this is a label dict unary, each entry will be stored here.
   repeated LabelDictUnaryEntry label_dict_unary_value = 19;
 
+  // If this is a label-keyed string dict, each entry will be stored here.
+  repeated LabelKeyedStringDictEntry label_keyed_string_dict_value = 22;
+
   // If this attribute's value is an expression containing one or more select
   // expressions, then its type is SELECTOR_LIST and a SelectorList will be
   // stored here.
diff --git a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
index 6f5012fa83..978f203e86 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
@@ -53,6 +53,181 @@ public class BuildTypeTest {
   }
 
   @Test
+  public void testLabelKeyedStringDictConvertsToMapFromLabelToString() throws Exception {
+    Map<Object, String> input = new ImmutableMap.Builder<Object, String>()
+        .put("//absolute:label", "absolute value")
+        .put(":relative", "theory of relativity")
+        .put("nocolon", "colonial times")
+        .put("//current/package:explicit", "explicit content")
+        .put(Label.parseAbsolute("//i/was/already/a/label"), "and that's okay")
+        .build();
+    Label context = Label.parseAbsolute("//current/package:this");
+
+    Map<Label, String> expected = new ImmutableMap.Builder<Label, String>()
+        .put(Label.parseAbsolute("//absolute:label"), "absolute value")
+        .put(Label.parseAbsolute("//current/package:relative"), "theory of relativity")
+        .put(Label.parseAbsolute("//current/package:nocolon"), "colonial times")
+        .put(Label.parseAbsolute("//current/package:explicit"), "explicit content")
+        .put(Label.parseAbsolute("//i/was/already/a/label"), "and that's okay")
+        .build();
+
+    assertThat(BuildType.LABEL_KEYED_STRING_DICT.convert(input, null, context))
+        .containsExactlyEntriesIn(expected);
+  }
+
+  @Test
+  public void testLabelKeyedStringDictConvertingStringShouldFail() throws Exception {
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert("//actually/a:label", null, currentRule);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage(
+              "expected value of type 'dict(label, string)', "
+                  + "but got \"//actually/a:label\" (string)");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictConvertingListShouldFail() throws Exception {
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert(
+          ImmutableList.of("//actually/a:label"), null, currentRule);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage(
+              "expected value of type 'dict(label, string)', "
+                  + "but got [\"//actually/a:label\"] (List)");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictConvertingMapWithNonStringKeyShouldFail() throws Exception {
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert(ImmutableMap.of(1, "OK"), null, currentRule);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage("expected value of type 'string' for dict key element, but got 1 (int)");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictConvertingMapWithNonStringValueShouldFail() throws Exception {
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert(
+          ImmutableMap.of("//actually/a:label", 3), null, currentRule);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage("expected value of type 'string' for dict value element, but got 3 (int)");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictConvertingMapWithInvalidLabelKeyShouldFail()
+      throws Exception {
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert(
+          ImmutableMap.of("//uplevel/references/are:../../forbidden", "OK"), null, currentRule);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage(
+              "invalid label '//uplevel/references/are:../../forbidden' in "
+                  + "dict key element: invalid target name '../../forbidden': "
+                  + "target names may not contain up-level references '..'");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictConvertingMapWithMultipleEquivalentKeysShouldFail()
+      throws Exception {
+    Label context = Label.parseAbsolute("//current/package:this");
+    Map<String, String> input = new ImmutableMap.Builder<String, String>()
+        .put(":reference", "value1")
+        .put("//current/package:reference", "value2")
+        .build();
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert(input, null, context);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage(
+              "duplicate labels: //current/package:reference "
+                  + "(as [\":reference\", \"//current/package:reference\"])");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictConvertingMapWithMultipleSetsOfEquivalentKeysShouldFail()
+      throws Exception {
+    Label context = Label.parseAbsolute("//current/rule:sibling");
+    Map<String, String> input = new ImmutableMap.Builder<String, String>()
+        .put(":rule", "first set")
+        .put("//current/rule:rule", "also first set")
+        .put("//other/package:package", "interrupting rule")
+        .put("//other/package", "interrupting rule's friend")
+        .put("//current/rule", "part of first set but non-contiguous in iteration order")
+        .put("//not/involved/in/any:collisions", "same value")
+        .put("//also/not/involved/in/any:collisions", "same value")
+        .build();
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert(input, null, context);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage(
+              "duplicate labels: //current/rule:rule "
+                  + "(as [\":rule\", \"//current/rule:rule\", \"//current/rule\"]), "
+                  + "//other/package:package "
+                  + "(as [\"//other/package:package\", \"//other/package\"])");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictErrorConvertingMapWithMultipleEquivalentKeysIncludesContext()
+      throws Exception {
+    Label context = Label.parseAbsolute("//current/package:this");
+    Map<String, String> input = new ImmutableMap.Builder<String, String>()
+        .put(":reference", "value1")
+        .put("//current/package:reference", "value2")
+        .build();
+    try {
+      BuildType.LABEL_KEYED_STRING_DICT.convert(input, "flag map", context);
+      fail("Expected a conversion exception to be thrown.");
+    } catch (ConversionException expected) {
+      assertThat(expected)
+          .hasMessage(
+              "duplicate labels in flag map: //current/package:reference "
+                  + "(as [\":reference\", \"//current/package:reference\"])");
+    }
+  }
+
+  @Test
+  public void testLabelKeyedStringDictCollectLabels() throws Exception {
+    Map<Label, String> input = new ImmutableMap.Builder<Label, String>()
+        .put(Label.parseAbsolute("//absolute:label"), "absolute value")
+        .put(Label.parseAbsolute("//current/package:relative"), "theory of relativity")
+        .put(Label.parseAbsolute("//current/package:nocolon"), "colonial times")
+        .put(Label.parseAbsolute("//current/package:explicit"), "explicit content")
+        .put(Label.parseAbsolute("//i/was/already/a/label"), "and that's okay")
+        .build();
+
+    ImmutableList<Label> expected =
+        ImmutableList.of(
+            Label.parseAbsolute("//absolute:label"),
+            Label.parseAbsolute("//current/package:relative"),
+            Label.parseAbsolute("//current/package:nocolon"),
+            Label.parseAbsolute("//current/package:explicit"),
+            Label.parseAbsolute("//i/was/already/a/label"));
+
+    assertThat(collectLabels(BuildType.LABEL_KEYED_STRING_DICT, input))
+        .containsExactlyElementsIn(expected);
+  }
+
+  @Test
   public void testFilesetEntry() throws Exception {
     Label srcDir = Label.create("foo", "src");
     Label entryLabel = Label.create("foo", "entry");
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 18942dcb09..9088a52f71 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -459,6 +459,45 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
   }
 
   @Test
+  public void labelKeyedStringDictAllowsAspects() throws Exception {
+    scratch.file(
+        "test/aspect.bzl",
+        "def _aspect_impl(target, ctx):",
+        "   return struct(aspect_data=target.label.name)",
+        "",
+        "def _rule_impl(ctx):",
+        "   return struct(",
+        "       data=','.join(['{}:{}'.format(dep.aspect_data, val)",
+        "                      for dep, val in ctx.attr.attr.items()]))",
+        "",
+        "MyAspect = aspect(",
+        "   implementation=_aspect_impl,",
+        ")",
+        "my_rule = rule(",
+        "   implementation=_rule_impl,",
+        "   attrs = { 'attr' : ",
+        "             attr.label_keyed_string_dict(aspects = [MyAspect]) },",
+        ")");
+
+    scratch.file(
+        "test/BUILD",
+        "load('/test/aspect', 'my_rule')",
+        "java_library(",
+        "     name = 'yyy',",
+        ")",
+        "my_rule(",
+        "     name = 'xxx',",
+        "     attr = {':yyy': 'zzz'},",
+        ")");
+
+    AnalysisResult analysisResult = update("//test:xxx");
+    ConfiguredTarget target = analysisResult.getTargetsToBuild().iterator().next();
+    SkylarkProviders skylarkProviders = target.getProvider(SkylarkProviders.class);
+    Object value = skylarkProviders.getValue("data");
+    assertThat(value).isEqualTo("yyy:zzz");
+  }
+
+  @Test
   public void aspectsDoNotAttachToFiles() throws Exception {
     FileSystemUtils.appendIsoLatin1(scratch.resolve("WORKSPACE"),
         "bind(name = 'yyy', actual = '//test:zzz.jar')");
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 578db1b98f..b5d7108dd7 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
@@ -775,6 +775,335 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
   }
 
   @Test
+  public void testLabelKeyedStringDictConvertsToTargetToStringMap() throws Exception {
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(),",
+        "  }",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "filegroup(name='dep')",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r',",
+        "        label_dict={':dep': 'value'})");
+
+    invalidatePackages();
+    SkylarkRuleContext context = createRuleContext("//:r");
+    Label keyLabel =
+        (Label) evalRuleContextCode(context, "ruleContext.attr.label_dict.keys()[0].label");
+    assertThat(keyLabel).isEqualTo(Label.parseAbsolute("//:dep"));
+    String valueString =
+        (String) evalRuleContextCode(context, "ruleContext.attr.label_dict.values()[0]");
+    assertThat(valueString).isEqualTo("value");
+  }
+
+  @Test
+  public void testLabelKeyedStringDictAcceptsDefaultValues() throws Exception {
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(default={Label('//:default'): 'defs'}),",
+        "  }",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "filegroup(name='default')",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r')");
+
+    invalidatePackages();
+    SkylarkRuleContext context = createRuleContext("//:r");
+    Label keyLabel =
+        (Label) evalRuleContextCode(context, "ruleContext.attr.label_dict.keys()[0].label");
+    assertThat(keyLabel).isEqualTo(Label.parseAbsolute("//:default"));
+    String valueString =
+        (String) evalRuleContextCode(context, "ruleContext.attr.label_dict.values()[0]");
+    assertThat(valueString).isEqualTo("defs");
+  }
+
+  @Test
+  public void testLabelKeyedStringDictAllowsFilesWhenAllowFilesIsTrue() throws Exception {
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(allow_files=True),",
+        "  }",
+        ")");
+
+    scratch.file("myfile.cc");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r',",
+        "        label_dict={'myfile.cc': 'value'})");
+
+    invalidatePackages();
+    createRuleContext("//:r");
+    assertNoEvents();
+  }
+
+  @Test
+  public void testLabelKeyedStringDictAllowsFilesOfAppropriateTypes() throws Exception {
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(allow_files=['.cc']),",
+        "  }",
+        ")");
+
+    scratch.file("myfile.cc");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r',",
+        "        label_dict={'myfile.cc': 'value'})");
+
+    invalidatePackages();
+    createRuleContext("//:r");
+    assertNoEvents();
+  }
+
+  @Test
+  public void testLabelKeyedStringDictForbidsFilesOfIncorrectTypes() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(allow_files=['.cc']),",
+        "  }",
+        ")");
+
+    scratch.file("myfile.cpp");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r',",
+        "        label_dict={'myfile.cpp': 'value'})");
+
+    invalidatePackages();
+    getConfiguredTarget("//:r");
+    assertContainsEvent("file '//:myfile.cpp' is misplaced here (expected .cc)");
+  }
+
+  @Test
+  public void testLabelKeyedStringDictForbidsFilesWhenAllowFilesIsFalse() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(allow_files=False),",
+        "  }",
+        ")");
+
+    scratch.file("myfile.cpp");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r',",
+        "        label_dict={'myfile.cpp': 'value'})");
+
+    invalidatePackages();
+    getConfiguredTarget("//:r");
+    assertContainsEvent("in label_dict attribute of my_rule rule //:r: "
+        + "file '//:myfile.cpp' is misplaced here (expected no files)");
+  }
+
+  @Test
+  public void testLabelKeyedStringDictAllowsRulesWithRequiredProviders() throws Exception {
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(providers=[['my_provider']]),",
+        "  }",
+        ")",
+        "def _dep_impl(ctx):",
+        "  return struct(my_provider=5)",
+        "my_dep_rule = rule(",
+        "  implementation = _dep_impl,",
+        "  attrs = {}",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule', 'my_dep_rule')",
+        "my_dep_rule(name='dep')",
+        "my_rule(name='r',",
+        "        label_dict={':dep': 'value'})");
+
+    invalidatePackages();
+    createRuleContext("//:r");
+    assertNoEvents();
+  }
+
+  @Test
+  public void testLabelKeyedStringDictForbidsRulesMissingRequiredProviders() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(providers=[['my_provider']]),",
+        "  }",
+        ")",
+        "def _dep_impl(ctx):",
+        "  return",
+        "my_dep_rule = rule(",
+        "  implementation = _dep_impl,",
+        "  attrs = {}",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule', 'my_dep_rule')",
+        "my_dep_rule(name='dep')",
+        "my_rule(name='r',",
+        "        label_dict={':dep': 'value'})");
+
+    invalidatePackages();
+    getConfiguredTarget("//:r");
+    assertContainsEvent("in label_dict attribute of my_rule rule //:r: "
+        + "'//:dep' does not have mandatory provider 'my_provider'");
+  }
+
+  @Test
+  public void testLabelKeyedStringDictForbidsEmptyDictWhenAllowEmptyIsFalse() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(allow_empty=False),",
+        "  }",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r',",
+        "        label_dict={})");
+
+    invalidatePackages();
+    getConfiguredTarget("//:r");
+    assertContainsEvent("in label_dict attribute of my_rule rule //:r: "
+        + "attribute must be non empty");
+  }
+
+  @Test
+  public void testLabelKeyedStringDictAllowsEmptyDictWhenAllowEmptyIsTrue() throws Exception {
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(allow_empty=True),",
+        "  }",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r',",
+        "        label_dict={})");
+
+    invalidatePackages();
+    createRuleContext("//:r");
+    assertNoEvents();
+  }
+
+  @Test
+  public void testLabelKeyedStringDictForbidsMissingAttributeWhenMandatoryIsTrue()
+      throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(mandatory=True),",
+        "  }",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r')");
+
+    invalidatePackages();
+    getConfiguredTarget("//:r");
+    assertContainsEvent("missing value for mandatory attribute 'label_dict' in 'my_rule' rule");
+  }
+
+  @Test
+  public void testLabelKeyedStringDictAllowsMissingAttributeWhenMandatoryIsFalse()
+      throws Exception {
+    scratch.file(
+        "my_rule.bzl",
+        "def _impl(ctx):",
+        "  return",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'label_dict': attr.label_keyed_string_dict(mandatory=False),",
+        "  }",
+        ")");
+
+    scratch.file(
+        "BUILD",
+        "load('//:my_rule.bzl', 'my_rule')",
+        "my_rule(name='r')");
+
+    invalidatePackages();
+    createRuleContext("//:r");
+    assertNoEvents();
+  }
+
+  @Test
   public void testLabelAttributeDefault() throws Exception {
     scratch.file(
         "my_rule.bzl",
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
index a0e36fc7bd..a4e42c7a32 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
@@ -493,7 +493,8 @@ public class TypeTest {
       Type.STRING_DICT.convert("some string", null);
       fail();
     } catch (ConversionException e) {
-      assertThat(e).hasMessage("Expected a map for dictionary but got a java.lang.String");
+      assertThat(e).hasMessage(
+          "expected value of type 'dict(string, string)', but got \"some string\" (string)");
     }
   }
 
@@ -509,4 +510,4 @@ public class TypeTest {
     }, value);
     return result.build();
   }
-}
\ No newline at end of file
+}