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-43224: Draft implementation of PEP 646 by mrahtz · Pull Request #30398 · python/cpython

Conversation

@mrahtz

@mrahtz mrahtz commented Jan 4, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

PEP 646 (Variadic Generics) hasn't yet been completely approved by the steering council, but here's a draft of its implementation so we can get started on code review.

Update 2021-01-19: the SC has approved the PEP, so full steam ahead!

TODO:

  • Finish addressing @pradeep90's comments from the previous PR, bpo-43224: Initial implementation of PEP 646 in typing.py #24527
  • Add tests for typing.get_args and typing.get_origin (rather than just checking __args__ etc)

A couple of notes below.

@pradeep90 @gvanrossum @Fidget-Spinner

Starred tuple types

I realised there's a tricky issue with starred tuple types.

If we have e.g. tuple[int, *tuple[str, bool]], the obvious thing to do would be to unpack the inner tuple at runtime, resulting in tuple[int, str, bool]. This would imply that iter(tuple[str, bool]) creates an iterator returning str then bool.

But this would cause problems when using starred tuple types as the type of *args, e.g. def foo(*args: *tuple[str, bool]):

  1. The type of foo.__annotations__['args'] should definitely not be tuple[str, bool]. That would instead imply def foo(*args: tuple[str, bool])
  2. The current PEP draft states that when a starred object is used as the annotation of *args, its iterator should return only a single value. (Or at least, this is the behaviour specified for *args: *Ts; @pradeep90, I've just realised, should we update that section of the PEP to make it explicit that that starred tuples can be also used for *args?)

I think the simplest solution to both of these problems is to not unpack tuple types as runtime, having *tuple[stuff] instead creating an iterator that returns only a single item, an instance of a new 'starred tuple'. (Note that such a type does have to exist anyway, in order to represent e.g. *tuple[int, ...].)

For the native tuple type, I've tentatively implemented this by adding a new starred flag to gaobject in genericaliasobject.c rather than creating a whole new type. This makes implementation easier, but maybe it's suboptimal considering that gaobject is used for lots of things other than tuple - feedback appreciated. For typing.Tuple, I've implemented a new class in typing.py called StarredTuple, which should behave in the same way. (This does mean that *tuple[int] != *Tuple[int], but given that tuple[int] != Tuple[int], this seems fine.)

Implementation of *tuple

I'm pretty uncertain about my implementation in genericaliasobject.c. In particular:

  • I don't know what I'm doing with garbage collection; scrutiny on whether I'm doing it right appreciated.
  • I wonder whether there's a way to make it simpler. In typing.py, the implementation is as simple as def __iter__(self): yield StarredTuple(self). Is there a way to do something similar in ga_iter, without the need to also implement ga_iternext?

https://bugs.python.org/issue43224

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mrahtz added 9 commits January 2, 2022 18:50
@mrahtz
PEP 646: Grammar and compiler changes
64abc0a
@mrahtz
PEP 646: Add tests for syntax changes
b725671
@mrahtz
PEP 646: Add AST tests for *args annotations
ea329b9 
Formatted, the generated AST is as follows.

`def f(*args: *Ts): pass`:

```
(
    "Module",
    [
        (
            "FunctionDef",
            (1, 0, 1, 23),
            "f",
            (
                "arguments",
                [],
                [],
                (
                    "arg",
                    (1, 7, 1, 16),
                    "args",
                    (
                        "Starred",
                        (1, 13, 1, 16),
                        ("Name", (1, 14, 1, 16), "Ts", ("Load",)),
                        ("Load",),
                    ),
                    None,
                ),
                [],
                [],
                None,
                [],
            ),
            [("Pass", (1, 19, 1, 23))],
            [],
            None,
            None,
        )
    ],
    [],
)
```

`def f(*args: *tuple[int, ...]): pass`:

```
(
    "Module",
    [
        (
            "FunctionDef",
            (1, 0, 1, 36),
            "f",
            (
                "arguments",
                [],
                [],
                (
                    "arg",
                    (1, 7, 1, 29),
                    "args",
                    (
                        "Starred",
                        (1, 13, 1, 29),
                        (
                            "Subscript",
                            (1, 14, 1, 29),
                            ("Name", (1, 14, 1, 19), "tuple", ("Load",)),
                            (
                                "Tuple",
                                (1, 20, 1, 28),
                                [
                                    ("Name", (1, 20, 1, 23), "int", ("Load",)),
                                    ("Constant", (1, 25, 1, 28), Ellipsis, None),
                                ],
                                ("Load",),
                            ),
                            ("Load",),
                        ),
                        ("Load",),
                    ),
                    None,
                ),
                [],
                [],
                None,
                [],
            ),
            [("Pass", (1, 32, 1, 36))],
            [],
            None,
            None,
        )
    ],
    [],
)
```

`def f(*args: *tuple[int, *Ts]): pass`:

```
(
    "Module",
    [
        (
            "FunctionDef",
            (1, 0, 1, 36),
            "f",
            (
                "arguments",
                [],
                [],
                (
                    "arg",
                    (1, 7, 1, 29),
                    "args",
                    (
                        "Starred",
                        (1, 13, 1, 29),
                        (
                            "Subscript",
                            (1, 14, 1, 29),
                            ("Name", (1, 14, 1, 19), "tuple", ("Load",)),
                            (
                                "Tuple",
                                (1, 20, 1, 28),
                                [
                                    ("Name", (1, 20, 1, 23), "int", ("Load",)),
                                    (
                                        "Starred",
                                        (1, 25, 1, 28),
                                        ("Name", (1, 26, 1, 28), "Ts", ("Load",)),
                                        ("Load",),
                                    ),
                                ],
                                ("Load",),
                            ),
                            ("Load",),
                        ),
                        ("Load",),
                    ),
                    None,
                ),
                [],
                [],
                None,
                [],
            ),
            [("Pass", (1, 32, 1, 36))],
            [],
            None,
            None,
        )
    ],
    [],
)
```
@mrahtz
PEP 646: Add AST tests for return types with unpacking
681dc4e 
Formatted, generated AST is as follows.

`def f() -> tuple[*Ts]: pass`:

```
(
    "Module",
    [
        (
            "FunctionDef",
            (1, 0, 1, 27),
            "f",
            ("arguments", [], [], None, [], [], None, []),
            [("Pass", (1, 23, 1, 27))],
            [],
            (
                "Subscript",
                (1, 11, 1, 21),
                ("Name", (1, 11, 1, 16), "tuple", ("Load",)),
                (
                    "Tuple",
                    (1, 17, 1, 20),
                    [
                        (
                            "Starred",
                            (1, 17, 1, 20),
                            ("Name", (1, 18, 1, 20), "Ts", ("Load",)),
                            ("Load",),
                        )
                    ],
                    ("Load",),
                ),
                ("Load",),
            ),
            None,
        )
    ],
    [],
)
```

`def f() -> tuple[int, *Ts]: pass`:

```
(
    "Module",
    [
        (
            "FunctionDef",
            (1, 0, 1, 32),
            "f",
            ("arguments", [], [], None, [], [], None, []),
            [("Pass", (1, 28, 1, 32))],
            [],
            (
                "Subscript",
                (1, 11, 1, 26),
                ("Name", (1, 11, 1, 16), "tuple", ("Load",)),
                (
                    "Tuple",
                    (1, 17, 1, 25),
                    [
                        ("Name", (1, 17, 1, 20), "int", ("Load",)),
                        (
                            "Starred",
                            (1, 22, 1, 25),
                            ("Name", (1, 23, 1, 25), "Ts", ("Load",)),
                            ("Load",),
                        ),
                    ],
                    ("Load",),
                ),
                ("Load",),
            ),
            None,
        )
    ],
    [],
)
```

`def f() -> tuple[int, *tuple[int, ...]]: pass`:

```
(
    "Module",
    [
        (
            "FunctionDef",
            (1, 0, 1, 45),
            "f",
            ("arguments", [], [], None, [], [], None, []),
            [("Pass", (1, 41, 1, 45))],
            [],
            (
                "Subscript",
                (1, 11, 1, 39),
                ("Name", (1, 11, 1, 16), "tuple", ("Load",)),
                (
                    "Tuple",
                    (1, 17, 1, 38),
                    [
                        ("Name", (1, 17, 1, 20), "int", ("Load",)),
                        (
                            "Starred",
                            (1, 22, 1, 38),
                            (
                                "Subscript",
                                (1, 23, 1, 38),
                                ("Name", (1, 23, 1, 28), "tuple", ("Load",)),
                                (
                                    "Tuple",
                                    (1, 29, 1, 37),
                                    [
                                        ("Name", (1, 29, 1, 32), "int", ("Load",)),
                                        ("Constant", (1, 34, 1, 37), Ellipsis, None),
                                    ],
                                    ("Load",),
                                ),
                                ("Load",),
                            ),
                            ("Load",),
                        ),
                    ],
                    ("Load",),
                ),
                ("Load",),
            ),
            None,
        )
    ],
    [],
)
```
@mrahtz
PEP 646: Add AST tests for variable annotations with unpacking
69f575f 
Formatted, generated AST is as follows:

`x: tuple[*Ts]`:

```
(
    "Module",
    [
        (
            "AnnAssign",
            (1, 0, 1, 13),
            ("Name", (1, 0, 1, 1), "x", ("Store",)),
            (
                "Subscript",
                (1, 3, 1, 13),
                ("Name", (1, 3, 1, 8), "tuple", ("Load",)),
                (
                    "Tuple",
                    (1, 9, 1, 12),
                    [
                        (
                            "Starred",
                            (1, 9, 1, 12),
                            ("Name", (1, 10, 1, 12), "Ts", ("Load",)),
                            ("Load",),
                        )
                    ],
                    ("Load",),
                ),
                ("Load",),
            ),
            None,
            1,
        )
    ],
    [],
)
```

`x: tuple[int, *Ts]`:

```
(
    "Module",
    [
        (
            "AnnAssign",
            (1, 0, 1, 18),
            ("Name", (1, 0, 1, 1), "x", ("Store",)),
            (
                "Subscript",
                (1, 3, 1, 18),
                ("Name", (1, 3, 1, 8), "tuple", ("Load",)),
                (
                    "Tuple",
                    (1, 9, 1, 17),
                    [
                        ("Name", (1, 9, 1, 12), "int", ("Load",)),
                        (
                            "Starred",
                            (1, 14, 1, 17),
                            ("Name", (1, 15, 1, 17), "Ts", ("Load",)),
                            ("Load",),
                        ),
                    ],
                    ("Load",),
                ),
                ("Load",),
            ),
            None,
            1,
        )
    ],
    [],
)
```

`x: tuple[int, *tuple[str, ...]]`:

```
(
    "Module",
    [
        (
            "AnnAssign",
            (1, 0, 1, 31),
            ("Name", (1, 0, 1, 1), "x", ("Store",)),
            (
                "Subscript",
                (1, 3, 1, 31),
                ("Name", (1, 3, 1, 8), "tuple", ("Load",)),
                (
                    "Tuple",
                    (1, 9, 1, 30),
                    [
                        ("Name", (1, 9, 1, 12), "int", ("Load",)),
                        (
                            "Starred",
                            (1, 14, 1, 30),
                            (
                                "Subscript",
                                (1, 15, 1, 30),
                                ("Name", (1, 15, 1, 20), "tuple", ("Load",)),
                                (
                                    "Tuple",
                                    (1, 21, 1, 29),
                                    [
                                        ("Name", (1, 21, 1, 24), "str", ("Load",)),
                                        ("Constant", (1, 26, 1, 29), Ellipsis, None),
                                    ],
                                    ("Load",),
                                ),
                                ("Load",),
                            ),
                            ("Load",),
                        ),
                    ],
                    ("Load",),
                ),
                ("Load",),
            ),
            None,
            1,
        )
    ],
    [],
)
```
@mrahtz
PEP 646: Add support for starring tuple types
095a620 
E.g. tuple[*tuple[int, str]].
@mrahtz
PEP 646: Add tests for starring tuple types
6713748
@mrahtz
PEP 646: Implement support in typing.py
038b8f0
@mrahtz
PEP 646: Add tests for everything in typing.py
f028339
@mrahtz
mrahtz requested review from Fidget-Spinner, gvanrossum, lysnikolaou, markshannon and pablogsal as code owners January 4, 2022 12:16
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 4, 2022
@bedevere-bot bedevere-bot added the awaiting review label Jan 4, 2022
@mrahtz
mrahtz marked this pull request as draft January 4, 2022 19:38
@mrahtz
PEP 646: Add tests for typing.get_{origin,args}
38c17d3 
and fix a couple of bugs the tests revealed
Fidget-Spinner
Fidget-Spinner reviewed Jan 10, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner 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.

Wow, this is seriously impressive work and a very strong start! I don't have time to review everything right now, so far I've covered genericaliasobject.c and compile.c. I still have typing.py and friends left. I'm hopeless at the parser and grammar so I'll leave those to someone else.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pep646_syntax.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genericaliasobject.c

if (gi->exhausted) {
gi->obj = NULL;
return NULL;

@Fidget-Spinner Fidget-Spinner Jan 10, 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 feel like you could potentially be missing a decref here for gi->obj since I see it being increfed in gi_iter. If so,

Suggested change
return NULL;
Py_SETREF(gi->obj, NULL);
return NULL;

If I'm just shooting my mouth off, then just please ignore what I said :).

One way to test for refleaks is to run the tests with the refleak hunter (requires a debug build):

python_d.exe -m test test_typing -R 3:3

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 15, 2022

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

Oh, super nice about the refleak hunter - I had no idea that was a thing!

And good catch about the decref too. One thing I'm unsure about is where we should do it, though. Reading https://docs.python.org/3/c-api/gcsupport.html, am I right in thinking that the iterator object gi counts as a container type, because it contains references to another object? If so, that same page says "Before fields which refer to other containers are invalidated, PyObject_GC_UnTrack() must be called". That would imply we should be setting gi->obj to NULL in the deallocator, in which case we'd also call the setref there?

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 25, 2022

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

Friendly poke :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed Jan 10, 2022
View reviewed changes
Comment thread Grammar/python.gram
slices[expr_ty]:
| a=slice !',' { a }
| a[asdl_expr_seq*]=','.slice+ [','] { _PyAST_Tuple(a, Load, EXTRA) }
| a[asdl_expr_seq*]=','.(slice | starred_expression)+ [','] { _PyAST_Tuple(a, Load, EXTRA) }

@pablogsal pablogsal Jan 10, 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 would prefer to move starred_expression as another option of slice as we already have named_expression there

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 15, 2022

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

I just tried this, but then I get SyntaxError: can't use starred expression here for e.g. a[*b].

I think what happens is, if we move starred_expression to slice (as | a=starred_expression { a }), then when we do a[*b], it triggers the first option of slices, where the starred_expression doesn't get processed by _PyAST_Tuple.

I can't see any way to make sure _PyAST_Tuple definitely gets called for starred_expression without doing something to slices, so I think the original attempt with (slice | starred_expression) is cleanest. Wdyt?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pablogsal
pablogsal reviewed Jan 10, 2022
View reviewed changes
Comment thread Grammar/python.gram
param_no_default[arg_ty]:
| a=param ',' tc=TYPE_COMMENT? { _PyPegen_add_type_comment_to_arg(p, a, tc) }
| a=param tc=TYPE_COMMENT? &')' { _PyPegen_add_type_comment_to_arg(p, a, tc) }
param_no_default_star_annotation[arg_ty]:

@pablogsal pablogsal Jan 10, 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.

Why not join these to param_no_default? That way we can remove the extra rule in star_etc.

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 15, 2022

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

Hmm, wouldn't that mess with other things that use param_no_default though? E.g. kwds uses param_no_default, so if we joined these to param_no_default, wouldn't we accidentally allow def foo(***args)?

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 25, 2022

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

Friendly poke :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mrahtz
mrahtz marked this pull request as ready for review January 14, 2022 19:33
@mrahtz
mrahtz marked this pull request as draft January 14, 2022 19:33
mrahtz added 2 commits January 15, 2022 10:36
@mrahtz
PEP 646: Various fixes to genericaliasobject.c
9991937 
Based on Fidget-Spinner's feedback:
1. Remove the explicit flag for 'exhausted', instead using 'gi->obj == NULL' to tell whether the iterator is exhausted.
2. Construct `starred_tuple` using the `Py_GenericAlias` constructor instead of constructing it manually.
3. Fix reference counting.
4. Make `Py_GenericAliasIterType` static.
@mrahtz
PEP 646: Fix test_pep646_syntax import
9017d22
JelleZijlstra
JelleZijlstra reviewed Jan 15, 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.

Just a couple of things I noticed while working on the typing-extensions implementation.

I'm not going to be the one to merge this, but I think it might make sense to split this PR into separate PRs for the grammar changes and the typing.py changes, since they touch very different parts of the codebase and need review from different people.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/typing.py
class StarredTuple(_Final, _Immutable, _root=True):
"""Implementation of starred tuple for older versions of Python.

A starred tuple is e.g. tuple[*tuple[int, str]]. In newer versions of

@JelleZijlstra JelleZijlstra Jan 15, 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.

But this implementation is only used in new versions of Python, so why do we need this?

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 17, 2022

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

Ah yeah, thanks for the reminder on this too - I was unsure about this.

The two things I had in mind were:

  • It was easier to just put everything in one PR so that when doing code review we could compare between the 'new' and the 'backwards compatible' implementations.
  • We probably want some of the 'backwards compatible' implementations even in modern typing.py for the sake of code (or maybe even pickle?) compatibility. Unpack is probably the most important one, so we can read backwards-compatible code written as class C(Generic[Unpack[Ts]]). I'm admittedly less sure about StarredTuple; I don't think there's ever a reason to instantiate it explicitly (and in fact maybe it should be private?). In any case, not sure what best practice is in cases like this - @gvanrossum can you comment?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/typing.py
def __class_getitem__(cls, item):
if isinstance(item, types.GenericAlias) and item.__origin__ is tuple:
# tuple[...]
return StarredTuple(item)

@JelleZijlstra JelleZijlstra Jan 15, 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.

Could this return an instance of Unpack instead so we don't need to add an additional class?

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 17, 2022

Copy link
Copy Markdown
Contributor 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's definitely an option, but I'd prefer to keep Unpack as only implementing the operator, rather implementing both the operator and the wrapper class for unpacked items - in line with the principle of "Each component should only do one thing".

Also, I think Unpack could get really messy if we have to merge in the guts of both StarredTuple and UnpackedTypeVarTuple. And there are a bunch of places where we want to check for an unpacked TypeVarTuple but not an unpacked StarredTuple, so they would get more complicated too.

Maybe there's context I'm missing though. Is there a big downside (e.g. maintenance burden) to adding another class?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/typing.py
elif isinstance(item, TypeVarTuple):
return item._unpacked
else:
raise TypeError(

@JelleZijlstra JelleZijlstra Jan 15, 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'd prefer to not raise an error here, so we don't hobble potential future extensions. For example, I anticipate that we may want to support Unpack[TypedDict] to type **kwargs in the future.

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Jan 17, 2022

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

a) If the concern is future extensions, couldn't we just amend this function when that need arises in the future?

b) Since the result of unpacking depends on what's being unpacked, what would we do instead of raising an error here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mrahtz
PEP 646: Add docstring for TypeVarTuple
07ed599
@mrahtz
mrahtz marked this pull request as ready for review January 19, 2022 11:23
kumaraditya303
kumaraditya303 reviewed Jan 19, 2022
View reviewed changes
Comment thread Lib/test/test_ast.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 reviewed Jan 19, 2022
View reviewed changes
Comment thread Lib/test/test_genericalias.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 reviewed Jan 19, 2022
View reviewed changes
Comment thread Lib/test/test_genericalias.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 reviewed Jan 19, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
mrahtz and others added 4 commits January 20, 2022 16:23
@mrahtz
PEP 646: Swap a Py_INCREF with a Py_NewRef
74e5252
@mrahtz
PEP 646: Add test for assigning to a slice containing a star
6cb3aa5
@blurb-it
📜🤖 Added by blurb_it.
daef59b
@mrahtz
PEP 646: Add line to Doc/whatsnew
3c2a7e7
kumaraditya303
kumaraditya303 reviewed Jan 21, 2022
View reviewed changes
Comment thread Lib/test/test_genericalias.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 reviewed Jan 21, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@mrahtz
PEP 646: Use stdbool.h; separate test cases
456156a
@mrahtz

mrahtz commented Jan 30, 2022

Copy link
Copy Markdown
Contributor Author

This PR has grown pretty big, and Pradeep agrees with Jelle that it might be better to split this up into separate parts - so I'm going to close this PR, and open a few new ones instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mrahtz mrahtz closed this Jan 30, 2022
This was referenced Jan 30, 2022
bpo-43224: Implement PEP 646 grammar changes #31018
Merged
bpo-43224: Implement PEP 646 changes to genericaliasobject.c #31019
Merged
bpo-43224: Implement PEP 646 changes to typing.py #31021
Merged
mrahtz
mrahtz commented Jan 30, 2022
View reviewed changes
Comment thread Lib/typing.py
return self._name


class UnpackedTypeVarTuple(_Final, _Immutable, _root=True):

@mrahtz mrahtz Jan 4, 2022

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

@pradeep90 In the previous review, you mentioned you'd prefer to avoid this middleman class, instead for example making it so that starring a TypeVarTuple results in an instance of Unpack.

That example solution seems suboptimal to me for two reasons:

  1. In a few places in the rest of typing.py, we want to check whether an object is an unpacked TypeVarTuple, and instance(obj, UnpackedTypeVarTuple) seems more readable to me than isinstance(obj, Unpacked).
  2. It seems potentially confusing to have Unpack both be an operator and an object.

Another option would be something like:

class TypeVarTuple:

    def __init__(self, name, _starred=False, _id=None):
        self.name = name
        self._starred = starred
        self._id = id

    def __iter__(self):
        yield TypeVarTuple(self.name, _starred=True, _id=id(self))

    def __eq__(self, other):
        if self._id is not None and other._id is not None:
            return self._id == other._id
        else:
            return object.__eq__(self, other)  # Not sure whether this is right, but you get the gist

    def __repr__(self):
         return ('*' if self._starred else '') + self.name

I lean away from this solution too because I feel like manually managing IDs could lead to subtle bugs, and isinstance(obj, UnpackedTypeVarTuple) is still simpler than isinstance(obj, TypeVarTuple) and obj._starred, but WDYT?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/typing.py
self.__module__ = def_mod


class TypeVarTuple(_Final, _Immutable, _root=True):

@mrahtz mrahtz Jan 4, 2022

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

@pradeep90 You mentioned in the previous review you'd prefer to have this inherit from _TypeVarLike. I'm not sure I understood your reasoning - could you explain what you meant by "The flags are things that the typechecker has to deal with"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pep646_syntax.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genericaliasobject.c

if (gi->exhausted) {
gi->obj = NULL;
return NULL;

@mrahtz mrahtz Jan 15, 2022

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

Oh, super nice about the refleak hunter - I had no idea that was a thing!

And good catch about the decref too. One thing I'm unsure about is where we should do it, though. Reading https://docs.python.org/3/c-api/gcsupport.html, am I right in thinking that the iterator object gi counts as a container type, because it contains references to another object? If so, that same page says "Before fields which refer to other containers are invalidated, PyObject_GC_UnTrack() must be called". That would imply we should be setting gi->obj to NULL in the deallocator, in which case we'd also call the setref there?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_genericalias.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/genericaliasobject.c

if (gi->exhausted) {
gi->obj = NULL;
return NULL;

@mrahtz mrahtz Jan 25, 2022

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

Friendly poke :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Grammar/python.gram
param_no_default[arg_ty]:
| a=param ',' tc=TYPE_COMMENT? { _PyPegen_add_type_comment_to_arg(p, a, tc) }
| a=param tc=TYPE_COMMENT? &')' { _PyPegen_add_type_comment_to_arg(p, a, tc) }
param_no_default_star_annotation[arg_ty]:

@mrahtz mrahtz Jan 25, 2022

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

Friendly poke :)

Sorry, something went wrong.

Uh oh!

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

mrahtz commented Jan 30, 2022

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra @pablogsal @Fidget-Spinner @pradeep90 Oh, gosh, I'm so sorry - I've just realised I'd misunderstood a critical part of GitHub's code review interface! I'd assumed all these comments had been posted weeks ago - I'd missed the part where you actually have to click the 'Submit code review' button to post them! Uff. I'll copy over these threads to the new PRs.
All reactions

Sorry, something went wrong.

Uh oh!

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

pablogsal commented Jan 30, 2022

Copy link
Copy Markdown
Member

@mrahtz I can review and help with the GC part. Feel free to add me as a reviewer in that PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mrahtz mrahtz mannequin mentioned this pull request Apr 11, 2022
Add support for PEP 646 #87390
Closed
@mrahtz
mrahtz deleted the pep646 branch May 1, 2022 15:30
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
@pablogsal pablogsal pablogsal left review comments
@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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.

7 participants

@mrahtz @pablogsal @JelleZijlstra @Fidget-Spinner @kumaraditya303 @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.