GeezerVison

@mrahtz Could you please add some expected syntax errors to Lib/test/test_syntax.py? Things like def f(**kwargs: *a) and the like.

@pablogsal

Could you please add some expected syntax errors to Lib/test/test_syntax.py? Things like def f(**kwargs: *a) and the like.

Sure, will do. Two questions:

• Is there a convention for which tests should be implemented as doctests vs test cases in test_syntax.py?
• Should I move the uses of PEP 646 syntax that don't cause failure to test_grammar.py, or can they be kept in a test module of their own?

• Is there a convention for which tests should be implemented as doctests vs test cases in test_syntax.py?

If you want to check that something is a Syntax error and just want to assert the message and can do they with a simple input, in th doctest.

If you required special compilation modes, some gigantic or dynamic string or check the location of the errors, then a specific test case is needed.

• Should I move the uses of PEP 646 syntax that don't cause failure to test_grammar.py, or can they be kept in a test module of their own?

In general we have one test file for testing the runtime implications of the grammar, and things go to test_grammar to check grammatical relates aspects or the shape of the AST. Normally you can do both if you want/need

Sorry for the slow reply - I'm only just now finding time again to work on this.

Looking through test_grammar.py and test_syntax.py, it seemed like the most appropriate thing was just to move the failure cases from test_pep646_syntax.py to test_syntax.py. This is what I've done in the latest commit, but let me know if you'd like more doing.

@pablogsal Friendly poke :)

@pablogsal Another friendly poke :) I'm getting a bit nervous we might miss the merge window for 3.11 on this - if you're busy at the moment, would you be able to recommend anyone else who'd be a good fit for a review of this? (Or @JelleZijlstra do you know of anyone?)

Maybe @isidentical if you'd like to take a look?

Seems like Pablo already looked at the diff to some extent, so if he doesn't have time for a full re-review, I can also do a review and help get this in. I'd of course prefer if someone familiar with the parser can do a review, though.

I noticed that this is allowed by the grammar: s[1:1, *x]. It's not meaningful for typing, but I don't think we need to go out of our way to disallow it. Could you add some tests that combine slicing with unpacking?

Maybe @isidentical if you'd like to take a look?

Seems like Pablo already looked at the diff to some extent, so if he doesn't have time for a full re-review, I can also do a review and help get this in. I'd of course prefer if someone familiar with the parser can do a review, though.

Apologies, unfortunately I was sick with COVID these past weeks and I couldn't dedicate much time to open source :(

I will do another pass today or tomorrow.

Thanks for your understanding @mrahtz

I just fixed a merge conflict and a whitespace issue found by make patchcheck. I think we can go ahead and merge, but I noticed a few things we still need to address:

(1) The ast.py changes aren't tested. They should be tested with a call to ast.unparse.

(2) There is also a C version of unparse in Python/ast_unparse.c, which is used for from __future__ import annotations. It likely needs changes too.

(3) If you use from __future__ import annotations and call get_type_hints() on a function that takes *args: *Ts, it fails because it tries to parse *Ts as an expression. I guess to solve this we'll have to detect an initial * and remove it before trying to parse the code.

Here's an example that demonstrates these issues:

from __future__ import annotations
import typing
import ast

Ts = typing.TypeVarTuple("Ts")

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

print(f.__annotations__)
print(ast.unparse(ast.parse("tuple[*Ts]")))
print(typing.get_type_hints(f))

% ./python.exe Doc/tools/annotvt.py
{'x': 'tuple[(*Ts,)]', 'args': '*Ts'}
tuple[*Ts,]
Traceback (most recent call last):
  File "/Users/jelle/py/cpython/Lib/typing.py", line 738, in __init__
    code = compile(arg, '<string>', 'eval')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<string>", line 1
    *Ts
    ^
SyntaxError: invalid syntax

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/jelle/py/cpython/Doc/tools/annotvt.py", line 12, in <module>
    print(typing.get_type_hints(f))
          ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jelle/py/cpython/Lib/typing.py", line 2199, in get_type_hints
    value = ForwardRef(
            ^^^^^^^^^^^
  File "/Users/jelle/py/cpython/Lib/typing.py", line 740, in __init__
    raise SyntaxError(f"Forward reference must be an expression -- got {arg!r}")
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: Forward reference must be an expression -- got '*Ts'

Also, there were a few buildbot failures when Pablo triggered builds on 1543785, but apart from the whitespace issue I fixed, they looked like random failures on unrelated tests.

Thanks, go ahead, but do add the needed tests and fix the issues. Agreed on the parsing in get_type_hints.

Thanks @mrahtz for the fixes! I'll do another review today or maybe tomorrow and then hopefully merge.

I think we can leave the get_type_hints() fix for another PR; no need to worry about that here.

@JelleZijlstra Oops, I only just saw your message 😅 Let me know if you'd prefer to take the get_type_hints() fix out of this PR.

If you already implemented it, that's fine too :) Thanks!

Whoop whoop! :D