From bf2b98d9fe5554190782ea354b3f326594302ed0 Mon Sep 17 00:00:00 2001 From: David Wengier Date: Thu, 27 Jan 2022 10:33:36 +1100 Subject: [PATCH 1/4] Format document after each provider --- ...CSharpNewDocumentFormattingServiceTests.cs | 26 +++++++++++++++++++ ...stractNewDocumentFormattingServiceTests.cs | 11 +++++--- .../AbstractNewDocumentFormattingService.cs | 2 ++ .../Implementation/AbstractEditorFactory.cs | 8 ++---- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/Formatting/CSharpNewDocumentFormattingServiceTests.cs b/src/EditorFeatures/CSharpTest/Formatting/CSharpNewDocumentFormattingServiceTests.cs index 12f72de194563..7213dbbad00a3 100644 --- a/src/EditorFeatures/CSharpTest/Formatting/CSharpNewDocumentFormattingServiceTests.cs +++ b/src/EditorFeatures/CSharpTest/Formatting/CSharpNewDocumentFormattingServiceTests.cs @@ -8,6 +8,7 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.CodeStyle; using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces; +using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.Test.Utilities.Formatting; using Roslyn.Test.Utilities; using Xunit; @@ -171,6 +172,31 @@ internal class C }); } + [Fact] + public async Task TestAccessibilityModifiers_FileScopedNamespace() + { + await TestAsync(testCode: @"using System; + +namespace Goo +{ + class C + { + } +}", + expected: @"using System; + +namespace Goo; +internal class C +{ +} +", + options: new (OptionKey, object)[] + { + (new OptionKey(CSharpCodeStyleOptions.NamespaceDeclarations), new CodeStyleOption2(NamespaceDeclarationPreference.FileScoped, NotificationOption2.Error)), + (new OptionKey(CodeStyleOptions2.RequireAccessibilityModifiers, Language), new CodeStyleOption2(AccessibilityModifiersRequired.Always, NotificationOption2.Error)) + }); + } + [Fact] [WorkItem(55703, "https://github.com/dotnet/roslyn/issues/55703")] public async Task TestAccessibilityModifiers_IgnoresPartial() diff --git a/src/EditorFeatures/TestUtilities/Formatting/AbstractNewDocumentFormattingServiceTests.cs b/src/EditorFeatures/TestUtilities/Formatting/AbstractNewDocumentFormattingServiceTests.cs index dc38f4d5bff3c..c51a70e87b39a 100644 --- a/src/EditorFeatures/TestUtilities/Formatting/AbstractNewDocumentFormattingServiceTests.cs +++ b/src/EditorFeatures/TestUtilities/Formatting/AbstractNewDocumentFormattingServiceTests.cs @@ -40,6 +40,14 @@ internal Task TestAsync(string testCode, string expected, (Option2, T)[]? parseOptions); } + internal Task TestAsync(string testCode, string expected, (OptionKey, object)[]? options = null, ParseOptions? parseOptions = null) + { + return TestCoreAsync(testCode, + expected, + options, + parseOptions); + } + private async Task TestCoreAsync(string testCode, string expected, (OptionKey, T)[]? options, ParseOptions? parseOptions) { using (var workspace = CreateTestWorkspace(testCode, parseOptions)) @@ -60,9 +68,6 @@ private async Task TestCoreAsync(string testCode, string expected, (OptionKey var formattingService = document.GetRequiredLanguageService(); var formattedDocument = await formattingService.FormatNewDocumentAsync(document, hintDocument: null, CancellationToken.None); - // Format to match what AbstractEditorFactory does - formattedDocument = await Formatter.FormatAsync(formattedDocument); - var actual = await formattedDocument.GetTextAsync(); AssertEx.EqualOrDiff(expected, actual.ToString()); } diff --git a/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs b/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs index ab129104e169e..13bc7675275b1 100644 --- a/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs +++ b/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs @@ -42,6 +42,8 @@ public async Task FormatNewDocumentAsync(Document document, Document? try { document = await provider.FormatNewDocumentAsync(document, hintDocument, cancellationToken).ConfigureAwait(false); + + document = await Formatter.FormatAsync(document, cancellationToken: cancellationToken).ConfigureAwait(false); } catch (Exception ex) when (FatalError.ReportAndCatchUnlessCanceled(ex, cancellationToken, ErrorSeverity.General)) { diff --git a/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs b/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs index 547c6fa76bff8..0697ca9343d3c 100644 --- a/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs +++ b/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs @@ -327,13 +327,9 @@ private async Task FormatDocumentCreatedFromTemplateAsync(IVsHierarchy hierarchy addedDocument = await formattingService.FormatNewDocumentAsync(addedDocument, hintDocument: null, cancellationToken).ConfigureAwait(true); } - var rootToFormat = await addedDocument.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(true); - var formattingOptions = await SyntaxFormattingOptions.FromDocumentAsync(addedDocument, cancellationToken).ConfigureAwait(true); - // Format document - var unformattedText = await addedDocument.GetTextAsync(cancellationToken).ConfigureAwait(true); - var formattedRoot = Formatter.Format(rootToFormat, workspace.Services, formattingOptions, cancellationToken); - var formattedText = formattedRoot.GetText(unformattedText.Encoding, unformattedText.ChecksumAlgorithm); + var formattingOptions = await SyntaxFormattingOptions.FromDocumentAsync(addedDocument, cancellationToken).ConfigureAwait(true); + var formattedText = await addedDocument.GetTextAsync(cancellationToken).ConfigureAwait(true); // Ensure the line endings are normalized. The formatter doesn't touch everything if it doesn't need to. var targetLineEnding = formattingOptions.GetOption(FormattingOptions2.NewLine)!; From 2966361f85964e894782cc4eb7aa2dccf332dada Mon Sep 17 00:00:00 2001 From: David Wengier Date: Thu, 27 Jan 2022 11:21:15 +1100 Subject: [PATCH 2/4] Doc --- .../Formatting/AbstractNewDocumentFormattingService.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs b/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs index 13bc7675275b1..d41d4f0d334ef 100644 --- a/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs +++ b/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs @@ -41,8 +41,13 @@ public async Task FormatNewDocumentAsync(Document document, Document? // other, so this shouldn't cause problems. try { + // First we ask the provider to "format" the document. This could be formatting in terms + // of adjusting block scopes to file scopes etc., but it could also be more akin to fixers + // like adding access modifiers, or adding .ConfigureAwait() calls etc. document = await provider.FormatNewDocumentAsync(document, hintDocument, cancellationToken).ConfigureAwait(false); + // Now that the above has changed the document, we need the formatter to make sure its correct + // before we call the next provider, otherwise they might not see things as they are meant to be. document = await Formatter.FormatAsync(document, cancellationToken: cancellationToken).ConfigureAwait(false); } catch (Exception ex) when (FatalError.ReportAndCatchUnlessCanceled(ex, cancellationToken, ErrorSeverity.General)) From f85f16d6b6260cca2fd4b0b32fae612ea468ba26 Mon Sep 17 00:00:00 2001 From: David Wengier Date: Thu, 27 Jan 2022 14:25:53 +1100 Subject: [PATCH 3/4] Call the code action engine to clean up the processed document --- .../Formatting/AbstractNewDocumentFormattingService.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs b/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs index d41d4f0d334ef..8be7bfe176e3e 100644 --- a/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs +++ b/src/Features/Core/Portable/Formatting/AbstractNewDocumentFormattingService.cs @@ -7,8 +7,10 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.ErrorReporting; using Microsoft.CodeAnalysis.Host.Mef; +using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.Formatting { @@ -46,9 +48,11 @@ public async Task FormatNewDocumentAsync(Document document, Document? // like adding access modifiers, or adding .ConfigureAwait() calls etc. document = await provider.FormatNewDocumentAsync(document, hintDocument, cancellationToken).ConfigureAwait(false); - // Now that the above has changed the document, we need the formatter to make sure its correct + // Now that the above has changed the document, we use the code action engine to clean up the document // before we call the next provider, otherwise they might not see things as they are meant to be. - document = await Formatter.FormatAsync(document, cancellationToken: cancellationToken).ConfigureAwait(false); + // Because formatting providers often re-use code fix logic, they are often written assuming this will + // happen. + document = await CodeAction.CleanupDocumentAsync(document, cancellationToken).ConfigureAwait(false); } catch (Exception ex) when (FatalError.ReportAndCatchUnlessCanceled(ex, cancellationToken, ErrorSeverity.General)) { From 2a06512eaa71f7dd73b9636a18f28d84722ba1d9 Mon Sep 17 00:00:00 2001 From: David Wengier Date: Thu, 27 Jan 2022 20:23:42 +1100 Subject: [PATCH 4/4] Revert --- .../Core/Def/Implementation/AbstractEditorFactory.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs b/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs index 0697ca9343d3c..547c6fa76bff8 100644 --- a/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs +++ b/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs @@ -327,9 +327,13 @@ private async Task FormatDocumentCreatedFromTemplateAsync(IVsHierarchy hierarchy addedDocument = await formattingService.FormatNewDocumentAsync(addedDocument, hintDocument: null, cancellationToken).ConfigureAwait(true); } - // Format document + var rootToFormat = await addedDocument.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(true); var formattingOptions = await SyntaxFormattingOptions.FromDocumentAsync(addedDocument, cancellationToken).ConfigureAwait(true); - var formattedText = await addedDocument.GetTextAsync(cancellationToken).ConfigureAwait(true); + + // Format document + var unformattedText = await addedDocument.GetTextAsync(cancellationToken).ConfigureAwait(true); + var formattedRoot = Formatter.Format(rootToFormat, workspace.Services, formattingOptions, cancellationToken); + var formattedText = formattedRoot.GetText(unformattedText.Encoding, unformattedText.ChecksumAlgorithm); // Ensure the line endings are normalized. The formatter doesn't touch everything if it doesn't need to. var targetLineEnding = formattingOptions.GetOption(FormattingOptions2.NewLine)!;