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

Instance methods compare equal when their self's are equal #44345

Closed
fniessink mannequin opened this issue Dec 16, 2006 · 20 comments
Closed

Instance methods compare equal when their self's are equal #44345

fniessink mannequin opened this issue Dec 16, 2006 · 20 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@fniessink
Copy link
Mannequin

fniessink mannequin commented Dec 16, 2006

BPO 1617161
Nosy @loewis, @abalkin, @devdanzin, @serhiy-storchaka, @jdemeyer, @MojoVampire
PRs
  • bpo-1617161: Make the hash and equality of methods not depending on the value of self. #7848
  • Files
  • instancemethod.py: Example
  • method_compare.diff: use identity instead of equality of 'self'
  • 1617161_test_update.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 = None
    closed_at = <Date 2018-07-31.06:43:10.168>
    created_at = <Date 2006-12-16.21:36:32.000>
    labels = ['interpreter-core', 'type-bug', '3.8']
    title = "Instance methods compare equal when their self's are equal"
    updated_at = <Date 2018-07-31.06:43:10.167>
    user = 'https://bugs.python.org/fniessink'

    bugs.python.org fields:

    activity = <Date 2018-07-31.06:43:10.167>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-07-31.06:43:10.168>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2006-12-16.21:36:32.000>
    creator = 'fniessink'
    dependencies = []
    files = ['2254', '9666', '34367']
    hgrepos = []
    issue_num = 1617161
    keywords = ['patch']
    message_count = 20.0
    messages = ['30822', '30823', '30824', '63376', '63413', '63414', '63415', '63421', '63500', '84597', '110512', '110518', '196476', '213201', '305155', '308106', '320168', '320200', '320212', '322729']
    nosy_count = 9.0
    nosy_names = ['loewis', 'belopolsky', 'fniessink', 'ajaksu2', '_doublep', 'westley.martinez', 'serhiy.storchaka', 'jdemeyer', 'josh.r']
    pr_nums = ['7848']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue1617161'
    versions = ['Python 3.8']

    @fniessink
    Copy link
    Mannequin Author

    fniessink mannequin commented Dec 16, 2006

    Python 2.5 was changed so that instance methods compare equal when their im_self attributes compare equal. Here's a link to that change: http://svn.python.org/view?rev=46739&view=rev

    This is a problem if we want to distinguish between methods of instances that compare equal, for example when methods can be registered as callbacks (see attached example).

    It seems unlogical to me that whether or not the instance methods of two different instances are equal or not depends on the equality of the instance.

    Thanks, Frank

    @fniessink fniessink mannequin assigned arigo Dec 16, 2006
    @fniessink fniessink mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Dec 16, 2006
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Dec 16, 2006

    Armin, can you please comment?

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Dec 17, 2006

    I see. Indeed, in the callback situation the 2.5 change
    is not very good. On the other hand, I have a (possibly
    more obscure) case where the new equality makes more
    sense. Note also that the change was meant to unify
    the behavior of built-in and user method objects; e.g.
    if you use callbacks as dict keys, then already in
    previous Python versions it was impossible to use say
    'mylist.append' as a callback. Moreover, the hash of
    user methods was strangely based on the hash of the
    object itself already, which made dict key collisions
    likely.

    All in all I think that this part was an accident
    and never designed; I won't pronounce myself and
    suggest that python-dev should decide which of the
    two behaviors is realy "expected".

    @abalkin
    Copy link
    Member

    abalkin commented Mar 8, 2008

    the change was meant to unify
    the behavior of built-in and
    user method objects

    I don't think it achieved that. Consider:

    >>> [].index == [].index
    False
    
    but
    >>> UserList().index == UserList().index         
    True

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Mar 9, 2008

    My mistake, you are right. I forgot about one of the many types that
    CPython uses internally for built-in methods. Indeed:

        >>> [].index == [].index
        False
        >>> [].__add__ == [].__add__
        True

    I can fix this so that the answer is True in all cases; alternatively,
    considering Frank Niessink's original issue, we could bring this
    discussion to python-dev and decide what the correct behavior should be
    and implement it consistently for all method types.

    @fniessink
    Copy link
    Mannequin Author

    fniessink mannequin commented Mar 9, 2008

    Just to reiterate the original bug report: the issue (for me) is that
    currently (python 2.5):
    >>> [].__add__  == [].__add__
    True
    >>> [].__add__  == [1].__add__
    False
    
    Or, using a non-builtin class:
    >>> class C(object):
    ...   def __eq__(self, other):
    ...     return False
    ...   def foo(self):
    ...      pass
    ...
    >>> C().foo == C().foo
    False
    >>> class C(object):
    ...   def __eq__(self, other):
    ...     return True
    ...   def foo(self):
    ...     pass
    ...
    >>> C().foo == C().foo
    True

    I think it makes little sense that the equality test for the instance
    methods takes the equality of the instances into account. Imho, this
    behaviour is inconsistent with the principle of no surprises. The
    correct behaviour (again imho of course) is that instance methods only
    compare equal to the same instance method of the same instance, where
    'same instance' is based on 'is' not on '=='.

    Cheers, Frank

    @abalkin
    Copy link
    Member

    abalkin commented Mar 9, 2008

    Armin,

    I think this should be discussed on python-dev. I don't understand your
    motivation for comparing methods of distinct but equal objects as equal.
    The callback argument on the other hand, is compelling.

    @doublep
    Copy link
    Mannequin

    doublep mannequin commented Mar 9, 2008

    Since I'm not on python-dev, I'll mention here that the new behavior
    makes no sense to me at all. Which is best highlighted by Frank in
    msg63414.

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Mar 13, 2008

    Attached patch for python trunk with tests.
    It makes all three method types use the identity of their 'self'.

    @devdanzin
    Copy link
    Mannequin

    devdanzin mannequin commented Mar 30, 2009

    Confirmed in trunk and py3k.

    @devdanzin devdanzin mannequin added the type-feature A feature request or enhancement label Mar 30, 2009
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 16, 2010

    msg84597 is ambiguous. At first readng I thought it meant "the patches have been committed to trunk and py3k". Second time I think it means "yes this is still a problem". What is the status?

    @abalkin
    Copy link
    Member

    abalkin commented Jul 16, 2010

    I believe "Confirmed in trunk and py3k." means that the issue was verified to exist in trunk and py3k branches at the time of the message.

    @serhiy-storchaka
    Copy link
    Member

    Could you please update the patch for tip? I looks too desynchronized.

    @westleymartinez
    Copy link
    Mannequin

    westleymartinez mannequin commented Mar 12, 2014

    I updated the tests to be in sync, but the implementation of the fix is not so trivial. The conversion from cmp() to rich comparison is the primary culprit, so it will take time for me to get familiar enough with the C source to update the fix. I couldn't seem to get the patch to apply even to the 2.x branch (I think it's because it's an SVN patch...) to see if the fix actually works.

    That said, this enhancement is so old that it might not warrant a fix at all. Maybe it should be brought up on python-dev?

    @arigo arigo mannequin removed their assignment Mar 13, 2014
    @serhiy-storchaka
    Copy link
    Member

    Armin, do you mind to create a pull request on GitHub?

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Oct 28, 2017
    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Dec 12, 2017

    Sorry, I think it is pointless to spend efforts to keep a relatively uncontroversial and small patch up-to-date, when it was not accepted in 9 years. Someone else would need to take it up.

    @jdemeyer
    Copy link
    Contributor

    I just posted to python-dev about this issue:

    https://mail.python.org/pipermail/python-dev/2018-June/153959.html

    However, I think that comparing using "==" is the right thing to do. So I think that

    >>> [].append == [].append
    False

    should really return True.

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Jun 22, 2018

    If [].append == [].append is True in the "unique set of callbacks" scenario, that implies that it's perfectly fine to not call one of them when both are registered. But this means that only one list ends up getting updated, when you tried to register both for updates. That's definitely surprising, and not in a good way.

    @serhiy-storchaka
    Copy link
    Member

    PR 7848 is based on method_compare.diff and 1617161_test_update.diff, but also makes types.MethodWrapperType unorderable, adds more tests and fixes some outdated tests.

    This change can be considered as a bugfix, but since it can break the user code (unlikely), it may be safer to merge it only in 3.8 and expose as a new feature.

    But changes to the hash of types.BuiltinMethodType may be backported. Currently the hash is not consistent with the equality.

    @serhiy-storchaka serhiy-storchaka added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Jun 22, 2018
    @serhiy-storchaka
    Copy link
    Member

    New changeset ac20e0f by Serhiy Storchaka in branch 'master':
    bpo-1617161: Make the hash and equality of methods not depending on the value of self. (GH-7848)
    ac20e0f

    @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
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants