Fix the TranscribeOnly bug (take three)

andyleejordan · andyleejordan · commit 9bea6a987a00 · 2023-05-30T14:12:04.000-07:00
diff --git a/src/PowerShellEditorServices.Hosting/EditorServicesLoader.cs b/src/PowerShellEditorServices.Hosting/EditorServicesLoader.cs
@@ -32,6 +32,7 @@ namespace Microsoft.PowerShell.EditorServices.Hosting
     public sealed class EditorServicesLoader : IDisposable
     {
 #if !CoreCLR
+        // TODO: Well, we're saying we need 4.8 here but we're building for 4.6.2...
         // See https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed
         private const int Net48Version = 528040;
 
@@ -342,7 +343,7 @@ private void LogHostInformation()
             _logger.Log(PsesLogLevel.Verbose, $@"
 == Environment Details ==
  - OS description:  {RuntimeInformation.OSDescription}
- - OS architecture: {GetOSArchitecture()}
+ - OS architecture: {RuntimeInformation.OSArchitecture}
  - Process bitness: {(Environment.Is64BitProcess ? "64" : "32")}
 ");
         }
@@ -355,27 +356,6 @@ private static string GetPSOutputEncoding()
                 useLocalScope: true).Invoke<string>()[0];
         }
 
-        // TODO: Deduplicate this with VersionUtils.
-        private static string GetOSArchitecture()
-        {
-#if CoreCLR
-            if (Environment.OSVersion.Platform != PlatformID.Win32NT)
-            {
-                return RuntimeInformation.OSArchitecture.ToString();
-            }
-#endif
-
-            // If on win7 (version 6.1.x), avoid System.Runtime.InteropServices.RuntimeInformation
-            if (Environment.OSVersion.Version < new Version(6, 2))
-            {
-                return Environment.Is64BitProcess
-                    ? "X64"
-                    : "X86";
-            }
-
-            return RuntimeInformation.OSArchitecture.ToString();
-        }
-
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "CA2208:Instantiate argument exceptions correctly", Justification = "Checking user-defined configuration")]
         private void ValidateConfiguration()
         {
diff --git a/src/PowerShellEditorServices.VSCode/PowerShellEditorServices.VSCode.csproj b/src/PowerShellEditorServices.VSCode/PowerShellEditorServices.VSCode.csproj
@@ -9,10 +9,6 @@
     <Configurations>Debug;Release</Configurations>
   </PropertyGroup>
 
-  <PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
-    <DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
-  </PropertyGroup>
-
   <!-- Fail the release build if there are missing public API documentation comments -->
   <PropertyGroup>
     <WarningsAsErrors>1591,1573,1572</WarningsAsErrors>
diff --git a/src/PowerShellEditorServices/PowerShellEditorServices.csproj b/src/PowerShellEditorServices/PowerShellEditorServices.csproj
@@ -9,9 +9,12 @@
     <Configurations>Debug;Release</Configurations>
   </PropertyGroup>
 
-  <PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
-    <DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
-  </PropertyGroup>
+  <!--
+    Be careful about using CoreCLR as a definition, it doesn't work for most of
+    our code because the shared libraries target netstandard2.0 and so can't use
+    a property group condition to define it. It's only available to code under
+    src/PowerShellEditorServices.Hosting and the tests.
+  -->
 
   <ItemGroup>
     <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo">
diff --git a/src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs b/src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs
@@ -106,7 +106,8 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok
             {
                 _psCommand.AddOutputCommand();
 
-                // Fix the transcription bug!
+                // Fix the transcription bug! Here we're fixing immediately before the invocation of
+                // our command, that has had `Out-Default` added to it.
                 if (!_pwsh.Runspace.RunspaceIsRemote)
                 {
                     _psesHost.DisableTranscribeOnly();
@@ -282,6 +283,14 @@ private IReadOnlyList<TResult> ExecuteInDebugger(CancellationToken cancellationT
                 {
                     _pwsh.Streams.Error.Clear();
                 }
+
+                // Fix the transcription bug! Since we don't depend on `Out-Default` for
+                // `ExecuteDebugger`, we fix the bug here so the original invocation (before the
+                // script is executed) is good to go.
+                if (!_pwsh.Runspace.RunspaceIsRemote)
+                {
+                    _psesHost.DisableTranscribeOnly();
+                }
             }
 
             _psesHost.DebugContext.ProcessDebuggerResult(debuggerResult);
diff --git a/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs b/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs
@@ -39,7 +39,6 @@ internal class PsesInternalHost : PSHost, IHostSupportsInteractiveSession, IRuns
 
         private static readonly PropertyInfo s_scriptDebuggerTriggerObjectProperty;
 
-#if !CoreCLR
         /// <summary>
         /// 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 <see
@@ -62,7 +61,6 @@ internal class PsesInternalHost : PSHost, IHostSupportsInteractiveSession, IRuns
         private static readonly PropertyInfo s_executionContextProperty;
 
         private static readonly PropertyInfo s_internalHostProperty;
-#endif
 
         private readonly ILoggerFactory _loggerFactory;
 
@@ -130,7 +128,12 @@ static PsesInternalHost()
                 "TriggerObject",
                 BindingFlags.Instance | BindingFlags.NonPublic);
 
-#if !CoreCLR
+            if (VersionUtils.IsNetCore)
+            {
+                // The following reflection methods are only needed for the .NET Framework.
+                return;
+            }
+
             PropertyInfo transcribeOnlyProperty = typeof(PSHostUserInterface)
                 .GetProperty("TranscribeOnly", BindingFlags.NonPublic | BindingFlags.Instance);
 
@@ -144,12 +147,11 @@ static PsesInternalHost()
             s_setTranscribeOnlyDelegate = (Action<PSHostUserInterface, bool>)Delegate.CreateDelegate(
                 typeof(Action<PSHostUserInterface, bool>), transcribeOnlySetMethod);
 
-            s_executionContextProperty = typeof(System.Management.Automation.Runspaces.Runspace)
+            s_executionContextProperty = typeof(Runspace)
                 .GetProperty("ExecutionContext", BindingFlags.NonPublic | BindingFlags.Instance);
 
             s_internalHostProperty = s_executionContextProperty.PropertyType
                 .GetProperty("InternalHost", BindingFlags.NonPublic | BindingFlags.Instance);
-#endif
         }
 
         public PsesInternalHost(
@@ -544,23 +546,29 @@ public void InvokePSDelegate(string representation, ExecutionOptions executionOp
         // 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
+            if (VersionUtils.IsNetCore)
+            {
+                return;
+            }
+
             // 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_internalHostProperty.GetValue(
                     s_executionContextProperty.GetValue(CurrentPowerShell.Runspace))
-                    as PSHost).UI;
+                    as PSHost)?.UI;
+
+            if (s_internalPSHostUserInterface is null)
+            {
+                return;
+            }
 
             if (s_getTranscribeOnlyDelegate(s_internalPSHostUserInterface))
             {
                 s_setTranscribeOnlyDelegate(s_internalPSHostUserInterface, false);
             }
-#endif
         }
 
         internal Task LoadHostProfilesAsync(CancellationToken cancellationToken)
diff --git a/src/PowerShellEditorServices/Utility/VersionUtils.cs b/src/PowerShellEditorServices/Utility/VersionUtils.cs
@@ -42,11 +42,6 @@ internal static class VersionUtils
         /// </summary>
         public static bool IsPS5 { get; } = PSVersion.Major == 5;
 
-        /// <summary>
-        /// True if we are running in PowerShell Core 6, false otherwise.
-        /// </summary>
-        public static bool IsPS6 { get; } = PSVersion.Major == 6;
-
         /// <summary>
         /// True if we are running in PowerShell 7, false otherwise.
         /// </summary>
@@ -70,7 +65,7 @@ internal static class VersionUtils
         /// <summary>
         /// The .NET Architecture as a string.
         /// </summary>
-        public static string Architecture { get; } = PowerShellReflectionUtils.GetOSArchitecture();
+        public static string Architecture { get; } = RuntimeInformation.OSArchitecture.ToString();
     }
 
     internal static class PowerShellReflectionUtils
@@ -117,24 +112,5 @@ internal static class PowerShellReflectionUtils
         public static string PSVersionString { get; } = s_psCurrentVersionProperty != null
             ? s_psCurrentVersionProperty.GetValue(null).ToString()
             : PSVersion.ToString(3);
-
-        public static string GetOSArchitecture()
-        {
-#if CoreCLR
-            if (Environment.OSVersion.Platform != PlatformID.Win32NT)
-            {
-                return RuntimeInformation.OSArchitecture.ToString();
-            }
-#endif
-            // If on win7 (version 6.1.x), avoid System.Runtime.InteropServices.RuntimeInformation
-            if (Environment.OSVersion.Version < new Version(6, 2))
-            {
-                return Environment.Is64BitProcess
-                    ? "X64"
-                    : "X86";
-            }
-
-            return RuntimeInformation.OSArchitecture.ToString();
-        }
     }
 }
diff --git a/test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj b/test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj
@@ -6,10 +6,6 @@
     <IsPackable>false</IsPackable>
   </PropertyGroup>
 
-  <PropertyGroup Condition=" '$(TargetFramework)' != 'net462' ">
-    <DefineConstants>$(DefineConstants);CoreCLR</DefineConstants>
-  </PropertyGroup>
-
   <ItemGroup>
     <PackageReference Include="Microsoft.Extensions.Logging" Version="7.0.0" />
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.6.0" />

Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,8 @@ private IReadOnlyList<TResult> ExecuteNormally(CancellationToken cancellationTok`
`106`	`106`	`{`
`107`	`107`	`_psCommand.AddOutputCommand();`
`108`	`108`
`109`		`- // Fix the transcription bug!`
	`109`	`+ // Fix the transcription bug! Here we're fixing immediately before the invocation of`
	`110`	+ // our command, that has had `Out-Default` added to it.
`110`	`111`	`if (!_pwsh.Runspace.RunspaceIsRemote)`
`111`	`112`	`{`
`112`	`113`	`_psesHost.DisableTranscribeOnly();`
`@@ -282,6 +283,14 @@ private IReadOnlyList<TResult> ExecuteInDebugger(CancellationToken cancellationT`
`282`	`283`	`{`
`283`	`284`	`_pwsh.Streams.Error.Clear();`
`284`	`285`	`}`
	`286`	`+`
	`287`	+ // Fix the transcription bug! Since we don't depend on `Out-Default` for
	`288`	+ // `ExecuteDebugger`, we fix the bug here so the original invocation (before the
	`289`	`+ // script is executed) is good to go.`
	`290`	`+ if (!_pwsh.Runspace.RunspaceIsRemote)`
	`291`	`+ {`
	`292`	`+ _psesHost.DisableTranscribeOnly();`
	`293`	`+ }`
`285`	`294`	`}`
`286`	`295`
`287`	`296`	`_psesHost.DebugContext.ProcessDebuggerResult(debuggerResult);`