8000 Don't duplicate ParamSpec prefixes and properly substitute Paramspecs by A5rocks · Pull Request #14677 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

Don't duplicate ParamSpec prefixes and properly substitute Paramspecs #14677

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.

Sign up for GitHub

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 5 commits into from
Mar 15, 2023
Merged

Don't duplicate ParamSpec prefixes and properly substitute Paramspecs #14677

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f8c36c
Don't duplicate ParamSpec prefixes and check Concatenate subtyping be…
A5rocks Feb 2, 2023
23b3ffb
Try a more minimal approach
A5rocks Feb 11, 2023
2d141f1
Fix crash in discord.py
A5rocks Feb 12, 2023
d58f194
Include errors on the regression test
A5rocks Feb 12, 2023
ed52034
PR feedback
A5rocks Feb 20, 2023
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
Prev Previous commit
Next Next commit
Try a more minimal approach
  • Loading branch information
@A5rocks
A5rocks committed Feb 11, 2023
commit 23b3ffbb7528e42b9b58f3b50bf9f75fc4740d98
33 changes: 23 additions & 10 deletions mypy/expandtype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,15 +234,10 @@ def visit_type_var(self, t: TypeVarType) -> Type:
return repl

def visit_param_spec(self, t: ParamSpecType) -> Type:
repl_ = self.variables.get(t.id)
if not repl_:
# in this case, we are trying to expand it into... nothing?
# if we continue, we will get e.g.:
# repl = t = [int, str, **P]
# at which point, expand_param_spec will duplicate the arguments.
return t

repl = get_proper_type(repl_)
# set prefix to something empty so we don't duplicate it
repl = get_proper_type(
self.variables.get(t.id, t.copy_modified(prefix=Parameters([], [], [])))
)
if isinstance(repl, Instance):
# TODO: what does prefix mean in this case?
# TODO: why does this case even happen? Instances aren't plural.
Expand Down Expand Up @@ -365,7 +360,7 @@ def visit_callable_type(self, t: CallableType) -> Type:
# must expand both of them with all the argument types,
# kinds and names in the replacement. The return type in
# the replacement is ignored.
if isinstance(repl, CallableType) or isinstance(repl, Parameters):
if isinstance(repl, (CallableType, Parameters)):
# Substitute *args: P.args, **kwargs: P.kwargs
prefix = param_spec.prefix
# we need to expand the types in the prefix, so might as well
Expand All @@ -378,6 +373,24 @@ def visit_callable_type(self, t: CallableType) -> Type:
ret_type=t.ret_type.accept(self),
type_guard=(t.type_guard.accept(self) if t.type_guard is not None else None),
)
# TODO: fix testConstraintsBetweenConcatenatePrefixes
# (it fails without `and repl != param_spec`)
elif isinstance(repl, ParamSpecType) and repl != param_spec:
# We're substituting one paramspec for another; this can mean that the prefix
# changes. (e.g. sub Concatenate[int, P] for Q)
prefix = repl.prefix
old_prefix = param_spec.prefix

# Check assumptions. I'm not sure what order to switch these:
assert not old_prefix.arg_types or not prefix.arg_types
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks a bit suspicious to me. Why can't both the arg types truthy?

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 haven't found a case where they were both truthy and yet not equal to each other yet -- I don't know what order to mix their arguments together. My gut says what I wrote next is correct but I would rather have an explicit test case (or be shown that this never happens)

# ... and I don't know where to put non-paramspec t.arg_types
assert len(t.arg_types) == 2 or t.arg_types[:-2] == old_prefix.arg_types

t = t.copy_modified(
arg_types=prefix.arg_types + old_prefix.arg_types + t.arg_types[-2:],
arg_kinds=prefix.arg_kinds + old_prefix.arg_kinds + t.arg_kinds[-2:],
arg_names=prefix.arg_names + old_prefix.arg_names + t.arg_names[-2:],
)

var_arg = t.var_arg()
if var_arg is not None and isinstance(var_arg.typ, UnpackType):
Expand Down
10 changes: 4 additions & 6 deletions mypy/messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2399,15 +2399,13 @@ def format_literal_value(typ: LiteralType) -> str:
return_type = format(func.ret_type)
if func.is_ellipsis_args:
return f"Callable[..., {return_type}]"

param_spec = func.param_spec()
if param_spec is not None:
return f"Callable[{format(param_spec)}, {return_type}]"
else:
args = format_callable_args(
func.arg_types, func.arg_kinds, func.arg_names, format, verbosity
)
return f"Callable[[{args}], {return_type}]"
args = format_callable_args(
func.arg_types, func.arg_kinds, func.arg_names, format, verbosity
)
return f"Callable[[{args}], {return_type}]"
else:
# Use a simple representation for function types; proper
# function types may result in long and difficult-to-read
Expand Down
46 changes: 0 additions & 46 deletions mypy/subtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,9 +642,6 @@ def visit_param_spec(self, left: ParamSpecType) -> bool:
isinstance(right, ParamSpecType)
and right.id == left.id
and right.flavor == left.flavor
and is_subtype(
left.as_parameters(), right.as_parameters(), subtype_context=self.subtype_context
)
):
return True
if isinstance(right, Parameters) and are_trivial_parameters(right):
Expand All @@ -667,22 +664,11 @@ def visit_parameters(self, left: Parameters) -> bool:
right = self.right
if isinstance(right, CallableType):
right = right.with_unpacked_kwargs()
# TODO: This heuristic sucks. We need it to make sure we normally do the strict thing.
# I wish we could just always do the strict thing....
# This takes into advantage that these Parameters probably just came from ParamSpecType#to_parameters()
anyt = AnyType(TypeOfAny.implementation_artifact)
concat_heuristic = all(
t.arg_kinds[-2:] == [ARG_STAR, ARG_STAR2] and t.arg_names[-2:] == [None, None] and t.arg_types[-2:] == [anyt, anyt] for t in (left, right)
)
return are_parameters_compatible(
left,
right,
is_compat=self._is_subtype,
subtype_context=self.subtype_context,
ignore_pos_arg_names=self.subtype_context.ignore_pos_arg_names,
strict_concatenate_check=self.options.strict_concatenate
if self.options and concat_heuristic
else True,
)
else:
return False
Expand All @@ -701,7 +687,6 @@ def visit_callable_type(self, left: CallableType) -> bool:
left,
right,
is_compat=self._is_subtype,
subtype_context=self.subtype_context,
ignore_pos_arg_names=self.subtype_context.ignore_pos_arg_names,
strict_concatenate=self.options.strict_concatenate if self.options else True,
)
Expand Down Expand Up @@ -736,7 +721,6 @@ def visit_callable_type(self, left: CallableType) -> bool:
left.with_unpacked_kwargs( 10000 ),
right,
is_compat=self._is_subtype,
subtype_context=self.subtype_context,
ignore_pos_arg_names=self.subtype_context.ignore_pos_arg_names,
)
else:
Expand Down Expand Up @@ -872,7 +856,6 @@ def visit_overloaded(self, left: Overloaded) -> bool:
left_item,
right_item,
is_compat=self._is_subtype,
subtype_context=self.subtype_context,
ignore_return=True,
ignore_pos_arg_names=self.subtype_context.ignore_pos_arg_names,
strict_concatenate=strict_concat,
Expand All @@ -881,7 +864,6 @@ def visit_overloaded(self, left: Overloaded) -> bool:
right_item,
left_item,
is_compat=self._is_subtype,
subtype_context=self.subtype_context,
ignore_return=True,
ignore_pos_arg_names=self.subtype_context.ignore_pos_arg_names,
strict_concatenate=strict_concat,
Expand Down Expand Up @@ -1316,8 +1298,6 @@ def is_callable_compatible(
right: CallableType,
*,
is_compat: Callable[[Type, Type], bool],
# TODO: should this be used more (instead of `ignore_pos_arg_names`, for instance)?
subtype_context: SubtypeContext | None = None,
is_compat_return: Callable[[Type, Type], bool] | None = None,
ignore_return: bool = False,
ignore_pos_arg_names: bool = False,
Expand Down Expand Up @@ -1472,7 +1452,6 @@ def g(x: int) -> int: ...
left,
right,
is_compat=is_compat,
subtype_context=subtype_context,
ignore_pos_arg_names=ignore_pos_arg_names,
check_args_covariantly=check_args_covariantly,
allow_partial_overlap=allow_partial_overlap,
Expand All @@ -1492,23 +1471,11 @@ def are_trivial_parameters(param: Parameters | NormalizedCallableType) -> bool:
)


def are_seperated_paramspecs(param: Parameters | NormalizedCallableType) -> bool:
# TODO: based on my tests, param.arg_types will always be of length 2. This is a potential
# optimization, however Callable#param_spec() does not assume this. If this is done, update that too.
return (
len(param.arg_types) >= 2
and isinstance(param.arg_types[-2], ParamSpecType)
and isinstance(param.arg_types[-1], ParamSpecType)
)


def are_parameters_compatible(
left: Parameters | NormalizedCallableType,
right: Parameters | NormalizedCallableType,
*,
is_compat: Callable[[Type, Type], bool],
# TODO: should this be used more (instead of `ignore_pos_arg_names`, for instance)?
subtype_context: SubtypeContext | None = None,
ignore_pos_arg_names: bool = False,
check_args_covariantly: bool = False,
allow_partial_overlap: bool = False,
Expand All @@ -1527,19 +1494,6 @@ def are_parameters_compatible(
if are_trivial_parameters(right):
return True

# sometimes we need to compare Callable[Concatenate[prefix, P], ...] and
# (prefix, *args: P.args, **kwargs: P.kwargs) -> ...
if (isinstance(left, CallableType) and isinstance(right, CallableType)) and (
are_seperated_paramspecs(left) or are_seperated_paramspecs(right)
):
# note: this doesn't lose parameters. I think. Otherwise, it would be unsafe.
left_ps = left.param_spec()
right_ps = right.param_spec()
if not right_ps or not left_ps:
return False

return is_subtype(left_ps, right_ps, subtype_context=subtype_context)

# Match up corresponding arguments and check them for compatibility. In
# every pair (argL, argR) of corresponding arguments from L and R, argL must
# be "more general" than argR if L is to be a subtype of R.
Expand Down
11 changes: 0 additions & 11 deletions mypy/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,17 +715,6 @@ def name_with_suffix(self) -> str:
return f"{n}.kwargs"
return n

def as_parameters(self) -> Parameters:
return self.prefix.copy_modified(
arg_types=self.prefix.arg_types
+ [
AnyType(TypeOfAny.implementation_artifact),
AnyType(TypeOfAny.implementation_artifact),
],
arg_names=self.prefix.arg_names + [None, None],
arg_kinds=self.prefix.arg_kinds + [ARG_STAR, ARG_STAR2],
)

def __hash__(self) -> int:
return hash((self.id, self.flavor, self.prefix))

Expand Down
0