Skip to content
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

More principled approach for callable vs callable inference #15910

Merged
merged 6 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 97 additions & 31 deletions mypy/constraints.py
Original file line number Diff line number Diff line change
Expand Up @@ -595,15 +595,11 @@ def visit_parameters(self, template: Parameters) -> list[Constraint]:
return self.infer_against_any(template.arg_types, self.actual)
if type_state.infer_polymorphic and isinstance(self.actual, Parameters):
# For polymorphic inference we need to be able to infer secondary constraints
# in situations like [x: T] <: P <: [x: int].
res = []
if len(template.arg_types) == len(self.actual.arg_types):
for tt, at in zip(template.arg_types, self.actual.arg_types):
# This avoids bogus constraints like T <: P.args
if isinstance(at, ParamSpecType):
continue
res.extend(infer_constraints(tt, at, self.direction))
return res
# in situations like [x: T] <: P <: [x: int]. Note we invert direction, since
# this function expects direction between callables.
return infer_callable_arguments_constraints(
template, self.actual, neg_op(self.direction)
)
raise RuntimeError("Parameters cannot be constrained to")

# Non-leaf types
Expand Down Expand Up @@ -722,7 +718,8 @@ def visit_instance(self, template: Instance) -> list[Constraint]:
prefix = mapped_arg.prefix
if isinstance(instance_arg, Parameters):
# No such thing as variance for ParamSpecs, consider them invariant
# TODO: constraints between prefixes
# TODO: constraints between prefixes using
# infer_callable_arguments_constraints()
suffix: Type = instance_arg.copy_modified(
instance_arg.arg_types[len(prefix.arg_types) :],
instance_arg.arg_kinds[len(prefix.arg_kinds) :],
Expand Down Expand Up @@ -793,7 +790,8 @@ def visit_instance(self, template: Instance) -> list[Constraint]:
prefix = template_arg.prefix
if isinstance(mapped_arg, Parameters):
# No such thing as variance for ParamSpecs, consider them invariant
# TODO: constraints between prefixes
# TODO: constraints between prefixes using
# infer_callable_arguments_constraints()
suffix = mapped_arg.copy_modified(
mapped_arg.arg_types[len(prefix.arg_types) :],
mapped_arg.arg_kinds[len(prefix.arg_kinds) :],
Expand Down Expand Up @@ -962,24 +960,12 @@ def visit_callable_type(self, template: CallableType) -> list[Constraint]:
unpack_constraints = build_constraints_for_simple_unpack(
template_types, actual_types, neg_op(self.direction)
)
template_args = []
cactual_args = []
res.extend(unpack_constraints)
else:
template_args = template.arg_types
cactual_args = cactual.arg_types
# TODO: use some more principled "formal to actual" logic
# instead of this lock-step loop over argument types. This identical
# logic should be used in 5 places: in Parameters vs Parameters
# inference, in Instance vs Instance inference for prefixes (two
# branches), and in Callable vs Callable inference (two branches).
for t, a in zip(template_args, cactual_args):
# This avoids bogus constraints like T <: P.args
if isinstance(a, (ParamSpecType, UnpackType)):
# TODO: can we infer something useful for *T vs P?
continue
# Negate direction due to function argument type contravariance.
res.extend(infer_constraints(t, a, neg_op(self.direction)))
res.extend(
infer_callable_arguments_constraints(template, cactual, self.direction)
)
else:
prefix = param_spec.prefix
prefix_len = len(prefix.arg_types)
Expand Down Expand Up @@ -1028,11 +1014,9 @@ def visit_callable_type(self, template: CallableType) -> list[Constraint]:
arg_kinds=cactual.arg_kinds[:prefix_len],
arg_names=cactual.arg_names[:prefix_len],
)

for t, a in zip(prefix.arg_types, cactual_prefix.arg_types):
if isinstance(a, ParamSpecType):
continue
res.extend(infer_constraints(t, a, neg_op(self.direction)))
res.extend(
infer_callable_arguments_constraints(prefix, cactual_prefix, self.direction)
)

template_ret_type, cactual_ret_type = template.ret_type, cactual.ret_type
if template.type_guard is not None:
Expand Down Expand Up @@ -1435,3 +1419,85 @@ def build_constraints_for_unpack(
for template_arg, item in zip(template_unpack.items, mapped_middle):
res.extend(infer_constraints(template_arg, item, direction))
return res, mapped_prefix + mapped_suffix, template_prefix + template_suffix


def infer_directed_constraints(left: Type, right: Type, direction: int) -> list[Constraint]:
"""Infer constraints between two arguments using direction between original callables."""
if isinstance(left, (ParamSpecType, UnpackType)) or isinstance(
right, (ParamSpecType, UnpackType)
):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this extra logic is necessary normally: the only place these can pop up is *args and **kwargs. Err well, maybe not given current lax validation 😅.

Also I think the function name should make incorporate "argument" somehow...?

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW some tests failed without this, and we can never do anything useful anyway, so we can just always skip.

# This avoids bogus constraints like T <: P.args
# TODO: can we infer something useful for *T vs P?
return []
if direction == SUBTYPE_OF:
# We invert direction to account for argument contravariance.
return infer_constraints(left, right, neg_op(direction))
else:
return infer_constraints(right, left, neg_op(direction))


def infer_callable_arguments_constraints(
template: CallableType | Parameters, actual: CallableType | Parameters, direction: int
) -> list[Constraint]:
"""Infer constraints between argument types of two callables.

This function essentially extracts four steps from are_parameters_compatible() in
subtypes.py that involve subtype checks between argument types. We keep the argument
matching logic, but ignore various strictness flags present there, and checks that
do not involve subtyping. Then in place of every subtype check we put an infer_constrains()
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

check for the same types.
"""
res = []
if direction == SUBTYPE_OF:
left, right = template, actual
else:
left, right = actual, template
left_star = left.var_arg()
left_star2 = left.kw_arg()
right_star = right.var_arg()
right_star2 = right.kw_arg()

# Numbering of steps below matches the one in are_parameters_compatible() for convenience.
# Phase 1a: compare star vs star arguments.
if left_star is not None and right_star is not None:
res.extend(infer_directed_constraints(left_star.typ, right_star.typ, direction))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the TODO in infer_directed_constraints only applies here? (Unless TypeVarMapping or whatever gets added)

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably yes, but TBH I didn't think much about this.

if left_star2 is not None and right_star2 is not None:
res.extend(infer_directed_constraints(left_star2.typ, right_star2.typ, direction))

# Phase 1b: compare left args with corresponding non-star right arguments.
for right_arg in right.formal_arguments():
left_arg = mypy.typeops.callable_corresponding_argument(left, right_arg)
if left_arg is None:
continue
res.extend(infer_directed_constraints(left_arg.typ, right_arg.typ, direction))

# Phase 1c: compare left args with right *args.
if right_star is not None:
right_by_position = right.try_synthesizing_arg_from_vararg(None)
assert right_by_position is not None
i = right_star.pos
assert i is not None
while i < len(left.arg_kinds) and left.arg_kinds[i].is_positional():
left_by_position = left.argument_by_position(i)
assert left_by_position is not None
res.extend(
infer_directed_constraints(left_by_position.typ, right_by_position.typ, direction)
)
i += 1

# Phase 1d: compare left args with right **kwargs.
if right_star2 is not None:
right_names = {name for name in right.arg_names if name is not None}
left_only_names = set()
for name, kind in zip(left.arg_names, left.arg_kinds):
if name is None or kind.is_star() or name in right_names:
continue
left_only_names.add(name)

right_by_name = right.try_synthesizing_arg_from_kwarg(None)
assert right_by_name is not None
for name in left_only_names:
left_by_name = left.argument_by_name(name)
assert left_by_name is not None
res.extend(infer_directed_constraints(left_by_name.typ, right_by_name.typ, direction))
return res
26 changes: 8 additions & 18 deletions mypy/subtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,7 @@ def check_mixed(
):
nominal = False
else:
# TODO: everywhere else ParamSpecs are handled as invariant.
if not check_type_parameter(
lefta, righta, COVARIANT, self.proper_subtype, self.subtype_context
):
Expand Down Expand Up @@ -666,13 +667,12 @@ def visit_unpack_type(self, left: UnpackType) -> bool:
return False

def visit_parameters(self, left: Parameters) -> bool:
if isinstance(self.right, (Parameters, CallableType)):
right = self.right
if isinstance(right, CallableType):
right = right.with_unpacked_kwargs()
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't tell why you removed support for callable types here. I guess that might be handled elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should normally never compare Callable vs Parameters, and if we do they can never be subtype of each other, in the same sense as int is never a sub- (or super-) type of (int) -> int.

if isinstance(self.right, Parameters):
# TODO: direction here should be opposite, this function expects
# order of callables, while parameters are contravariant.
return are_parameters_compatible(
left,
right,
self.right,
is_compat=self._is_subtype,
ignore_pos_arg_names=self.subtype_context.ignore_pos_arg_names,
)
Expand Down Expand Up @@ -723,14 +723,6 @@ def visit_callable_type(self, left: CallableType) -> bool:
elif isinstance(right, TypeType):
# This is unsound, we don't check the __init__ signature.
return left.is_type_obj() and self._is_subtype(left.ret_type, right.item)
elif isinstance(right, Parameters):
# this doesn't check return types.... but is needed for is_equivalent
return are_parameters_compatible(
left.with_unpacked_kwargs(),
right,
is_compat=self._is_subtype,
ignore_pos_arg_names=self.subtype_context.ignore_pos_arg_names,
)
else:
return False

Expand Down Expand Up @@ -1456,7 +1448,6 @@ def g(x: int) -> int: ...
right,
is_compat=is_compat,
ignore_pos_arg_names=ignore_pos_arg_names,
check_args_covariantly=check_args_covariantly,
allow_partial_overlap=allow_partial_overlap,
strict_concatenate_check=strict_concatenate_check,
)
Expand All @@ -1480,7 +1471,6 @@ def are_parameters_compatible(
*,
is_compat: Callable[[Type, Type], bool],
ignore_pos_arg_names: bool = False,
check_args_covariantly: bool = False,
allow_partial_overlap: bool = False,
strict_concatenate_check: bool = False,
) -> bool:
Expand Down Expand Up @@ -1534,7 +1524,7 @@ def _incompatible(left_arg: FormalArgument | None, right_arg: FormalArgument | N

# Phase 1b: Check non-star args: for every arg right can accept, left must
# also accept. The only exception is if we are allowing partial
# partial overlaps: in that case, we ignore optional args on the right.
# overlaps: in that case, we ignore optional args on the right.
for right_arg in right.formal_arguments():
left_arg = mypy.typeops.callable_corresponding_argument(left, right_arg)
if left_arg is None:
Expand All @@ -1548,7 +1538,7 @@ def _incompatible(left_arg: FormalArgument | None, right_arg: FormalArgument | N

# Phase 1c: Check var args. Right has an infinite series of optional positional
# arguments. Get all further positional args of left, and make sure
# they're more general then the corresponding member in right.
# they're more general than the corresponding member in right.
if right_star is not None:
# Synthesize an anonymous formal argument for the right
right_by_position = right.try_synthesizing_arg_from_vararg(None)
Expand All @@ -1575,7 +1565,7 @@ def _incompatible(left_arg: FormalArgument | None, right_arg: FormalArgument | N

# Phase 1d: Check kw args. Right has an infinite series of optional named
# arguments. Get all further named args of left, and make sure
# they're more general then the corresponding member in right.
# they're more general than the corresponding member in right.
if right_star2 is not None:
right_names = {name for name in right.arg_names if name is not None}
left_only_names = set()
Expand Down
5 changes: 1 addition & 4 deletions mypy/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1545,9 +1545,6 @@ class FormalArgument(NamedTuple):
required: bool


# TODO: should this take bound typevars too? what would this take?
# ex: class Z(Generic[P, T]): ...; Z[[V], V]
# What does a typevar even mean in this context?
class Parameters(ProperType):
"""Type that represents the parameters to a function.

Expand Down Expand Up @@ -1602,7 +1599,7 @@ def copy_modified(
variables=variables if variables is not _dummy else self.variables,
)

# the following are copied from CallableType. Is there a way to decrease code duplication?
# TODO: here is a lot of code duplication with Callable type, fix this.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I should write this historical note here in case it helps or something: I originally wanted to make every Callable have a Parameters attribute, meaning it doesn't hold its own args anymore. That seemed like too much work though! ... maybe these can inherit instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are three possible ways to fix this:

  • Delete Parameters and use CallableType instead. (But I don't like this way, because it would be a hack.)
  • Make arg_types etc. properties on CallableType that forward values from its parameters attribute. This way I guess it will be much less work, but may have visible performance impact.
  • Indeed, both can inherit these methods from say ArgumentHelpers mixin.

Ideally I think we should try second option, measure performance, if it is OK, go with it, if it is bad, try option 3.

def var_arg(self) -> FormalArgument | None:
"""The formal argument for *args."""
for position, (type, kind) in enumerate(zip(self.arg_types, self.arg_kinds)):
Expand Down
69 changes: 69 additions & 0 deletions test-data/unit/check-inference.test
Original file line number Diff line number Diff line change
Expand Up @@ -3553,3 +3553,72 @@ class E(D): ...

reveal_type([E(), D()]) # N: Revealed type is "builtins.list[__main__.D]"
reveal_type([D(), E()]) # N: Revealed type is "builtins.list[__main__.D]"

[case testCallableInferenceAgainstCallablePosVsStar]
from typing import TypeVar, Callable, Tuple

T = TypeVar('T')
S = TypeVar('S')

def f(x: Callable[[T, S], None]) -> Tuple[T, S]: ...
def g(*x: int) -> None: ...
reveal_type(f(g)) # N: Revealed type is "Tuple[builtins.int, builtins.int]"
[builtins fixtures/list.pyi]

[case testCallableInferenceAgainstCallableStarVsPos]
from typing import TypeVar, Callable, Tuple, Protocol

T = TypeVar('T', contravariant=True)
S = TypeVar('S', contravariant=True)

class Call(Protocol[T, S]):
def __call__(self, __x: T, *args: S) -> None: ...

def f(x: Call[T, S]) -> Tuple[T, S]: ...
def g(*x: int) -> None: ...
reveal_type(f(g)) # N: Revealed type is "Tuple[builtins.int, builtins.int]"
[builtins fixtures/list.pyi]

[case testCallableInferenceAgainstCallableNamedVsStar]
from typing import TypeVar, Callable, Tuple, Protocol

T = TypeVar('T', contravariant=True)
S = TypeVar('S', contravariant=True)

class Call(Protocol[T, S]):
def __call__(self, *, x: T, y: S) -> None: ...

def f(x: Call[T, S]) -> Tuple[T, S]: ...
def g(**kwargs: int) -> None: ...
reveal_type(f(g)) # N: Revealed type is "Tuple[builtins.int, builtins.int]"
[builtins fixtures/list.pyi]

[case testCallableInferenceAgainstCallableStarVsNamed]
from typing import TypeVar, Callable, Tuple, Protocol

T = TypeVar('T', contravariant=True)
S = TypeVar('S', contravariant=True)

class Call(Protocol[T, S]):
def __call__(self, *, x: T, **kwargs: S) -> None: ...

def f(x: Call[T, S]) -> Tuple[T, S]: ...
def g(**kwargs: int) -> None: pass
reveal_type(f(g)) # N: Revealed type is "Tuple[builtins.int, builtins.int]"
[builtins fixtures/list.pyi]

[case testCallableInferenceAgainstCallableNamedVsNamed]
from typing import TypeVar, Callable, Tuple, Protocol

T = TypeVar('T', contravariant=True)
S = TypeVar('S', contravariant=True)

class Call(Protocol[T, S]):
def __call__(self, *, x: T, y: S) -> None: ...

def f(x: Call[T, S]) -> Tuple[T, S]: ...

# Note: order of names is different w.r.t. protocol
def g(*, y: int, x: str) -> None: pass
reveal_type(f(g)) # N: Revealed type is "Tuple[builtins.str, builtins.int]"
[builtins fixtures/list.pyi]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also test a few cases where we shouldn't be inferring constraints, e.g. positional-only vs keyword-only?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, added such tests (note btw if there are no constraints, it now means they can never by subtypes, so there will be also an error in each test).