Add analyzer to detect assertions in catch blocks (MSTEST0058)

src/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.md

@@ -1,2 +1,8 @@
 ; Unshipped analyzer release
 ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md
+
+### New Rules
+
+Rule ID | Category | Severity | Notes
+--------|----------|----------|-------
+MSTEST0058 | Usage | Info | AvoidAssertsInCatchBlocksAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/core/testing/mstest-analyzers/mstest0058)

src/Analyzers/MSTest.Analyzers/AvoidAssertsInCatchBlocksAnalyzer.cs

@@ -0,0 +1,99 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Collections.Immutable;
+
+using Analyzer.Utilities.Extensions;
+
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+
+using MSTest.Analyzers.Helpers;
+
+namespace MSTest.Analyzers;
+
+/// <summary>
+/// MSTEST0058: <inheritdoc cref="Resources.AvoidAssertsInCatchBlocksTitle"/>.
+/// </summary>
+[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+public sealed class AvoidAssertsInCatchBlocksAnalyzer : DiagnosticAnalyzer
+{
+    private static readonly LocalizableResourceString Title = new(nameof(Resources.AvoidAssertsInCatchBlocksTitle), Resources.ResourceManager, typeof(Resources));
+    private static readonly LocalizableResourceString MessageFormat = new(nameof(Resources.AvoidAssertsInCatchBlocksMessageFormat), Resources.ResourceManager, typeof(Resources));
+    private static readonly LocalizableResourceString Description = new(nameof(Resources.AvoidAssertsInCatchBlocksDescription), Resources.ResourceManager, typeof(Resources));
+
+    internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create(
+        DiagnosticIds.AvoidAssertsInCatchBlocksRuleId,
+        Title,
+        MessageFormat,
+        Description,
+        Category.Usage,
+        DiagnosticSeverity.Info,
+        isEnabledByDefault: true);
+
+    /// <inheritdoc />
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; }
+        = ImmutableArray.Create(Rule);
+
+    /// <inheritdoc />
+    public override void Initialize(AnalysisContext context)
+    {
+        context.EnableConcurrentExecution();
+        context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+
+        context.RegisterCompilationStartAction(context =>
+        {
+            Compilation compilation = context.Compilation;
+            INamedTypeSymbol? assertSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingAssert);
+            INamedTypeSymbol? stringAssertSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingStringAssert);
+            INamedTypeSymbol? collectionAssertSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingCollectionAssert);
+
+            if (assertSymbol is not null || stringAssertSymbol is not null || collectionAssertSymbol is not null)
+            {
+                context.RegisterOperationAction(context => AnalyzeOperation(context, assertSymbol, stringAssertSymbol, collectionAssertSymbol), OperationKind.Invocation);
+            }
+        });
+    }
+
+    private static void AnalyzeOperation(
+        OperationAnalysisContext context,
+        INamedTypeSymbol? assertSymbol,
+        INamedTypeSymbol? stringAssertSymbol,
+        INamedTypeSymbol? collectionAssertSymbol)
+    {
+        var operation = (IInvocationOperation)context.Operation;
+        INamedTypeSymbol targetType = operation.TargetMethod.ContainingType;
+        bool isAssertType =
+            targetType.Equals(assertSymbol, SymbolEqualityComparer.Default) ||
+            targetType.Equals(stringAssertSymbol, SymbolEqualityComparer.Default) ||
+            targetType.Equals(collectionAssertSymbol, SymbolEqualityComparer.Default);
+
+        if (!isAssertType)
+        {
+            return;
+        }
+
+        // Walk up the operation tree to check if we're inside a catch clause
+        if (IsInsideCatchClause(operation))
+        {
+            context.ReportDiagnostic(operation.CreateDiagnostic(Rule));
+        }
+    }
+
+    private static bool IsInsideCatchClause(IOperation operation)
+    {
+        IOperation? current = operation;
+        while (current is not null)
+        {
+            if (current is ICatchClauseOperation)
+            {
+                return true;
+            }
+
+            current = current.Parent;
+        }
+
+        return false;
+    }
+}

src/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs

@@ -62,4 +62,5 @@ internal static class DiagnosticIds
     public const string IgnoreStringMethodReturnValueRuleId = "MSTEST0055";
     public const string TestMethodAttributeShouldSetDisplayNameCorrectlyRuleId = "MSTEST0056";
     public const string TestMethodAttributeShouldPropagateSourceInformationRuleId = "MSTEST0057";
+    public const string AvoidAssertsInCatchBlocksRuleId = "MSTEST0058";
 }

src/Analyzers/MSTest.Analyzers/Resources.resx

@@ -684,4 +684,13 @@ The type declaring these methods should also respect the following rules:
   <data name="IgnoreStringMethodReturnValueDescription" xml:space="preserve">
     <value>Methods like Contains, StartsWith, and EndsWith return boolean values that indicate whether the condition was met. Ignoring these return values is likely a mistake.</value>
   </data>
+  <data name="AvoidAssertsInCatchBlocksTitle" xml:space="preserve">
+    <value>Do not use asserts in catch blocks</value>
+  </data>
+  <data name="AvoidAssertsInCatchBlocksMessageFormat" xml:space="preserve">
+    <value>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</value>
+  </data>
+  <data name="AvoidAssertsInCatchBlocksDescription" xml:space="preserve">
+    <value>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</value>
+  </data>
 </root>

src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlf

@@ -989,6 +989,21 @@ Typ deklarující tyto metody by měl také respektovat následující pravidla:
         <target state="translated">Parametr Assert.Throws by měl obsahovat jenom jeden příkaz nebo výraz</target>
         <note />
       </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksTitle">
+        <source>Do not use asserts in catch blocks</source>
+        <target state="new">Do not use asserts in catch blocks</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksMessageFormat">
+        <source>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</source>
+        <target state="new">Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksDescription">
+        <source>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</source>
+        <target state="new">Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlf

@@ -990,6 +990,21 @@ Der Typ, der diese Methoden deklariert, sollte auch die folgenden Regeln beachte
         <target state="translated">Assert.Throws darf nur eine einzelne Anweisung/einen einzelnen Ausdruck enthalten</target>
         <note />
       </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksTitle">
+        <source>Do not use asserts in catch blocks</source>
+        <target state="new">Do not use asserts in catch blocks</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksMessageFormat">
+        <source>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</source>
+        <target state="new">Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksDescription">
+        <source>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</source>
+        <target state="new">Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlf

@@ -989,6 +989,21 @@ El tipo que declara estos métodos también debe respetar las reglas siguientes:
         <target state="translated">Assert.Throws debe contener solo una única instrucción o expresión</target>
         <note />
       </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksTitle">
+        <source>Do not use asserts in catch blocks</source>
+        <target state="new">Do not use asserts in catch blocks</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksMessageFormat">
+        <source>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</source>
+        <target state="new">Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksDescription">
+        <source>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</source>
+        <target state="new">Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf

@@ -989,6 +989,21 @@ Le type doit être une classe
         <target state="translated">Assert.Throws ne doit contenir qu’une seule instruction</target>
         <note />
       </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksTitle">
+        <source>Do not use asserts in catch blocks</source>
+        <target state="new">Do not use asserts in catch blocks</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksMessageFormat">
+        <source>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</source>
+        <target state="new">Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksDescription">
+        <source>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</source>
+        <target state="new">Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlf

@@ -989,6 +989,21 @@ Anche il tipo che dichiara questi metodi deve rispettare le regole seguenti:
         <target state="translated">Assert.Throws deve contenere solo una singola istruzione/espressione</target>
         <note />
       </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksTitle">
+        <source>Do not use asserts in catch blocks</source>
+        <target state="new">Do not use asserts in catch blocks</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksMessageFormat">
+        <source>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</source>
+        <target state="new">Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksDescription">
+        <source>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</source>
+        <target state="new">Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlf

@@ -989,6 +989,21 @@ The type declaring these methods should also respect the following rules:
         <target state="translated">Assert.Throws には 1 つのステートメントまたは式のみを含める必要があります</target>
         <note />
       </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksTitle">
+        <source>Do not use asserts in catch blocks</source>
+        <target state="new">Do not use asserts in catch blocks</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksMessageFormat">
+        <source>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</source>
+        <target state="new">Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksDescription">
+        <source>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</source>
+        <target state="new">Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>

src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlf

@@ -989,6 +989,21 @@ The type declaring these methods should also respect the following rules:
         <target state="translated">Assert.Throws는 단일 문/식만 포함해야 합니다.</target>
         <note />
       </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksTitle">
+        <source>Do not use asserts in catch blocks</source>
+        <target state="new">Do not use asserts in catch blocks</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksMessageFormat">
+        <source>Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</source>
+        <target state="new">Do not use asserts in catch blocks because they may not fail the test if no exception is thrown</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="AvoidAssertsInCatchBlocksDescription">
+        <source>Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</source>
+        <target state="new">Using asserts in catch blocks is problematic because the test will pass even if no exception is thrown and the catch block is never executed. Use 'Assert.Throws', 'Assert.ThrowsExactly', 'Assert.ThrowsAsync' or 'Assert.ThrowsExactlyAsync' to verify that an exception is thrown, and then make additional assertions on the caught exception without using the try-catch block.</target>
+        <note />
+      </trans-unit>
     </body>
   </file>
 </xliff>