Move the Hover LSP provider to use the new quickinfo service (#1870)

* Move the Hover LSP provider to use the new quickinfo service. * Fixed a bug in the new QuickInfo provider to escape markdown content in the actual text. * Fix tests, don't escape text for a few cases. Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
OmniSharp · Aug 2, 2020 · 52621cb · 52621cb
1 parent 80ab3f0
commit 52621cb
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 33 deletions.
diff --git a/src/OmniSharp.LanguageServerProtocol/Handlers/OmniSharpHoverHandler.cs b/src/OmniSharp.LanguageServerProtocol/Handlers/OmniSharpHoverHandler.cs
@@ -3,11 +3,9 @@
 using System.Threading;
 using System.Threading.Tasks;
 using OmniSharp.Extensions.JsonRpc;
-using OmniSharp.Extensions.LanguageServer.Protocol.Client.Capabilities;
 using OmniSharp.Extensions.LanguageServer.Protocol.Document;
 using OmniSharp.Extensions.LanguageServer.Protocol.Models;
-using OmniSharp.Extensions.LanguageServer.Protocol.Server;
-using OmniSharp.Models.TypeLookup;
+using OmniSharp.Models;
 
 namespace OmniSharp.LanguageServerProtocol.Handlers
 {
@@ -16,14 +14,14 @@ class OmniSharpHoverHandler : HoverHandler
         public static IEnumerable<IJsonRpcHandler> Enumerate(RequestHandlers handlers)
         {
             foreach (var (selector, handler) in handlers
-                .OfType<Mef.IRequestHandler<TypeLookupRequest, TypeLookupResponse>>())
+                .OfType<Mef.IRequestHandler<QuickInfoRequest, QuickInfoResponse>>())
                 if (handler != null)
                     yield return new OmniSharpHoverHandler(handler, selector);
         }
 
-        private readonly Mef.IRequestHandler<TypeLookupRequest, TypeLookupResponse> _definitionHandler;
+        private readonly Mef.IRequestHandler<QuickInfoRequest, QuickInfoResponse> _definitionHandler;
 
-        public OmniSharpHoverHandler(Mef.IRequestHandler<TypeLookupRequest, TypeLookupResponse> definitionHandler, DocumentSelector documentSelector)
+        public OmniSharpHoverHandler(Mef.IRequestHandler<QuickInfoRequest, QuickInfoResponse> definitionHandler, DocumentSelector documentSelector)
             : base(new HoverRegistrationOptions()
             {
                 DocumentSelector = documentSelector
@@ -34,12 +32,11 @@ public OmniSharpHoverHandler(Mef.IRequestHandler<TypeLookupRequest, TypeLookupRe
 
         public async override Task<Hover> Handle(HoverParams request, CancellationToken token)
         {
-            var omnisharpRequest = new TypeLookupRequest()
+            var omnisharpRequest = new QuickInfoRequest()
             {
                 FileName = Helpers.FromUri(request.TextDocument.Uri),
                 Column = Convert.ToInt32(request.Position.Character),
                 Line = Convert.ToInt32(request.Position.Line),
-                IncludeDocumentation = true
             };
 
             var omnisharpResponse = await _definitionHandler.Handle(omnisharpRequest);
@@ -48,7 +45,7 @@ public async override Task<Hover> Handle(HoverParams request, CancellationToken
             {
                 // TODO: Range?  We don't currently have that!
                 // Range =
-                Contents = new MarkedStringsOrMarkupContent(new Container<MarkedString>(Helpers.EscapeMarkdown(omnisharpResponse.Type), Helpers.EscapeMarkdown(omnisharpResponse.Documentation)))
+                Contents = new MarkedStringsOrMarkupContent(new MarkupContent() { Value = omnisharpResponse.Markdown })
             };
         }
     }

diff --git a/src/OmniSharp.LanguageServerProtocol/Helpers.cs b/src/OmniSharp.LanguageServerProtocol/Helpers.cs
@@ -1,7 +1,6 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
-using System.Text.RegularExpressions;
 using OmniSharp.Extensions.LanguageServer.Protocol;
 using OmniSharp.Extensions.LanguageServer.Protocol.Models;
 using OmniSharp.Models;
@@ -122,13 +121,6 @@ public static Range ToRange(OmniSharp.Models.V2.Range range)
             };
         }
 
-        public static string EscapeMarkdown(string markdown)
-        {
-            if (markdown == null)
-                return null;
-            return Regex.Replace(markdown, @"([\\`\*_\{\}\[\]\(\)#+\-\.!])", @"\$1");
-        }
-
         private static readonly IDictionary<string, SymbolKind> Kinds = new Dictionary<string, SymbolKind>
         {
             { OmniSharp.Models.V2.SymbolKinds.Class, SymbolKind.Class },

diff --git a/src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs b/src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs
@@ -9,6 +9,7 @@
 using OmniSharp.Mef;
 using OmniSharp.Models;
 using OmniSharp.Options;
+using OmniSharp.Utilities;
 
 #nullable enable
 
@@ -186,12 +187,12 @@ static void buildSectionAsMarkdown(QuickInfoSection section, StringBuilder strin
                     switch (current.Tag)
                     {
                         case TextTags.Text when !isInCodeBlock:
-                            stringBuilder.Append(current.Text);
+                            addText(current.Text);
                             break;
 
                         case TextTags.Text:
                             endBlock();
-                            stringBuilder.Append(current.Text);
+                            addText(current.Text);
                             break;
 
                         case TextTags.Space when isInCodeBlock:
@@ -203,14 +204,18 @@ static void buildSectionAsMarkdown(QuickInfoSection section, StringBuilder strin
                             stringBuilder.Append(current.Text);
                             break;
 
+                        case TextTags.Punctuation when isInCodeBlock && current.Text != "`":
+                            stringBuilder.Append(current.Text);
+                            break;
+
                         case TextTags.Space:
                         case TextTags.Punctuation:
                             stringBuilder.Append(current.Text);
                             break;
 
                         case ContainerStart:
                             addNewline();
-                            stringBuilder.Append(current.Text);
+                            addText(current.Text);
                             break;
 
                         case ContainerEnd:
@@ -246,6 +251,11 @@ static void buildSectionAsMarkdown(QuickInfoSection section, StringBuilder strin
 
                 return;
 
+                void addText(string text)
+                {
+                    stringBuilder.Append(MarkdownHelpers.Escape(text));
+                }
+
                 void addNewline()
                 {
                     if (isInCodeBlock)

diff --git a/src/OmniSharp.Shared/Utilities/MarkdownHelpers.cs b/src/OmniSharp.Shared/Utilities/MarkdownHelpers.cs
@@ -0,0 +1,16 @@
+using System.Text.RegularExpressions;
+
+namespace OmniSharp.Utilities
+{
+    public static class MarkdownHelpers
+    {
+        private static Regex EscapeRegex = new Regex(@"([\\`\*_\{\}\[\]\(\)#+\-\.!])", RegexOptions.Compiled);
+
+        public static string Escape(string markdown)
+        {
+            if (markdown == null)
+                return null;
+            return EscapeRegex.Replace(markdown, @"\$1");
+        }
+    }
+}
diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/QuickInfoProviderFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/QuickInfoProviderFacts.cs
@@ -77,7 +77,7 @@ public async Task TypesFromInlineAssemlbyReferenceContainDocumentation()
             var controller = new QuickInfoProvider(workspace, new FormattingOptions(), null);
             var response = await controller.Handle(new QuickInfoRequest { FileName = testFile.FileName, Line = position.Line, Column = position.Offset });
 
-            Assert.Equal("```csharp\nclass ClassLibraryWithDocumentation.DocumentedClass\n```\n\nThis class performs an important function.", response.Markdown);
+            Assert.Equal("```csharp\nclass ClassLibraryWithDocumentation.DocumentedClass\n```\n\nThis class performs an important function\\.", response.Markdown);
         }
 
         [Fact]
@@ -310,7 +310,7 @@ class testissue
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nYou may have some additional information about this class here.", response.Markdown);
+            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nYou may have some additional information about this class here\\.", response.Markdown);
         }
 
         [Fact]
@@ -325,7 +325,7 @@ class testissue
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nChecks if object is tagged with the tag.", response.Markdown);
+            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nChecks if object is tagged with the tag\\.", response.Markdown);
         }
 
         [Fact]
@@ -340,7 +340,7 @@ class testissue
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nReturns:\n\n  Returns true if object is tagged with tag.", response.Markdown);
+            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nReturns:\n\n  Returns true if object is tagged with tag\\.", response.Markdown);
         }
 
         [Fact]
@@ -466,7 +466,7 @@ public string Na$$me
 }
 ";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nstring Employee.Name { }\n```\n\nThe Name property represents the employee's name.\n\nValue:\n\n  The Name property gets/sets the value of the string field, _name.", response.Markdown);
+            Assert.Equal("```csharp\nstring Employee.Name { }\n```\n\nThe Name property represents the employee's name\\.\n\nValue:\n\n  The Name property gets/sets the value of the string field, \\_name\\.", response.Markdown);
         }
 
         [Fact]
@@ -481,7 +481,7 @@ public class TestClass
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class. `System.Console.WriteLine(string)` for information about output statements.", response.Markdown);
+            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class\\. `System.Console.WriteLine(string)` for information about output statements\\.", response.Markdown);
         }
 
         [Fact]
@@ -542,7 +542,7 @@ public class TestClass
 }
             ";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class.\n\n\n\nHere's how you could make a second paragraph in a description.", response.Markdown);
+            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class\\.\n\n\n\nHere's how you could make a second paragraph in a description\\.", response.Markdown);
         }
 
         [Fact]
@@ -563,7 +563,7 @@ static void Main()
             }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class. `TestClass.Main()`", response.Markdown);
+            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class\\. `TestClass.Main()`", response.Markdown);
         }
 
         [Fact]
@@ -580,7 +580,7 @@ class testissue
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nChecks if object is tagged with the tag.", response.Markdown);
+            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nChecks if object is tagged with the tag\\.", response.Markdown);
         }
 
         [Fact]
@@ -602,7 +602,7 @@ class testissue
 }";
             var response = await GetTypeLookUpResponse(content);
             Assert.Equal(
-                "```csharp\nT[] testissue.Compare(int gameObject)\n```\n\nChecks if object is tagged with the tag.\n\nYou may have some additional information about this class here.\n\nReturns:\n\n  Returns an array of type `T`.\n\n\n\nExceptions:\n\n  `System.Exception`",
+                "```csharp\nT[] testissue.Compare(int gameObject)\n```\n\nChecks if object is tagged with the tag\\.\n\nYou may have some additional information about this class here\\.\n\nReturns:\n\n  Returns an array of type `T`\\.\n\n\n\nExceptions:\n\n  `System.Exception`",
                 response.Markdown);
         }
 
@@ -618,7 +618,7 @@ public class TestClass
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class.", response.Markdown);
+            Assert.Equal("```csharp\nvoid TestClass.DoWork(int Int1)\n```\n\nDoWork is a method in the TestClass class\\.", response.Markdown);
         }
 
         [Fact]
@@ -634,7 +634,7 @@ public static bool Compare(int gam$$eObject, string tagName)
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\n(parameter) int gameObject\n```\n\nThe game object.", response.Markdown);
+            Assert.Equal("```csharp\n(parameter) int gameObject\n```\n\nThe game object\\.", response.Markdown);
         }
 
         [Fact]
@@ -650,7 +650,7 @@ public static bool Compare(int gameObject, string tag$$Name)
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\n(parameter) string tagName\n```\n\nName of the tag.", response.Markdown);
+            Assert.Equal("```csharp\n(parameter) string tagName\n```\n\nName of the tag\\.", response.Markdown);
         }
 
         [Fact]
@@ -692,6 +692,25 @@ public static void Main()
             Assert.Equal("```csharp\nvoid Program.B()\n```\n\nHello World", response.Markdown);
         }
 
+        [Fact]
+        public async Task MarkdownInComment()
+        {
+            string content = @"
+class Program
+{
+	/// <summary>*This should be escaped*</summary>
+	public static void B() { }
+
+    public static void A()
+    {
+        B$$();
+    }
+
+}";
+            var response = await GetTypeLookUpResponse(content);
+            Assert.Equal("```csharp\nvoid Program.B()\n```\n\n\\*This should be escaped\\*", response.Markdown);
+        }
+
         private async Task<QuickInfoResponse> GetTypeLookUpResponse(string content)
         {
             TestFile testFile = new TestFile("dummy.cs", content);