Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec: mutable types are unhashable even when frozen #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stepancheg
Copy link
Contributor

Motivation is this:

  • it is not possible for user to freeze a value
  • it might not be possible to provide ability to freeze
    individual values in all implementation architectures
  • Python does not allow mutable types in dict keys
  • we may relax this restriction later, and relaxing will be easier
    than restricting, so better restrict it earlier

Related issue: #64

Motivation is this:
* it is not possible for user to freeze a value
* it might not be possible to provide ability to freeze
  individual values in all implementation architectures
* Python does not allow mutable types in dict keys
* we may relax this restriction later, and relaxing will be easier
  than restricting, so better restrict it earlier
@alandonovan
Copy link
Contributor

it is not possible for user to freeze a value

True, but a user may assume that a value loaded from another module is frozen.

I agree with the motivation, and I like this change. However, according to this interpretation, the Java implementation has a serious bug: although a list or dict (frozen or unfrozen) is not suitable as a dict key, a struct or tuple containing frozen lists or dicts is suitable as a dict key. See:
https://github.com/bazelbuild/bazel/blob/8bd6fbebe8835eefb18074c5eb993eeb59ab3272/src/main/java/net/starlark/java/eval/StarlarkValue.java#L84

(also Google internal issue b/120837801)

$ cat test/BUILD

unfrozendict={}
unfrozenlist=[]
load("inc.bzl", "frozendict", "frozenlist", struct="makestruct")

# print({unfrozendict: None}) # unhashable type: 'dict'
# print({unfrozenlist: None}) # unhashable type: 'list'
# print({frozendict: None}) # unhashable type: 'dict'
# print({frozenlist: None}) # unhashable type: 'list'

print({struct(f=frozendict): None}) # ok
print({struct(f=frozenlist): None}) # ok
# print({struct(f=unfrozendict): None}) # unhashable type: 'struct' (a poor error message)
# print({struct(f=unfrozenlist): None}) # unhashable type: 'struct'

$ cat test/inc.bzl 

frozendict = {}
frozenlist = []

mystruct = struct

@alandonovan alandonovan requested a review from brandjon November 30, 2020 17:21
@stepancheg
Copy link
Contributor Author

the Java implementation has a serious bug

This is obviously easy to fix.

We can introduce a new semantics key StarlarkSemantics.ALLOW_CERTAIN_MUTABLE_HASHING, on by default now, off by default later; to make migration easier.

@alandonovan
Copy link
Contributor

This is obviously easy to fix.

Unfortunately that is not the case. Even a trivial syntactic change (like disallowing invalid escape sequences such as \k) requires days of cleanup in Google's code base. This change is very subtle, because it affects the semantics of analysis code. I expect it would require a month of work, and I would not be surprised if the effort got mired before completion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants