Fix instability due to trailing comma logic (#2572)

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.

Some things format differently (with extra parens)

CHANGES.md

@@ -139,6 +139,7 @@ and the first release covered by our new stability policy.
   when `--target-version py310` is explicitly specified (#2586)
 - Add support for parenthesized with (#2586)
 - Declare support for Python 3.10 for running Black (#2562)
+- Fix unstable black runs around magic trailing comma (#2572)
 
 ### Integrations
 

src/black/__init__.py

@@ -1332,7 +1332,7 @@ def get_imports_from_children(children: List[LN]) -> Generator[str, None, None]:
     return imports
 
 
-def assert_equivalent(src: str, dst: str, *, pass_num: int = 1) -> None:
+def assert_equivalent(src: str, dst: str) -> None:
     """Raise AssertionError if `src` and `dst` aren't equivalent."""
     try:
         src_ast = parse_ast(src)
@@ -1349,7 +1349,7 @@ def assert_equivalent(src: str, dst: str, *, pass_num: int = 1) -> None:
     except Exception as exc:
         log = dump_to_file("".join(traceback.format_tb(exc.__traceback__)), dst)
         raise AssertionError(
-            f"INTERNAL ERROR: Black produced invalid code on pass {pass_num}: {exc}. "
+            f"INTERNAL ERROR: Black produced invalid code: {exc}. "
             "Please report a bug on https://github.com/psf/black/issues.  "
             f"This invalid output might be helpful: {log}"
         ) from None
@@ -1360,7 +1360,7 @@ def assert_equivalent(src: str, dst: str, *, pass_num: int = 1) -> None:
         log = dump_to_file(diff(src_ast_str, dst_ast_str, "src", "dst"))
         raise AssertionError(
             "INTERNAL ERROR: Black produced code that is not equivalent to the"
-            f" source on pass {pass_num}.  Please report a bug on "
+            f" source.  Please report a bug on "
             f"https://github.com/psf/black/issues.  This diff might be helpful: {log}"
         ) from None
 

src/black/linegen.py

@@ -543,7 +543,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

@@ -3,7 +3,6 @@
 import sys
 from typing import (
     Callable,
-    Collection,
     Dict,
     Iterator,
     List,
@@ -22,7 +21,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
@@ -645,7 +644,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?
 
@@ -683,12 +681,6 @@ def can_omit_invisible_parens(
 
     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
@@ -710,10 +702,6 @@ def can_omit_invisible_parens(
             # 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
-
         if _can_omit_closing_paren(line, last=last, line_length=line_length):
             return True
 

src/black/nodes.py

@@ -4,13 +4,11 @@
 
 import sys
 from typing import (
-    Collection,
     Generic,
     Iterator,
     List,
     Optional,
     Set,
-    Tuple,
     TypeVar,
     Union,
 )
@@ -439,27 +437,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: Optional[Leaf] = None
-    last: Optional[Leaf] = 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/torture.py

@@ -31,20 +31,17 @@ def test(self, othr):
     ** 101234234242352525425252352352525234890264906820496920680926538059059209922523523525
 )  #
 
-assert (
-    sort_by_dependency(
-        {
-            "1": {"2", "3"},
-            "2": {"2a", "2b"},
-            "3": {"3a", "3b"},
-            "2a": set(),
-            "2b": set(),
-            "3a": set(),
-            "3b": set(),
-        }
-    )
-    == ["2a", "2b", "2", "3a", "3b", "3", "1"]
-)
+assert sort_by_dependency(
+    {
+        "1": {"2", "3"},
+        "2": {"2a", "2b"},
+        "3": {"3a", "3b"},
+        "2a": set(),
+        "2b": set(),
+        "3a": set(),
+        "3b": set(),
+    }
+) == ["2a", "2b", "2", "3a", "3b", "3", "1"]
 
 importA
 0

tests/data/trailing_comma_optional_parens1.py

@@ -2,6 +2,10 @@
                         _winapi.ERROR_PIPE_BUSY) or _check_timeout(t):
     pass
 
+if x:
+    if y:
+        new_id = max(Vegetable.objects.order_by('-id')[0].id,
+                     Mineral.objects.order_by('-id')[0].id) + 1
 
 class X:
     def get_help_text(self):
@@ -23,39 +27,37 @@ def b(self):
 
 # output
 
-if (
-    e1234123412341234.winerror
-    not in (
-        _winapi.ERROR_SEM_TIMEOUT,
-        _winapi.ERROR_PIPE_BUSY,
-    )
-    or _check_timeout(t)
-):
+if e1234123412341234.winerror not in (
+    _winapi.ERROR_SEM_TIMEOUT,
+    _winapi.ERROR_PIPE_BUSY,
+) or _check_timeout(t):
     pass
 
+if x:
+    if y:
+        new_id = (
+            max(
+                Vegetable.objects.order_by("-id")[0].id,
+                Mineral.objects.order_by("-id")[0].id,
+            )
+            + 1
+        )
+
 
 class X:
     def get_help_text(self):
-        return (
-            ngettext(
-                "Your password must contain at least %(min_length)d character.",
-                "Your password must contain at least %(min_length)d characters.",
-                self.min_length,
-            )
-            % {"min_length": self.min_length}
-        )
+        return ngettext(
+            "Your password must contain at least %(min_length)d character.",
+            "Your password must contain at least %(min_length)d characters.",
+            self.min_length,
+        ) % {"min_length": self.min_length}
 
 
 class A:
     def b(self):
-        if (
-            self.connection.mysql_is_mariadb
-            and (
-                10,
-                4,
-                3,
-            )
-            < self.connection.mysql_version
-            < (10, 5, 2)
-        ):
+        if self.connection.mysql_is_mariadb and (
+            10,
+            4,
+            3,
+        ) < self.connection.mysql_version < (10, 5, 2):
             pass

tests/data/trailing_comma_optional_parens2.py

@@ -4,14 +4,9 @@
 
 # output
 
-if (
-    e123456.get_tk_patchlevel() >= (8, 6, 0, "final")
-    or (
-        8,
-        5,
-        8,
-    )
-    <= get_tk_patchlevel()
-    < (8, 6)
-):
+if e123456.get_tk_patchlevel() >= (8, 6, 0, "final") or (
+    8,
+    5,
+    8,
+) <= get_tk_patchlevel() < (8, 6):
     pass

tests/data/trailing_comma_optional_parens3.py

@@ -18,7 +18,4 @@
                 "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweas "
                 + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwegqweasdzxcqweasdzxc.",
                 "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwe",
-            ) % {
-                "reported_username": reported_username,
-                "report_reason": report_reason,
-            }
+            ) % {"reported_username": reported_username, "report_reason": report_reason}