classification
Title: Regression with __hash__ definition and rich comparison operators
Type: crash Stage:
Components: Interpreter Core Versions: Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: gvanrossum Nosy List: exarkun, gvanrossum, therve
Priority: normal Keywords: patch

Created on 2007-12-03 13:51 by therve, last changed 2007-12-20 09:31 by therve. This issue is now closed.

Files
File name Uploaded Description Edit
1549_1.diff therve, 2007-12-04 11:02
Messages (15)
msg58124 - (view) Author: Thomas Herve (therve) * Date: 2007-12-03 13:51
A new behavior was introduced in r59106 in python trunk, which look
suspicious to me. Basically, every time a class defines a comparison
operator, it's __hash__ method is defined to None. The simple following
example shows it:

>>> class A(object):
...     def __init__(self, b):
...             self.b = b
...     def __cmp__(self, other):
...             if not isinstance(other, A):
...                     return -1
...             return cmp(self.b, other.b)
...
>>> hash(A(2))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'NoneType' object is not callable


The problematic change is here:
http://svn.python.org/view/python/trunk/Objects/typeobject.c?rev=59106&r1=58032&r2=59106

And mainly the overrides_hash call in inherit_slots.

FWIW, I've encounter the problem because zope.interface is now usable on
trunk.
msg58125 - (view) Author: Thomas Herve (therve) * Date: 2007-12-03 13:52
Of course, I meant unusable.
msg58126 - (view) Author: Thomas Herve (therve) * Date: 2007-12-03 14:15
Also, to be more clear, that happens when you define any of the
functions in the name_op array (__lt__, __le___, __gt__, __ge__, ...).
It's not just a problem about the object being non hashable.
msg58140 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-03 19:09
This was done intentionally -- if you define a comparison without
defining a hash, the default hash will not match your comparison, and
your objects will misbehave when used as dictionary keys.

Can you give more details about the Zope issue?  I agree that needs to
be resolved before we release 2.6.
msg58141 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2007-12-03 19:15
zope.interface.interface.InterfaceClass implements __lt__ and __gt__. 
It doesn't override any of the other comparison methods, and it relies
on inheriting an identity-based __hash__ from its parent.

Disabling __hash__ when __cmp__ or __eq__ is defined makes sense, but it
does not seem like it needs to be disabled if only __lt__ or __gt__ is
defined.
msg58148 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-03 19:32
You have a point. Can you suggest a patch that fixes this?

Question though -- what semantics are used for __lt__ and __gt__ that
they don't require overriding __eq__?
msg58153 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2007-12-03 20:31
It implements ordering based on the interface's fully-qualified Python
name (ie, (__module__, __name__)).  This is compatible with the default
__eq__ implementation.
msg58154 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-03 20:51
Fair enough.

Still, this would go much faster if someone other than myself could be
coerced into researching how to fix this (at least the Zope use -- I
haven't heard back from the OP).
msg58156 - (view) Author: Thomas Herve (therve) * Date: 2007-12-03 21:23
There are different ways to fix this. Reverting r59016 is one of them,
but I guess it's not an acceptable solution :).

From what I understand of the changes, removing the operations (__lt__,
__gt__, __le__, __ge__) from the name_op array would fix this. But I
have to understand why they have been put in there in the first place.

I'll try tomorrow, you assign this to me if you want.
msg58157 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-03 21:32
Well, the name_op array is used for other purposes.  Perhaps the main
issue is that the presence of any of these causes the "rich comparison"
functionality to be overridden.  But please do look into this if you can!
msg58182 - (view) Author: Thomas Herve (therve) * Date: 2007-12-04 11:02
I gave it a try. The following patch corrects 2 problems:
 * classes with only __lt__ and __gt__ defined are hashable
 * classes defining __eq__ get a meaningful exception.

I've restricted the hash_name_op to the bare minimum, but no tests
failed, so I supposed it was enough :).

Please review.
msg58803 - (view) Author: Thomas Herve (therve) * Date: 2007-12-19 10:23
This is probably not the best moment (holidays etc), but please take
this issue seriously. This problem completely block the community
builders at pybots.org, and make any tests on current python trunk
impossible. This also means that if in the meantime another regression
appear, it will be even more difficult to solve it afterwards.

Also, zope is not the only one affected, storm seems to suffer from the
problem too (not exacly the same, though). As twisted is also unusable
without zope.interface, that makes several project that can't follow
python developpement at all (zope.interface error happens at import
time, for the record).

If this problem isn't solved in a reasonable time, it really questions
the future of the community builders and their purpose.

Thanks.
msg58817 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-19 18:33
To be honest, I have no idea what pybots is, and how it's affected. 
Maybe you can get someone else interested in reviewing the code?  I'm
focusing mostly on Py3k.
msg58836 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-12-19 22:51
Committed revision 59576.
I found some time.
Happy pybotting!
msg58866 - (view) Author: Thomas Herve (therve) * Date: 2007-12-20 09:31
Thanks a lot!
History
Date User Action Args
2007-12-20 09:31:22thervesetmessages: + msg58866
2007-12-19 22:51:37gvanrossumsetstatus: open -> closed
resolution: accepted
messages: + msg58836
2007-12-19 18:33:40gvanrossumsetmessages: + msg58817
2007-12-19 10:23:57thervesetmessages: + msg58803
2007-12-18 22:37:08gvanrossumsetkeywords: + patch
2007-12-18 22:37:03gvanrossumsetpriority: high -> normal
2007-12-04 11:02:52thervesetfiles: + 1549_1.diff
messages: + msg58182
2007-12-03 21:32:48gvanrossumsetmessages: + msg58157
2007-12-03 21:23:39thervesetmessages: + msg58156
2007-12-03 20:51:28gvanrossumsetmessages: + msg58154
2007-12-03 20:31:57exarkunsetmessages: + msg58153
2007-12-03 19:32:33gvanrossumsetmessages: + msg58148
2007-12-03 19:15:04exarkunsetmessages: + msg58141
2007-12-03 19:09:46gvanrossumsetmessages: + msg58140
2007-12-03 14:40:53christian.heimessetpriority: high
assignee: gvanrossum
nosy: + gvanrossum
2007-12-03 14:15:45thervesetmessages: + msg58126
2007-12-03 13:52:35thervesetnosy: + exarkun
messages: + msg58125
2007-12-03 13:51:31thervecreate