Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip even more redundant for parentheses #3243

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Strip even more redundant for parentheses #3243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3370b68
Remove even more unnecessary for target parentheses
ichard26 Aug 28, 2022
31d0d26
Minor rename for clarity
ichard26 Aug 28, 2022
01dbe47
Add line too long test cases
ichard26 Aug 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
subscript expressions with more than 1 element (#3209)
- Fix a string merging/split issue when a comment is present in the middle of implicitly
concatenated strings on its own line (#3227)
- All unnecessary parentheses in `for` assignments are now removed, previously certain
redundant parentheses were still kept (#3243)

### _Blackd_

Expand Down
49 changes: 34 additions & 15 deletions src/black/linegen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ def visit_funcdef(self, node: Node) -> Iterator[Line]:
if maybe_make_parens_invisible_in_atom(
child,
parent=node,
remove_brackets_around_comma=False,
remove_parens_around_comma=False,
):
wrap_in_parentheses(node, child, visible=False)
else:
Expand Down Expand Up @@ -951,18 +951,14 @@ def normalize_invisible_parens(
if check_lpar:
if (
preview
and child.type == syms.atom
and isinstance(child, Node)
and child.type in (syms.atom, syms.exprlist)
and node.type == syms.for_stmt
and isinstance(child.prev_sibling, Leaf)
and child.prev_sibling.type == token.NAME
and child.prev_sibling.value == "for"
):
if maybe_make_parens_invisible_in_atom(
child,
parent=node,
remove_brackets_around_comma=True,
):
wrap_in_parentheses(node, child, visible=False)
remove_for_target_parens(child, node)
elif preview and isinstance(child, Node) and node.type == syms.with_stmt:
remove_with_parens(child, node)
elif child.type == syms.atom:
Expand Down Expand Up @@ -1015,7 +1011,7 @@ def remove_await_parens(node: Node) -> None:
if maybe_make_parens_invisible_in_atom(
node.children[1],
parent=node,
remove_brackets_around_comma=True,
remove_parens_around_comma=True,
):
wrap_in_parentheses(node, node.children[1], visible=False)

Expand All @@ -1042,6 +1038,28 @@ def remove_await_parens(node: Node) -> None:
remove_await_parens(bracket_contents)


def remove_for_target_parens(node: Node, parent: Node) -> None:
"""Recursively hide optional parens in `for` statements."""
# The main goal is to run `maybe_make_parens_invisible_in_atom` on every atom Node
# between "for" and "in".
if node.type == syms.atom:
# Parenthesized group of nodes/leaves, eg. `(x, y)`
# First try removing the group's surrounding parentheses.
if maybe_make_parens_invisible_in_atom(
node, parent, remove_parens_around_comma=(parent.type == syms.for_stmt)
):
wrap_in_parentheses(parent, node, visible=False)
# Then check if this atom could contain more atoms.
middle = node.children[1]
if isinstance(middle, Node):
remove_for_target_parens(middle, node)
elif node.type in (syms.exprlist, syms.testlist_gexp):
# A series of nodes/leaves separated by commas, eg. `(x), (y)`
for c in node.children:
if isinstance(c, Node) and c.type == syms.atom:
remove_for_target_parens(c, node)


def remove_with_parens(node: Node, parent: Node) -> None:
"""Recursively hide optional parens in `with` statements."""
# Removing all unnecessary parentheses in with statements in one pass is a tad
Expand All @@ -1064,7 +1082,7 @@ def remove_with_parens(node: Node, parent: Node) -> None:
if maybe_make_parens_invisible_in_atom(
node,
parent=parent,
remove_brackets_around_comma=True,
remove_parens_around_comma=True,
):
wrap_in_parentheses(parent, node, visible=False)
if isinstance(node.children[1], Node):
Expand All @@ -1079,15 +1097,16 @@ def remove_with_parens(node: Node, parent: Node) -> None:
if maybe_make_parens_invisible_in_atom(
node.children[0],
parent=node,
remove_brackets_around_comma=True,
remove_parens_around_comma=True,
):
wrap_in_parentheses(node, node.children[0], visible=False)


def maybe_make_parens_invisible_in_atom(
node: LN,
parent: LN,
remove_brackets_around_comma: bool = False,
*,
remove_parens_around_comma: bool = False,
) -> bool:
"""If it's safe, make the parens in the atom `node` invisible, recursively.
Additionally, remove repeated, adjacent invisible parens from the atom `node`
Expand All @@ -1101,10 +1120,10 @@ def maybe_make_parens_invisible_in_atom(
or is_one_tuple(node)
or (is_yield(node) and parent.type != syms.expr_stmt)
or (
# This condition tries to prevent removing non-optional brackets
# This condition tries to prevent removing non-optional parentheses
# around a tuple, however, can be a bit overzealous so we provide
# and option to skip this check for `for` and `with` statements.
not remove_brackets_around_comma
not remove_parens_around_comma
and max_delimiter_priority_in_atom(node) >= COMMA_PRIORITY
)
):
Expand Down Expand Up @@ -1132,7 +1151,7 @@ def maybe_make_parens_invisible_in_atom(
maybe_make_parens_invisible_in_atom(
middle,
parent=parent,
remove_brackets_around_comma=remove_brackets_around_comma,
remove_parens_around_comma=remove_parens_around_comma,
)

if is_atom_with_invisible_parens(middle):
Expand Down
107 changes: 107 additions & 0 deletions tests/data/preview/remove_for_brackets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
for (((x))) in points:
ichard26 marked this conversation as resolved.
Show resolved Hide resolved
pass

# Only remove tuple brackets after `for`
for (k, v) in d.items():
print(k, v)
Expand All @@ -8,6 +11,15 @@
module._verify_python3_env = lambda: None

# Brackets remain for long for loop lines
for (one_super_long_name_as_the_for_target_list_because_why_not_you_dont_control_me) in points:
pass

for (one_super_long_name_as_the_for_target_list_because_why_not_you_dont_control_me_i_have_total_freedom) in points:
pass

for ((many_long_name_tuples, many_long_name_tuples), (many_long_name_tuples, many_long_name_tuples), (many_long_name_tuples, many_long_name_tuples)) in points:
pass

for (why_would_anyone_choose_to_name_a_loop_variable_with_a_name_this_long, i_dont_know_but_we_should_still_check_the_behaviour_if_they_do) in d.items():
print(k, v)

Expand All @@ -18,7 +30,47 @@
for (((((k, v))))) in d.items():
print(k, v)

# One-tuple
for (x,) in points:
pass

# A series of atoms that each need their brackets removed
for (x), (y) in points:
pass

# A mix of "simple" atoms and atoms that contain more atoms
for ((x), (y)) in points:
pass

for ((((x)), (((y))))) in points:
pass

# Mixed again; some of these brackets matter.
for ((x), (y)), z in points:
pass

for ((x, y), z) in points:
pass

for ((x, (((y)))), (((z)))) in points:
pass

for (((x,), (y)), ((z)),) in points:
pass

for ((a, b)), ((c, d)) in points:
pass

for ((((a), (b))), (((c), (d)))) in points:
pass

for (a, (b, (c, (d)))) in points:
pass

# output
for x in points:
pass

# Only remove tuple brackets after `for`
for k, v in d.items():
print(k, v)
Expand All @@ -29,6 +81,21 @@
module._verify_python3_env = lambda: None

# Brackets remain for long for loop lines
for (
one_super_long_name_as_the_for_target_list_because_why_not_you_dont_control_me
) in points:
pass

for one_super_long_name_as_the_for_target_list_because_why_not_you_dont_control_me_i_have_total_freedom in (points):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The odd parentheses is a pre-existing bug in main. Something in right_hand_split or something similar is deciding to make the invisible parentheses around points visible even though the split failed. I'm happy to try to fix this before merging, but I probably won't get to it until September.

pass

for (
(many_long_name_tuples, many_long_name_tuples),
(many_long_name_tuples, many_long_name_tuples),
(many_long_name_tuples, many_long_name_tuples),
) in points:
pass

for (
why_would_anyone_choose_to_name_a_loop_variable_with_a_name_this_long,
i_dont_know_but_we_should_still_check_the_behaviour_if_they_do,
Expand All @@ -46,3 +113,43 @@
# Test deeply nested brackets
for k, v in d.items():
print(k, v)

# One-tuple
for (x,) in points:
pass

# A series of atoms that each need their brackets removed
for x, y in points:
pass

# A mix of "simple" atoms and atoms that contain more atoms
for x, y in points:
pass

for x, y in points:
pass

# Mixed again; some of these brackets matter.
for (x, y), z in points:
pass

for (x, y), z in points:
pass

for (x, y), z in points:
pass

for (
((x,), y),
z,
) in points:
pass

for (a, b), (c, d) in points:
pass

for (a, b), (c, d) in points:
pass

for a, (b, (c, d)) in points:
pass