Skip to content

Fix unnecessary parentheses in indexed assignment RHS#5095

Merged
cobaltt7 merged 16 commits into
psf:mainfrom
ParamChordiya:fix/unnecessary-parens-indexed-assignment
Jun 10, 2026
Merged

Fix unnecessary parentheses in indexed assignment RHS#5095
cobaltt7 merged 16 commits into
psf:mainfrom
ParamChordiya:fix/unnecessary-parens-indexed-assignment

Conversation

@ParamChordiya

Copy link
Copy Markdown
Contributor

Summary

Fixes #4349.

When an assignment target contains brackets (e.g. indexed access like x[key] = expr), Black would wrap the right-hand side expression in unnecessary parentheses when the line was too long.

Before:

dictionary_of_arrays["long_key_name_for_the_example"][
    very_long_index_name, index_zero
] = (10 - 5)

After:

dictionary_of_arrays["long_key_name_for_the_example"][
    very_long_index_name, index_zero
] = 10 - 5

Details

The root cause is in can_omit_invisible_parens() in src/black/lines.py. This function decides whether optional parentheses around the RHS can be safely removed. It returned False for simple expressions with operators (like 10 - 5) because they don't start or end with brackets — falling through to the default return False.

The fix adds a targeted check: when the RHS head ends with = and the LHS contains brackets (indicating an indexed/subscript assignment), and the body expression is short enough, allow omitting the optional parens. The downstream _prefer_split_rhs_oop_over_rhs() still makes the final decision on whether the resulting layout is better.

The line_length // 2 threshold ensures we only omit parens for genuinely short RHS expressions — long expressions that need their own line splitting still get wrapped as before.

Test plan

When an assignment target contains brackets (e.g. indexed access like
`x[key] = expr`), Black would wrap the right-hand side expression in
unnecessary parentheses when the line was too long. For example:

    dictionary["key"][idx] = (10 - 5)

This happened because `can_omit_invisible_parens` returned False for
simple expressions with operators (like `10 - 5`) that don't start or
end with brackets. The function was too conservative: it didn't consider
that the line could instead be split at the LHS brackets, producing:

    dictionary["key"][
        idx
    ] = 10 - 5

The fix allows omitting optional parens when the RHS body is short
enough and the LHS contains brackets that can absorb the split. The
downstream `_prefer_split_rhs_oop_over_rhs` still decides whether the
resulting layout is actually better.

Fixes psf#4349.
@github-actions

github-actions Bot commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

diff-shades results comparing this PR (0ce601c) to main (e34bb1b):

--preview style: no changes

--stable style: no changes


What is this? | Workflow run | diff-shades documentation

…S.md

Move the fix for unnecessary parentheses in indexed assignment RHS
behind a Preview flag to avoid unintentional stable style changes.
Test cases moved to a preview-specific test file.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread src/black/lines.py Outdated
Comment thread src/black/lines.py Outdated
ParamChordiya and others added 3 commits April 12, 2026 21:37
…ck, add docs

- Make `mode` parameter required in `can_omit_invisible_parens` to prevent
  accidental omission at call sites (fixes flake8 B008)
- Replace `str_width(str(line))` with `enumerate_with_length()` for an
  AST-only body length calculation (avoids expensive string conversion)
- Document `fix_unnecessary_parens_in_indexed_assignment` in
  `docs/the_black_code_style/future_style.md` (fixes test_feature_lists_are_up_to_date)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ment

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread docs/the_black_code_style/future_style.md Outdated
Comment thread src/black/lines.py Outdated
cobaltt7
cobaltt7 previously approved these changes Apr 24, 2026
@cobaltt7 cobaltt7 dismissed their stale review April 24, 2026 18:32

Didn't realize the changes to Black and in shades

@cobaltt7

cobaltt7 commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

Almost missed this:
There's a few side-effects here that don't look like an improvement to me
image
image
image

Black would previously prefer to wrap long lines in parentheses to get it under the line length:

# -l 80
middle.children[1].prefix = (
    middle.children[0].prefix + middle.children[1].prefix
)

With this change, Black always prefers to split lines instead of wrap them, which isn't an improvement

# -l 80
middle.children[1].prefix = middle.children[0].prefix + middle.children[
    1
].prefix

…r_rhs

When the OOP split lands mid-chain (tail starts with ] followed by more
tokens like .attr, and = is already in head), the split is inside a
subscript access on the RHS — not the LHS target. Guard against this
by returning False so paren-wrapping is preferred instead.

Adds regression tests for attribute-subscript chains on the RHS.
Black formats itself as part of CI (run_self). After the formatting
rule change in _prefer_split_rhs_oop_over_rhs, 4 files needed
reformatting to stay consistent with the updated formatter output.
The guard in _prefer_split_rhs_oop_over_rhs that prevents mid-chain
subscript splits on the RHS was running unconditionally, affecting
stable mode formatting across a large corpus (diff-shades failure).

Gate it behind Preview.fix_unnecessary_parens_in_indexed_assignment,
matching the flag used in can_omit_invisible_parens, so stable code
style is unchanged.
The unstable-mode self-format (eba22d0) broke test_piping which
formats __init__.py in stable mode and expects an idempotent result.
The two modes disagree on the ternary expression formatting, so the
files need to stay in the stable-mode-idempotent form from the
upstream merge (87d9925).
In linegen.py: tighten the subscript-chain guard to only suppress OOP
splitting when the token after `]` is a DOT (attribute access), not any
token. Without this, list literal assignments with magic trailing commas
were incorrectly paren-wrapped because the tail had `]` + NEWLINE (count=2).

In lines.py: fix can_omit_invisible_parens to check only visible brackets
in the LHS when deciding whether to allow the indexed-assignment OOP path.
Invisible bracket tokens (empty LPAR/RPAR from tuple assignments like
`a, b = expr`) were matching the condition, causing tuple-target assignments
to incorrectly omit optional parens and produce unstable formatting.
@ParamChordiya

ParamChordiya commented Apr 26, 2026

Copy link
Copy Markdown
Contributor Author

Hey @cobaltt7 , thanks for catching that, really appreciate the thorough review. You were right, and it took a fair bit of digging to get to the bottom of it.

What was happening

The regression you spotted (middle.children[1].prefix = ... splitting mid-chain at the subscript instead of paren-wrapping the whole RHS) was happening because _prefer_split_rhs_oop_over_rhs in linegen.py was choosing the OOP (omit-optional-parens) split path even when the result was ugly. The OOP split would break at middle.children[ and put the index 1 on its own line, which is clearly wrong for a chained attribute access.
To fix it, I added a guard in _prefer_split_rhs_oop_over_rhs that says: if the OOP tail starts with ] followed by a . (meaning we'd be splitting inside a subscript access chain like obj[idx].attr), and the = is in the head (so the split is on the RHS, not the LHS), prefer paren-wrapping instead.

The first round of issues
After gating this guard behind Preview.fix_unnecessary_parens_in_indexed_assignment (to avoid affecting stable mode), the diff-shades corpus check was clean, but "Format ourselves" started failing on lint and the uvloop platforms. Turned out the guard was slightly too broad the condition len(rhs_oop.tail.leaves) > 1 was firing for list literal assignments like expected: list[Path] = [items] because the tail had ] plus a NEWLINE token (count=2, not just ] alone). That caused those list assignments to get incorrectly paren-wrapped. Fixed by tightening to rhs_oop.tail.leaves[1].type == token.DOT only suppress OOP when the character immediately after ] is actually a dot (attribute access).

The second bug, the trickier one
Even after that, "Format ourselves" was still failing because init.py kept getting reformatted. Traced it down to a separate bug in can_omit_invisible_parens in lines.py. The indexed-assignment early-return block checks whether there are brackets in the LHS: any(leaf.type in BRACKETS for leaf in rhs.head.leaves[:-2])
The problem: Python's AST wraps multi-target assignments like root, method = expr with invisible parentheses — these show up as empty-value LPAR/RPAR tokens in the leaf list. Those invisible tokens were matching the bracket condition and causing the indexed-assignment OOP path to fire for completely unrelated tuple assignments. That led to the outer parens being removed from the ternary in init.py, which is stable-idempotent but not unstable-idempotent, creating a formatting loop.

The fix was a one-character addition: and leaf.value, only count a bracket if it has an actual visible value, which correctly excludes the implicit/invisible tokens that the parser inserts.

How we verified

  • All 211 format tests pass, including the prefer_rhs_split regression cases added in the original PR
  • test_piping passes (reads init.py in stable mode and expects idempotency)
  • python -m black --check src/ tests/ with the project's own unstable = true config reports 57 files unchanged
  • init.py is idempotent under both stable and unstable mode now

The net change across both files is two lines, but it took some real archaeology to get there, the invisible-bracket issue in particular was subtle since nothing in the usual formatting output hinted that those tokens even existed.

@cobaltt7

Copy link
Copy Markdown
Collaborator

Please check the shades changes - it's still splitting within function calls.

@cobaltt7

cobaltt7 commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

Hey @ParamChordiya, are you still updating this PR?

@ParamChordiya

Copy link
Copy Markdown
Contributor Author

Hey @ParamChordiya, are you still updating this PR?

Hey @cobaltt7 sorry i got caught up with something else and lost track of this PR. let me circle back soon.

ParamChordiya and others added 3 commits June 9, 2026 19:39
…ens-indexed-assignment

# Conflicts:
#	CHANGES.md
#	docs/the_black_code_style/future_style.md
#	src/black/resources/black.schema.json
The previous heuristic in can_omit_invisible_parens was too broad and
caused 758 unwanted formatting changes across the diff-shades corpus:

- It allowed omitting optional parens whenever the RHS contained any
  bracket, even when the RHS did not fit on one line, producing
  mid-expression splits inside function calls and ternaries.
- Its LHS check matched any bracket before the '=', so it fired for
  parenthesized tuple targets like '(x,) = ...' and annotated
  assignments like 'x: list[str] = ...', which are not indexed
  assignments.

The path now requires: a visible ']' immediately before the '=' (a
genuine subscripted target), no annotation, a head too long to fit on
one line (so the subscript split is forced regardless), and an RHS that
fits on the '] = ' tail line. This preserves the intended psf#4349 fix
while leaving every other assignment shape untouched.

With the narrowed check, bad candidates no longer reach the
rhs-oop-vs-rhs comparison, so the subscript-chain guard added to
_prefer_split_rhs_oop_over_rhs is no longer needed and is removed.

Verified: all format tests pass; every regression hunk from the
diff-shades report now formats unchanged; black's own sources are
stable and idempotent under stable, preview, and unstable modes.
@ParamChordiya

Copy link
Copy Markdown
Contributor Author

Hey @cobaltt7, back on this — thanks for your patience. You were right that the earlier approach was still too aggressive, so I reworked the heuristic instead of patching around it.

What was causing the bad shades changes

The check in can_omit_invisible_parens had an escape hatch that allowed omitting the parens whenever the RHS contained any bracket, even when the RHS didn't fit on one line. The splitter would then break at the last bracket inside the expression — which is exactly the "splitting within function calls" you flagged (ternaries like kwargs["empty_label"] = kwargs.get(...) if ... else None, and method chains like _get_totp(...).generate(...).decode()). It was also matching things that aren't indexed assignments at all: parenthesized tuple targets ((self.next,) = ...) and annotated assignments (self.args: list[str] = ...), because it only looked for "any bracket before the =".

The new approach

The paren-omission path now only fires when all of these hold:

  • the target genuinely ends in a subscript — a visible ] immediately before the =
  • it isn't an annotated assignment (so the ] of list[str] doesn't count)
  • the LHS is too long to fit on one line, so Black is forced to split at the subscript brackets regardless
  • the RHS fits on the resulting ] = expr tail line

In other words, it only removes the parens in the exact #4349 situation: the line has to split at the LHS anyway, and the parens on the tail are pure noise. When the LHS fits, paren-wrapping the RHS stays the preferred style, untouched. With the bad candidates eliminated at the source, the subscript-chain guard I'd added to _prefer_split_rhs_oop_over_rhs became dead code, so it's removed — the diff is smaller than before.

How I verified

  • I reconstructed every hunk from the last diff-shades report (all 88 hunks / 758 changes across the 9 projects) and ran the new build over them: zero changes remain, so the only diff-shades delta left should be the intended Unnecessary parentheses added to expression in indexed assignment #4349 shape.
  • All format tests pass, and I added regression cases for each failure mode you surfaced (ternary RHS, method chains, tuple targets, annotated assignments, and a too-long RHS that should still wrap).
  • Black's own sources stay clean and idempotent under stable, preview, and unstable modes.
  • A generated sweep of ~1000 assignment variants (slices, augmented/chained/annotated targets, walrus, lambdas, comments, wide chars) shows no stable-vs-preview differences outside the intended improvement, and no idempotency issues.

Also merged main and resolved the conflicts, so the branch is mergeable again. Would appreciate another look when you get a chance!

@cobaltt7

Copy link
Copy Markdown
Collaborator

Thanks!

@cobaltt7 cobaltt7 merged commit 00cd1e4 into psf:main Jun 10, 2026
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unnecessary parentheses added to expression in indexed assignment

2 participants