python / importlib_metadata

Library to access metadata for Python packages
https://importlib-metadata.readthedocs.io
Apache License 2.0
122 stars 79 forks source link

Invalid annotation in `SimplePath` protocol #468

Closed sobolevn closed 7 months ago

sobolevn commented 10 months ago

Hi!

I was working on typeshed's types and we (well, @AlexWaygood) found that annotation of __truediv__ is not valid: https://github.com/python/importlib_metadata/blob/8e7b9689fc38ef3e834265fd78423a9933a33607/importlib_metadata/_meta.py#L55-L56

Here's a small repro: https://mypy-play.net/?mypy=latest&python=3.11&gist=2f16bbbbaf5a1330cbbcae76b5987868

from typing import TypeVar

T = TypeVar('T')

class A:
    def method(self, other: T | str) -> T: ...

a: A
reveal_type(a.method(1))  # N: Revealed type is "builtins.int"
reveal_type(a.method('a'))  # N: Revealed type is "<nothing>"

I propose using an @overload instead:


from typing import TypeVar, overload

T = TypeVar('T')

class A:
    @overload
    def method(self, other: str) -> str: ...
    @overload
    def method(self, other: T) -> T: ...

a: A
reveal_type(a.method(1))  # N: Revealed type is "builtins.int"
reveal_type(a.method('a'))  # N: Revealed type is "builtins.str"

Playground: https://mypy-play.net/?mypy=latest&python=3.11

I will send a PR with the fix :)

sobolevn commented 10 months ago

Or we can just drop str part :)

Because _T has no bounds there.

jaraco commented 7 months ago

Based on a comment in #469, it sounds as if this concern isn't in fact a problem because SimplePath itself is parameterized by _T. For now, I'm marking this as invalid, but happy to revisit, especially if someone can demonstrate for me how the current definition manifests problematic behavior.