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

Incorrect explanation for PYI019 #8765

Closed
henribru opened this issue Nov 19, 2023 · 4 comments · Fixed by #8766
Closed

Incorrect explanation for PYI019 #8765

henribru opened this issue Nov 19, 2023 · 4 comments · Fixed by #8766
Labels
documentation Improvements or additions to documentation

Comments

@henribru
Copy link

henribru commented Nov 19, 2023

The explanation for PYI019 at https://docs.astral.sh/ruff/rules/custom-type-var-return-type/ seems incorrect to me. The rationale there applies when you use a fixed class as the return type (like what PYI034 flags), but not when you use a TypeVar. You can see that Mypy and Pyright both correctly infer return types on a subclass based on that example:

https://mypy-play.net/?mypy=latest&python=3.11&gist=523f6373f5a85dd8f78e8b7038d9d74d

https://pyright-play.net/?code=GYJw9gtgBALgngBwJYDsDmUkQWEMoAqiApgGoCGIAsAFC0D6AylALyEkUgAUAREzwEpatAMYAbcgGdJUAGJgwALlpRVUACbFgUevRTEA7rq7jJi2CQDaTALoAaKACpKaM1EkwQDx44DWBlzdUGAEoAFoAPh1GZRo1eKgAOmThOLVNbWAFLkliMWBzJgcXcwAjOBhiSVDI6NiEtWTE1PiAAXEpSQhiGAALMHUVdK0oUsoTMTd4BGJrRnsoEswUEPCopnqGpJS0htEJaSgAIXH5MAFzJtSQYgA3YnIxemniLhPuASEaG-vH55I3uMBIksmAuKUeIIvj8Hk8XoCQIkxtwAMyfIA

The PEP for Self also shows the TypeVar variant as being equivalent, but describes Self as "more intuitive and succinct". flake8-pyi doesn't really explain the rationale for the rule in their explanation, but based on this I assume it's actually just meant to be stylistic. If so Ruff's explanation should reflect this.

@charliermarsh
Copy link
Member

\cc @AlexWaygood - only if quick / easy for you, do you know if Y019 is about clearer style / intent, or correctness?

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 19, 2023

@henribru is correct — Y019 is purely concerned with style, not with correctness (though it is slightly easier to be correct if you use the more modern syntax — in a .py file, you don't have to worry about binding the TypeVar to a forward reference etc :)

The "use instead" snippet ruff gives in the docs also isn't ideal — currently it's this:

from typing import Self


class Foo:
    def __new__(cls: type[Self], *args: str, **kwargs: int) -> Self:
        ...

    def foo(self: Self, arg: bytes) -> Self:
        ...

    @classmethod
    def bar(cls: type[Self], arg: int) -> Self:
        ...

But part of the point of typing.Self is that (unlike a normal TypeVar) you can skip the annotations on cls or self — so a better "use instead" snippet would be this:

from typing import Self


class Foo:
    def __new__(cls, *args: str, **kwargs: int) -> Self:
        ...

    def foo(self, arg: bytes) -> Self:
        ...

    @classmethod
    def bar(cls, arg: int) -> Self:
        ...

@charliermarsh
Copy link
Member

Awesome, thanks @AlexWaygood for the characteristically clear + through answer! Will fix.

@charliermarsh charliermarsh added the documentation Improvements or additions to documentation label Nov 19, 2023
AlexWaygood added a commit to PyCQA/flake8-pyi that referenced this issue Nov 20, 2023
This PR tries to make the motivation for each flake8-pyi rule clear. As
ruff has reimplemented flake8-pyi's error codes, it's come up a number
of times that it isn't always 100% clear why we're enforcing a
particular rule: some of our error codes enforce things to do with
correctness, but some of them are purely stylistic. (See
astral-sh/ruff#8765 for a recent example.)
I've tried to divide our error codes into four broad categories to
rectify this.

Several of our rules are specifically to do with things working
differently in a stub file compared to at runtime. For some of these
rules, I've also tried to add a few more words to explain exactly how
stubs differ from `.py` files.
@AlexWaygood
Copy link
Member

I just overhauled our flake8-pyi docs in PyCQA/flake8-pyi@504ef82, so hopefully it should be a bit clearer in the future to figure out what the motivation is for each of our rules :)

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

Successfully merging a pull request may close this issue.

3 participants