Skip to content

Commit

Permalink
Stop changing return type annotations to tuples (#2384)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ichard26 committed Aug 26, 2021
1 parent d249f2d commit 8a59528
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 2 deletions.
2 changes: 2 additions & 0 deletions CHANGES.md
Expand Up @@ -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_

Expand Down
16 changes: 15 additions & 1 deletion src/black/linegen.py
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
2 changes: 2 additions & 0 deletions src/black/nodes.py
Expand Up @@ -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()`."""
Expand Down
64 changes: 63 additions & 1 deletion tests/data/function_trailing_comma.py
Expand Up @@ -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(
Expand Down Expand Up @@ -85,4 +113,38 @@ 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

0 comments on commit 8a59528

Please sign in to comment.