Fix typing of _infer to allow a return value in the Generator
#1655
Conversation
| def infer( | ||
| self: _ProxyT, context: InferenceContext | None = None, **kwargs: Any | ||
| ) -> collections.abc.Generator[_ProxyT, None, None]: | ||
| def infer( # type: ignore[return] |
There was a problem hiding this comment.
mypy complains about wanting a return None statement whereas pylint complains about useless-return. Who is right here?
There was a problem hiding this comment.
I don't think you need an explicit return at the end of a generator, so I think mypy is wrong... But they say in their doc:
You should give a statically typed function an explicit None return type even if it doesn’t return a value, as this lets mypy catch additional type errors:
So you must be explicit to help mypy it seems.
There was a problem hiding this comment.
I think that refers to:
def func(x: str):
print(x)Which should have a -> None as well.
There was a problem hiding this comment.
I would say this is probably a mypy bug, i.e. let's leave the type: ignore[return] comment.
Mypy should only require an explicit return inside a Generator if None is not an option.
@DanielNoord Would you like to report it in the mypy repo?
There was a problem hiding this comment.
Yeah I'll create a small reproducer in the coming days.
There was a problem hiding this comment.
Probably needs to be an option as it's a little opinionated, we were both agreeing that mypy is wrong here. As JelleZijlstra said they're consistent but another typer (or pylint) could take another decision. I don't think there's a consensus here. But the real question is what should the default value be. mypy is used a lot so probably the one mypy chose to reduce friction.
There was a problem hiding this comment.
Thanks for opening the issue @DanielNoord!
But the real question is what should the default value be. mypy is used a lot so probably the one mypy chose to reduce friction.
I would agree. Both checks (mypy and pylint) are opinionated. The only thing for us is to make sure they don't collide if a users chooses to enable either one.
Should we update
pylintto allowreturn Nonefor annotated functions?
Would it make sense to go one step further and disable useless-return entirely by default?
There was a problem hiding this comment.
Would it make sense to go one step further and disable useless-return entirely by default?
Yeap, we don't need an option, my bad.
There was a problem hiding this comment.
We should really tweak the default values of pylint.
There was a problem hiding this comment.
We should really tweak the default values of pylint.
Agreed. I'm thinking about applying 3512 but there's a lot of other issues to handle and I never have the time. This is probably one of the big pain point of pylint right now though. The issue that take us a lot of time can be very niche in very specific cases sometime but default values are really freaking important. (Also invalid-name for < 3 char value. I get bitten by it for new project myself, first thing I need to do is always to disable this warning, someone trying pylint always talk to me about it, it's not a good look).
Pull Request Test Coverage Report for Build 2547380225
💛 - Coveralls |
| def infer( | ||
| self: _ProxyT, context: InferenceContext | None = None, **kwargs: Any | ||
| ) -> collections.abc.Generator[_ProxyT, None, None]: | ||
| def infer( # type: ignore[return] |
There was a problem hiding this comment.
I would say this is probably a mypy bug, i.e. let's leave the type: ignore[return] comment.
Mypy should only require an explicit return inside a Generator if None is not an option.
@DanielNoord Would you like to report it in the mypy repo?
Steps
Description
Ref. #1653.
Type of Changes
Related Issue