Remove magic comma logic from can_omit_invisible_parens

It was causing stability issues because the first pass
could cause a "magic trailing comma" to appear, meaning
that the second pass might get a different result. It's
not critical.

black-primer previously crashed on several projects due to instability, but now passes.
The preexisting disabled tests can now be enabled. Added a new
test for a case that came up in my testing.

Some things format differently (with extra parens)

CHANGES.md

@@ -8,6 +8,7 @@
 - Add new `--workers` parameter (#2514)
 - Fixed feature detection for positional-only arguments in lambdas (#2532)
 - Bumped typed-ast version minimum to 1.4.3 for 3.10 compatiblity (#2519)
+- Fix unstable black runs around magic trailing comma (#2572)
 - Add primer support for --projects (#2555)
 
 ### _Blackd_

src/black/linegen.py

@@ -496,7 +496,7 @@ def right_hand_split(
         # there are no standalone comments in the body
         and not body.contains_standalone_comments(0)
         # and we can actually remove the parens
-        and can_omit_invisible_parens(body, line_length, omit_on_explode=omit)
+        and can_omit_invisible_parens(body, line_length)
     ):
         omit = {id(closing_bracket), *omit}
         try:

src/black/lines.py

@@ -22,7 +22,7 @@
 from black.nodes import STANDALONE_COMMENT, TEST_DESCENDANTS
 from black.nodes import BRACKETS, OPENING_BRACKETS, CLOSING_BRACKETS
 from black.nodes import syms, whitespace, replace_child, child_towards
-from black.nodes import is_multiline_string, is_import, is_type_comment, last_two_except
+from black.nodes import is_multiline_string, is_import, is_type_comment
 from black.nodes import is_one_tuple_between
 
 # types
@@ -609,7 +609,6 @@ def can_be_split(line: Line) -> bool:
 def can_omit_invisible_parens(
     line: Line,
     line_length: int,
-    omit_on_explode: Collection[LeafID] = (),
 ) -> bool:
     """Does `line` have a shape safe to reformat without optional parens around it?
 
@@ -645,14 +644,7 @@ def can_omit_invisible_parens(
         # a leading opening bracket and a trailing closing bracket.  If the
         # opening bracket doesn't match our rule, maybe the closing will.
 
-    penultimate = line.leaves[-2]
     last = line.leaves[-1]
-    if line.magic_trailing_comma:
-        try:
-            penultimate, last = last_two_except(line.leaves, omit=omit_on_explode)
-        except LookupError:
-            # Turns out we'd omit everything.  We cannot skip the optional parentheses.
-            return False
 
     if (
         last.type == token.RPAR
@@ -665,18 +657,14 @@ def can_omit_invisible_parens(
             and last.parent.type != syms.trailer
         )
     ):
-        if penultimate.type in OPENING_BRACKETS:
-            # Empty brackets don't help.
-            return False
-
         if is_multiline_string(first):
             # Additional wrapping of a multiline string in this situation is
             # unnecessary.
             return True
 
-        if line.magic_trailing_comma and penultimate.type == token.COMMA:
-            # The rightmost non-omitted bracket pair is the one we want to explode on.
-            return True
+        elif line.leaves[-2].type in OPENING_BRACKETS:
+            # Empty brackets don't help.
+            return False
 
         if _can_omit_closing_paren(line, last=last, line_length=line_length):
             return True

src/black/nodes.py

@@ -439,27 +439,6 @@ def prev_siblings_are(node: Optional[LN], tokens: List[Optional[NodeType]]) -> b
     return prev_siblings_are(node.prev_sibling, tokens[:-1])
 
 
-def last_two_except(leaves: List[Leaf], omit: Collection[LeafID]) -> Tuple[Leaf, Leaf]:
-    """Return (penultimate, last) leaves skipping brackets in `omit` and contents."""
-    stop_after = None
-    last = None
-    for leaf in reversed(leaves):
-        if stop_after:
-            if leaf is stop_after:
-                stop_after = None
-            continue
-
-        if last:
-            return leaf, last
-
-        if id(leaf) in omit:
-            stop_after = leaf.opening_bracket
-        else:
-            last = leaf
-    else:
-        raise LookupError("Last two leaves were also skipped")
-
-
 def parent_type(node: Optional[LN]) -> Optional[NodeType]:
     """
     Returns:

tests/data/function_trailing_comma.py

@@ -89,16 +89,19 @@ def f(
         "a": 1,
         "b": 2,
     }["a"]
-    if a == {
-        "a": 1,
-        "b": 2,
-        "c": 3,
-        "d": 4,
-        "e": 5,
-        "f": 6,
-        "g": 7,
-        "h": 8,
-    }["a"]:
+    if (
+        a
+        == {
+            "a": 1,
+            "b": 2,
+            "c": 3,
+            "d": 4,
+            "e": 5,
+            "f": 6,
+            "g": 7,
+            "h": 8,
+        }["a"]
+    ):
         pass
 
 

tests/data/long_strings_flag_disabled.py

@@ -133,11 +133,14 @@
     "Use f-strings instead!",
 )
 
-old_fmt_string3 = "Whereas only the strings after the percent sign were long in the last example, this example uses a long initial string as well. This is another %s %s %s %s" % (
-    "really really really really really",
-    "old",
-    "way to format strings!",
-    "Use f-strings instead!",
+old_fmt_string3 = (
+    "Whereas only the strings after the percent sign were long in the last example, this example uses a long initial string as well. This is another %s %s %s %s"
+    % (
+        "really really really really really",
+        "old",
+        "way to format strings!",
+        "Use f-strings instead!",
+    )
 )
 
 fstring = f"f-strings definitely make things more {difficult} than they need to be for {{black}}. But boy they sure are handy. The problem is that some lines will need to have the 'f' whereas others do not. This {line}, for example, needs one."

tests/data/trailing_comma_optional_parens1.py

@@ -1,3 +1,8 @@
 if e1234123412341234.winerror not in (_winapi.ERROR_SEM_TIMEOUT,
                         _winapi.ERROR_PIPE_BUSY) or _check_timeout(t):
-    pass
+    pass
+
+if x:
+    if y:
+        new_id = max(Vegetable.objects.order_by('-id')[0].id,
+                     Mineral.objects.order_by('-id')[0].id) + 1

tests/test_black.py

@@ -220,21 +220,18 @@ def _test_wip(self) -> None:
         black.assert_equivalent(source, actual)
         black.assert_stable(source, actual, black.FileMode())
 
-    @unittest.expectedFailure
     @patch("black.dump_to_file", dump_to_stderr)
-    def test_trailing_comma_optional_parens_stability1(self) -> None:
+    def test_trailing_comma_optional_parens_stability11(self) -> None:
         source, _expected = read_data("trailing_comma_optional_parens1")
         actual = fs(source)
         black.assert_stable(source, actual, DEFAULT_MODE)
 
-    @unittest.expectedFailure
     @patch("black.dump_to_file", dump_to_stderr)
     def test_trailing_comma_optional_parens_stability2(self) -> None:
         source, _expected = read_data("trailing_comma_optional_parens2")
         actual = fs(source)
         black.assert_stable(source, actual, DEFAULT_MODE)
 
-    @unittest.expectedFailure
     @patch("black.dump_to_file", dump_to_stderr)
     def test_trailing_comma_optional_parens_stability3(self) -> None:
         source, _expected = read_data("trailing_comma_optional_parens3")