From 4cf69db852253d78d1177ef4eb675f001f0c180c Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 13:25:24 -0800 Subject: [PATCH] Remove false positive for Moq1200 when using parameterized lambda (#301) When using Moq's expression features for type construction, the Moq1200 analyzer was firing unexpectedly indicating a matching constructor on the type was not found. The `ConstructorArgumentsShouldMatchAnalyzer` was extracting the parameters from the Mock type and comparing them to the parameters of the constructed type as is, including the lambda and the `MockBehavior`. Example: ```csharp _ = new Mock(() => new Calculator(), MockBehavior.Loose); ``` > See #234 for more details This is incorrect for several reasons: 1. The parenthesized lambda is not itself a parameter for the target type, but rather the body 2. The `MockBehavior` would not likely be a parameter on the target type Correct analysis would be to drop the `MockBehavior` argument as with other configurations of the `Mock` type, and use the body of the lambda. However, using the body of the lambda is not necessary. The purpose of this analyzer is to detect errors that would not be caught at compile time that would result in a runtime error. In this case, using a constructor not available on the type would result in a compiler error. As such, the constructor detection method has been updated to return a tertiary result: `true` when there is a matching constructor found, `false` when there is not, and `null` when the constructor is a lambda (i.e., the constructor should be ignored). Changes: - Add unit test for issue #234 - Add support to ignore parameterized lambda arguments when performing constructor detection - Add support `MockBehavior` definitions to be in ordinal position 0 or 1 Fixes #234 --- ...ConstructorArgumentsShouldMatchAnalyzer.cs | 47 +++++++++++++++---- ...ntsShouldMatchAnalyzerTests.Expressions.cs | 39 +++++++++++++++ 2 files changed, 78 insertions(+), 8 deletions(-) create mode 100644 tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs diff --git a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index ae93b7ed..8af2adb0 100644 --- a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -99,9 +99,9 @@ private static bool IsExpressionMockBehavior(SyntaxNodeAnalysisContext context, return targetSymbol.IsInstanceOf(knownSymbols.MockBehavior); } - private static bool IsFirstArgumentMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ArgumentListSyntax? argumentList) + private static bool IsArgumentMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ArgumentListSyntax? argumentList, uint argumentOrdinal) { - ExpressionSyntax? expression = argumentList?.Arguments[0].Expression; + ExpressionSyntax? expression = argumentList?.Arguments.Count > argumentOrdinal ? argumentList.Arguments[(int)argumentOrdinal].Expression : null; return IsExpressionMockBehavior(context, knownSymbols, expression); } @@ -267,10 +267,13 @@ private static void AnalyzeNewObject(SyntaxNodeAnalysisContext context, MoqKnown /// The constructors. /// The arguments. /// The context. - /// true if a suitable constructor was found; otherwise false. + /// + /// if a suitable constructor was found; otherwise . + /// If the construction method is a parenthesized lambda expression, is returned. + /// /// Handles and optional parameters. [SuppressMessage("Design", "MA0051:Method is too long", Justification = "This should be refactored; suppressing for now to enable TreatWarningsAsErrors in CI.")] - private static bool AnyConstructorsFound( + private static bool? AnyConstructorsFound( IMethodSymbol[] constructors, ArgumentSyntax[] arguments, SyntaxNodeAnalysisContext context) @@ -348,6 +351,24 @@ private static bool AnyConstructorsFound( } } + // Special case for Lambda expression syntax + // In Moq you can specify a Lambda expression that creates an instance + // of the specified type + // See https://github.com/devlooped/moq/blob/18dc7410ad4f993ce0edd809c5dfcaa3199f13ff/src/Moq/Mock%601.cs#L200 + // + // The parenthesized lambda takes arguments as the first child node + // which may be empty or have args defined as part of a closure. + // Either way, we don't care about that, we only care that the + // constructor is valid. + // + // Since this does not use reflection through Castle, an invalid + // lambda here would cause the compiler to break, so no need to + // do additional checks. + if (arguments.Length == 1 && arguments[0].Expression.IsKind(SyntaxKind.ParenthesizedLambdaExpression)) + { + return null; + } + return false; } @@ -386,10 +407,18 @@ private static void VerifyMockAttempt( ArgumentSyntax[] arguments = argumentList?.Arguments.ToArray() ?? []; #pragma warning restore ECS0900 // Consider using an alternative implementation to avoid boxing and unboxing - if (hasMockBehavior && arguments.Length > 0 && IsFirstArgumentMockBehavior(context, knownSymbols, argumentList)) + if (hasMockBehavior && arguments.Length > 0) { - // They passed a mock behavior as the first argument; ignore as Moq swallows it - arguments = arguments.RemoveAt(0); + if (arguments.Length >= 1 && IsArgumentMockBehavior(context, knownSymbols, argumentList, 0)) + { + // They passed a mock behavior as the first argument; ignore as Moq swallows it + arguments = arguments.RemoveAt(0); + } + else if (arguments.Length >= 2 && IsArgumentMockBehavior(context, knownSymbols, argumentList, 1)) + { + // They passed a mock behavior as the second argument; ignore as Moq swallows it + arguments = arguments.RemoveAt(1); + } } switch (mockedClass.TypeKind) @@ -433,7 +462,9 @@ private static void VerifyClassMockAttempt( } // We have constructors, now we need to check if the arguments match any of them - if (!AnyConstructorsFound(constructors, arguments, context)) + // If the value is null it means we want to ignore and not create a diagnostic + bool? matchingCtorFound = AnyConstructorsFound(constructors, arguments, context); + if (matchingCtorFound.HasValue && !matchingCtorFound.Value) { Diagnostic diagnostic = constructorIsEmpty.Location.CreateDiagnostic(ClassMustHaveMatchingConstructor, argumentList); context.ReportDiagnostic(diagnostic); diff --git a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs new file mode 100644 index 00000000..d6d3b94f --- /dev/null +++ b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs @@ -0,0 +1,39 @@ +using Verifier = Moq.Analyzers.Test.Helpers.AnalyzerVerifier; + +namespace Moq.Analyzers.Test; + +public partial class ConstructorArgumentsShouldMatchAnalyzerTests +{ + public static IEnumerable ExpressionTestData() + { + return new object[][] + { + ["""_ = new Mock(() => new Calculator(), MockBehavior.Loose);"""], + ["""_ = new Mock(() => new Calculator(), MockBehavior.Strict);"""], + ["""_ = new Mock(() => new Calculator(), MockBehavior.Default);"""], + ["""_ = new Mock(() => new Calculator());"""], + }.WithNamespaces().WithMoqReferenceAssemblyGroups(); + } + + [Theory] + [MemberData(nameof(ExpressionTestData))] + public async Task ShouldPassIfExpressionWithDefaultCtorIsUsedWithMockBehavior(string referenceAssemblyGroup, string @namespace, string mock) + { + await Verifier.VerifyAnalyzerAsync( + $@" + {@namespace} + public class Calculator + {{ + public int Add(int a, int b) => a + b; + }} + internal class UnitTest + {{ + private void Test() + {{ + {mock} + }} + }} + ", + referenceAssemblyGroup); + } +}