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

Add a Python 3 warning for implementing __eq__ without also implentnting __hash__ #1115

Merged
merged 2 commits into from
Sep 26, 2016

Conversation

rowillia
Copy link
Contributor

@rowillia rowillia commented Sep 23, 2016

I've been exploring transitioning our codebase to Python 3, and I've been using the immensely helpful -3 flag when running tests as described here:

https://docs.python.org/3/howto/pyporting.html#prevent-compatibility-regressions

Unfortunately, one of the most common issues I'm finding is around classes implementing __eq__ but not __hash__. The behavior around this changed in Python 3. See below:

class JustEq(object):
   def __init__(self, x):
     self.x = x
   def __eq__(self, other):
     return self.x == other.x

class Neither(object):
  def __init__(self, x):
    self.x = x

class HashAndEq(object):
   def __init__(self, x):
     self.x = x
   def __eq__(self, other):
     return self.x == other.x
   def __hash__(self):
     return hash(self.x)

{Neither(1), Neither(2)}  # OK
{HashAndEq(1), HashAndEq(2)}  # OK
{JustEq(1), JustEq(2)}  # Works in Python 2, throws in Python 3

# In general this is bad practice which is what motivated the behavior change in Python 3.

as_set = {JustEq(1), JustEq(2)}
print(JustEq(1) in as_set)  # prints False
print(JustEq(1) in list(as_set))  # prints True

In general this is bad practice for the reasons above, so I'd actually love to have this warn
both as a general warning and as a Python 3 warning, but I don't see a clean way to have the same rule apply in multiple contexts.

…nting `__hash__`

I've been exploring transitioning our codebase to Python 3, and I've been using the immensely helpful `-3` flag
when running tests as described here:

https://docs.python.org/3/howto/pyporting.html#prevent-compatibility-regressions

Unfortunately, one of the most common issues I'm finding is around classes implementing `__eq__` but not
`__hash__`.  The behavior around this changed in Python 3.  See below:

```python
class JustEq(object):
   def __init__(self, x):
     self.x = x
   def __eq__(self, other):
     return self.x == other.x

class Neither(object):
  def __init__(self, x):
    self.x = x

class HashAndEq(object):
   def __init__(self, x):
     self.x = x
   def __eq__(self, other):
     return self.x == other.x
   def __hash__(self):
     return hash(self.x)

{Neither(1), Neither(2)}  # OK
{HashAndEq(1), HashAndEq(2)}  # OK
{JustEq(1), JustEq(2)}  # Works in Python 2, throws in Python 3

as_set = {JustEq(1), JustEq(2)}
print(JustEq(1) in as_set)  # prints False
print(JustEq(1) in list(as_set))  # prints True
```

In general this is bad practice for the reasons above, so I'd actually love to havet this warn
both as a general warning and as a Python 3 warning, but I don't see a clean way to have the same
rule apply in multiple contexts.
@The-Compiler
Copy link
Contributor

I'll have to disagree about this being bad practise in general. I have a Python 3 only codebase where I often need objects to be comparable, but I don't need to hash them anywhere - why should I be forced to do __hash__ = None?

@rowillia
Copy link
Contributor Author

@The-Compiler The main reason the -3 python2.7 flag issues a warning on this is the behavior changed from Python 2 to Python 3. In Python 2, objects like that implicitly get object.__hash__ as their __hash__ implementation. In Python 3, objects like that implicitly get None as their __hash__ implementation (which is likely a safer behavior).

In the example above, {JustEq(1), JustEq(2)} is still hashable even though JustEq's hash method doesn't make sense (leading to the problem below in the example).

I opened an issue on bugs.python.org about moving this warning to be at time-of-access as oppsoed to time-of-declaration and it was shot down - http://bugs.python.org/issue28263 . I generally agree - this is a large-ish behavior difference between 2 and 3 so the tools should flag it.

As this is one of the most common compatibility issues I'm finding with 2->3 code, I'd love to have PyLint flag it as core devs like @brettcannon have called this out as being one of the tools to be used when converting from 2 to 3.

@The-Compiler
Copy link
Contributor

I was referring to "In general this is bad practice for the reasons above, so I'd actually love to have this warn both as a general warning and as a Python 3 warning" and arguing that it shouldn't be a general warning. 😉

@rowillia
Copy link
Contributor Author

@The-Compiler Got it! Context gets lost in GH comments 😄 . Ideally this would be a Python 2 warning all the time and a Python 3 compat warning.

@PCManticore
Copy link
Contributor

Sounds good for me if this is a Python 3 porting checker error (--py3k).

@PCManticore
Copy link
Contributor

Oh, this is actually a PR! Thought this was an issue, sorry.

@PCManticore
Copy link
Contributor

Looks great, thank you!

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.

3 participants