[syntax-errors] Parenthesized context managers before Python 3.9

[ntBre]

Summary

I thought this was very complicated based on the comment here: #16106 (comment) and on some of the discussion in the CPython issue here: python/cpython#56991. However, after a little bit of experimentation, I think it boils down to this example:

with (x as y): ...

The issue is parentheses around a with item with an optional_var, as we (and Python) call the trailing variable name (y in this case). It's not actually about line breaks after all, except that line breaks are allowed in parenthesized expressions, which explains the validity of cases like

>>> with (
...     x,
...     y
... ) as foo:
...     pass
... 

even on Python 3.8.

I followed pyright's example again here on the diagnostic range (just the opening paren) and the wording of the error.

Test Plan

Inline tests

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[ntBre]

Thanks for the reviews! I

• relocated the check to parse_with_items
• added variant docs
• expanded the test_ok cases with Micha's suggestion and some of the other cases I put in the docs

[MichaReiser]

Okay, this is a bit more subtle. I wrote a longer inline comment about the problem and listed a few options

[MichaReiser]

@BurntSushi's new diagnostic system will allow us to mark both parentheses.

[MichaReiser]

It is not directly related to your PR, but it is somewhat related that the parser now supports a target_version option.

If given:

with (CtxManager1(), CtxManager2()):
    ...

• Pre 3.9: The context manager expression is a tuple which is always a runtime error (I think)
• 3.9 or newer: This parses as a multiple-item context manager.

There are two concerns here:

1. Always parsing the above as a context manager with multiple with items affects downstream tools. E.g., a type checker will error when parsed as a tuple because the tuple doesn't implement the context manager protocol. However, the code might be valid when parsed as a with statement with multiple context expressions.
2. We'll miss errors that are technically not syntax errors, but they are most certainly not what the user wanted.

I think we have two options here:

1. We make the backwards compatibility stricter than it has to be and flag any parenthesized context manager with more than one expression or with a trailing comma as an error because it is parsed as a tuple on Python 3.8 or older
2. We leave it as is and consider this a concern of a lint rule to catch tuples in context manager positions.

I'm not sure if we should change the parser to correctly parse this as a tuple instead of a statement with multiple context managers. I think it's fine to leave it as is for 1, but we should definitely change the parser if we go for 2.

I just tested how pyright handles this and pyright always flags:

with (CtxManager1(), CtxManager2()):
    ...

While this is not a 100% correct, I think it's accurate enough and could be a simplified fix for 1 and 2, but I'm interested in more opinions.

[dhruvmanila]

Hmm, that's a good point.

• Pre 3.9: The context manager expression is a tuple which is always a runtime error (I think)

Yes, it's a runtime error. I quickly tested it out:

Context managers as tuple test code

from contextlib import contextmanager


@contextmanager
def context1():
    yield 1


@contextmanager
def context2():
    yield 2


with (context1(), context2()):
    ...

My initial thought was to change the parser to emit the correct AST as per the target version but that is something that we only need to question for this specific change as it seems to be the only change which not only changes the grammar but that in turn changes the parsed AST depending on the target version. But, I'm not sure if that'd actually provide any advantages.

I think my preference would be to go for (1) given that (a) it's already done by Pyright and I don't see any issues that users have brought up, (b) mypy doesn't flag this case on 3.8 either (mypy playground), and (c) Python 3.8 is has reached EOL.

[ntBre]

Oh that's a very good point, thanks for catching that.

Option (1) sounds like the easier of the two, but I see some appeal in having it as a lint rule too. I guess it's a bit weird to flag this as a SyntaxError, but like you said, I don't think there's any way for this not to cause an error at runtime. It also seems like a pain to change the parser for such an old Python version.

pyright is actually a little touchy here, I think. It flags the case with multiple elements as a parenthesized context manager, but for a single-element tuple, you get a type error about the tuple not implementing __enter__ and __exit__.

[dhruvmanila]

(A bit embarrassing but I just found out that my comment was still pending, submitting it now)

[dhruvmanila]

Hmm, that's a good point.

• Pre 3.9: The context manager expression is a tuple which is always a runtime error (I think)

Yes, it's a runtime error. I quickly tested it out:

Context managers as tuple test code

from contextlib import contextmanager


@contextmanager
def context1():
    yield 1


@contextmanager
def context2():
    yield 2


with (context1(), context2()):
    ...

My initial thought was to change the parser to emit the correct AST as per the target version but that is something that we only need to question for this specific change as it seems to be the only change which not only changes the grammar but that in turn changes the parsed AST depending on the target version. But, I'm not sure if that'd actually provide any advantages.

I think my preference would be to go for (1) given that (a) it's already done by Pyright and I don't see any issues that users have brought up, (b) mypy doesn't flag this case on 3.8 either (mypy playground), and (c) Python 3.8 is has reached EOL.

[ntBre]

I implemented option (1) as discussed above (always flagging cases that we parse as parenthesized context managers).

The only remaining ok case is with (foo, bar, baz) as tup: .... This could be a good candidate for a lint rule because we parse this accurately as a parenthesized expression within the with statement, making it actually accessible to ruff or red-knot as a tuple without having to modify the parser. This matches pyright too, which raises a type error about a tuple not implementing __enter__ or __exit__.