Skip to content

Keep the required trailing comma of a one-element subscript under --skip-magic-trailing-comma - #5272

Open
LuShadowX wants to merge 3 commits into
psf:mainfrom
LuShadowX:subscript-one-tuple-comma
Open

Keep the required trailing comma of a one-element subscript under --skip-magic-trailing-comma#5272
LuShadowX wants to merge 3 commits into
psf:mainfrom
LuShadowX:subscript-one-tuple-comma

Conversation

@LuShadowX

Copy link
Copy Markdown

Description

--skip-magic-trailing-comma drops the trailing comma from a one-element subscript when the line is long enough to be split and contains a power operator, which turns a[x,] (indexing with a one-tuple) into a[x] and trips Black's own AST safety check:

value = alpha[beta,]() ** gamma * delta << epsilon | zeta < eta ^ theta * iota + kappa + mu
$ black --skip-magic-trailing-comma file.py
error: cannot format file.py: INTERNAL ERROR: Black produced code that is not equivalent to the source.

The comma survives on a short line — tests/data/cases/skip_magic_trailing_comma.py opens by asserting exactly that — so this is the same rule failing on a longer one. Under --fast there is no safety net and the changed code is written out.

hug_power_op is the path. It rebuilds the line with leaf.clone() for every leaf, and a clone has no parent, so both tree-based guards that protect a required comma stop working: has_magic_trailing_comma's single-element-subscript branch needs closing.parent.type == syms.trailer, and the one-tuple guard in Line.append needs leaf.parent. Neither can fire, the comma looks magic, and remove_trailing_comma() takes it.

Only the operands either side of a hugged ** actually need their prefix rewritten, so this stops cloning the rest and reuses the leaf, keeping it attached to the tree. Reusing leaves across lines is what bracket_split_build_line already does, and with preformatted=True Line.append does not touch the prefix — it only re-runs BracketTracker.mark, which recomputes the same metadata for an identical leaf sequence.

The one-tuple guard the same clone breaks (foo(lambda x=(1,): x), already in that case file) is fixed by this too; I could not build a failing example for it because the lambda cases are short enough not to reach hug_power_op.

Found with a randomised run of scripts/fuzz.py's property — the committed harness pins derandomize=True, max_examples=1000, so it re-explores the same thousand inputs on every CI run and never reaches this one.

Verification

  • Case added to tests/data/cases/skip_magic_trailing_comma.py. It fails on main (alpha[beta]() in the output) and passes with the change.
  • pytest: 474 passed, 3 skipped, unchanged from main. black --check ., flake8, isort, mypy clean.
  • Formatted 716 files from scrapy, pydantic, Pillow and fontTools with --skip-magic-trailing-comma before and after, comparing output hashes: zero differences, no errors either way. Power-operator hugging is unaffected (x = a ** b still becomes x = a**b).

Checklist - did you ...

  • Implement any code style changes under the --preview style, following the stability policy? — no style change; this restores the documented behaviour for a case that was crashing
  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation? — no documented behaviour changes

hug_power_op rebuilt the line from cloned leaves, and a clone has no parent,
so Line.append's tree-based guards could not tell a syntactically required
trailing comma from a magic one. Under --skip-magic-trailing-comma the comma
of a one-element subscript was dropped, turning a[x,] into a[x] and failing
Black's own AST safety check. Only the operands around a hugged ** need a
copy now; every other leaf is reused.
@LuShadowX

Copy link
Copy Markdown
Author

One more data point: 6000 randomised examples through scripts/fuzz.py's property on top of this branch — the same run that produced the failing input on main — come back clean.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

diff-shades results comparing this PR (85055d1) to main (c7801d9):

--preview style: no changes

--stable style: no changes


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

Comment thread CHANGES.md Outdated
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.

2 participants