Fix `DebuggerSetsVariablesWithConversion` test #1664

andyleejordan · 2022-01-13T18:00:06Z

With these two bugs fixed, we can now set variables which are strongly typed (and so need a conversion) through the debugger. This was a great unit test that indicated a bug, and it was easily reproducible.

While testing manually, the scope bug revealed itself when I attempted to set a variable in the "automatic variables" container (the test is using the local variables container). I think assuming a default scope of "0" is sane because if it's an automatic variable, it must be in the local scope, and every other container is mapped to a named scope. As we learned previously, we cannot "guess" at a scope because they do not map one-to-one with stack frames.

The conversion bug was due to SessionStateProxy.GetVariable("ExecutionContext") throwing NoSessionStateProxyWhenPipelineInProgress. Initially I thought it could be solved by running it forcibly in the foreground (as the stack-trace showed it was being run via OnIdle); however, the same exception was thrown even then. I am not certain, but I think it is because the debugger is currently in a stopped state, which has left the pipeline running, and so this concurrency check fails. I don't know why it can't work concurrently, because this alternative means to get the EngineIntrinsics value works, and indeed, was the way we did it previously:

PowerShellEditorServices/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

Lines 499 to 521 in 99b5b7e

    
           if (argTypeConverterAttr != null) 
        
           { 
        
               // PSVariable is strongly typed. Need to apply the conversion/transform to the new value. 
        
               psCommand.Commands.Clear(); 
        
               psCommand = new PSCommand(); 
        
               psCommand.AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable"); 
        
               psCommand.AddParameter("Name", "ExecutionContext"); 
        
               psCommand.AddParameter("ValueOnly"); 
        
               errorMessages.Clear(); 
        
               var getExecContextResults = 
        
                   await this.powerShellContext.ExecuteCommandAsync<object>( 
        
                       psCommand, 
        
                       errorMessages, 
        
                       sendErrorToHost: false).ConfigureAwait(false); 
        
               EngineIntrinsics executionContext = getExecContextResults.OfType<EngineIntrinsics>().FirstOrDefault(); 
        
               var msg = $"Setting variable '{name}' using conversion to value: {psobject ?? "<null>"}"; 
        
               this.logger.LogTrace(msg); 
        
               psVariable.Value = argTypeConverterAttr.Transform(executionContext, psobject);

Fixes #1661

andyleejordan · 2022-01-13T18:01:40Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+            // variable in the debugger and so change it through this interface, it's either in the
+            // top-most scope or in one of the following named scopes. The default scope is most
+            // likely in the case of changing from the "auto variables" container.
+            string scope = "0";


@SeeminglyScience This seem sane to you?

andyleejordan · 2022-01-13T18:02:21Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+                // run concurrently (threw 'NoSessionStateProxyWhenPipelineInProgress').
+                IReadOnlyList<EngineIntrinsics> results = await _executionService.ExecutePSCommandAsync<EngineIntrinsics>(
+                    new PSCommand()
+                        .AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable")


@rjmholt Do you have a better idea or explanation as to why that exception was being thrown?

This looks correct to me. The debugger knows it is running in an active pipeline and runs commands as 'nested' on the target (locally or remotely), whereas SessionStateProxy does not.

andyleejordan · 2022-01-13T18:03:46Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+                // TODO: This is almost (but not quite) the same as 'LanguagePrimitives.Convert()',
+                // which does not require the pipeline thread. We should investigate changing it.


I looked into this, but I don't think it will quite as "well" as the transformation, and I'm unsure actually how to go about it because LanguagePrimitives.ConvertTo() doesn't accept an ArgumentTypeConverterAttribute.

This will only work for local debugging sessions. Does this code make local/remote distinctions?

It does not make that distinction. Also, do you mean that LanguagePrimitives would only work for local, or the currently proposed change?

The ExecutionContext variable (engineIntrinsics) passes through the serialization system for the remoting case, and is just a PSObject property bag and not a System.Management.Automation.EngineIntrinsics type. The serialization system does not rehydrate returned object into their original types, except for a small set of special cases.

So if you have script code that determines variable scope, that script should run entirely on the remote target, so that it can run in the correct context and 'live' type instances.

I'm not quite sure I'm following. Can I ask: will the proposed change behave differently than the existing code in the remoting scenario? If not, then let's kick that down the road a bit.

(I.e. are both implementations broken in the same way?)

Yes, I think so (remoting may be broken). But I need to look more closely at the code (and not just the changes).

andyleejordan · 2022-01-13T18:04:11Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

@@ -641,7 +645,7 @@ private Task<VariableContainerDetails> FetchVariableContainerAsync(string scope)

        private async Task<VariableContainerDetails> FetchVariableContainerAsync(string scope, bool autoVarsOnly)
        {
-            PSCommand psCommand = new PSCommand().AddCommand("Get-Variable").AddParameter("Scope", scope);
+            PSCommand psCommand = new PSCommand().AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable").AddParameter("Scope", scope);


I'm not sure why we consistently use the fully qualified name, but since we do, fixed this one too.

andyleejordan · 2022-01-13T18:05:32Z

test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs

@@ -628,7 +628,7 @@ public async Task DebuggerSetsVariablesWithConversion()
            VariableDetailsBase[] variables = GetVariables(VariableContainerDetails.LocalScopeName);

            // Test set of a local string variable (not strongly typed but force conversion)
-            const string newStrValue = "False";
+            const string newStrValue = "\"False\"";


This was erroneously changed by me previously, and is now back to its original value.

andyleejordan · 2022-01-13T18:08:08Z

test/PowerShellEditorServices.Test/PsesHostFactory.cs

                consoleReplEnabled: false,
                usesLegacyReadLine: false,
                bundledModulePath: BundledModulePath);

            var psesHost = new PsesInternalHost(loggerFactory, null, testHostDetails);

            // NOTE: Because this is used by constructors it can't use await.
-            // TODO: Should we actually load profiles here?
            if (psesHost.TryStartAsync(new HostStartOptions { LoadProfiles = true }, CancellationToken.None).GetAwaiter().GetResult())


Hm, now I'm actually wondering if this should be false. I thought I had my answer, and that's that we test we can load the profiles but we actually do the profile loading call explicitly. @rjmholt Do you know why this is explicitly set to true?

* Remove unused field * Fix some indentation * Name all arguments * Delete TODO that's been answered

While we could put the entire task inside the lambda, thus eliminating the warning, we then lose the ability to assert that the task itself was cancelled. This feels important to be able to do since the exception does not necessarily prove that this explicit task was canceled.

andyleejordan · 2022-01-13T18:46:49Z

test/PowerShellEditorServices.Test/PsesHostFactory.cs

                consoleReplEnabled: false,
                usesLegacyReadLine: false,
                bundledModulePath: BundledModulePath);

            var psesHost = new PsesInternalHost(loggerFactory, null, testHostDetails);

            // NOTE: Because this is used by constructors it can't use await.
-            // TODO: Should we actually load profiles here?
-            if (psesHost.TryStartAsync(new HostStartOptions { LoadProfiles = true }, CancellationToken.None).GetAwaiter().GetResult())
+            if (psesHost.TryStartAsync(new HostStartOptions { LoadProfiles = false }, CancellationToken.None).GetAwaiter().GetResult())


I've now set this to false because the tests which assert profile loading works do so by explicitly running the same code as would otherwise run in startup:

PowerShellEditorServices/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs

Lines 217 to 221 in 20a3516

if (startOptions.LoadProfiles)

{

await LoadHostProfilesAsync(cancellationToken).ConfigureAwait(false);

_logger.LogInformation("Profiles loaded");

}

I don't think we really need to be calling this delegate for every other test.

PaulHigin

Added a couple of comments/questions.

PaulHigin · 2022-01-18T21:51:53Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+                // run concurrently (threw 'NoSessionStateProxyWhenPipelineInProgress').
+                IReadOnlyList<EngineIntrinsics> results = await _executionService.ExecutePSCommandAsync<EngineIntrinsics>(
+                    new PSCommand()
+                        .AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable")


This looks correct to me. The debugger knows it is running in an active pipeline and runs commands as 'nested' on the target (locally or remotely), whereas SessionStateProxy does not.

PaulHigin · 2022-01-18T21:57:20Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

+                // TODO: This is almost (but not quite) the same as 'LanguagePrimitives.Convert()',
+                // which does not require the pipeline thread. We should investigate changing it.


This will only work for local debugging sessions. Does this code make local/remote distinctions?

andyleejordan requested review from daxian-dbw, rjmholt and SeeminglyScience January 13, 2022 18:00

ghost added Area-Debugging Issue-Bug A bug to squash. labels Jan 13, 2022

andyleejordan commented Jan 13, 2022

View reviewed changes

andyleejordan added 4 commits January 13, 2022 10:42

Clean up PsesHostFactory

4ea50b7

* Remove unused field * Fix some indentation * Name all arguments * Delete TODO that's been answered

Fix scope guessing logic in SetVariableAsync

fa5db9e

Fix bug in SetVariableAsync for strongly typed values

2fdae0e

andyleejordan force-pushed the andschwa/set-var branch from 721a9a6 to ccddeba Compare January 13, 2022 18:42

andyleejordan commented Jan 13, 2022

View reviewed changes

PaulHigin approved these changes Jan 18, 2022

View reviewed changes

andyleejordan merged commit 39d26ee into master Jan 19, 2022

andyleejordan deleted the andschwa/set-var branch January 19, 2022 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `DebuggerSetsVariablesWithConversion` test #1664

Fix `DebuggerSetsVariablesWithConversion` test #1664

andyleejordan commented Jan 13, 2022

andyleejordan Jan 13, 2022

andyleejordan Jan 13, 2022

PaulHigin Jan 18, 2022

andyleejordan Jan 13, 2022

PaulHigin Jan 18, 2022

andyleejordan Jan 18, 2022

PaulHigin Jan 18, 2022

andyleejordan Jan 18, 2022

andyleejordan Jan 19, 2022

PaulHigin Jan 19, 2022

andyleejordan Jan 13, 2022

andyleejordan Jan 13, 2022

andyleejordan Jan 13, 2022

andyleejordan Jan 13, 2022

PaulHigin left a comment

PaulHigin Jan 18, 2022

PaulHigin Jan 18, 2022

	if (argTypeConverterAttr != null)
	{
	// PSVariable is strongly typed. Need to apply the conversion/transform to the new value.
	psCommand.Commands.Clear();
	psCommand = new PSCommand();
	psCommand.AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable");
	psCommand.AddParameter("Name", "ExecutionContext");
	psCommand.AddParameter("ValueOnly");

	errorMessages.Clear();

	var getExecContextResults =
	await this.powerShellContext.ExecuteCommandAsync<object>(
	psCommand,
	errorMessages,
	sendErrorToHost: false).ConfigureAwait(false);

	EngineIntrinsics executionContext = getExecContextResults.OfType<EngineIntrinsics>().FirstOrDefault();

	var msg = $"Setting variable '{name}' using conversion to value: {psobject ?? "<null>"}";
	this.logger.LogTrace(msg);

	psVariable.Value = argTypeConverterAttr.Transform(executionContext, psobject);

		// TODO: This is almost (but not quite) the same as 'LanguagePrimitives.Convert()',
		// which does not require the pipeline thread. We should investigate changing it.

	if (startOptions.LoadProfiles)
	{
	await LoadHostProfilesAsync(cancellationToken).ConfigureAwait(false);
	_logger.LogInformation("Profiles loaded");
	}

Fix DebuggerSetsVariablesWithConversion test #1664

Fix DebuggerSetsVariablesWithConversion test #1664

Conversation

andyleejordan commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulHigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix `DebuggerSetsVariablesWithConversion` test #1664

Fix `DebuggerSetsVariablesWithConversion` test #1664