From cff3035a4ab99d15bdf34869ee935e54aca2f529 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Wed, 19 Jun 2024 08:47:16 -0700 Subject: [PATCH] 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 `` from `Moq.Analyzers.csproj` - Added `` and `MSBuildTreatWarningsAsErrors` elements to `CodeAnalysis.props` --- .editorconfig | 4 ++- ...tureShouldMatchMockedMethodCodeFixTests.cs | 1 + .../Moq.Analyzers.Test.csproj | 1 - ...ignatureShouldMatchMockedMethodAnalyzer.cs | 15 +++++++++- ...SignatureShouldMatchMockedMethodCodeFix.cs | 6 ++-- ...ConstructorArgumentsShouldMatchAnalyzer.cs | 19 +++++++------ Source/Moq.Analyzers/Helpers.cs | 28 +++++++++++-------- ...ructorArgumentsForInterfaceMockAnalyzer.cs | 10 +++++-- build/targets/codeanalysis/CodeAnalysis.props | 2 ++ 9 files changed, 56 insertions(+), 30 deletions(-) diff --git a/.editorconfig b/.editorconfig index a403677d..012d9871 100644 --- 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 \ No newline at end of file +dotnet_diagnostic.AV2318.severity = none +# MA0026: Fix TODO comment +dotnet_diagnostic.MA0026.severity = none \ No newline at end of file diff --git a/Source/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodCodeFixTests.cs b/Source/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodCodeFixTests.cs index af3280c0..23df15cc 100644 --- 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 TestData() { return new object[][] diff --git a/Source/Moq.Analyzers.Test/Moq.Analyzers.Test.csproj b/Source/Moq.Analyzers.Test/Moq.Analyzers.Test.csproj index 92b40846..10e52888 100644 --- a/Source/Moq.Analyzers.Test/Moq.Analyzers.Test.csproj +++ b/Source/Moq.Analyzers.Test/Moq.Analyzers.Test.csproj @@ -7,7 +7,6 @@ - 1701;1702;SA1600;SA1402 diff --git a/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs b/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs index 1b21a798..35e486e7 100644 --- a/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs +++ b/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs @@ -1,3 +1,5 @@ +using System.Diagnostics; + namespace Moq.Analyzers; /// @@ -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 index 29ea2956..8f29e2e2 100644 --- a/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodCodeFix.cs +++ b/Source/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodCodeFix.cs @@ -59,8 +59,6 @@ private async Task 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 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 index 65695cae..d33cdb6f 100644 --- 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() 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 index 7b04bfcd..f3f024db 100644 --- 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,24 +66,29 @@ internal static IEnumerable GetAllMatchingMockedMethodSymbolsFrom LambdaExpressionSyntax? setupLambdaArgument = setupMethodInvocation?.ArgumentList.Arguments[0].Expression as LambdaExpressionSyntax; InvocationExpressionSyntax? mockedMethodInvocation = setupLambdaArgument?.Body as InvocationExpressionSyntax; - return GetAllMatchingSymbols(semanticModel, mockedMethodInvocation); + return mockedMethodInvocation == null + ? [] + : GetAllMatchingSymbols(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 GetAllMatchingSymbols(SemanticModel semanticModel, ExpressionSyntax? expression) + internal static IEnumerable GetAllMatchingSymbols(SemanticModel semanticModel, ExpressionSyntax expression) where T : class { - if (expression == null) - { - return Enumerable.Empty(); - } - List 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) { @@ -92,7 +96,7 @@ internal static IEnumerable GetAllMatchingSymbols(SemanticModel semanticMo } 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 index e0dd7528..32d2bfaa 100644 --- 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 index 34eb6a65..c095d019 100644 --- a/build/targets/codeanalysis/CodeAnalysis.props +++ b/build/targets/codeanalysis/CodeAnalysis.props @@ -5,6 +5,8 @@ preview 9999 true + true + true