Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Black fails to parse parenthesized (manager() as x) in with statements #1948

Closed
altendky opened this issue Jan 29, 2021 · 8 comments
Closed
Labels
C: parser How we parse code. Or fail to parse it. help wanted Extra attention is needed S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: bug Something isn't working

Comments

@altendky
Copy link

Describe the bug A clear and concise description of what the bug is.

Black fails to parse with statements that are both parenthesized and use as such as with (open("a", "w") as a):. I found this while noting that black would not reformat an overly long line of the form with a as b, x as y:.

To Reproduce Steps to reproduce the behavior:

https://black.now.sh/?version=master&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4ADxAGhdAD2IimZxl1N_WmufDs0LpGEw9emYfCBakMKmw1q3jXUj3wtHo6NCOupRzg7ydqs0H1WF8HT1qPymFuq8UolBICiBSQevQsn3TFo9kaHrTJtsFiiPLpaMZ8_MAWjg5A9X2Ar4c0sX0RkzAApiLbCr57chAAGEAfIBAAC4H7lWscRn-wIAAAAABFla

with open("a", "w"):
    pass

with open("a", "w") as a:
    pass

with (open("a", "w")):
    pass

with (open("a", "w") as a):
    pass
Cannot parse: 10:21: with (open("a", "w") as a):

Expected behavior A clear and concise description of what you expected to happen.

I expect that black does not fail to parse and also reformats as needed when the line is too long. Reformatting would include adding parentheses and wrapping such as below (for actually too-long lines).

with (
    open("c", "w") as a,
    open("d", "w") as b,
):

Environment (please complete the following information):

  • Version: 20.8b1 and 71117e7
  • OS and Python version: Locally with Linux and Python 3.9.1, also whatever black.now.sh is running right now

Does this bug also happen on master? To answer this, you have two options:

yes

Additional context Add any other context about the problem here.

Perhaps I should enter two bugs. One for the parsing failure and one for the lack of willingness to add parentheses and reformat. Or perhaps they are sufficiently intertwined... let me know if I need to file a second issue.

@altendky altendky added the T: bug Something isn't working label Jan 29, 2021
@JelleZijlstra
Copy link
Collaborator

This is new syntax in 3.9, right? We'll just have to update our parser.

@altendky
Copy link
Author

I didn't realize it was new, but I did just confirm that you are correct. I really thought I had, at some point, done some multi-line with with parentheses etc. And I rarely run exclusively 3.9. Oh well...

@ichard26
Copy link
Collaborator

ichard26 commented Jan 30, 2021

This might be harder than it appears at first glance because IIRC this syntax is only allowed by the new PEG parser available in CPython 3.9 and higher... and our parser is LL(1). IIRC we can allow this syntax while continuing to use our LL(1) parser by hacking the tokenizer, but I'm not sure of any this. I should really check and play around, but I wanted to provide a possible explanation to why this support hasn't landed in Black yet.

Also while slightly off-topic, I dislike that we advertise that Black handles with statements in a sane manner in the documentation, yet none of the work needed for any of it has seen the light of day. Doing documentation-driven development is good, but I think we're sending the wrong signal if the actual development ends up never finished. But, just my opinion, shrugs.

@JelleZijlstra
Copy link
Collaborator

I agree about the documentation; we should make the documentation reflect what Black actually does. We can also mention what we'd like Black to do, but the primary focus should be on what Black actually does.

@NeilGirdhar
Copy link

NeilGirdhar commented Mar 12, 2021

Just came here to file an issue regarding the documentation:

However there is one exception: with statements using multiple context managers. Python’s grammar does not allow organizing parentheses around the series of context managers.

This is no longer true, and Black should break up with statements using parentheses now.

@ichard26
Copy link
Collaborator

ichard26 commented Mar 12, 2021

@NeilGirdhar, good to know (I'm rewriting the docs right now), but note that Black will only be able to use parentheses for with statements when targeting Python 3.9 and higher and since Black won't be defaulting to --target-version py39 for the foreseeable future (well unless Python 3.9+ syntax features are detected in which Black will adjust the target version automatically), the old (garbage) style will be used.

@ichard26
Copy link
Collaborator

With the merge of #2586 Black can now parse parenthesized with statements 🎉 Although we are still undecided with how we want to handle both the detection and rewriting of with statements to use this newer prettier syntax. Discussion on Python Discord quoted below:

[Jelle — Today at 9:24 PM]
parenthesized with is a bit problematic as a detection feature because it exists undocumented in 3.9
so I don't know if we should really emit if the target version is 3.9
and conversely, maybe we shouldn't go into 3.10 mode if we see it

[daylily richard — Today at 9:29 PM]
Hmm yeah I don't mind, I just felt like we would eventually have to support it as it does look way cleaner so might as well add in the detection code now.

[daylily richard — Today at 9:31 PM] (replying to Jelle's first message)
Yeah it's probably not the best-est of ideas as it would then enable the experimental pattern matching grammar

[daylily richard — Today at 9:32 PM]
I guess the main problem with emitting parenthesized withs starting from 3.9 would be that the code wouldn't be safe for other Python implementations as it was technically an implementation detail in that feature version. Otherwise I'd say ignoring the undocumented nature of it is probably fine.
We could keep the detection code and use it as a marker of 3.9+ but then don't emit parenthesized withs until -t 3.10 but that's rather hacky and I don't even know if we pass enough state for that to work.

[Jelle — Today at 9:37 PM]
yes. I guess that's mostly a theoretical problem since pypy is still working on 3.8
though I suspect they'd aim for bug-for-bug compatibility with CPython and support parenthesized with

[daylily richard — Today at 9:38 PM]
They in general do aim for bug-for-bug compatibility but there's more implementations of Python than just PyPy and CPython at this point

The gist of it is that we shouldn't detect parenthesized withs as a marker of 3.9+ code (and automagically set target-version) since it's technically an implementation detail of CPython until 3.10. Unfortunately we can't really use it as marker of 3.10+ code because it'd activate the currently experimental pattern matching support (i.e. special grammar). FWIW I mentioned one potential solution above. Another potential solution is to not detect parenthesized withs at all and use --target-version as the safeguard to enable rewriting to parenthesized withs (kinda like with pattern matching).

Anyway this discussion happened after I wrote up some code to detect parenthesized with so I'll just leave this here for when we make a decision: parenthesized-with-detection-support-needs-tests.patch.txt (it still needs tests).

@ichard26 ichard26 added help wanted Extra attention is needed S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) labels Dec 27, 2021
@JelleZijlstra
Copy link
Collaborator

This works now on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: parser How we parse code. Or fail to parse it. help wanted Extra attention is needed S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants