From a24e1f795975350f7b1d8898d831916a9f6dbc6a Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Fri, 28 Jan 2022 18:13:18 -0800 Subject: [PATCH 1/1] 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 | 1 + src/black/__init__.py | 6 +-- src/black/linegen.py | 2 +- src/black/lines.py | 14 +---- src/black/nodes.py | 23 -------- tests/data/function_trailing_comma.py | 23 ++++---- tests/data/long_strings_flag_disabled.py | 13 +++-- tests/data/torture.py | 25 ++++----- tests/data/trailing_comma_optional_parens1.py | 54 ++++++++++--------- tests/data/trailing_comma_optional_parens2.py | 15 ++---- tests/data/trailing_comma_optional_parens3.py | 5 +- 11 files changed, 72 insertions(+), 109 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b57a360..6775cee 100644 --- a/CHANGES.md +++ b/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 diff --git a/src/black/__init__.py b/src/black/__init__.py index 769e693..6192f5c 100644 --- a/src/black/__init__.py +++ b/src/black/__init__.py @@ -1332,7 +1332,7 @@ def get_future_imports(node: Node) -> Set[str]: 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 diff --git a/src/black/linegen.py b/src/black/linegen.py index 495d323..4dc242a 100644 --- a/src/black/linegen.py +++ b/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: diff --git a/src/black/lines.py b/src/black/lines.py index 1c4e38a..f35665c 100644 --- a/src/black/lines.py +++ b/src/black/lines.py @@ -3,7 +3,6 @@ import itertools import sys from typing import ( Callable, - Collection, Dict, Iterator, List, @@ -22,7 +21,7 @@ from black.mode import Mode 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 diff --git a/src/black/nodes.py b/src/black/nodes.py index 7466670..f130bff 100644 --- a/src/black/nodes.py +++ b/src/black/nodes.py @@ -4,13 +4,11 @@ blib2to3 Node/Leaf transformation-related utility functions. 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: diff --git a/tests/data/function_trailing_comma.py b/tests/data/function_trailing_comma.py index 0207821..429eb0e 100644 --- a/tests/data/function_trailing_comma.py +++ b/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 diff --git a/tests/data/long_strings_flag_disabled.py b/tests/data/long_strings_flag_disabled.py index ef3094f..db3954e 100644 --- a/tests/data/long_strings_flag_disabled.py +++ b/tests/data/long_strings_flag_disabled.py @@ -133,11 +133,14 @@ old_fmt_string2 = "This is a %s %s %s %s" % ( "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." diff --git a/tests/data/torture.py b/tests/data/torture.py index 79a44c2..7cabd4c 100644 --- a/tests/data/torture.py +++ b/tests/data/torture.py @@ -31,20 +31,17 @@ importA ** 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 diff --git a/tests/data/trailing_comma_optional_parens1.py b/tests/data/trailing_comma_optional_parens1.py index f9f4ae5..85aa8ba 100644 --- a/tests/data/trailing_comma_optional_parens1.py +++ b/tests/data/trailing_comma_optional_parens1.py @@ -2,6 +2,10 @@ 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): @@ -23,39 +27,37 @@ class A: # 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 diff --git a/tests/data/trailing_comma_optional_parens2.py b/tests/data/trailing_comma_optional_parens2.py index 1dfb54c..9541670 100644 --- a/tests/data/trailing_comma_optional_parens2.py +++ b/tests/data/trailing_comma_optional_parens2.py @@ -4,14 +4,9 @@ if (e123456.get_tk_patchlevel() >= (8, 6, 0, 'final') or # 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 diff --git a/tests/data/trailing_comma_optional_parens3.py b/tests/data/trailing_comma_optional_parens3.py index bccf474..c0ed699 100644 --- a/tests/data/trailing_comma_optional_parens3.py +++ b/tests/data/trailing_comma_optional_parens3.py @@ -18,7 +18,4 @@ if True: "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweas " + "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwegqweasdzxcqweasdzxc.", "qweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqweasdzxcqwe", - ) % { - "reported_username": reported_username, - "report_reason": report_reason, - } \ No newline at end of file + ) % {"reported_username": reported_username, "report_reason": report_reason} -- 2.39.5