Fix invalid IR when flattening nested BestFitting elements#25398
Merged
Conversation
Summary -- This PR fixes #25397 by dropping `StartBestFittingEntry` and `EndBestFittingEntry` tags in `RemoveSoftLinesBuffer`. #25144 changed `RemoveSoftLinesBuffer` to inline the most-flat variant of `BestFitting` elements, but it inlined them with their start and end tags, which caused the problem observed in #25397 when these tags became nested in another `BestFitting` element, which interpreted the first `End` tag as ending the outer `BestFitting` rather than the nested one. You can see this clearly in the IR. First, the diff between the 0.15.13 and 0.15.14 IR, which reveals the unmatched start tags: <details> <summary>0.15.13 vs main</summary> ```diff @@ -38,29 +38,13 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 1> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *1>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *1>]), - hard_line_break, - ")" - ] + [ + <interned 1> [ + "str(", + group([indent([group(["arg"])])]), + ")" ] - ]), + ], ", self.args" ]) ]) @@ -105,27 +89,9 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 2> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *2>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *2>]), - hard_line_break, - ")" - ] + [ + <interned 2> [ + "str(", + group([indent([group(["arg"])])]), + ")" ] - ]), - ", self.args" + ] ]) + <START_WITHOUT_END<Group>> ]) - ]), - ")" + <START_WITHOUT_END<Indent>> + ]) + <START_WITHOUT_END<Group>> ]) + <START_WITHOUT_END<Group>> ]) - ]), - ")})\"" - ]), - hard_line_break, - ")" + <START_WITHOUT_END<Indent>> + ]) + <START_WITHOUT_END<Group>> + ]) + <START_WITHOUT_END<Indent>> ]) - ] + <START_WITHOUT_END<Group>> + ]) + <START_WITHOUT_END<BestFittingEntry>> ] ]) ]), ``` </details> and then the diff between main and this PR, which shows the removed brackets that represent the nested tags: <details> <summary>main vs PR</summary> ```diff @@ -38,12 +38,10 @@ "lambda ", group(["arg"]), ": ", - [ - <interned 1> [ - "str(", - group([indent([group(["arg"])])]), - ")" - ] + <interned 1> [ + "str(", + group([indent([group(["arg"])])]), + ")" ], ", self.args" ]) @@ -89,24 +87,15 @@ "lambda ", group(["arg"]), ": ", - [ - <interned 2> [ - "str(", - group([indent([group(["arg"])])]), - ")" - ] - ] + <interned 2> [ + "str(", + group([indent([group(["arg"])])]), + ")" + ], + ", self.args" ]) - <START_WITHOUT_END<Group>> ]) - <START_WITHOUT_END<Indent>> - ]) - <START_WITHOUT_END<Group>> + ]), + ")" ]) - <START_WITHOUT_END<Group>> ]) - <START_WITHOUT_END<Indent>> - ]) - <START_WITHOUT_END<Group>> - ]) - <START_WITHOUT_END<Indent>> + ]), + ")})\"" + ]), + hard_line_break, + ")" ]) - <START_WITHOUT_END<Group>> - ]) - <START_WITHOUT_END<BestFittingEntry>> + ] ] ]) ]), ``` </details> I think the diff between 0.15.13 and this PR is also nice to see because it makes it a bit more clear that the `BestFitting` element has been inlined: <details> <summary>0.15.13 vs this PR</summary> ```diff @@ -38,29 +38,11 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 1> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *1>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *1>]), - hard_line_break, - ")" - ] - ] - ]), + <interned 1> [ + "str(", + group([indent([group(["arg"])])]), + ")" + ], ", self.args" ]) ]) @@ -105,29 +87,11 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 2> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *2>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *2>]), - hard_line_break, - ")" - ] - ] - ]), + <interned 2> [ + "str(", + group([indent([group(["arg"])])]), + ")" + ], ", self.args" ]) ]) @@ -155,7 +119,6 @@ def __str__(self) -> str: return ( - f"first stringlist of args long={list(map(lambda arg: str( - arg - ), self.args))})" + "first string" + f"list of args long={list(map(lambda arg: str(arg), self.args))})" ) ``` </details> And you can also see at the bottom here that the actual formatting that #25144 sought to resolve is still fixed. Test Plan -- New regression test based on the code in the issue
|
MichaReiser
approved these changes
May 26, 2026
anishgirianish
pushed a commit
to anishgirianish/ruff
that referenced
this pull request
May 28, 2026
…sh#25398) Summary -- This PR fixes astral-sh#25397 by dropping `StartBestFittingEntry` and `EndBestFittingEntry` tags in `RemoveSoftLinesBuffer`. astral-sh#25144 changed `RemoveSoftLinesBuffer` to inline the most-flat variant of `BestFitting` elements, but it inlined them with their start and end tags, which caused the problem observed in astral-sh#25397 when these tags became nested in another `BestFitting` element, which interpreted the first `End` tag as ending the outer `BestFitting` rather than the nested one. You can see this clearly in the IR. First, the diff between the 0.15.13 and 0.15.14 IR, which reveals the unmatched start tags: <details> <summary>0.15.13 vs main</summary> ```diff @@ -38,29 +38,13 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 1> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *1>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *1>]), - hard_line_break, - ")" - ] + [ + <interned 1> [ + "str(", + group([indent([group(["arg"])])]), + ")" ] - ]), + ], ", self.args" ]) ]) @@ -105,27 +89,9 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 2> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *2>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *2>]), - hard_line_break, - ")" - ] + [ + <interned 2> [ + "str(", + group([indent([group(["arg"])])]), + ")" ] - ]), - ", self.args" + ] ]) + <START_WITHOUT_END<Group>> ]) - ]), - ")" + <START_WITHOUT_END<Indent>> + ]) + <START_WITHOUT_END<Group>> ]) + <START_WITHOUT_END<Group>> ]) - ]), - ")})\"" - ]), - hard_line_break, - ")" + <START_WITHOUT_END<Indent>> + ]) + <START_WITHOUT_END<Group>> + ]) + <START_WITHOUT_END<Indent>> ]) - ] + <START_WITHOUT_END<Group>> + ]) + <START_WITHOUT_END<BestFittingEntry>> ] ]) ]), ``` </details> and then the diff between main and this PR, which shows the removed brackets that represent the nested tags: <details> <summary>main vs PR</summary> ```diff @@ -38,12 +38,10 @@ "lambda ", group(["arg"]), ": ", - [ - <interned 1> [ - "str(", - group([indent([group(["arg"])])]), - ")" - ] + <interned 1> [ + "str(", + group([indent([group(["arg"])])]), + ")" ], ", self.args" ]) @@ -89,24 +87,15 @@ "lambda ", group(["arg"]), ": ", - [ - <interned 2> [ - "str(", - group([indent([group(["arg"])])]), - ")" - ] - ] + <interned 2> [ + "str(", + group([indent([group(["arg"])])]), + ")" + ], + ", self.args" ]) - <START_WITHOUT_END<Group>> ]) - <START_WITHOUT_END<Indent>> - ]) - <START_WITHOUT_END<Group>> + ]), + ")" ]) - <START_WITHOUT_END<Group>> ]) - <START_WITHOUT_END<Indent>> - ]) - <START_WITHOUT_END<Group>> - ]) - <START_WITHOUT_END<Indent>> + ]), + ")})\"" + ]), + hard_line_break, + ")" ]) - <START_WITHOUT_END<Group>> - ]) - <START_WITHOUT_END<BestFittingEntry>> + ] ] ]) ]), ``` </details> I think the diff between 0.15.13 and this PR is also nice to see because it makes it a bit more clear that the `BestFitting` element has been inlined: <details> <summary>0.15.13 vs this PR</summary> ```diff @@ -38,29 +38,11 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 1> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *1>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *1>]), - hard_line_break, - ")" - ] - ] - ]), + <interned 1> [ + "str(", + group([indent([group(["arg"])])]), + ")" + ], ", self.args" ]) ]) @@ -105,29 +87,11 @@ "lambda ", group(["arg"]), ": ", - best_fitting([ - [ - [ - <interned 2> [ - "str(", - group([ - indent([soft_line_break, group(["arg"])]), - soft_line_break - ]), - ")" - ] - ] - ] - [[group(expand: true, [<ref interned *2>])]] - [ - [ - "(", - indent([hard_line_break, <ref interned *2>]), - hard_line_break, - ")" - ] - ] - ]), + <interned 2> [ + "str(", + group([indent([group(["arg"])])]), + ")" + ], ", self.args" ]) ]) @@ -155,7 +119,6 @@ def __str__(self) -> str: return ( - f"first stringlist of args long={list(map(lambda arg: str( - arg - ), self.args))})" + "first string" + f"list of args long={list(map(lambda arg: str(arg), self.args))})" ) ``` </details> And you can also see at the bottom here that the actual formatting that astral-sh#25144 sought to resolve is still fixed. Test Plan -- New regression test based on the code in the issue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes #25397 by dropping
StartBestFittingEntryandEndBestFittingEntrytags inRemoveSoftLinesBuffer. #25144 changedRemoveSoftLinesBufferto inline the most-flat variant ofBestFittingelements, but it inlined them with their start and end tags, which caused the problemobserved in #25397 when these tags became nested in another
BestFittingelement, which interpretedthe first
Endtag as ending the outerBestFittingrather than the nested one. You can see thisclearly in the IR. First, the diff between the 0.15.13 and 0.15.14 IR, which reveals the unmatched
start tags:
0.15.13 vs main
and then the diff between main and this PR, which shows the removed brackets that represent the
nested tags:
main vs PR
I think the diff between 0.15.13 and this PR is also nice to see because it makes it a bit more
clear that the
BestFittingelement has been inlined:0.15.13 vs this PR
And you can also see at the bottom here that the actual formatting that #25144 sought to resolve
is still fixed.
Test Plan
New regression test based on the code in the issue