-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use a separate type variable for static methods on Output #16172
Conversation
af29ea5
to
f6baf41
Compare
a66a8cf
to
7cddcf8
Compare
e5ee7ef
to
03a4afd
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does covariance make sense on methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
03a4afd
to
59ba669
Compare
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
export function all<T1, T2, T3, T4, T5, T6, T7, T8>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]`.
d1c653b
to
d0ce289
Compare
@@ -0,0 +1,4 @@ | |||
changes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this been added accidentally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh this is kinda sad but probably the most practical we can do for now
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)
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)
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 tidy
to update any new dependenciesmake lint
to verify my code passes the lint checkgofumpt
make changelog
and committed thechangelog/pending/<file>
documenting my change