Skip to content

Commit 53220a6

Browse files
andyleejordanandyleejordan
committed
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.
1 parent 253422a commit 53220a6

File tree

8 files changed

+96
-52
lines changed

8 files changed

+96
-52
lines changed

‎.editorconfig‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,8 @@ dotnet_diagnostic.IDE0052.severity = error
205205
dotnet_diagnostic.IDE0053.severity = error
206206
# IDE0054: Use compound assignment
207207
dotnet_diagnostic.IDE0054.severity = error
208+
# IDE0059: Unnecessary assignment of a value
209+
dotnet_diagnostic.IDE0059.severity = error
208210
# IDE0063: Use simple 'using' statement
209211
dotnet_diagnostic.IDE0063.severity = error
210212
# IDE0066: Use switch expression

‎src/PowerShellEditorServices.Hosting/PowerShellEditorServices.Hosting.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
<AssemblyName>Microsoft.PowerShell.EditorServices.Hosting</AssemblyName>
77
</PropertyGroup>
88

9-
<PropertyGroup Condition=" '$(TargetFramework)' == 'net6.0' ">
9+
<PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
1010
<DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
1111
</PropertyGroup>
1212

‎src/PowerShellEditorServices.VSCode/PowerShellEditorServices.VSCode.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
<Description>Provides added functionality to PowerShell Editor Services for the Visual Studio Code editor.</Description>
77
<TargetFrameworks>netstandard2.0</TargetFrameworks>
88
<AssemblyName>Microsoft.PowerShell.EditorServices.VSCode</AssemblyName>
9-
<Configurations>Debug;Release;CoreCLR</Configurations>
9+
<Configurations>Debug;Release</Configurations>
1010
</PropertyGroup>
1111

1212
<!-- Fail the release build if there are missing public API documentation comments -->

‎src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,12 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok
105105
if (PowerShellExecutionOptions.WriteOutputToHost)
106106
{
107107
_psCommand.AddOutputCommand();
108+
109+
// Fix the transcription bug!
110+
if (!_pwsh.Runspace.RunspaceIsRemote)
111+
{
112+
_psesHost.DisableTranscribeOnly();
113+
}
108114
}
109115

110116
cancellationToken.Register(CancelNormalExecution);
@@ -148,7 +154,7 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok
148154
if (e is PSRemotingTransportException)
149155
{
150156
_ = System.Threading.Tasks.Task.Run(
151-
() => _psesHost.UnwindCallStack(),
157+
_psesHost.UnwindCallStack,
152158
CancellationToken.None)
153159
.HandleErrorsAsync(_logger);
154160

@@ -189,8 +195,6 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok
189195

190196
private IReadOnlyList<TResult> ExecuteInDebugger(CancellationToken cancellationToken)
191197
{
192-
// TODO: How much of this method can we remove now that it only processes PowerShell's
193-
// intrinsic debugger commands?
194198
cancellationToken.Register(CancelDebugExecution);
195199

196200
PSDataCollection<PSObject> outputCollection = new();
@@ -247,7 +251,7 @@ private IReadOnlyList<TResult> ExecuteInDebugger(CancellationToken cancellationT
247251
if (e is PSRemotingTransportException)
248252
{
249253
_ = System.Threading.Tasks.Task.Run(
250-
() => _psesHost.UnwindCallStack(),
254+
_psesHost.UnwindCallStack,
251255
CancellationToken.None)
252256
.HandleErrorsAsync(_logger);
253257

‎src/PowerShellEditorServices/Services/PowerShell/Host/EditorServicesConsolePSHostUserInterface.cs‎

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -7,39 +7,21 @@
77
using System.Collections.ObjectModel;
88
using System.Management.Automation;
99
using System.Management.Automation.Host;
10-
using System.Reflection;
1110
using System.Security;
1211
using Microsoft.Extensions.Logging;
13-
using Microsoft.PowerShell.EditorServices.Utility;
1412

1513
namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Host
1614
{
1715
internal class EditorServicesConsolePSHostUserInterface : PSHostUserInterface, IHostUISupportsMultipleChoiceSelection
1816
{
1917
private readonly PSHostUserInterface _underlyingHostUI;
2018

21-
private static readonly Action<PSHostUserInterface, bool> s_setTranscribeOnlyDelegate;
22-
2319
/// <summary>
2420
/// We use a ConcurrentDictionary because ConcurrentHashSet does not exist, hence the value
2521
/// is never actually used, and `WriteProgress` must be thread-safe.
2622
/// </summary>
2723
private readonly ConcurrentDictionary<(long, int), object> _currentProgressRecords = new();
2824

29-
static EditorServicesConsolePSHostUserInterface()
30-
{
31-
if (VersionUtils.IsPS5)
32-
{
33-
PropertyInfo transcribeOnlyProperty = typeof(PSHostUserInterface)
34-
.GetProperty("TranscribeOnly", BindingFlags.NonPublic | BindingFlags.Instance);
35-
36-
MethodInfo transcribeOnlySetMethod = transcribeOnlyProperty.GetSetMethod(nonPublic: true);
37-
38-
s_setTranscribeOnlyDelegate = (Action<PSHostUserInterface, bool>)Delegate.CreateDelegate(
39-
typeof(Action<PSHostUserInterface, bool>), transcribeOnlySetMethod);
40-
}
41-
}
42-
4325
public EditorServicesConsolePSHostUserInterface(
4426
ILoggerFactory loggerFactory,
4527
PSHostUserInterface underlyingHostUI)
@@ -105,17 +87,6 @@ internal void ResetProgress()
10587
// TODO: Maybe send the OSC sequence to turn off progress indicator.
10688
}
10789

108-
// This works around a bug in PowerShell 5.1 (that was later fixed) where a running
109-
// transcription could cause output to disappear since the `TranscribeOnly` property was
110-
// accidentally not reset to false.
111-
internal void DisableTranscribeOnly()
112-
{
113-
if (VersionUtils.IsPS5)
114-
{
115-
s_setTranscribeOnlyDelegate(_underlyingHostUI, false);
116-
}
117-
}
118-
11990
public override void WriteVerboseLine(string message) => _underlyingHostUI.WriteVerboseLine(message);
12091

12192
public override void WriteWarningLine(string message) => _underlyingHostUI.WriteWarningLine(message);

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

Lines changed: 76 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,33 @@ internal class PsesInternalHost : PSHost, IHostSupportsInteractiveSession, IRuns
3939

4040
private static readonly PropertyInfo s_scriptDebuggerTriggerObjectProperty;
4141

42+
#if !CoreCLR
43+
/// <summary>
44+
/// To workaround a horrid bug where the `TranscribeOnly` field of the PSHostUserInterface
45+
/// can accidentally remain true, we have to use a bunch of reflection so that <see
46+
/// cref="DisableTranscribeOnly()" /> can reset it to false. (This was fixed in PowerShell
47+
/// 7.) Note that it must be the internal UI instance, not our own UI instance, otherwise
48+
/// this would be easier. Because of the amount of reflection involved, we contain it to
49+
/// only PowerShell 5.1 at compile-time, and we have to set this up in this class, not <see
50+
/// cref="SynchronousPowerShellTask" /> because that's templated, making statics practically
51+
/// useless. <see cref = "SynchronousPowerShellTask.ExecuteNormally" /> method calls <see
52+
/// cref="DisableTranscribeOnly()" /> when necessary.
53+
/// See: https://github.com/PowerShell/PowerShell/pull/3436
54+
/// </summary>
55+
[ThreadStatic] // Because we can re-use it, but only for each PowerShell.
56+
private static PSHostUserInterface s_internalPSHostUserInterface;
57+
58+
private static readonly Func<PSHostUserInterface, bool> s_getTranscribeOnlyDelegate;
59+
60+
private static readonly Action<PSHostUserInterface, bool> s_setTranscribeOnlyDelegate;
61+
62+
private static readonly PropertyInfo s_executionContextProperty;
63+
64+
private static readonly PropertyInfo s_internalHostProperty;
65+
66+
private static readonly PropertyInfo s_internalHostUIProperty;
67+
#endif
68+
4269
private readonly ILoggerFactory _loggerFactory;
4370

4471
private readonly ILogger _logger;
@@ -104,6 +131,31 @@ static PsesInternalHost()
104131
s_scriptDebuggerTriggerObjectProperty = scriptDebuggerType.GetProperty(
105132
"TriggerObject",
106133
BindingFlags.Instance | BindingFlags.NonPublic);
134+
135+
#if !CoreCLR
136+
PropertyInfo transcribeOnlyProperty = typeof(PSHostUserInterface)
137+
.GetProperty("TranscribeOnly", BindingFlags.NonPublic | BindingFlags.Instance);
138+
139+
MethodInfo transcribeOnlyGetMethod = transcribeOnlyProperty.GetGetMethod(nonPublic: true);
140+
141+
s_getTranscribeOnlyDelegate = (Func<PSHostUserInterface, bool>)Delegate.CreateDelegate(
142+
typeof(Func<PSHostUserInterface, bool>), transcribeOnlyGetMethod);
143+
144+
MethodInfo transcribeOnlySetMethod = transcribeOnlyProperty.GetSetMethod(nonPublic: true);
145+
146+
s_setTranscribeOnlyDelegate = (Action<PSHostUserInterface, bool>)Delegate.CreateDelegate(
147+
typeof(Action<PSHostUserInterface, bool>), transcribeOnlySetMethod);
148+
149+
s_executionContextProperty = typeof(System.Management.Automation.Runspaces.Runspace)
150+
.GetProperty("ExecutionContext", BindingFlags.NonPublic | BindingFlags.Instance);
151+
152+
s_internalHostProperty = s_executionContextProperty.PropertyType
153+
.GetProperty("InternalHost", BindingFlags.NonPublic | BindingFlags.Instance);
154+
155+
// It's public but we want the override and reflection confuses me.
156+
s_internalHostUIProperty = s_internalHostProperty.PropertyType
157+
.GetProperty("UI", BindingFlags.Public | BindingFlags.Instance);
158+
#endif
107159
}
108160

109161
public PsesInternalHost(
@@ -476,19 +528,7 @@ public void InvokeDelegate(string representation, ExecutionOptions executionOpti
476528
public IReadOnlyList<TResult> InvokePSCommand<TResult>(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken)
477529
{
478530
SynchronousPowerShellTask<TResult> task = new(_logger, this, psCommand, executionOptions, cancellationToken);
479-
try
480-
{
481-
return task.ExecuteAndGetResult(cancellationToken);
482-
}
483-
finally
484-
{
485-
// At the end of each PowerShell command we need to reset PowerShell 5.1's
486-
// `TranscribeOnly` property to avoid a bug where output disappears.
487-
if (UI is EditorServicesConsolePSHostUserInterface ui)
488-
{
489-
ui.DisableTranscribeOnly();
490-
}
491-
}
531+
return task.ExecuteAndGetResult(cancellationToken);
492532
}
493533

494534
public void InvokePSCommand(PSCommand psCommand, PowerShellExecutionOptions executionOptions, CancellationToken cancellationToken) => InvokePSCommand<PSObject>(psCommand, executionOptions, cancellationToken);
@@ -507,6 +547,29 @@ public void InvokePSDelegate(string representation, ExecutionOptions executionOp
507547

508548
internal void AddToHistory(string historyEntry) => _readLineProvider.ReadLine.AddToHistory(historyEntry);
509549

550+
// This works around a bug in PowerShell 5.1 (that was later fixed) where a running
551+
// transcription could cause output to disappear since the `TranscribeOnly` property was
552+
// accidentally not reset to false.
553+
#pragma warning disable CA1822 // Warning to make it static when it's empty for CoreCLR.
554+
internal void DisableTranscribeOnly()
555+
#pragma warning restore CA1822
556+
{
557+
#if !CoreCLR
558+
// To fix the TranscribeOnly bug, we have to get the internal UI, which involves a lot
559+
// of reflection since we can't always just use PowerShell to execute `$Host.UI`.
560+
s_internalPSHostUserInterface ??=
561+
s_internalHostUIProperty.GetValue(
562+
s_internalHostProperty.GetValue(
563+
s_executionContextProperty.GetValue(CurrentPowerShell.Runspace)))
564+
as PSHostUserInterface;
565+
566+
if (s_getTranscribeOnlyDelegate(s_internalPSHostUserInterface))
567+
{
568+
s_setTranscribeOnlyDelegate(s_internalPSHostUserInterface, false);
569+
}
570+
#endif
571+
}
572+
510573
internal Task LoadHostProfilesAsync(CancellationToken cancellationToken)
511574
{
512575
// NOTE: This is a special task run on startup!

‎test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@
66
<IsPackable>false</IsPackable>
77
</PropertyGroup>
88

9+
<PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
10+
<DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
11+
</PropertyGroup>
12+
913
<ItemGroup>
1014
<PackageReference Include="Microsoft.Extensions.Logging" Version="7.0.0" />
1115
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.5.0" />

‎test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@
77
<TargetPlatform>x64</TargetPlatform>
88
</PropertyGroup>
99

10+
<PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
11+
<DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
12+
</PropertyGroup>
13+
1014
<ItemGroup>
1115
<ProjectReference Include="..\..\src\PowerShellEditorServices\PowerShellEditorServices.csproj" />
1216
<ProjectReference Include="..\PowerShellEditorServices.Test.Shared\PowerShellEditorServices.Test.Shared.csproj" />
@@ -27,10 +31,6 @@
2731
<PackageReference Include="Microsoft.PowerShell.5.ReferenceAssemblies" Version="1.1.0" />
2832
</ItemGroup>
2933

30-
<PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
31-
<DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
32-
</PropertyGroup>
33-
3434
<ItemGroup>
3535
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.5.0" />
3636
<PackageReference Include="xunit" Version="2.4.2" />

0 commit comments

Comments
 (0)