From 24c42ecabadfe6c69da3e9cfc1a3b9ec7b0055da Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 21 Nov 2025 10:29:09 +0100 Subject: [PATCH 1/3] Fix typing of 'else' causing unwanted indentation of following statement --- .../FormattingCommandHandlerTests.vb | 68 ++++++++++++++ .../CSharpSyntaxFormattingService.cs | 88 ++++++++++++------- 2 files changed, 125 insertions(+), 31 deletions(-) diff --git a/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb b/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb index 2504dc7823b57..ea3ef48dc58c1 100644 --- a/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb +++ b/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb @@ -79,6 +79,74 @@ class TestClass End Using End Sub + + Public Sub TypingElseKeywordDoesNotIndentFollowingNonBlockConstruct() + Using state = TestStateFactory.CreateCSharpTestState( + using System; +class TestClass +{ + void TestMethod(string[] args) + { + if (v != null) + Console.WriteLine("v is not null"); + els$$ + + if (v != null) + Console.WriteLine("v is not null"); + } +}, includeFormatCommandHandler:=True) + state.SendTypeChars("e") + + AssertEx.Equal("using System; +class TestClass +{ + void TestMethod(string[] args) + { + if (v != null) + Console.WriteLine(""v is not null""); + else + + if (v != null) + Console.WriteLine(""v is not null""); + } +}", state.GetDocumentText()) + End Using + End Sub + + + Public Sub TypingElseKeywordDoesNotIndentFollowingBlockConstruct() + Using state = TestStateFactory.CreateCSharpTestState( + using System; +class TestClass +{ + void TestMethod(string[] args) + { + if (v != null) + Console.WriteLine("v is not null"); + els$$ + { + Console.WriteLine("v is null"); + } + } +}, includeFormatCommandHandler:=True) + state.SendTypeChars("e") + + AssertEx.Equal("using System; +class TestClass +{ + void TestMethod(string[] args) + { + if (v != null) + Console.WriteLine(""v is not null""); + else + { + Console.WriteLine(""v is null""); + } + } +}", state.GetDocumentText()) + End Using + End Sub + Private Shared Sub AssertVirtualCaretColumn(state As TestState, expectedCol As Integer) Dim caretLine = state.GetLineFromCurrentCaretPosition() Dim caret = state.GetCaretPoint() diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Formatting/CSharpSyntaxFormattingService.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Formatting/CSharpSyntaxFormattingService.cs index 05bbaf4e19eca..2319ae8d6ddb3 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Formatting/CSharpSyntaxFormattingService.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Formatting/CSharpSyntaxFormattingService.cs @@ -79,37 +79,7 @@ public ImmutableArray GetFormattingChangesOnTypedCharacter( var token = root.FindToken(Math.Max(0, caretPosition - 1), findInsideTrivia: true); var formattingRules = GetFormattingRules(document, caretPosition, token); - // Do not attempt to format on open/close brace if autoformat on close brace feature is off, instead just smart - // indent. - // - // We want this behavior because it's totally reasonable for a user to want to not have on automatic formatting - // because they feel it is too aggressive. However, by default, if you have smart-indentation on and are just - // hitting enter, you'll common have the caret placed one indent higher than your current construct. For - // example, if you have: - // - // if (true) - // $ <-- smart indent will have placed the caret here here. - // - // This is acceptable given that the user may want to just write a simple statement there. However, if they - // start writing `{`, then things should snap over to be: - // - // if (true) - // { - // - // Importantly, this is just an indentation change, no actual 'formatting' is done. We do the same with close - // brace. If you have: - // - // if (...) - // { - // bad . ly ( for (mmated+code) ) ; - // $ <-- smart indent will have placed the care here. - // - // If the user hits `}` then we will properly smart indent the `}` to match the `{`. However, we won't touch any - // of the other code in that block, unlike if we were formatting. - var onlySmartIndent = - (token.IsKind(SyntaxKind.CloseBraceToken) && OnlySmartIndentCloseBrace(indentationOptions.AutoFormattingOptions)) || - (token.IsKind(SyntaxKind.OpenBraceToken) && OnlySmartIndentOpenBrace(indentationOptions.AutoFormattingOptions)); - + var onlySmartIndent = OnlySmartIndentBraceToken() || OnlySmartIndentElseKeyword(); if (onlySmartIndent) { // if we're only doing smart indent, then ignore all edits to this token that occur before the span of the @@ -124,6 +94,62 @@ public ImmutableArray GetFormattingChangesOnTypedCharacter( return changes; return [.. FormatToken(document, indentationOptions, token, formattingRules, cancellationToken)]; + + bool OnlySmartIndentBraceToken() + { + // Do not attempt to format on open/close brace if autoformat on close brace feature is off, instead just smart + // indent. + // + // We want this behavior because it's totally reasonable for a user to want to not have on automatic formatting + // because they feel it is too aggressive. However, by default, if you have smart-indentation on and are just + // hitting enter, you'll common have the caret placed one indent higher than your current construct. For + // example, if you have: + // + // if (true) + // $ <-- smart indent will have placed the caret here here. + // + // This is acceptable given that the user may want to just write a simple statement there. However, if they + // start writing `{`, then things should snap over to be: + // + // if (true) + // { + // + // Importantly, this is just an indentation change, no actual 'formatting' is done. We do the same with close + // brace. If you have: + // + // if (...) + // { + // bad . ly ( for (mmated+code) ) ; + // $ <-- smart indent will have placed the care here. + // + // If the user hits `}` then we will properly smart indent the `}` to match the `{`. However, we won't touch any + // of the other code in that block, unlike if we were formatting. + return + (token.IsKind(SyntaxKind.CloseBraceToken) && OnlySmartIndentCloseBrace(indentationOptions.AutoFormattingOptions)) || + (token.IsKind(SyntaxKind.OpenBraceToken) && OnlySmartIndentOpenBrace(indentationOptions.AutoFormattingOptions)); + } + + bool OnlySmartIndentElseKeyword() + { + // If the user has typed the `e` in `else` we only want to indent the 'else' token itself (the purpose is to + // place the `else` token properly in cases like: + // + // foreach (var item in collection) + // if (item.IsValid) + // Process(item); + // els //<-- user types 'e' here + // + // In this case we want to indent 'else' to match the 'if' above. We do not want to adjust the formatting of + // anything else. This is especially important as 'else' takes an embedded statement, and we don't want the + // following unrelated statement to get reformatted (unless it's a block that should travel with the else. + if (token.Kind() != SyntaxKind.ElseKeyword) + return false; + + if (token.Parent is not ElseClauseSyntax elseClause) + return false; + + return elseClause.Statement is not BlockSyntax; + } } private static bool OnlySmartIndentCloseBrace(in AutoFormattingOptions options) From ba19b2fe455bc2feb2ea874369d6c59d9f3b6103 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 21 Nov 2025 10:30:56 +0100 Subject: [PATCH 2/3] Apply suggestions from code review --- .../Test2/Formatting/FormattingCommandHandlerTests.vb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb b/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb index ea3ef48dc58c1..9a713842cdc00 100644 --- a/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb +++ b/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb @@ -79,7 +79,7 @@ class TestClass End Using End Sub - + Public Sub TypingElseKeywordDoesNotIndentFollowingNonBlockConstruct() Using state = TestStateFactory.CreateCSharpTestState( using System; @@ -107,13 +107,13 @@ class TestClass else if (v != null) - Console.WriteLine(""v is not null""); + Console.WriteLine(""v is not null""); } }", state.GetDocumentText()) End Using End Sub - + Public Sub TypingElseKeywordDoesNotIndentFollowingBlockConstruct() Using state = TestStateFactory.CreateCSharpTestState( using System; From de6dde63d32dff14bdecf6bad3431af7fd716e45 Mon Sep 17 00:00:00 2001 From: Joey Robichaud Date: Fri, 21 Nov 2025 08:55:03 -0800 Subject: [PATCH 3/3] pply suggestions from code review --- .../Test2/Formatting/FormattingCommandHandlerTests.vb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb b/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb index 9a713842cdc00..071c10211922c 100644 --- a/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb +++ b/src/EditorFeatures/Test2/Formatting/FormattingCommandHandlerTests.vb @@ -92,7 +92,7 @@ class TestClass els$$ if (v != null) - Console.WriteLine("v is not null"); + Console.WriteLine("v is not null"); } }, includeFormatCommandHandler:=True) state.SendTypeChars("e")