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-46581: Propagate private vars via _GenericAlias.copy_with #31061

Merged

Conversation

posita
Copy link
Contributor

@posita posita commented Feb 1, 2022

GH-26091 added the _typevar_types and _paramspec_tvars instance variables to _GenericAlias. However, they were not propagated consistently. This commit addresses the most prominent deficiency identified in bpo-46581 (namely their absence from _GenericAlias.copy_with), but there could be others.

https://bugs.python.org/issue46581

Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra self-assigned this Feb 19, 2022
@posita posita force-pushed the posita/0/fix-bpo-46581-generic-alias branch from 90d730e to ae11f6f Compare February 19, 2022 05:47
@posita
Copy link
Contributor Author

posita commented Feb 19, 2022

Ah! Crap. Sorry for the force-push that moved us backward. Let me rebase and push again…. Should be cleaned up now. Apologies for the distraction!

@posita posita force-pushed the posita/0/fix-bpo-46581-generic-alias branch from ae11f6f to 4efbc2c Compare February 19, 2022 05:55
@posita posita force-pushed the posita/0/fix-bpo-46581-generic-alias branch from 4efbc2c to 7b9a6ff Compare February 26, 2022 19:34
Lib/typing.py Show resolved Hide resolved
posita added a commit to posita/cpython that referenced this pull request Mar 4, 2022
@posita posita force-pushed the posita/0/fix-bpo-46581-generic-alias branch from 7b9a6ff to 9510c95 Compare March 4, 2022 15:05
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 5, 2022
)

(cherry picked from commit 2031149)

Co-authored-by: Matt Bogosian <eb3f73+github+com@yaymail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 5, 2022
)

(cherry picked from commit 2031149)

Co-authored-by: Matt Bogosian <eb3f73+github+com@yaymail.com>
miss-islington added a commit that referenced this pull request Mar 5, 2022
(cherry picked from commit 2031149)

Co-authored-by: Matt Bogosian <eb3f73+github+com@yaymail.com>
miss-islington added a commit that referenced this pull request Mar 5, 2022
(cherry picked from commit 2031149)

Co-authored-by: Matt Bogosian <eb3f73+github+com@yaymail.com>
@posita posita force-pushed the posita/0/fix-bpo-46581-generic-alias branch from ba8b094 to c9def58 Compare March 6, 2022 14:10
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

There is more straightforward way to fix a conflict.

  1. Add _typevar_types=(TypeVar, ParamSpec), _paramspec_tvars=True to the call of _ConcatenateGenericAlias in _Concatenate.
  2. Remove _ConcatenateGenericAlias.__initt__.
  3. Remove a special case from _GenericAlias.copy_with.

Also please move changes in comments and docstrings to enother PR. They distract.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@posita posita force-pushed the posita/0/fix-bpo-46581-generic-alias branch from 72b9e76 to 6bf1f6c Compare March 10, 2022 00:10
@posita posita force-pushed the posita/0/fix-bpo-46581-generic-alias branch from 6bf1f6c to 6db57f5 Compare March 10, 2022 00:11
original = Callable[P, int]
self.assertEqual(original.__parameters__, (P,))
copied = original[P]
self.assertEqual(original.__parameters__, copied.__parameters__)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some more complicated test cases? Ideas:

  • The original has a Concatenate[int, P] in it (original = Callable[Concatenate[int, P], int]; original[P].__paramaters__ == (P,))
  • Instead of substituting a P in, substitute in a Concatenate or something like [int, T], and then the T should be in __parameters__.

P2 = ParamSpec('P2')
C1 = Callable[P, int]
self.assertEqual(C1.__parameters__, (P,))
self.assertEqual(C1[P2].__parameters__, (P2,))
Copy link
Member

Choose a reason for hiding this comment

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

Only this test was failed.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Thank you for pulling through @posita . This was a pretty tricky PR.

@serhiy-storchaka serhiy-storchaka merged commit 32bf359 into python:main Mar 10, 2022
@miss-islington
Copy link
Contributor

Thanks @posita for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @posita and @serhiy-storchaka, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 32bf3597922ac3f613989582afa2bff43bea8a2f 3.10

@posita
Copy link
Contributor Author

posita commented Mar 10, 2022

Sorry, @posita and @serhiy-storchaka, I could not cleanly backport this to 3.10 due to a conflict. Please backport using cherry_picker on command line. cherry_picker 32bf3597922ac3f613989582afa2bff43bea8a2f 3.10

Should I work on resolving this?

@JelleZijlstra
Copy link
Member

Yes please :) Let me know if you need any help.

@posita
Copy link
Contributor Author

posita commented Mar 11, 2022

I would love a nudge on where to learn how to do this manually. (I'm unfamiliar with the process.) Is it as simple as resolving any conflicts against the 3.10 branch and then submitting a PR or is there more involved?

@Fidget-Spinner
Copy link
Member

I would love a nudge on where to learn how to do this manually. (I'm unfamiliar with the process.) Is it as simple as resolving any conflicts against the 3.10 branch and then submitting a PR or is there more involved?

Yup. Submit a PR against the 3.10 and/or 3.9 branch after resolving conflicts.

The bot is a little off. An easier way without installing things is to use the git cherry-pick command directly. StackOverflow link here does a great job explaining it.

The other even easier alternative if you're using GitHub desktop, is to drag and drop the commit from the left panel from main into your new branch that is based off 3.10/3.9. The app will then warn you about conflicts, and you can resolve them in a code editor (this means accepting/rejecting changes or overwriting them altogether manually).

@posita posita deleted the posita/0/fix-bpo-46581-generic-alias branch March 11, 2022 13:53
posita added a commit to posita/cpython that referenced this pull request Mar 11, 2022
…ythonGH-31061)

(Cherry-picked from 32bf359.)

pythonGH-26091 added the _typevar_types and _paramspec_tvars instance
variables to _GenericAlias. However, they were not propagated
consistently. This commit addresses the most prominent deficiency
identified in bpo-46581 (namely their absence from
_GenericAlias.copy_with), but there could be others.

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot
Copy link

GH-31821 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Mar 11, 2022
@posita
Copy link
Contributor Author

posita commented Mar 11, 2022

I would love a nudge on where to learn how to do this manually. (I'm unfamiliar with the process.) Is it as simple as resolving any conflicts against the 3.10 branch and then submitting a PR or is there more involved?

Yup. Submit a PR against the 3.10 and/or 3.9 branch after resolving conflicts.

Thanks @Fidget-Spinner! Cherry-picking is all good. I just didn't know if there was other repo- or CI-specific magical incantations I needed to invoke or procedures I needed to follow.

Question (maybe for @JelleZijlstra): This is marked as needed a back-port to 3.10, but not to 3.9. Should this also be back-ported to 3.9?

serhiy-storchaka added a commit that referenced this pull request Mar 11, 2022
…H-31061) (GH-31821)

(Cherry-picked from 32bf359.)

GH-26091 added the _typevar_types and _paramspec_tvars instance
variables to _GenericAlias. However, they were not propagated
consistently. This commit addresses the most prominent deficiency
identified in bpo-46581 (namely their absence from
_GenericAlias.copy_with), but there could be others.

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@Fidget-Spinner
Copy link
Member

@posita re backporting to 3.9, we don't need it as pre-3.10 ParamSpec didn't exist so we never needed these parameters then.

Anyways, congrats on becoming a CPython contributor!

@posita
Copy link
Contributor Author

posita commented Mar 11, 2022

@posita re backporting to 3.9, we don't need it as pre-3.10 ParamSpec didn't exist so we never needed these parameters then.

Oh, right! Duh. Thanks all! I very much appreciate all the hand-holding.

@serhiy-storchaka serhiy-storchaka removed their assignment Apr 25, 2022
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
)

(cherry picked from commit 2031149)

Co-authored-by: Matt Bogosian <eb3f73+github+com@yaymail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants