Skip to content

Update dependency pyright to v1.1.359 #11780

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Apr 18, 2024

Mend Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
pyright ==1.1.358 -> ==1.1.359 age adoption passing confidence

Release Notes

RobertCraigie/pyright-python (pyright)

v1.1.359

Compare Source


Configuration

📅 Schedule: Branch creation - "before 4am" (UTC), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

@renovate renovate bot added the bot: dependencies 🤖 Dependency file updates by renovate 🤖 label Apr 18, 2024
@Avasam
Copy link
Collaborator

Avasam commented Apr 19, 2024

Hum, I guess this new rule / spec change makes sense on the surface, probably simplifies type-checkers' job. We were unfortunately using TypeVars in __init__'s self generic params to hide away explicit Any ^^"

@renovate renovate bot changed the title chore(deps): update dependency pyright to v1.1.359 Update dependency pyright to v1.1.359 Apr 19, 2024
@srittau
Copy link
Collaborator

srittau commented Apr 19, 2024

I'm not sure how we are supposed to type the following case now:

    def __init__(self: dict[str, _VT], **kwargs: _VT) -> None: ...

This looks to be a regression.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 19, 2024

I'm not sure how we are supposed to type the following case now:

    def __init__(self: dict[str, _VT], **kwargs: _VT) -> None: ...

This looks to be a regression.

I suppose pyright might like us to do this instead, so that the TypeVar in the constructor method is unambiguously scoped to the function rather than the class:

_T = TypeVar("_T")
_KT = TypeVar("_KT")
_VT = TypeVar("_VT")

class dict(Generic[_KT, _VT]):
    def __new__(cls, **kwargs: _T) -> dict[str, _T]: ...

Unfortunately, however, there's quite a few generic classes that have __init__ methods but don't have __new__ methods. I don't love the idea of lying about those methods and pretending that they have __new__ methods when they actually don't.

@AlexWaygood
Copy link
Member

It appears that the new pyright check was added in microsoft/pyright#7691, which was added in response to a change to the typing spec in python/typing#1667. The change to the typing spec was signed off by the typing council in python/typing-council#25.

@srittau
Copy link
Collaborator

srittau commented Apr 21, 2024

Considering that this typing-council change was extensive, and this point in particular did not seem to have been discussed, I'd hold off on regressing on our stubs for now. This seems like an oversight in the new spec.

@srittau
Copy link
Collaborator

srittau commented Apr 21, 2024

@Akuli
Copy link
Collaborator

Akuli commented Apr 21, 2024

Maybe we could just use a different TypeVar in the __init__ annotation? I get this with pyright 1.1.359:

from __future__ import annotations
from typing import Generic, TypeVar

_KT = TypeVar("_KT")
_VT = TypeVar("_VT")
_T = TypeVar("_T")

class Foo(Generic[_KT, _VT]):
    def __init__(self: Foo[str, _T], **kwargs: _T) -> None: ...

reveal_type(Foo(x=1, y=2))  # information: Type of "Foo(x=1, y=2)" is "Foo[str, int]"

@erictraut
Copy link
Contributor

In this case, you should use a function-scoped TypeVar rather than a class-scoped TypeVar, as shown in @Akuli's example. Using a class-scoped TypeVar here is problematic in the general case (leads to ambiguities in meaning), which is why it's now disallowed by the typing spec. Mypy doesn't tend to run into these ambiguities because its handling of constructor calls is quite naive; for example, it completely ignores the return type of a __new__ method if there's also an __init__ method. Once it improves its support for constructor calls, it would have run into these same problems.

The use of a class-scoped TypeVar in a self annotation for __init__ happens to work in the few places where typeshed stubs currently uses it because you've been very careful not to create any conflicts (e.g. with a __new__ method that returns a specialized type). While pyright now generates an error if you use a class-scoped TypeVar here (which is mandated by the updated spec), it still evaluates the intended type for the constructor call.

Unfortunately, mypy doesn't currently handle the case where a function-scoped TypeVar is used here. Until it does, I think it makes sense to leave the typeshed stubs unmodified. Once mypy is updated to conform with the typing spec in this area, then I recommend that you switch all of these cases to use function-scoped TypeVars.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 22, 2024

Thanks @erictraut -- I now agree that pyright's behaviour is correct here (and better than mypy's). Unfortunately, as you say, switching to TypeVars that are unambiguously function-scoped won't be possible for us until mypy becomes conformant with the spec. #11802 switched only a few TypeVars in constructor methods to be unambiguously function-scoped, and the mypy_primer output is pretty catastrophic.

Would you consider making this new check a dedicated pyright diagnostic? That would enable us to easily bump pyright to the latest version and -- while we wait for mypy to become conformant with the new spec -- switch off this new check in CI for now. Otherwise we may be unable to upgrade to the latest version of pyright in CI for a while. There are so many CI errors that adding pyright: ignore to all of them may not be a feasible approach for us.

@AlexWaygood
Copy link
Member

By the way, one edge case (and it is an edge case -- I don't think it brings into doubt the validity of the new pyright check) that I came up against while working on #11802 was this overload for the constructor of collections.defaultdict:

@overload
def __init__(self, default_factory: Callable[[], _VT] | None, /) -> None: ...

I don't know how to type this overload according to the new rules. I think what I'd want is the following behaviour:

d = defaultdict(list[int])  # `d` inferred as `defaultdict[Unknown, list[int]]`
d: defaultdict[str, list[int]] = defaultdict(list)  # `d` inferred as `defaultdict[str, list[int]]`

But I don't know how to get type checkers to infer that if I can only use function-scoped TypeVars.

@srittau
Copy link
Collaborator

srittau commented Apr 22, 2024

Shouldn't we be able to # pyright: ignore these errors for now, until mypy is fixed?

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 22, 2024

Shouldn't we be able to # pyright: ignore these errors for now, until mypy is fixed?

We can, but it's just a lot of pyright: ignores. I'd prefer to have a single diagnostic we could just switch off for now.

@erictraut
Copy link
Contributor

Would you consider making this new check a dedicated pyright diagnostic?

Sorry, but I can't justify making a separate diagnostic rule just for this error. The bar is high for adding a new diagnostic rule. It's something we need to document and maintain forever.

I could move it from reportGeneralTypeIssue to reportInvalidTypeVarUse, which would perhaps be more palatable to disable.

Out of curiosity, in how many locations does this appear in typeshed currently? Is it just a few or hundreds? If it's just a few, then # pyright: ignore might be the best route.

@JelleZijlstra
Copy link
Member

I could move it from reportGeneralTypeIssue to reportInvalidTypeVarUse, which would perhaps be more palatable to disable.

I think that would be better, yes.

Out of curiosity, in how many locations does this appear in typeshed currently?

There are 43.

@AlexWaygood
Copy link
Member

Sorry, but I can't justify making a separate diagnostic rule just for this error. The bar is high for adding a new diagnostic rule. It's something we need to document and maintain forever.

Understandable.

I could move it from reportGeneralTypeIssue to reportInvalidTypeVarUse, which would perhaps be more palatable to disable.

That makes sense to me, even if we end up going the pyright: ignore route. We prefer to only ignore specific pyright diagnostics inline where possible, and I'd feel a lot better about doing # pyright: ignore[reportInvalidTypeVarUse] than # pyright: ignore[reportGeneralTypeIssue]. The latter feels like it could possibly hide a lot more issues.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 22, 2024

There are 43.

I'm not sure how many it is (I'm trying to find out now), but it's more than 43, because the pyright 3.8 job only reports type errors on branches that exist on Python 3.8, and the pyright 3.10 job only reports type errors on branches that exist on Python 3.10, etc. For contextlib.nullcontext, for example, the pyright 3.10+ jobs report an error on the sys.version_info >= (3, 8) branch, and the pyright <=3.9 jobs report an error on the other branch:

if sys.version_info >= (3, 10):
class nullcontext(AbstractContextManager[_T, None], AbstractAsyncContextManager[_T, None]):
enter_result: _T
@overload
def __init__(self: nullcontext[None], enter_result: None = None) -> None: ...
@overload
def __init__(self: nullcontext[_T], enter_result: _T) -> None: ...
def __enter__(self) -> _T: ...
def __exit__(self, *exctype: Unused) -> None: ...
async def __aenter__(self) -> _T: ...
async def __aexit__(self, *exctype: Unused) -> None: ...
else:
class nullcontext(AbstractContextManager[_T, None]):
enter_result: _T
@overload
def __init__(self: nullcontext[None], enter_result: None = None) -> None: ...
@overload
def __init__(self: nullcontext[_T], enter_result: _T) -> None: ...
def __enter__(self) -> _T: ...
def __exit__(self, *exctype: Unused) -> None: ...

erictraut added a commit to microsoft/pyright that referenced this pull request Apr 22, 2024
…nnotation in `__init__`. It was previously reported under `reportGeneralTypeIssues`, but it's now moved to `reportInvalidTypeVarUse`. This was done to help typeshed maintainers migrate away from this pattern. python/typeshed#11780 (comment)
@erictraut
Copy link
Contributor

OK, I've moved this check from reportGeneralTypeIssue to reportInvalidTypeVarUse. This rule has a warning severity level by default, so if you're using pyright's "standard" type checking mode (the default), you will see this change from an error to a warning when you update to the next pyright release.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 22, 2024

OK, I've moved this check from reportGeneralTypeIssue to reportInvalidTypeVarUse.

Thanks very much!

This rule has a warning severity level by default, so if you're using pyright's "standard" type checking mode (the default), you will see this change from an error to a warning when you update to the next pyright release.

We use the "strict" change for most of our stdlib stubs and many of our third-party stubs.

#11810 is what it looks like if we just pyright: ignore all the errors away. The number of pyright: ignores is in the forties somewhere, but black ends up exploding a lot of __init__ definitions across multiple lines as (unlike type: ignores), it doesn't view pyright: ignore comments as part of the AST.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bot: dependencies 🤖 Dependency file updates by renovate 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants