BuildCheck has better message when Check fails on Initialize (#10612)

Partial fix of #10522

Context
When a Check throws an exception it falls with an internal logger exception and crashes the build. This is not ideal. It decided that when a Check fails, we do not fail the build, just give a warning and deregister the check.

Changes Made
Changed the BuildCheck logger to catch initialization exceptions, and dispatch them as warnings and not errors.

Testing
Added end to end test with a sample custom check that throws an exception.

Notes
This is just a catch for exception on Check initialization. There are tests added for other cases but the fix will be different for them, so it will be done in another PR.

src/Build/BackEnd/Shared/EventsCreatorHelper.cs

@@ -60,4 +60,35 @@ public static BuildErrorEventArgs CreateErrorEventFromText(BuildEventContext bui
 
         return buildEvent;
     }
+
+    public static BuildWarningEventArgs CreateWarningEventFromText(BuildEventContext buildEventContext, string? subcategoryResourceName, string? errorCode, string? helpKeyword, BuildEventFileInfo file, string message)
+    {
+        ErrorUtilities.VerifyThrowInternalNull(buildEventContext, nameof(buildEventContext));
+        ErrorUtilities.VerifyThrowInternalNull(file, nameof(file));
+        ErrorUtilities.VerifyThrowInternalNull(message, nameof(message));
+
+        string? subcategory = null;
+
+        if (subcategoryResourceName != null)
+        {
+            subcategory = AssemblyResources.GetString(subcategoryResourceName);
+        }
+
+        BuildWarningEventArgs buildEvent =
+        new BuildWarningEventArgs(
+            subcategory,
+            errorCode,
+            file!.File,
+            file.Line,
+            file.Column,
+            file.EndLine,
+            file.EndColumn,
+            message,
+            helpKeyword,
+            "MSBuild");
+
+        buildEvent.BuildEventContext = buildEventContext;
+
+        return buildEvent;
+    }
 }

src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs

@@ -188,6 +188,7 @@ internal void RegisterCustomCheck(
         {
             if (_enabledDataSources[(int)buildCheckDataSource])
             {
+                List<CheckFactoryContext> invalidChecksToRemove = new();
                 foreach (var factory in factories)
                 {
                     var instance = factory();
@@ -201,10 +202,24 @@ internal void RegisterCustomCheck(
                         if (checkFactoryContext != null)
                         {
                             _checkRegistry.Add(checkFactoryContext);
-                            SetupSingleCheck(checkFactoryContext, projectPath);
-                            checkContext.DispatchAsComment(MessageImportance.Normal, "CustomCheckSuccessfulAcquisition", instance.FriendlyName);
+                            try
+                            {
+                                SetupSingleCheck(checkFactoryContext, projectPath);
+                                checkContext.DispatchAsComment(MessageImportance.Normal, "CustomCheckSuccessfulAcquisition", instance.FriendlyName);
+                            }
+                            catch (BuildCheckConfigurationException e)
+                            {
+                                checkContext.DispatchAsWarningFromText(
+                                    null,
+                                    null,
+                                    null,
+                                    new BuildEventFileInfo(projectPath),
+                                    e.Message);
+                                invalidChecksToRemove.Add(checkFactoryContext);
+                            }
                         }
                     }
+                    RemoveChecks(invalidChecksToRemove, checkContext);
                 }
             }
         }
@@ -286,7 +301,7 @@ private void SetupChecksForNewProject(string projectFullPath, ICheckContext chec
 
             // If it's already constructed - just control the custom settings do not differ
             Stopwatch stopwatch = Stopwatch.StartNew();
-            List<CheckFactoryContext> checksToRemove = new();
+            List<CheckFactoryContext> invalidChecksToRemove = new();
             foreach (CheckFactoryContext checkFactoryContext in _checkRegistry)
             {
                 try
@@ -295,16 +310,24 @@ private void SetupChecksForNewProject(string projectFullPath, ICheckContext chec
                 }
                 catch (BuildCheckConfigurationException e)
                 {
-                    checkContext.DispatchAsErrorFromText(
+                    checkContext.DispatchAsWarningFromText(
                         null,
                         null,
                         null,
                         new BuildEventFileInfo(projectFullPath),
                         e.Message);
-                    checksToRemove.Add(checkFactoryContext);
+                    invalidChecksToRemove.Add(checkFactoryContext);
                 }
             }
 
+            RemoveChecks(invalidChecksToRemove, checkContext);
+
+            stopwatch.Stop();
+            _tracingReporter.AddNewProjectStats(stopwatch.Elapsed);
+        }
+
+        private void RemoveChecks(List<CheckFactoryContext> checksToRemove, ICheckContext checkContext)
+        {
             checksToRemove.ForEach(c =>
             {
                 _checkRegistry.Remove(c);
@@ -316,9 +339,6 @@ private void SetupChecksForNewProject(string projectFullPath, ICheckContext chec
                 _tracingReporter.AddCheckStats(checkToRemove!.Check.FriendlyName, checkToRemove.Elapsed);
                 checkToRemove.Check.Dispose();
             }
-
-            stopwatch.Stop();
-            _tracingReporter.AddNewProjectStats(stopwatch.Elapsed);
         }
 
         public void ProcessEvaluationFinishedEventArgs(

src/Build/BuildCheck/Infrastructure/CheckContext/CheckDispatchingContext.cs

@@ -62,4 +62,11 @@ public void DispatchAsErrorFromText(string? subcategoryResourceName, string? err
 
         _eventDispatcher.Dispatch(buildEvent);
     }
+
+    public void DispatchAsWarningFromText(string? subcategoryResourceName, string? errorCode, string? helpKeyword, BuildEventFileInfo file, string message)
+    {
+        BuildWarningEventArgs buildEvent = EventsCreatorHelper.CreateWarningEventFromText(_eventContext, subcategoryResourceName, errorCode, helpKeyword, file, message);
+
+        _eventDispatcher.Dispatch(buildEvent);
+    }
 }

src/Build/BuildCheck/Infrastructure/CheckContext/CheckLoggingContext.cs

@@ -39,4 +39,8 @@ public void DispatchAsCommentFromText(MessageImportance importance, string messa
     public void DispatchAsErrorFromText(string? subcategoryResourceName, string? errorCode, string? helpKeyword, BuildEventFileInfo file, string message)
         => loggingService
             .LogErrorFromText(eventContext, subcategoryResourceName, errorCode, helpKeyword, file, message);
+
+    public void DispatchAsWarningFromText(string? subcategoryResourceName, string? errorCode, string? helpKeyword, BuildEventFileInfo file, string message)
+        => loggingService
+            .LogWarningFromText(eventContext, subcategoryResourceName, errorCode, helpKeyword, file, message);
 }

src/Build/BuildCheck/Infrastructure/CheckContext/ICheckContext.cs

@@ -40,4 +40,9 @@ internal interface ICheckContext
     /// Dispatch the instance of <see cref="BuildEventContext"/> as a comment with provided text for the message.
     /// </summary>
     void DispatchAsCommentFromText(MessageImportance importance, string message);
+
+    /// <summary>
+    /// Dispatch the instance of <see cref="BuildEventContext"/> as a warning message.
+    /// </summary>
+    void DispatchAsWarningFromText(string? subcategoryResourceName, string? errorCode, string? helpKeyword, BuildEventFileInfo file, string message);
 }

src/BuildCheck.UnitTests/EndToEndTests.cs

@@ -375,6 +375,39 @@ public void CustomCheckTest_WithEditorConfig(string checkCandidate, string ruleI
         }
     }
 
+    [Theory]
+    [InlineData("X01236", "Something went wrong initializing")]
+    // These tests are for failure one different points, will be addressed in a different PR
+    // https://github.com/dotnet/msbuild/issues/10522
+    // [InlineData("X01237", "message")]
+    // [InlineData("X01238", "message")]
+    public void CustomChecksFailGracefully(string ruleId, string expectedMessage)
+    {
+        using (var env = TestEnvironment.Create())
+        {
+            string checkCandidate = "CheckCandidateWithMultipleChecksInjected";
+            string checkCandidatePath = Path.Combine(TestAssetsRootPath, checkCandidate);
+
+            // Can't use Transitive environment due to the need to dogfood local nuget packages.
+            AddCustomDataSourceToNugetConfig(checkCandidatePath);
+            string editorConfigName = Path.Combine(checkCandidatePath, EditorConfigFileName);
+            File.WriteAllText(editorConfigName, ReadEditorConfig(
+                new List<(string, string)>() { (ruleId, "warning") },
+                ruleToCustomConfig: null,
+                checkCandidatePath));
+
+            string projectCheckBuildLog = RunnerUtilities.ExecBootstrapedMSBuild(
+                $"{Path.Combine(checkCandidatePath, $"{checkCandidate}.csproj")} /m:1 -nr:False -restore -check -verbosity:n", out bool success);
+
+            success.ShouldBeTrue();
+            projectCheckBuildLog.ShouldContain(expectedMessage);
+            projectCheckBuildLog.ShouldNotContain("This check should have been disabled");
+
+            // Cleanup
+            File.Delete(editorConfigName);
+        }
+    }
+
     [Theory]
     [InlineData(true)]
     [InlineData(false)]

src/BuildCheck.UnitTests/Microsoft.Build.BuildCheck.UnitTests.csproj

@@ -20,6 +20,7 @@
     <ProjectReference Include=".\TestAssets\CustomCheck\CustomCheck.csproj" />
     <ProjectReference Include=".\TestAssets\CustomCheck2\CustomCheck2.csproj" />
     <ProjectReference Include=".\TestAssets\InvalidCustomCheck\InvalidCustomCheck.csproj" />
+    <ProjectReference Include=".\TestAssets\ErrorCustomCheck\ErrorCustomCheck.csproj" />
   </ItemGroup>
 
   <ItemGroup>

src/BuildCheck.UnitTests/TestAssets/CheckCandidateWithMultipleChecksInjected/.editorconfigtest

@@ -2,3 +2,7 @@ root = true
 
 [*.csproj]
 build_check.X01234.Severity=X01234Severity
+
+build_check.X01236.Severity=X01236Severity
+build_check.X01237.Severity=X01237Severity
+build_check.X01238.Severity=X01238Severity

src/BuildCheck.UnitTests/TestAssets/CheckCandidateWithMultipleChecksInjected/CheckCandidateWithMultipleChecksInjected.csproj

@@ -9,6 +9,7 @@
     <PackageReference Include="CustomCheck" Version="1.0.0"/>
     <PackageReference Include="CustomCheck2" Version="1.0.0"/>
     <PackageReference Include="InvalidCustomCheck" Version="1.0.0"/>
+    <PackageReference Include="ErrorCustomCheck" Version="1.0.0"/>
   </ItemGroup>
 
   <ItemGroup>

src/BuildCheck.UnitTests/TestAssets/ErrorCustomCheck/ErrorCustomCheck.csproj

@@ -0,0 +1,17 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <Import Project="..\Common\CommonTest.props" />
+
+  <PropertyGroup>
+    <TargetFramework>netstandard2.0</TargetFramework>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <None Include="ErrorCustomCheck.props" Pack="true" PackagePath="build\ErrorCustomCheck.props" />
+    <Content Include="README.md" />
+  </ItemGroup>
+
+  <Import Project="..\Common\CommonTest.targets" />
+
+</Project>

src/BuildCheck.UnitTests/TestAssets/ErrorCustomCheck/ErrorCustomCheck.props

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project>
+  <PropertyGroup>
+	  <MSBuildCheck>$([MSBuild]::RegisterBuildCheck($(MSBuildThisFileDirectory)ErrorCustomCheck.dll))</MSBuildCheck>
+  </PropertyGroup>
+</Project>

src/BuildCheck.UnitTests/TestAssets/ErrorCustomCheck/ErrorOnInitializeCheck.cs

@@ -0,0 +1,43 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using Microsoft.Build.Construction;
+using Microsoft.Build.Experimental.BuildCheck;
+
+namespace ErrorCustomCheck
+{
+    public sealed class ErrorOnInitializeCheck : Check
+    {
+        public static CheckRule SupportedRule = new CheckRule(
+            "X01236",
+            "Title",
+            "Description",
+            "Message format: {0}",
+            new CheckConfiguration());
+
+        public override string FriendlyName => "ErrorOnInitializeCheck";
+
+        public override IReadOnlyList<CheckRule> SupportedRules { get; } = new List<CheckRule>() { SupportedRule };
+
+        public override void Initialize(ConfigurationContext configurationContext)
+        {
+            // configurationContext to be used only if check needs external configuration data.
+            throw new Exception("Something went wrong initializing");
+        }
+
+        public override void RegisterActions(IBuildCheckRegistrationContext registrationContext)
+        {
+            registrationContext.RegisterEvaluatedPropertiesAction(EvaluatedPropertiesAction);
+        }
+
+        private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedPropertiesCheckData> context)
+        {
+            context.ReportResult(BuildCheckResult.Create(
+                SupportedRule,
+                ElementLocation.EmptyLocation,
+                "This check should have been disabled"));
+        }
+    }
+}

src/BuildCheck.UnitTests/TestAssets/ErrorCustomCheck/ErrorOnRegisteredAction.cs

@@ -0,0 +1,39 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using Microsoft.Build.Construction;
+using Microsoft.Build.Experimental.BuildCheck;
+
+namespace ErrorCustomCheck
+{
+    public sealed class ErrorOnRegisteredAction : Check
+    {
+        public static CheckRule SupportedRule = new CheckRule(
+            "X01237",
+            "Title",
+            "Description",
+            "Message format: {0}",
+            new CheckConfiguration());
+
+        public override string FriendlyName => "ErrorOnEvaluatedPropertiesCheck";
+
+        public override IReadOnlyList<CheckRule> SupportedRules { get; } = new List<CheckRule>() { SupportedRule };
+
+        public override void Initialize(ConfigurationContext configurationContext)
+        {
+            // configurationContext to be used only if check needs external configuration data.
+        }
+
+        public override void RegisterActions(IBuildCheckRegistrationContext registrationContext)
+        {
+            registrationContext.RegisterEvaluatedPropertiesAction(EvaluatedPropertiesAction);
+        }
+
+        private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedPropertiesCheckData> context)
+        {
+            throw new Exception("something went wrong");
+        }
+    }
+}

src/BuildCheck.UnitTests/TestAssets/ErrorCustomCheck/ErrorWhenRegisteringActions.cs

@@ -0,0 +1,43 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using Microsoft.Build.Construction;
+using Microsoft.Build.Experimental.BuildCheck;
+
+namespace ErrorCustomCheck
+{
+    public sealed class ErrorWhenRegisteringActions : Check
+    {
+        public static CheckRule SupportedRule = new CheckRule(
+            "X01238",
+            "Title",
+            "Description",
+            "Message format: {0}",
+            new CheckConfiguration());
+
+        public override string FriendlyName => "ErrorOnEvaluatedPropertiesCheck";
+
+        public override IReadOnlyList<CheckRule> SupportedRules { get; } = new List<CheckRule>() { SupportedRule };
+
+        public override void Initialize(ConfigurationContext configurationContext)
+        {
+            // configurationContext to be used only if check needs external configuration data.
+        }
+
+        public override void RegisterActions(IBuildCheckRegistrationContext registrationContext)
+        {
+            registrationContext.RegisterEvaluatedPropertiesAction(EvaluatedPropertiesAction);
+            throw new Exception("something went wrong");
+        }
+
+        private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedPropertiesCheckData> context)
+        {
+            context.ReportResult(BuildCheckResult.Create(
+                SupportedRule,
+                ElementLocation.EmptyLocation,
+                "This check should have been disabled"));
+        }
+    }
+}

src/BuildCheck.UnitTests/TestAssets/ErrorCustomCheck/README.md

@@ -0,0 +1,21 @@
+# MSBuild Custom Check Template
+
+## Overview
+MSBuild Custom Check Template is a .NET template designed to streamline the creation of MSBuild check libraries. This template facilitates the development of custom checks targeting .NET Standard, enabling developers to inspect and enforce conventions, standards, or patterns within their MSBuild builds.
+
+## Features
+- Simplified template for creating MSBuild check libraries.
+- Targeting .NET Standard for cross-platform compatibility.
+- Provides a starting point for implementing custom check rules.
+
+## Getting Started
+To use the MSBuild Custom Check Template, follow these steps:
+1. Install the template using the following command:
+   ```bash
+   dotnet new install msbuildcheck
+2. Instantiate a custom template:
+   ```bash
+   dotnet new msbuildcheck -n <ProjectName>
+
+### Prerequisites
+- .NET SDK installed on your machine.