Merge pull request #1577 from rjmholt/review-fixes

Address review comments

Author: andyleejordan

src/PowerShellEditorServices/Server/PsesLanguageServer.cs

@@ -113,7 +113,6 @@ public async Task StartAsync()
                     // _Initialize_ request:
                     // https://microsoft.github.io/language-server-protocol/specifications/specification-current/#initialize
                     .OnInitialize(
-                        // TODO: Either fix or ignore "method lacks 'await'" warning.
                         (languageServer, request, cancellationToken) =>
                         {
                             Log.Logger.Debug("Initializing OmniSharp Language Server");

src/PowerShellEditorServices/Server/PsesServiceCollectionExtensions.cs

@@ -47,6 +47,11 @@ public static IServiceCollection AddPsesLanguageServices(
                             provider.GetService<EditorOperationsService>(),
                             provider.GetService<PowerShellExecutionService>());
 
+                        // This is where we create the $psEditor variable
+                        // so that when the console is ready, it will be available
+                        // TODO: Improve the sequencing here so that:
+                        //  - The variable is guaranteed to be initialized when the console first appears
+                        //  - Any errors that occur are handled rather than lost by the unawaited task
                         extensionService.InitializeAsync();
 
                         return extensionService;

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

@@ -17,7 +17,6 @@
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Execution;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Host;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Debugging;
-using Microsoft.PowerShell.EditorServices.Services.PowerShell.Runspace;
 
 namespace Microsoft.PowerShell.EditorServices.Services
 {
@@ -182,7 +181,7 @@ public async Task<BreakpointDetails[]> SetLineBreakpointsAsync(
 
             // Fix for issue #123 - file paths that contain wildcard chars [ and ] need to
             // quoted and have those wildcard chars escaped.
-            string escapedScriptPath = PathUtils.WildcardEscape(scriptPath);
+            string escapedScriptPath = PathUtils.WildcardEscapePath(scriptPath);
 
             if (dscBreakpoints == null || !dscBreakpoints.IsDscResourcePath(escapedScriptPath))
             {

src/PowerShellEditorServices/Services/PowerShell/Console/ColorConfiguration.cs

@@ -2,7 +2,7 @@
 using System.Collections.Generic;
 using System.Text;
 
-namespace PowerShellEditorServices.Services.PowerShell.Console
+namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Console
 {
     internal class ColorConfiguration
     {

src/PowerShellEditorServices/Services/PowerShell/Context/PowerShellVersionDetails.cs

@@ -103,8 +103,6 @@ public static PowerShellVersionDetails GetVersionDetails(ILogger logger, PowerSh
 
             try
             {
-                var psVersionTableCommand = new PSCommand().AddScript("$PSVersionTable", useLocalScope: true);
-
                 Hashtable psVersionTable = pwsh
                     .AddScript("$PSVersionTable", useLocalScope: true)
                     .InvokeAndClear<Hashtable>()

src/PowerShellEditorServices/Services/PowerShell/Debugging/PowerShellDebugContext.cs

@@ -10,6 +10,29 @@ namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Debugging
     using OmniSharp.Extensions.LanguageServer.Protocol.Server;
     using System.Threading.Tasks;
 
+    /// <summary>
+    /// Handles the state of the PowerShell debugger.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Debugging through a PowerShell Host is implemented by registering a handler
+    /// for the <see cref="System.Management.Automation.Debugger.DebuggerStop"/> event.
+    /// Registering that handler causes debug actions in PowerShell like Set-PSBreakpoint
+    /// and Wait-Debugger to drop into the debugger and trigger the handler.
+    /// The handler is passed a mutable <see cref="System.Management.Automation.DebuggerStopEventArgs"/> object
+    /// and the debugger stop lasts for the duration of the handler call.
+    /// The handler sets the <see cref="System.Management.Automation.DebuggerStopEventArgs.ResumeAction"/> property
+    /// when after it returns, the PowerShell debugger uses that as the direction on how to proceed.
+    /// </para>
+    /// <para>
+    /// When we handle the <see cref="System.Management.Automation.Debugger.DebuggerStop"/> event,
+    /// we drop into a nested debug prompt and execute things in the debugger with <see cref="System.Management.Automation.Debugger.ProcessCommand(PSCommand, PSDataCollection{PSObject})"/>,
+    /// which enables debugger commands like <c>l</c>, <c>c</c>, <c>s</c>, etc.
+    /// <see cref="Microsoft.PowerShell.EditorServices.Services.PowerShell.Debugging.PowerShellDebugContext"/> saves the event args object in its state,
+    /// and when one of the debugger commands is used, the result returned is used to set <see cref="System.Management.Automation.DebuggerStopEventArgs.ResumeAction"/>
+    /// on the saved event args object so that when the event handler returns, the PowerShell debugger takes the correct action.
+    /// </para>
+    /// </remarks>
     internal class PowerShellDebugContext : IPowerShellDebugContext
     {
         private readonly ILogger _logger;

src/PowerShellEditorServices/Services/PowerShell/Execution/ExecutionCanceledException.cs

@@ -71,10 +71,13 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok
                 result = _pwsh.InvokeCommand<TResult>(_psCommand);
                 cancellationToken.ThrowIfCancellationRequested();
             }
+            // Test if we've been cancelled. If we're remoting, PSRemotingDataStructureException effectively means the pipeline was stopped.
             catch (Exception e) when (cancellationToken.IsCancellationRequested || e is PipelineStoppedException || e is PSRemotingDataStructureException)
             {
                 throw new OperationCanceledException();
             }
+            // We only catch RuntimeExceptions here in case writing errors to output was requested
+            // Other errors are bubbled up to the caller
             catch (RuntimeException e)
             {
                 Logger.LogWarning($"Runtime exception occurred while executing command:{Environment.NewLine}{Environment.NewLine}{e}");
@@ -107,6 +110,9 @@ private IReadOnlyList<TResult> ExecuteInDebugger(CancellationToken cancellationT
 
             var outputCollection = new PSDataCollection<PSObject>();
 
+            // Out-Default doesn't work as needed in the debugger
+            // Instead we add Out-String to the command and collect results in a PSDataCollection
+            // and use the event handler to print output to the UI as its added to that collection
             if (_executionOptions.WriteOutputToHost)
             {
                 _psCommand.AddDebugOutputCommand();
@@ -124,14 +130,21 @@ private IReadOnlyList<TResult> ExecuteInDebugger(CancellationToken cancellationT
             DebuggerCommandResults debuggerResult = null;
             try
             {
+                // In the PowerShell debugger, extra debugger commands are made available, like "l", "s", "c", etc.
+                // Executing those commands produces a result that needs to be set on the debugger stop event args.
+                // So we use the Debugger.ProcessCommand() API to properly execute commands in the debugger
+                // and then call DebugContext.ProcessDebuggerResult() later to handle the command appropriately
                 debuggerResult = _pwsh.Runspace.Debugger.ProcessCommand(_psCommand, outputCollection);
                 cancellationToken.ThrowIfCancellationRequested();
             }
+            // Test if we've been cancelled. If we're remoting, PSRemotingDataStructureException effectively means the pipeline was stopped.
             catch (Exception e) when (cancellationToken.IsCancellationRequested || e is PipelineStoppedException || e is PSRemotingDataStructureException)
             {
                 StopDebuggerIfRemoteDebugSessionFailed();
                 throw new OperationCanceledException();
             }
+            // We only catch RuntimeExceptions here in case writing errors to output was requested
+            // Other errors are bubbled up to the caller
             catch (RuntimeException e)
             {
                 Logger.LogWarning($"Runtime exception occurred while executing command:{Environment.NewLine}{Environment.NewLine}{e}");

src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs

@@ -10,6 +10,10 @@
 
 namespace Microsoft.PowerShell.EditorServices.Handlers
 {
+    /// <summary>
+    /// Handler for a custom request type for evaluating PowerShell.
+    /// This is generally for F8 support, to allow execution of a highlighted code snippet in the console as if it were copy-pasted.
+    /// </summary>
     internal class EvaluateHandler : IEvaluateHandler
     {
         private readonly ILogger _logger;
@@ -25,6 +29,9 @@ public EvaluateHandler(
 
         public Task<EvaluateResponseBody> Handle(EvaluateRequestArguments request, CancellationToken cancellationToken)
         {
+            // TODO: Understand why we currently handle this asynchronously and why we return a dummy result value
+            //       instead of awaiting the execution and returing a real result of some kind
+
             _executionService.ExecutePSCommandAsync(
                 new PSCommand().AddScript(request.Expression),
                 new PowerShellExecutionOptions { WriteInputToHost = true, WriteOutputToHost = true, AddToHistory = true, InterruptCommandPrompt = true },

src/PowerShellEditorServices/Services/PowerShell/Handlers/EvaluateHandler.cs

@@ -22,10 +22,9 @@ internal class SessionDetails
         private const string Property_InstanceId = "instanceId";
 
         /// <summary>
-        /// Gets the PSCommand that gathers details from the
-        /// current session.
+        /// Runs a PowerShell command to gather details about the current session.
         /// </summary>
-        /// <returns>A PSCommand used to gather session details.</returns>
+        /// <returns>A data object containing details about the PowerShell session.</returns>
         public static SessionDetails GetFromPowerShell(PowerShell pwsh)
         {
             Hashtable detailsObject = pwsh

src/PowerShellEditorServices/Services/PowerShell/Runspace/SessionDetails.cs

@@ -4,6 +4,23 @@
 
 namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Utility
 {
+    /// <summary>
+    /// Encapsulates the scoping logic for cancellation tokens.
+    /// As PowerShell commands nest, this class maintains a stack of cancellation scopes
+    /// that allow each scope of logic to be cancelled at its own level.
+    /// Implicitly handles the merging and cleanup of cancellation token sources.
+    /// </summary>
+    /// <example>
+    /// The <see cref="Microsoft.PowerShell.EditorServices.Services.PowerShell.Utility.CancellationContext"/> class
+    /// and the <see cref="Microsoft.PowerShell.EditorServices.Services.PowerShell.Utility.CancellationScope"/> struct
+    /// are intended to be used with a <c>using</c> block so you can do this:
+    /// <code>
+    ///     using (CancellationScope cancellationScope = _cancellationContext.EnterScope(_globalCancellationSource.CancellationToken, localCancellationToken))
+    ///     {
+    ///         ExecuteCommandAsync(command, cancellationScope.CancellationToken);
+    ///     }
+    /// </code>
+    /// </example>
     internal class CancellationContext
     {
         private readonly ConcurrentStack<CancellationTokenSource> _cancellationSourceStack;

src/PowerShellEditorServices/Services/PowerShell/Utility/CancellationContext.cs

@@ -98,12 +98,7 @@ public static async Task<CommandInfo> GetCommandInfoAsync(
                 .AddArgument(commandName)
                 .AddParameter("ErrorAction", "Ignore");
 
-            CommandInfo commandInfo = null;
-            foreach (CommandInfo result in await executionService.ExecutePSCommandAsync<CommandInfo>(command, new PowerShellExecutionOptions(), CancellationToken.None).ConfigureAwait(false))
-            {
-                commandInfo = result;
-                break;
-            }
+            CommandInfo commandInfo = (await executionService.ExecutePSCommandAsync<CommandInfo>(command, new PowerShellExecutionOptions(), CancellationToken.None).ConfigureAwait(false)).FirstOrDefault();
 
             // Only cache CmdletInfos since they're exposed in binaries they are likely to not change throughout the session.
             if (commandInfo?.CommandType == CommandTypes.Cmdlet)

src/PowerShellEditorServices/Services/PowerShell/Utility/CommandHelpers.cs

@@ -24,11 +24,15 @@ static ErrorRecordExtensions()
 
                 var errorObjectParameter = Expression.Parameter(typeof(PSObject));
 
+                // Generates a call like:
+                //  $errorPSObject.WriteStream = [System.Management.Automation.WriteStreamType]::Error
+                // So that error record PSObjects will be rendered in the console properly
+                // See https://github.com/PowerShell/PowerShell/blob/946341b2ebe6a61f081f4c9143668dc7be1f9119/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs#L2088-L2091
                 s_setWriteStreamProperty = Expression.Lambda<Action<PSObject>>(
                     Expression.Call(
                         errorObjectParameter,
                         writeStreamProperty.GetSetMethod(),
-                        Expression.Constant(errorStreamType)), 
+                        Expression.Constant(errorStreamType)),
                     errorObjectParameter)
                     .Compile();
             }

src/PowerShellEditorServices/Services/PowerShell/Utility/ErrorRecordExtensions.cs

@@ -21,22 +21,26 @@ public static PSCommand AddDebugOutputCommand(this PSCommand psCommand)
 
         public static PSCommand MergePipelineResults(this PSCommand psCommand)
         {
+            // We need to do merge errors and output before rendering with an Out- cmdlet
             Command lastCommand = psCommand.Commands[psCommand.Commands.Count - 1];
             lastCommand.MergeMyResults(PipelineResultTypes.Error, PipelineResultTypes.Output);
             lastCommand.MergeMyResults(PipelineResultTypes.Information, PipelineResultTypes.Output);
             return psCommand;
         }
 
+        /// <summary>
+        /// Get a representation of the PSCommand, for logging purposes.
+        /// </summary>
         public static string GetInvocationText(this PSCommand command)
         {
-            Command lastCommand = command.Commands[0];
+            Command currentCommand = command.Commands[0];
             var sb = new StringBuilder().AddCommandText(command.Commands[0]);
 
             for (int i = 1; i < command.Commands.Count; i++)
             {
-                sb.Append(lastCommand.IsEndOfStatement ? "; " : " | ");
-                lastCommand = command.Commands[i];
-                sb.AddCommandText(lastCommand);
+                sb.Append(currentCommand.IsEndOfStatement ? "; " : " | ");
+                currentCommand = command.Commands[i];
+                sb.AddCommandText(currentCommand);
             }
 
             return sb.ToString();

src/PowerShellEditorServices/Services/PowerShell/Utility/PSCommandExtensions.cs

@@ -71,13 +71,21 @@ public static void InvokeCommand(this PowerShell pwsh, PSCommand psCommand)
             pwsh.InvokeAndClear();
         }
 
+        /// <summary>
+        /// When running a remote session, waits for remote processing and output to complete.
+        /// </summary>
         public static void WaitForRemoteOutputIfNeeded(this PowerShell pwsh)
         {
             if (!pwsh.Runspace.RunspaceIsRemote)
             {
                 return;
             }
 
+            // These methods are required when running commands remotely.
+            // Remote rendering from command output is done asynchronously.
+            // So to ensure we wait for output to be rendered,
+            // we need these methods to wait for rendering.
+            // PowerShell does this in its own implementation: https://github.com/PowerShell/PowerShell/blob/883ca98dd74ea13b3d8c0dd62d301963a40483d6/src/System.Management.Automation/engine/debugger/debugger.cs#L4628-L4652
             s_waitForServicingComplete(pwsh);
             s_suspendIncomingData(pwsh);
         }
@@ -155,10 +163,10 @@ public static void LoadProfiles(this PowerShell pwsh, ProfilePathInfo profilePat
         {
             var profileVariable = new PSObject();
 
-            pwsh.AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.AllUsersAllHosts), profilePaths.AllUsersAllHosts);
-            pwsh.AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.AllUsersCurrentHost), profilePaths.AllUsersCurrentHost);
-            pwsh.AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.CurrentUserAllHosts), profilePaths.CurrentUserAllHosts);
-            pwsh.AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.CurrentUserCurrentHost), profilePaths.CurrentUserCurrentHost);
+            pwsh.AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.AllUsersAllHosts), profilePaths.AllUsersAllHosts)
+                .AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.AllUsersCurrentHost), profilePaths.AllUsersCurrentHost)
+                .AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.CurrentUserAllHosts), profilePaths.CurrentUserAllHosts)
+                .AddProfileMemberAndLoadIfExists(profileVariable, nameof(profilePaths.CurrentUserCurrentHost), profilePaths.CurrentUserCurrentHost);
 
             pwsh.Runspace.SessionStateProxy.SetVariable("PROFILE", profileVariable);
         }
@@ -189,7 +197,7 @@ public static string GetErrorString(this PowerShell pwsh)
             return sb.ToString();
         }
 
-        private static void AddProfileMemberAndLoadIfExists(this PowerShell pwsh, PSObject profileVariable, string profileName, string profilePath)
+        private static PowerShell AddProfileMemberAndLoadIfExists(this PowerShell pwsh, PSObject profileVariable, string profileName, string profilePath)
         {
             profileVariable.Members.Add(new PSNoteProperty(profileName, profilePath));
 
@@ -201,6 +209,8 @@ private static void AddProfileMemberAndLoadIfExists(this PowerShell pwsh, PSObje
 
                 pwsh.InvokeCommand(psCommand);
             }
+
+            return pwsh;
         }
 
         private static StringBuilder AddErrorString(this StringBuilder sb, ErrorRecord error, int errorIndex)

src/PowerShellEditorServices/Services/PowerShell/Utility/PowerShellExtensions.cs

@@ -18,12 +18,9 @@ internal static class RunspaceExtensions
         static RunspaceExtensions()
         {
             // PowerShell ApartmentState APIs aren't available in PSStandard, so we need to use reflection.
-            if (!VersionUtils.IsNetCore || VersionUtils.IsPS7OrGreater)
-            {
-                MethodInfo setterInfo = typeof(Runspace).GetProperty("ApartmentState").GetSetMethod();
-                Delegate setter = Delegate.CreateDelegate(typeof(Action<Runspace, ApartmentState>), firstArgument: null, method: setterInfo);
-                s_runspaceApartmentStateSetter = (Action<Runspace, ApartmentState>)setter;
-            }
+            MethodInfo setterInfo = typeof(Runspace).GetProperty("ApartmentState").GetSetMethod();
+            Delegate setter = Delegate.CreateDelegate(typeof(Action<Runspace, ApartmentState>), firstArgument: null, method: setterInfo);
+            s_runspaceApartmentStateSetter = (Action<Runspace, ApartmentState>)setter;
 
             MethodInfo getRemotePromptMethod = typeof(HostUtilities).GetMethod("GetRemotePrompt", BindingFlags.NonPublic | BindingFlags.Static);
             ParameterExpression runspaceParam = Expression.Parameter(typeof(Runspace));
@@ -45,6 +42,13 @@ public static void SetApartmentStateToSta(this Runspace runspace)
             s_runspaceApartmentStateSetter?.Invoke(runspace, ApartmentState.STA);
         }
 
+        /// <summary>
+        /// Augment a given prompt string with a remote decoration.
+        /// This is an internal method on <c>Runspace</c> in PowerShell that we reuse via reflection.
+        /// </summary>
+        /// <param name="runspace">The runspace the prompt is for.</param>
+        /// <param name="basePrompt">The base prompt to decorate.</param>
+        /// <returns>A prompt string decorated with remote connection details.</returns>
         public static string GetRemotePrompt(this Runspace runspace, string basePrompt)
         {
             return s_getRemotePromptFunc(runspace, basePrompt);