[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.

[MichaReiser]

Do we also need to emit the same diagnostic on line 2144, e.g. for with (foo, bar):

[ntBre]

No, that syntax is actually okay. I think (foo, bar) is just a tuple, so it falls under the expression grammar. The tuple doesn't have an __enter__ method, so it causes a runtime error, but it does parse and even compile I guess.

As far as I can tell, the problem with parentheses is specific to the as case since that's part of the with statement grammar.

[ntBre]

Python 3.7.9 (default, Aug 23 2020, 00:57:53)
[Clang 10.0.1 ] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> ast.parse("with (foo, bar): ...")
<_ast.Module object at 0x70849fd20c10>
>>> ast.dump(_)
"Module(body=[With(items=[withitem(context_expr=Tuple(elts=[Name(id='foo', ctx=Load()), Name(id='bar', ctx=Load())], ctx=Load()), optional_vars=None)], body=[Expr(value=Ellipsis())])])"

[MichaReiser]

It would be nice to add documentation, similar to what you did for other variants.

[dhruvmanila]

Should we move this check down below where we're sure that the parenthesis belongs to the with items and not the expression? i.e., on line 2162

        if with_item_kind.is_parenthesized() {

[dhruvmanila]

Or, in parse_with_items like:

        if self.at(TokenKind::Lpar) {
			let lpar_range = self.current_token_range();
            if let Some(items) = self.try_parse_parenthesized_with_items() {
				// check target version and raise error
                self.expect(TokenKind::Rpar);
                items
            } else {

[ntBre]

Oh nice, I moved it to parse_with_items. I just had to add a separate check for optional_vars otherwise we get a false positive on the case Micha mentioned above. I added that as a test_ok case to be sure.

[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