Enable warnings as errors (#91)

- Updated `.editorconfig` to set policy on some duplicate warnings for TODO. Borrowed from #89 to make merging easier - Added suppressions for analyzers that seem to be misfiring. Tracked in #90 - Updated cases where nullability was incorrect (something could be passed as null but wasn't, something was marked as nullable but wasn't). Where possible added asserts so we can catch cases when they arise. - Removed `<NoWarn>` from `Moq.Analyzers.csproj` - Added `<TreatWarningsAsErrors>` and `MSBuildTreatWarningsAsErrors` elements to `CodeAnalysis.props`
rjmurillo · Jun 19, 2024 · cff3035 · cff3035
1 parent e239295
commit cff3035
Show file tree

Hide file tree

Showing 9 changed files with 56 additions and 30 deletions.
diff --git a/.editorconfig b/.editorconfig
@@ -406,4 +406,6 @@ dotnet_diagnostic.CA2016.severity = error
 # S1135: Track uses of "TODO" tags
 dotnet_diagnostic.S1135.severity = suggestion
 # AV2318: Work-tracking TODO comment should be removed
-dotnet_diagnostic.AV2318.severity = none
+dotnet_diagnostic.AV2318.severity = none
+# MA0026: Fix TODO comment
+dotnet_diagnostic.MA0026.severity = none
diff --git a/Source/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodCodeFixTests.cs b/Source/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodCodeFixTests.cs
@@ -4,6 +4,7 @@ namespace Moq.Analyzers.Test;
 
 public class CallbackSignatureShouldMatchMockedMethodCodeFixTests
 {
+    [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "MA0051:Method is too long", Justification = "Contains test data")]
     public static IEnumerable<object[]> TestData()
     {
         return new object[][]

diff --git a/Source/Moq.Analyzers.Test/Moq.Analyzers.Test.csproj b/Source/Moq.Analyzers.Test/Moq.Analyzers.Test.csproj
@@ -7,7 +7,6 @@
   </PropertyGroup>
 
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'">
-    <NoWarn>1701;1702;SA1600;SA1402</NoWarn>
   </PropertyGroup>
 
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'">

diff --git a/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs b/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs
@@ -1,3 +1,5 @@
+using System.Diagnostics;
+
 namespace Moq.Analyzers;
 
 /// <summary>
@@ -69,10 +71,21 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
         {
             for (int argumentIndex = 0; argumentIndex < mockedMethodArguments.Count; argumentIndex++)
             {
+                TypeSyntax? lambdaParameterTypeSyntax = lambdaParameters[argumentIndex].Type;
+                Debug.Assert(lambdaParameterTypeSyntax != null, nameof(lambdaParameterTypeSyntax) + " != null");
+
+                // TODO: Don't know if continue or break is the right thing to do here
+#pragma warning disable S2589 // Boolean expressions should not be gratuitous
+                if (lambdaParameterTypeSyntax is null) continue;
+#pragma warning restore S2589 // Boolean expressions should not be gratuitous
+
+                TypeInfo lambdaParameterType = context.SemanticModel.GetTypeInfo(lambdaParameterTypeSyntax, context.CancellationToken);
+
                 TypeInfo mockedMethodArgumentType = context.SemanticModel.GetTypeInfo(mockedMethodArguments[argumentIndex].Expression, context.CancellationToken);
-                TypeInfo lambdaParameterType = context.SemanticModel.GetTypeInfo(lambdaParameters[argumentIndex].Type, context.CancellationToken);
+
                 string? mockedMethodTypeName = mockedMethodArgumentType.ConvertedType?.ToString();
                 string? lambdaParameterTypeName = lambdaParameterType.ConvertedType?.ToString();
+
                 if (!string.Equals(mockedMethodTypeName, lambdaParameterTypeName, StringComparison.Ordinal))
                 {
                     Diagnostic? diagnostic = Diagnostic.Create(Rule, callbackLambda.ParameterList.GetLocation());

diff --git a/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodCodeFix.cs b/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodCodeFix.cs
@@ -59,8 +59,6 @@ private async Task<Document> FixCallbackSignatureAsync(SyntaxNode root, Document
     {
         SemanticModel? semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
 
-        Debug.Assert(semanticModel != null, nameof(semanticModel) + " != null");
-
         if (semanticModel == null)
         {
             return document;
@@ -73,9 +71,9 @@ private async Task<Document> FixCallbackSignatureAsync(SyntaxNode root, Document
 
         InvocationExpressionSyntax? setupMethodInvocation = Helpers.FindSetupMethodFromCallbackInvocation(semanticModel, callbackInvocation, cancellationToken);
         Debug.Assert(setupMethodInvocation != null, nameof(setupMethodInvocation) + " != null");
-        IMethodSymbol[]? matchingMockedMethods = Helpers.GetAllMatchingMockedMethodSymbolsFromSetupMethodInvocation(semanticModel, setupMethodInvocation).ToArray();
+        IMethodSymbol[] matchingMockedMethods = Helpers.GetAllMatchingMockedMethodSymbolsFromSetupMethodInvocation(semanticModel, setupMethodInvocation).ToArray();
 
-        if (matchingMockedMethods.Length != 1 || oldParameters == null)
+        if (matchingMockedMethods.Length != 1)
         {
             return document;
         }

diff --git a/Source/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/Source/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs
@@ -36,6 +36,7 @@ public override void Initialize(AnalysisContext context)
     }
 
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Maintainability", "AV1500:Member or local function contains too many statements", Justification = "Tracked in https://github.com/rjmurillo/moq.analyzers/issues/90")]
+    [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "MA0051:Method is too long", Justification = "Tracked in #90")]
     private static void Analyze(SyntaxNodeAnalysisContext context)
     {
         ObjectCreationExpressionSyntax? objectCreation = (ObjectCreationExpressionSyntax)context.Node;
@@ -48,18 +49,17 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
         // Full check that we are calling new Mock<T>()
         IMethodSymbol? constructorSymbol = GetConstructorSymbol(context, objectCreation);
 
-        // Vararg parameter is the one that takes all arguments for mocked class constructor
-        IParameterSymbol? varArgsConstructorParameter = constructorSymbol?.Parameters.FirstOrDefault(parameterSymbol => parameterSymbol.IsParams);
+        Debug.Assert(constructorSymbol != null, nameof(constructorSymbol) + " != null");
 
-        // Vararg parameter are not used, so there are no arguments for mocked class constructor
-        if (varArgsConstructorParameter == null) return;
+#pragma warning disable S2589 // Boolean expressions should not be gratuitous
+        if (constructorSymbol is null) return;
+#pragma warning restore S2589 // Boolean expressions should not be gratuitous
 
-        Debug.Assert(constructorSymbol != null, nameof(constructorSymbol) + " != null");
+            // Vararg parameter is the one that takes all arguments for mocked class constructor
+        IParameterSymbol? varArgsConstructorParameter = constructorSymbol.Parameters.FirstOrDefault(parameterSymbol => parameterSymbol.IsParams);
 
-        if (constructorSymbol == null)
-        {
-            return;
-        }
+        // Vararg parameter are not used, so there are no arguments for mocked class constructor
+        if (varArgsConstructorParameter == null) return;
 
         int varArgsConstructorParameterIndex = constructorSymbol.Parameters.IndexOf(varArgsConstructorParameter);
 
@@ -167,6 +167,7 @@ private static bool IsMockGenericType(GenericNameSyntax genericName)
     {
         SymbolInfo constructorSymbolInfo = context.SemanticModel.GetSymbolInfo(objectCreation, context.CancellationToken);
         IMethodSymbol? constructorSymbol = constructorSymbolInfo.Symbol as IMethodSymbol;
+
         return constructorSymbol?.MethodKind == MethodKind.Constructor &&
                string.Equals(
                    constructorSymbol.ContainingType?.ConstructedFrom.ToDisplayString(),

diff --git a/Source/Moq.Analyzers/Helpers.cs b/Source/Moq.Analyzers/Helpers.cs
@@ -1,4 +1,5 @@
 using System.Diagnostics;
+using System.Linq.Expressions;
 
 namespace Moq.Analyzers;
 
@@ -17,14 +18,12 @@ internal static bool IsCallbackOrReturnInvocation(SemanticModel semanticModel, I
     {
         MemberAccessExpressionSyntax? callbackOrReturnsMethod = callbackOrReturnsInvocation.Expression as MemberAccessExpressionSyntax;
 
-        Debug.Assert(callbackOrReturnsMethod != null, nameof(callbackOrReturnsMethod) + " != null");
-
         if (callbackOrReturnsMethod == null)
         {
             return false;
         }
 
-        string? methodName = callbackOrReturnsMethod.Name.ToString();
+        string methodName = callbackOrReturnsMethod.Name.ToString();
 
         // First fast check before walking semantic model
         if (!string.Equals(methodName, "Callback", StringComparison.Ordinal)
@@ -67,32 +66,37 @@ internal static IEnumerable<IMethodSymbol> GetAllMatchingMockedMethodSymbolsFrom
         LambdaExpressionSyntax? setupLambdaArgument = setupMethodInvocation?.ArgumentList.Arguments[0].Expression as LambdaExpressionSyntax;
         InvocationExpressionSyntax? mockedMethodInvocation = setupLambdaArgument?.Body as InvocationExpressionSyntax;
 
-        return GetAllMatchingSymbols<IMethodSymbol>(semanticModel, mockedMethodInvocation);
+        return mockedMethodInvocation == null
+            ? []
+            : GetAllMatchingSymbols<IMethodSymbol>(semanticModel, mockedMethodInvocation);
     }
 
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Maintainability", "AV1500:Member or local function contains too many statements", Justification = "Tracked in https://github.com/rjmurillo/moq.analyzers/issues/90")]
-    internal static IEnumerable<T> GetAllMatchingSymbols<T>(SemanticModel semanticModel, ExpressionSyntax? expression)
+    internal static IEnumerable<T> GetAllMatchingSymbols<T>(SemanticModel semanticModel, ExpressionSyntax expression)
         where T : class
     {
-        if (expression == null)
-        {
-            return Enumerable.Empty<T>();
-        }
-
         List<T> matchingSymbols = new();
 
         SymbolInfo symbolInfo = semanticModel.GetSymbolInfo(expression);
         if (symbolInfo is { CandidateReason: CandidateReason.None, Symbol: T })
         {
-            matchingSymbols.Add(symbolInfo.Symbol as T);
+            T? value = symbolInfo.Symbol as T;
+            Debug.Assert(value != null, "Value should not be null.");
+
+#pragma warning disable S2589 // Boolean expressions should not be gratuitous
+            if (value != default(T))
+            {
+                matchingSymbols.Add(value);
+            }
+#pragma warning restore S2589 // Boolean expressions should not be gratuitous
         }
         else if (symbolInfo.CandidateReason == CandidateReason.OverloadResolutionFailure)
         {
             matchingSymbols.AddRange(symbolInfo.CandidateSymbols.OfType<T>());
         }
         else
         {
-            throw new NotSupportedException("Symbol not supported.");
+            return matchingSymbols;
         }
 
         return matchingSymbols;

diff --git a/Source/Moq.Analyzers/NoConstructorArgumentsForInterfaceMockAnalyzer.cs b/Source/Moq.Analyzers/NoConstructorArgumentsForInterfaceMockAnalyzer.cs
@@ -54,7 +54,13 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
 
         // Full check
         SymbolInfo constructorSymbolInfo = context.SemanticModel.GetSymbolInfo(objectCreation, context.CancellationToken);
-        if (constructorSymbolInfo.Symbol is not IMethodSymbol constructorSymbol || constructorSymbol.ContainingType == null || constructorSymbol.ContainingType.ConstructedFrom == null) return;
+        if (constructorSymbolInfo.Symbol is not IMethodSymbol constructorSymbol
+            || constructorSymbol.ContainingType == null
+            || constructorSymbol.ContainingType.ConstructedFrom == null)
+        {
+            return;
+        }
+
         if (constructorSymbol.MethodKind != MethodKind.Constructor) return;
         if (!string.Equals(
                 constructorSymbol.ContainingType.ConstructedFrom.ToDisplayString(),
@@ -64,7 +70,7 @@ private static void Analyze(SyntaxNodeAnalysisContext context)
             return;
         }
 
-        if (constructorSymbol.Parameters == null || constructorSymbol.Parameters.Length == 0) return;
+        if (constructorSymbol.Parameters.Length == 0) return;
         if (!constructorSymbol.Parameters.Any(parameterSymbol => parameterSymbol.IsParams)) return;
 
         // Find mocked type

diff --git a/build/targets/codeanalysis/CodeAnalysis.props b/build/targets/codeanalysis/CodeAnalysis.props
@@ -5,6 +5,8 @@
     <AnalysisMode>preview</AnalysisMode>
     <WarningLevel>9999</WarningLevel>
     <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <TreatWarningsAsErrors Condition=" '$(ContinuousIntegrationBuild)' == 'true' ">true</TreatWarningsAsErrors>
+    <MSBuildTreatWarningsAsErrors Condition=" '$(ContinuousIntegrationBuild)' == 'true' ">true</MSBuildTreatWarningsAsErrors>
   </PropertyGroup>
 
   <ItemGroup>