From a109f92991b955959e064c9c5526e2da7d4c816c Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Mon, 23 Nov 2020 01:06:42 -0800 Subject: [PATCH] Add documentation comment creation to the FormatAfterKeystrokeService IDocumentationCommentSnippetService was recently moved down to the Features layer, which means omnisharp can now take advantage of it and generate documentation comment snippets on typing. There are a couple of caveats: 1. LSP's "/onTypingFormat" endpoint has no support for setting the cursor location after format, which means that the cursor ends up after the inserted block. This is fine for newlines inside doc comments, but awkward for the initial expansion. However, unless we want to add special support with a custom endpoint, we can't get around this (the custom endpoint is how Roslyn does it). 2. The service itself is still internal, so I had to add a few new reflection points. Server-side of https://github.com/OmniSharp/omnisharp-vscode/issues/8. It also needs a vscode change, but it's a one-line change I'll submit later after we get this merged and I can write some integration tests. --- .../DocumentationCommentSnippetService.cs | 110 +++++ .../Workers/Formatting/FormattingWorker.cs | 54 ++- .../FormatAfterKeystrokeTests.cs | 414 ++++++++++++++++++ tests/TestUtility/AssertUtils.cs | 23 +- 4 files changed, 597 insertions(+), 4 deletions(-) create mode 100644 src/OmniSharp.Roslyn.CSharp/DocumentationComments/DocumentationCommentSnippetService.cs create mode 100644 tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs diff --git a/src/OmniSharp.Roslyn.CSharp/DocumentationComments/DocumentationCommentSnippetService.cs b/src/OmniSharp.Roslyn.CSharp/DocumentationComments/DocumentationCommentSnippetService.cs new file mode 100644 index 0000000000..8b72c8669c --- /dev/null +++ b/src/OmniSharp.Roslyn.CSharp/DocumentationComments/DocumentationCommentSnippetService.cs @@ -0,0 +1,110 @@ +#nullable enable + +using System; +using System.Reflection; +using System.Threading; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Completion; +using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.Options; +using Microsoft.CodeAnalysis.Text; + +namespace OmniSharp.Roslyn.DocumentationComments +{ + /// + /// Proxy service for Microsoft.CodeAnalysis.DocumentationComments.IDocumentationCommentSnippetService. + /// Implementation was based on the service as of this commit: 2834b74995bb66a7cb19cb09069c17812819afdc + /// See: https://github.com/dotnet/roslyn/blob/2834b74995bb66a7cb19cb09069c17812819afdc/src/Features/Core/Portable/DocumentationComments/IDocumentationCommentSnippetService.cs + /// + public struct DocumentationCommentSnippetService + { + /// + /// IDocumentationCommentService HostLanguageServices.GetRequiredService() + /// + private static MethodInfo s_getRequiredService; + /// + /// DocumentationCommentSnippet IDocumentationCommentService.GetDocumentationCommentSnippetOnCharacterTyped(SyntaxTree, SourceText, int, DocumentOptionSet, CancellationToken) + /// + private static MethodInfo s_getDocumentationCommentSnippetOnCharacterTyped; + /// + /// DocumentationCommentSnippet IDocumentationCommentService.GetDocumentationCommentSnippetOnEnterTyped(SyntaxTree, SourceText, int, DocumentOptionSet, CancellationToken) + /// + private static MethodInfo s_getDocumentationCommentSnippetOnEnterTyped; + /// + /// TextSpan DocumentationCommentSnippet.SpanToReplace + /// + private static PropertyInfo s_spanToReplace; + /// + /// string DocumentationCommentSnippet.SnippetText + /// + private static PropertyInfo s_snippetText; + + static DocumentationCommentSnippetService() + { + var iDocumentationCommentSnippetServiceType = typeof(CompletionItem).Assembly.GetType("Microsoft.CodeAnalysis.DocumentationComments.IDocumentationCommentSnippetService"); + s_getDocumentationCommentSnippetOnCharacterTyped = iDocumentationCommentSnippetServiceType.GetMethod(nameof(GetDocumentationCommentSnippetOnCharacterTyped)); + s_getDocumentationCommentSnippetOnEnterTyped = iDocumentationCommentSnippetServiceType.GetMethod(nameof(GetDocumentationCommentSnippetOnEnterTyped)); + + var documentationCommentSnippet = typeof(CompletionItem).Assembly.GetType("Microsoft.CodeAnalysis.DocumentationComments.DocumentationCommentSnippet"); + s_spanToReplace = documentationCommentSnippet.GetProperty(nameof(DocumentationCommentSnippet.SpanToReplace)); + s_snippetText = documentationCommentSnippet.GetProperty(nameof(DocumentationCommentSnippet.SnippetText)); + + s_getRequiredService = typeof(HostLanguageServices).GetMethod(nameof(HostLanguageServices.GetRequiredService)).MakeGenericMethod(iDocumentationCommentSnippetServiceType); + } + + public static DocumentationCommentSnippetService GetDocumentationCommentSnippetService(Document document) + { + var service = s_getRequiredService.Invoke(document.Project.LanguageServices, Array.Empty()); + return new DocumentationCommentSnippetService(service); + } + + private object _underlying; + + private DocumentationCommentSnippetService(object underlying) + { + _underlying = underlying; + } + + public DocumentationCommentSnippet? GetDocumentationCommentSnippetOnCharacterTyped(SyntaxTree syntaxTree, SourceText text, int position, DocumentOptionSet options, CancellationToken cancellationToken) + { + var originalSnippet = s_getDocumentationCommentSnippetOnCharacterTyped.Invoke(_underlying, new object[] { syntaxTree, text, position, options, cancellationToken }); + return ConvertSnippet(originalSnippet); + } + + public DocumentationCommentSnippet? GetDocumentationCommentSnippetOnEnterTyped(SyntaxTree syntaxTree, SourceText text, int position, DocumentOptionSet options, CancellationToken cancellationToken) + { + var originalSnippet = s_getDocumentationCommentSnippetOnEnterTyped.Invoke(_underlying, new object[] { syntaxTree, text, position, options, cancellationToken }); + return ConvertSnippet(originalSnippet); + } + + private static DocumentationCommentSnippet? ConvertSnippet(object? originalSnippet) + { + if (originalSnippet == null) + { + return null; + } + else + { + return new DocumentationCommentSnippet((TextSpan)s_spanToReplace.GetValue(originalSnippet), (string)s_snippetText.GetValue(originalSnippet)); + } + } + } + + public struct DocumentationCommentSnippet + { + public TextSpan SpanToReplace { get; } + public string SnippetText { get; } + + public DocumentationCommentSnippet(TextSpan spanToReplace, string snippetText) + { + SpanToReplace = spanToReplace; + SnippetText = snippetText; + } + } + + public static class WorkspaceExtensions + { + public static DocumentationCommentSnippetService GetDocumentationCommentSnippetService(this Document document) + => DocumentationCommentSnippetService.GetDocumentationCommentSnippetService(document); + } +} diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs index 0c7d4e9b02..17cd5405a2 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs @@ -1,13 +1,19 @@ +#nullable enable + using System.Collections.Generic; +using System.Diagnostics; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.Text; using Microsoft.Extensions.Logging; +using OmniSharp.Extensions; using OmniSharp.Models; using OmniSharp.Options; +using OmniSharp.Roslyn.DocumentationComments; using OmniSharp.Roslyn.Utilities; namespace OmniSharp.Roslyn.CSharp.Workers.Formatting @@ -16,13 +22,19 @@ public static class FormattingWorker { public static async Task> GetFormattingChangesAfterKeystroke(Document document, int position, char character, OmniSharpOptions omnisharpOptions, ILoggerFactory loggerFactory) { + if (await GetDocumentationCommentChanges(document, position, character, omnisharpOptions) is LinePositionSpanTextChange change) + { + return new[] { change }; + } + if (character == '\n') { // format previous line on new line var text = await document.GetTextAsync(); var lines = text.Lines; var targetLine = lines[lines.GetLineFromPosition(position).LineNumber - 1]; - if (!string.IsNullOrWhiteSpace(targetLine.Text.ToString(targetLine.Span))) + Debug.Assert(targetLine.Text != null); + if (!string.IsNullOrWhiteSpace(targetLine.Text!.ToString(targetLine.Span))) { return await GetFormattingChanges(document, targetLine.Start, targetLine.End, omnisharpOptions, loggerFactory); } @@ -31,7 +43,8 @@ public static async Task> GetFormattingC { // format after ; and } var root = await document.GetSyntaxRootAsync(); - var node = FindFormatTarget(root, position); + Debug.Assert(root != null); + var node = FindFormatTarget(root!, position); if (node != null) { return await GetFormattingChanges(document, node.FullSpan.Start, node.FullSpan.End, omnisharpOptions, loggerFactory); @@ -41,7 +54,7 @@ public static async Task> GetFormattingC return Enumerable.Empty(); } - public static SyntaxNode FindFormatTarget(SyntaxNode root, int position) + public static SyntaxNode? FindFormatTarget(SyntaxNode root, int position) { // todo@jo - refine this var token = root.FindToken(position); @@ -112,5 +125,40 @@ private static async Task FormatDocument(Document document, OmniSharpO return newDocument; } + + private static async Task GetDocumentationCommentChanges(Document document, int position, char character, OmniSharpOptions omnisharpOptions) + { + if (character != '\n' && character != '/') + { + return null; + } + + var text = await document.GetTextAsync(); + var syntaxTree = await document.GetSyntaxTreeAsync(); + + var optionSet = await document.GetOptionsAsync(); + + var documentationService = document.GetDocumentationCommentSnippetService(); + var snippet = character == '\n' ? + documentationService.GetDocumentationCommentSnippetOnEnterTyped(syntaxTree!, text, position, optionSet, CancellationToken.None) : + documentationService.GetDocumentationCommentSnippetOnCharacterTyped(syntaxTree!, text, position, optionSet, CancellationToken.None); + + if (snippet == null) + { + return null; + } + else + { + var range = text.GetRangeFromSpan(snippet.Value.SpanToReplace); + return new LinePositionSpanTextChange + { + NewText = snippet.Value.SnippetText, + StartLine = range.Start.Line, + StartColumn = range.Start.Column, + EndLine = range.End.Line, + EndColumn = range.End.Column + }; + } + } } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs new file mode 100644 index 0000000000..e87ca36993 --- /dev/null +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs @@ -0,0 +1,414 @@ +#nullable enable + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Formatting; +using OmniSharp.Models.Format; +using OmniSharp.Models.UpdateBuffer; +using OmniSharp.Roslyn.CSharp.Services.Buffer; +using OmniSharp.Roslyn.CSharp.Services.Formatting; +using TestUtility; +using Xunit; +using Xunit.Abstractions; + +namespace OmniSharp.Roslyn.CSharp.Tests +{ + public class FormatAfterKeystrokeTests : AbstractTestFixture + { + public FormatAfterKeystrokeTests(ITestOutputHelper output, SharedOmniSharpHostFixture sharedOmniSharpHostFixture) + : base(output, sharedOmniSharpHostFixture) + { + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnEnter(string fileName) + { + await VerifyNoChange(fileName, "\n", +@"class C +{ +$$ +}"); + + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnSingleForwardSlash(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + /$$ +} +"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnDoubleForwardSlash(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + //$$ +} +"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnTripleForwardSlash_NoMember(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + ///$$ +} +"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnTripleForwardSlash_BeforeMethod(string fileName) + { + await VerifyChange(fileName, "/", +@"class C +{ + ///$$ + public string M(string param1, int param2) { } +}", +@"class C +{ + /// + /// + /// + /// + /// + /// + /// + public string M(string param1, int param2) { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnTripleForwardSlash_BeforeType(string fileName) + { + await VerifyChange(fileName, "/", +@"///$$ +class C +{ + public string M(string param1, int param2) { } +}", +@"/// +/// +/// +class C +{ + public string M(string param1, int param2) { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnTripleForwardSlash_BeforeProperty(string fileName) + { + await VerifyChange(fileName, "/", +@"class C +{ + ///$$ + public int Prop { get; set; } +}", +@"class C +{ + /// + /// + /// + public int Prop { get; set; } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnTripleForwardSlash_BeforeIndexer(string fileName) + { + await VerifyChange(fileName, "/", +@"class C +{ + ///$$ + public int this[int i] { get; set; } +}", +@"class C +{ + /// + /// + /// + /// + /// + public int this[int i] { get; set; } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnEnterInComment_BetweenTags_Newline(string fileName) + { + await VerifyChange(fileName, "\n", +@"class C +{ + /// + /// +$$ + /// + /// + /// + /// + public string M(string param1, int param2) { } +}", +@"class C +{ + /// + /// + /// + /// + /// + /// + /// + public string M(string param1, int param2) { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnEnterInComment_BetweenTags_SameLine(string fileName) + { + await VerifyChange(fileName, "\n", +@"class C +{ + /// + /// + /// + /// +$$ + /// + /// + public string M(string param1, int param2) { } +}", +@"class C +{ + /// + /// + /// + /// + /// + /// + /// + public string M(string param1, int param2) { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnEnterInComment_AfterTags(string fileName) + { + await VerifyChange(fileName, "\n", +@"class C +{ + /// + /// + /// + /// + /// + /// +$$ + public string M(string param1, int param2) { } +}", +@"class C +{ + /// + /// + /// + /// + /// + /// + /// + public string M(string param1, int param2) { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnTripleForwardSlash_VerbatimNames(string fileName) + { + await VerifyChange(fileName, "/", +@"class C +{ + ///$$ + public string M<@int>(string @float) { } +}", +@"class C +{ + /// + /// + /// + /// + /// + /// + public string M<@int>(string @float) { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task Comment_OnTripleForwardSlash_VoidMethod(string fileName) + { + await VerifyChange(fileName, "/", +@"class C +{ + ///$$ + public void M() { } +}", +@"class C +{ + /// + /// + /// + public void M() { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnTripleForwardSlash_ExistingLineAbove(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + /// + ///$$ + public void M() { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnTripleForwardSlash_ExistingLineBelow_01(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + ///$$ + /// + public void M() { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnTripleForwardSlash_ExistingLineBelow_02(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + ///$$ + /// + public void M() { } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnTripleForwardSlash_InsideMethodBody(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + public void M() + { + ///$$ + } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task NoComment_OnNewLine_InsideMethodBody(string fileName) + { + await VerifyNoChange(fileName, "/", +@"class C +{ + public void M() + { + /// +$$ + } +}"); + } + + private async Task VerifyNoChange(string fileName, string typedCharacter, string originalMarkup) + { + var (response, _) = await GetResponse(originalMarkup, typedCharacter, fileName); + Assert.Empty(response.Changes); + } + + private async Task VerifyChange(string fileName, string typedCharacter, string originalMarkup, string expected) + { + var (response, testFile) = await GetResponse(originalMarkup, typedCharacter, fileName); + Assert.NotNull(response); + + var fileChangedService = SharedOmniSharpTestHost.GetRequestHandler(OmniSharpEndpoints.UpdateBuffer); + _ = await fileChangedService.Handle(new UpdateBufferRequest() + { + FileName = testFile.FileName, + Changes = response.Changes, + ApplyChangesTogether = true, + }); + + var actualDoc = SharedOmniSharpTestHost.Workspace.GetDocument(testFile.FileName); + Assert.NotNull(actualDoc); + var actualText = (await actualDoc.GetTextAsync()).ToString(); + AssertUtils.Equal(expected, actualText); + } + + private async Task<(FormatRangeResponse, TestFile)> GetResponse(string text, string character, string fileName) + { + // Ensure system newlines are used + var options = SharedOmniSharpTestHost.Workspace.Options.WithChangedOption(FormattingOptions.NewLine, LanguageNames.CSharp, System.Environment.NewLine); + SharedOmniSharpTestHost.Workspace.TryApplyChanges(SharedOmniSharpTestHost.Workspace.CurrentSolution.WithOptions(options)); + + var file = new TestFile(fileName, text); + SharedOmniSharpTestHost.AddFilesToWorkspace(file); + var point = file.Content.GetPointFromPosition(); + + var request = new FormatAfterKeystrokeRequest + { + Line = point.Line, + Column = point.Offset, + FileName = fileName, + Character = character, + }; + + var requestHandler = SharedOmniSharpTestHost.GetRequestHandler(OmniSharpEndpoints.FormatAfterKeystroke); + return (await requestHandler.Handle(request), file); + } + } +} diff --git a/tests/TestUtility/AssertUtils.cs b/tests/TestUtility/AssertUtils.cs index f67f32d2ad..02f1b06b24 100644 --- a/tests/TestUtility/AssertUtils.cs +++ b/tests/TestUtility/AssertUtils.cs @@ -1,4 +1,6 @@ +using System; using System.Linq; +using System.Text; using Xunit; namespace TestUtility @@ -24,5 +26,24 @@ private static string TrimLines(string source) { return string.Join("\n", source.Split('\n').Select(s => s.Trim())); } + + // Taken from dotnet/roslyn, MIT License. + // https://github.com/dotnet/roslyn/blob/2834b74995bb66a7cb19cb09069c17812819afdc/src/Compilers/Test/Core/Assert/AssertEx.cs#L188-L203 + public static void Equal(string expected, string actual) + { + if (string.Equals(expected, actual, StringComparison.Ordinal)) + { + return; + } + + var message = new StringBuilder(); + message.AppendLine(); + message.AppendLine("Expected:"); + message.AppendLine(expected); + message.AppendLine("Actual:"); + message.AppendLine(actual); + + Assert.True(false, message.ToString()); + } } -} \ No newline at end of file +}