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

bpo-44796: Unify TypeVar and ParamSpec substitution #31143

Merged
merged 4 commits into from
Mar 11, 2022

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Feb 5, 2022

Add methods typing_subst() in TypeVar and ParamSpec.

This is an alternative to #27511.

https://bugs.python.org/issue44796

Add methods __typing_subst__() in TypeVar and ParamSpec.
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, this does make things simpler, but we need to be careful that we don't break too many external consumers of typing code. Also, there may be interactions with #31021, which adds another TypeVar variant.

Here's a few comments, but I may need to look at this more to understand it better.

args = (*t_args, t_result)
elif not _is_param_expr(t_args):
raise TypeError(f"Expected a list of types, an ellipsis, "
f"ParamSpec, or Concatenate. Got {t_args}")
return super().__new__(cls, origin, args)

@property
Copy link
Member

Choose a reason for hiding this comment

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

I think we have to keep __parameters__, it's de facto a public API by now.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is inherited from GenericAlias now.

Lib/typing.py Outdated
return arg
if isinstance(arg, _SpecialForm) or arg in (Generic, Protocol):
raise TypeError(f"Plain {arg} is not valid as type argument")
if isinstance(arg, (type, TypeVar, ForwardRef, types.UnionType, ParamSpec)):
if isinstance(arg, (type, TypeVar, ForwardRef, types.UnionType)):
Copy link
Member

Choose a reason for hiding this comment

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

Will this affect putting ParamSpec inside Annotated? Looks like we don't have tests for that yet.

Copy link
Contributor

@GBeauregard GBeauregard Feb 6, 2022

Choose a reason for hiding this comment

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

I looked into this.

This check is needed in order to allow instances of ParamSpec (i.e. P in P = ParamSpec("P")) to pass typing._type_check because callable(P) is False. This can show up for instance like Callable[Annotated[P, ""], T]. This line's change would regress the behavior to a runtime error.

n.b. this is moot if #31151 gets merged since this line is removed entirely

Copy link
Member Author

Choose a reason for hiding this comment

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

Callable[Annotated[P, ""], T] does not conform the PEP 612 specification.

callable ::= Callable "[" parameters_expression, type_expression "]"

parameters_expression ::=
  | "..."
  | "[" [ type_expression ("," type_expression)* ] "]"
  | parameter_specification_variable
  | concatenate "["
                   type_expression ("," type_expression)* ","
                   parameter_specification_variable
                "]"

Copy link
Contributor

@GBeauregard GBeauregard Feb 6, 2022

Choose a reason for hiding this comment

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

I'm not convinced it was the intent of the spec here to disallow Annotated, but you can also reach this code here:

from typing import Callable, TypeVar, ParamSpec, get_type_hints
T = TypeVar("T")
P = ParamSpec("P")
def add_logging(f: Callable["P", T]):
    pass
get_type_hints(add_logging)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I re-added ParamSpec. Now some errors will not be caught at runtime. We will return to this in future.

new_arg = new_arg_by_param[old_arg]
elif (TypeVarTuple in self._typevar_types
and _is_unpacked_typevartuple(old_arg)):
if _is_unpacked_typevartuple(old_arg):
original_typevartuple = old_arg.__parameters__[0]
new_arg = new_arg_by_param[original_typevartuple]
Copy link
Member Author

Choose a reason for hiding this comment

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

This code is not covered by tests.

It perhaps could be simplified too, but since it is not covered by tests I left it as is.

@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review March 10, 2022 10:34
@serhiy-storchaka
Copy link
Member Author

It now includes PEP 646 changes.

@gvanrossum
Copy link
Member

Did anyone review this?

@serhiy-storchaka
Copy link
Member Author

Sorry, it was not reviewed after the last changes. But I addressed previous review comments. Is there something wrong with this code?

I merged this PR after short delay because it allows the C code for TypeVarTuple substitution (#31828) to be of reasonable complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants