GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44796: Unify TypeVar and ParamSpec substitution by serhiy-storchaka · Pull Request #31143 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Feb 5, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Add methods typing_subst() in TypeVar and ParamSpec.

This is an alternative to #27511.

https://bugs.python.org/issue44796

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
bpo-44796: Unify TypeVar and ParamSpec substitution
a03c0d1 
Add methods __typing_subst__() in TypeVar and ParamSpec.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 5, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Feb 5, 2022
@JelleZijlstra
JelleZijlstra self-requested a review February 5, 2022 16:44
JelleZijlstra
JelleZijlstra reviewed Feb 6, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/_collections_abc.py
f"ParamSpec, or Concatenate. Got {t_args}")
return super().__new__(cls, origin, args)

@property

@JelleZijlstra JelleZijlstra Feb 6, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Feb 6, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/typing.py Outdated
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)):

@JelleZijlstra JelleZijlstra Feb 6, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@GBeauregard GBeauregard Feb 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Feb 6, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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
                "]"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@GBeauregard GBeauregard Feb 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Mar 10, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
GBeauregard reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
serhiy-storchaka added 2 commits February 6, 2022 22:01
@serhiy-storchaka
Merge branch 'main' into typevar-subst2
af9a60c
@serhiy-storchaka
Merge branch 'main' into typevar-subst2
e4345da
serhiy-storchaka
serhiy-storchaka commented Mar 10, 2022
View reviewed changes
Comment thread Lib/typing.py
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]

@serhiy-storchaka serhiy-storchaka Mar 10, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
serhiy-storchaka marked this pull request as ready for review March 10, 2022 10:34
@serhiy-storchaka
serhiy-storchaka requested review from Fidget-Spinner, gvanrossum and rhettinger as code owners March 10, 2022 10:34
@serhiy-storchaka

serhiy-storchaka commented Mar 10, 2022

Copy link
Copy Markdown
Member Author

It now includes PEP 646 changes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Merge branch 'main' into typevar-subst2
845676d
@serhiy-storchaka serhiy-storchaka added the skip news label Mar 11, 2022
@serhiy-storchaka
serhiy-storchaka merged commit b6a5d85 into python:main Mar 11, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Mar 11, 2022
@serhiy-storchaka
serhiy-storchaka deleted the typevar-subst2 branch March 11, 2022 08:47
@posita posita mentioned this pull request Mar 11, 2022
[3.10] bpo-46581: Propagate private vars via _GenericAlias.copy_with (GH-31061) #31821
Merged
@gvanrossum

gvanrossum commented Mar 12, 2022

Copy link
Copy Markdown
Member

Did anyone review this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Mar 17, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hrnciar hrnciar mentioned this pull request May 17, 2022
BUG: Python 3.11.0b1, test_generic_alias, TypeError: 'property' object is not iterable numpy/numpy#21526
Closed
@Eclips4 Eclips4 mentioned this pull request Oct 30, 2022
Bug or new behavior of generics parameterization? #98852
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
+1 more reviewer
@GBeauregard GBeauregard GBeauregard left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@serhiy-storchaka @gvanrossum @JelleZijlstra @GBeauregard @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.