Skip to content

Commit

Permalink
Moq1100 incorrectly firing on nullable parameters (#187)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rjmurillo
rjmurillo authored Aug 28, 2024
1 parent d3582e0 commit 993844f
Show file tree
Hide file tree
Showing 2 changed files with 196 additions and 18 deletions.
71 changes: 56 additions & 15 deletions src/Moq.Analyzers/CallbackSignatureShouldMatchMockedMethodAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,31 +58,72 @@ private static void Analyze(SyntaxNodeAnalysisContext context)

if (mockedMethodArguments.Count != lambdaParameters.Count)
{
Diagnostic diagnostic = Diagnostic.Create(Rule, callbackLambda.ParameterList.GetLocation());
Diagnostic diagnostic = callbackLambda.ParameterList.GetLocation().CreateDiagnostic(Rule);
context.ReportDiagnostic(diagnostic);
}
else
{
for (int argumentIndex = 0; argumentIndex < mockedMethodArguments.Count; argumentIndex++)
{
TypeSyntax? lambdaParameterTypeSyntax = lambdaParameters[argumentIndex].Type;
ValidateParameters(context, mockedMethodArguments, lambdaParameters);
}
}

// TODO: Don't know if continue or break is the right thing to do here
if (lambdaParameterTypeSyntax is null) continue;
private static void ValidateParameters(
SyntaxNodeAnalysisContext context,
SeparatedSyntaxList<ArgumentSyntax> mockedMethodArguments,
SeparatedSyntaxList<ParameterSyntax> lambdaParameters)
{
for (int argumentIndex = 0; argumentIndex < mockedMethodArguments.Count; argumentIndex++)
{
TypeSyntax? lambdaParameterTypeSyntax = lambdaParameters[argumentIndex].Type;

// We're unable to get the type from the Syntax Tree, so abort the type checking because something else
// is happening (e.g., we're compiling on partial code) and we need the type to do the additional checks.
if (lambdaParameterTypeSyntax is null)
{
continue;
}

TypeInfo lambdaParameterType = context.SemanticModel.GetTypeInfo(lambdaParameterTypeSyntax, context.CancellationToken);
TypeInfo lambdaParameterType = context.SemanticModel.GetTypeInfo(lambdaParameterTypeSyntax, context.CancellationToken);
TypeInfo mockedMethodArgumentType = context.SemanticModel.GetTypeInfo(mockedMethodArguments[argumentIndex].Expression, context.CancellationToken);

TypeInfo mockedMethodArgumentType = context.SemanticModel.GetTypeInfo(mockedMethodArguments[argumentIndex].Expression, context.CancellationToken);
ITypeSymbol? lambdaParameterTypeSymbol = lambdaParameterType.Type;
ITypeSymbol? mockedMethodTypeSymbol = mockedMethodArgumentType.Type;

string? mockedMethodTypeName = mockedMethodArgumentType.ConvertedType?.ToString();
string? lambdaParameterTypeName = lambdaParameterType.ConvertedType?.ToString();
if (lambdaParameterTypeSymbol is null || mockedMethodTypeSymbol is null)
{
continue;
}

if (!string.Equals(mockedMethodTypeName, lambdaParameterTypeName, StringComparison.Ordinal))
{
Diagnostic diagnostic = callbackLambda.ParameterList.CreateDiagnostic(Rule);
context.ReportDiagnostic(diagnostic);
}
if (!HasConversion(context.SemanticModel, mockedMethodTypeSymbol, lambdaParameterTypeSymbol))
{
Diagnostic diagnostic = lambdaParameters[argumentIndex].GetLocation().CreateDiagnostic(Rule);
context.ReportDiagnostic(diagnostic);
}
}
}

private static bool HasConversion(SemanticModel semanticModel, ITypeSymbol source, ITypeSymbol destination)
{
// This condition checks whether a valid type conversion exists between the parameter in the mocked method
// and the corresponding parameter in the callback lambda expression.
//
// - `conversion.Exists` checks if there is any type conversion possible between the two types
//
// The second part ensures that the conversion is either:
// 1. an implicit conversion,
// 2. an identity conversion (meaning the types are exactly the same), or
// 3. an explicit conversion.
//
// If the conversion exists, and it is one of these types (implicit, identity, or explicit), the analyzer will
// skip the diagnostic check, as the callback parameter type is considered compatible with the mocked method's
// parameter type.
//
// There are circumstances where the syntax tree will present an item with an explicit conversion, but the
// ITypeSymbol instance passed in here is reduced to the same type. For example, we have a test that has
// an explicit conversion operator from a string to a custom type. That is presented here as two instances
// of CustomType, which is an implicit identity conversion, not an explicit
Conversion conversion = semanticModel.Compilation.ClassifyConversion(source, destination);

return conversion.Exists && (conversion.IsImplicit || conversion.IsExplicit || conversion.IsIdentity);
}
}
143 changes: 140 additions & 3 deletions tests/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodCodeFixTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
using Xunit.Abstractions;
using Verifier = Moq.Analyzers.Test.Helpers.CodeFixVerifier<Moq.Analyzers.CallbackSignatureShouldMatchMockedMethodAnalyzer, Moq.Analyzers.CallbackSignatureShouldMatchMockedMethodCodeFix>;

namespace Moq.Analyzers.Test;

#pragma warning disable SA1204 // Static members should appear before non-static members

public class CallbackSignatureShouldMatchMockedMethodCodeFixTests
{
private readonly ITestOutputHelper _output;

public CallbackSignatureShouldMatchMockedMethodCodeFixTests(ITestOutputHelper output)
{
_output = output;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "MA0051:Method is too long", Justification = "Contains test data")]
public static IEnumerable<object[]> TestData()
{
Expand All @@ -22,7 +32,7 @@ public static IEnumerable<object[]> TestData()
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<List<string>>())).Returns((List<string> l) => { return 0; });""",
],
[
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<string>())).Callback({|Moq1100:(int i)|} => { });""",
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<string>())).Callback(({|Moq1100:int i|}) => { });""",
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<string>())).Callback((string s) => { });""",
],
[
Expand All @@ -34,7 +44,7 @@ public static IEnumerable<object[]> TestData()
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<int>(), It.IsAny<string>(), It.IsAny<DateTime>())).Callback((int i, string s, DateTime dt) => { });""",
],
[
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<List<string>>())).Callback({|Moq1100:(int i)|} => { });""",
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<List<string>>())).Callback(({|Moq1100:int i|}) => { });""",
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<List<string>>())).Callback((List<string> l) => { });""",
],
[
Expand Down Expand Up @@ -73,6 +83,30 @@ public static IEnumerable<object[]> TestData()
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<List<string>>())).Returns(0).Callback((List<string> l) => { });""",
"""new Mock<IFoo>().Setup(x => x.Do(It.IsAny<List<string>>())).Returns(0).Callback((List<string> l) => { });""",
],
[ // Repros for https://github.com/rjmurillo/moq.analyzers/issues/172
"""new Mock<IFoo>().Setup(m => m.Do(It.IsAny<object?>())).Returns((object? bar) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do(It.IsAny<object?>())).Returns((object? bar) => true);""",
],
[
"""new Mock<IFoo>().Setup(m => m.Do(42)).Returns((long bar) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do(42)).Returns((long bar) => true);""",
],
[
"""new Mock<IFoo>().Setup(m => m.Do((long)42)).Returns((long bar) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do((long)42)).Returns((long bar) => true);""",
],
[
"""new Mock<IFoo>().Setup(m => m.Do((long)42)).Returns((long bar) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do((long)42)).Returns((long bar) => true);""",
],
[
"""new Mock<IFoo>().Setup(m => m.Do(42)).Returns((object? bar) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do(42)).Returns((object? bar) => true);""",
],
[ // This was also reported as part of 172, but is a different error
"""new Mock<IFoo>().Setup(m => m.Do(42)).Returns((int bar) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do(42)).Returns((int bar) => true);""",
],
}.WithNamespaces().WithMoqReferenceAssemblyGroups();
}

Expand All @@ -91,6 +125,10 @@ internal interface IFoo
int Do(int i, string s, DateTime dt);

int Do(List<string> l);

bool Do(object? bar);

bool Do(long bar);
}

internal class UnitTest
Expand All @@ -102,6 +140,105 @@ private void Test()
}
""";

await Verifier.VerifyCodeFixAsync(Template(@namespace, original), Template(@namespace, quickFix), referenceAssemblyGroup);
string o = Template(@namespace, original);
string f = Template(@namespace, quickFix);

_output.WriteLine("Original:");
_output.WriteLine(o);
_output.WriteLine(string.Empty);
_output.WriteLine("Fixed:");
_output.WriteLine(f);

await Verifier.VerifyCodeFixAsync(o, f, referenceAssemblyGroup);
}

public static IEnumerable<object[]> ConversionTestData()
{
return new object[][]
{
[ // This should be allowed because of the implicit conversion from int to CustomType
"""new Mock<IFoo>().Setup(x => x.Do(42)).Returns((CustomType i) => true);""",
"""new Mock<IFoo>().Setup(x => x.Do(42)).Returns((CustomType i) => true);""",
],
[ // This should be allowed because of identity
"""new Mock<IFoo>().Setup(x => x.Do(new CustomType(42))).Returns((CustomType i) => true);""",
"""new Mock<IFoo>().Setup(x => x.Do(new CustomType(42))).Returns((CustomType i) => true);""",
],
[ // This should be allowed because of the explicit conversion from string to CustomType
"""new Mock<IFoo>().Setup(x => x.Do((CustomType)"42")).Returns((CustomType i) => true);""",
"""new Mock<IFoo>().Setup(x => x.Do((CustomType)"42")).Returns((CustomType i) => true);""",
],
[ // This should be allowed because of numeric conversion (explicit)
"""new Mock<IFoo>().Setup(x => x.Do((int)42L)).Returns((CustomType i) => true);""",
"""new Mock<IFoo>().Setup(x => x.Do((int)42L)).Returns((CustomType i) => true);""",
],
[ // This should be allowed because of numeric conversion (explicit)
"""new Mock<IFoo>().Setup(x => x.Do((int)42.0)).Returns((CustomType i) => true);""",
"""new Mock<IFoo>().Setup(x => x.Do((int)42.0)).Returns((CustomType i) => true);""",
],
[
"""new Mock<IFoo>().Setup(m => m.Do(It.IsAny<int>())).Returns((CustomType i) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do(It.IsAny<int>())).Returns((CustomType i) => true);""",
],
[
"""new Mock<IFoo>().Setup(m => m.Do(It.IsAny<CustomType>())).Returns((CustomType i) => true);""",
"""new Mock<IFoo>().Setup(m => m.Do(It.IsAny<CustomType>())).Returns((CustomType i) => true);""",
],
}.WithNamespaces().WithMoqReferenceAssemblyGroups();
}

[Theory]
[MemberData(nameof(ConversionTestData))]
public async Task ConversionTests(string referenceAssemblyGroup, string @namespace, string original, string quickFix)
{
static string Template(string ns, string mock) =>
$$"""
{{ns}}

internal interface IFoo
{
bool Do(CustomType custom);
}

public class CustomType
{
public int Value { get; }

public CustomType(int value)
{
Value = value;
}

// User-defined conversions
public static implicit operator CustomType(int value)
{
return new CustomType(value);
}

public static explicit operator CustomType(string str)
{
return new CustomType(int.Parse(str));
}
}

internal class UnitTest
{
private void Test()
{
{{mock}}
}
}
""";

string o = Template(@namespace, original);
string f = Template(@namespace, quickFix);

_output.WriteLine("Original:");
_output.WriteLine(o);
_output.WriteLine(string.Empty);
_output.WriteLine("Fixed:");
_output.WriteLine(f);

await Verifier.VerifyCodeFixAsync(o, f, referenceAssemblyGroup);
}
}

0 comments on commit 993844f

Please sign in to comment.