Revert "Reverted and rebuilt for localizable strings." (dotnet#5246)

This adds back support for logging an error when a task returns false
without logging an error. This was originally added in dotnet#4940 but was
reverted because of multiple difficulties.

src/Build.UnitTests/BackEnd/BuildResult_Tests.cs

@@ -10,8 +10,8 @@
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
 using Microsoft.Build.Unittest;
+using Shouldly;
 using Xunit;
-
 using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem;
 
 namespace Microsoft.Build.UnitTests.BackEnd

src/Build.UnitTests/BackEnd/LoggingContext_Tests.cs

@@ -4,8 +4,10 @@
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.BackEnd.Logging;
 using Microsoft.Build.Shared;
+using Shouldly;
 using System;
 using Xunit;
+using Xunit.Abstractions;
 
 namespace Microsoft.Build.UnitTests.BackEnd
 {
@@ -14,29 +16,36 @@ namespace Microsoft.Build.UnitTests.BackEnd
     /// </summary>
     public class LoggingContext_Tests
     {
+        private readonly ITestOutputHelper _output;
+
+        public LoggingContext_Tests(ITestOutputHelper outputHelper)
+        {
+            _output = outputHelper;
+        }
+
         /// <summary>
         /// A few simple tests for NodeLoggingContexts. 
         /// </summary>
         [Fact]
         public void CreateValidNodeLoggingContexts()
         {
-            NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(), 1, true);
-            Assert.True(context.IsInProcNode);
-            Assert.True(context.IsValid);
+            NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(_output.WriteLine), 1, true);
+            context.IsInProcNode.ShouldBeTrue();
+            context.IsValid.ShouldBeTrue();
 
             context.LogBuildFinished(true);
-            Assert.False(context.IsValid);
+            context.IsValid.ShouldBeFalse();
 
-            Assert.Equal(1, context.BuildEventContext.NodeId);
+            context.BuildEventContext.NodeId.ShouldBe(1);
 
-            NodeLoggingContext context2 = new NodeLoggingContext(new MockLoggingService(), 2, false);
-            Assert.False(context2.IsInProcNode);
-            Assert.True(context2.IsValid);
+            NodeLoggingContext context2 = new NodeLoggingContext(new MockLoggingService(_output.WriteLine), 2, false);
+            context2.IsInProcNode.ShouldBeFalse();
+            context2.IsValid.ShouldBeTrue();
 
             context2.LogBuildFinished(true);
-            Assert.False(context2.IsValid);
+            context2.IsValid.ShouldBeFalse();
 
-            Assert.Equal(2, context2.BuildEventContext.NodeId);
+            context2.BuildEventContext.NodeId.ShouldBe(2);
         }
 
         /// <summary>
@@ -49,9 +58,25 @@ public void InvalidNodeIdOnNodeLoggingContext()
         {
             Assert.Throws<InternalErrorException>(() =>
             {
-                NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(), -2, true);
+                _ = new NodeLoggingContext(new MockLoggingService(), -2, true);
             }
            );
         }
+
+        [Fact]
+        public void HasLoggedErrors()
+        {
+            NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(_output.WriteLine), 1, true);
+            context.HasLoggedErrors.ShouldBeFalse();
+
+            context.LogCommentFromText(Framework.MessageImportance.High, "Test message");
+            context.HasLoggedErrors.ShouldBeFalse();
+
+            context.LogWarningFromText(null, null, null, null, "Test warning");
+            context.HasLoggedErrors.ShouldBeFalse();
+
+            context.LogErrorFromText(null, null, null, null, "Test error");
+            context.HasLoggedErrors.ShouldBeTrue();
+        }
     }
-}
+}

src/Build.UnitTests/BackEnd/ReturnFailureWithoutLoggingErrorTask.cs

@@ -0,0 +1,26 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using Microsoft.Build.BackEnd.Logging;
+using Microsoft.Build.Utilities;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+namespace Microsoft.Build.UnitTests
+{
+    /// <summary>
+    /// This task was created for https://github.com/microsoft/msbuild/issues/2036
+    /// </summary>
+    public class ReturnFailureWithoutLoggingErrorTask : Task
+    {
+        /// <summary>
+        /// Intentionally return false without logging an error to test proper error catching.
+        /// </summary>
+        /// <returns></returns>
+        public override bool Execute()
+        {
+            return false;
+        }
+    }
+}

src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs

@@ -4,7 +4,9 @@
 using System.Linq;
 using Microsoft.Build.Framework;
 using Microsoft.Build.UnitTests;
+using Shouldly;
 using Xunit;
+using Xunit.Abstractions;
 
 namespace Microsoft.Build.Engine.UnitTests
 {
@@ -13,6 +15,13 @@ public sealed class WarningsAsMessagesAndErrorsTests
         private const string ExpectedEventMessage = "03767942CDB147B98D0ECDBDE1436DA3";
         private const string ExpectedEventCode = "0BF68998";
 
+        ITestOutputHelper _output;
+
+        public WarningsAsMessagesAndErrorsTests(ITestOutputHelper output)
+        {
+            _output = output;
+        }
+
         [Fact]
         public void TreatAllWarningsAsErrors()
         {
@@ -29,7 +38,7 @@ public void TreatAllWarningsAsErrors()
         [Fact]
         public void TreatWarningsAsErrorsWhenBuildingSameProjectMultipleTimes()
         {
-            using (TestEnvironment testEnvironment = TestEnvironment.Create())
+            using (TestEnvironment testEnvironment = TestEnvironment.Create(_output))
             {
                 TransientTestProjectWithFiles project2 = testEnvironment.CreateTestProjectWithFiles($@"
                 <Project xmlns=""msbuildnamespace"">
@@ -123,7 +132,7 @@ public void TreatWarningsAsMessagesWhenSpecified()
         [Fact]
         public void TreatWarningsAsMessagesWhenBuildingSameProjectMultipleTimes()
         {
-            using (TestEnvironment testEnvironment = TestEnvironment.Create())
+            using (TestEnvironment testEnvironment = TestEnvironment.Create(_output))
             {
                 TransientTestProjectWithFiles project2 = testEnvironment.CreateTestProjectWithFiles($@"
                 <Project xmlns=""msbuildnamespace"">
@@ -263,5 +272,111 @@ private string GetTestProject(bool? treatAllWarningsAsErrors = null, string warn
                 </Target>
             </Project>";
         }
+
+        [Fact]
+        public void TaskReturnsFailureButDoesNotLogError_ShouldCauseBuildFailure()
+        {
+
+            using (TestEnvironment env = TestEnvironment.Create(_output))
+            {
+                TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
+                <Project>
+                    <UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
+                    <Target Name='Build'>
+                        <ReturnFailureWithoutLoggingErrorTask/>
+                    </Target>
+                </Project>");
+
+                MockLogger logger = proj.BuildProjectExpectFailure();
+
+                logger.AssertLogContains("MSB4132");
+            }
+        }
+
+        [Fact]
+        public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_WarnAndContinue()
+        {
+
+            using (TestEnvironment env = TestEnvironment.Create(_output))
+            {
+                TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
+                <Project>
+                    <UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
+                    <Target Name='Build'>
+                        <ReturnFailureWithoutLoggingErrorTask
+                            ContinueOnError=""WarnAndContinue""/>
+                    </Target>
+                </Project>");
+
+                MockLogger logger = proj.BuildProjectExpectSuccess();
+
+                logger.WarningCount.ShouldBe(1);
+
+                logger.AssertLogContains("MSB4132");
+            }
+        }
+
+        [Fact]
+        public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True()
+        {
+
+            using (TestEnvironment env = TestEnvironment.Create(_output))
+            {
+                TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
+                <Project>
+                    <UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
+                    <Target Name='Build'>
+                        <ReturnFailureWithoutLoggingErrorTask
+                            ContinueOnError=""true""/>
+                    </Target>
+                </Project>");
+
+                MockLogger logger = proj.BuildProjectExpectSuccess();
+
+                logger.AssertLogContains("MSB4132");
+            }
+        }
+
+        [Fact]
+        public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_ErrorAndStop()
+        {
+
+            using (TestEnvironment env = TestEnvironment.Create(_output))
+            {
+                TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
+                <Project>
+                    <UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
+                    <Target Name='Build'>
+                        <ReturnFailureWithoutLoggingErrorTask
+                            ContinueOnError=""ErrorAndStop""/>
+                    </Target>
+                </Project>");
+
+                MockLogger logger = proj.BuildProjectExpectFailure();
+
+                logger.AssertLogContains("MSB4132");
+            }
+        }
+
+        [Fact]
+        public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_False()
+        {
+
+            using (TestEnvironment env = TestEnvironment.Create(_output))
+            {
+                TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
+                <Project>
+                    <UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
+                    <Target Name='Build'>
+                        <ReturnFailureWithoutLoggingErrorTask
+                            ContinueOnError=""false""/>
+                    </Target>
+                </Project>");
+
+                MockLogger logger = proj.BuildProjectExpectFailure();
+
+                logger.AssertLogContains("MSB4132");
+            }
+        }
     }
 }

src/Build/BackEnd/Components/Logging/BuildLoggingContext.cs

@@ -65,6 +65,7 @@ internal void LogFatalTaskError(Exception exception, BuildEventFileInfo file, st
         {
             ErrorUtilities.VerifyThrow(IsValid, "must be valid");
             LoggingService.LogFatalTaskError(BuildEventContext, exception, file, taskName);
+            _hasLoggedErrors = true;
         }
     }
 }

src/Build/BackEnd/Components/Logging/LoggingContext.cs

@@ -28,6 +28,8 @@ internal class LoggingContext
         /// </summary>
         private bool _isValid;
 
+        protected bool _hasLoggedErrors;
+
         /// <summary>
         /// Constructs the logging context from a logging service and an event context.
         /// </summary>
@@ -41,6 +43,7 @@ public LoggingContext(ILoggingService loggingService, BuildEventContext eventCon
             _loggingService = loggingService;
             _eventContext = eventContext;
             _isValid = false;
+            _hasLoggedErrors = false;
         }
 
         /// <summary>
@@ -106,6 +109,8 @@ protected set
             }
         }
 
+        internal bool HasLoggedErrors { get { return _hasLoggedErrors; } set { _hasLoggedErrors = value; } }
+
         /// <summary>
         ///  Helper method to create a message build event from a string resource and some parameters
         /// </summary>
@@ -139,6 +144,7 @@ internal void LogError(BuildEventFileInfo file, string messageResourceName, para
         {
             ErrorUtilities.VerifyThrow(_isValid, "must be valid");
             _loggingService.LogError(_eventContext, file, messageResourceName, messageArgs);
+            _hasLoggedErrors = true;
         }
 
         /// <summary>
@@ -152,6 +158,7 @@ internal void LogErrorWithSubcategory(string subcategoryResourceName, BuildEvent
         {
             ErrorUtilities.VerifyThrow(_isValid, "must be valid");
             _loggingService.LogError(_eventContext, subcategoryResourceName, file, messageResourceName, messageArgs);
+            _hasLoggedErrors = true;
         }
 
         /// <summary>
@@ -166,6 +173,7 @@ internal void LogErrorFromText(string subcategoryResourceName, string errorCode,
         {
             ErrorUtilities.VerifyThrow(_isValid, "must be valid");
             _loggingService.LogErrorFromText(_eventContext, subcategoryResourceName, errorCode, helpKeyword, file, message);
+            _hasLoggedErrors = true;
         }
 
         /// <summary>
@@ -176,6 +184,7 @@ internal void LogInvalidProjectFileError(InvalidProjectFileException invalidProj
         {
             ErrorUtilities.VerifyThrow(_isValid, "must be valid");
             _loggingService.LogInvalidProjectFileError(_eventContext, invalidProjectFileException);
+            _hasLoggedErrors = true;
         }
 
         /// <summary>
@@ -189,6 +198,7 @@ internal void LogFatalError(Exception exception, BuildEventFileInfo file, string
         {
             ErrorUtilities.VerifyThrow(_isValid, "must be valid");
             _loggingService.LogFatalError(_eventContext, exception, file, messageResourceName, messageArgs);
+            _hasLoggedErrors = true;
         }
 
         /// <summary>
@@ -237,6 +247,7 @@ internal void LogFatalBuildError(Exception exception, BuildEventFileInfo file)
         {
             ErrorUtilities.VerifyThrow(IsValid, "must be valid");
             LoggingService.LogFatalBuildError(BuildEventContext, exception, file);
+            _hasLoggedErrors = true;
         }
     }
 }

src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs

@@ -768,6 +768,7 @@ private async Task<WorkUnitResult> ExecuteInstantiatedTask(ITaskExecutionHost ta
                     if (taskType == typeof(MSBuild))
                     {
                         MSBuild msbuildTask = host.TaskInstance as MSBuild;
+
                         ErrorUtilities.VerifyThrow(msbuildTask != null, "Unexpected MSBuild internal task.");
 
                         var undeclaredProjects = GetUndeclaredProjects(msbuildTask);
@@ -943,6 +944,29 @@ private async Task<WorkUnitResult> ExecuteInstantiatedTask(ITaskExecutionHost ta
                     }
                 }
 
+                // When a task fails it must log an error. If a task fails to do so,
+                // that is logged as an error. MSBuild tasks are an exception because
+                // errors are not logged directly from them, but the tasks spawned by them.
+                IBuildEngine be = host.TaskInstance.BuildEngine;
+                if (taskReturned && !taskResult && !taskLoggingContext.HasLoggedErrors && (be is TaskHost th ? th.BuildRequestsSucceeded : false))
+                {
+                    if (_continueOnError == ContinueOnError.WarnAndContinue)
+                    {
+                        taskLoggingContext.LogWarning(null,
+                            new BuildEventFileInfo(_targetChildInstance.Location),
+                            "TaskReturnedFalseButDidNotLogError",
+                            _taskNode.Name);
+
+                        taskLoggingContext.LogComment(MessageImportance.Normal, "ErrorConvertedIntoWarning");
+                    }
+                    else
+                    {
+                        taskLoggingContext.LogError(new BuildEventFileInfo(_targetChildInstance.Location),
+                            "TaskReturnedFalseButDidNotLogError",
+                            _taskNode.Name);
+                    }
+                }
+
                 // If the task returned attempt to gather its outputs.  If gathering outputs fails set the taskResults
                 // to false
                 if (taskReturned)