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

Merged
merged 3 commits into from Jul 3, 2018

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented Jul 2, 2018

Problem

Overriding __eq__ on datatype violates structural equality, and the assumptions that people have about datatype 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 on datatype 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 just Key instances).

@stuhood stuhood changed the title WIP: Remove and prevent inaccurate __eq__ implementations on datatype Remove and prevent inaccurate __eq__ implementations on datatype Jul 2, 2018

@stuhood stuhood requested review from illicitonion , cosmicexplorer and kwlzn Jul 2, 2018

@kwlzn

kwlzn approved these changes Jul 2, 2018

Copy link
Member

kwlzn left a comment

lgtm!

@@ -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

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 3, 2018

Contributor

This is why I love dynamic languages!!!

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 3, 2018

Integration test failed on something that is clearly a network error... merging.

@stuhood stuhood merged commit f22781c into pantsbuild:master Jul 3, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@stuhood stuhood deleted the twitter:stuhood/accurate-equals branch Jul 3, 2018

@@ -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

This comment has been minimized.

@illicitonion

illicitonion Jul 3, 2018

Contributor

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

?

This comment has been minimized.

@stuhood

stuhood Jul 3, 2018

Member

That would check super classes, but not subclasses, I think?

This comment has been minimized.

@illicitonion

illicitonion Jul 3, 2018

Contributor

Aah, we subclass these things? Got it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment