diff --git a/src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs b/src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs index 9af1bff60..7ae41e0eb 100644 --- a/src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs +++ b/src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs @@ -1,22 +1,22 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -using Microsoft.Extensions.Logging; -using Microsoft.PowerShell.EditorServices.Services.TextDocument; using System; using System.Collections; using System.Collections.Generic; using System.Collections.ObjectModel; using System.IO; using System.Linq; -using System.Management.Automation; -using System.Management.Automation.Runspaces; using System.Text; -using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.PowerShell.EditorServices.Services.TextDocument; namespace Microsoft.PowerShell.EditorServices.Services.Analysis { + using System.Management.Automation; + using System.Management.Automation.Runspaces; + /// /// PowerShell script analysis engine that uses PSScriptAnalyzer /// cmdlets run through a PowerShell API to drive analysis. @@ -37,7 +37,7 @@ public class Builder /// /// Create a builder for PssaCmdletAnalysisEngine construction. /// - /// The logger to use. + /// The logger to use. public Builder(ILoggerFactory loggerFactory) => _loggerFactory = loggerFactory; /// @@ -51,17 +51,6 @@ public Builder WithSettingsFile(string settingsPath) return this; } - /// - /// Uses a settings hashtable for PSSA rule configuration. - /// - /// The settings hashtable to pass to PSSA. - /// The builder for chaining. - public Builder WithSettings(Hashtable settings) - { - _settingsParameter = settings; - return this; - } - /// /// Uses a set of unconfigured rules for PSSA configuration. /// @@ -85,11 +74,11 @@ public PssaCmdletAnalysisEngine Build() ILogger logger = _loggerFactory.CreateLogger(); try { - RunspacePool pssaRunspacePool = CreatePssaRunspacePool(out PSModuleInfo pssaModuleInfo); + RunspacePool pssaRunspacePool = CreatePssaRunspacePool(); - PssaCmdletAnalysisEngine cmdletAnalysisEngine = _settingsParameter != null - ? new PssaCmdletAnalysisEngine(logger, pssaRunspacePool, pssaModuleInfo, _settingsParameter) - : new PssaCmdletAnalysisEngine(logger, pssaRunspacePool, pssaModuleInfo, _rules); + PssaCmdletAnalysisEngine cmdletAnalysisEngine = _settingsParameter is not null + ? new PssaCmdletAnalysisEngine(logger, pssaRunspacePool, _settingsParameter) + : new PssaCmdletAnalysisEngine(logger, pssaRunspacePool, _rules); cmdletAnalysisEngine.LogAvailablePssaFeatures(); return cmdletAnalysisEngine; @@ -102,7 +91,10 @@ public PssaCmdletAnalysisEngine Build() } } - private const string PSSA_MODULE_NAME = "PSScriptAnalyzer"; + // This is a default that can be overriden at runtime by the user or tests. + // TODO: Deduplicate this logic with PsesInternalHost. + private static readonly string s_pssaModulePath = Path.GetFullPath(Path.Combine( + Path.GetDirectoryName(typeof(PssaCmdletAnalysisEngine).Assembly.Location), "..", "..", "..", "PSScriptAnalyzer")); /// /// The indentation to add when the logger lists errors. @@ -122,8 +114,6 @@ public PssaCmdletAnalysisEngine Build() private readonly RunspacePool _analysisRunspacePool; - private readonly PSModuleInfo _pssaModuleInfo; - private readonly object _settingsParameter; private readonly string[] _rulesToInclude; @@ -131,25 +121,21 @@ public PssaCmdletAnalysisEngine Build() private PssaCmdletAnalysisEngine( ILogger logger, RunspacePool analysisRunspacePool, - PSModuleInfo pssaModuleInfo, string[] rulesToInclude) - : this(logger, analysisRunspacePool, pssaModuleInfo) => _rulesToInclude = rulesToInclude; + : this(logger, analysisRunspacePool) => _rulesToInclude = rulesToInclude; private PssaCmdletAnalysisEngine( ILogger logger, RunspacePool analysisRunspacePool, - PSModuleInfo pssaModuleInfo, object analysisSettingsParameter) - : this(logger, analysisRunspacePool, pssaModuleInfo) => _settingsParameter = analysisSettingsParameter; + : this(logger, analysisRunspacePool) => _settingsParameter = analysisSettingsParameter; private PssaCmdletAnalysisEngine( ILogger logger, - RunspacePool analysisRunspacePool, - PSModuleInfo pssaModuleInfo) + RunspacePool analysisRunspacePool) { _logger = logger; _analysisRunspacePool = analysisRunspacePool; - _pssaModuleInfo = pssaModuleInfo; } /// @@ -158,14 +144,14 @@ private PssaCmdletAnalysisEngine( /// The full text of a script. /// The formatter settings to use. /// A possible range over which to run the formatter. - /// + /// Formatted script as string public async Task FormatAsync(string scriptDefinition, Hashtable formatSettings, int[] rangeList) { // We cannot use Range type therefore this workaround of using -1 default value. // Invoke-Formatter throws a ParameterBinderValidationException if the ScriptDefinition is an empty string. if (string.IsNullOrEmpty(scriptDefinition)) { - _logger.LogDebug("Script Definition was: " + scriptDefinition == null ? "null" : "empty string"); + _logger.LogDebug("Script Definition was: " + scriptDefinition is null ? "null" : "empty string"); return scriptDefinition; } @@ -174,17 +160,17 @@ public async Task FormatAsync(string scriptDefinition, Hashtable formatS .AddParameter("ScriptDefinition", scriptDefinition) .AddParameter("Settings", formatSettings); - if (rangeList != null) + if (rangeList is not null) { psCommand.AddParameter("Range", rangeList); } PowerShellResult result = await InvokePowerShellAsync(psCommand).ConfigureAwait(false); - if (result == null) + if (result is null) { _logger.LogError("Formatter returned null result"); - return scriptDefinition; + return null; } if (result.HasErrors) @@ -195,7 +181,7 @@ public async Task FormatAsync(string scriptDefinition, Hashtable formatS errorBuilder.Append(err).Append(s_indentJoin); } _logger.LogWarning($"Errors found while formatting file: {errorBuilder}"); - return scriptDefinition; + return null; } foreach (PSObject resultObj in result.Output) @@ -206,8 +192,8 @@ public async Task FormatAsync(string scriptDefinition, Hashtable formatS } } - _logger.LogError("Couldn't get result from output. Returning original script."); - return scriptDefinition; + _logger.LogError("Couldn't get result from output. Returning null."); + return null; } /// @@ -239,7 +225,7 @@ public Task AnalyzeScriptAsync(string scriptContent, Hashtab .AddParameter("Severity", s_scriptMarkerLevels); object settingsValue = settings ?? _settingsParameter; - if (settingsValue != null) + if (settingsValue is not null) { command.AddParameter("Settings", settingsValue); } @@ -251,11 +237,9 @@ public Task AnalyzeScriptAsync(string scriptContent, Hashtab return GetSemanticMarkersFromCommandAsync(command); } - public PssaCmdletAnalysisEngine RecreateWithNewSettings(string settingsPath) => new(_logger, _analysisRunspacePool, _pssaModuleInfo, settingsPath); - - public PssaCmdletAnalysisEngine RecreateWithNewSettings(Hashtable settingsHashtable) => new(_logger, _analysisRunspacePool, _pssaModuleInfo, settingsHashtable); + public PssaCmdletAnalysisEngine RecreateWithNewSettings(string settingsPath) => new(_logger, _analysisRunspacePool, settingsPath); - public PssaCmdletAnalysisEngine RecreateWithRules(string[] rules) => new(_logger, _analysisRunspacePool, _pssaModuleInfo, rules); + public PssaCmdletAnalysisEngine RecreateWithRules(string[] rules) => new(_logger, _analysisRunspacePool, rules); #region IDisposable Support private bool disposedValue; // To detect redundant calls @@ -298,19 +282,20 @@ private async Task GetSemanticMarkersFromCommandAsync(PSComm return scriptMarkers; } + // TODO: Deduplicate this logic and cleanup using lessons learned from pipeline rewrite. private Task InvokePowerShellAsync(PSCommand command) => Task.Run(() => InvokePowerShell(command)); private PowerShellResult InvokePowerShell(PSCommand command) { - using System.Management.Automation.PowerShell powerShell = System.Management.Automation.PowerShell.Create(RunspaceMode.NewRunspace); - powerShell.RunspacePool = _analysisRunspacePool; - powerShell.Commands = command; + using PowerShell pwsh = PowerShell.Create(RunspaceMode.NewRunspace); + pwsh.RunspacePool = _analysisRunspacePool; + pwsh.Commands = command; PowerShellResult result = null; try { - Collection output = InvokePowerShellWithModulePathPreservation(powerShell); - PSDataCollection errors = powerShell.Streams.Error; - result = new PowerShellResult(output, errors, powerShell.HadErrors); + Collection output = pwsh.Invoke(); + PSDataCollection errors = pwsh.Streams.Error; + result = new PowerShellResult(output, errors, pwsh.HadErrors); } catch (CommandNotFoundException ex) { @@ -330,20 +315,6 @@ private PowerShellResult InvokePowerShell(PSCommand command) return result; } - /// - /// Execute PSScriptAnalyzer cmdlets in PowerShell while preserving the PSModulePath. - /// Attempts to prevent PSModulePath mutation by runspace creation within the PSScriptAnalyzer module. - /// - /// The PowerShell instance to execute. - /// The output of PowerShell execution. - private static Collection InvokePowerShellWithModulePathPreservation(System.Management.Automation.PowerShell powershell) - { - using (PSModulePathPreserver.Take()) - { - return powershell.Invoke(); - } - } - /// /// Log the features available from the PSScriptAnalyzer module that has been imported /// for use with the AnalysisService. @@ -356,32 +327,13 @@ private void LogAvailablePssaFeatures() return; } - if (_pssaModuleInfo == null) - { - throw new FileNotFoundException("Unable to find loaded PSScriptAnalyzer module for logging"); - } - StringBuilder sb = new(); - sb.AppendLine("PSScriptAnalyzer successfully imported:"); - - // Log version - sb.Append(" Version: "); - sb.AppendLine(_pssaModuleInfo.Version.ToString()); - - // Log exported cmdlets - sb.AppendLine(" Exported Cmdlets:"); - foreach (string cmdletName in _pssaModuleInfo.ExportedCmdlets.Keys.OrderBy(name => name)) - { - sb.Append(" "); - sb.AppendLine(cmdletName); - } + sb.AppendLine("PSScriptAnalyzer successfully imported:").AppendLine(" Available Rules:"); // Log available rules - sb.AppendLine(" Available Rules:"); foreach (string ruleName in GetPSScriptAnalyzerRules()) { - sb.Append(" "); - sb.AppendLine(ruleName); + sb.Append(" ").AppendLine(ruleName); } _logger.LogDebug(sb.ToString()); @@ -393,7 +345,7 @@ private void LogAvailablePssaFeatures() private IEnumerable GetPSScriptAnalyzerRules() { PowerShellResult getRuleResult = InvokePowerShell(new PSCommand().AddCommand("Get-ScriptAnalyzerRule")); - if (getRuleResult == null) + if (getRuleResult is null) { _logger.LogWarning("Get-ScriptAnalyzerRule returned null result"); return Enumerable.Empty(); @@ -413,33 +365,9 @@ private IEnumerable GetPSScriptAnalyzerRules() /// This looks for the latest version of PSScriptAnalyzer on the path and loads that. /// /// A runspace pool with PSScriptAnalyzer loaded for running script analysis tasks. - private static RunspacePool CreatePssaRunspacePool(out PSModuleInfo pssaModuleInfo) + private static RunspacePool CreatePssaRunspacePool() { - using System.Management.Automation.PowerShell ps = System.Management.Automation.PowerShell.Create(RunspaceMode.NewRunspace); - // Run `Get-Module -ListAvailable -Name "PSScriptAnalyzer"` - ps.AddCommand("Get-Module") - .AddParameter("ListAvailable") - .AddParameter("Name", PSSA_MODULE_NAME); - - try - { - using (PSModulePathPreserver.Take()) - { - // Get the latest version of PSScriptAnalyzer we can find - pssaModuleInfo = ps.Invoke()? - .OrderByDescending(moduleInfo => moduleInfo.Version) - .FirstOrDefault(); - } - } - catch (Exception e) - { - throw new FileNotFoundException("Unable to find PSScriptAnalyzer module on the module path", e); - } - - if (pssaModuleInfo == null) - { - throw new FileNotFoundException("Unable to find PSScriptAnalyzer module on the module path"); - } + using PowerShell pwsh = PowerShell.Create(RunspaceMode.NewRunspace); // Now that we know where the PSScriptAnalyzer we want to use is, create a base // session state with PSScriptAnalyzer loaded @@ -448,18 +376,15 @@ private static RunspacePool CreatePssaRunspacePool(out PSModuleInfo pssaModuleIn // only, which is a more minimal and therefore safer state. InitialSessionState sessionState = InitialSessionState.CreateDefault2(); - sessionState.ImportPSModule(new[] { pssaModuleInfo.ModuleBase }); + sessionState.ImportPSModulesFromPath(s_pssaModulePath); + // pwsh.ImportModule(s_pssaModulePath); + // sessionState.ImportPSModule(new[] { pssaModuleInfo.ModuleBase }); RunspacePool runspacePool = RunspaceFactory.CreateRunspacePool(sessionState); runspacePool.SetMaxRunspaces(1); runspacePool.ThreadOptions = PSThreadOptions.ReuseThread; - - // Open the runspace pool here so we can deterministically handle the PSModulePath change issue - using (PSModulePathPreserver.Take()) - { - runspacePool.Open(); - } + runspacePool.Open(); return runspacePool; } @@ -486,33 +411,5 @@ public PowerShellResult( public bool HasErrors { get; } } - - /// - /// Struct to manage a call that may change the PSModulePath, so that it can be safely reset afterward. - /// - /// - /// If the user manages to set the module path at the same time, using this struct may override that. - /// But this happening is less likely than the current issue where the PSModulePath is always reset. - /// - private struct PSModulePathPreserver : IDisposable - { - private static readonly object s_psModulePathMutationLock = new(); - - public static PSModulePathPreserver Take() - { - Monitor.Enter(s_psModulePathMutationLock); - return new PSModulePathPreserver(Environment.GetEnvironmentVariable("PSModulePath")); - } - - private readonly string _psModulePath; - - private PSModulePathPreserver(string psModulePath) => _psModulePath = psModulePath; - - public void Dispose() - { - Environment.SetEnvironmentVariable("PSModulePath", _psModulePath); - Monitor.Exit(s_psModulePathMutationLock); - } - } } } diff --git a/src/PowerShellEditorServices/Services/TextDocument/Handlers/FormattingHandlers.cs b/src/PowerShellEditorServices/Services/TextDocument/Handlers/FormattingHandlers.cs index 0ddeedacd..5b72f25cd 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/Handlers/FormattingHandlers.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/Handlers/FormattingHandlers.cs @@ -72,8 +72,14 @@ public override async Task Handle(DocumentFormattingParams re if (formattedScript is null) { - _logger.LogWarning($"Formatting returned null. Returning original contents for file: {scriptFile.DocumentUri}"); - formattedScript = scriptFile.Contents; + _logger.LogWarning($"Formatting returned null. Not formatting: {scriptFile.DocumentUri}"); + return null; + } + + if (cancellationToken.IsCancellationRequested) + { + _logger.LogWarning($"Formatting request canceled for: {scriptFile.DocumentUri}"); + return null; } return new TextEditContainer(new TextEdit @@ -152,8 +158,14 @@ public override async Task Handle(DocumentRangeFormattingPara if (formattedScript is null) { - _logger.LogWarning($"Formatting returned null. Returning original contents for file: {scriptFile.DocumentUri}"); - formattedScript = scriptFile.Contents; + _logger.LogWarning($"Formatting returned null. Not formatting: {scriptFile.DocumentUri}"); + return null; + } + + if (cancellationToken.IsCancellationRequested) + { + _logger.LogWarning($"Formatting request canceled for: {scriptFile.DocumentUri}"); + return null; } return new TextEditContainer(new TextEdit