Skip to content

Fix lambda body formatting for multiline calls and subscripts#23866

Merged
ntBre merged 6 commits intomainfrom
brent/lambda-strings
Mar 12, 2026
Merged

Fix lambda body formatting for multiline calls and subscripts#23866
ntBre merged 6 commits intomainfrom
brent/lambda-strings

Conversation

@ntBre
Copy link
Copy Markdown
Contributor

@ntBre ntBre commented Mar 10, 2026

Summary

This PR fixes #23851 by expanding the special case for fluent call chains that previously used parenthesize_if_expands to include any calls or subscripts with OptionalParentheses::Multiline. The minimal change is in the third commit (just swapping out the fluent chain check for matches!(needs_parentheses, OptionalParentheses::Multiline)), but then I reordered the conditional branches in a way that seemed to make more sense.

Test Plan

New tests based on the issue

@ntBre ntBre added bug Something isn't working formatter Related to the formatter labels Mar 10, 2026
@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot bot commented Mar 10, 2026

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@ntBre ntBre marked this pull request as ready for review March 10, 2026 15:47
@ntBre ntBre requested a review from MichaReiser as a code owner March 10, 2026 15:47
Copy link
Copy Markdown
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. The change makes sense to me and relying on needs_parantheses is the right approach. The only thing that makes me a little hesitant is that I have a hard time judging the ecosystem impact.

What could be helpful is to try to come up with a few more tests (Claude might be helpful here). What other cases were affected by the same bug before your fix? Which cases should remain unchanged?

Can you think of cases where the new IR might result in different formatting than before because we no longer use best fitting? Specifically, are there cases where we'll now parenthesize the entire expression where we only parenthesized the call expression before?

@ntBre
Copy link
Copy Markdown
Contributor Author

ntBre commented Mar 11, 2026

Yep, makes sense! I was a bit disappointed to see an empty ecosystem report again.

Claude unfortunately hasn't been too helpful yet, but I'll keep thinking about it. I think we're looking for cases that are still Calls or Subscripts but with bodies that cause needs_parentheses to return either Always or Multiline since those are the only cases that now take precedence over the old Call | Subscript check.

@ntBre
Copy link
Copy Markdown
Contributor Author

ntBre commented Mar 11, 2026

I pushed a couple of somewhat tricky cases for other expression types inside of calls, but these are handled fine on main. After looking through the branches in needs_parentheses, I think I'm fully convinced that the only special cases are implicitly concatenated strings and fluent call chains, both of which are handled correctly now (fluent call chains were handled properly before, and now the special case is generalized). Other expression types already require their own parentheses when they're the target of a call or subscript and thus aren't affected by this bug.

@MichaReiser
Copy link
Copy Markdown
Member

MichaReiser commented Mar 12, 2026

Sounds good. Thanks for taking the time to assess the impact of this change.

An example like this (specifically testing the different break points) might be good to verify:

foo = lambda: call()(extra_argument_one, extra_argument_two, extra_argument_three)

@ntBre ntBre enabled auto-merge (squash) March 12, 2026 13:32
@ntBre ntBre merged commit 37cdd61 into main Mar 12, 2026
42 checks passed
@ntBre ntBre deleted the brent/lambda-strings branch March 12, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working formatter Related to the formatter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

format produces sematically incorrect output when lambda body contains implicit string concatenation

2 participants