From 4ea50b7befcbd4040414bb66d71f2801979a200b Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Wed, 12 Jan 2022 19:09:11 -0800 Subject: [PATCH 1/4] Clean up `PsesHostFactory` * Remove unused field * Fix some indentation * Name all arguments * Delete TODO that's been answered --- .../PsesHostFactory.cs | 35 +++++++------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/test/PowerShellEditorServices.Test/PsesHostFactory.cs b/test/PowerShellEditorServices.Test/PsesHostFactory.cs index 14cb822b4..84c4e5ee5 100644 --- a/test/PowerShellEditorServices.Test/PsesHostFactory.cs +++ b/test/PowerShellEditorServices.Test/PsesHostFactory.cs @@ -20,21 +20,13 @@ internal static class PsesHostFactory // NOTE: These paths are arbitrarily chosen just to verify that the profile paths can be set // to whatever they need to be for the given host. - public static readonly ProfilePathInfo TestProfilePaths = - new( - Path.GetFullPath( - TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/Test.PowerShellEditorServices_profile.ps1")), - Path.GetFullPath( - TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/ProfileTest.ps1")), - Path.GetFullPath( - TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Test.PowerShellEditorServices_profile.ps1")), - Path.GetFullPath( - TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/ProfileTest.ps1"))); + public static readonly ProfilePathInfo TestProfilePaths = new( + Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/Test.PowerShellEditorServices_profile.ps1")), + Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/ProfileTest.ps1")), + Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Test.PowerShellEditorServices_profile.ps1")), + Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/ProfileTest.ps1"))); - public static readonly string BundledModulePath = Path.GetFullPath( - TestUtilities.NormalizePath("../../../../../module")); - - public static System.Management.Automation.Runspaces.Runspace InitialRunspace; + public static readonly string BundledModulePath = Path.GetFullPath(TestUtilities.NormalizePath("../../../../../module")); public static PsesInternalHost Create(ILoggerFactory loggerFactory) { @@ -53,16 +45,16 @@ public static PsesInternalHost Create(ILoggerFactory loggerFactory) } HostStartupInfo testHostDetails = new( - "PowerShell Editor Services Test Host", - "Test.PowerShellEditorServices", - new Version("1.0.0"), + name: "PowerShell Editor Services Test Host", + profileId: "Test.PowerShellEditorServices", + version: new Version("1.0.0"), psHost: new NullPSHost(), - TestProfilePaths, + profilePaths: TestProfilePaths, featureFlags: Array.Empty(), additionalModules: Array.Empty(), - initialSessionState, + initialSessionState: initialSessionState, logPath: null, - (int)LogLevel.None, + logLevel: (int)LogLevel.None, consoleReplEnabled: false, usesLegacyReadLine: false, bundledModulePath: BundledModulePath); @@ -70,8 +62,7 @@ public static PsesInternalHost Create(ILoggerFactory loggerFactory) 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()) { return psesHost; } From fa5db9ea5aae46f47263e07e053817494cd7078f Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Wed, 12 Jan 2022 19:19:30 -0800 Subject: [PATCH 2/4] Fix scope guessing logic in `SetVariableAsync` --- .../Services/DebugAdapter/DebugService.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs b/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs index dba5c391f..064e5cea0 100644 --- a/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs +++ b/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs @@ -400,8 +400,14 @@ public async Task SetVariableAsync(int variableContainerReferenceId, str } VariableDetailsBase variable = variableContainer.Children[name]; - // Determine scope in which the variable lives so we can pass it to `Get-Variable -Scope`. - string scope = null; // TODO: Can this use a fancy pattern matcher? + + // Determine scope in which the variable lives so we can pass it to `Get-Variable + // -Scope`. The default is scope 0 which is safe because if a user is able to see a + // 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"; + // NOTE: This can't use a switch because the IDs aren't constant. if (variableContainerReferenceId == localScopeVariables.Id) { scope = VariableContainerDetails.LocalScopeName; @@ -414,11 +420,6 @@ public async Task SetVariableAsync(int variableContainerReferenceId, str { scope = VariableContainerDetails.GlobalScopeName; } - else - { - // Hmm, this would be unexpected. No scope means do not pass GO, do not collect $200. - throw new Exception("Could not find the scope for this variable."); - } // Now that we have the scope, get the associated PSVariable object for the variable to be set. var getVariableCommand = new PSCommand() From 2fdae0ef9fa5849ea49a7aca1888ae4f8d43c10f Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Thu, 13 Jan 2022 09:50:57 -0800 Subject: [PATCH 3/4] Fix bug in `SetVariableAsync` for strongly typed values --- .../Services/DebugAdapter/DebugService.cs | 31 ++++++++++--------- .../Debugging/DebugServiceTests.cs | 6 ++-- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs b/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs index 064e5cea0..383ecf4fa 100644 --- a/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs +++ b/src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs @@ -457,22 +457,25 @@ public async Task SetVariableAsync(int variableContainerReferenceId, str if (argTypeConverterAttr is not null) { + // PSVariable *is* strongly typed, so we have to convert it. _logger.LogTrace($"Setting variable '{name}' using conversion to value: {expressionResult ?? ""}"); - // TODO: This is throwing a 'PSInvalidOperationException' thus causing - // 'DebuggerSetsVariablesWithConversion' to fail. - psVariable.Value = await _executionService.ExecuteDelegateAsync( - "PS debugger argument converter", - ExecutionOptions.Default, - (pwsh, _) => - { - var engineIntrinsics = (EngineIntrinsics)pwsh.Runspace.SessionStateProxy.GetVariable("ExecutionContext"); - - // TODO: This is almost (but not quite) the same as LanguagePrimitives.Convert(), which does not require the pipeline thread. - // We should investigate changing it. - return argTypeConverterAttr.Transform(engineIntrinsics, expressionResult); - }, + // NOTE: We use 'Get-Variable' here instead of 'SessionStateProxy.GetVariable()' + // because we already have a pipeline running (the debugger) and the latter cannot + // run concurrently (threw 'NoSessionStateProxyWhenPipelineInProgress'). + IReadOnlyList results = await _executionService.ExecutePSCommandAsync( + new PSCommand() + .AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable") + .AddParameter("Name", "ExecutionContext") + .AddParameter("ValueOnly"), CancellationToken.None).ConfigureAwait(false); + EngineIntrinsics engineIntrinsics = results.Count > 0 + ? results[0] + : throw new Exception("Couldn't get EngineIntrinsics!"); + + // TODO: This is almost (but not quite) the same as 'LanguagePrimitives.Convert()', + // which does not require the pipeline thread. We should investigate changing it. + psVariable.Value = argTypeConverterAttr.Transform(engineIntrinsics, expressionResult); } else { @@ -642,7 +645,7 @@ private Task FetchVariableContainerAsync(string scope) private async Task 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); var scopeVariableContainer = new VariableContainerDetails(nextVariableId++, "Scope: " + scope); variables.Add(scopeVariableContainer); diff --git a/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs b/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs index d36ced933..51e2f49ca 100644 --- a/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs +++ b/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs @@ -613,7 +613,7 @@ await debugService.SetLineBreakpointsAsync( Assert.Equal(newGlobalIntValue, intGlobalVar.ValueString); } - [Fact(Skip = "Variable conversion is broken")] + [Fact] public async Task DebuggerSetsVariablesWithConversion() { await debugService.SetLineBreakpointsAsync( @@ -628,7 +628,7 @@ await debugService.SetLineBreakpointsAsync( 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\""; const string newStrExpr = "$false"; VariableScope localScope = Array.Find(scopes, s => s.Name == VariableContainerDetails.LocalScopeName); string setStrValue = await debugService.SetVariableAsync(localScope.Id, "$strVar2", newStrExpr).ConfigureAwait(true); @@ -658,8 +658,6 @@ await debugService.SetLineBreakpointsAsync( var strVar = Array.Find(variables, v => v.Name == "$strVar2"); Assert.Equal(newStrValue, strVar.ValueString); - scopes = debugService.GetVariableScopes(0); - // Test set of script scope bool variable (strongly typed) variables = GetVariables(VariableContainerDetails.ScriptScopeName); var boolVar = Array.Find(variables, v => v.Name == "$scriptBool"); From ccddebaaca0f3911a805a761d28e02c9720e37b6 Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Thu, 13 Jan 2022 10:41:07 -0800 Subject: [PATCH 4/4] Suppress warning in `PsesInternalHostTests` 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. --- .../Session/PsesInternalHostTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs index dffc444c4..1598c10e2 100644 --- a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs +++ b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs @@ -94,6 +94,7 @@ await Assert.ThrowsAsync(() => } [Fact] + [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD003:Avoid awaiting foreign Tasks", Justification = "Explicitly checking task cancellation status.")] public async Task CanCancelExecutionWithMethod() { var executeTask = psesHost.ExecutePSCommandAsync(