-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
Changes from 4 commits
0f8c36c
23b3ffb
2d141f1
d58f194
ed52034
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,7 +234,10 @@ def visit_type_var(self, t: TypeVarType) -> Type: | |
| return repl | ||
|
|
||
| def visit_param_spec(self, t: ParamSpecType) -> Type: | ||
| repl = get_proper_type(self.variables.get(t.id, t)) | ||
| # 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. | ||
|
|
@@ -357,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 | ||
|
|
@@ -370,6 +373,23 @@ 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: it seems this only has to be done *sometimes*. Conceptually this should only | ||
| # be done once; we should update that "once" location rather than here. | ||
| # (see testAlreadyExpandedCallableWithParamSpecReplacement) | ||
| elif isinstance(repl, ParamSpecType) and len(t.arg_types) == 2: | ||
| # 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
|
|
||
| t = t.copy_modified( | ||
| arg_types=prefix.arg_types + old_prefix.arg_types + t.arg_types, | ||
| arg_kinds=prefix.arg_kinds + old_prefix.arg_kinds + t.arg_kinds, | ||
| arg_names=prefix.arg_names + old_prefix.arg_names + t.arg_names, | ||
| ) | ||
|
|
||
| var_arg = t.var_arg() | ||
| if var_arg is not None and isinstance(var_arg.typ, UnpackType): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.