`ast.parse()` believes valid context manager py38 syntax to be invalid when `feature_version=(3, 8)` is passed

[AlexWaygood]

Bug report

Bug description:

The following code is completely valid on Python 3.8:

from contextlib import nullcontext

with (
    nullcontext() if bool() else nullcontext()
):
    pass

However, following 0daba82 (which was backported to Python 3.11 and Python 3.10), ast.parse() incorrectly throws an error if you try to parse this code with feature_version=(3, 8):

% python                                                                                                                         
Python 3.10.6 (main, Feb 24 2024, 10:35:05) [Clang 15.0.0 (clang-1500.1.0.2.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> source = """
... with (
...     nullcontext() if bool() else nullcontext()
... ):
...     pass
... """
>>> ast.parse(source, feature_version=(3, 8))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/alexw/.pyenv/versions/3.10.6/lib/python3.10/ast.py", line 50, in parse
    return compile(source, filename, mode, flags,
  File "<unknown>", line 5
    pass
        ^
SyntaxError: Parenthesized context managers are only supported in Python 3.9 and greater

Cc. @hauntsaninja / @pablogsal

CPython versions tested on:

3.8, 3.9, 3.10, 3.11, 3.12

Operating systems tested on:

macOS

Linked PRs

• gh-115881: Ensure ast.parse() parses conditional context managers even with low feature_version passed #115920
• [3.12] gh-115881: Ensure ast.parse() parses conditional context managers even with low feature_version passed (#115920) #115959
• [3.11] gh-115881: Ensure ast.parse() parses conditional context managers even with low feature_version passed (#115920) #115960
• gh-115881: Document feature_version limitations #115980
• [3.12] gh-115881: Document feature_version limitations (#115980) #116173
• [3.11] gh-115881: Document feature_version limitations (#115980) #116174

[pablogsal]

I think there isn't going to be any super clean way to do this because this new construct has a non trivial syntactic overlap with the case reported here. This was precisely what made it impossible to do with the old parser. I think the best way is to revert 0daba82 but maybe I am missing some elegant way to do it so I am open to alternatives

[pablogsal]

CC: @lysnikolaou

[hauntsaninja]

Thanks for reporting! I think we can fix by adding another grammar rule. I don't know if that meets the bar for elegant, if not, I agree reverting could be fine. I'm thinking of something like:

diff --git a/Grammar/python.gram b/Grammar/python.gram
index 174b4dbb6f..1526b52ec8 100644
--- a/Grammar/python.gram
+++ b/Grammar/python.gram
@@ -392,6 +392,8 @@ for_stmt[stmt_ty]:
 
 with_stmt[stmt_ty]:
     | invalid_with_stmt_indent
+    | 'with' '(' e=expression ','? ')' ':' tc=[TYPE_COMMENT] b=block {
+       _PyAST_With(CHECK(asdl_withitem_seq*, _PyPegen_singleton_seq(p, _PyAST_withitem(e, NULL, p->arena))), b, NEW_TYPE_COMMENT(p, tc), EXTRA) }
     | 'with' '(' a[asdl_withitem_seq*]=','.with_item+ ','? ')' ':' tc=[TYPE_COMMENT] b=block {
        CHECK_VERSION(stmt_ty, 9, "Parenthesized context managers are", _PyAST_With(a, b, NEW_TYPE_COMMENT(p, tc), EXTRA)) }
     | 'with' a[asdl_withitem_seq*]=','.with_item+ ':' tc=[TYPE_COMMENT] b=block {

By the way, I notice that with (contextlib.nullcontext(),): ... and with (contextlib.nullcontext(),): ... parse differently with old and new parser! :-)

[pablogsal]

I prefer to revert the commit. The grammar is already quite complicated and we are automatically generating docs from it so I would recommend you try to keep complexity controlled

[pablogsal]

By the way, I notice that with (contextlib.nullcontext(),): ... and with (contextlib.nullcontext(),): ... parse differently with old and new parser! :-)

Maybe it's because I am reading this in a phone but I cannot see any difference 😅

In any case we were aware of that being a thing and other similar cases but in all of them we were ok with the change since tuples will always fail as context managers 😉

[AlexWaygood]

I prefer to revert the commit. The grammar is already quite complicated and we are automatically generating docs from it so I would recommend you try to keep complexity controlled

We'll want to backport the fix as well, so I agree that simply reverting feels safer for now (and possibly consider a more complex solution for Python 3.13+ only? But I'll defer to those more experienced with hacking on the parser on whether that's worth it or not :)

[lysnikolaou]

Yup, I agree with reverting this.

Like I've said a few times before, deprecating feature_version might also be a good idea. @hauntsaninja Do I remember correctly that mypy uses this? Is it still relevant?