From: Łukasz Langa Date: Sun, 25 Apr 2021 18:15:54 +0000 (+0200) Subject: Work around stability errors due to optional trailing commas (#2126) X-Git-Url: https://git.madduck.net/etc/vim.git/commitdiff_plain/8672af35f052a636545e38110f0419ea92aeca0f?ds=inline 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 2999abf..0ca0b84 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,10 @@ #### _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) diff --git a/src/black/__init__.py b/src/black/__init__.py index 0a893aa..08267d5 100644 --- 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) - 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 @@ -6484,7 +6494,7 @@ def _stringify_ast( 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) @@ -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( - 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)) @@ -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" - " 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 diff --git a/tests/test_black.py b/tests/test_black.py index 201edfa..8733b7a 100644 --- 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) + @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")