Reject hashing of frozen mutable types

Related: #7800

Closes #8980.

PiperOrigin-RevId: 261150154

site/docs/skylark/backward-compatibility.md

@@ -15,6 +15,7 @@ Full, authorative list of incompatible changes is [GitHub issues with
 General Starlark
 
 *   [Dictionary lookup of unhashable types](#dictionary-lookup-of-unhashable-types)
+*   [Frozen mutable types are no longer hashable](#frozen-mutable-types-are-no-longer-hashable)
 *   [Dictionary concatenation](#dictionary-concatenation)
 *   [String escapes](#string-escapes)
 *   [String.split empty separator](#stringsplit-empty-separator)
@@ -80,6 +81,15 @@ a dict will fail.
 *   Default: `false`
 *   Tracking issue: [#8730](https://github.com/bazelbuild/bazel/issues/8730)
 
+### Frozen mutable types are no longer hashable
+
+When this flag is enabled, mutable types (e.g. list, dict) do not become
+hashable when frozen and thus cannot be used as dictionary keys.
+
+*   Flag: `--incompatible_disallow_hashing_frozen_mutables`
+*   Default: `false`
+*   Tracking issue: [#7800](https://github.com/bazelbuild/bazel/issues/7800)
+
 ### Dictionary concatenation
 
 We are removing the `+` operator on dictionaries. This includes the `+=` form

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

@@ -348,6 +348,18 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
       help = "If set to true, the default value of the `allow_empty` argument of glob() is False.")
   public boolean incompatibleDisallowEmptyGlob;
 
+  @Option(
+      name = "incompatible_disallow_hashing_frozen_mutables",
+      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, freezing a mutable object will not make it hashable.")
+  public boolean incompatibleDisallowHashingFrozenMutables;
+
   @Option(
       name = "incompatible_disallow_legacy_java_provider",
       defaultValue = "false",
@@ -761,6 +773,7 @@ public StarlarkSemantics toSkylarkSemantics() {
             .incompatibleAllowTagsPropagation(incompatibleAllowTagsPropagation)
             .incompatibleAssignmentIdentifiersHaveLocalScope(
                 incompatibleAssignmentIdentifiersHaveLocalScope)
+            .incompatibleDisallowHashingFrozenMutables(incompatibleDisallowHashingFrozenMutables)
             .build();
     return INTERNER.intern(semantics);
   }

src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkValue.java

@@ -22,12 +22,21 @@
 public interface SkylarkValue extends SkylarkPrintable {
 
   /**
-   * Returns if the value is immutable and thus suitable for being used as a dictionary key.
+   * Returns if the value is immutable.
    *
    * <p>Immutability is deep, i.e. in order for a value to be immutable, all values it is composed
    * of must be immutable, too.
    */
   default boolean isImmutable() {
       return false;
   }
+
+  /**
+   * Returns if the value is hashable and thus suitable for being used as a dictionary key.
+   *
+   * <p>Hashability implies immutability, but not vice versa.
+   */
+  default boolean isHashable() {
+    return this.isImmutable();
+  }
 }

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

@@ -112,7 +112,7 @@ public List<Clause> getClauses() {
     @Override
     public void evaluateAndCollect(Environment env) throws EvalException, InterruptedException {
       Object key = keyExpression.eval(env);
-      EvalUtils.checkValidDictKey(key);
+      EvalUtils.checkValidDictKey(key, env);
       result.put(key, valueExpression.eval(env), getLocation(), env);
     }
 

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

@@ -100,18 +100,36 @@ public int compare(Object o1, Object o2) {
 
   /**
    * Checks that an Object is a valid key for a Skylark dict.
+   *
    * @param o an Object to validate
    * @throws EvalException if o is not a valid key
    */
-  public static void checkValidDictKey(Object o) throws EvalException {
+  public static void checkValidDictKey(Object o, Environment env) throws EvalException {
     // TODO(bazel-team): check that all recursive elements are both Immutable AND Comparable.
-    if (isImmutable(o)) {
+    if (env != null && env.getSemantics().incompatibleDisallowHashingFrozenMutables()) {
+      if (isHashable(o)) {
+        return;
+      }
+    } else if (isImmutable(o)) {
       return;
     }
     // Same error message as Python (that makes it a TypeError).
     throw new EvalException(null, Printer.format("unhashable type: '%r'", o.getClass()));
   }
 
+  /**
+   * Is this object known or assumed to be recursively hashable by Skylark?
+   *
+   * @param o an Object
+   * @return true if the object is known to be a hashable value.
+   */
+  public static boolean isHashable(Object o) {
+    if (o instanceof SkylarkValue) {
+      return ((SkylarkValue) o).isHashable();
+    }
+    return isImmutable(o.getClass());
+  }
+
   /**
    * Is this object known or assumed to be recursively immutable by Skylark?
    * @param o an Object

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

@@ -222,7 +222,7 @@ public Runtime.NoneType update(
     SkylarkDict<K, V> dict =
         (args instanceof SkylarkDict) ? (SkylarkDict<K, V>) args : getDictFromArgs(args, loc, env);
     dict = SkylarkDict.plus(dict, (SkylarkDict<K, V>) kwargs, env);
-    putAll(dict, loc, env.mutability());
+    putAll(dict, loc, env.mutability(), env);
     return Runtime.NONE;
   }
 
@@ -355,7 +355,7 @@ protected Map<K, V> getContentsUnsafe() {
    */
   public void put(K key, V value, Location loc, Mutability mutability) throws EvalException {
     checkMutable(loc, mutability);
-    EvalUtils.checkValidDictKey(key);
+    EvalUtils.checkValidDictKey(key, null);
     contents.put(key, value);
   }
 
@@ -365,7 +365,9 @@ public void put(K key, V value, Location loc, Mutability mutability) throws Eval
    */
   // TODO(bazel-team): Decide whether to eliminate this overload.
   public void put(K key, V value, Location loc, Environment env) throws EvalException {
-    put(key, value, loc, env.mutability());
+    checkMutable(loc, mutability);
+    EvalUtils.checkValidDictKey(key, env);
+    contents.put(key, value);
   }
 
   /**
@@ -377,11 +379,11 @@ public void put(K key, V value, Location loc, Environment env) throws EvalExcept
    * @throws EvalException if some key is invalid or the dict is frozen
    */
   public <KK extends K, VV extends V> void putAll(
-      Map<KK, VV> map, Location loc, Mutability mutability) throws EvalException {
+      Map<KK, VV> map, Location loc, Mutability mutability, Environment env) throws EvalException {
     checkMutable(loc, mutability);
     for (Map.Entry<KK, VV> e : map.entrySet()) {
       KK k = e.getKey();
-      EvalUtils.checkValidDictKey(k);
+      EvalUtils.checkValidDictKey(k, env);
       contents.put(k, e.getValue());
     }
   }
@@ -508,7 +510,7 @@ public final boolean containsKey(Object key, Location loc, StarlarkContext conte
   @Override
   public final boolean containsKey(Object key, Location loc, Environment env) throws EvalException {
     if (env.getSemantics().incompatibleDisallowDictLookupUnhashableKeys()) {
-      EvalUtils.checkValidDictKey(key);
+      EvalUtils.checkValidDictKey(key, env);
     }
     return this.containsKey(key);
   }

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

@@ -687,6 +687,16 @@ public boolean isImmutable() {
       return true;
     }
 
+    @Override
+    public boolean isHashable() {
+      for (Object item : this) {
+        if (!EvalUtils.isHashable(item)) {
+          return false;
+        }
+      }
+      return true;
+    }
+
     @Override
     public Mutability mutability() {
       return Mutability.SHALLOW_IMMUTABLE;

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

@@ -69,6 +69,12 @@ public boolean isImmutable() {
     return mutability().isFrozen();
   }
 
+  @Override
+  public boolean isHashable() {
+    // Mutable structures are unhashable even if frozen
+    return false;
+  }
+
   /**
    * Add a new lock at {@code loc}. No effect if frozen.
    */

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

@@ -220,6 +220,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
 
   public abstract boolean incompatibleAllowTagsPropagation();
 
+  public abstract boolean incompatibleDisallowHashingFrozenMutables();
+
   @Memoized
   @Override
   public abstract int hashCode();
@@ -301,6 +303,7 @@ public static Builder builderWithDefaults() {
           .incompatibleDisablePartitionDefaultParameter(false)
           .incompatibleAllowTagsPropagation(false)
           .incompatibleAssignmentIdentifiersHaveLocalScope(false)
+          .incompatibleDisallowHashingFrozenMutables(false)
           .build();
 
   /** Builder for {@link StarlarkSemantics}. All fields are mandatory. */
@@ -405,6 +408,8 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo
 
     public abstract Builder incompatibleAssignmentIdentifiersHaveLocalScope(boolean value);
 
+    public abstract Builder incompatibleDisallowHashingFrozenMutables(boolean value);
+
     public abstract StarlarkSemantics build();
   }
 }

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

@@ -171,6 +171,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--incompatible_disallow_dict_lookup_unhashable_keys=" + rand.nextBoolean(),
         "--incompatible_disable_partition_default_parameter=" + rand.nextBoolean(),
         "--incompatible_assignment_identifiers_have_local_scope=" + rand.nextBoolean(),
+        "--incompatible_disallow_hashing_frozen_mutables=" + rand.nextBoolean(),
         "--internal_skylark_flag_test_canary=" + rand.nextBoolean());
   }
 
@@ -230,6 +231,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleDisallowDictLookupUnhashableKeys(rand.nextBoolean())
         .incompatibleDisablePartitionDefaultParameter(rand.nextBoolean())
         .incompatibleAssignmentIdentifiersHaveLocalScope(rand.nextBoolean())
+        .incompatibleDisallowHashingFrozenMutables(rand.nextBoolean())
         .internalSkylarkFlagTestCanary(rand.nextBoolean())
         .build();
   }

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

@@ -3292,6 +3292,46 @@ public void testIdentifierAssignmentFromOuterScopeForbidden() throws Exception {
     assertContainsEvent("local variable 'a' is referenced before assignment");
   }
 
+  @Test
+  public void testHashFrozenList() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_hashing_frozen_mutables=false");
+    scratch.file("test/extension.bzl", "y = []");
+
+    scratch.file(
+        "test/BUILD", "load('//test:extension.bzl', 'y')", "{y: 1}", "cc_library(name = 'r')");
+
+    getConfiguredTarget("//test:r");
+  }
+
+  @Test
+  public void testHashFrozenListForbidden() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_hashing_frozen_mutables=true");
+    scratch.file("test/extension.bzl", "y = []");
+
+    scratch.file(
+        "test/BUILD", "load('//test:extension.bzl', 'y')", "{y: 1}", "cc_library(name = 'r')");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:r");
+    assertContainsEvent("unhashable type: 'list'");
+  }
+
+  @Test
+  public void testHashFrozenDeepMutableForbidden() throws Exception {
+    setSkylarkSemanticsOptions("--incompatible_disallow_hashing_frozen_mutables=true");
+    scratch.file("test/extension.bzl", "y = {}");
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:extension.bzl', 'y')",
+        "{('a', (y,), True): None}",
+        "cc_library(name = 'r')");
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test:r");
+    assertContainsEvent("unhashable type: 'tuple'");
+  }
+
   @Test
   public void testNoOutputsError() throws Exception {
     scratch.file(