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

Regression with __hash__ definition and rich comparison operators #45890

Closed
therve mannequin opened this issue Dec 3, 2007 · 15 comments
Closed

Regression with __hash__ definition and rich comparison operators #45890

therve mannequin opened this issue Dec 3, 2007 · 15 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@therve
Copy link
Mannequin

therve mannequin commented Dec 3, 2007

BPO 1549
Nosy @gvanrossum
Files
  • 1549_1.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/gvanrossum'
    closed_at = <Date 2007-12-19.22:51:37.125>
    created_at = <Date 2007-12-03.13:51:31.627>
    labels = ['interpreter-core', 'type-crash']
    title = 'Regression with __hash__ definition and rich comparison operators'
    updated_at = <Date 2007-12-20.09:31:22.706>
    user = 'https://bugs.python.org/therve'

    bugs.python.org fields:

    activity = <Date 2007-12-20.09:31:22.706>
    actor = 'therve'
    assignee = 'gvanrossum'
    closed = True
    closed_date = <Date 2007-12-19.22:51:37.125>
    closer = 'gvanrossum'
    components = ['Interpreter Core']
    creation = <Date 2007-12-03.13:51:31.627>
    creator = 'therve'
    dependencies = []
    files = ['8871']
    hgrepos = []
    issue_num = 1549
    keywords = ['patch']
    message_count = 15.0
    messages = ['58124', '58125', '58126', '58140', '58141', '58148', '58153', '58154', '58156', '58157', '58182', '58803', '58817', '58836', '58866']
    nosy_count = 3.0
    nosy_names = ['gvanrossum', 'exarkun', 'therve']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue1549'
    versions = ['Python 2.6']

    @therve
    Copy link
    Mannequin Author

    therve mannequin commented Dec 3, 2007

    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.

    @therve therve mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Dec 3, 2007
    @therve
    Copy link
    Mannequin Author

    therve mannequin commented Dec 3, 2007

    Of course, I meant unusable.

    @therve
    Copy link
    Mannequin Author

    therve mannequin commented Dec 3, 2007

    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.

    @gvanrossum
    Copy link
    Member

    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.

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Dec 3, 2007

    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.

    @gvanrossum
    Copy link
    Member

    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__?

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Dec 3, 2007

    It implements ordering based on the interface's fully-qualified Python
    name (ie, (module, __name__)). This is compatible with the default
    __eq__ implementation.

    @gvanrossum
    Copy link
    Member

    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).

    @therve
    Copy link
    Mannequin Author

    therve mannequin commented Dec 3, 2007

    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.

    @gvanrossum
    Copy link
    Member

    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!

    @therve
    Copy link
    Mannequin Author

    therve mannequin commented Dec 4, 2007

    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.

    @therve
    Copy link
    Mannequin Author

    therve mannequin commented Dec 19, 2007

    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.

    @gvanrossum
    Copy link
    Member

    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.

    @gvanrossum
    Copy link
    Member

    Committed revision 59576.
    I found some time.
    Happy pybotting!

    @therve
    Copy link
    Mannequin Author

    therve mannequin commented Dec 20, 2007

    Thanks a lot!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant