Skip to content

Implement basic subtyping & inferrence for variadic classes. #13105

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

Merged
merged 2 commits into from
Jul 20, 2022

Conversation

jhance
Copy link
Collaborator

@jhance jhance commented Jul 11, 2022

This makes several basic testcases for using classes with variadic generics pass. Some pieces of this are left as TODOs to flesh out various edge cases to avoid the diff growing in complexity.

@@ -702,7 +753,7 @@ def visit_tuple_type(self, template: TupleType) -> List[Constraint]:
isinstance(actual, Instance)
and actual.type.fullname == "builtins.tuple"
)
unpack_index = find_unpack_in_tuple(template)
unpack_index = find_unpack_in_list(template.items)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to migrate this to use the other helpers, but couldn't figure out how to cleanly do it.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Ts = TypeVarTuple("Ts")
T = TypeVar("T")

class Variadic(Generic[T, Ts]):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails if I add a suffix type variable, with a weird error Argument 1 to "foo" has incompatible type "Variadic[int, str, bool, float]"; expected "Variadic[int, str, bool, float]" (diff)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment below about what seems to be going on.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a full review, but I think that I've figured out why suffix doesn't work.

mypy/subtypes.py Outdated
assert unpack_index is not None
type_params = zip(
left_prefix + right_prefix,
left_suffix + right_suffix,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not correct. It should be left_prefix + left_suffix and right_prefix + right_suffix, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya that does seem like a bad problem.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I only have a few nits.

template_unpack.type.fullname == "builtins.tuple"
):
# TODO: check homogenous tuple case
assert NotImplementedError
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a TODO about fixed-length tuple here?

)
modified_actual = actual.copy_modified(
items=list(actual_items)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also infer constraints for the prefix and suffix? I guess they may contain regular type variable types?

@@ -26,8 +27,27 @@ def expand_type_by_instance(typ: Type, instance: Instance) -> Type:
return typ
else:
variables: Dict[TypeVarId, Type] = {}
for binder, arg in zip(instance.type.defn.type_vars, instance.args):
if instance.type.has_type_var_tuple_type:
import mypy.constraints
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this import unused?


from mypy.types import Instance, UnpackType, ProperType, get_proper_type, Type

"""Helpers for interacting with type var tuples."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: move docstring to top of file.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

dragonchain (https://github.com/dragonchain/dragonchain)
- dragonchain/lib/database/redis.py:399:36: error: Argument 2 to "zadd" of "SortedSetCommands" has incompatible type "Dict[str, int]"; expected "Mapping[Union[str, bytes], Union[bytes, float, int, str]]"
+ dragonchain/lib/database/redis.py:399:36: error: Argument 2 to "zadd" of "SortedSetCommands" has incompatible type "Dict[str, int]"; expected "Mapping[Union[str, bytes], Union[bytes, float, str]]"
- dragonchain/lib/database/redis_utest.py:218:9: error: "Callable[[Union[str, bytes], Mapping[Union[str, bytes], Union[bytes, float, int, str]], bool, bool, bool, bool, Optional[Any], Optional[Any]], int]" has no attribute "assert_called_once_with"
+ dragonchain/lib/database/redis_utest.py:218:9: error: "Callable[[Union[str, bytes], Mapping[Union[str, bytes], Union[bytes, float, str]], bool, bool, bool, bool, Optional[Any], Optional[Any]], int]" has no attribute "assert_called_once_with"

@JukkaL JukkaL merged commit 15e4de5 into python:master Jul 20, 2022
Comment on lines +347 to +351
left_items = t.args[:right.type.type_var_tuple_prefix]
right_items = right.args[:right.type.type_var_tuple_prefix]
if right.type.type_var_tuple_suffix:
left_items += t.args[-right.type.type_var_tuple_suffix:]
right_items += right.args[-right.type.type_var_tuple_suffix:]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhance maybe I'm just totally missing something, but I think these aren't actually used anywhere? The slicing means we're not mutating the original t.args and right.args

@tyralla tyralla mentioned this pull request Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants