Reject unhashable types in dict.get

Related: bazelbuild/starlark#65, #8730

Closes #8837.

PiperOrigin-RevId: 257578468

site/docs/skylark/backward-compatibility.md

@@ -14,6 +14,7 @@ Full, authorative list of incompatible changes is [GitHub issues with
 
 General Starlark
 
+*   [Dictionary lookup of unhashable types](#dictionary-lookup-of-unhashable-types)
 *   [Dictionary concatenation](#dictionary-concatenation)
 *   [String escapes](#string-escapes)
 *   [Load must appear at top of file](#load-must-appear-at-top-of-file)
@@ -65,6 +66,17 @@ C++
 *   [Disable legacy C++ toolchain API](#disable-legacy-c-toolchain-api)
 
 
+### Dictionary lookup of unhashable types
+
+We are restricting the lookup of keys in dictionaries to hashable
+types only. Trying to search or retrieve a key (for example by
+using the `in` operator or `dict.get`) of an unhashable type from
+a dict will fail.
+
+*   Flag: `--incompatible_disallow_dict_lookup_unhashable_keys`
+*   Default: `false`
+*   Tracking issue: [#8730](https://github.com/bazelbuild/bazel/issues/8730)
+
 ### Dictionary concatenation
 
 We are removing the `+` operator on dictionaries. This includes the `+=` form

src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java

@@ -251,6 +251,20 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
       help = "If true, disables all license checking logic")
   public boolean incompatibleDisableThirdPartyLicenseChecking;
 
+  @Option(
+      name = "incompatible_disallow_dict_lookup_unhashable_keys",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help =
+          "If set to true, dict key lookups using `in` or `dict.get` will fail with unhashable"
+              + " types.")
+  public boolean incompatibleDisallowDictLookupUnhashableKeys;
+
   @Option(
       name = "incompatible_disallow_dict_plus",
       defaultValue = "true",
@@ -654,6 +668,8 @@ public StarlarkSemantics toSkylarkSemantics() {
             .incompatibleDoNotSplitLinkingCmdline(incompatibleDoNotSplitLinkingCmdline)
             .incompatibleDepsetForLibrariesToLinkGetter(incompatibleDepsetForLibrariesToLinkGetter)
             .incompatibleRestrictStringEscapes(incompatibleRestrictStringEscapes)
+            .incompatibleDisallowDictLookupUnhashableKeys(
+                incompatibleDisallowDictLookupUnhashableKeys)
             .build();
     return INTERNER.intern(semantics);
   }

src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java

@@ -113,7 +113,7 @@ private static boolean in(Object x, Object y, Environment env, Location location
               + "Use --incompatible_depset_is_not_iterable=false to temporarily disable "
               + "this check.");
     } else if (y instanceof SkylarkQueryable) {
-      return ((SkylarkQueryable) y).containsKey(x, location, env.getStarlarkContext());
+      return ((SkylarkQueryable) y).containsKey(x, location, env);
     } else if (y instanceof String) {
       if (x instanceof String) {
         return ((String) y).contains((String) x);

src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java

@@ -80,18 +80,25 @@ private SkylarkDict(@Nullable Environment env) {
     this.mutability = env == null ? Mutability.IMMUTABLE : env.mutability();
   }
 
-  @SkylarkCallable(name = "get",
-    doc = "Returns the value for <code>key</code> if <code>key</code> is in the dictionary, "
-        + "else <code>default</code>. If <code>default</code> is not given, it defaults to "
-        + "<code>None</code>, so that this method never throws an error.",
-    parameters = {
-      @Param(name = "key", noneable = true, doc = "The key to look for."),
-      @Param(name = "default", defaultValue = "None", noneable = true, named = true,
-          doc = "The default value to use (instead of None) if the key is not found.")},
-    allowReturnNones = true
-  )
-  public Object get(Object key, Object defaultValue) {
-    if (this.containsKey(key)) {
+  @SkylarkCallable(
+      name = "get",
+      doc =
+          "Returns the value for <code>key</code> if <code>key</code> is in the dictionary, "
+              + "else <code>default</code>. If <code>default</code> is not given, it defaults to "
+              + "<code>None</code>, so that this method never throws an error.",
+      parameters = {
+        @Param(name = "key", noneable = true, doc = "The key to look for."),
+        @Param(
+            name = "default",
+            defaultValue = "None",
+            noneable = true,
+            named = true,
+            doc = "The default value to use (instead of None) if the key is not found.")
+      },
+      allowReturnNones = true,
+      useEnvironment = true)
+  public Object get(Object key, Object defaultValue, Environment env) throws EvalException {
+    if (containsKey(key, null, env)) {
       return this.get(key);
     }
     return defaultValue;
@@ -495,7 +502,14 @@ public final Object getIndex(Object key, Location loc, StarlarkContext context)
   @Override
   public final boolean containsKey(Object key, Location loc, StarlarkContext context)
       throws EvalException {
-    EvalUtils.checkValidDictKey(key);
+    return this.containsKey(key);
+  }
+
+  @Override
+  public final boolean containsKey(Object key, Location loc, Environment env) throws EvalException {
+    if (env.getSemantics().incompatibleDisallowDictLookupUnhashableKeys()) {
+      EvalUtils.checkValidDictKey(key);
+    }
     return this.containsKey(key);
   }
 

src/main/java/com/google/devtools/build/lib/syntax/SkylarkQueryable.java

@@ -25,4 +25,8 @@ public interface SkylarkQueryable {
 
   /** Returns whether the key is in the object. */
   boolean containsKey(Object key, Location loc, StarlarkContext context) throws EvalException;
+
+  default boolean containsKey(Object key, Location loc, Environment env) throws EvalException {
+    return this.containsKey(key, loc, env.getStarlarkContext());
+  }
 }

src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java

@@ -204,6 +204,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
 
   public abstract boolean incompatibleRestrictStringEscapes();
 
+  public abstract boolean incompatibleDisallowDictLookupUnhashableKeys();
+
   @Memoized
   @Override
   public abstract int hashCode();
@@ -278,6 +280,7 @@ public static Builder builderWithDefaults() {
           .incompatibleDoNotSplitLinkingCmdline(true)
           .incompatibleDepsetForLibrariesToLinkGetter(true)
           .incompatibleRestrictStringEscapes(false)
+          .incompatibleDisallowDictLookupUnhashableKeys(false)
           .build();
 
   /** Builder for {@link StarlarkSemantics}. All fields are mandatory. */
@@ -368,6 +371,8 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo
 
     public abstract Builder incompatibleRestrictStringEscapes(boolean value);
 
+    public abstract Builder incompatibleDisallowDictLookupUnhashableKeys(boolean value);
+
     public abstract StarlarkSemantics build();
   }
 }

src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java

@@ -164,6 +164,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--incompatible_run_shell_command_string=" + rand.nextBoolean(),
         "--incompatible_string_join_requires_strings=" + rand.nextBoolean(),
         "--incompatible_restrict_string_escapes=" + rand.nextBoolean(),
+        "--incompatible_disallow_dict_lookup_unhashable_keys=" + rand.nextBoolean(),
         "--internal_skylark_flag_test_canary=" + rand.nextBoolean());
   }
 
@@ -216,6 +217,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleRunShellCommandString(rand.nextBoolean())
         .incompatibleStringJoinRequiresStrings(rand.nextBoolean())
         .incompatibleRestrictStringEscapes(rand.nextBoolean())
+        .incompatibleDisallowDictLookupUnhashableKeys(rand.nextBoolean())
         .internalSkylarkFlagTestCanary(rand.nextBoolean())
         .build();
   }

src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java

@@ -3038,6 +3038,43 @@ public void testRecursiveImport2() throws Exception {
     }
   }
 
+  @Test
+  public void testUnhashableInDictForbidden() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_dict_lookup_unhashable_keys=true");
+
+    scratch.file("test/extension.bzl", "y = [] in {}");
+
+    scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:r");
+    assertContainsEvent("unhashable type: 'list'");
+  }
+
+  @Test
+  public void testDictGetUnhashableForbidden() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_dict_lookup_unhashable_keys=true");
+
+    scratch.file("test/extension.bzl", "y = {}.get({})");
+
+    scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:r");
+    assertContainsEvent("unhashable type: 'dict'");
+  }
+
+  @Test
+  public void testUnhashableLookupDict() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_dict_lookup_unhashable_keys=false");
+
+    scratch.file("test/extension.bzl", "y = [[] in {}, {}.get({})]");
+
+    scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
+
+    getConfiguredTarget("//test:r");
+  }
+
   @Test
   public void testUnknownStringEscapesForbidden() throws Exception {
     setSkylarkSemanticsOptions("--incompatible_restrict_string_escapes=true");

src/test/starlark/testdata/dict.sky

@@ -90,8 +90,18 @@ assert_eq(1 in {1: 0}, True)
 assert_eq(() in {}, False)
 assert_eq("a" in dict(a=1), True)
 ---
-{} in {} ### unhashable type: 'dict'
+
+# should fail after --incompatible_disallow_dict_lookup_unhashable_keys is flipped to true
+# unhashable types
+
+{} in {} ## unhashable type: 'dict'
+---
+[] in {} ## unhashable type: 'list'
+---
+len in {} ## unhashable type: 'function'
+---
+{}.get([]) ## unhashable type: 'list'
 ---
-[] in {} ### unhashable type: 'list'
+dict().get({}) ## unhashable type: 'dict'
 ---
-len in {} ### unhashable type: 'function'
+{1: 2}.get(len) ## unhashable type: 'function'