diff --git a/src/PowerShellEditorServices.Engine/LanguageServer/OmnisharpLanguageServer.cs b/src/PowerShellEditorServices.Engine/LanguageServer/OmnisharpLanguageServer.cs index 8f8792980..ccf221008 100644 --- a/src/PowerShellEditorServices.Engine/LanguageServer/OmnisharpLanguageServer.cs +++ b/src/PowerShellEditorServices.Engine/LanguageServer/OmnisharpLanguageServer.cs @@ -94,7 +94,8 @@ public async Task StartAsync() .WithHandler() .WithHandler() .WithHandler() - .WithHandler(); + .WithHandler() + .WithHandler(); logger.LogInformation("Handlers added"); }); diff --git a/src/PowerShellEditorServices.Engine/PowerShellEditorServices.Engine.csproj b/src/PowerShellEditorServices.Engine/PowerShellEditorServices.Engine.csproj index c0fb4a3df..032e410d8 100644 --- a/src/PowerShellEditorServices.Engine/PowerShellEditorServices.Engine.csproj +++ b/src/PowerShellEditorServices.Engine/PowerShellEditorServices.Engine.csproj @@ -12,7 +12,7 @@ - + diff --git a/src/PowerShellEditorServices.Engine/Services/Analysis/AnalysisService.cs b/src/PowerShellEditorServices.Engine/Services/Analysis/AnalysisService.cs index 992fb65ab..d36a0c987 100644 --- a/src/PowerShellEditorServices.Engine/Services/Analysis/AnalysisService.cs +++ b/src/PowerShellEditorServices.Engine/Services/Analysis/AnalysisService.cs @@ -15,6 +15,7 @@ using OmniSharp.Extensions.LanguageServer.Protocol.Models; using OmniSharp.Extensions.LanguageServer.Protocol.Server; using System.Threading; +using System.Collections.Concurrent; namespace Microsoft.PowerShell.EditorServices { @@ -22,7 +23,7 @@ namespace Microsoft.PowerShell.EditorServices /// Provides a high-level service for performing semantic analysis /// of PowerShell scripts. /// - public class AnalysisService : IDisposable + internal class AnalysisService : IDisposable { #region Static fields @@ -54,9 +55,6 @@ public class AnalysisService : IDisposable private static readonly string[] s_emptyGetRuleResult = new string[0]; - private Dictionary> codeActionsPerFile = - new Dictionary>(); - private static CancellationTokenSource s_existingRequestCancellation; /// @@ -96,8 +94,11 @@ public class AnalysisService : IDisposable private PSModuleInfo _pssaModuleInfo; private readonly ILanguageServer _languageServer; + private readonly ConfigurationService _configurationService; + private readonly ConcurrentDictionary)> _mostRecentCorrectionsByFile; + #endregion // Private Fields #region Properties @@ -149,6 +150,7 @@ private AnalysisService( _configurationService = configurationService; _logger = logger; _pssaModuleInfo = pssaModuleInfo; + _mostRecentCorrectionsByFile = new ConcurrentDictionary)>(); } #endregion // constructors @@ -813,26 +815,57 @@ private void PublishScriptDiagnostics( ScriptFile scriptFile, List markers) { - List diagnostics = new List(); + var diagnostics = new List(); - // Hold on to any corrections that may need to be applied later - Dictionary fileCorrections = - new Dictionary(); + // Create the entry for this file if it does not already exist + SemaphoreSlim fileLock; + Dictionary fileCorrections; + bool newEntryNeeded = false; + if (_mostRecentCorrectionsByFile.TryGetValue(scriptFile.DocumentUri, out (SemaphoreSlim, Dictionary) fileCorrectionsEntry)) + { + fileLock = fileCorrectionsEntry.Item1; + fileCorrections = fileCorrectionsEntry.Item2; + } + else + { + fileLock = new SemaphoreSlim(initialCount: 1, maxCount: 1); + fileCorrections = new Dictionary(); + newEntryNeeded = true; + } - foreach (var marker in markers) + fileLock.Wait(); + try { - // Does the marker contain a correction? - Diagnostic markerDiagnostic = GetDiagnosticFromMarker(marker); - if (marker.Correction != null) + if (newEntryNeeded) { - string diagnosticId = GetUniqueIdFromDiagnostic(markerDiagnostic); - fileCorrections[diagnosticId] = marker.Correction; + // If we create a new entry, we should do it after acquiring the lock we just created + // to ensure a competing thread can never acquire it first and read invalid information from it + _mostRecentCorrectionsByFile[scriptFile.DocumentUri] = (fileLock, fileCorrections); } + else + { + // Otherwise we need to clear the stale corrections + fileCorrections.Clear(); + } + + foreach (ScriptFileMarker marker in markers) + { + // Does the marker contain a correction? + Diagnostic markerDiagnostic = GetDiagnosticFromMarker(marker); + if (marker.Correction != null) + { + string diagnosticId = GetUniqueIdFromDiagnostic(markerDiagnostic); + fileCorrections[diagnosticId] = marker.Correction; + } - diagnostics.Add(markerDiagnostic); + diagnostics.Add(markerDiagnostic); + } + } + finally + { + fileLock.Release(); } - codeActionsPerFile[scriptFile.DocumentUri] = fileCorrections; var uriBuilder = new UriBuilder() { @@ -850,9 +883,34 @@ private void PublishScriptDiagnostics( }); } + public async Task> GetMostRecentCodeActionsForFileAsync(string documentUri) + { + if (!_mostRecentCorrectionsByFile.TryGetValue(documentUri, out (SemaphoreSlim fileLock, Dictionary corrections) fileCorrectionsEntry)) + { + return null; + } + + await fileCorrectionsEntry.fileLock.WaitAsync(); + // We must copy the dictionary for thread safety + var corrections = new Dictionary(fileCorrectionsEntry.corrections.Count); + try + { + foreach (KeyValuePair correction in fileCorrectionsEntry.corrections) + { + corrections.Add(correction.Key, correction.Value); + } + + return corrections; + } + finally + { + fileCorrectionsEntry.fileLock.Release(); + } + } + // 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) + internal static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic) { Position start = diagnostic.Range.Start; Position end = diagnostic.Range.End; @@ -860,7 +918,7 @@ private static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic) var sb = new StringBuilder(256) .Append(diagnostic.Source ?? "?") .Append("_") - .Append(diagnostic.Code.ToString()) + .Append(diagnostic.Code.IsString ? diagnostic.Code.String : diagnostic.Code.Long.ToString()) .Append("_") .Append(diagnostic.Severity?.ToString() ?? "?") .Append("_") diff --git a/src/PowerShellEditorServices.Engine/Services/TextDocument/Handlers/CodeActionHandler.cs b/src/PowerShellEditorServices.Engine/Services/TextDocument/Handlers/CodeActionHandler.cs new file mode 100644 index 000000000..64f994423 --- /dev/null +++ b/src/PowerShellEditorServices.Engine/Services/TextDocument/Handlers/CodeActionHandler.cs @@ -0,0 +1,120 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.ComponentModel; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Newtonsoft.Json.Linq; +using OmniSharp.Extensions.JsonRpc.Client; +using OmniSharp.Extensions.LanguageServer.Protocol.Client.Capabilities; +using OmniSharp.Extensions.LanguageServer.Protocol.Models; +using OmniSharp.Extensions.LanguageServer.Protocol.Server; +using PowerShellEditorServices.Engine.Services.Handlers; + +namespace Microsoft.PowerShell.EditorServices.TextDocument +{ + internal class CodeActionHandler : ICodeActionHandler + { + private static readonly CodeActionKind[] s_supportedCodeActions = new[] + { + CodeActionKind.QuickFix + }; + + private readonly CodeActionRegistrationOptions _registrationOptions; + + private readonly ILogger _logger; + + private readonly AnalysisService _analysisService; + + private CodeActionCapability _capability; + + public CodeActionHandler(ILoggerFactory factory, AnalysisService analysisService) + { + _logger = factory.CreateLogger(); + _analysisService = analysisService; + _registrationOptions = new CodeActionRegistrationOptions() + { + DocumentSelector = new DocumentSelector(new DocumentFilter() { Pattern = "**/*.ps*1" }), + CodeActionKinds = s_supportedCodeActions + }; + } + + public CodeActionRegistrationOptions GetRegistrationOptions() + { + return _registrationOptions; + } + + public void SetCapability(CodeActionCapability capability) + { + _capability = capability; + } + + public async Task Handle(CodeActionParams request, CancellationToken cancellationToken) + { + IReadOnlyDictionary corrections = await _analysisService.GetMostRecentCodeActionsForFileAsync(request.TextDocument.Uri.ToString()); + + if (corrections == null) + { + // TODO: Find out if we can cache this empty value + return new CommandOrCodeActionContainer(); + } + + var codeActions = 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. + foreach (Diagnostic diagnostic in request.Context.Diagnostics) + { + if (diagnostic.Code.IsLong) + { + _logger.LogWarning( + $"textDocument/codeAction skipping diagnostic with non-string code {diagnostic.Code.Long}: {diagnostic.Source} {diagnostic.Message}"); + } + else if (string.IsNullOrEmpty(diagnostic.Code.String)) + { + _logger.LogWarning( + $"textDocument/codeAction skipping diagnostic with empty Code field: {diagnostic.Source} {diagnostic.Message}"); + + continue; + } + + + string diagnosticId = AnalysisService.GetUniqueIdFromDiagnostic(diagnostic); + if (corrections.TryGetValue(diagnosticId, out MarkerCorrection correction)) + { + codeActions.Add(new Command() + { + Title = correction.Name, + Name = "PowerShell.ApplyCodeActionEdits", + Arguments = JArray.FromObject(correction.Edits) + }); + } + } + + // 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 (Diagnostic diagnostic in request.Context.Diagnostics) + { + if (!diagnostic.Code.IsString || string.IsNullOrEmpty(diagnostic.Code.String)) { continue; } + + if (string.Equals(diagnostic.Source, "PSScriptAnalyzer", StringComparison.OrdinalIgnoreCase) && + !ruleNamesProcessed.Contains(diagnostic.Code.String)) + { + ruleNamesProcessed.Add(diagnostic.Code.String); + + codeActions.Add( + new Command + { + Title = $"Show documentation for \"{diagnostic.Code}\"", + Name = "PowerShell.ShowCodeActionDocumentation", + Arguments = JArray.FromObject(new[] { diagnostic.Code }) + }); + } + } + + return codeActions; + } + } +} diff --git a/src/PowerShellEditorServices.Engine/Services/TextDocument/Handlers/FormattingHandlers.cs b/src/PowerShellEditorServices.Engine/Services/TextDocument/Handlers/FormattingHandlers.cs index b66e581de..57c0744a1 100644 --- a/src/PowerShellEditorServices.Engine/Services/TextDocument/Handlers/FormattingHandlers.cs +++ b/src/PowerShellEditorServices.Engine/Services/TextDocument/Handlers/FormattingHandlers.cs @@ -10,7 +10,7 @@ namespace PowerShellEditorServices.Engine.Services.Handlers { - public class DocumentFormattingHandler : IDocumentFormattingHandler + internal class DocumentFormattingHandler : IDocumentFormattingHandler { private readonly DocumentSelector _documentSelector = new DocumentSelector( new DocumentFilter() @@ -88,7 +88,7 @@ public void SetCapability(DocumentFormattingCapability capability) } } - public class DocumentRangeFormattingHandler : IDocumentRangeFormattingHandler + internal class DocumentRangeFormattingHandler : IDocumentRangeFormattingHandler { private readonly DocumentSelector _documentSelector = new DocumentSelector( new DocumentFilter() diff --git a/src/PowerShellEditorServices.Engine/Services/TextDocument/ScriptFileMarker.cs b/src/PowerShellEditorServices.Engine/Services/TextDocument/ScriptFileMarker.cs index 9700464dd..a6f518c75 100644 --- a/src/PowerShellEditorServices.Engine/Services/TextDocument/ScriptFileMarker.cs +++ b/src/PowerShellEditorServices.Engine/Services/TextDocument/ScriptFileMarker.cs @@ -147,11 +147,11 @@ internal static ScriptFileMarker FromDiagnosticRecord(PSObject psObject) new ScriptRegion( diagnosticRecord.ScriptPath, suggestedCorrection.Text, - suggestedCorrection.StartLineNumber, - suggestedCorrection.StartColumnNumber, + startLineNumber: suggestedCorrection.StartLineNumber, + startColumnNumber: suggestedCorrection.StartColumnNumber, + endLineNumber: suggestedCorrection.EndLineNumber, + endColumnNumber: suggestedCorrection.EndColumnNumber, startOffset: -1, - suggestedCorrection.EndLineNumber, - suggestedCorrection.EndColumnNumber, endOffset: -1)); correctionMessage = suggestedCorrection.Description; diff --git a/src/PowerShellEditorServices.Engine/Services/Workspace/Handlers/ConfigurationHandler.cs b/src/PowerShellEditorServices.Engine/Services/Workspace/Handlers/ConfigurationHandler.cs index b97485dc0..c1d672999 100644 --- a/src/PowerShellEditorServices.Engine/Services/Workspace/Handlers/ConfigurationHandler.cs +++ b/src/PowerShellEditorServices.Engine/Services/Workspace/Handlers/ConfigurationHandler.cs @@ -10,7 +10,7 @@ namespace Microsoft.PowerShell.EditorServices { - public class ConfigurationHandler : IDidChangeConfigurationHandler + internal class ConfigurationHandler : IDidChangeConfigurationHandler { private readonly ILogger _logger; private readonly AnalysisService _analysisService; diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index c8029aed5..06ac6bd7b 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -1732,7 +1732,6 @@ await DelayThenInvokeDiagnosticsAsync( cancellationToken); } - private static async Task DelayThenInvokeDiagnosticsAsync( int delayMilliseconds, ScriptFile[] filesToAnalyze, diff --git a/test/Pester/EditorServices.Integration.Tests.ps1 b/test/Pester/EditorServices.Integration.Tests.ps1 index 2dd7079ba..edc53f684 100644 --- a/test/Pester/EditorServices.Integration.Tests.ps1 +++ b/test/Pester/EditorServices.Integration.Tests.ps1 @@ -163,20 +163,34 @@ function Get-Foo { It "Can get Diagnostics after changing settings" { $file = New-TestFile -Script 'gci | % { $_ }' - $request = Send-LspDidChangeConfigurationRequest -Client $client -Settings @{ - PowerShell = @{ - ScriptAnalysis = @{ - Enable = $false + try + { + $request = Send-LspDidChangeConfigurationRequest -Client $client -Settings @{ + PowerShell = @{ + ScriptAnalysis = @{ + Enable = $false + } } } - } - # Grab notifications for just the file opened in this test. - $notifications = Get-LspNotification -Client $client | Where-Object { - $_.Params.uri -match ([System.IO.Path]::GetFileName($file.PSPath)) + # Grab notifications for just the file opened in this test. + $notifications = Get-LspNotification -Client $client | Where-Object { + $_.Params.uri -match ([System.IO.Path]::GetFileName($file.PSPath)) + } + $notifications | Should -Not -BeNullOrEmpty + $notifications.Params.diagnostics | Should -BeNullOrEmpty + } + finally + { + # Restore PSSA state + Send-LspDidChangeConfigurationRequest -Client $client -Settings @{ + PowerShell = @{ + ScriptAnalysis = @{ + Enable = $true + } + } + } } - $notifications | Should -Not -BeNullOrEmpty - $notifications.Params.diagnostics | Should -BeNullOrEmpty } It "Can handle folding request" { @@ -208,7 +222,7 @@ $_ $sortedResults[1].endCharacter | Should -Be 2 } - It "can handle a normal formatting request" { + It "Can handle a normal formatting request" { $filePath = New-TestFile -Script ' gci | % { Get-Process @@ -225,7 +239,7 @@ Get-Process $response.Result.newText.Contains("`t") | Should -BeTrue -Because "We expect a tab." } - It "can handle a range formatting request" { + It "Can handle a range formatting request" { $filePath = New-TestFile -Script ' gci | % { Get-Process @@ -407,6 +421,49 @@ Get-Foo $response.Result.command.command | Should -Be 'editor.action.showReferences' } + It "Can handle a textDocument/codeAction request" { + $script = 'gci' + $file = Set-Content -Path (Join-Path $TestDrive "$([System.IO.Path]::GetRandomFileName()).ps1") -Value $script -PassThru -Force + + $request = Send-LspDidOpenTextDocumentRequest -Client $client ` + -Uri ([Uri]::new($file.PSPath).AbsoluteUri) ` + -Text ($file[0].ToString()) + + # There's no response for this message, but we need to call Get-LspResponse + # to increment the counter. + Get-LspResponse -Client $client -Id $request.Id | Out-Null + + Start-Sleep 1 + + # Grab notifications for just the file opened in this test. + $notifications = Get-LspNotification -Client $client | Where-Object { + $_.Params.uri -match ([System.IO.Path]::GetFileName($file.PSPath)) + } + + $notifications | Should -Not -BeNullOrEmpty + + $codeActionParams = @{ + Client = $client + Uri = $notifications.Params.uri + StartLine = 1 + StartCharacter = 1 + EndLine = 1 + EndCharacter = 4 + Diagnostics = $notifications.Params.diagnostics + } + $request = Send-LspCodeActionRequest @codeActionParams + + $response = Get-LspResponse -Client $client -Id $request.Id + + $edit = $response.Result | Where-Object command -eq 'PowerShell.ApplyCodeActionEdits' | Select-Object -First 1 + $edit | Should -Not -BeNullOrEmpty + $edit.Arguments.Text | Should -BeExactly 'Get-ChildItem' + $edit.Arguments.StartLineNumber | Should -Be 1 + $edit.Arguments.StartColumnNumber | Should -Be 1 + $edit.Arguments.EndLineNumber | Should -Be 1 + $edit.Arguments.EndColumnNumber | Should -Be 4 + } + # This test MUST be last It "Shuts down the process properly" { $request = Send-LspShutdownRequest -Client $client diff --git a/tools/PsesPsClient/PsesPsClient.psd1 b/tools/PsesPsClient/PsesPsClient.psd1 index 5a296e017..34c3921fe 100644 --- a/tools/PsesPsClient/PsesPsClient.psd1 +++ b/tools/PsesPsClient/PsesPsClient.psd1 @@ -74,6 +74,7 @@ FunctionsToExport = @( 'Connect-PsesServer', 'Send-LspRequest', 'Send-LspInitializeRequest', + 'Send-LspCodeActionRequest', 'Send-LspDidOpenTextDocumentRequest', 'Send-LspDidChangeConfigurationRequest', 'Send-LspFormattingRequest', diff --git a/tools/PsesPsClient/PsesPsClient.psm1 b/tools/PsesPsClient/PsesPsClient.psm1 index 69404f03a..099007ad9 100644 --- a/tools/PsesPsClient/PsesPsClient.psm1 +++ b/tools/PsesPsClient/PsesPsClient.psm1 @@ -560,6 +560,76 @@ function Send-LspCodeLensResolveRequest return Send-LspRequest -Client $Client -Method 'codeLens/resolve' -Parameters $params } +function Send-LspCodeActionRequest +{ + param( + [Parameter()] + [PsesPsClient.PsesLspClient] + $Client, + + [Parameter()] + [string] + $Uri, + + [Parameter()] + [int] + $StartLine, + + [Parameter()] + [int] + $StartCharacter, + + [Parameter()] + [int] + $EndLine, + + [Parameter()] + [int] + $EndCharacter, + + [Parameter()] + $Diagnostics + ) + + $params = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.CodeActionParams]@{ + TextDocument = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.TextDocumentIdentifier]@{ + Uri = $Uri + } + Range = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.Range]@{ + Start = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.Position]@{ + Line = $StartLine + Character = $StartCharacter + } + End = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.Position]@{ + Line = $EndLine + Character = $EndCharacter + } + } + Context = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.CodeActionContext]@{ + Diagnostics = $Diagnostics | ForEach-Object { + [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.Diagnostic]@{ + Code = $_.code + Severity = $_.severity + Source = $_.source + Message = $_.message + Range = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.Range]@{ + Start = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.Position]@{ + Line = $_.range.start.line + Character = $_.range.start.character + } + End = [Microsoft.PowerShell.EditorServices.Protocol.LanguageServer.Position]@{ + Line = $_.range.end.line + Character = $_.range.end.character + } + } + } + } + } + } + + return Send-LspRequest -Client $Client -Method 'textDocument/codeAction' -Parameters $params +} + function Send-LspShutdownRequest { [OutputType([PsesPsClient.LspRequest])]