From 0e17030d9ed2859facb5a2e7d49cdf5c5b5b8dbd Mon Sep 17 00:00:00 2001 From: Andy Jordan Date: Wed, 17 May 2023 12:08:56 -0700 Subject: [PATCH 1/2] Fix the `TranscribeOnly` bug (take two) We were using our own UI, not the byzantine internal UI where it actually needed to be fixed. Whole lot of reflection. Also had to fix our `CoreCLR` compiler constant. --- .editorconfig | 2 + .../PowerShellEditorServices.Hosting.csproj | 2 +- .../PowerShellEditorServices.VSCode.csproj | 6 +- .../PowerShellEditorServices.csproj | 4 + .../Execution/SynchronousPowerShellTask.cs | 12 ++- ...ditorServicesConsolePSHostUserInterface.cs | 29 ------ .../PowerShell/Host/PsesInternalHost.cs | 89 ++++++++++++++++--- .../PowerShellEditorServices.Test.E2E.csproj | 4 + .../PowerShellEditorServices.Test.csproj | 8 +- 9 files changed, 104 insertions(+), 52 deletions(-) diff --git a/.editorconfig b/.editorconfig index 1cb572ef7..79e4abe9a 100644 --- a/.editorconfig +++ b/.editorconfig @@ -205,6 +205,8 @@ dotnet_diagnostic.IDE0052.severity = error dotnet_diagnostic.IDE0053.severity = error # IDE0054: Use compound assignment dotnet_diagnostic.IDE0054.severity = error +# IDE0059: Unnecessary assignment of a value +dotnet_diagnostic.IDE0059.severity = error # IDE0063: Use simple 'using' statement dotnet_diagnostic.IDE0063.severity = error # IDE0066: Use switch expression diff --git a/src/PowerShellEditorServices.Hosting/PowerShellEditorServices.Hosting.csproj b/src/PowerShellEditorServices.Hosting/PowerShellEditorServices.Hosting.csproj index 118b25d73..5c31f0758 100644 --- a/src/PowerShellEditorServices.Hosting/PowerShellEditorServices.Hosting.csproj +++ b/src/PowerShellEditorServices.Hosting/PowerShellEditorServices.Hosting.csproj @@ -6,7 +6,7 @@ Microsoft.PowerShell.EditorServices.Hosting - + $(DefineConstants);CoreCLR diff --git a/src/PowerShellEditorServices.VSCode/PowerShellEditorServices.VSCode.csproj b/src/PowerShellEditorServices.VSCode/PowerShellEditorServices.VSCode.csproj index b6022679f..2624f1ddf 100644 --- a/src/PowerShellEditorServices.VSCode/PowerShellEditorServices.VSCode.csproj +++ b/src/PowerShellEditorServices.VSCode/PowerShellEditorServices.VSCode.csproj @@ -6,7 +6,11 @@ Provides added functionality to PowerShell Editor Services for the Visual Studio Code editor. netstandard2.0 Microsoft.PowerShell.EditorServices.VSCode - Debug;Release;CoreCLR + Debug;Release + + + + $(DefineConstants);CoreCLR diff --git a/src/PowerShellEditorServices/PowerShellEditorServices.csproj b/src/PowerShellEditorServices/PowerShellEditorServices.csproj index ef9b2696b..22517410a 100644 --- a/src/PowerShellEditorServices/PowerShellEditorServices.csproj +++ b/src/PowerShellEditorServices/PowerShellEditorServices.csproj @@ -9,6 +9,10 @@ Debug;Release + + $(DefineConstants);CoreCLR + + <_Parameter1>Microsoft.PowerShell.EditorServices.Hosting diff --git a/src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs b/src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs index 5256f5ad9..b96beae9e 100644 --- a/src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs +++ b/src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs @@ -105,6 +105,12 @@ private IReadOnlyList ExecuteNormally(CancellationToken cancellationTok if (PowerShellExecutionOptions.WriteOutputToHost) { _psCommand.AddOutputCommand(); + + // Fix the transcription bug! + if (!_pwsh.Runspace.RunspaceIsRemote) + { + _psesHost.DisableTranscribeOnly(); + } } cancellationToken.Register(CancelNormalExecution); @@ -148,7 +154,7 @@ private IReadOnlyList ExecuteNormally(CancellationToken cancellationTok if (e is PSRemotingTransportException) { _ = System.Threading.Tasks.Task.Run( - () => _psesHost.UnwindCallStack(), + _psesHost.UnwindCallStack, CancellationToken.None) .HandleErrorsAsync(_logger); @@ -189,8 +195,6 @@ private IReadOnlyList ExecuteNormally(CancellationToken cancellationTok private IReadOnlyList ExecuteInDebugger(CancellationToken cancellationToken) { - // TODO: How much of this method can we remove now that it only processes PowerShell's - // intrinsic debugger commands? cancellationToken.Register(CancelDebugExecution); PSDataCollection outputCollection = new(); @@ -247,7 +251,7 @@ private IReadOnlyList ExecuteInDebugger(CancellationToken cancellationT if (e is PSRemotingTransportException) { _ = System.Threading.Tasks.Task.Run( - () => _psesHost.UnwindCallStack(), + _psesHost.UnwindCallStack, CancellationToken.None) .HandleErrorsAsync(_logger); diff --git a/src/PowerShellEditorServices/Services/PowerShell/Host/EditorServicesConsolePSHostUserInterface.cs b/src/PowerShellEditorServices/Services/PowerShell/Host/EditorServicesConsolePSHostUserInterface.cs index badcaef6b..55ab29e7c 100644 --- a/src/PowerShellEditorServices/Services/PowerShell/Host/EditorServicesConsolePSHostUserInterface.cs +++ b/src/PowerShellEditorServices/Services/PowerShell/Host/EditorServicesConsolePSHostUserInterface.cs @@ -7,10 +7,8 @@ using System.Collections.ObjectModel; using System.Management.Automation; using System.Management.Automation.Host; -using System.Reflection; using System.Security; using Microsoft.Extensions.Logging; -using Microsoft.PowerShell.EditorServices.Utility; namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Host { @@ -18,28 +16,12 @@ internal class EditorServicesConsolePSHostUserInterface : PSHostUserInterface, I { private readonly PSHostUserInterface _underlyingHostUI; - private static readonly Action s_setTranscribeOnlyDelegate; - /// /// We use a ConcurrentDictionary because ConcurrentHashSet does not exist, hence the value /// is never actually used, and `WriteProgress` must be thread-safe. /// private readonly ConcurrentDictionary<(long, int), object> _currentProgressRecords = new(); - static EditorServicesConsolePSHostUserInterface() - { - if (VersionUtils.IsPS5) - { - PropertyInfo transcribeOnlyProperty = typeof(PSHostUserInterface) - .GetProperty("TranscribeOnly", BindingFlags.NonPublic | BindingFlags.Instance); - - MethodInfo transcribeOnlySetMethod = transcribeOnlyProperty.GetSetMethod(nonPublic: true); - - s_setTranscribeOnlyDelegate = (Action)Delegate.CreateDelegate( - typeof(Action), transcribeOnlySetMethod); - } - } - public EditorServicesConsolePSHostUserInterface( ILoggerFactory loggerFactory, PSHostUserInterface underlyingHostUI) @@ -105,17 +87,6 @@ internal void ResetProgress() // TODO: Maybe send the OSC sequence to turn off progress indicator. } - // This works around a bug in PowerShell 5.1 (that was later fixed) where a running - // transcription could cause output to disappear since the `TranscribeOnly` property was - // accidentally not reset to false. - internal void DisableTranscribeOnly() - { - if (VersionUtils.IsPS5) - { - s_setTranscribeOnlyDelegate(_underlyingHostUI, false); - } - } - public override void WriteVerboseLine(string message) => _underlyingHostUI.WriteVerboseLine(message); public override void WriteWarningLine(string message) => _underlyingHostUI.WriteWarningLine(message); diff --git a/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs b/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs index 32a728291..e87d97744 100644 --- a/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs +++ b/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs @@ -39,6 +39,33 @@ internal class PsesInternalHost : PSHost, IHostSupportsInteractiveSession, IRuns private static readonly PropertyInfo s_scriptDebuggerTriggerObjectProperty; +#if !CoreCLR + /// + /// To workaround a horrid bug where the `TranscribeOnly` field of the PSHostUserInterface + /// can accidentally remain true, we have to use a bunch of reflection so that can reset it to false. (This was fixed in PowerShell + /// 7.) Note that it must be the internal UI instance, not our own UI instance, otherwise + /// this would be easier. Because of the amount of reflection involved, we contain it to + /// only PowerShell 5.1 at compile-time, and we have to set this up in this class, not because that's templated, making statics practically + /// useless. method calls when necessary. + /// See: https://github.com/PowerShell/PowerShell/pull/3436 + /// + [ThreadStatic] // Because we can re-use it, but only for each PowerShell. + private static PSHostUserInterface s_internalPSHostUserInterface; + + private static readonly Func s_getTranscribeOnlyDelegate; + + private static readonly Action s_setTranscribeOnlyDelegate; + + private static readonly PropertyInfo s_executionContextProperty; + + private static readonly PropertyInfo s_internalHostProperty; + + private static readonly PropertyInfo s_internalHostUIProperty; +#endif + private readonly ILoggerFactory _loggerFactory; private readonly ILogger _logger; @@ -104,6 +131,31 @@ static PsesInternalHost() s_scriptDebuggerTriggerObjectProperty = scriptDebuggerType.GetProperty( "TriggerObject", BindingFlags.Instance | BindingFlags.NonPublic); + +#if !CoreCLR + PropertyInfo transcribeOnlyProperty = typeof(PSHostUserInterface) + .GetProperty("TranscribeOnly", BindingFlags.NonPublic | BindingFlags.Instance); + + MethodInfo transcribeOnlyGetMethod = transcribeOnlyProperty.GetGetMethod(nonPublic: true); + + s_getTranscribeOnlyDelegate = (Func)Delegate.CreateDelegate( + typeof(Func), transcribeOnlyGetMethod); + + MethodInfo transcribeOnlySetMethod = transcribeOnlyProperty.GetSetMethod(nonPublic: true); + + s_setTranscribeOnlyDelegate = (Action)Delegate.CreateDelegate( + typeof(Action), transcribeOnlySetMethod); + + s_executionContextProperty = typeof(System.Management.Automation.Runspaces.Runspace) + .GetProperty("ExecutionContext", BindingFlags.NonPublic | BindingFlags.Instance); + + s_internalHostProperty = s_executionContextProperty.PropertyType + .GetProperty("InternalHost", BindingFlags.NonPublic | BindingFlags.Instance); + + // It's public but we want the override and reflection confuses me. + s_internalHostUIProperty = s_internalHostProperty.PropertyType + .GetProperty("UI", BindingFlags.Public | BindingFlags.Instance); +#endif } public PsesInternalHost( @@ -476,19 +528,7 @@ public void InvokeDelegate(string representation, ExecutionOptions executionOpti public IReadOnlyList InvokePSCommand(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken) { SynchronousPowerShellTask task = new(_logger, this, psCommand, executionOptions, cancellationToken); - try - { - return task.ExecuteAndGetResult(cancellationToken); - } - finally - { - // At the end of each PowerShell command we need to reset PowerShell 5.1's - // `TranscribeOnly` property to avoid a bug where output disappears. - if (UI is EditorServicesConsolePSHostUserInterface ui) - { - ui.DisableTranscribeOnly(); - } - } + return task.ExecuteAndGetResult(cancellationToken); } public void InvokePSCommand(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken) => InvokePSCommand(psCommand, executionOptions, cancellationToken); @@ -507,6 +547,29 @@ public void InvokePSDelegate(string representation, ExecutionOptions executionOp internal void AddToHistory(string historyEntry) => _readLineProvider.ReadLine.AddToHistory(historyEntry); + // This works around a bug in PowerShell 5.1 (that was later fixed) where a running + // transcription could cause output to disappear since the `TranscribeOnly` property was + // accidentally not reset to false. +#pragma warning disable CA1822 // Warning to make it static when it's empty for CoreCLR. + internal void DisableTranscribeOnly() +#pragma warning restore CA1822 + { +#if !CoreCLR + // To fix the TranscribeOnly bug, we have to get the internal UI, which involves a lot + // of reflection since we can't always just use PowerShell to execute `$Host.UI`. + s_internalPSHostUserInterface ??= + s_internalHostUIProperty.GetValue( + s_internalHostProperty.GetValue( + s_executionContextProperty.GetValue(CurrentPowerShell.Runspace))) + as PSHostUserInterface; + + if (s_getTranscribeOnlyDelegate(s_internalPSHostUserInterface)) + { + s_setTranscribeOnlyDelegate(s_internalPSHostUserInterface, false); + } +#endif + } + internal Task LoadHostProfilesAsync(CancellationToken cancellationToken) { // NOTE: This is a special task run on startup! diff --git a/test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj b/test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj index 429720042..8c5ba099d 100644 --- a/test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj +++ b/test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj @@ -6,6 +6,10 @@ false + + $(DefineConstants);CoreCLR + + diff --git a/test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj b/test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj index de4666633..dc37d8d2c 100644 --- a/test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj +++ b/test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj @@ -7,6 +7,10 @@ x64 + + $(DefineConstants);CoreCLR + + @@ -27,10 +31,6 @@ - - $(DefineConstants);CoreCLR - - From ec17160ac01c06d4996af30a0671c06c485b7f26 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andschwa@users.noreply.github.com> Date: Thu, 18 May 2023 10:56:40 -0700 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Patrick Meinecke --- .../Services/PowerShell/Host/PsesInternalHost.cs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs b/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs index e87d97744..a94406e4c 100644 --- a/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs +++ b/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs @@ -52,7 +52,7 @@ internal class PsesInternalHost : PSHost, IHostSupportsInteractiveSession, IRuns /// cref="DisableTranscribeOnly()" /> when necessary. /// See: https://github.com/PowerShell/PowerShell/pull/3436 /// - [ThreadStatic] // Because we can re-use it, but only for each PowerShell. + [ThreadStatic] // Because we can re-use it, but only once per instance of PSES. private static PSHostUserInterface s_internalPSHostUserInterface; private static readonly Func s_getTranscribeOnlyDelegate; @@ -62,8 +62,6 @@ internal class PsesInternalHost : PSHost, IHostSupportsInteractiveSession, IRuns private static readonly PropertyInfo s_executionContextProperty; private static readonly PropertyInfo s_internalHostProperty; - - private static readonly PropertyInfo s_internalHostUIProperty; #endif private readonly ILoggerFactory _loggerFactory; @@ -151,10 +149,6 @@ static PsesInternalHost() s_internalHostProperty = s_executionContextProperty.PropertyType .GetProperty("InternalHost", BindingFlags.NonPublic | BindingFlags.Instance); - - // It's public but we want the override and reflection confuses me. - s_internalHostUIProperty = s_internalHostProperty.PropertyType - .GetProperty("UI", BindingFlags.Public | BindingFlags.Instance); #endif } @@ -558,10 +552,9 @@ internal void DisableTranscribeOnly() // To fix the TranscribeOnly bug, we have to get the internal UI, which involves a lot // of reflection since we can't always just use PowerShell to execute `$Host.UI`. s_internalPSHostUserInterface ??= - s_internalHostUIProperty.GetValue( - s_internalHostProperty.GetValue( - s_executionContextProperty.GetValue(CurrentPowerShell.Runspace))) - as PSHostUserInterface; + (s_internalHostProperty.GetValue( + s_executionContextProperty.GetValue(CurrentPowerShell.Runspace)) + as PSHost).UI; if (s_getTranscribeOnlyDelegate(s_internalPSHostUserInterface)) {