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

Return NotImplemented in newstr.__eq__ #432

Closed
max-vogler opened this issue Jan 21, 2019 · 4 comments · Fixed by #462
Closed

Return NotImplemented in newstr.__eq__ #432

max-vogler opened this issue Jan 21, 2019 · 4 comments · Fixed by #462
Labels

Comments

@max-vogler
Copy link

@max-vogler max-vogler commented Jan 21, 2019

We stumbled upon an edge case in our codebase, related to newstr.__eq__ returning False instead of NotImplemented. Is this intended?

def __eq__(self, other):
if (isinstance(other, unicode) or
isinstance(other, bytes) and not isnewbytes(other)):
return super(newstr, self).__eq__(other)
else:
return False

For clarification, returning False in __eq__ indicates a successful comparison of inequal objects. Judging by the code, the else branch should return Notimplemented, which indicates that the check was not successful. This allows the other object's __eq__ to be called.

See Python 2 data model and Python 3 data model:

A rich comparison method may return the singleton NotImplemented if it does not implement the operation for a given pair of arguments. By convention, False and True are returned for a successful comparison.
[...]
Numeric methods and rich comparison methods should return [NotImplemented] if they do not implement the operation for the operands provided. (The interpreter will then try the reflected operation, or some other fallback, depending on the operator.)

Our codebase has string-like objects that are not a subclass of str or unicode but have identical semantics. Whether this is a good idea is big internal debate, but for now it is part of the codebase. This leads to asymmetrical equality, which we would like to fix:

our_str = OurCustomString("foobar")
new_str = future.builtins.str("foobar")

our_str == new_str  # -> True
new_str == our_str  # -> False
@jmadler jmadler added 0.18 bug question string and removed question labels May 6, 2019
@jmadler

This comment has been minimized.

Copy link
Collaborator

@jmadler jmadler commented May 7, 2019

Using this code as a reference:

class CustomStringClass(object):

    def __init__(self, string):
        self.string = string

    def __str__(self):
        return self.string

fb = "foobar"
cs = CustomStringClass("foobar")
print(type(fb), type(cs))
print(fb == cs)
print(cs == fb)

Python 3 and 2's built-in string functions produce the same result:

<class 'str'> <class '__main__.CustomStringClass'>
False
False

This behavior is true regardless of the presence of the __str__ method, suggesting the reference interpreters do not coerce the object into a string primitive type as part of its __eq__ implementation.

It's true that this is misaligned with the documented data model, and I'll follow-up with the core python development team. I'd prefer to keep the behavior consistent with python built-ins as much as possible, so we'll follow whatever decision is made there.

@max-vogler

This comment has been minimized.

Copy link
Author

@max-vogler max-vogler commented May 7, 2019

The issue we encountered is slightly different. CustomStringClass does not define __eq__ and coercion is not what I would expect.

Instead, consider this example:

from __future__ import print_function
import platform
from future.types import newstr


class CustomString(object):

    def __init__(self, string):
        self.string = string

    def __eq__(self, other):
        if isinstance(other, (str, newstr)):
            return self.string == other
        else:
            return NotImplemented


native = "foo"
custom = CustomString("foo")

print("Python", platform.python_version())
print("native", native == custom, custom == native)

if platform.python_version_tuple()[0] == "2":
    future = newstr("foo")
    print("future", future == custom, custom == future)

Running it with Python 2 and 3 results in the following output:

> python2 misc/futuretest.py
Python 2.7.15
native True True
future False True

> python3 misc/futuretest.py
Python 3.7.2
native True True

Both Python 2 and 3's native string types correctly handle the symmetry of ==. future.types.newstr on the other hand does not expose this symmetry, because its __eq__ implementation returns False instead of NotImplemented.

@ethanfurman

This comment has been minimized.

Copy link

@ethanfurman ethanfurman commented May 7, 2019

Max-Vogler is correct -- when implementing the rich-comparison operators care should be taken to only return True/False when the object knows how it compares to the other object. If it doesn't know, it should return NotImplemented1 and let Python figure out the next step.


1 Note: raise NotImplementedError is not the same and should not be used.

@jmadler jmadler reopened this May 8, 2019
@jmadler

This comment has been minimized.

Copy link
Collaborator

@jmadler jmadler commented May 8, 2019

Ah! I was unaware of that detail about the layer of indirection between __eq__() and the == operator. Sorry about that. I'll get this addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.