Reject hashing of frozen mutable types #8980
Conversation
When this flag is enabled, freezing a mutable object will no longer make it hashable. RELNOTES: Flag `--incompatible_disallow_hashing_frozen_mutables` is added. See bazelbuild#7800
Quarz0
requested review from
aiuto,
brandjon,
c-parsons,
dslomov,
laurentlb and
vladmos
as code owners
| @@ -355,7 +355,7 @@ public void unsafeShallowFreeze() { | |||
| */ | |||
| public void put(K key, V value, Location loc, Mutability mutability) throws EvalException { | |||
| checkMutable(loc, mutability); | |||
| EvalUtils.checkValidDictKey(key); | |||
| EvalUtils.checkValidDictKey(key, null); | |||
There was a problem hiding this comment.
I think that this function (put) is only used here: https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java#L178
However, I couldn't find a way to get environment here and passing null will just assume the flag is false.
|
I also tried to reject frozen types in But also due to the incompatible flag I couldn't find a way to pass the environment. |
|
@brandjon, I think you were familiar with this area (hashability). Do you want to take a look at the change? |
jin
added
the
team-ExternalDeps
|
|
||
| reporter.removeHandler(failFastHandler); | ||
| getConfiguredTarget("//test:r"); | ||
| assertContainsEvent("unhashable type: 'tuple'"); |
There was a problem hiding this comment.
This error message is a bit confusing, implying that tuples are never hashable. Would it be feasible to change it to mention the actual unhashable type within this PR? The similar error in Python is "unhashable type: 'dict'".
There was a problem hiding this comment.
We might want to submit this PR first (and hopefully be included in Bazel 0.29) and iterate later.
There was a problem hiding this comment.
Since this is really the object that is unhashable rather than the type, how about changing the message to
"unhashable object of type: dict"
There was a problem hiding this comment.
btw the same happens, regardless of this PR, if you try something like {([],): None} you will also get the same error unhashable type: 'tuple'.
I could still fix it in this PR by modifying the SkylarkValue interface to get the mutable/hashable object type within an immutable structure (like tuple), but I'm not sure if that's the best way to do so.
There was a problem hiding this comment.
I like @aiuto's suggestion.
| * Flag: `--incompatible_disallow_hashing_frozen_mutables` | ||
| * Default: `false` | ||
| * Tracking issue: [#7800](https://github.com/bazelbuild/bazel/issues/7800) | ||
|
|
There was a problem hiding this comment.
fyi, I'm removing this page. Instead we just use GitHub issues to track the changes.
philwo
added
the
team-OSS
philwo
removed
the
team-OSS
Related: #7800