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

Jump to bottom

Handle NotImplemented from __ne__ #4770

Merged
merged 11 commits into from Mar 28, 2018
Merged

Handle NotImplemented from __ne__ #4770

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4ac02d5
Handle NotImplemented from __ne__; improve test cases
kamahen Mar 20, 2018
dafce6c
Handle NotImplemented from __ne__; improve test cases
kamahen Mar 20, 2018
4dfae58
Merge branch 'master' of https://github.com/kamahen/mypy
kamahen Mar 20, 2018
c084266
Handle NotImplemented from __ne__; improve test cases
kamahen Mar 20, 2018
2a1bd83
adjusted comment in test to remove reference to pylint
kamahen Mar 20, 2018
bc76eed
added more __xxx__ methods and tests
kamahen Mar 21, 2018
7ce5488
Refactored sets of magic methods to make it more obvious what changes…
kamahen Mar 27, 2018
6176031
revert sharedparse.py and add only __ne__; comment out tests that fail
kamahen Mar 27, 2018
e969ca2
Removed commented-out tests that currently fail (they'll be dealt wit…
kamahen Mar 27, 2018
a5c1c7f
Merge branch 'master' of git://github.com/python/mypy
kamahen Mar 28, 2018
67b979f
remove extra tests; these will be handled separately
kamahen Mar 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
114 changes: 36 additions & 78 deletions mypy/sharedparse.py
View file
Open in desktop
Expand Up @@ -3,67 +3,39 @@
"""Shared logic between our three mypy parser files."""


MAGIC_METHODS = {
"__abs__",
BINARY_MAGIC_METHODS = {
"__add__",
"__and__",
"__call__",
"__cmp__",
"__complex__",
"__contains__",
"__del__",
"__delattr__",
"__delitem__",
"__divmod__",
"__div__",
"__enter__",
"__exit__",
"__eq__",
"__floordiv__",
"__float__",
"__ge__",
"__getattr__",
"__getattribute__",
"__getitem__",
"__gt__",
"__hex__",
"__iadd__",
"__iand__",
"__idiv__",
"__ifloordiv__",
"__ilshift__",
"__imod__",
"__imul__",
"__init__",
"__init_subclass__",
"__int__",
"__invert__",
"__ior__",
"__ipow__",
"__irshift__",
"__isub__",
"__iter__",
"__ixor__",
"__le__",
"__len__",
"__long__",
"__lshift__",
"__lt__",
"__mod__",
"__mul__",
"__ne__",
"__neg__",
"__new__",
"__nonzero__",
"__oct__",
"__or__",
"__pos__",
"__pow__",
"__radd__",
"__rand__",
"__rdiv__",
"__repr__",
"__reversed__",
"__rfloordiv__",
"__rlshift__",
"__rmod__",
Expand All @@ -74,13 +46,45 @@
"__rshift__",
"__rsub__",
"__rxor__",
"__sub__",
"__xor__",
}


MAGIC_METHODS = {
"__abs__",
"__call__",
"__complex__",
"__contains__",
"__del__",
"__delattr__",
"__delitem__",
"__enter__",
"__exit__",
"__float__",
"__getattr__",
"__getattribute__",
"__getitem__",
"__hex__",
"__init__",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should __index__ be added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should add test cases for all the operators, to ensure that nothing is missing.

(I don't know enough about index to know whether it should be in this list or not.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a bunch more methods; not entirely sure if they should all be there (e.g., __call__, __del__).

The results are inconsistent. If you look in the test case, I've commented out the ones that fail. I'm guessing that there's "magic" code for handling some of them and that magic code doesn't cover all the cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gvanrossum - this change is a bit complicated to review because I also refactored it to avoid the problem of a method being specified in MAGIC_BINARY_METHODS but not in MAGIC_METHODS. Would it make things easier if I reverted that refactoring and submitted it as a separate change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gvanrossum - I've refactored my changes a bit to make the changes more obvious.

"__init_subclass__",
"__int__",
"__invert__",
"__iter__",
"__len__",
"__long__",
"__neg__",
"__new__",
"__nonzero__",
"__oct__",
"__pos__",
"__repr__",
"__reversed__",
"__setattr__",
"__setitem__",
"__str__",
"__sub__",
"__unicode__",
"__xor__",
}
}.union(BINARY_MAGIC_METHODS)

MAGIC_METHODS_ALLOWING_KWARGS = {
"__init__",
Expand All @@ -91,52 +95,6 @@

MAGIC_METHODS_POS_ARGS_ONLY = MAGIC_METHODS - MAGIC_METHODS_ALLOWING_KWARGS

BINARY_MAGIC_METHODS = {
"__add__",
"__and__",
"__cmp__",
"__divmod__",
"__div__",
"__eq__",
"__floordiv__",
"__ge__",
"__gt__",
"__iadd__",
"__iand__",
"__idiv__",
"__ifloordiv__",
"__ilshift__",
"__imod__",
"__imul__",
"__ior__",
"__ipow__",
"__irshift__",
"__isub__",
"__ixor__",
"__le__",
"__lshift__",
"__lt__",
"__mod__",
"__mul__",
"__or__",
"__pow__",
"__radd__",
"__rand__",
"__rdiv__",
"__rfloordiv__",
"__rlshift__",
"__rmod__",
"__rmul__",
"__ror__",
"__rpow__",
"__rrshift__",
"__rshift__",
"__rsub__",
"__rxor__",
"__sub__",
"__xor__",
}


def special_function_elide_names(name: str) -> bool:
return name in MAGIC_METHODS_POS_ARGS_ONLY
Expand Down
23 changes: 22 additions & 1 deletion test-data/unit/check-warnings.test
View file
Open in desktop
Expand Up @@ -145,8 +145,29 @@ main:4: warning: Returning Any from function declared to return "int"

[case testReturnAnyForNotImplementedInBinaryMagicMethods]
# flags: --warn-return-any
from typing import Any
class A:
def __eq__(self, other: object) -> bool: return NotImplemented
def __init__(self, x: Any) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the tests in this file are specifically for the --warn-*-any flags, and this particular test just tests that if a binary magic method returns NotImplemented you don't get a warning about returning Any (which apparently requires some effort since NotImplemented is declared as having type Any in typeshed as well as in test-data/unit/fixtures/notimplemented.pyi.

Tests that all binary methods support returning NotImplemented (or that other methods don't!) belong somewhere else, and tests that mypy infers the right return type when various forms of branching are present belong elsewhere again.

I trust that the branching logic is very well tested already, so I don't think we need more tests for that. As to where the basic return NotImplemented tests should go, I think the best place is probably check-classes.test. This is somewhat arbitrary.

self.x = x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent with four spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

def __eq__(self, other: Any) -> bool:
if self.__class__ is other.__class__:
return bool(self.x == other.x)
return NotImplemented
def __lt__(self, other: Any) -> bool: return NotImplemented
def __gt__(self, other: Any) -> bool: return NotImplemented
def __le__(self, other: Any) -> bool: return NotImplemented
def __ge__(self, other: Any) -> bool: return NotImplemented
def __ne__(self, other: Any) -> bool:
if self.__class__ is other.__class__:
return bool(self.x != other.x)
else:
# pylint will complain about this return not needing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems irrelevant; we don't run pylint on test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I put this comment in is that someone might think that the "else" could be removed, leaving the "return" outside then if-then-else (pylint encourages this kind of thing). Do you want me to just remove the comment about pylint complaining?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the comment is helpful, but I won't be the person deciding whether to merge this.

Why does it matter whether the else is there? It seems to me that it shouldn't matter to mypy, since the control flow should be the same in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type inferencing might produce different results (I'm essentially doing black-box test thinking). So, I have two ways of generating a Union result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the comment about pylint complaining, and added an explanation of why I have the test done two different ways

# an "else", but want to check that Union[bool, NotImplemented]
# works as the inferred return typ
return NotImplemented
# TODO: If the programmer specifies "-> Union[bool, type(NotImplemented)]",
# mypy suggests changing this to "-> Union[bool, type[NotImplemented]]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Type[NotImplemented] work? Maybe we should suggest that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

>>> from typing import *
>>> Type[NotImplemented]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.5/typing.py", line 996, in __getitem__
    params = tuple(_type_check(p, msg) for p in params)
  File "/usr/lib/python3.5/typing.py", line 996, in <genexpr>
    params = tuple(_type_check(p, msg) for p in params)
  File "/usr/lib/python3.5/typing.py", line 312, in _type_check
    raise TypeError(msg + " Got %.100r." % (arg,))
TypeError: Parameters to generic types must be types. Got NotImplemented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a runtime error, and there's nothing you can do about it. The return type should just be -> bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I should open another bug report instead of discussing here ... anyway, there seem to be two issues:

  1. Only magic methods can return NotImplemented without a complaint from mypy
  2. If the programmer tries to get rid of the mypy complaint by specifying -> Union[bool, type(NotImplemented)], mypy doesn't like that and suggests changing it to -> Union[bool, type[NotImplemented]], which gets a runtime error (Type[NotImplemented] doesn't work either).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened issue #4791.

# which gives a run-time error (issue #4767)
[builtins fixtures/notimplemented.pyi]
[out]

Expand Down
4 changes: 3 additions & 1 deletion test-data/unit/fixtures/notimplemented.pyi
View file
Open in desktop
Expand Up @@ -3,11 +3,13 @@ from typing import Any, cast


class object:
__class__ = None
def __init__(self) -> None: pass

class type: pass
class function: pass
class bool: pass
class bool:
def __init__(self, o: Any) -> None: pass
class int: pass
class str: pass
NotImplemented = cast(Any, None)