PR feedback: Kill IBuildEngineInternal, use class with virtual method…

…s instead
dotnet · May 21, 2021 · b30ab8e · b30ab8e
1 parent 6bd5320
commit b30ab8e
Show file tree

Hide file tree

Showing 11 changed files with 58 additions and 56 deletions.
diff --git a/ref/Microsoft.Build.Framework/net/Microsoft.Build.Framework.cs b/ref/Microsoft.Build.Framework/net/Microsoft.Build.Framework.cs
@@ -3,6 +3,11 @@
 namespace Microsoft.Build.Framework
 {
     public delegate void AnyEventHandler(object sender, Microsoft.Build.Framework.BuildEventArgs e);
+    public abstract partial class BuildEngineInterface
+    {
+        protected BuildEngineInterface() { }
+        public virtual Microsoft.Build.Framework.MessageImportance MinimumRequiredMessageImportance { get { throw null; } }
+    }
     [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
     public partial struct BuildEngineResult
     {
@@ -185,6 +190,10 @@ public partial interface IBuildEngine
         void LogMessageEvent(Microsoft.Build.Framework.BuildMessageEventArgs e);
         void LogWarningEvent(Microsoft.Build.Framework.BuildWarningEventArgs e);
     }
+    public partial interface IBuildEngine10 : Microsoft.Build.Framework.IBuildEngine, Microsoft.Build.Framework.IBuildEngine2, Microsoft.Build.Framework.IBuildEngine3, Microsoft.Build.Framework.IBuildEngine4, Microsoft.Build.Framework.IBuildEngine5, Microsoft.Build.Framework.IBuildEngine6, Microsoft.Build.Framework.IBuildEngine7, Microsoft.Build.Framework.IBuildEngine8, Microsoft.Build.Framework.IBuildEngine9
+    {
+        Microsoft.Build.Framework.BuildEngineInterface EngineInterface { get; }
+    }
     public partial interface IBuildEngine2 : Microsoft.Build.Framework.IBuildEngine
     {
         bool IsRunningMultipleNodes { get; }

diff --git a/ref/Microsoft.Build.Framework/netstandard/Microsoft.Build.Framework.cs b/ref/Microsoft.Build.Framework/netstandard/Microsoft.Build.Framework.cs
@@ -3,6 +3,11 @@
 namespace Microsoft.Build.Framework
 {
     public delegate void AnyEventHandler(object sender, Microsoft.Build.Framework.BuildEventArgs e);
+    public abstract partial class BuildEngineInterface
+    {
+        protected BuildEngineInterface() { }
+        public virtual Microsoft.Build.Framework.MessageImportance MinimumRequiredMessageImportance { get { throw null; } }
+    }
     [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
     public partial struct BuildEngineResult
     {
@@ -185,6 +190,10 @@ public partial interface IBuildEngine
         void LogMessageEvent(Microsoft.Build.Framework.BuildMessageEventArgs e);
         void LogWarningEvent(Microsoft.Build.Framework.BuildWarningEventArgs e);
     }
+    public partial interface IBuildEngine10 : Microsoft.Build.Framework.IBuildEngine, Microsoft.Build.Framework.IBuildEngine2, Microsoft.Build.Framework.IBuildEngine3, Microsoft.Build.Framework.IBuildEngine4, Microsoft.Build.Framework.IBuildEngine5, Microsoft.Build.Framework.IBuildEngine6, Microsoft.Build.Framework.IBuildEngine7, Microsoft.Build.Framework.IBuildEngine8, Microsoft.Build.Framework.IBuildEngine9
+    {
+        Microsoft.Build.Framework.BuildEngineInterface EngineInterface { get; }
+    }
     public partial interface IBuildEngine2 : Microsoft.Build.Framework.IBuildEngine
     {
         bool IsRunningMultipleNodes { get; }

diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs
@@ -34,8 +34,7 @@ internal class TaskHost :
 #if FEATURE_APPDOMAIN
         MarshalByRefObject,
 #endif
-        IBuildEngineInternal,
-        IBuildEngine9
+        IBuildEngine10
     {
         /// <summary>
         /// True if the "secret" environment variable MSBUILDNOINPROCNODE is set.
@@ -126,6 +125,7 @@ public TaskHost(IBuildComponentHost host, BuildRequestEntry requestEntry, Elemen
             _continueOnError = false;
             _activeProxy = true;
             _callbackMonitor = new Object();
+            EngineInterface = new BuildEngineInterfaceImpl(this);
         }
 
         /// <summary>
@@ -868,11 +868,25 @@ internal void ReleaseAllCores()
             }
         }
 
-        /// <summary>
-        /// Returns the minimum message importance not guaranteed to be ignored by registered loggers.
-        /// </summary>
-        MessageImportance IBuildEngineInternal.MinimumRequiredMessageImportance =>
-            _taskLoggingContext?.LoggingService.MinimumRequiredMessageImportance ?? MessageImportance.Low;
+        #endregion
+
+        #region IBuildEngine10 Members
+
+        private class BuildEngineInterfaceImpl : BuildEngineInterface
+        {
+            private TaskHost _taskHost;
+
+            internal BuildEngineInterfaceImpl(TaskHost taskHost)
+            {
+                _taskHost = taskHost;
+            }
+
+            /// <inheritdoc/>
+            public override MessageImportance MinimumRequiredMessageImportance =>
+                _taskHost._taskLoggingContext?.LoggingService.MinimumRequiredMessageImportance ?? MessageImportance.Low;
+        }
+
+        public BuildEngineInterface EngineInterface { get; }
 
         #endregion
 

diff --git a/src/Build/Instance/TaskFactoryLoggingHost.cs b/src/Build/Instance/TaskFactoryLoggingHost.cs
@@ -22,8 +22,7 @@ internal class TaskFactoryLoggingHost :
 #if FEATURE_APPDOMAIN
         MarshalByRefObject,
 #endif
-        IBuildEngine,
-        IBuildEngineInternal
+        IBuildEngine
     {
         /// <summary>
         /// Location of the task node in the original file
@@ -246,12 +245,6 @@ public bool BuildProjectFile(string projectFileName, string[] targetNames, Syste
 
         #endregion
 
-        #region IBuildEngineInternal Members
-
-        MessageImportance IBuildEngineInternal.MinimumRequiredMessageImportance => MessageImportance.Low;
-
-        #endregion
-
 #if FEATURE_APPDOMAIN
         /// <summary>
         /// InitializeLifetimeService is called when the remote object is activated.

diff --git a/src/Framework/IBuildEngineInternal.cs b/src/Framework/IBuildEngineInternal.cs
diff --git a/src/MSBuild/OutOfProcTaskHostNode.cs b/src/MSBuild/OutOfProcTaskHostNode.cs
@@ -29,11 +29,11 @@ internal class OutOfProcTaskHostNode :
 #if FEATURE_APPDOMAIN
         MarshalByRefObject,
 #endif
-        INodePacketFactory, INodePacketHandler, IBuildEngineInternal,
+        INodePacketFactory, INodePacketHandler,
 #if CLR2COMPATIBILITY
         IBuildEngine3
 #else
-        IBuildEngine9
+        IBuildEngine10
 #endif
     {
         /// <summary>
@@ -492,17 +492,23 @@ public void ReleaseCores(int coresToRelease)
         }
 
         #endregion
-#endif
 
-        #region IBuildEngineInternal Members
+        #region IBuildEngine10 Members
 
-        /// <summary>
-        /// No logging verbosity optimization in OOP nodes.
-        /// </summary>
-        MessageImportance IBuildEngineInternal.MinimumRequiredMessageImportance => MessageImportance.Low;
+        private class BuildEngineInterfaceImpl : BuildEngineInterface
+        {
+            /// <summary>
+            /// No logging verbosity optimization in OOP nodes.
+            /// </summary>
+            public override MessageImportance MinimumRequiredMessageImportance => MessageImportance.Low;
+        }
+
+        public BuildEngineInterface EngineInterface { get; } = new BuildEngineInterfaceImpl();
 
         #endregion
 
+#endif
+
         #region INodePacketFactory Members
 
         /// <summary>

diff --git a/src/MSBuildTaskHost/MSBuildTaskHost.csproj b/src/MSBuildTaskHost/MSBuildTaskHost.csproj
@@ -48,9 +48,6 @@
     <Compile Include="..\Framework\IBuildEngine3.cs">
       <Link>IBuildEngine3.cs</Link>
     </Compile>
-    <Compile Include="..\Framework\IBuildEngineInternal.cs">
-      <Link>IBuildEngineInternal.cs</Link>
-    </Compile>
     <Compile Include="..\Framework\RunInSTAAttribute.cs">
       <Link>RunInSTAAtribute.cs</Link>
     </Compile>

diff --git a/src/Shared/TaskLoggingHelper.cs b/src/Shared/TaskLoggingHelper.cs
@@ -246,7 +246,7 @@ public virtual string GetResourceMessage(string resourceName)
         /// <returns>True if messages of the given importance should be logged, false if it's guaranteed that such messages would be ignored.</returns>
         public bool ShouldLogMessage(MessageImportance importance)
         {
-            return importance <= ((IBuildEngineInternal)BuildEngine).MinimumRequiredMessageImportance;
+            return importance <= ((IBuildEngine10)BuildEngine).EngineInterface.MinimumRequiredMessageImportance;
         }
 
         /// <summary>

diff --git a/src/Shared/UnitTests/MockEngine.cs b/src/Shared/UnitTests/MockEngine.cs
@@ -30,7 +30,7 @@ namespace Microsoft.Build.UnitTests
      * is somewhat of a no-no for task assemblies.
      * 
      **************************************************************************/
-    internal sealed class MockEngine : IBuildEngine7, IBuildEngineInternal
+    internal sealed class MockEngine : IBuildEngine7
     {
         private readonly object _lockObj = new object();  // Protects _log, _output
         private readonly ITestOutputHelper _output;
@@ -488,7 +488,5 @@ public object UnregisterTaskObject(object key, RegisteredTaskObjectLifetime life
             _objectCache.TryRemove(key, out object obj);
             return obj;
         }
-
-        MessageImportance IBuildEngineInternal.MinimumRequiredMessageImportance => MessageImportance.Low;
     }
 }
diff --git a/src/Utilities.UnitTests/MockEngine.cs b/src/Utilities.UnitTests/MockEngine.cs
@@ -25,7 +25,7 @@ namespace Microsoft.Build.UnitTests
      * is somewhat of a no-no for task assemblies.
      * 
      **************************************************************************/
-    internal sealed class MockEngine : IBuildEngine3, IBuildEngineInternal
+    internal sealed class MockEngine : IBuildEngine3
     {
         private StringBuilder _log = new StringBuilder();
 
@@ -121,8 +121,6 @@ public void Reacquire()
         {
         }
 
-        MessageImportance IBuildEngineInternal.MinimumRequiredMessageImportance => MessageImportance.Low;
-
         /// <summary>
         /// Assert that the log doesn't contain the given string.
         /// </summary>

diff --git a/src/Utilities.UnitTests/TrackedDependencies/MockEngine.cs b/src/Utilities.UnitTests/TrackedDependencies/MockEngine.cs
@@ -22,7 +22,7 @@ namespace Microsoft.Build.UnitTests.TrackedDependencies
      * is somewhat of a no-no for task assemblies.
      * 
      **************************************************************************/
-    internal sealed class MockEngine : IBuildEngine2, IBuildEngineInternal
+    internal sealed class MockEngine : IBuildEngine2
     {
         private string _upperLog;
 
@@ -107,8 +107,6 @@ public void LogMessageEvent(BuildMessageEventArgs eventArgs)
             bool useResultsCache,
             bool unloadProjectsOnCompletion) => false;
 
-        MessageImportance IBuildEngineInternal.MinimumRequiredMessageImportance => MessageImportance.Low;
-
         /// <summary>
         /// Assert that the log file contains the given string.
         /// Case insensitive.