starlark: use Unhashable errors to indicate hash failures #165
Conversation
Fixes google#113. See that issue for discussion.
There was a problem hiding this comment.
FYI, recently in another thread we were discussing whether to allow objects of mutable types (such as dict and especially list) to become hashable once frozen. The Java implementation has always allowed this (and thus Bazel depends on it), but the Go impl, following Python, does not. The main argument in favor is that unlike in Python users cannot define their own classes and hash/eq relations so we need to allow simple structs to be used as dict keys even if they contain (frozen) lists. The main argument against is that it is non-monotonic (freezing an object takes away some operations but adds others), and that it is tricky to implement robustly because mutable objects may contain cycles (e.g. a list that contains itself).
I don't think a decision has been reached, but the implementation of that feature would require significant changes to the existing Hash method to make it resemble EqualDepth (with a depth bound) or writeValue (with a cycle check). Either way, we would need a new Hash method with an extra parameter exposed in the API. We could also piggyback a change of type of the hash from uint32 to uint, and perhaps add a seed value into the hasher. The old Value.Hash method would be deprecated as we migrate to a new optional Hashable interface.
| Hash() (uint32, error) | ||
| } | ||
|
|
||
| // An Unhashable error indicates that a call to Hash has failed because the Value is not hashable. | ||
| type Unhashable string |
| @@ -524,7 +530,7 @@ func (si stringIterable) Type() string { | |||
| } | |||
| func (si stringIterable) Freeze() {} // immutable | |||
| func (si stringIterable) Truth() Bool { return True } | |||
| func (si stringIterable) Hash() (uint32, error) { return 0, fmt.Errorf("unhashable: %s", si.Type()) } | |||
| func (si stringIterable) Hash() (uint32, error) { return 0, Unhashable("unhashable: " + si.Type()) } | |||
There was a problem hiding this comment.
Helper function?
func unhashable(x Value) (uint, error) {
return 0, Unhashable(fmt.Sprintf("unhashable: " + x.Type())
}
Ack. Shall we put this PR on hold until a decision has been reached? |
Fixes #113. See that issue for discussion.