Stability fixup: address stability issues relating to interaction between magic commas and invisible parentheses

CHANGES.md

@@ -38,6 +38,8 @@
 - Added `--stdin-filename` argument to allow stdin to respect `--force-exclude` rules
   (#1780)
 
+- Reduce second-pass formatting stability errors (#1629)
+
 - Lines ending with `fmt: skip` will now be not formatted (#1800)
 
 - PR #2053: Black no longer relies on typed-ast for Python 3.8 and higher

src/black/__init__.py

@@ -216,7 +216,6 @@ class Feature(Enum):
     ASSIGNMENT_EXPRESSIONS = 8
     POS_ONLY_ARGUMENTS = 9
     RELAXED_DECORATORS = 10
-    FORCE_OPTIONAL_PARENTHESES = 50
 
 
 VERSION_TO_FEATURES: Dict[TargetVersion, Set[Feature]] = {
@@ -1359,7 +1358,6 @@ class BracketTracker:
     previous: Optional[Leaf] = None
     _for_loop_depths: List[int] = field(default_factory=list)
     _lambda_argument_depths: List[int] = field(default_factory=list)
-    invisible: List[Leaf] = field(default_factory=list)
 
     def mark(self, leaf: Leaf) -> None:
         """Mark `leaf` with bracket-related metadata. Keep track of delimiters.
@@ -1391,8 +1389,6 @@ def mark(self, leaf: Leaf) -> None:
                     f" bracket: {leaf}"
                 ) from e
             leaf.opening_bracket = opening_bracket
-            if not leaf.value:
-                self.invisible.append(leaf)
         leaf.bracket_depth = self.depth
         if self.depth == 0:
             delim = is_split_before_delimiter(leaf, self.previous)
@@ -1405,8 +1401,6 @@ def mark(self, leaf: Leaf) -> None:
         if leaf.type in OPENING_BRACKETS:
             self.bracket_match[self.depth, BRACKET[leaf.type]] = leaf
             self.depth += 1
-            if not leaf.value:
-                self.invisible.append(leaf)
         self.previous = leaf
         self.maybe_increment_lambda_arguments(leaf)
         self.maybe_increment_for_loop_variable(leaf)
@@ -2810,8 +2804,17 @@ def rhs(line: Line, features: Collection[Feature]) -> Iterator[Line]:
         # We are accumulating lines in `result` because we might want to abort
         # mission and return the original line in the end, or attempt a different
         # split altogether.
+        result: List[Line] = []
         try:
-            result = run_transformer(line, transform, mode, features, line_str=line_str)
+            for transformed_line in transform(line, features):
+                if str(transformed_line).strip("\n") == line_str:
+                    raise CannotTransform(
+                        "Line transformer returned an unchanged result"
+                    )
+
+                result.extend(
+                    transform_line(transformed_line, mode=mode, features=features)
+                )
         except CannotTransform:
             continue
         else:
@@ -2852,7 +2855,6 @@ class StringTransformer(ABC):
 
     line_length: int
     normalize_strings: bool
-    __name__ = "StringTransformer"
 
     @abstractmethod
     def do_match(self, line: Line) -> TMatchResult:
@@ -4199,9 +4201,10 @@ def _return_match(LL: List[Leaf]) -> Optional[int]:
         """
         # If this line is apart of a return/yield statement and the first leaf
         # contains either the "return" or "yield" keywords...
-        if parent_type(LL[0]) in [syms.return_stmt, syms.yield_expr] and LL[
-            0
-        ].value in ["return", "yield"]:
+        if (
+            parent_type(LL[0]) in [syms.return_stmt, syms.yield_expr]
+            and LL[0].value in ["return", "yield"]
+        ):
             is_valid_index = is_valid_index_factory(LL)
 
             idx = 2 if is_valid_index(1) and is_empty_par(LL[1]) else 1
@@ -4926,9 +4929,8 @@ def right_hand_split(
     tail = bracket_split_build_line(tail_leaves, line, opening_bracket)
     bracket_split_succeeded_or_raise(head, body, tail)
     if (
-        Feature.FORCE_OPTIONAL_PARENTHESES not in features
         # the opening bracket is an optional paren
-        and opening_bracket.type == token.LPAR
+        opening_bracket.type == token.LPAR
         and not opening_bracket.value
         # the closing bracket is an optional paren
         and closing_bracket.type == token.RPAR
@@ -6008,18 +6010,6 @@ def generate_trailers_to_omit(line: Line, line_length: int) -> Iterator[Set[Leaf
             if leaf is opening_bracket:
                 opening_bracket = None
             elif leaf.type in CLOSING_BRACKETS:
-                prev = line.leaves[index - 1] if index > 0 else None
-                if (
-                    prev
-                    and prev.type == token.COMMA
-                    and not is_one_tuple_between(
-                        leaf.opening_bracket, leaf, line.leaves
-                    )
-                ):
-                    # Never omit bracket pairs with trailing commas.
-                    # We need to explode on those.
-                    break
-
                 inner_brackets.add(id(leaf))
         elif leaf.type in CLOSING_BRACKETS:
             prev = line.leaves[index - 1] if index > 0 else None
@@ -6031,20 +6021,17 @@ def generate_trailers_to_omit(line: Line, line_length: int) -> Iterator[Set[Leaf
                 continue
 
             if closing_bracket:
+                # Omit tokens to the right of brackets that contain a trailing comma
+                if closing_bracket is line.magic_trailing_comma:
+                    yield omit
+                # TODO: Understand and explain this line; are there edge cases here?
+                if line.magic_trailing_comma and inner_brackets:
+                    yield omit
                 omit.add(id(closing_bracket))
                 omit.update(inner_brackets)
                 inner_brackets.clear()
                 yield omit
 
-            if (
-                prev
-                and prev.type == token.COMMA
-                and not is_one_tuple_between(leaf.opening_bracket, leaf, line.leaves)
-            ):
-                # Never omit bracket pairs with trailing commas.
-                # We need to explode on those.
-                break
-
             if leaf.value:
                 opening_bracket = leaf.opening_bracket
                 closing_bracket = leaf
@@ -6696,13 +6683,17 @@ def can_omit_invisible_parens(
 
     penultimate = line.leaves[-2]
     last = line.leaves[-1]
-    if line.magic_trailing_comma:
+    if omit_on_explode:
         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
 
+    # TODO: Is this reasonable?
+    if last.type in COMPARATORS or last.type in MATH_OPERATORS:
+        return True
+
     if (
         last.type == token.RPAR
         or last.type == token.RBRACE
@@ -6723,10 +6714,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
 
@@ -6796,47 +6783,6 @@ def last_two_except(leaves: List[Leaf], omit: Collection[LeafID]) -> Tuple[Leaf,
         raise LookupError("Last two leaves were also skipped")
 
 
-def run_transformer(
-    line: Line,
-    transform: Transformer,
-    mode: Mode,
-    features: Collection[Feature],
-    *,
-    line_str: str = "",
-) -> List[Line]:
-    if not line_str:
-        line_str = line_to_string(line)
-    result: List[Line] = []
-    for transformed_line in transform(line, features):
-        if str(transformed_line).strip("\n") == line_str:
-            raise CannotTransform("Line transformer returned an unchanged result")
-
-        result.extend(transform_line(transformed_line, mode=mode, features=features))
-
-    if not (
-        transform.__name__ == "rhs"
-        and line.bracket_tracker.invisible
-        and not any(bracket.value for bracket in line.bracket_tracker.invisible)
-        and not line.contains_multiline_strings()
-        and not result[0].contains_uncollapsable_type_comments()
-        and not result[0].contains_unsplittable_type_ignore()
-        and not is_line_short_enough(result[0], line_length=mode.line_length)
-    ):
-        return result
-
-    line_copy = line.clone()
-    append_leaves(line_copy, line, line.leaves)
-    features_fop = set(features) | {Feature.FORCE_OPTIONAL_PARENTHESES}
-    second_opinion = run_transformer(
-        line_copy, transform, mode, features_fop, line_str=line_str
-    )
-    if all(
-        is_line_short_enough(ln, line_length=mode.line_length) for ln in second_opinion
-    ):
-        result = second_opinion
-    return result
-
-
 def get_cache_file(mode: Mode) -> Path:
     return CACHE_DIR / f"cache.{mode.get_cache_key()}.pickle"
 

src/black_primer/primer.json

@@ -17,7 +17,7 @@
     },
     "bandersnatch": {
       "cli_arguments": [],
-      "expect_formatting_changes": false,
+      "expect_formatting_changes": true,
       "git_clone_url": "https://github.com/pypa/bandersnatch.git",
       "long_checkout": false,
       "py_versions": ["all"]
@@ -40,7 +40,7 @@
     },
     "flake8-bugbear": {
       "cli_arguments": [],
-      "expect_formatting_changes": false,
+      "expect_formatting_changes": true,
       "git_clone_url": "https://github.com/PyCQA/flake8-bugbear.git",
       "long_checkout": false,
       "py_versions": ["all"]
@@ -98,7 +98,7 @@
     },
     "sqlalchemy": {
       "cli_arguments": [],
-      "expect_formatting_changes": false,
+      "expect_formatting_changes": true,
       "git_clone_url": "https://github.com/sqlalchemy/sqlalchemy.git",
       "long_checkout": false,
       "py_versions": ["all"]

tests/data/cantfit.py

@@ -67,15 +67,11 @@
 normal_name = (
     but_the_function_name_is_now_ridiculously_long_and_it_is_still_super_annoying()
 )
-normal_name = (
-    but_the_function_name_is_now_ridiculously_long_and_it_is_still_super_annoying(
-        arg1, arg2, arg3
-    )
+normal_name = but_the_function_name_is_now_ridiculously_long_and_it_is_still_super_annoying(
+    arg1, arg2, arg3
 )
-normal_name = (
-    but_the_function_name_is_now_ridiculously_long_and_it_is_still_super_annoying(
-        [1, 2, 3], arg1, [1, 2, 3], arg2, [1, 2, 3], arg3
-    )
+normal_name = but_the_function_name_is_now_ridiculously_long_and_it_is_still_super_annoying(
+    [1, 2, 3], arg1, [1, 2, 3], arg2, [1, 2, 3], arg3
 )
 # long arguments
 normal_name = normal_function_name(

tests/data/collections.py

@@ -148,10 +148,9 @@
 print("foo %r", (foo.bar,))
 
 if True:
-    IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING = (
-        Config.IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING
-        | {pylons.controllers.WSGIController}
-    )
+    IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING = Config.IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING | {
+        pylons.controllers.WSGIController
+    }
 
 if True:
     ec2client.get_waiter("instance_stopped").wait(

tests/data/composition.py

@@ -39,9 +39,12 @@ def test(self) -> None:
             # Only send the first n items.
             items=items[:num_items]
         )
-        return (
-            'Utterly failed doctest test for %s\n   File "%s", line %s, in %s\n\n%s'
-            % (test.name, test.filename, lineno, lname, err)
+        return 'Utterly failed doctest test for %s\n   File "%s", line %s, in %s\n\n%s' % (
+            test.name,
+            test.filename,
+            lineno,
+            lname,
+            err,
         )
 
     def omitting_trailers(self) -> None:
@@ -165,17 +168,14 @@ def tricky_asserts(self) -> None:
             _C.__init__.__code__.co_firstlineno + 1,
         )
 
-        assert (
-            expectedexpectedexpectedexpectedexpectedexpectedexpectedexpectedexpect
-            == {
-                key1: value1,
-                key2: value2,
-                key3: value3,
-                key4: value4,
-                key5: value5,
-                key6: value6,
-                key7: value7,
-                key8: value8,
-                key9: value9,
-            }
-        )
+        assert expectedexpectedexpectedexpectedexpectedexpectedexpectedexpectedexpect == {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        }

tests/data/composition_no_trailing_comma.py

@@ -225,9 +225,12 @@ def test(self) -> None:
             # Only send the first n items.
             items=items[:num_items]
         )
-        return (
-            'Utterly failed doctest test for %s\n   File "%s", line %s, in %s\n\n%s'
-            % (test.name, test.filename, lineno, lname, err)
+        return 'Utterly failed doctest test for %s\n   File "%s", line %s, in %s\n\n%s' % (
+            test.name,
+            test.filename,
+            lineno,
+            lname,
+            err,
         )
 
     def omitting_trailers(self) -> None:
@@ -351,17 +354,14 @@ def tricky_asserts(self) -> None:
             _C.__init__.__code__.co_firstlineno + 1,
         )
 
-        assert (
-            expectedexpectedexpectedexpectedexpectedexpectedexpectedexpectedexpect
-            == {
-                key1: value1,
-                key2: value2,
-                key3: value3,
-                key4: value4,
-                key5: value5,
-                key6: value6,
-                key7: value7,
-                key8: value8,
-                key9: value9,
-            }
-        )
+        assert expectedexpectedexpectedexpectedexpectedexpectedexpectedexpectedexpect == {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        }