Skip to content

Add a check for methods that hardcode their return type, but shouldn't #174

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

Merged
merged 35 commits into from
Feb 5, 2022

Conversation

AlexWaygood
Copy link
Collaborator

An attempt at implementing #168 (closes #168).

Marking this as a draft for now, as I don't think the error message is very good, and I'm not sure how to improve it. I would also be curious to hear what others think of the overall idea before I put any more work in, as well.

@AlexWaygood
Copy link
Collaborator Author

@AlexWaygood
Copy link
Collaborator Author

I think this is more-or-less there now.

@AlexWaygood AlexWaygood marked this pull request as ready for review February 3, 2022 15:38
@JelleZijlstra
Copy link
Collaborator

Thanks for putting this together, I'll take a look soon.

Are the remaining typeshed hits false positives?

@AlexWaygood
Copy link
Collaborator Author

AlexWaygood commented Feb 3, 2022

Thanks for putting this together, I'll take a look soon.

Are the remaining typeshed hits false positives?

There are several typing hits which are false positives; I think we should just disable the check in typing, as it's a special case (we don't want Iterator.__iter__ to return _typeshed.Self, as even though it's by far the most common implementation at runtime for iterators just to return self, it's not mandated for a class to do that in order to satisfy the Iterator interface).

There are two hits in builtins: zip and reversed. I think the reversed hit in builtins is a false positive. I need to check with zip.

The Python 2 hits just need a backport of the changes I've already made in the Python 3 stdlib, if we decide to go ahead with this.

@JelleZijlstra
Copy link
Collaborator

Maybe we can disable the check in classes that are Protocols? That seems like it would fix the Iterator case, and it's also generally good for Protocols to be less precise about the return type.

@JelleZijlstra
Copy link
Collaborator

zip.__iter__ and reversed.__iter__ do return self, I'll submit a PR.

@AlexWaygood
Copy link
Collaborator Author

zip.__iter__ and reversed.__iter__ do return self, I'll submit a PR.

Looks to me like zip.__iter__ returns self but reversed.__iter__ doesn't?

>>> class Foo(reversed): ...
...
>>> type(iter(Foo([1, 2, 3])))
<class 'list_reverseiterator'>
>>> class Foo(zip): ...
...
>>> type(iter(Foo([1, 2], [3, 4])))
<class '__main__.Foo'>

@AlexWaygood
Copy link
Collaborator Author

Maybe we can disable the check in classes that are Protocols? That seems like it would fix the Iterator case, and it's also generally good for Protocols to be less precise about the return type.

I was wrong: the three false positives in typing are BinaryIO.__enter__, TextIO.__enter__, and IO.__iter__. None of these classes are protocols, but, all of these methods are abstract methods. So we can just disable the check for abstract methods, which makes sense in exactly the same way as your suggestion re. protocols!

@AlexWaygood
Copy link
Collaborator Author

Nice, excluding abstractmethods got rid of the typing hits!

@AlexWaygood AlexWaygood requested a review from Akuli February 4, 2022 12:04
Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

We can merge once the typeshed check is green.

@AlexWaygood
Copy link
Collaborator Author

typeshed backport PR here: python/typeshed#7128

@AlexWaygood
Copy link
Collaborator Author

(python/typeshed#7123 will also be required in order to make the CI go green.)

@AlexWaygood AlexWaygood closed this Feb 4, 2022
@AlexWaygood AlexWaygood reopened this Feb 4, 2022
@AlexWaygood AlexWaygood closed this Feb 5, 2022
@AlexWaygood AlexWaygood reopened this Feb 5, 2022
@AlexWaygood
Copy link
Collaborator Author

Hooray!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect methods that hardcode class names in their return type, but shouldn't
3 participants