Skip to content

Commit

Permalink
Merge pull request #949 from stakx/issue-932
Browse files Browse the repository at this point in the history
Let `InnerMockSetup` (i.e. internal setups for cached return values)
match generic args exactly instead of by assignment compatibility.
  • Loading branch information
stakx authored Oct 19, 2019
2 parents 7415f68 + dfbd9cc commit 9ff1cd7
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 56 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1

#### Fixed

* Regression: `InvalidCastException` caused by Moq erroneously reusing a cached auto-mocked (`DefaultValue.Mock`) return value for a different generic method instantiation (@BrunoJuchli, #932)
* AmbiguousMatchException when setting up the property, that hides another one (@ishatalkin, #939)
* `ArgumentException` ("Interface not found") when setting up `object.ToString` on an interface mock (@vslynko, #942)
* Cannot "return" to original mocked type after downcasting with `Mock.Get` and then upcasting with `mock.As<>` (@pjquirk, #943)
Expand Down
64 changes: 24 additions & 40 deletions src/Moq/Extensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ public static IEnumerable<MethodInfo> GetMethods(this Type type, string name)
return type.GetMember(name).OfType<MethodInfo>();
}

public static bool CompareTo<TTypes, TOtherTypes>(this TTypes types, TOtherTypes otherTypes, TypeComparison comparisonType)
public static bool CompareTo<TTypes, TOtherTypes>(this TTypes types, TOtherTypes otherTypes, bool exact, bool considerTypeMatchers)
where TTypes : IReadOnlyList<Type>
where TOtherTypes : IReadOnlyList<Type>
{
Expand All @@ -214,51 +214,35 @@ public static bool CompareTo<TTypes, TOtherTypes>(this TTypes types, TOtherTypes
return false;
}

switch (comparisonType)
for (int i = 0; i < count; ++i)
{
case TypeComparison.Equality:
for (int i = 0; i < count; ++i)
if (considerTypeMatchers && types[i].IsTypeMatcher(out var typeMatcherType))
{
Debug.Assert(typeMatcherType.ImplementsTypeMatcherProtocol());

var typeMatcher = (ITypeMatcher)Activator.CreateInstance(typeMatcherType);
if (typeMatcher.Matches(otherTypes[i]) == false)
{
if (types[i] != otherTypes[i])
{
return false;
}
return false;
}
return true;

case TypeComparison.AssignmentCompatibility:
for (int i = 0; i < count; ++i)
}
else if (exact)
{
if (types[i] != otherTypes[i])
{
if (types[i].IsAssignableFrom(otherTypes[i]) == false)
{
return false;
}
return false;
}
return true;

case TypeComparison.TypeMatchersOrElseAssignmentCompatibility:
for (int i = 0; i < count; ++i)
}
else
{
if (types[i].IsAssignableFrom(otherTypes[i]) == false)
{
if (types[i].IsTypeMatcher(out var typeMatcherType))
{
Debug.Assert(typeMatcherType.ImplementsTypeMatcherProtocol());

var typeMatcher = (ITypeMatcher)Activator.CreateInstance(typeMatcherType);
if (typeMatcher.Matches(otherTypes[i]) == false)
{
return false;
}
}
else if (types[i].IsAssignableFrom(otherTypes[i]) == false)
{
return false;
}
return false;
}
return true;

default:
throw new ArgumentOutOfRangeException(nameof(comparisonType));
}
}

return true;
}

public static string GetParameterTypeList(this MethodInfo method)
Expand All @@ -275,7 +259,7 @@ public static bool CompareParameterTypesTo<TOtherTypes>(this Delegate function,
where TOtherTypes : IReadOnlyList<Type>
{
var method = function.GetMethodInfo();
if (method.GetParameterTypes().CompareTo(otherTypes, TypeComparison.AssignmentCompatibility))
if (method.GetParameterTypes().CompareTo(otherTypes, exact: false, considerTypeMatchers: false))
{
// the backing method for the literal delegate is compatible, DynamicInvoke(...) will succeed
return true;
Expand All @@ -286,7 +270,7 @@ public static bool CompareParameterTypesTo<TOtherTypes>(this Delegate function,
// an instance delegate invocation is created for an extension method (bundled with a receiver)
// or at times for DLR code generation paths because the CLR is optimized for instance methods.
var invokeMethod = GetInvokeMethodFromUntypedDelegateCallback(function);
if (invokeMethod != null && invokeMethod.GetParameterTypes().CompareTo(otherTypes, TypeComparison.AssignmentCompatibility))
if (invokeMethod != null && invokeMethod.GetParameterTypes().CompareTo(otherTypes, exact: false, considerTypeMatchers: false))
{
// the Invoke(...) method is compatible instead. DynamicInvoke(...) will succeed.
return true;
Expand Down
7 changes: 5 additions & 2 deletions src/Moq/InvocationShape.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ internal sealed class InvocationShape : IEquatable<InvocationShape>
#if DEBUG
private Type proxyType;
#endif
private readonly bool exactGenericTypeArguments;

public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnlyList<Expression> arguments = null)
public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnlyList<Expression> arguments = null, bool exactGenericTypeArguments = false)
{
Debug.Assert(expression != null);
Debug.Assert(method != null);
Expand All @@ -54,6 +55,8 @@ public InvocationShape(LambdaExpression expression, MethodInfo method, IReadOnly
this.argumentMatchers = noArgumentMatchers;
this.Arguments = noArguments;
}

this.exactGenericTypeArguments = exactGenericTypeArguments;
}

public void Deconstruct(out LambdaExpression expression, out MethodInfo method, out IReadOnlyList<Expression> arguments)
Expand Down Expand Up @@ -130,7 +133,7 @@ private bool IsOverride(Invocation invocation)

if (method.IsGenericMethod || invocationMethod.IsGenericMethod)
{
if (!method.GetGenericArguments().CompareTo(invocationMethod.GetGenericArguments(), TypeComparison.TypeMatchersOrElseAssignmentCompatibility))
if (!method.GetGenericArguments().CompareTo(invocationMethod.GetGenericArguments(), exact: this.exactGenericTypeArguments, considerTypeMatchers: true))
{
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Moq/Mock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ internal void AddInnerMockSetup(MethodInfo method, IReadOnlyList<Expression> arg
Debug.Assert(method == property.GetGetMethod(true));
}

this.Setups.Add(new InnerMockSetup(new InvocationShape(expression, method, arguments), returnValue));
this.Setups.Add(new InnerMockSetup(new InvocationShape(expression, method, arguments, exactGenericTypeArguments: true), returnValue));
}

#endregion
Expand Down
2 changes: 1 addition & 1 deletion src/Moq/Protected/ProtectedMock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ private static MethodInfo GetMethod(string methodName, bool exact, params object
{
var argTypes = ToArgTypes(args);
return typeof(T).GetMethods(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public)
.SingleOrDefault(m => m.Name == methodName && m.GetParameterTypes().CompareTo(argTypes, exact ? TypeComparison.Equality : TypeComparison.AssignmentCompatibility));
.SingleOrDefault(m => m.Name == methodName && m.GetParameterTypes().CompareTo(argTypes, exact, considerTypeMatchers: false));
}

private static Expression<Func<T, TResult>> GetMethodCall<TResult>(MethodInfo method, object[] args)
Expand Down
12 changes: 0 additions & 12 deletions src/Moq/TypeComparison.cs

This file was deleted.

30 changes: 30 additions & 0 deletions tests/Moq.Tests/Regressions/IssueReportsFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3150,6 +3150,36 @@ private void SetupIntMethod(Action callback)

#endregion

#region 932

public class Issue932
{
[Fact]
public void Should_not_reuse_cached_return_value_that_is_assignment_incompatible()
{
var mock = new Mock<X> { DefaultValue = DefaultValue.Mock };

// The following call will cause Moq to produce and return an auto-mocked instance of `IReadOnlyList<I>`.
// Additionally, an internal setup is added that will keep returning the same instance on subsequent calls:
mock.Object.Method<I>();

// The following call should not trigger the above setup, since we expect to get a `IReadOnlyList<C>`
// and the cached `IReadOnlyList<I>` wouldn't fit:
mock.Object.Method<C>();
}

public interface X
{
IReadOnlyList<T> Method<T>();
}

public interface I { }

public class C : I { }
}

#endregion

#region 942

public class Issue942
Expand Down

0 comments on commit 9ff1cd7

Please sign in to comment.