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?

[AlexWaygood]

Do I remember correctly that mypy uses this? Is it still relevant?

Yes, we do use it at mypy! That's how I came across this issue in the first place -- after we started using a new version of black at mypy, I realised mypy could no longer type check its own codebase if you were using --python-version 3.8 on Python 3.12: python/mypy#16945

[lysnikolaou]

Got it. Thanks for the context @AlexWaygood!

[pablogsal]

The problem with feature_version is that is quite challenging to get this working in a consistent fashion. Different syntax constructs are in different version of the parser and having feature_version working consistently forces us to technically parse multiple grammars and our parser is not prepared to do so unless there is a fundamental check in the AST nodes that we can pinpoint to tell the difference.

I think at the very least we should document the limitations in the docs, to ensure that the fact that for example context managers with parentheses may not be detected is clear to end users.

What do people think?

[AlexWaygood]

The fix for the original issue has been merged and backported, but I'll leave this open so we can continue discussing what to do about the broader issue, and if necessary link more PRs here

[lysnikolaou]

Yup, let's document this at the very least. If we were ever to remove this, we'd have to go through a deprecation, cause it seems to be used in quite a few places.

[hauntsaninja]

Thanks Alex! Yes, I believe it's used by at least mypy, black, pytype. We should document more clearly thatfeature_version is a "best effort" sort of thing, here's a PR with some possible wording: #115980

I think the functionality is useful, even if limited, so I'd prefer not to deprecate

[hauntsaninja]

Okay, we did the revert, we've updated docs and I did the backports, so I think we can close. Let me know if I missed something!