Use a separate type variable for static methods on Output #16172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julienp
force-pushed
the
julienp/py-output-static-typevar
branch
from
af29ea5
to
f6baf41
Compare
Frassle
reviewed
May 10, 2024
julienp
force-pushed
the
julienp/py-output-static-typevar
branch
2 times, most recently
from
a66a8cf
to
7cddcf8
Compare
julienp
commented
May 10, 2024
julienp
force-pushed
the
julienp/py-output-static-typevar
branch
4 times, most recently
from
e5ee7ef
to
03a4afd
Compare
Frassle
reviewed
May 13, 2024
sdk/python/lib/pulumi/output.py
Outdated
| @@ -50,6 +50,7 @@ | |||
| T3 = TypeVar("T3") | |||
| T_co = TypeVar("T_co", covariant=True) | |||
| U = TypeVar("U") | |||
| U_co = TypeVar("U_co", covariant=True) | |||
There was a problem hiding this comment.
does covariance make sense on methods?
There was a problem hiding this comment.
I don't think so, at least for static methods.
https://typing.readthedocs.io/en/latest/spec/generics.html#variance
Variance is meaningful only when a type variable is bound to a generic class. If a type variable declared as covariant or contravariant is bound to a generic function or type alias, type checkers may warn users about this.
I'll update the PR to use plain U.
Pyright >= 1.1.354 assumes the typevar is Unknown if the generic class that holds the staticmethods has no default type parameter, however in our case the staticmethods have no link to the class's typevar.
julienp
force-pushed
the
julienp/py-output-static-typevar
branch
from
03a4afd
to
59ba669
Compare
julienp
commented
May 15, 2024
| @@ -437,22 +437,24 @@ def secret(val: Input[T]) -> "Output[T]": | |||
| # https://mypy.readthedocs.io/en/stable/more_types.html#type-checking-the-variants:~:text=considered%20unsafely%20overlapping | |||
| @overload | |||
| @staticmethod | |||
| def all(*args: "Output[T_co]") -> "Output[List[T_co]]": ... # type: ignore | |||
| def all(*args: "Output[Any]") -> "Output[List[Any]]": ... # type: ignore | |||
There was a problem hiding this comment.
Previous types stated that all arguments have the same type, however you can pass mixed arguments. Because this returns a list type, and not a tuple, we can't provide overloads for specific types like we do in nodejs
Line 612 in dbb887b
There was a problem hiding this comment.
Yeh this is kinda sad but probably the most practical we can do for now
The previous types stated that all the arguments to `Output.all` should be of the same type, however you can pass through a list of mixed types. If `Output.all` returned a tuple isntead, we could provide overloads that returned a `Output[tuple[T1, T2, ...]]` with the precise types. With the return type being a list, we are limited to `List[Any]`.
julienp
force-pushed
the
julienp/py-output-static-typevar
branch
from
d1c653b
to
d0ce289
Compare
tgummerer
reviewed
May 22, 2024
| @@ -0,0 +1,4 @@ | |||
| changes: | |||
There was a problem hiding this comment.
Has this been added accidentally?
There was a problem hiding this comment.
No, there are two changes here.
The initial change changed all T_co uses on static methods to U.
The 2nd change changes removes the type variable for from all and uses Any.
Frassle
approved these changes
May 22, 2024
| @@ -437,22 +437,24 @@ def secret(val: Input[T]) -> "Output[T]": | |||
| # https://mypy.readthedocs.io/en/stable/more_types.html#type-checking-the-variants:~:text=considered%20unsafely%20overlapping | |||
| @overload | |||
| @staticmethod | |||
| def all(*args: "Output[T_co]") -> "Output[List[T_co]]": ... # type: ignore | |||
| def all(*args: "Output[Any]") -> "Output[List[Any]]": ... # type: ignore | |||
There was a problem hiding this comment.
Yeh this is kinda sad but probably the most practical we can do for now
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 24, 2024
Tentative changelog: ### Features - [engine] Guess ID references of dependant resources when generating code for import operations [#16208](#16208) ### Bug Fixes - [engine] Check property dependencies and deleted-with relationships for target dependents [#16220](#16220) - [engine] Propagate dependencies of untargeted resources correctly during targeted updates [#16247](#16247) - [backend/diy] Rewrite DeletedWith references when renaming stacks [#16216](#16216) - [sdk/python] Use a separate type variable for static methods on Output [#16172](#16172) - [sdk/python] Relax Output.all types to better match the implementation [#16172](#16172) - [sdkgen/python] Generate __init__.py files for modules that only contain enumerations [#16229](#16229)
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 24, 2024
To be merged after: - #16261 - pulumi/pulumi-docker-containers#195 Tentative changelog... ### Features - [engine] Guess ID references of dependant resources when generating code for import operations [#16208](#16208) ### Bug Fixes - [engine] Check property dependencies and deleted-with relationships for target dependents [#16220](#16220) - [engine] Propagate dependencies of untargeted resources correctly during targeted updates [#16247](#16247) - [backend/diy] Rewrite DeletedWith references when renaming stacks [#16216](#16216) - [cli/state] Fix state renames involving DeletedWith - [sdk/python] Use a separate type variable for static methods on Output [#16172](#16172) - [sdk/python] Relax Output.all types to better match the implementation [#16172](#16172) - [sdkgen/python] Generate __init__.py files for modules that only contain enumerations [#16229](#16229)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Pyright >= 1.1.354 assumes the typevar is Unknown if the generic class
that holds the staticmethods has no default type parameter, however in
our case the staticmethods have no link to the class's typevar.
Fixes #15914
Fixes #16194
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change