Downgrading Type Annotations #6
Conversation
|
The linter is failing, which is to be expected, as things don't currently typecheck. However, the test runner is working for 3.8/3.9, but failing for 3.10, specifically here, where we need to instantiate the generic |
There was a problem hiding this comment.
Thanks for taking a pass at this. Unfortunately many of the changes here are semantically incorrect. It seems like the motivation for those incorrect changes is some backward-incompatible changes to ParamSpec in Python 3.10 (?). I haven't run into this before, which is a little surprising. If there's no workaround maybe we can just drop CI linting support for Python <3.10 for now.
| @@ -67,7 +69,7 @@ def register( | |||
|
|
|||
|
|
|||
| def apply( | |||
| intp: Interpretation[S, T], op: Operation[P, S], *args: P.args, **kwargs: P.kwargs | |||
| intp: Interpretation[P, S], op: Operation[P, S], *args: P.args, **kwargs: P.kwargs | |||
There was a problem hiding this comment.
This change is not correct. The signature of apply says interpretation intp should associate a semantic function with return type T to the operation op with return type S.
| @@ -33,11 +33,11 @@ def times_plus_1(x: int, y: int) -> int: | |||
| return x * y + 1 | |||
|
|
|||
|
|
|||
| def defaults(*ops: Operation[..., int]) -> Interpretation[int, int]: | |||
| def defaults(*ops: Operation[int, int]) -> Interpretation[int, int]: | |||
There was a problem hiding this comment.
This change is not correct, some operations have more than one argument
| return {op: bind_result(value_or_result(op.default)) for op in ops} | ||
|
|
||
|
|
||
| def times_n_handler(n: int, *ops: Operation[..., int]) -> Interpretation[int, int]: | ||
| def times_n_handler(n: int, *ops: Operation[int, int]) -> Interpretation[int, int]: |
There was a problem hiding this comment.
This change is not correct, some operations have more than one argument
| kwargs: Mapping[str, "Term[T]" | T | "Variable[T]"] | ||
| op: Operation[P, T] | ||
| args: P.args | ||
| kwargs: P.kwargs |
There was a problem hiding this comment.
Unfortunately I don't think ParamSpecs can be used in this way, that's why the previous types were what they were.
| op: Operation[..., T] | ||
| args: Iterable["Term[T]" | T | "Variable[T]"] | ||
| kwargs: Mapping[str, "Term[T]" | T | "Variable[T]"] | ||
| op: Operation[P, T] |
There was a problem hiding this comment.
P is unbound here, this is not allowed
| def _op_times_n( | ||
| n: int, op: Operation[..., int], result: Optional[int], *args: int | ||
| n: int, op: Operation[int, int], result: Optional[int], *args: int |
There was a problem hiding this comment.
This change is not correct, some operations have multiple arguments
| @@ -34,15 +34,15 @@ def times_plus_1(x: int, y: int) -> int: | |||
| return x * y + 1 | |||
|
|
|||
|
|
|||
| def block(*ops: Operation[..., int]) -> Interpretation[int, int]: | |||
| def block(*ops: Operation[int, int]) -> Interpretation[int, int]: | |||
There was a problem hiding this comment.
This is not correct, some operations have multiple arguments
| return {op: bind_result(lambda v, *args, **kwargs: reflect(v)) for op in ops} | ||
|
|
||
|
|
||
| def defaults(*ops: Operation[..., int]) -> Interpretation[int, int]: | ||
| def defaults(*ops: Operation[int, int]) -> Interpretation[int, int]: |
There was a problem hiding this comment.
This is not correct, some operations have multiple arguments
| return {op: bind_result(value_or_result(op.default)) for op in ops} | ||
|
|
||
|
|
||
| def times_n_handler(n: int, *ops: Operation[..., int]) -> Interpretation[int, int]: | ||
| def times_n_handler(n: int, *ops: Operation[int, int]) -> Interpretation[int, int]: |
There was a problem hiding this comment.
This is not correct, some operations have multiple arguments
| @@ -67,7 +67,7 @@ def _wrapper(*args: P.args, **kwargs: P.kwargs) -> T: | |||
| return _wrapper | |||
|
|
|||
|
|
|||
| Prompt = Operation[[Optional[T]], T] | |||
| Prompt = Operation[Optional[T], T] | |||
There was a problem hiding this comment.
This is not correct. Prompt is an alias for a unary operation.
|
Hey Eli - thanks for reviewing this. Should we plan on dropping support for 3.8, at least for now? The current code doesn't seem to run at all on 3.8 and 3.9, so bumping the minimum version to 3.10 seems to make the most sense anyways. |
This downgrades the type annotations to ones which work on python 3.8. The changes are mainly turning
A | Btypes intotyping.Union[A, B]and removing ellipses from type parameters.