Forbid duplicate keys in dictionary literals

RELNOTES:
  When using the dictionary literal syntax, it is now an error to have
  duplicated keys (e.g.  {'ab': 3, 'ab': 5}).
PiperOrigin-RevId: 159945431

6 files changed, 57 insertions, 32 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
index 87756990ae..c2328b190c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
@@ -72,11 +72,12 @@ public final class DictionaryLiteral extends Expression {
     SkylarkDict<Object, Object> dict = SkylarkDict.<Object, Object>of(env);
     Location loc = getLocation();
     for (DictionaryEntryLiteral entry : entries) {
-      if (entry == null) {
-        throw new EvalException(loc, "null expression in " + this);
-      }
       Object key = entry.key.eval(env);
       Object val = entry.value.eval(env);
+      if (env.getSemantics().incompatibleDictLiteralHasNoDuplicates && dict.containsKey(key)) {
+        throw new EvalException(
+            loc, "Duplicated key " + Printer.repr(key) + " when creating dictionary");
+      }
       dict.put(key, val, loc, env);
     }
     return dict;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
index d622a7ccce..01f104bacb 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
@@ -22,29 +22,29 @@ import java.util.LinkedHashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
 
-/**
- * Skylark Dict module.
- */
-@SkylarkModule(name = "dict",
-    category = SkylarkModuleCategory.BUILTIN,
-    doc = "A language built-in type representating a dictionary (associative mapping). "
-    + "Dictionaries may be constructed with a special literal syntax:<br>"
-    + "<pre class=\"language-python\">d = {\"a\": 2, \"b\": 5}</pre>"
-    + "Use square brackets to access elements:<br>"
-    + "<pre class=\"language-python\">e = d[\"a\"]   # e == 2</pre>"
-    + "Like lists, they can also be constructed using a comprehension syntax:<br>"
-    + "<pre class=\"language-python\">d = {i: 2*i for i in range(20)}\n"
-    + "e = d[8]       # e == 16</pre>"
-    + "See also the <a href=\"globals.html#dict\">dict()</a> constructor function. "
-    + "<p>Iterating over a dict is equivalent to iterating over its keys. The <code>in</code> "
-    + "operator tests for membership in the keyset of the dict.<br>"
-    + "<pre class=\"language-python\">\"a\" in {\"a\" : 2, \"b\" : 5}   # evaluates as True</pre>"
-    + "The iteration order for a dict is deterministic but not specified. When constructing a dict "
-    + "using any of the above methods, if there are two identical keys with conflicting values "
-    + "then the last value takes precedence."
+/** Skylark Dict module. */
+@SkylarkModule(
+  name = "dict",
+  category = SkylarkModuleCategory.BUILTIN,
+  doc =
+      "A language built-in type representating a dictionary (associative mapping). "
+          + "Dictionaries may be constructed with a special literal syntax:<br>"
+          + "<pre class=\"language-python\">d = {\"a\": 2, \"b\": 5}</pre>"
+          + "When using the literal syntax, it is an error to have duplicated keys. "
+          + "Use square brackets to access elements:<br>"
+          + "<pre class=\"language-python\">e = d[\"a\"]   # e == 2</pre>"
+          + "Like lists, they can also be constructed using a comprehension syntax:<br>"
+          + "<pre class=\"language-python\">d = {i: 2*i for i in range(20)}\n"
+          + "e = d[8]       # e == 16</pre>"
+          + "See also the <a href=\"globals.html#dict\">dict()</a> constructor function. "
+          + "<p>Iterating over a dict is equivalent to iterating over its keys. The "
+          + "<code>in</code> operator tests for membership in the keyset of the dict.<br>"
+          + "<pre class=\"language-python\">\"a\" in {\"a\" : 2, \"b\" : 5} "
+          + "# evaluates as True</pre>"
+          + "The iteration order for a dict is deterministic but not specified."
 )
-public final class SkylarkDict<K, V>
-    extends MutableMap<K, V> implements Map<K, V>, SkylarkIndexable {
+public final class SkylarkDict<K, V> extends MutableMap<K, V>
+    implements Map<K, V>, SkylarkIndexable {
 
   private final LinkedHashMap<K, V> contents = new LinkedHashMap<>();
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
index 4cca541ac8..07419e4992 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
@@ -129,4 +129,12 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
             + "convert to a list."
   )
   public boolean incompatibleDepsetIsNotIterable;
+
+  @Option(
+    name = "incompatible_dict_literal_has_no_duplicates",
+    defaultValue = "false",
+    category = "incompatible changes",
+    help = "If set to true, the dictionary literal syntax doesn't allow duplicated keys."
+  )
+  public boolean incompatibleDictLiteralHasNoDuplicates;
 }
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/Crosstool.java b/src/test/java/com/google/devtools/build/lib/packages/util/Crosstool.java
index ddd5b584c9..3642fda958 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/Crosstool.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/Crosstool.java
@@ -20,6 +20,8 @@ import com.google.protobuf.TextFormat;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.LinkedHashSet;
+import java.util.Set;
 
 /**
  * A helper class to create a crosstool package containing a CROSSTOOL file, and the various
@@ -111,9 +113,15 @@ final class Crosstool {
         CrosstoolConfig.CrosstoolRelease.newBuilder();
     TextFormat.merge(crosstoolFileContents, configBuilder);
     StringBuilder compilerMap = new StringBuilder();
+    // Remove duplicates
+    Set<String> keys = new LinkedHashSet<>();
     for (CrosstoolConfig.CToolchain toolchain : configBuilder.build().getToolchainList()) {
-      compilerMap.append(String.format("'%s|%s': ':cc-compiler-%s',\n",
-          toolchain.getTargetCpu(), toolchain.getCompiler(), toolchain.getTargetCpu()));
+      String key = String.format("%s|%s", toolchain.getTargetCpu(), toolchain.getCompiler());
+      if (!keys.contains(key)) {
+        keys.add(key);
+        compilerMap.append(
+            String.format("'%s': ':cc-compiler-%s',\n", key, toolchain.getTargetCpu()));
+      }
     }
 
     for (String arch : archs) {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index a3251da4f3..ff5940a95e 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -359,6 +359,19 @@ public class EvaluationTest extends EvaluationTestCase {
   }
 
   @Test
+  public void testDictWithDuplicatedKey() throws Exception {
+    new SkylarkTest("--incompatible_dict_literal_has_no_duplicates=true")
+        .testIfErrorContains(
+            "Duplicated key \"str\" when creating dictionary", "{'str': 1, 'x': 2, 'str': 3}");
+  }
+
+  @Test
+  public void testDictAllowDuplicatedKey() throws Exception {
+    new SkylarkTest("--incompatible_dict_literal_has_no_duplicates=false")
+        .testStatement("{'str': 1, 'x': 2, 'str': 3}", ImmutableMap.of("str", 3, "x", 2));
+  }
+
+  @Test
   public void testRecursiveTupleDestructuring() throws Exception {
     newTest()
         .setUp("((a, b), (c, d)) = [(1, 2), (3, 4)]")
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index 6b93e7e938..a76bbc6847 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -1122,11 +1122,6 @@ public class MethodLibraryTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testDictionaryWithMultipleKeys() throws Exception {
-    new BothModesTest().testStatement("{0: 'a', 1: 'b', 0: 'c'}[0]", "c");
-  }
-
-  @Test
   public void testDictionaryKeyNotFound() throws Exception {
     new BothModesTest()
         .testIfErrorContains("key \"0\" not found in dictionary", "{}['0']")