Unverified Commit 8a59528c authored by Richard Si's avatar Richard Si Committed by GitHub
Browse files

Stop changing return type annotations to tuples (#2384) 


This fixes a bug where a trailing comma would be added to a
parenthesized return annotation changing its type to a tuple.
Here's one case where this bug shows up:

```
def spam() -> (
    this_is_a_long_type_annotation_which_should_NOT_get_a_trailing_comma
):
    pass
```

The root problem was that the type annotation was treated as if it was
a parameter & import list (is_body=True to linegen::bracket_split_build_line)
where a trailing comma is usually fine. Now there's another check in the
aforementioned function to make sure the body it's operating on isn't
a return annotation before truly adding a trailing comma.
parent d249f2d8

CHANGES.md

+2 −0
Changes for CHANGES.md: 2 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -9,6 +9,8 @@ 
- Present a more user-friendly error if .gitignore is invalid (#2414)

- The failsafe for accidentally added backslashes in f-string expressions has been

  hardened to handle more edge cases during quote normalization (#2437)

- Avoid changing a function return type annotation's type to a tuple by adding a

  trailing comma (#2384)



### _Blackd_

src/black/linegen.py

+15 −1
Changes for src/black/linegen.py: 15 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -7,7 +7,7 @@ 


from dataclasses import dataclass, field



from black.nodes import WHITESPACE, STATEMENT, STANDALONE_COMMENT

from black.nodes import WHITESPACE, RARROW, STATEMENT, STANDALONE_COMMENT

from black.nodes import ASSIGNMENTS, OPENING_BRACKETS, CLOSING_BRACKETS

from black.nodes import Visitor, syms, first_child_is_arith, ensure_visible

from black.nodes import is_docstring, is_empty_tuple, is_one_tuple, is_one_tuple_between
@@ -574,6 +574,20 @@ def bracket_split_build_line( 
                original.is_def

                and opening_bracket.value == "("

                and not any(leaf.type == token.COMMA for leaf in leaves)

                # In particular, don't add one within a parenthesized return annotation.

                # Unfortunately the indicator we're in a return annotation (RARROW) may

                # be defined directly in the parent node, the parent of the parent ...

                # and so on depending on how complex the return annotation is.

                # This isn't perfect and there's some false negatives but they are in

                # contexts were a comma is actually fine.

                and not any(

                    node.prev_sibling.type == RARROW

                    for node in (

                        leaves[0].parent,

                        getattr(leaves[0].parent, "parent", None),

                    )

                    if isinstance(node, Node) and isinstance(node.prev_sibling, Leaf)

                )

            )



            if original.is_import or no_commas:

src/black/nodes.py

+2 −0
Changes for src/black/nodes.py: 2 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -135,6 +135,8 @@ 
BRACKETS = OPENING_BRACKETS | CLOSING_BRACKETS

ALWAYS_NO_SPACE = CLOSING_BRACKETS | {token.COMMA, STANDALONE_COMMENT}



RARROW = 55





class Visitor(Generic[T]):

    """Basic lib2to3 visitor that yields things of type `T` on `visit()`."""

tests/data/function_trailing_comma.py

+62 −0
Changes for tests/data/function_trailing_comma.py: 62 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -21,6 +21,34 @@ def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> Set[ 
]:

    json = {"k": {"k2": {"k3": [1,]}}}







# The type annotation shouldn't get a trailing comma since that would change its type.

# Relevant bug report: https://github.com/psf/black/issues/2381.

def some_function_with_a_really_long_name() -> (

    returning_a_deeply_nested_import_of_a_type_i_suppose

):

    pass





def some_method_with_a_really_long_name(very_long_parameter_so_yeah: str, another_long_parameter: int) -> (

    another_case_of_returning_a_deeply_nested_import_of_a_type_i_suppose_cause_why_not

):

    pass





def func() -> (

    also_super_long_type_annotation_that_may_cause_an_AST_related_crash_in_black(this_shouldn_t_get_a_trailing_comma_too)

):

    pass





def func() -> ((also_super_long_type_annotation_that_may_cause_an_AST_related_crash_in_black(

        this_shouldn_t_get_a_trailing_comma_too

    ))

):

    pass



# output



def f(
@@ -86,3 +114,37 @@ def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> Set[ 
            }

        }

    }





# The type annotation shouldn't get a trailing comma since that would change its type.

# Relevant bug report: https://github.com/psf/black/issues/2381.

def some_function_with_a_really_long_name() -> (

    returning_a_deeply_nested_import_of_a_type_i_suppose

):

    pass





def some_method_with_a_really_long_name(

    very_long_parameter_so_yeah: str, another_long_parameter: int

) -> (

    another_case_of_returning_a_deeply_nested_import_of_a_type_i_suppose_cause_why_not

):

    pass





def func() -> (

    also_super_long_type_annotation_that_may_cause_an_AST_related_crash_in_black(

        this_shouldn_t_get_a_trailing_comma_too

    )

):

    pass





def func() -> (

    (

        also_super_long_type_annotation_that_may_cause_an_AST_related_crash_in_black(

            this_shouldn_t_get_a_trailing_comma_too

        )

    )

):

    pass