GeezerVison

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

GeezerVison is showing github.com — bpo-43224: Implement substitution of unpacked TypeVarTuple in C by serhiy-storchaka · Pull Request #31828 · python/cpython

Conversation

serhiy-storchaka commented Mar 11, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

https://bugs.python.org/issue43224

Sorry, something went wrong.

Uh oh!

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

All reactions


          bpo-43224: Implement substitution of unpacked TypeVarTuple in C

048cd5a

serhiy-storchaka added the type-feature A feature request or enhancement label

Mar 11, 2022

serhiy-storchaka requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners

March 11, 2022 19:48

bedevere-bot added the awaiting core review label

Mar 11, 2022

the-knights-who-say-ni added the CLA signed label

Mar 11, 2022

JelleZijlstra commented Mar 11, 2022

Copy link

Copy Markdown

Member

Thanks for working on this! Please don't merge this without approval from the PEP 646 authors or the typing experts; I'm not confident we'll get all the edge cases right. I'd have liked a chance to review #31800 in more detail too.

All reactions

Sorry, something went wrong.

Uh oh!

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

JelleZijlstra requested changes

Mar 12, 2022

View reviewed changes

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.

I found a crash; see comment.

In general I'm worried that this approach will be too difficult to get right in all cases. I'd prefer the approach in #31804, which I suggested in #31021 (review).

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/genericaliasobject.c Outdated

		if (varparam < nparams) {
		if (nitems < nparams - 1) {
		return PyErr_Format(PyExc_TypeError,
		"!Too few arguments for %R",

JelleZijlstra Mar 12, 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.

Suggested change

	"!Too few arguments for %R",
	"Too few arguments for %R",

Or does ! do something here?

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Mar 12, 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 was temporary, for debugging.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_typing.py

		class G(Generic[Unpack[Ts]]): pass

		for A in G, Tuple:
		for A in G, Tuple, tuple:

JelleZijlstra Mar 12, 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.

There need to be more tests for cases where we substitute an unpacked tuple into the TypeVarTuple. I found a crash on your branch:

 % ./python.exe 
Python 3.11.0a6+ (heads/serhiy-storchaka-typevartuple-subst-c:a35af462a3, Mar 11 2022, 19:02:45) [Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information
>>> from typing import Unpack, TypeVarTuple
>>> Ts = TypeVarTuple("Ts")
>>> t = tuple[Ts]
>>> t[int, Unpack[tuple[int, str]], str]
Assertion failed: (iparam != varparam), function _Py_subs_parameters, file genericaliasobject.c, line 382.
zsh: abort      ./python.exe

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Mar 12, 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.

Good catch. Is not tuple[Ts] illegal?

I have added a runtime error for this case. It is consistent for Tuple, tuple and user generics.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting changes and removed awaiting core review labels

Mar 12, 2022

bedevere-bot commented Mar 12, 2022

Copy link

Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

All reactions

Sorry, something went wrong.

Uh oh!

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


          Raise explicit error if substitute a bare TypeVarTuple

f4dd9b3

serhiy-storchaka commented Mar 12, 2022

Copy link

Copy Markdown

Member Author

I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting change review and removed awaiting changes labels

Mar 12, 2022

bedevere-bot commented Mar 12, 2022

Copy link

Copy Markdown

Thanks for making the requested changes!

@JelleZijlstra: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot requested a review from JelleZijlstra

March 12, 2022 09:09

serhiy-storchaka added the skip news label

Mar 12, 2022

mrahtz commented Mar 12, 2022

Copy link

Copy Markdown

Contributor

Wow, thanks for implementing this - it hadn't even occurred to me this would be needed!

I also feel uneasy, though - as I mentioned in #31800 we'd decided against implementing it this way. I think we should pause work on this until we've decided on exactly which approach we're proceeding with.

JelleZijlstra reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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


          Merge branch 'main' into typevartuple-subst-c

30b5e9f

mrahtz commented Mar 13, 2022

Copy link

Copy Markdown

Contributor

Based on your note in #31800, I'm guessing you're going to proceed with this anyway. In that case, I think it would be worth adding some tests to this PR along the lines of b9b1c80#diff-04d29c98076c2d6bb75921ea9becb26a862544d39b71db87b6e354c759b9305dL794.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka commented Mar 13, 2022

Copy link

Copy Markdown

Member Author

I do not understand what tests are in question. The change you referred removes tests, not adds new tests.

All reactions

Sorry, something went wrong.

Uh oh!

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

mrahtz commented Mar 13, 2022

Copy link

Copy Markdown

Contributor

Right - I'm proposing adding them back, with the changes necessary to also test the code in this PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Mar 13, 2022

bpo-43224: Add more tests for typevar substitution #31844

Closed

bpo-43224: Add tests for TypeVarTuple substitution in Annotated #31846

Merged

serhiy-storchaka mentioned this pull request

Mar 17, 2022

bpo-44796: Unify TypeVar and ParamSpec substitution #31143

Merged

JelleZijlstra approved these changes

Apr 11, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels

Apr 11, 2022

mrahtz mannequin mentioned this pull request

Apr 11, 2022

Add support for PEP 646 #87390

Closed

AlexWaygood added the topic-typing label

Apr 13, 2022

mrahtz mentioned this pull request

Apr 29, 2022

PEP 646: Decide on substitution behavior #91162

Closed

JelleZijlstra and others added 2 commits

April 29, 2022 14:04


          Merge branch 'main' into typevartuple-subst-c

985f50f


          Update unpack test cases

0b9a4d3

(Code by Matthew Rahtz)

Co-authored-by: Matthew Rahtz <[email protected]>

JelleZijlstra approved these changes

Apr 29, 2022

View reviewed changes

JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Apr 29, 2022

bedevere-bot commented Apr 29, 2022

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 0b9a4d3 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Apr 29, 2022

serhiy-storchaka merged commit e8c2f72 into python:main

Apr 30, 2022

bedevere-bot removed the awaiting merge label

Apr 30, 2022

serhiy-storchaka deleted the typevartuple-subst-c branch

April 30, 2022 05:22

Reviewers

JelleZijlstra

JelleZijlstra approved these changes

gvanrossum

Awaiting requested review from gvanrossum

Fidget-Spinner

Awaiting requested review from Fidget-Spinner

Uh oh!

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

Assignees

No one assigned

Labels

skip news topic-typing type-feature

A feature request or enhancement

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

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.

GeezerVison

Conversation

serhiy-storchaka commented Mar 11, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JelleZijlstra commented Mar 11, 2022

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 12, 2022

Uh oh!

serhiy-storchaka commented Mar 12, 2022

Uh oh!

bedevere-bot commented Mar 12, 2022

Uh oh!

mrahtz commented Mar 12, 2022

Uh oh!

mrahtz commented Mar 13, 2022

Uh oh!

serhiy-storchaka commented Mar 13, 2022

Uh oh!

mrahtz commented Mar 13, 2022

Uh oh!

bedevere-bot commented Apr 29, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

serhiy-storchaka commented Mar 11, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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