-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Remove and prevent inaccurate __eq__ implementations on datatype #6061
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
…y struct equality.
@@ -89,6 +94,9 @@ def __eq__(self, other): | |||
return False | |||
# Explicitly return super.__eq__'s value in case super returns NotImplemented | |||
return super(DataType, self).__eq__(other) | |||
# We define an attribute on the `cls` level definition of `__eq__` that will allow us to detect | |||
# that it has been overridden. | |||
__eq__._eq_override_canary = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why I love dynamic languages!!!
Integration test failed on something that is clearly a network error... merging. |
@@ -60,6 +60,11 @@ def make_type_error(cls, msg, *args, **kwargs): | |||
return TypeCheckError(cls.__name__, msg, *args, **kwargs) | |||
|
|||
def __new__(cls, *args, **kwargs): | |||
# TODO: Ideally we could execute this exactly once per `cls` but it should be a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just do this in the datatype
constructor, changing return type(superclass_name, (DataType,), {})
to:
t = type(superclass_name, (DataType,), {})
if not hasattr(t.__eq__, '_eq_override_canary'):
raise ...
return t
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would check super classes, but not subclasses, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah, we subclass these things? Got it :)
Problem
Overriding
__eq__
ondatatype
violates structural equality, and the assumptions that people have aboutdatatype
instances. Moreover, it is very error prone in the presence of #6059, which will be using__eq__
to determine whether objects have changed.Solution
Make it impossible to override
__eq__
accidentally ondatatype
by attaching a canary to the__eq__
method definition on the generated class. Remove a few__eq__
implementations that violated structural equality and were thus causing issues in #6059.Result
datatypes
behave as expected more frequently, and bugs are avoided. There is no noticeable impact on performance in my testing (likely these were overridden back when every object ended up memoized, rather than justKey
instances).