Skip to content

Commit

Permalink
Fix the TranscribeOnly bug (take two)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
andyleejordan committed May 18, 2023
1 parent 253422a commit 7ba4620
Show file tree
Hide file tree
Showing 8 changed files with 91 additions and 55 deletions.
2 changes: 2 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions PowerShellEditorServices.Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,7 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<!-- TODO: Enable <AnalysisMode>All</AnalysisMode> -->
</PropertyGroup>
<PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
<DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
</PropertyGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@
<AssemblyName>Microsoft.PowerShell.EditorServices.Hosting</AssemblyName>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net6.0' ">
<DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="NETStandard.Library" Version="2.0.3" />
<PackageReference Include="PowerShellStandard.Library" Version="5.1.1" PrivateAssets="all" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<Description>Provides added functionality to PowerShell Editor Services for the Visual Studio Code editor.</Description>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<AssemblyName>Microsoft.PowerShell.EditorServices.VSCode</AssemblyName>
<Configurations>Debug;Release;CoreCLR</Configurations>
<Configurations>Debug;Release</Configurations>
</PropertyGroup>

<!-- Fail the release build if there are missing public API documentation comments -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
using Microsoft.PowerShell.EditorServices.Utility;
using SMA = System.Management.Automation;

#if !CoreCLR
using System.Management.Automation.Host;
using System.Reflection;
#endif

namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Execution
{
internal interface ISynchronousPowerShellTask
Expand All @@ -27,6 +32,55 @@ internal class SynchronousPowerShellTask<TResult> : SynchronousTask<IReadOnlyLis
{
private static readonly PowerShellExecutionOptions s_defaultPowerShellExecutionOptions = new();

#if !CoreCLR
/// <summary>
/// To workaround a bug where the `TranscribeOnly` field of the PSHostUserInterface can
/// accidentally remain true, we have to use a PowerShell pipeline to get the internal
/// instance (saved here) and reflection to get delegates for the field's getter and setter
/// methods, and then reset it to false (see <see cref="ExecuteNormally"/>). Note that it
/// must be the internal instance, not our own UI instance.
/// See https://github.com/PowerShell/PowerShell/pull/3436
/// </summary>
[ThreadStatic] // Because we can re-use it, but only for each PowerShell.
private static PSHostUserInterface s_internalPSHostUserInterface;

private static readonly Func<PSHostUserInterface, bool> s_getTranscribeOnlyDelegate;

private static readonly Action<PSHostUserInterface, bool> s_setTranscribeOnlyDelegate;

private static readonly PropertyInfo s_executionContextProperty;

private static readonly PropertyInfo s_internalHostProperty;

private static readonly PropertyInfo s_internalHostUIProperty;

static SynchronousPowerShellTask()
{
PropertyInfo transcribeOnlyProperty = typeof(PSHostUserInterface)
.GetProperty("TranscribeOnly", BindingFlags.NonPublic | BindingFlags.Instance);

MethodInfo transcribeOnlyGetMethod = transcribeOnlyProperty.GetGetMethod(nonPublic: true);

s_getTranscribeOnlyDelegate = (Func<PSHostUserInterface, bool>)Delegate.CreateDelegate(
typeof(Func<PSHostUserInterface, bool>), transcribeOnlyGetMethod);

MethodInfo transcribeOnlySetMethod = transcribeOnlyProperty.GetSetMethod(nonPublic: true);

s_setTranscribeOnlyDelegate = (Action<PSHostUserInterface, bool>)Delegate.CreateDelegate(
typeof(Action<PSHostUserInterface, bool>), transcribeOnlySetMethod);

s_executionContextProperty = typeof(SMA.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

private readonly ILogger _logger;

private readonly PsesInternalHost _psesHost;
Expand Down Expand Up @@ -105,6 +159,21 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok
if (PowerShellExecutionOptions.WriteOutputToHost)
{
_psCommand.AddOutputCommand();
#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`
// (we tried). With that internal UI we can reset its `TranscribeOnly` flag and so
// avoid the disappearing output that happens when a transcription is running.
if (!_pwsh.Runspace.RunspaceIsRemote)
{
s_internalPSHostUserInterface ??=
s_internalHostUIProperty.GetValue(
s_internalHostProperty.GetValue(
s_executionContextProperty.GetValue(_pwsh.Runspace)))
as PSHostUserInterface;
DisableTranscribeOnly();
}
#endif
}

cancellationToken.Register(CancelNormalExecution);
Expand Down Expand Up @@ -148,7 +217,7 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok
if (e is PSRemotingTransportException)
{
_ = System.Threading.Tasks.Task.Run(
() => _psesHost.UnwindCallStack(),
_psesHost.UnwindCallStack,
CancellationToken.None)
.HandleErrorsAsync(_logger);

Expand Down Expand Up @@ -189,8 +258,6 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok

private IReadOnlyList<TResult> 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<PSObject> outputCollection = new();
Expand Down Expand Up @@ -247,7 +314,7 @@ private IReadOnlyList<TResult> ExecuteInDebugger(CancellationToken cancellationT
if (e is PSRemotingTransportException)
{
_ = System.Threading.Tasks.Task.Run(
() => _psesHost.UnwindCallStack(),
_psesHost.UnwindCallStack,
CancellationToken.None)
.HandleErrorsAsync(_logger);

Expand Down Expand Up @@ -396,5 +463,18 @@ private void CancelDebugExecution()

_pwsh.Runspace.Debugger.StopProcessCommand();
}

#if !CoreCLR
// 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.
private static void DisableTranscribeOnly()
{
if (s_getTranscribeOnlyDelegate(s_internalPSHostUserInterface))
{
s_setTranscribeOnlyDelegate(s_internalPSHostUserInterface, false);
}
}
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,39 +7,21 @@
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
{
internal class EditorServicesConsolePSHostUserInterface : PSHostUserInterface, IHostUISupportsMultipleChoiceSelection
{
private readonly PSHostUserInterface _underlyingHostUI;

private static readonly Action<PSHostUserInterface, bool> s_setTranscribeOnlyDelegate;

/// <summary>
/// We use a ConcurrentDictionary because ConcurrentHashSet does not exist, hence the value
/// is never actually used, and `WriteProgress` must be thread-safe.
/// </summary>
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<PSHostUserInterface, bool>)Delegate.CreateDelegate(
typeof(Action<PSHostUserInterface, bool>), transcribeOnlySetMethod);
}
}

public EditorServicesConsolePSHostUserInterface(
ILoggerFactory loggerFactory,
PSHostUserInterface underlyingHostUI)
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -476,19 +476,7 @@ public void InvokeDelegate(string representation, ExecutionOptions executionOpti
public IReadOnlyList<TResult> InvokePSCommand<TResult>(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken)
{
SynchronousPowerShellTask<TResult> 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<PSObject>(psCommand, executionOptions, cancellationToken);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@
<PackageReference Include="Microsoft.PowerShell.5.ReferenceAssemblies" Version="1.1.0" />
</ItemGroup>

<PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
<DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.5.0" />
<PackageReference Include="xunit" Version="2.4.2" />
Expand Down

0 comments on commit 7ba4620

Please sign in to comment.