-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix property pattern indentation in positional and parenthesized patterns #81199
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
| } | ||
| """); | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/54288")] |
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.
| { | ||
| Identifier: { ValueText: var formerIdentifier } | ||
| } | ||
| }, |
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 test parenthesized patterns as well. so just (...) without comma separation.
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 FormatPropertyPatternInParenthesizedPattern1 for parenthesized patterns without comma separation in commit f31e665.
f31e665 to
efdb235
Compare
|
@dotnet/roslyn-ide ptal. |
| public void DoThing() | ||
| { | ||
| OrderingSyntax former = orderings[i], latter = orderings[j]; | ||
| if ((former, latter) is not ( |
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.
This test code doesn't exactly match the case from the issue. Should we add a test where (former, latter) is not ( is on a new line?
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.
sure. i can add in followup!
| } | ||
| } | ||
| """, """ | ||
| public class Goo |
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.
confusing that the expected comes before the original
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.
yes. i highly dislike formatter tests for this reason :'(
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.
Can copilot add named arguments and reverse the order? =)
Fix Property Pattern Indentation in Positional and Parenthesized Patterns ✅
Problem
FormattingTests_Patterns.csIndentBlockFormattingRule.csSummary
Fixed issue #54434 where property patterns inside positional (tuple) patterns were being incorrectly dedented during formatting. The fix also handles parenthesized patterns without comma separation.
Before: Property patterns in tuple patterns would dedent by 2 levels instead of maintaining proper indentation.
After: Property patterns are correctly indented with one additional level relative to the containing pattern.
Root Cause
When a
RecursivePatternSyntaxwas inside aSubpatternSyntax(positional pattern context), the code attempted to userecursivePatternParent.GetFirstToken()as the alignment base. However, since the RecursivePattern could start with the property pattern's opening brace itself, this resulted inbaseTokenForAlignment == propertyPatternClause.OpenBraceToken, causing the alignment operation to be skipped entirely. This led to incorrect dedentation as fallback rules took over.Solution
Modified
IndentBlockFormattingRule.AddAlignmentBlockOperationfor thePropertyPatternClauseSyntaxcase to:RecursivePatternis inside aSubpatternSyntax(positional pattern context)SubpatternSyntax→PositionalPatternClauseSyntax→ outerRecursivePatternSyntax→ use its parent as the alignment baseFormattingOperations.CreateRelativeIndentBlockOperationdirectly withindentationDelta = 1to add one indentation levelIndentIfConditionOfAnchorTokenflag for positional patterns (only applies to regularispatterns)Testing Results
✅ New test
FormatPropertyPatternInPositionalPattern1passes (tuple patterns with comma separation)✅ New test
FormatPropertyPatternInParenthesizedPattern1passes (parenthesized patterns without comma separation)✅ All 56 pattern formatting tests pass
✅ All 597 formatting tests pass (1 skipped)
✅ CodeQL security scan: No issues found
Security Summary
No security vulnerabilities were introduced or fixed by this change.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.