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

refactor: use typing_extensions.Self #9251

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bswck
Copy link
Member

@bswck bswck commented Mar 29, 2024

Used typing_extensions.Self instead of (self: T, ...) -> T.
typing_extensions has a special stub in typeshed and is seen by mypy even if not installed as a direct/transitive dependency, so there's no need to change anything in dependency specs.
typing_extensions was used instead of typing because the Self type was introduced in 3.11 with PEP 673 and typing_extensions offers a backport for 3.8+, which is our case.

Operation.skip() and Operation.unskip() have an anti-pattern, i.e. they return Self despite operating in-place.
This will be addressed in a separate PR; I need an idea how to address the API incompatibility that would bring.

@bswck bswck reopened this Mar 29, 2024
@bswck bswck changed the title Use typing_extensions.Self refactor: use typing_extensions.Self Mar 29, 2024
@dimbleby
Copy link
Contributor

typing_extensions has a special stub in typeshed and is seen by mypy even if not installed as a direct/transitive dependency, so there's no need to change anything in dependency specs.

citation required

mypy has a direct dependency on typing-extensions so you will get away with it. But eg if you uninstall mypy (and typing-extensions) and try pyright instead you will see the warning warning: Import "typing_extensions" could not be resolved from source (reportMissingModuleSource)

if poetry imports from typing-extensions then it should express that dependency directly.

@bswck
Copy link
Member Author

bswck commented Mar 29, 2024

typing_extensions has a special stub in typeshed and is seen by mypy even if not installed as a direct/transitive dependency, so there's no need to change anything in dependency specs.

citation required

Sure:

  1. From the official typing_extensions README:

    typing_extensions is treated specially by static type checkers such as mypy and pyright. Objects defined in typing_extensions are treated the same way as equivalent forms in typing.

    Moreover, I believe

    if poetry imports from typing-extensions then it should express that dependency directly.

    is not true if we import from typing_extensions only in TYPE_CHECKING blocks (i.e. not at runtime), as this PR proposes, because of (3) and (4).

  2. https://mypy.readthedocs.io/en/stable/stubs.html#stub-files

    Mypy uses stub files stored in the typeshed repository to determine the types of standard library and third-party library functions, classes, and other definitions.

    Due to this and (3), installing typing_extensions directly isn't inherently necessary.

  3. mypy has typing_extensions stubs in its (stdlib) typeshed vendor: https://github.com/python/mypy/blob/1.9.0/mypy/typeshed/stdlib/typing_extensions.pyi.

  4. You mentioned pyright, and pyright also vendors typeshed, and typing_extensions is also there (in the stdlib stubs) :)
    https://github.com/microsoft/pyright/blob/1.1.356/packages/pyright-internal/typeshed-fallback/stdlib/typing_extensions.pyi.

mypy has a direct dependency on typing_extensions so you will get away with it.

Yeah, I had been aware of that and happened to mention it, but I see it as irrelevant and personally think it shouldn't be relied on. AFAIK, it's not the direct reason typing_extensions can be reliably used in TYPE_CHECKING blocks without occuring in the project's dependencies, as proven earlier.

But eg if you uninstall mypy (and typing_extensions) and try pyright instead

First things first, if we rely on pyright additionally to mypy, I believe it should be included in the CI suite. Probably some issues that pyright would typically report (and mypy wouldn't due to some of their discrepancies) could have been (or can in the future be) merged undetected.

But eg if you uninstall mypy (and typing_extensions) and try pyright instead you will see the warning warning: Import "typing_extensions" could not be resolved from source (reportMissingModuleSource)

Well, that's weird since the typing_extensions stub is present in the pyright's internal typeshed vendor, as I mentioned before. It looks like an issue with pyright/pylance that I will inspect further. For the time of this issue being unresolved, temporarily adding typing_extensions as a dependency can be a workaround (but I must admit I'm not quite happy with that).

@dimbleby
Copy link
Contributor

From the official typing_extensions README ...

no part of what you quote says "therefore you do not need typing-extensions to be available in your environment.",

Most of the rest of what you write confuses stubs for typing-extensions with the typing-extensions package,

I think it is - for the moment, just about - true that it would be sufficient to put typing-extensions in the group of typing dependencies in pyproject.toml. However even that makes me uncomfortable

  • are future contributions going to remember that they are only allowed to import typing-extensions in if TYPE_CHECKING: blocks? eg typing-extensions includes plenty of decorators, it would be easy to be confused about things going wrong if we started to use one of those
  • it just is not the done thing. Possibly you can offer counter-examples, but I see plenty of projects depending on typing-extensions - including poetry itself until recently (5bad73c)

why even worry about adding the dependency?

@bswck
Copy link
Member Author

bswck commented Mar 30, 2024

no part of what you quote says "therefore you do not need typing-extensions to be available in your environment.",

It's a conclusion. It just isn't explicitly stated in the docs.

Most of the rest of what you write confuses stubs for typing-extensions with the typing-extensions package

Sorry, but I'm afraid you've completely missed my point.

are future contributions going to remember that they are only allowed to import typing-extensions in if TYPE_CHECKING: blocks? eg typing-extensions includes plenty of decorators, it would be easy to be confused about things going wrong if we started to use one of those.

When typing_extensions is unavailable at runtime, CI is going to fail.
Nobody needs to remember anything.

why even worry about adding the dependency?

Because it is unnecessary. Poetry already relies on a lot of packages.

If you're still hesitant that installing typing-extensions is completely unnecessary, here's my short conversation on the topic with @AlexWaygood, a CPython Core Developer, typeshed & typing_extensions maintainer and mypy contributor:
image

I hope that that is convincing enough, if I myself hadn't been.

@dimbleby
Copy link
Contributor

dimbleby commented Mar 30, 2024

as we have established, actual typecheckers do not approve of this approach: warning: Import "typing_extensions" could not be resolved from source (reportMissingModuleSource)

As I say, declaring the dependency only in the typing dependency group would I think be sufficient, though I still do not like it (for the reasons I already gave).

CI might or might not catch missing dependencies. Eg it is very plausible to fail to spot a missing dependency on a package that is made transitively available by a test dependency.

@AlexWaygood
Copy link

AlexWaygood commented Mar 30, 2024

as we have established, actual typecheckers do not approve of this approach: warning: Import "typing_extensions" could not be resolved from source (reportMissingModuleSource)

I don't think that warning would impact users of poetry? But yeah, it's true that that's a lint that pyright has and mypy doesn't

  • are future contributions going to remember that they are only allowed to import typing-extensions in if TYPE_CHECKING: blocks? eg typing-extensions includes plenty of decorators, it would be easy to be confused about things going wrong if we started to use one of those

FWIW, while @bswck is correct that it is strictly speaking unnecessary to list typing_extensions as a dependency just for type checkers to understand the symbols being imported, I also think this is a very valid point. Importing things in if TYPE_CHECKING blocks can be useful, but it can also make code more fragile and harder to understand; that's a trade-off you'll have to make for yourselves. We work hard at typing_extensions to minimise breakage between versions, and it's a single-module pure-Python package, so it shouldn't be a huge dependency to add by any means.

Anyway, I think both ways of doing it are workable, and it's up to you folks to weigh the tradeoff appropriately :-) ping me again if you need me!

@dimbleby
Copy link
Contributor

pyright lint seems to have been in response to a bug report saying "you are failing to detect that typing-extensions is missing" - microsoft/pyright#7318

possibly that fix was an overshoot and they could be convinced that the lint is only desirable for imports made outside of if TYPE_CHECKING:

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 this pull request may close these issues.

None yet

3 participants