Skip to content

Commit

Permalink
Moq1002: Update constructor checks (#115)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@rjmurillo
rjmurillo authored Jun 25, 2024
1 parent 48a4859 commit e3e5dec
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 46 deletions.
105 changes: 65 additions & 40 deletions src/Moq.Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,56 +39,93 @@ 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<T>()
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<T>()
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)
{
return;
}

// Skip first argument if it is not vararg - typically it is MockingBehavior argument
IEnumerable<ArgumentSyntax>? 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<ArgumentSyntax>? 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<ArgumentSyntax>? constructorArguments,
ObjectCreationExpressionSyntax objectCreation,
GenericNameSyntax genericName)
{
if (constructorArguments != null
&& IsConstructorMismatch(context, objectCreation, genericName, constructorArguments))
{
Diagnostic diagnostic = Diagnostic.Create(Rule, objectCreation.ArgumentList?.GetLocation());
context.ReportDiagnostic(diagnostic);
}
}

Expand All @@ -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))
Expand All @@ -123,21 +163,6 @@ private static void AnalyzeAbstract(
context.ReportDiagnostic(diagnostic);
}

private static void AnalyzeConcrete(
SyntaxNodeAnalysisContext context,
IEnumerable<ArgumentSyntax>? 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)
Expand Down Expand Up @@ -212,7 +237,7 @@ private static bool IsConstructorMismatch(
SyntaxNodeAnalysisContext context,
ObjectCreationExpressionSyntax objectCreation,
GenericNameSyntax genericName,
IEnumerable<ArgumentSyntax> constructorArguments)
IEnumerable<ArgumentSyntax>? constructorArguments)
{
ObjectCreationExpressionSyntax fakeConstructorCall = SyntaxFactory.ObjectCreationExpression(
genericName.TypeArgumentList.Arguments.First(),
Expand Down
23 changes: 17 additions & 6 deletions tests/Moq.Analyzers.Test/ConstructorArgumentsShouldMatchAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ public static IEnumerable<object[]> TestData()
{
return new object[][]
{
["""new Mock<Foo>(MockBehavior.Default);"""],
["""new Mock<Foo>(MockBehavior.Strict);"""],
["""new Mock<Foo>(MockBehavior.Loose);"""],
["""new Mock<Foo>{|Moq1002:(MockBehavior.Default)|};"""],
["""new Mock<Foo>{|Moq1002:(MockBehavior.Strict)|};"""],
["""new Mock<Foo>{|Moq1002:(MockBehavior.Loose)|};"""],
["""new Mock<Foo>("3");"""],
["""new Mock<Foo>("4");"""],
["""new Mock<Foo>(MockBehavior.Default, "5");"""],
Expand All @@ -35,9 +35,15 @@ public static IEnumerable<object[]> TestData()
["""new Mock<AbstractGenericClassDefaultCtor<object>>{|Moq1002:(42)|};"""],
["""new Mock<AbstractGenericClassDefaultCtor<object>>();"""],
["""new Mock<AbstractGenericClassDefaultCtor<object>>(MockBehavior.Default);"""],

// TODO: "I think this _should_ fail, but currently passes. Tracked by #55."
// ["""new Mock<AbstractClassWithCtor>();"""],
["""new Mock<AbstractClassWithCtor>{|Moq1002:()|};"""],
["""new Mock<AbstractClassWithCtor>{|Moq1002:(MockBehavior.Strict)|};"""],
["""new Mock<AbstractClassWithCtor>{|Moq1002:(MockBehavior.Loose)|};"""],
["""new Mock<AbstractClassWithDefaultParamCtor>();"""],
["""new Mock<AbstractClassWithDefaultParamCtor>(MockBehavior.Strict);"""],
["""new Mock<AbstractClassWithDefaultParamCtor>(MockBehavior.Loose);"""],
["""new Mock<AbstractGenericClassWithCtor<object>>{|Moq1002:()|};"""],
["""new Mock<AbstractGenericClassWithCtor<object>>{|Moq1002:(MockBehavior.Strict)|};"""],
["""new Mock<AbstractGenericClassWithCtor<object>>{|Moq1002:(MockBehavior.Loose)|};"""],
["""new Mock<AbstractClassWithCtor>{|Moq1002:("42")|};"""],
["""new Mock<AbstractClassWithCtor>{|Moq1002:("42", 42)|};"""],
["""new Mock<AbstractClassDefaultCtor>{|Moq1002:(42)|};"""],
Expand Down Expand Up @@ -77,6 +83,11 @@ internal abstract class AbstractGenericClassDefaultCtor<T>
protected AbstractGenericClassDefaultCtor() { }
}
internal abstract class AbstractClassWithDefaultParamCtor
{
protected AbstractClassWithDefaultParamCtor(int a = 42) { }
}
internal abstract class AbstractClassWithCtor
{
protected AbstractClassWithCtor(int a) { }
Expand Down

0 comments on commit e3e5dec

Please sign in to comment.