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

--warn-return-any should not complain on NotImplemented when using abc #4534

Closed
drtyrsa opened this issue Feb 2, 2018 · 7 comments · Fixed by #4545
Closed

--warn-return-any should not complain on NotImplemented when using abc #4534

drtyrsa opened this issue Feb 2, 2018 · 7 comments · Fixed by #4545

Comments

@drtyrsa
Copy link
Contributor

drtyrsa commented Feb 2, 2018

I'm using abc.abstractmethod:

@abc.abstractmethod
def encode(self) -> bytes:
    return NotImplemented

And I get a warning: warning: Returning Any from function declared to return "bytes"

I don't think something is wrong with the code, it's totally normal way to declare and describe an interface and mypy shouldn't complain about it.

@ilevkivskyi
Copy link
Member

Why do you think this is a standard pattern? I always thought that NotImplemented is reserved for reversed operators (like __radd__ vs __add__) and few other special methods (like __subclasshook__). I think the standard way of doing what you want is:

@abc.abstractmethod
def encode(self) -> bytes:
    raise NotImplementedError("Not a valid encoder -- implement `encode` before use")

and I hope mypy doesn't complain about this (if yes, then this should be fixed).

@gvanrossum
Copy link
Member

gvanrossum commented Feb 2, 2018 via email

@ilevkivskyi
Copy link
Member

@gvanrossum It is not possible because NotImplemented is actually used in many __op__ methods that return non-Any types. All such cases will be flagged as error by mypy if we type NotImplemented as object.

@ilevkivskyi
Copy link
Member

A simplest example is __eq__, it is required to return bool, and therefore a standard pattern like:

def __eq__(self, other: object) -> bool:
    if isinstance(other, type(self)):
        ...
    return NotImplemented

will be flagged as an error.

@gvanrossum
Copy link
Member

Right. Maybe --disallow-any-expr can be hacked to ignore this case?

@ilevkivskyi
Copy link
Member

Maybe --disallow-any-expr can be hacked to ignore this case?

Yes, this is easy, but would not be my priority TBH, if OP wants to make a PR then OK.

@drtyrsa
Copy link
Contributor Author

drtyrsa commented Feb 2, 2018

@ilevkivskyi I've reread the docs and you are right, I shouldn't use NotImplemented here. I'll make a PR about return NotImplemented case.

Strangely abc docs doesn't say anything about what should a body of an abstract method look like. PEP 3119 doesn't either (and has an example with pass).

I guess I should raise an exception here, thank you.

Hoboneer added a commit to Hoboneer/Test-Pygame-Framework that referenced this issue Mar 29, 2018
I made `_get_matching_entity_no_optional` as a workaround to enable
`get_matching_entities` to construct its entity views without having to check
for `None` every time-- I'm not even sure *how* it would do that, considering
the structure of the dict comprehension. So that means `get_matching_entity`
no longer takes `_checked` as a keyword argument because the code for checked
entities has been delegated to `_get_matching_entity_no_optional`.

I also made a bunch of other changes:

- Various type annotation changes to accomodate the complaints of the type checker
(mypy in this case)
- Changed `Aspect.__eq__`s type annotation and implementation-- Turns out that I
should return `NotImplemented` if the object doesn't support operations (==, >, etc.)
with its input. BUT mypy complains when it sees that `NotImplemented` is able to be
returned, rather than a value of type `bool`. So I made a hack to trick mypy
into thinking that the operator (method) doesn't return `NotImplemented`, but
actually does so at runtime-- though it seems that the mypy team recognises this
and are working on a fix/whatever.

Issue Reference:
- python/mypy#4534
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants