From 3a1e9afcca1b4693ef50800785b8a31ef2f01070 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Mon, 24 Jun 2024 11:03:18 -0700 Subject: [PATCH 1/4] Add messy implementation to check for default ctor --- ...ConstructorArgumentsShouldMatchAnalyzer.cs | 115 +++++++++++------- 1 file changed, 71 insertions(+), 44 deletions(-) diff --git a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index d33cdb6fb..0a7587336 100644 --- a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -39,75 +39,100 @@ 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) { - ObjectCreationExpressionSyntax? objectCreation = (ObjectCreationExpressionSyntax)context.Node; - + // Pre-requisite checks + ObjectCreationExpressionSyntax objectCreation = (ObjectCreationExpressionSyntax)context.Node; GenericNameSyntax? genericName = GetGenericNameSyntax(objectCreation.Type); - if (genericName == null) return; + if (genericName == null) + { + return; + } if (!IsMockGenericType(genericName)) return; // Full check that we are calling new Mock() - IMethodSymbol? constructorSymbol = GetConstructorSymbol(context, objectCreation); - - Debug.Assert(constructorSymbol != null, nameof(constructorSymbol) + " != null"); - -#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 - - // Vararg parameter is the one that takes all arguments for mocked class constructor - IParameterSymbol? varArgsConstructorParameter = constructorSymbol.Parameters.FirstOrDefault(parameterSymbol => parameterSymbol.IsParams); + IMethodSymbol? mockCtorSymbol = GetConstructorSymbol(context, objectCreation); - // Vararg parameter are not used, so there are no arguments for mocked class constructor - if (varArgsConstructorParameter == null) return; - - int varArgsConstructorParameterIndex = constructorSymbol.Parameters.IndexOf(varArgsConstructorParameter); + if (mockCtorSymbol is null) + { + return; + } // Find mocked type INamedTypeSymbol? mockedTypeSymbol = GetMockedSymbol(context, genericName); - if (mockedTypeSymbol == null) return; + if (mockedTypeSymbol == null) + { + return; + } + + // All the basic checks are done, now we need to check if the constructor arguments match + // the mocked class constructor - // Skip first argument if it is not vararg - typically it is MockingBehavior argument - ArgumentSyntax[]? constructorArguments = objectCreation.ArgumentList?.Arguments.Skip(varArgsConstructorParameterIndex == 0 ? 0 : 1).ToArray(); + // 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) { - if (constructorArguments != null - && IsConstructorMismatch(context, objectCreation, genericName, constructorArguments) - && objectCreation.ArgumentList != null) + // Check if the mocked type has a default constructor + if (mockedTypeSymbol.Constructors.Any(methodSymbol => methodSymbol.Parameters.Length == 0)) { - Diagnostic? diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList.GetLocation()); - context.ReportDiagnostic(diagnostic); + return; } + + // There is no default constructor 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 + int varArgsConstructorParameterIndex = mockCtorSymbol.Parameters.IndexOf(varArgsConstructorParameter); - // The mocked symbol is abstract, so we need to check if the constructor arguments match the abstract class constructor + // Skip first argument if it is not vararg - typically it is MockingBehavior argument + ArgumentSyntax[]? constructorArguments = objectCreation.ArgumentList?.Arguments + .Skip(varArgsConstructorParameterIndex == 0 ? 0 : 1).ToArray(); - // Extract types of arguments passed in the constructor call - if (constructorArguments != null) + if (mockedTypeSymbol.IsAbstract) { - ITypeSymbol[] argumentTypes = constructorArguments - .Select(arg => context.SemanticModel.GetTypeInfo(arg.Expression, context.CancellationToken).Type) - .ToArray()!; + // Issue #1: Currently detection does not work well for abstract classes because they cannot be instantiated - // Check all constructors of the abstract type - for (int constructorIndex = 0; constructorIndex < mockedTypeSymbol.Constructors.Length; constructorIndex++) + // The mocked symbol is abstract, so we need to check if the constructor arguments match the abstract class constructor + + // Extract types of arguments passed in the constructor call + if (constructorArguments != null) { - IMethodSymbol constructor = mockedTypeSymbol.Constructors[constructorIndex]; - if (AreParametersMatching(constructor.Parameters, argumentTypes)) + ITypeSymbol[] argumentTypes = constructorArguments + .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++) { - return; // Found a matching constructor + IMethodSymbol constructor = mockedTypeSymbol.Constructors[constructorIndex]; + if (AreParametersMatching(constructor.Parameters, argumentTypes)) + { + return; // Found a matching constructor + } } } - } - Debug.Assert(objectCreation.ArgumentList != null, "objectCreation.ArgumentList != null"); + Debug.Assert(objectCreation.ArgumentList != null, "objectCreation.ArgumentList != null"); - Diagnostic? diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList?.GetLocation()); - context.ReportDiagnostic(diagnostic); + Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList?.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + else + { + if (constructorArguments != null + && IsConstructorMismatch(context, objectCreation, genericName, constructorArguments) + && objectCreation.ArgumentList != null) + { + Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + } } } @@ -122,7 +147,9 @@ private static void Analyze(SyntaxNodeAnalysisContext context) return mockedTypeSymbol; } - private static bool AreParametersMatching(ImmutableArray constructorParameters, ITypeSymbol[] argumentTypes2) + private static bool AreParametersMatching( + ImmutableArray constructorParameters, + ITypeSymbol[] argumentTypes2) { // Check if the number of parameters matches if (constructorParameters.Length != argumentTypes2.Length) @@ -179,7 +206,7 @@ private static bool IsMockGenericType(GenericNameSyntax genericName) private static bool IsConstructorMismatch(SyntaxNodeAnalysisContext context, ObjectCreationExpressionSyntax objectCreation, GenericNameSyntax genericName, ArgumentSyntax[] constructorArguments) { - ObjectCreationExpressionSyntax? fakeConstructorCall = SyntaxFactory.ObjectCreationExpression( + ObjectCreationExpressionSyntax fakeConstructorCall = SyntaxFactory.ObjectCreationExpression( genericName.TypeArgumentList.Arguments.First(), SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(constructorArguments)), null); From 203f9fd7a15f527aed30b42ed0b42c0286ffce99 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Mon, 24 Jun 2024 11:35:43 -0700 Subject: [PATCH 2/4] Add test case for when all ctors have default values (effective like a default ctor) --- ...ConstructorArgumentsShouldMatchAnalyzer.cs | 5 ++-- ...ructorArgumentsShouldMatchAnalyzerTests.cs | 23 ++++++++++++++----- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index 0a7587336..87b4fe0e4 100644 --- a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -73,8 +73,9 @@ private static void Analyze(SyntaxNodeAnalysisContext context) // Vararg parameter are not used, so there are no arguments for mocked type constructor if (varArgsConstructorParameter == null) { - // Check if the mocked type has a default constructor - if (mockedTypeSymbol.Constructors.Any(methodSymbol => methodSymbol.Parameters.Length == 0)) + // 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; } diff --git a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.cs b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.cs index 4ce25e004..669ff259c 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) { } From 09304eea413d613d4cceae4ec0bd7037d6b7fe30 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Mon, 24 Jun 2024 11:38:55 -0700 Subject: [PATCH 3/4] Improve documentation for future maintainers --- src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index 87b4fe0e4..ed5f434c2 100644 --- a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -80,14 +80,17 @@ private static void Analyze(SyntaxNodeAnalysisContext context) return; } - // There is no default constructor on the mocked type + // 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 { + // 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 ArgumentSyntax[]? constructorArguments = objectCreation.ArgumentList?.Arguments .Skip(varArgsConstructorParameterIndex == 0 ? 0 : 1).ToArray(); From 924c078b1f61eaa7df7cb1ed24128d90553cbb52 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Mon, 24 Jun 2024 11:50:43 -0700 Subject: [PATCH 4/4] Refactoring to improve readability --- ...ConstructorArgumentsShouldMatchAnalyzer.cs | 101 +++++++++++------- 1 file changed, 63 insertions(+), 38 deletions(-) diff --git a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index ed5f434c2..f3f2f89e0 100644 --- a/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -92,52 +92,71 @@ private static void Analyze(SyntaxNodeAnalysisContext context) // REVIEW: Assert the MockingBehavior is the first argument in this case // Skip first argument if it is not vararg - typically it is MockingBehavior argument - ArgumentSyntax[]? constructorArguments = objectCreation.ArgumentList?.Arguments - .Skip(varArgsConstructorParameterIndex == 0 ? 0 : 1).ToArray(); + 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 - - // Extract types of arguments passed in the constructor call - if (constructorArguments != null) - { - ITypeSymbol[] argumentTypes = constructorArguments - .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++) - { - IMethodSymbol constructor = mockedTypeSymbol.Constructors[constructorIndex]; - if (AreParametersMatching(constructor.Parameters, argumentTypes)) - { - return; // Found a matching constructor - } - } - } - - Debug.Assert(objectCreation.ArgumentList != null, "objectCreation.ArgumentList != null"); - - Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList?.GetLocation()); - context.ReportDiagnostic(diagnostic); + AnalyzeAbstract(context, constructorArguments, mockedTypeSymbol, objectCreation); } else { - if (constructorArguments != null - && IsConstructorMismatch(context, objectCreation, genericName, constructorArguments) - && objectCreation.ArgumentList != null) + 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); + } + } + + [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")] + private static void AnalyzeAbstract( + SyntaxNodeAnalysisContext context, + IEnumerable? constructorArguments, + INamedTypeSymbol mockedTypeSymbol, + ObjectCreationExpressionSyntax objectCreation) + { + // Extract types of arguments passed in the constructor call + if (constructorArguments != null) + { + ITypeSymbol[] argumentTypes = constructorArguments + .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++) + { + IMethodSymbol constructor = mockedTypeSymbol.Constructors[constructorIndex]; + if (AreParametersMatching(constructor.Parameters, argumentTypes)) { - Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList.GetLocation()); - context.ReportDiagnostic(diagnostic); + return; } } } + + Debug.Assert(objectCreation.ArgumentList != null, "objectCreation.ArgumentList != null"); + + Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList?.GetLocation()); + context.ReportDiagnostic(diagnostic); } private static INamedTypeSymbol? GetMockedSymbol( @@ -153,10 +172,10 @@ private static void Analyze(SyntaxNodeAnalysisContext context) private static bool AreParametersMatching( ImmutableArray constructorParameters, - ITypeSymbol[] argumentTypes2) + ITypeSymbol[] argumentTypes) { // Check if the number of parameters matches - if (constructorParameters.Length != argumentTypes2.Length) + if (constructorParameters.Length != argumentTypes.Length) { return false; } @@ -164,7 +183,7 @@ private static bool AreParametersMatching( // Check if each parameter type matches in order for (int constructorParameterIndex = 0; constructorParameterIndex < constructorParameters.Length; constructorParameterIndex++) { - if (!constructorParameters[constructorParameterIndex].Type.Equals(argumentTypes2[constructorParameterIndex], SymbolEqualityComparer.IncludeNullability)) + if (!constructorParameters[constructorParameterIndex].Type.Equals(argumentTypes[constructorParameterIndex], SymbolEqualityComparer.IncludeNullability)) { return false; } @@ -175,6 +194,8 @@ private static bool AreParametersMatching( private static GenericNameSyntax? GetGenericNameSyntax(TypeSyntax typeSyntax) { + // REVIEW: Switch and ifs are equal in this case, but switch causes AV1535 to trigger + // The switch expression adds more instructions to do the same, so stick with ifs if (typeSyntax is GenericNameSyntax genericNameSyntax) { return genericNameSyntax; @@ -208,12 +229,16 @@ private static bool IsMockGenericType(GenericNameSyntax genericName) : null; } - private static bool IsConstructorMismatch(SyntaxNodeAnalysisContext context, ObjectCreationExpressionSyntax objectCreation, GenericNameSyntax genericName, ArgumentSyntax[] constructorArguments) + private static bool IsConstructorMismatch( + SyntaxNodeAnalysisContext context, + ObjectCreationExpressionSyntax objectCreation, + GenericNameSyntax genericName, + IEnumerable? constructorArguments) { ObjectCreationExpressionSyntax fakeConstructorCall = SyntaxFactory.ObjectCreationExpression( genericName.TypeArgumentList.Arguments.First(), SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(constructorArguments)), - null); + initializer: null); SymbolInfo mockedClassConstructorSymbolInfo = context.SemanticModel.GetSpeculativeSymbolInfo( objectCreation.SpanStart, fakeConstructorCall, SpeculativeBindingOption.BindAsExpression);