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

Fix internal magic commas not being force expanded #3377

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

KindaOK
Copy link

@KindaOK KindaOK commented Nov 9, 2022

Description

Fix for issue #3268, #2052
As per the Black spec

The magic trailing comma

...
However, there are cases where you put a short collection or function call in your code but you anticipate it will grow in the future.
...
Now, you can communicate that you don’t want that by putting a trailing comma in the collection yourself. When you do, Black will know to always explode your collection into one item per line.

This means the current formatting output (the code below)

assert foo(1, 2, 3,)[
    0
] == {'bar': 'baz'}

should instead be

assert foo(
    1,
    2,
    3,
)[0] == {'bar': 'baz'}

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

Copied the examples from the bug report and added an additional test case with the failing behavior multiple times in the same expression.
@KindaOK
Copy link
Author

KindaOK commented Nov 9, 2022

Can someone verify that the test output is correct? I checked some simplified versions of the test and used that was my reference for what the output should be.

Black always uses double quotes. Test would also fail on this, but that's not the reason the test is failing to begin with.
@JelleZijlstra
Copy link
Collaborator

Does this overlap with #3370?

@yilei
Copy link
Contributor

yilei commented Nov 9, 2022

#3370 doesn't fix this issue, though some of the changes should make this easier. It probably has more overlap with #2052.

@KindaOK
Copy link
Author

KindaOK commented Nov 9, 2022

#2052 does seem to be the same problem, making #3268 a duplicate of it.

@KindaOK
Copy link
Author

KindaOK commented Nov 11, 2022

So I did some code coverage comparison and lines 207 and 208 in lines.py seem to be behind the problem.

black/src/black/lines.py

Lines 195 to 210 in 8429ebc

def contains_uncollapsable_type_comments(self) -> bool:
ignored_ids = set()
try:
last_leaf = self.leaves[-1]
ignored_ids.add(id(last_leaf))
if last_leaf.type == token.COMMA or (
last_leaf.type == token.RPAR and not last_leaf.value
):
# When trailing commas or optional parens are inserted by Black for
# consistency, comments after the previous last element are not moved
# (they don't have to, rendering will still be correct). So we ignore
# trailing commas and invisible.
last_leaf = self.leaves[-2]
ignored_ids.add(id(last_leaf))
except IndexError:
return False

I ran two test minimal test cases with code coverage, shown below.

# failing
[1,][
    2
](3)
# passing
[
    1,
][2]()

The failing test did not run lines 207 and 208, while the passing test did. The issue seems to be that parentheses pairs that are empty do not cause the last leaf to be shifted back and for them to be marked as ignored. (mixed up the tests)

Some other test cases which more clearly show the issue are below.

# passing
[
    1,
]()()()()

# passing regardless of which parentheses pair the 2 is in as long as there is only one
[
    1,
](2)()()()

# failing if two or more of the entries are filled
[1,](
    2
)(2)()()

@KindaOK
Copy link
Author

KindaOK commented Nov 11, 2022

Some other missed lines in the buggy test case are 116 and 216 in black/brackets.py and 1162-1163 and 1227-1228 in black/linegen.py. It's very interesting how the buggy test case has reduced coverage despite having a single extra character.

@KindaOK
Copy link
Author

KindaOK commented Nov 11, 2022

So a clear point where things go wrong is in black/brackets.py:116. In the mark function for the buggy test case, , never has a depth of 0, which means the comma is never marked as a delimiter with COMMA_PRIORITY.

Correct output was missing a parentheses group in the original.
Fix `omit` generator exiting early after seeing two consecutive non-empty bracket pairs before seeing a magic comma. This does cause a regression with `trailing_comma_optional_parens2` and it still does not fix lines with multiple magic commas.
@KindaOK
Copy link
Author

KindaOK commented Nov 16, 2022

So the problem that I'm finding is that generate_trailers_to_omit in linegen.py is breaking early after having 2 non-empty pairs of brackets. This is why the test cases in #3377 (comment) fail.

# passing
[
    1,
]()()()()

# passing regardless of which parentheses pair the 2 is in as long as there is only one
[
    1,
](2)()()()

# failing if two or more of the entries are filled
[1,](
    2
)(2)()()

I moved the yield to the end of the loop, but this does cause a regression with trailing_comma_optional_parens2. It also does not completely fix multiple internal magic commas. Some sample failing cases are below.

# input
[1,](3,)

# expected output
[
    1,
](
    3,
)

# actual output
[1,](
    3,
)

@KindaOK
Copy link
Author

KindaOK commented Nov 16, 2022

So in general, in the case of several magic commas on the same line, unless there is not enough room on the line, only the rightmost one will be forcefully split. Is this intended behavior? The documentation suggests that Black will always expand the collection to fit, but this is clearly not the case.

@KindaOK KindaOK marked this pull request as ready for review November 17, 2022 22:46
@github-actions
Copy link

diff-shades results comparing this PR (48042ee) to main (2793249). The full diff is available in the logs under the "Generate HTML diff report" step.

╭──────────────────────── Summary ─────────────────────────╮
│ 10 projects & 44 files changed / 385 changes [+203/-182] │
│                                                          │
│ ... out of 2 333 569 lines, 10 748 files & 23 projects   │
╰──────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

@JelleZijlstra
Copy link
Collaborator

I looked at a few of the diff-shades changes and I'm unfortunately not a fan; they mostly seem like gratuitous changes that don't make the formatting better. Can this fix be limited to only cases where a magic trailing comma is present?

@KindaOK
Copy link
Author

KindaOK commented Dec 15, 2022

I'm not sure what changes to make at this point. One problem has to do with sibling-level magic trailing commas. In right_hand_split, with something like [1,][3,], it comes out with head = "[1,][", body="3,", and tail="]". I think is is going to require a more significant rewrite to handle that possibility, and I'm still not familiar enough with the library to be comfortable doing that.

Another problem is that the "fix" that I made changes how long lines are handled by always returning all trailers found by generate_trailers_to_omit, which is why there are regressions like below. The function seems to currently assume that any trailing magic commas will be found very close to the end. The placement of the yield omit that I used had the least amount of regressions in the unit tests, but that isn't good enough.

     def _is_venv_activated(self) -> bool:
-        return bool(environ.get("POETRY_ACTIVE")) or getattr(
-            sys, "real_prefix", sys.prefix
-        ) == str(self.env.path)
+        return bool(
+            environ.get("POETRY_ACTIVE")
+        ) or getattr(sys, "real_prefix", sys.prefix) == str(self.env.path)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants