Fix running untitled scripts with arguments (but break debugging)

So the extant hack that enabled debugging untitled scripts is really
untenable. It shifted the user's args by one since it ran the untitled
script as the first arg to the dot-source operator. Unfortunately, there
doesn't seem to be a clear way to break on lines in an untitled script.
However, `Wait-Debugger` and command breakpoints still work.

Attempting to be "helpful" by escaping arguments proved to cause more
issues than it solved. We can instead massively simplify our script
launching logic (and fix yet another test that had a "maybe bug") by
just launching what the user gave us. This should also be easier for the
user to debug.

Author: andyleejordan

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs

@@ -1,6 +1,10 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
+using System.Management.Automation;
+using System.Management.Automation.Language;
+using System.Threading;
+using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Microsoft.PowerShell.EditorServices.Services;
 using Microsoft.PowerShell.EditorServices.Services.DebugAdapter;
@@ -13,10 +17,6 @@
 using OmniSharp.Extensions.DebugAdapter.Protocol.Events;
 using OmniSharp.Extensions.DebugAdapter.Protocol.Requests;
 using OmniSharp.Extensions.DebugAdapter.Protocol.Server;
-using System.Management.Automation;
-using System.Management.Automation.Language;
-using System.Threading;
-using System.Threading.Tasks;
 
 namespace Microsoft.PowerShell.EditorServices.Handlers
 {
@@ -102,48 +102,14 @@ public Task<ConfigurationDoneResponse> Handle(ConfigurationDoneArguments request
 
         private async Task LaunchScriptAsync(string scriptToLaunch)
         {
-            // Is this an untitled script?
-            if (ScriptFile.IsUntitledPath(scriptToLaunch))
-            {
-                ScriptFile untitledScript = _workspaceService.GetFile(scriptToLaunch);
-
-                if (BreakpointApiUtils.SupportsBreakpointApis(_runspaceContext.CurrentRunspace))
-                {
-                    // Parse untitled files with their `Untitled:` URI as the file name which will cache the URI & contents within the PowerShell parser.
-                    // By doing this, we light up the ability to debug Untitled files with breakpoints.
-                    // This is only possible via the direct usage of the breakpoint APIs in PowerShell because
-                    // Set-PSBreakpoint validates that paths are actually on the filesystem.
-                    ScriptBlockAst ast = Parser.ParseInput(untitledScript.Contents, untitledScript.DocumentUri.ToString(), out Token[] tokens, out ParseError[] errors);
-
-                    // This seems to be the simplest way to invoke a script block (which contains breakpoint information) via the PowerShell API.
-                    //
-                    // TODO: Fix this so the added script doesn't show up.
-                    var cmd = new PSCommand().AddScript(". $args[0]").AddArgument(ast.GetScriptBlock());
-                    await _executionService
-                        .ExecutePSCommandAsync<object>(cmd, CancellationToken.None, s_debuggerExecutionOptions)
-                        .ConfigureAwait(false);
-                }
-                else
-                {
-                    await _executionService
-                        .ExecutePSCommandAsync(
-                            new PSCommand().AddScript(untitledScript.Contents),
-                            CancellationToken.None,
-                            s_debuggerExecutionOptions)
-                        .ConfigureAwait(false);
-                }
-            }
-            else
-            {
-                // TODO: Fix this so the added script doesn't show up.
-                await _executionService
-                    .ExecutePSCommandAsync(
-                        PSCommandHelpers.BuildCommandFromArguments(scriptToLaunch, _debugStateService.Arguments),
-                        CancellationToken.None,
-                        s_debuggerExecutionOptions)
-                    .ConfigureAwait(false);
-            }
-
+            // TODO: Theoretically we can make PowerShell respect line breakpoints in untitled
+            // files, but the previous method was a hack that conflicted with correct passing of
+            // arguments to the debugged script. We are prioritizing the latter over the former, as
+            // command breakpoints and `Wait-Debugger` work fine.
+            PSCommand cmd = ScriptFile.IsUntitledPath(scriptToLaunch)
+                ? PSCommandHelpers.BuildCommandFromArguments(string.Concat("{ ", _workspaceService.GetFile(scriptToLaunch).Contents, " }"), _debugStateService.Arguments)
+                : PSCommandHelpers.BuildCommandFromArguments(string.Concat('"', scriptToLaunch, '"'), _debugStateService.Arguments);
+            await _executionService.ExecutePSCommandAsync(cmd, CancellationToken.None, s_debuggerExecutionOptions).ConfigureAwait(false);
             _debugAdapterServer.SendNotification(EventNames.Terminated);
         }
     }

src/PowerShellEditorServices/Utility/ArgumentUtils.cs

@@ -129,25 +129,10 @@ private static StringBuilder AddCommandText(this StringBuilder sb, Command comma
             return sb;
         }
 
-        public static PSCommand BuildCommandFromArguments(string command, IReadOnlyList<string> arguments)
+        public static PSCommand BuildCommandFromArguments(string command, IEnumerable<string> arguments = default)
         {
             // HACK: We use AddScript instead of AddArgument/AddParameter to reuse Powershell parameter binding logic.
-            // We quote the command parameter so that expressions can still be used in the arguments.
-            var sb = new StringBuilder()
-                .Append('.')
-                .Append(' ')
-                .Append('"')
-                .Append(command)
-                .Append('"');
-
-            foreach (string arg in arguments ?? System.Linq.Enumerable.Empty<string>())
-            {
-                sb
-                .Append(' ')
-                .Append(ArgumentEscaping.Escape(arg));
-            }
-
-            return new PSCommand().AddScript(sb.ToString());
+            return new PSCommand().AddScript(string.Concat(". ", command, " ", string.Join(" ", arguments)));
         }
     }
 }

src/PowerShellEditorServices/Utility/PSCommandExtensions.cs

@@ -96,7 +96,7 @@ private VariableDetailsBase[] GetVariables(string scopeName)
         private Task ExecutePowerShellCommand(string command, params string[] args)
         {
             return psesHost.ExecutePSCommandAsync(
-                PSCommandHelpers.BuildCommandFromArguments(command, args),
+                PSCommandHelpers.BuildCommandFromArguments(string.Concat('"', command, '"'), args),
                 CancellationToken.None);
         }
 
@@ -176,8 +176,16 @@ await debugService.SetCommandBreakpointsAsync(
             Assert.Equal("[ArrayList: 0]", var.ValueString);
         }
 
-        [Fact]
-        public async Task DebuggerAcceptsScriptArgs()
+        // See https://www.thomasbogholm.net/2021/06/01/convenient-member-data-sources-with-xunit/
+        public static IEnumerable<object[]> DebuggerAcceptsScriptArgsTestData => new List<object[]>()
+        {
+            new object[] { new object[] { "Foo -Param2 @('Bar','Baz') -Force Extra1" } },
+            new object[] { new object[] { "Foo", "-Param2", "@('Bar','Baz')", "-Force", "Extra1" } }
+        };
+
+        [Theory]
+        [MemberData(nameof(DebuggerAcceptsScriptArgsTestData))]
+        public async Task DebuggerAcceptsScriptArgs(string[] args)
         {
             // The path is intentionally odd (some escaped chars but not all) because we are testing
             // the internal path escaping mechanism - it should escape certains chars ([, ] and space) but
@@ -197,9 +205,6 @@ public async Task DebuggerAcceptsScriptArgs()
                 Assert.True(breakpoint.Verified);
             });
 
-            // TODO: This test used to also pass the args as a single string, but that doesn't seem
-            // to work any more. Perhaps that's a bug?
-            var args = new[] { "Foo", "-Param2", "@('Bar','Baz')", "-Force", "Extra1" };
             Task _ = ExecutePowerShellCommand(debugWithParamsFile.FilePath, args);
 
             AssertDebuggerStopped(debugWithParamsFile.FilePath, 3);