From 7c2cc1fb93a3dac61aef279c5c8dbbc52a3769ed Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Sun, 4 Nov 2018 16:47:44 -0700 Subject: [PATCH 1/5] 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. --- .../Server/LanguageServer.cs | 89 ++++++++++++------- .../Workspace/ScriptFileMarker.cs | 9 +- 2 files changed, 63 insertions(+), 35 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index ab3e7a040..c2a869f28 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -27,7 +27,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 +48,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> codeActionsPerFile = new Dictionary>(); @@ -1182,6 +1183,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 requestContext) @@ -1194,8 +1196,17 @@ protected async Task HandleCodeActionRequest( { 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 @@ -1205,6 +1216,17 @@ protected async Task HandleCodeActionRequest( Arguments = JArray.FromObject(correction.Edits) }); } + + if (string.Equals(diagnostic.Source, "PSScriptAnalyzer", StringComparison.OrdinalIgnoreCase)) + { + codeActionCommands.Add( + new CodeActionCommand + { + Title = $"Show documentation for \"{diagnostic.Code}\"", + Command = "PowerShell.ShowCodeActionDocumentation", + Arguments = JArray.FromObject(new[] { diagnostic.Code }) + }); + } } } @@ -1454,15 +1476,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 +1501,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,7 +1522,7 @@ private Task RunScriptDiagnostics( editorSession, eventSender, this.Logger, - existingRequestCancellation.Token), + s_existingRequestCancellation.Token), CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); @@ -1502,28 +1530,6 @@ private Task RunScriptDiagnostics( return Task.FromResult(true); } - private static async Task DelayThenInvokeDiagnostics( - int delayMilliseconds, - ScriptFile[] filesToAnalyze, - bool isScriptAnalysisEnabled, - Dictionary> 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, @@ -1534,6 +1540,12 @@ private static async Task DelayThenInvokeDiagnostics( ILogger Logger, CancellationToken cancellationToken) { + // If filesToAnalzye is empty, nothing to do so return early. + if (filesToAnalyze.Length == 0) + { + return; + } + // First of all, wait for the desired delay period before // analyzing the provided list of files try @@ -1620,7 +1632,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 +1652,23 @@ await eventSender( }); } + private static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic) + { + string source = diagnostic.Source ?? "?"; + string code = diagnostic.Code ?? "?"; + string severity = diagnostic.Severity != null ? diagnostic.Severity.ToString() : "?"; + Position start = diagnostic.Range.Start; + Position end = diagnostic.Range.End; + return $"{source}_{code}_{severity}_{start.Line}:{start.Character}-{end.Line}:{end.Character}"; + } + 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 { diff --git a/src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs b/src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs index c3ee44026..3737b7169 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs @@ -62,6 +62,11 @@ public class ScriptFileMarker /// Gets or sets the marker's message string. /// public string Message { get; set; } + + /// + /// Gets or sets the ruleName associated with this marker. + /// + public string RuleName { get; set; } /// /// 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. . // 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, From ede686653bde4b6f31db7224ed579f87aa0313e7 Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Wed, 7 Nov 2018 23:08:53 -0700 Subject: [PATCH 2/5] 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. --- .../Server/LanguageServer.cs | 60 ++++++++++++++----- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index c2a869f28..4de2aa90a 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/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; @@ -1192,6 +1193,7 @@ protected async Task HandleCodeActionRequest( Dictionary markerIndex = null; List codeActionCommands = new List(); + // 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) @@ -1216,22 +1218,34 @@ protected async Task HandleCodeActionRequest( Arguments = JArray.FromObject(correction.Edits) }); } + } + } - if (string.Equals(diagnostic.Source, "PSScriptAnalyzer", StringComparison.OrdinalIgnoreCase)) - { - codeActionCommands.Add( - new CodeActionCommand - { - Title = $"Show documentation for \"{diagnostic.Code}\"", - Command = "PowerShell.ShowCodeActionDocumentation", - Arguments = JArray.FromObject(new[] { diagnostic.Code }) - }); - } + // 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(); + 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()); + codeActionCommands.ToArray()); } protected async Task HandleDocumentFormattingRequest( @@ -1585,6 +1599,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); @@ -1652,14 +1667,29 @@ 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) { - string source = diagnostic.Source ?? "?"; - string code = diagnostic.Code ?? "?"; - string severity = diagnostic.Severity != null ? diagnostic.Severity.ToString() : "?"; Position start = diagnostic.Range.Start; Position end = diagnostic.Range.End; - return $"{source}_{code}_{severity}_{start.Line}:{start.Character}-{end.Line}:{end.Character}"; + + var sb = new StringBuilder(256); + sb.Append(diagnostic.Source ?? "?"); + sb.Append("_"); + sb.Append(diagnostic.Code ?? "?"); + sb.Append("_"); + sb.Append((diagnostic.Severity != null) ? diagnostic.Severity.ToString() : "?"); + sb.Append("_"); + sb.Append(start.Line); + sb.Append(":"); + sb.Append(start.Character); + sb.Append("-"); + sb.Append(end.Line); + sb.Append(":"); + sb.Append(end.Character); + + return sb.ToString(); } private static Diagnostic GetDiagnosticFromMarker(ScriptFileMarker scriptFileMarker) From c0df7289e67a8a1e1e542fa2e93bdd8a18f15f86 Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Sun, 11 Nov 2018 20:06:30 -0700 Subject: [PATCH 3/5] Remove extra filesToAnalyze empty array check per PR feedback --- .../Server/LanguageServer.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index 4de2aa90a..ca4edc571 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -1554,12 +1554,6 @@ private static async Task DelayThenInvokeDiagnostics( ILogger Logger, CancellationToken cancellationToken) { - // If filesToAnalzye is empty, nothing to do so return early. - if (filesToAnalyze.Length == 0) - { - return; - } - // First of all, wait for the desired delay period before // analyzing the provided list of files try From dd73d269e42ecb7843e0c89131ea0762145209bb Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Tue, 13 Nov 2018 18:33:46 -0700 Subject: [PATCH 4/5] Address PR feedback on StringBuilder --- .../Server/LanguageServer.cs | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index ca4edc571..054f6843b 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -1668,22 +1668,23 @@ private static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic) Position start = diagnostic.Range.Start; Position end = diagnostic.Range.End; - var sb = new StringBuilder(256); - sb.Append(diagnostic.Source ?? "?"); - sb.Append("_"); - sb.Append(diagnostic.Code ?? "?"); - sb.Append("_"); - sb.Append((diagnostic.Severity != null) ? diagnostic.Severity.ToString() : "?"); - sb.Append("_"); - sb.Append(start.Line); - sb.Append(":"); - sb.Append(start.Character); - sb.Append("-"); - sb.Append(end.Line); - sb.Append(":"); - sb.Append(end.Character); - - return sb.ToString(); + 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) From 96e83e6ffebb94670818d0037d9c8b5ee85288be Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Wed, 14 Nov 2018 17:56:15 -0700 Subject: [PATCH 5/5] Fix indentation issue as per PR feedback --- src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index 054f6843b..0f6550f90 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -1244,8 +1244,7 @@ protected async Task HandleCodeActionRequest( } } - await requestContext.SendResult( - codeActionCommands.ToArray()); + await requestContext.SendResult(codeActionCommands.ToArray()); } protected async Task HandleDocumentFormattingRequest(