From e3e5dec23cf7660fc531006755cff62748d3c22b Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 25 Jun 2024 10:04:53 -0700 Subject: [PATCH] Moq1002: Update constructor checks (#115) There are cases where Moq1002 did not catch when there was a default constructor or a constructor with all default parameters. For more information see #55 Changes: - Add additional documentation for future reviewers - Add new case to check when the mocked type constructor has no parameters or all parameters have explicit default values - Refactored existing code to extract analysis for abstract and concrete types - Removed multiple array allocations in concrete type cases - Updated existing test case that was previously passing but shouldn't (see details in #55) - Added new test case for a type that only has ctors with default parameters --- ...ConstructorArgumentsShouldMatchAnalyzer.cs | 105 +++++++++++------- ...ructorArgumentsShouldMatchAnalyzerTests.cs | 23 +++- 2 files changed, 82 insertions(+), 46 deletions(-) diff --git a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index 69922915..d53d7779 100644 --- a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -39,36 +39,28 @@ public override void Initialize(AnalysisContext context) [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "MA0051:Method is too long", Justification = "Tracked in #90")] private static void Analyze(SyntaxNodeAnalysisContext context) { + // Pre-requisite checks ObjectCreationExpressionSyntax objectCreation = (ObjectCreationExpressionSyntax)context.Node; - GenericNameSyntax? genericName = GetGenericNameSyntax(objectCreation.Type); - if (genericName == null) return; - - if (!IsMockGenericType(genericName)) + if (genericName == null) { return; } - // Full check that we are calling new Mock() - IMethodSymbol? constructorSymbol = GetConstructorSymbol(context, objectCreation); - - // If constructorSymbol is null, we should have caught that earlier (and we cannot proceed) - if (constructorSymbol == null) + if (!IsMockGenericType(genericName)) { return; } - // Vararg parameter is the one that takes all arguments for mocked class constructor - IParameterSymbol? varArgsConstructorParameter = constructorSymbol.Parameters.FirstOrDefault(parameterSymbol => parameterSymbol.IsParams); + // Full check that we are calling new Mock() + IMethodSymbol? mockCtorSymbol = GetConstructorSymbol(context, objectCreation); - // Vararg parameter are not used, so there are no arguments for mocked class constructor - if (varArgsConstructorParameter == null) + // If mockCtorSymbol is null we cannot proceed! + if (mockCtorSymbol == null) { return; } - int varArgsConstructorParameterIndex = constructorSymbol.Parameters.IndexOf(varArgsConstructorParameter); - // Find mocked type INamedTypeSymbol? mockedTypeSymbol = GetMockedSymbol(context, genericName); if (mockedTypeSymbol == null) @@ -76,19 +68,64 @@ private static void Analyze(SyntaxNodeAnalysisContext context) return; } - // Skip first argument if it is not vararg - typically it is MockingBehavior argument - IEnumerable? constructorArguments = objectCreation.ArgumentList?.Arguments.Skip(varArgsConstructorParameterIndex == 0 ? 0 : 1); + // All the basic checks are done, now we need to check if the constructor arguments match + // the mocked class constructor + + // Vararg parameter is the one that takes all arguments for mocked class constructor + IParameterSymbol? varArgsConstructorParameter = mockCtorSymbol.Parameters.FirstOrDefault(parameterSymbol => parameterSymbol.IsParams); - if (!mockedTypeSymbol.IsAbstract) + // Vararg parameter are not used, so there are no arguments for mocked type constructor + if (varArgsConstructorParameter == null) { - AnalyzeConcrete(context, constructorArguments, objectCreation, genericName); + // Check if the mocked type has a default constructor or a constructor with all optional parameters + if (mockedTypeSymbol.Constructors.Any(methodSymbol => methodSymbol.Parameters.Length == 0 + || methodSymbol.Parameters.All(parameterSymbol => parameterSymbol.HasExplicitDefaultValue))) + { + return; + } + + // There is no default or constructor with all optional parameters on the mocked type + Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList?.GetLocation()); + context.ReportDiagnostic(diagnostic); } else { - // Issue #1: Currently detection does not work well for abstract classes because they cannot be instantiated + // Parameters were defined with the Mock ctor. We're only interested in the vararg parameter + // as that contains the arguments for the mocked type constructor + int varArgsConstructorParameterIndex = mockCtorSymbol.Parameters.IndexOf(varArgsConstructorParameter); + + // REVIEW: Assert the MockingBehavior is the first argument in this case + // Skip first argument if it is not vararg - typically it is MockingBehavior argument + IEnumerable? constructorArguments = objectCreation + .ArgumentList? + .Arguments + .Skip(varArgsConstructorParameterIndex == 0 ? 0 : 1); + + if (mockedTypeSymbol.IsAbstract) + { + // Issue #1: Currently detection does not work well for abstract classes because they cannot be instantiated - // The mocked symbol is abstract, so we need to check if the constructor arguments match the abstract class constructor - AnalyzeAbstract(context, constructorArguments, mockedTypeSymbol, objectCreation); + // The mocked symbol is abstract, so we need to check if the constructor arguments match the abstract class constructor + AnalyzeAbstract(context, constructorArguments, mockedTypeSymbol, objectCreation); + } + else + { + AnalyzeConcrete(context, constructorArguments, objectCreation, genericName); + } + } + } + + private static void AnalyzeConcrete( + SyntaxNodeAnalysisContext context, + IEnumerable? constructorArguments, + ObjectCreationExpressionSyntax objectCreation, + GenericNameSyntax genericName) + { + if (constructorArguments != null + && IsConstructorMismatch(context, objectCreation, genericName, constructorArguments)) + { + Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList?.GetLocation()); + context.ReportDiagnostic(diagnostic); } } @@ -103,11 +140,14 @@ private static void AnalyzeAbstract( if (constructorArguments != null) { ITypeSymbol[] argumentTypes = constructorArguments - .Select(arg => context.SemanticModel.GetTypeInfo(arg.Expression, context.CancellationToken).Type) + .Select(arg => + context.SemanticModel.GetTypeInfo(arg.Expression, context.CancellationToken).Type) .ToArray()!; // Check all constructors of the abstract type - for (int constructorIndex = 0; constructorIndex < mockedTypeSymbol.Constructors.Length; constructorIndex++) + for (int constructorIndex = 0; + constructorIndex < mockedTypeSymbol.Constructors.Length; + constructorIndex++) { IMethodSymbol constructor = mockedTypeSymbol.Constructors[constructorIndex]; if (AreParametersMatching(constructor.Parameters, argumentTypes)) @@ -123,21 +163,6 @@ private static void AnalyzeAbstract( context.ReportDiagnostic(diagnostic); } - private static void AnalyzeConcrete( - SyntaxNodeAnalysisContext context, - IEnumerable? constructorArguments, - ObjectCreationExpressionSyntax objectCreation, - GenericNameSyntax genericName) - { - if (constructorArguments != null - && IsConstructorMismatch(context, objectCreation, genericName, constructorArguments) - && objectCreation.ArgumentList != null) - { - Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList.GetLocation()); - context.ReportDiagnostic(diagnostic); - } - } - private static INamedTypeSymbol? GetMockedSymbol( SyntaxNodeAnalysisContext context, GenericNameSyntax genericName) @@ -212,7 +237,7 @@ private static bool IsConstructorMismatch( SyntaxNodeAnalysisContext context, ObjectCreationExpressionSyntax objectCreation, GenericNameSyntax genericName, - IEnumerable constructorArguments) + IEnumerable? constructorArguments) { ObjectCreationExpressionSyntax fakeConstructorCall = SyntaxFactory.ObjectCreationExpression( genericName.TypeArgumentList.Arguments.First(), diff --git a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.cs b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.cs index 4ce25e00..669ff259 100644 --- a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.cs +++ b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.cs @@ -8,9 +8,9 @@ public static IEnumerable TestData() { return new object[][] { - ["""new Mock(MockBehavior.Default);"""], - ["""new Mock(MockBehavior.Strict);"""], - ["""new Mock(MockBehavior.Loose);"""], + ["""new Mock{|Moq1002:(MockBehavior.Default)|};"""], + ["""new Mock{|Moq1002:(MockBehavior.Strict)|};"""], + ["""new Mock{|Moq1002:(MockBehavior.Loose)|};"""], ["""new Mock("3");"""], ["""new Mock("4");"""], ["""new Mock(MockBehavior.Default, "5");"""], @@ -35,9 +35,15 @@ public static IEnumerable TestData() ["""new Mock>{|Moq1002:(42)|};"""], ["""new Mock>();"""], ["""new Mock>(MockBehavior.Default);"""], - - // TODO: "I think this _should_ fail, but currently passes. Tracked by #55." - // ["""new Mock();"""], + ["""new Mock{|Moq1002:()|};"""], + ["""new Mock{|Moq1002:(MockBehavior.Strict)|};"""], + ["""new Mock{|Moq1002:(MockBehavior.Loose)|};"""], + ["""new Mock();"""], + ["""new Mock(MockBehavior.Strict);"""], + ["""new Mock(MockBehavior.Loose);"""], + ["""new Mock>{|Moq1002:()|};"""], + ["""new Mock>{|Moq1002:(MockBehavior.Strict)|};"""], + ["""new Mock>{|Moq1002:(MockBehavior.Loose)|};"""], ["""new Mock{|Moq1002:("42")|};"""], ["""new Mock{|Moq1002:("42", 42)|};"""], ["""new Mock{|Moq1002:(42)|};"""], @@ -77,6 +83,11 @@ internal abstract class AbstractGenericClassDefaultCtor protected AbstractGenericClassDefaultCtor() { } } + internal abstract class AbstractClassWithDefaultParamCtor + { + protected AbstractClassWithDefaultParamCtor(int a = 42) { } + } + internal abstract class AbstractClassWithCtor { protected AbstractClassWithCtor(int a) { }