Remove local variables from stack frames

Thanks to PowerShell idiosyncracies around scopes, it is impossible for
the debug service to get the "local variables" for each stack frame. The
prior behavior used an assumption that from the 0 index, each increment
to the stack frame corresponded 1-to-1 to an increment in scope, and
that at each of those frames we could get local variables. This does not
work because that 1-to-1 assumption does not hold, as evidenced by
crashes exhibited in the preview extension because there are often more
stack frames than there are scopes. Since this was but a guess, it was
decided that it is worse to provide inaccurate information than a "best
guess" and so each stack frame now only gets auto variables.

In the process of fixing this, the variable detection mechanism was
improved to rely on the raw results of `Get-PSCallStack` when available
locally, and only pass it through a serialization and deserialization
process when necessary for remote debugging.

Finally, the `StackFrameDetails.Create` method had an ancient TODO from
2019 that we decided to drop the skeleton support for, namely passing
the workspace root path and processing the invoation information.

Co-authored-by: Andy Schwartzmeyer <andrew@schwartzmeyer.com>
Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>

Author: 3 people

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

@@ -17,6 +17,7 @@
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Execution;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Host;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Debugging;
+using System.Collections;
 
 namespace Microsoft.PowerShell.EditorServices.Services
 {
@@ -45,6 +46,7 @@ internal class DebugService
         private List<VariableDetailsBase> variables;
         private VariableContainerDetails globalScopeVariables;
         private VariableContainerDetails scriptScopeVariables;
+        private VariableContainerDetails localScopeVariables;
         private StackFrameDetails[] stackFrameDetails;
         private readonly PropertyInfo invocationTypeScriptPositionProperty;
 
@@ -445,11 +447,6 @@ public async Task<string> SetVariableAsync(int variableContainerReferenceId, str
                 for (int i = 0; i < stackFrames.Length; i++)
                 {
                     var stackFrame = stackFrames[i];
-                    if (stackFrame.LocalVariables.ContainsVariable(variable.Id))
-                    {
-                        scope = i.ToString();
-                        break;
-                    }
                 }
             }
 
@@ -626,13 +623,12 @@ internal async Task<StackFrameDetails[]> GetStackFramesAsync(CancellationToken c
         public VariableScope[] GetVariableScopes(int stackFrameId)
         {
             var stackFrames = this.GetStackFrames();
-            int localStackFrameVariableId = stackFrames[stackFrameId].LocalVariables.Id;
             int autoVariablesId = stackFrames[stackFrameId].AutoVariables.Id;
 
             return new VariableScope[]
             {
                 new VariableScope(autoVariablesId, VariableContainerDetails.AutoVariablesName),
-                new VariableScope(localStackFrameVariableId, VariableContainerDetails.LocalScopeName),
+                new VariableScope(this.localScopeVariables.Id, VariableContainerDetails.LocalScopeName),
                 new VariableScope(this.scriptScopeVariables.Id, VariableContainerDetails.ScriptScopeName),
                 new VariableScope(this.globalScopeVariables.Id, VariableContainerDetails.GlobalScopeName),
             };
@@ -655,30 +651,27 @@ private async Task FetchStackFramesAndVariablesAsync(string scriptNameOverride)
                     new VariableDetails("Dummy", null)
                 };
 
-                // Must retrieve global/script variales before stack frame variables
-                // as we check stack frame variables against globals.
-                await FetchGlobalAndScriptVariablesAsync().ConfigureAwait(false);
+
+                // Must retrieve in order of broadest to narrowest scope for efficient deduplication: global, script, local
+                this.globalScopeVariables =
+                    await FetchVariableContainerAsync(VariableContainerDetails.GlobalScopeName).ConfigureAwait(false);
+
+                this.scriptScopeVariables =
+                    await FetchVariableContainerAsync(VariableContainerDetails.ScriptScopeName).ConfigureAwait(false);
+
+                this.localScopeVariables =
+                    await FetchVariableContainerAsync(VariableContainerDetails.LocalScopeName).ConfigureAwait(false);
+
                 await FetchStackFramesAsync(scriptNameOverride).ConfigureAwait(false);
+
             }
             finally
             {
                 this.debugInfoHandle.Release();
             }
         }
 
-        private async Task FetchGlobalAndScriptVariablesAsync()
-        {
-            // Retrieve globals first as script variable retrieval needs to search globals.
-            this.globalScopeVariables =
-                await FetchVariableContainerAsync(VariableContainerDetails.GlobalScopeName, null).ConfigureAwait(false);
-
-            this.scriptScopeVariables =
-                await FetchVariableContainerAsync(VariableContainerDetails.ScriptScopeName, null).ConfigureAwait(false);
-        }
-
-        private async Task<VariableContainerDetails> FetchVariableContainerAsync(
-            string scope,
-            VariableContainerDetails autoVariables)
+        private async Task<VariableContainerDetails> FetchVariableContainerAsync(string scope)
         {
             PSCommand psCommand = new PSCommand()
                 .AddCommand("Get-Variable")
@@ -704,11 +697,6 @@ private async Task<VariableContainerDetails> FetchVariableContainerAsync(
                     var variableDetails = new VariableDetails(psVariableObject) { Id = this.nextVariableId++ };
                     this.variables.Add(variableDetails);
                     scopeVariableContainer.Children.Add(variableDetails.Name, variableDetails);
-
-                    if ((autoVariables != null) && AddToAutoVariables(psVariableObject, scope))
-                    {
-                        autoVariables.Children.Add(variableDetails.Name, variableDetails);
-                    }
                 }
             }
 
@@ -792,55 +780,90 @@ private bool AddToAutoVariables(PSObject psvariable, string scope)
         private async Task FetchStackFramesAsync(string scriptNameOverride)
         {
             PSCommand psCommand = new PSCommand();
+            // The serialization depth to retrieve variables from remote runspaces.
+            const int serializationDepth = 3;
 
             // This glorious hack ensures that Get-PSCallStack returns a list of CallStackFrame
             // objects (or "deserialized" CallStackFrames) when attached to a runspace in another
             // process.  Without the intermediate variable Get-PSCallStack inexplicably returns
             // an array of strings containing the formatted output of the CallStackFrame list.
-            var callStackVarName = $"$global:{PsesGlobalVariableNamePrefix}CallStack";
-            psCommand.AddScript($"{callStackVarName} = Get-PSCallStack; {callStackVarName}");
+            string callStackVarName = $"$global:{PsesGlobalVariableNamePrefix}CallStack";
+
+            string getPSCallStack = $"Get-PSCallStack | ForEach-Object {{ [void]{callStackVarName}.add(@($PSItem,$PSItem.GetFrameVariables())) }}";
+
+            // If we're attached to a remote runspace, we need to serialize the callstack prior to transport
+            // because the default depth is too shallow
+            bool isOnRemoteMachine = _psesHost.CurrentRunspace.IsOnRemoteMachine;
+            string returnSerializedIfOnRemoteMachine = isOnRemoteMachine
+                ? $"[Management.Automation.PSSerializer]::Serialize({callStackVarName}, {serializationDepth})"
+                : callStackVarName;
 
-            var results = await _executionService.ExecutePSCommandAsync<PSObject>(psCommand, CancellationToken.None).ConfigureAwait(false);
+            // We have to deal with a shallow serialization depth with ExecutePSCommandAsync as well, hence the serializer to get full var information
+            psCommand.AddScript($"[Collections.ArrayList]{callStackVarName} = @(); {getPSCallStack}; {returnSerializedIfOnRemoteMachine}");
 
-            var callStackFrames = results.ToArray();
 
-            this.stackFrameDetails = new StackFrameDetails[callStackFrames.Length];
+            // PSObject is used here instead of the specific type because we get deserialized objects from remote sessions and want a common interface
+            IReadOnlyList<PSObject> results = await _executionService.ExecutePSCommandAsync<PSObject>(psCommand, CancellationToken.None).ConfigureAwait(false);
 
-            for (int i = 0; i < callStackFrames.Length; i++)
+            IEnumerable callStack = isOnRemoteMachine
+                ? (PSSerializer.Deserialize(results[0].BaseObject as string) as PSObject).BaseObject as IList
+                : results;
+
+            List<StackFrameDetails> stackFrameDetailList = new List<StackFrameDetails>();
+            foreach (var callStackFrameItem in callStack)
             {
-                VariableContainerDetails autoVariables =
-                    new VariableContainerDetails(
-                        this.nextVariableId++,
-                        VariableContainerDetails.AutoVariablesName);
+                var callStackFrameComponents = (callStackFrameItem as PSObject).BaseObject as IList;
+                var callStackFrame = callStackFrameComponents[0] as PSObject;
+                IDictionary callStackVariables = isOnRemoteMachine
+                    ? (callStackFrameComponents[1] as PSObject).BaseObject as IDictionary
+                    : callStackFrameComponents[1] as IDictionary;
 
-                this.variables.Add(autoVariables);
+                var autoVariables = new VariableContainerDetails(
+                    nextVariableId++,
+                    VariableContainerDetails.AutoVariablesName);
 
-                VariableContainerDetails localVariables =
-                    await FetchVariableContainerAsync(i.ToString(), autoVariables).ConfigureAwait(false);
+                variables.Add(autoVariables);
 
-                // When debugging, this is the best way I can find to get what is likely the workspace root.
-                // This is controlled by the "cwd:" setting in the launch config.
-                string workspaceRootPath = _psesHost.InitialWorkingDirectory;
+                foreach (DictionaryEntry entry in callStackVariables)
+                {
+                    // TODO: This should be deduplicated into a new function for the other variable handling as well
+                    object psVarValue = isOnRemoteMachine
+                        ? (entry.Value as PSObject).Properties["Value"].Value
+                        : (entry.Value as PSVariable).Value;
+                    // The constructor we are using here does not automatically add the dollar prefix
+                    string psVarName = VariableDetails.DollarPrefix + entry.Key.ToString();
+                    var variableDetails = new VariableDetails(psVarName, psVarValue) { Id = nextVariableId++ };
+                    variables.Add(variableDetails);
+
+                    if (AddToAutoVariables(new PSObject(entry.Value), scope: null))
+                    {
+                        autoVariables.Children.Add(variableDetails.Name, variableDetails);
+                    }
+                }
 
-                this.stackFrameDetails[i] =
-                    StackFrameDetails.Create(callStackFrames[i], autoVariables, localVariables, workspaceRootPath);
+                var stackFrameDetailsEntry = StackFrameDetails.Create(callStackFrame, autoVariables);
 
-                string stackFrameScriptPath = this.stackFrameDetails[i].ScriptPath;
-                if (scriptNameOverride != null &&
+                string stackFrameScriptPath = stackFrameDetailsEntry.ScriptPath;
+                if (scriptNameOverride is not null &&
                     string.Equals(stackFrameScriptPath, StackFrameDetails.NoFileScriptPath))
                 {
-                    this.stackFrameDetails[i].ScriptPath = scriptNameOverride;
+                    stackFrameDetailsEntry.ScriptPath = scriptNameOverride;
                 }
-                else if (_psesHost.CurrentRunspace.IsOnRemoteMachine
-                    && this.remoteFileManager != null
+                else if (isOnRemoteMachine
+                    && remoteFileManager is not null
                     && !string.Equals(stackFrameScriptPath, StackFrameDetails.NoFileScriptPath))
                 {
-                    this.stackFrameDetails[i].ScriptPath =
-                        this.remoteFileManager.GetMappedPath(
+                    stackFrameDetailsEntry.ScriptPath =
+                        remoteFileManager.GetMappedPath(
                             stackFrameScriptPath,
                             _psesHost.CurrentRunspace);
                 }
+
+                stackFrameDetailList.Add(
+                    stackFrameDetailsEntry);
             }
+
+            stackFrameDetails = stackFrameDetailList.ToArray();
         }
 
         private static string TrimScriptListingLine(PSObject scriptLineObj, ref int prefixLength)

src/PowerShellEditorServices/Services/DebugAdapter/Debugging/StackFrameDetails.cs

@@ -64,11 +64,6 @@ internal class StackFrameDetails
         /// </summary>
         public VariableContainerDetails AutoVariables { get; private set; }
 
-        /// <summary>
-        /// Gets or sets the VariableContainerDetails that contains the local variables.
-        /// </summary>
-        public VariableContainerDetails LocalVariables { get; private set; }
-
         #endregion
 
         #region Constructors
@@ -83,47 +78,26 @@ internal class StackFrameDetails
         /// <param name="autoVariables">
         /// A variable container with all the filtered, auto variables for this stack frame.
         /// </param>
-        /// <param name="localVariables">
-        /// A variable container with all the local variables for this stack frame.
-        /// </param>
-        /// <param name="workspaceRootPath">
-        /// Specifies the path to the root of an open workspace, if one is open. This path is used to
-        /// determine whether individua stack frames are external to the workspace.
-        /// </param>
         /// <returns>A new instance of the StackFrameDetails class.</returns>
         static internal StackFrameDetails Create(
             PSObject callStackFrameObject,
-            VariableContainerDetails autoVariables,
-            VariableContainerDetails localVariables,
-            string workspaceRootPath = null)
+            VariableContainerDetails autoVariables)
         {
-            string moduleId = string.Empty;
-            var isExternal = false;
-
-            var invocationInfo = callStackFrameObject.Properties["InvocationInfo"]?.Value as InvocationInfo;
             string scriptPath = (callStackFrameObject.Properties["ScriptName"].Value as string) ?? NoFileScriptPath;
             int startLineNumber = (int)(callStackFrameObject.Properties["ScriptLineNumber"].Value ?? 0);
 
-            // TODO: RKH 2019-03-07 Temporarily disable "external" code until I have a chance to add
-            // settings to control this feature.
-            //if (workspaceRootPath != null &&
-            //    invocationInfo != null &&
-            //    !scriptPath.StartsWith(workspaceRootPath, StringComparison.OrdinalIgnoreCase))
-            //{
-            //    isExternal = true;
-            //}
-
             return new StackFrameDetails
             {
                 ScriptPath = scriptPath,
                 FunctionName = callStackFrameObject.Properties["FunctionName"].Value as string,
                 StartLineNumber = startLineNumber,
                 EndLineNumber = startLineNumber, // End line number isn't given in PowerShell stack frames
-                StartColumnNumber = 0,   // Column number isn't given in PowerShell stack frames
+                StartColumnNumber = 0, // Column number isn't given in PowerShell stack frames
                 EndColumnNumber = 0,
                 AutoVariables = autoVariables,
-                LocalVariables = localVariables,
-                IsExternalCode = isExternal
+                // TODO: Re-enable `isExternal` detection along with a setting. Will require
+                // `workspaceRootPath`, see Git blame.
+                IsExternalCode = false
             };
         }