From 20c0af0d84316a93cb0ac5677d3d67f4a27e2e67 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 09:53:35 -0800 Subject: [PATCH 1/7] Add test to reproduce issue #234 --- ...ntsShouldMatchAnalyzerTests.Expressions.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs diff --git a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs new file mode 100644 index 00000000..b79f209f --- /dev/null +++ b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs @@ -0,0 +1,36 @@ +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);"""] + }.WithNamespaces().WithMoqReferenceAssemblyGroups(); + } + + [Theory] + [MemberData(nameof(ExpressionTestData))] + public async Task ExpressionConstructor(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); + } +} From 4838e221b529ec6226ec84fd1ef87c6f10a2965b Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 10:24:39 -0800 Subject: [PATCH 2/7] Remove false positive for Moq1200 when using parameterized lambda Fixes #234 --- ...ConstructorArgumentsShouldMatchAnalyzer.cs | 50 ++++++++++++++++--- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index ae93b7ed..d5f6ac79 100644 --- a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -106,6 +106,13 @@ private static bool IsFirstArgumentMockBehavior(SyntaxNodeAnalysisContext contex return IsExpressionMockBehavior(context, knownSymbols, expression); } + private static bool IsSecondArgumentMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ArgumentListSyntax? argumentList) + { + ExpressionSyntax? expression = argumentList?.Arguments[1].Expression; + + return IsExpressionMockBehavior(context, knownSymbols, expression); + } + private static void VerifyDelegateMockAttempt( SyntaxNodeAnalysisContext context, ArgumentListSyntax? argumentList, @@ -267,10 +274,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 +358,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 +414,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 && IsFirstArgumentMockBehavior(context, knownSymbols, argumentList)) + { + // They passed a mock behavior as the first argument; ignore as Moq swallows it + arguments = arguments.RemoveAt(0); + } + else if (arguments.Length >= 2 && IsSecondArgumentMockBehavior(context, knownSymbols, argumentList)) + { + // They passed a mock behavior as the second argument; ignore as Moq swallows it + arguments = arguments.RemoveAt(1); + } } switch (mockedClass.TypeKind) @@ -433,7 +469,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); From accfc0a3be87daa852f6a9eb1dabebc37ee7e497 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 10:43:56 -0800 Subject: [PATCH 3/7] Add more descriptive name for the test --- .../ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs index b79f209f..e3c201d9 100644 --- a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs +++ b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs @@ -14,7 +14,7 @@ public static IEnumerable ExpressionTestData() [Theory] [MemberData(nameof(ExpressionTestData))] - public async Task ExpressionConstructor(string referenceAssemblyGroup, string @namespace, string mock) + public async Task ShouldPassIfExpressionWithDefaultCtorIsUsedWithMockBehavior(string referenceAssemblyGroup, string @namespace, string mock) { await Verifier.VerifyAnalyzerAsync( $@" From 2a0edfd842c7ec676c68e7466c188c58ef86bd99 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 10:46:45 -0800 Subject: [PATCH 4/7] DRY out detection of MockBehavior on argument list --- .../ConstructorArgumentsShouldMatchAnalyzer.cs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index d5f6ac79..16ca1393 100644 --- a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs +++ b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs @@ -99,16 +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, int argumentOrdinal) { - ExpressionSyntax? expression = argumentList?.Arguments[0].Expression; - - return IsExpressionMockBehavior(context, knownSymbols, expression); - } - - private static bool IsSecondArgumentMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ArgumentListSyntax? argumentList) - { - ExpressionSyntax? expression = argumentList?.Arguments[1].Expression; + ExpressionSyntax? expression = argumentList?.Arguments.Count > argumentOrdinal ? argumentList.Arguments[argumentOrdinal].Expression : null; return IsExpressionMockBehavior(context, knownSymbols, expression); } @@ -416,12 +409,12 @@ private static void VerifyMockAttempt( if (hasMockBehavior && arguments.Length > 0) { - if (arguments.Length >= 1 && IsFirstArgumentMockBehavior(context, knownSymbols, argumentList)) + 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 && IsSecondArgumentMockBehavior(context, knownSymbols, argumentList)) + 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); From 55acc3e6b52d95e47833ac790b78c66ac2816a34 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 10:50:58 -0800 Subject: [PATCH 5/7] Fix for SA1413 --- .../ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs index e3c201d9..1337565a 100644 --- a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs +++ b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs @@ -8,7 +8,7 @@ public static IEnumerable ExpressionTestData() { return new object[][] { - ["""_ = new Mock(() => new Calculator(), MockBehavior.Loose);"""] + ["""_ = new Mock(() => new Calculator(), MockBehavior.Loose);"""], }.WithNamespaces().WithMoqReferenceAssemblyGroups(); } From c2584f6fefb4c3e87574c449b41272a5a8851d19 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 10:52:18 -0800 Subject: [PATCH 6/7] Guard against negative argumentOrdinal to prevent IndexOutOfRangeException --- src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs b/src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs index 16ca1393..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 IsArgumentMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ArgumentListSyntax? argumentList, int argumentOrdinal) + private static bool IsArgumentMockBehavior(SyntaxNodeAnalysisContext context, MoqKnownSymbols knownSymbols, ArgumentListSyntax? argumentList, uint argumentOrdinal) { - ExpressionSyntax? expression = argumentList?.Arguments.Count > argumentOrdinal ? argumentList.Arguments[argumentOrdinal].Expression : null; + ExpressionSyntax? expression = argumentList?.Arguments.Count > argumentOrdinal ? argumentList.Arguments[(int)argumentOrdinal].Expression : null; return IsExpressionMockBehavior(context, knownSymbols, expression); } From 0d618c7ba7da9f544e760303666d89b17eb0c93b Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Tue, 24 Dec 2024 10:54:49 -0800 Subject: [PATCH 7/7] Add more unit test scenarios for lambda Add more variations with and without MockBehavior specified --- ...ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs index 1337565a..d6d3b94f 100644 --- a/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs +++ b/tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.Expressions.cs @@ -9,6 +9,9 @@ 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(); }