Add support for a "Show Documentation" quick fix menu entry (#789)

* Add support for a "Show Documentation" quick fix menu entry

This requires a corresponding PR to VSCode to handle the new command:
PowerShell.ShowCodeActionDocumentation

Changed the use of the Diagnostic.Code field to hold the PSSA rule name.
According to the LSP docs, this field is meant to be displayed and
tslint uses it to display the vioated rule. Update the logic to use a
combination of diagnostic fields to get a unique id in order to
retrieve the stored "correction edits" when the
textDocument/codeAction message is processed with the diagnostic
array returned earlier.

Also, observed a few times during startup that RunScriptDiagnostics was
getting called with an empty filesToAnalyze array.  Modified to return
early in that case.

Remove a private overload of DelayThenInvokeDiagnostics that wasn't
being used.

* Address PR comments by switching to StringBuilder

Also update codeAction processing to ensure we don't have multiple
show documentation commands for the same rule.

* Remove extra filesToAnalyze empty array check per PR feedback

* Address PR feedback on StringBuilder

* Fix indentation issue as per PR feedback

Author: rkeithhill

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

@@ -17,6 +17,7 @@
 using System.Linq;
 using System.Management.Automation;
 using System.Management.Automation.Language;
+using System.Text;
 using System.Text.RegularExpressions;
 using System.Threading;
 using System.Threading.Tasks;
@@ -27,7 +28,7 @@ namespace Microsoft.PowerShell.EditorServices.Protocol.Server
 {
     public class LanguageServer
     {
-        private static CancellationTokenSource existingRequestCancellation;
+        private static CancellationTokenSource s_existingRequestCancellation;
 
         private static readonly Location[] s_emptyLocationResult = new Location[0];
 
@@ -48,6 +49,7 @@ public class LanguageServer
         private LanguageServerEditorOperations editorOperations;
         private LanguageServerSettings currentSettings = new LanguageServerSettings();
 
+        // The outer key is the file's uri, the inner key is a unique id for the diagnostic
         private Dictionary<string, Dictionary<string, MarkerCorrection>> codeActionsPerFile =
             new Dictionary<string, Dictionary<string, MarkerCorrection>>();
 
@@ -1182,6 +1184,7 @@ private bool IsQueryMatch(string query, string symbolName)
             return symbolName.IndexOf(query, StringComparison.OrdinalIgnoreCase) >= 0;
         }
 
+        // https://microsoft.github.io/language-server-protocol/specification#textDocument_codeAction
         protected async Task HandleCodeActionRequest(
             CodeActionParams codeActionParams,
             RequestContext<CodeActionCommand[]> requestContext)
@@ -1190,12 +1193,22 @@ protected async Task HandleCodeActionRequest(
             Dictionary<string, MarkerCorrection> markerIndex = null;
             List<CodeActionCommand> codeActionCommands = new List<CodeActionCommand>();
 
+            // If there are any code fixes, send these commands first so they appear at top of "Code Fix" menu in the client UI.
             if (this.codeActionsPerFile.TryGetValue(codeActionParams.TextDocument.Uri, out markerIndex))
             {
                 foreach (var diagnostic in codeActionParams.Context.Diagnostics)
                 {
-                    if (!string.IsNullOrEmpty(diagnostic.Code) &&
-                        markerIndex.TryGetValue(diagnostic.Code, out correction))
+                    if (string.IsNullOrEmpty(diagnostic.Code))
+                    {
+                        this.Logger.Write(
+                            LogLevel.Warning,
+                            $"textDocument/codeAction skipping diagnostic with empty Code field: {diagnostic.Source} {diagnostic.Message}");
+
+                        continue;
+                    }
+
+                    string diagnosticId = GetUniqueIdFromDiagnostic(diagnostic);
+                    if (markerIndex.TryGetValue(diagnosticId, out correction))
                     {
                         codeActionCommands.Add(
                             new CodeActionCommand
@@ -1208,8 +1221,30 @@ protected async Task HandleCodeActionRequest(
                 }
             }
 
-            await requestContext.SendResult(
-                codeActionCommands.ToArray());
+            // Add "show documentation" commands last so they appear at the bottom of the client UI. 
+            // These commands do not require code fixes. Sometimes we get a batch of diagnostics
+            // to create commands for. No need to create multiple show doc commands for the same rule.
+            var ruleNamesProcessed = new HashSet<string>();
+            foreach (var diagnostic in codeActionParams.Context.Diagnostics)
+            {
+                if (string.IsNullOrEmpty(diagnostic.Code)) { continue; }
+
+                if (string.Equals(diagnostic.Source, "PSScriptAnalyzer", StringComparison.OrdinalIgnoreCase) &&
+                    !ruleNamesProcessed.Contains(diagnostic.Code))
+                {
+                    ruleNamesProcessed.Add(diagnostic.Code);
+
+                    codeActionCommands.Add(
+                        new CodeActionCommand
+                        {
+                            Title = $"Show documentation for \"{diagnostic.Code}\"",
+                            Command = "PowerShell.ShowCodeActionDocumentation",
+                            Arguments = JArray.FromObject(new[] { diagnostic.Code })
+                        });
+                }
+            }
+
+            await requestContext.SendResult(codeActionCommands.ToArray());
         }
 
         protected async Task HandleDocumentFormattingRequest(
@@ -1454,15 +1489,15 @@ private Task RunScriptDiagnostics(
             // If there's an existing task, attempt to cancel it
             try
             {
-                if (existingRequestCancellation != null)
+                if (s_existingRequestCancellation != null)
                 {
                     // Try to cancel the request
-                    existingRequestCancellation.Cancel();
+                    s_existingRequestCancellation.Cancel();
 
                     // If cancellation didn't throw an exception,
                     // clean up the existing token
-                    existingRequestCancellation.Dispose();
-                    existingRequestCancellation = null;
+                    s_existingRequestCancellation.Dispose();
+                    s_existingRequestCancellation = null;
                 }
             }
             catch (Exception e)
@@ -1479,11 +1514,17 @@ private Task RunScriptDiagnostics(
                 return cancelTask.Task;
             }
 
+            // If filesToAnalzye is empty, nothing to do so return early.
+            if (filesToAnalyze.Length == 0)
+            {
+                return Task.FromResult(true);
+            }
+
             // Create a fresh cancellation token and then start the task.
             // We create this on a different TaskScheduler so that we
             // don't block the main message loop thread.
             // TODO: Is there a better way to do this?
-            existingRequestCancellation = new CancellationTokenSource();
+            s_existingRequestCancellation = new CancellationTokenSource();
             Task.Factory.StartNew(
                 () =>
                     DelayThenInvokeDiagnostics(
@@ -1494,36 +1535,14 @@ private Task RunScriptDiagnostics(
                         editorSession,
                         eventSender,
                         this.Logger,
-                        existingRequestCancellation.Token),
+                        s_existingRequestCancellation.Token),
                 CancellationToken.None,
                 TaskCreationOptions.None,
                 TaskScheduler.Default);
 
             return Task.FromResult(true);
         }
 
-        private static async Task DelayThenInvokeDiagnostics(
-            int delayMilliseconds,
-            ScriptFile[] filesToAnalyze,
-            bool isScriptAnalysisEnabled,
-            Dictionary<string, Dictionary<string, MarkerCorrection>> correctionIndex,
-            EditorSession editorSession,
-            EventContext eventContext,
-            ILogger Logger,
-            CancellationToken cancellationToken)
-        {
-            await DelayThenInvokeDiagnostics(
-                delayMilliseconds,
-                filesToAnalyze,
-                isScriptAnalysisEnabled,
-                correctionIndex,
-                editorSession,
-                eventContext.SendEvent,
-                Logger,
-                cancellationToken);
-        }
-
-
         private static async Task DelayThenInvokeDiagnostics(
             int delayMilliseconds,
             ScriptFile[] filesToAnalyze,
@@ -1573,6 +1592,7 @@ private static async Task DelayThenInvokeDiagnostics(
 
                 await PublishScriptDiagnostics(
                     scriptFile,
+                    // Concat script analysis errors to any existing parse errors
                     scriptFile.SyntaxMarkers.Concat(semanticMarkers).ToArray(),
                     correctionIndex,
                     eventSender);
@@ -1620,7 +1640,8 @@ private static async Task PublishScriptDiagnostics(
                 Diagnostic markerDiagnostic = GetDiagnosticFromMarker(marker);
                 if (marker.Correction != null)
                 {
-                    fileCorrections.Add(markerDiagnostic.Code, marker.Correction);
+                    string diagnosticId = GetUniqueIdFromDiagnostic(markerDiagnostic);
+                    fileCorrections.Add(diagnosticId, marker.Correction);
                 }
 
                 diagnostics.Add(markerDiagnostic);
@@ -1639,13 +1660,39 @@ await eventSender(
                 });
         }
 
+        // Generate a unique id that is used as a key to look up the associated code action (code fix) when 
+        // we receive and process the textDocument/codeAction message.
+        private static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic)
+        {
+            Position start = diagnostic.Range.Start;
+            Position end = diagnostic.Range.End;
+
+            var sb = new StringBuilder(256)
+            .Append(diagnostic.Source ?? "?")
+            .Append("_")
+            .Append(diagnostic.Code ?? "?")
+            .Append("_")
+            .Append(diagnostic.Severity?.ToString() ?? "?")
+            .Append("_")
+            .Append(start.Line)
+            .Append(":")
+            .Append(start.Character)
+            .Append("-")
+            .Append(end.Line)
+            .Append(":")
+            .Append(end.Character);
+
+            var id = sb.ToString();
+            return id;
+        }
+
         private static Diagnostic GetDiagnosticFromMarker(ScriptFileMarker scriptFileMarker)
         {
             return new Diagnostic
             {
                 Severity = MapDiagnosticSeverity(scriptFileMarker.Level),
                 Message = scriptFileMarker.Message,
-                Code = scriptFileMarker.Source + Guid.NewGuid().ToString(),
+                Code = scriptFileMarker.RuleName,
                 Source = scriptFileMarker.Source,
                 Range = new Range
                 {

src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs

@@ -62,6 +62,11 @@ public class ScriptFileMarker
         /// Gets or sets the marker's message string.
         /// </summary>
         public string Message { get; set; }
+        
+        /// <summary>
+        /// Gets or sets the ruleName associated with this marker.
+        /// </summary>
+        public string RuleName { get; set; }
 
         /// <summary>
         /// Gets or sets the marker's message level.
@@ -130,7 +135,6 @@ internal static ScriptFileMarker FromDiagnosticRecord(PSObject psObject)
             // the diagnostic record's properties directly i.e. <instance>.<propertyName>
             // without having to go through PSObject's Members property.
             var diagnosticRecord = psObject as dynamic;
-            string ruleName = diagnosticRecord.RuleName as string;
 
             if (diagnosticRecord.SuggestedCorrections != null)
             {
@@ -160,7 +164,8 @@ internal static ScriptFileMarker FromDiagnosticRecord(PSObject psObject)
 
             return new ScriptFileMarker
             {
-                Message = $"{diagnosticRecord.Message as string} ({ruleName})",
+                Message = $"{diagnosticRecord.Message as string}",
+                RuleName = $"{diagnosticRecord.RuleName as string}",
                 Level = GetMarkerLevelFromDiagnosticSeverity((diagnosticRecord.Severity as Enum).ToString()),
                 ScriptRegion = ScriptRegion.Create(diagnosticRecord.Extent as IScriptExtent),
                 Correction = correction,