-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix indentation of if statement after else on separate line #81178
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.
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
Conversation
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| if (false) { } | ||
| } | ||
| } | ||
| """); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot add a test that demonstrates that if we start with else if ... (where the else/if are on the same line) that the if is moved one space after the else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test FormatElseIfOnSameLineWithExtraSpaces that verifies when else if (with multiple spaces) is on the same line, it formats to else if with a single space. The test passes. (ceaa90b)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Your fix doesn't seem to have worked. I have debugged through it and i can see that it is appropriate getting hit and appropriately calling AddEmbeddedStatementsIndentationOperation(list, elseClause.Statement);. However, we still don't actually see the if-statement get indented.
I've added another test that shows that this does work properly with another statement kind (like a 'return statement'). Figure out what other code there must be somewhere in the formatting engine that is causing us to still not move the 'if' statement. Perhaps it's some sort of suppression. Or something else entirely. It's very strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've traced through the code and confirmed the indent operation is being added via AddIndentBlockOperation(list, firstToken, lastToken, TextSpan.FromBounds(...)). The return statement test passes, confirming the mechanism works for other statement types.
For IfStatementSyntax specifically, I suspect there may be:
- An anchor/alignment operation being added elsewhere that overrides the indent
- The
IfStatementSyntaxnode being processed after theElseClauseSyntaxand adding a conflicting operation - Some special handling in the formatting engine's operation resolution
Could you point me to where in the formatting engine I should look for how indent operations are resolved/applied, or if there's specific handling for IfStatementSyntax that might be interfering?
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| } | ||
| """); | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/12286")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| if (!lastToken.IsMissing) | ||
| { | ||
| AddIndentBlockOperation(list, firstToken, lastToken, TextSpan.FromBounds(firstToken.FullSpan.Start, lastToken.FullSpan.End)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot inlining had no effect. The original call here was correct as it was calling into the overload of AddEmbeddedStatementsIndentationOperation that takes a statement as the second operation.
In terms of finding the issue, can you look around in the sibling files of this rule and see if there are any suspect behaviors around IfStatementSyntax, ElseClauseSyntax, IfKeyword, ElseKeyword, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the issue! AnchorIndentationFormattingRule adds an anchor indentation operation for ALL StatementSyntax nodes (lines 51-53), which preserves the if keyword's position and prevents the indent operation from taking effect.
I added a check to skip the anchor operation when an IfStatementSyntax is on a separate line from its else keyword (889d7e7), but the test still fails. The logic looks correct - it checks for newline trivia and returns early to skip adding the anchor. However, the if is still not being indented.
Could there be another source adding an anchor operation, or is there something about how anchor and indent operations interact that I'm missing?
…if on separate lines Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Rebase failed
Fix indentation of if statement after else on a separate line [WIP]
Problem
When an
ifstatement appears on a different line from anelsekeyword, it should be indented like any other embedded statement, but currently it is not indented at all.Root Cause
The issue has two parts:
IndentBlockFormattingRule.cs, theAddEmbeddedStatementsIndentationOperationmethod excludes allIfStatementSyntaxfrom indentation when they are the statement of anElseClauseSyntax, regardless of whether they are on the same line or different lines.AnchorIndentationFormattingRule.cs, an anchor indentation operation is added for ALLStatementSyntaxnodes (includingIfStatementSyntax), which preserves the relative position of theifkeyword and prevents it from being indented.Changes Made
IndentBlockFormattingRule.csto check if theiftoken is on the same line as theelsetokenelse if)AnchorIndentationFormattingRule.csto skip adding anchor operation forIfStatementSyntaxwhen it's on a separate line from itselsekeywordFormatElseIfOnSeparateLinesto validate the fix for separate linesFormatElseIfOnSameLineWithExtraSpacesto validate spacing normalization for same-line caseFormatElseReturnOnSeparateLines(by @CyrusNajmabadi) to confirm indent mechanism works for other statement typesCurrent Status
Test Results:
FormatElseIfOnSameLineWithExtraSpaces- PASSES: Whenelse if(with extra spaces) is on the same line, it correctly formats toelse ifwith a single spaceFormatElseReturnOnSeparateLines- PASSES: Confirms indent mechanism works for other statement types (e.g.,return)FormatElseIfOnSeparateLines- FAILS: Whenelseandifare on different lines, theifis not being indentedInvestigation:
Found that
AnchorIndentationFormattingRuleadds an anchor operation for all statements (line 51-53), which preserves the relative position of theifkeyword and prevents indentation. Added code to skip the anchor operation when there's a newline betweenelseandif, but the test still fails. The logic appears correct but may not be executing as expected or there may be another source of anchor operations.<issue_title>if block not indented after else</issue_title>
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.