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

Handle NotImplemented from __ne__ #4770

Merged
merged 11 commits into from Mar 28, 2018

Conversation

Projects
None yet
3 participants
@kamahen
Contributor

kamahen commented Mar 20, 2018

Also change sharedparse.py to avoid this kind of bug in future.
Fixes #4767.

Handle NotImplemented from __ne__; improve test cases
Also change sharedparse.py to avoid this kind of bug in future.
Fixes #4767.
class A:
def __eq__(self, other: object) -> bool: return NotImplemented
def __init__(self, x: Any) -> None:
self.x = x

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Mar 20, 2018

Collaborator

Indent with four spaces.

This comment has been minimized.

@kamahen

kamahen Mar 20, 2018

Contributor

Done

if self.__class__ is other.__class__:
return bool(self.x != other.x)
else:
# pylint will complain about this return not needing

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Mar 20, 2018

Collaborator

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

This comment has been minimized.

@kamahen

kamahen Mar 20, 2018

Contributor

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?

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Mar 20, 2018

Collaborator

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.

This comment has been minimized.

@kamahen

kamahen Mar 20, 2018

Contributor

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.

This comment has been minimized.

@kamahen

kamahen Mar 27, 2018

Contributor

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

# 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]]",

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Mar 20, 2018

Collaborator

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

This comment has been minimized.

@kamahen

kamahen Mar 20, 2018

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

This comment has been minimized.

@gvanrossum

gvanrossum Mar 21, 2018

Member

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

This comment has been minimized.

@kamahen

kamahen Mar 27, 2018

Contributor

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

This comment has been minimized.

@kamahen

kamahen Mar 27, 2018

Contributor

Opened issue #4791.

kamahen added some commits Mar 20, 2018

Handle NotImplemented from __ne__; improve test cases
Also change sharedparse.py to avoid this kind of bug in future.
Fixes #4767.
Handle NotImplemented from __ne__; improve test cases
Also change sharedparse.py to avoid this kind of bug in future.
Fixes #4767.
"__getattribute__",
"__getitem__",
"__hex__",
"__init__",

This comment has been minimized.

@gvanrossum

gvanrossum Mar 21, 2018

Member

Should __index__ be added here?

This comment has been minimized.

@kamahen

kamahen Mar 27, 2018

Contributor

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

This comment has been minimized.

@kamahen

kamahen Mar 27, 2018

Contributor

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.

This comment has been minimized.

@kamahen

kamahen Mar 27, 2018

Contributor

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

This comment has been minimized.

@kamahen

kamahen Mar 27, 2018

Contributor

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

# 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]]",

This comment has been minimized.

@gvanrossum

gvanrossum Mar 21, 2018

Member

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

Refactored sets of magic methods to make it more obvious what changes…
… I had made.

  (Also removed a few more dups)
@gvanrossum

I am really unhappy with this change. I spent 10 minutes writing a comparison between the old and new contents of each set and found several serious problems. We should not do this refactor and instead go for the smallest diff possible (adding __ne__ somewhere).

@kamahen

This comment has been minimized.

Contributor

kamahen commented Mar 27, 2018

return bool(self.x != other.x)
else:
return NotImplemented
# TODO: If the programmer specifies "-> Union[bool, type(NotImplemented)]",

This comment has been minimized.

@gvanrossum

gvanrossum Mar 27, 2018

Member

The need for all these comments here still makes me unhappy, and nothing seems to answer the question why __ne__ is special.

class A:
def __eq__(self, other: object) -> bool: return NotImplemented
# The commented-out items give a warning: Returning Any from function declared to return "A"

This comment has been minimized.

@gvanrossum

gvanrossum Mar 27, 2018

Member

Well given the reason why NotImplemented exists that's not surprising. Do any of these do something reasonable at runtime? (Where reasonable == "triggers some default action other than passing through NotImplemented".)

The crashes are interesting though -- please report those separately.

@kamahen

This comment has been minimized.

Contributor

kamahen commented Mar 27, 2018

I've removed all the commented-out tests ... only the tests that pass are there now. (I'll do separate PRs for the refactoring and the tests that aren't passing).

For looking at the validity of NotImplemented, I'd need to inspect the inner workings of both mypy and the Python interpreter (the documentation is a bit hand-wavy on the details, although if I think about it hard enough, I think the answer about whether NotImplemented makes sense or not for an individual __ method is "obvious"). One additional question ... is it intended that NotImplemeted be only used by __ methods that correspond to operators, or should it be available for general use?

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Mar 27, 2018

@kamahen

This comment has been minimized.

Contributor

kamahen commented Mar 27, 2018

@@ -145,8 +145,70 @@ main:4: warning: Returning Any from function declared to return "int"
[case testReturnAnyForNotImplementedInBinaryMagicMethods]
# flags: --warn-return-any
from typing import Any
class A:
def __init__(self, x: Any) -> None:

This comment has been minimized.

@gvanrossum

gvanrossum Mar 28, 2018

Member

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.

@kamahen kamahen changed the title from Handle NotImplemented from __ne__; improve test cases to Handle NotImplemented from __ne__ Mar 28, 2018

@kamahen

This comment has been minimized.

Contributor

kamahen commented Mar 28, 2018

It's now a 1-line fix ... I'll deal with the other issues separately. Some of them may not be worth the price of "improvement". ;)

@gvanrossum gvanrossum merged commit 8b8d285 into python:master Mar 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

kamahen added a commit to kamahen/mypy that referenced this pull request Apr 2, 2018

Refactor "magic" method constants so that there are no dups
  to avoid bugs like issue python#4770
This has been hand-checked to verify that MAGIC_METHODS is the same
  (the other constants have only been moved within the file)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment