Work around stability errors due to optional trailing commas (#2126)

Optional trailing commas put by Black become magic trailing commas on another
pass of the tool.  Since they are influencing formatting around optional
parentheses, on rare occasions the tool changes its mind in terms of putting
parentheses or not.

Ideally this would never be the case but sadly the decision to put optional
parentheses or not (which looks at pre-existing "magic" trailing commas) is
happening around the same time as the decision to put an optional trailing
comma.  Untangling the two proved to be impractically difficult.

This shameful workaround uses the fact that the formatting instability
introduced by magic trailing commas is deterministic: if the optional trailing
comma becoming a pre-existing "magic" trailing comma changes formatting, the
second pass becomes stable since there is no variable factor anymore on pass 3,
4, and so on.

For most files, this will introduce no performance penalty since `--safe` is
already re-formatting everything twice to ensure formatting stability.  We're
using this result and if all's good, the behavior is equivalent.  If there is
a difference, we treat the second result as the binding one, and check its
sanity again.

diff --git a/CHANGES.md b/CHANGES.md
index 2999abfff4ebf08417cb67abde609910640bb366..0ca0b84f6d1b15366e3796f7ef38fff08148dd1e 100644 (file)
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -4,6 +4,10 @@
 
 #### _Black_
 
 
 #### _Black_
 

+- Fixed a rare but annoying formatting instability created by the combination of
+  optional trailing commas inserted by `Black` and optional parentheses looking at
+  pre-existing "magic" trailing commas (#2126)
+

 - `Black` now processes one-line docstrings by stripping leading and trailing spaces,
   and adding a padding space when needed to break up """". (#1740)
 
 - `Black` now processes one-line docstrings by stripping leading and trailing spaces,
   and adding a padding space when needed to break up """". (#1740)
 

diff --git a/src/black/__init__.py b/src/black/__init__.py
index 0a893aa80feba991670b3b51b726e7b5b194b833..08267d50f89c3b50dec0e08d62e0cd9cc28ecf0e 100644 (file)
--- a/src/black/__init__.py
+++ b/src/black/__init__.py
@@ -1016,7 +1016,17 @@ def format_file_contents(src_contents: str, *, fast: bool, mode: Mode) -> FileCo
 
     if not fast:
         assert_equivalent(src_contents, dst_contents)
 
     if not fast:
         assert_equivalent(src_contents, dst_contents)

-        assert_stable(src_contents, dst_contents, mode=mode)
+
+        # Forced second pass to work around optional trailing commas (becoming
+        # forced trailing commas on pass 2) interacting differently with optional
+        # parentheses.  Admittedly ugly.
+        dst_contents_pass2 = format_str(dst_contents, mode=mode)
+        if dst_contents != dst_contents_pass2:
+            dst_contents = dst_contents_pass2
+            assert_equivalent(src_contents, dst_contents, pass_num=2)
+            assert_stable(src_contents, dst_contents, mode=mode)
+        # Note: no need to explicitly call `assert_stable` if `dst_contents` was
+        # the same as `dst_contents_pass2`.

     return dst_contents
 
 
     return dst_contents
 
 


@@ -6484,7 +6494,7 @@ def _stringify_ast(
     yield f"{'  ' * depth})  # /{node.__class__.__name__}"
 
 
     yield f"{'  ' * depth})  # /{node.__class__.__name__}"
 
 

-def assert_equivalent(src: str, dst: str) -> None:
+def assert_equivalent(src: str, dst: str, *, pass_num: int = 1) -> None:

     """Raise AssertionError if `src` and `dst` aren't equivalent."""
     try:
         src_ast = parse_ast(src)
     """Raise AssertionError if `src` and `dst` aren't equivalent."""
     try:
         src_ast = parse_ast(src)


@@ -6499,9 +6509,9 @@ def assert_equivalent(src: str, dst: str) -> None:
     except Exception as exc:
         log = dump_to_file("".join(traceback.format_tb(exc.__traceback__)), dst)
         raise AssertionError(
     except Exception as exc:
         log = dump_to_file("".join(traceback.format_tb(exc.__traceback__)), dst)
         raise AssertionError(

-            f"INTERNAL ERROR: Black produced invalid code: {exc}. Please report a bug"
-            " on https://github.com/psf/black/issues.  This invalid output might be"
-            f" helpful: {log}"
+            f"INTERNAL ERROR: Black produced invalid code on pass {pass_num}: {exc}. "
+            "Please report a bug on https://github.com/psf/black/issues.  "
+            f"This invalid output might be helpful: {log}"

         ) from None
 
     src_ast_str = "\n".join(_stringify_ast(src_ast))
         ) from None
 
     src_ast_str = "\n".join(_stringify_ast(src_ast))


@@ -6510,8 +6520,8 @@ def assert_equivalent(src: str, dst: str) -> 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"
         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"

-            " source.  Please report a bug on https://github.com/psf/black/issues. "
-            f" This diff might be helpful: {log}"
+            f" source on pass {pass_num}.  Please report a bug on "
+            f"https://github.com/psf/black/issues.  This diff might be helpful: {log}"

         ) from None
 
 
         ) from None
 
 

diff --git a/tests/test_black.py b/tests/test_black.py
index 201edfa2de522b17b84a153333a969100c933e87..8733b7a63ad092a4f0f16a17283b33e9ccebb8c2 100644 (file)
--- a/tests/test_black.py
+++ b/tests/test_black.py
@@ -254,6 +254,24 @@ class BlackTestCase(BlackBaseTestCase):
         actual = fs(source)
         black.assert_stable(source, actual, DEFAULT_MODE)
 
         actual = fs(source)
         black.assert_stable(source, actual, DEFAULT_MODE)
 

+    @patch("black.dump_to_file", dump_to_stderr)
+    def test_trailing_comma_optional_parens_stability1_pass2(self) -> None:
+        source, _expected = read_data("trailing_comma_optional_parens1")
+        actual = fs(fs(source))  # this is what `format_file_contents` does with --safe
+        black.assert_stable(source, actual, DEFAULT_MODE)
+
+    @patch("black.dump_to_file", dump_to_stderr)
+    def test_trailing_comma_optional_parens_stability2_pass2(self) -> None:
+        source, _expected = read_data("trailing_comma_optional_parens2")
+        actual = fs(fs(source))  # this is what `format_file_contents` does with --safe
+        black.assert_stable(source, actual, DEFAULT_MODE)
+
+    @patch("black.dump_to_file", dump_to_stderr)
+    def test_trailing_comma_optional_parens_stability3_pass2(self) -> None:
+        source, _expected = read_data("trailing_comma_optional_parens3")
+        actual = fs(fs(source))  # this is what `format_file_contents` does with --safe
+        black.assert_stable(source, actual, DEFAULT_MODE)
+

     @patch("black.dump_to_file", dump_to_stderr)
     def test_pep_572(self) -> None:
         source, expected = read_data("pep_572")
     @patch("black.dump_to_file", dump_to_stderr)
     def test_pep_572(self) -> None:
         source, expected = read_data("pep_572")