From eb8ddd4211c8844b24d0ca796e2859ea48b8a52f Mon Sep 17 00:00:00 2001 From: stakx Date: Thu, 29 Aug 2019 21:24:22 +0200 Subject: [PATCH 1/6] Split up composite `if` conditions --- src/Moq/Match.cs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/Moq/Match.cs b/src/Moq/Match.cs index ed1169147..bb8c7ab69 100644 --- a/src/Moq/Match.cs +++ b/src/Moq/Match.cs @@ -108,24 +108,29 @@ internal Match(Predicate condition, Expression> renderExpression, Act internal override bool Matches(object value) { - if (value != null && !(value is T)) + if (value != null) { - return false; + if (!(value is T)) + { + return false; + } } var matchType = typeof(T); - if (value == null && matchType.IsValueType - && (!matchType.IsGenericType || matchType.GetGenericTypeDefinition() != typeof(Nullable<>))) + if (value == null) { - // If this.Condition expects a value type and we've been passed null, - // it can't possibly match. - // This tends to happen when you are trying to match a parameter of type int? - // with IsAny but then pass null into the mock. - // We have to return early from here because you can't cast null to T - // when T is a value type. - // - // See GitHub issue #90: https://github.com/moq/moq4/issues/90 - return false; + if (matchType.IsValueType && (!matchType.IsGenericType || matchType.GetGenericTypeDefinition() != typeof(Nullable<>))) + { + // If this.Condition expects a value type and we've been passed null, + // it can't possibly match. + // This tends to happen when you are trying to match a parameter of type int? + // with IsAny but then pass null into the mock. + // We have to return early from here because you can't cast null to T + // when T is a value type. + // + // See GitHub issue #90: https://github.com/moq/moq4/issues/90 + return false; + } } return this.Condition((T)value); } From 081f8e7ef32fee8dfe879578d0e038562af1b0ac Mon Sep 17 00:00:00 2001 From: stakx Date: Thu, 29 Aug 2019 21:25:00 +0200 Subject: [PATCH 2/6] Turn second `if` into `else` --- src/Moq/Match.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Moq/Match.cs b/src/Moq/Match.cs index bb8c7ab69..e3d3884a4 100644 --- a/src/Moq/Match.cs +++ b/src/Moq/Match.cs @@ -115,10 +115,9 @@ internal override bool Matches(object value) return false; } } - - var matchType = typeof(T); - if (value == null) + else { + var matchType = typeof(T); if (matchType.IsValueType && (!matchType.IsGenericType || matchType.GetGenericTypeDefinition() != typeof(Nullable<>))) { // If this.Condition expects a value type and we've been passed null, From 95465bd8235f51d114176b5d2c5668e4107d0c8a Mon Sep 17 00:00:00 2001 From: stakx Date: Thu, 29 Aug 2019 21:33:25 +0200 Subject: [PATCH 3/6] Convert early `return` into boolean --- src/Moq/Match.cs | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/src/Moq/Match.cs b/src/Moq/Match.cs index e3d3884a4..88d8f376c 100644 --- a/src/Moq/Match.cs +++ b/src/Moq/Match.cs @@ -108,30 +108,19 @@ internal Match(Predicate condition, Expression> renderExpression, Act internal override bool Matches(object value) { + bool canCastValueToT; + if (value != null) { - if (!(value is T)) - { - return false; - } + canCastValueToT = value is T; } else { var matchType = typeof(T); - if (matchType.IsValueType && (!matchType.IsGenericType || matchType.GetGenericTypeDefinition() != typeof(Nullable<>))) - { - // If this.Condition expects a value type and we've been passed null, - // it can't possibly match. - // This tends to happen when you are trying to match a parameter of type int? - // with IsAny but then pass null into the mock. - // We have to return early from here because you can't cast null to T - // when T is a value type. - // - // See GitHub issue #90: https://github.com/moq/moq4/issues/90 - return false; - } + canCastValueToT = !matchType.IsValueType || (matchType.IsGenericType && matchType.GetGenericTypeDefinition() == typeof(Nullable<>)); } - return this.Condition((T)value); + + return canCastValueToT && this.Condition((T)value); } internal override void SetupEvaluatedSuccessfully(object value) From ca229eba03bec52016300848f402c40538501889 Mon Sep 17 00:00:00 2001 From: stakx Date: Thu, 29 Aug 2019 21:44:37 +0200 Subject: [PATCH 4/6] Rename variable `matchType` -> `t` for clarity --- src/Moq/Match.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Moq/Match.cs b/src/Moq/Match.cs index 88d8f376c..adf2ffc8a 100644 --- a/src/Moq/Match.cs +++ b/src/Moq/Match.cs @@ -116,8 +116,8 @@ internal override bool Matches(object value) } else { - var matchType = typeof(T); - canCastValueToT = !matchType.IsValueType || (matchType.IsGenericType && matchType.GetGenericTypeDefinition() == typeof(Nullable<>)); + var t = typeof(T); + canCastValueToT = !t.IsValueType || (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(Nullable<>)); } return canCastValueToT && this.Condition((T)value); From 3fbed15304d553eca6e2988b4cdf985a583c550a Mon Sep 17 00:00:00 2001 From: stakx Date: Thu, 29 Aug 2019 22:09:00 +0200 Subject: [PATCH 5/6] Remove redundant `is` check in `It` ... because `Match` matchers will already have checked `value is T` by the time the predicate delegate gets invoked. --- src/Moq/It.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Moq/It.cs b/src/Moq/It.cs index f9e6fa6dd..3993e1d4d 100644 --- a/src/Moq/It.cs +++ b/src/Moq/It.cs @@ -50,8 +50,7 @@ public static TValue IsAny() { return Match.Create( #if FEATURE_COM - value => value == null || (typeof(TValue).IsAssignableFrom(value.GetType()) - || (IsComObject(value) && value is TValue)), + value => value == null || (typeof(TValue).IsAssignableFrom(value.GetType()) || IsComObject(value)), #else value => value == null || typeof(TValue).IsAssignableFrom(value.GetType()), #endif @@ -73,8 +72,7 @@ public static TValue IsNotNull() { return Match.Create( #if FEATURE_COM - value => value != null && (typeof(TValue).IsAssignableFrom(value.GetType()) - || (IsComObject(value) && value is TValue)), + value => value != null && (typeof(TValue).IsAssignableFrom(value.GetType()) || IsComObject(value)), #else value => value != null && typeof(TValue).IsAssignableFrom(value.GetType()), #endif From 950870cf23cbdc2b2e912123dd63f78944be8419 Mon Sep 17 00:00:00 2001 From: stakx Date: Thu, 29 Aug 2019 22:58:10 +0200 Subject: [PATCH 6/6] Simplify `It.IsAny`, `It.IsNotNull` matchers by documenting that `IsAssignableFrom` generally yields the same results as the `is` operator when run on an `object`-typed variable. Note that this is removing the `IsComObject` check as well. This check was originally added because it was found that `IsAssignableFrom` does not always work properly for COM interfaces whereas the `is` works. Since we're generally going back on `is`, the COM check becomes redun- dant. --- src/Moq/It.cs | 15 +- .../IsAssignableFromVsIsOperatorFixture.cs | 163 ++++++++++++++++++ 2 files changed, 165 insertions(+), 13 deletions(-) create mode 100644 tests/Moq.Tests/Matchers/IsAssignableFromVsIsOperatorFixture.cs diff --git a/src/Moq/It.cs b/src/Moq/It.cs index 3993e1d4d..3fcc738fc 100644 --- a/src/Moq/It.cs +++ b/src/Moq/It.cs @@ -8,9 +8,6 @@ using System.Text.RegularExpressions; using System.Diagnostics.CodeAnalysis; using System.Reflection; -#if FEATURE_COM -using static System.Runtime.InteropServices.Marshal; -#endif using Moq.Protected; @@ -49,11 +46,7 @@ public static class Ref public static TValue IsAny() { return Match.Create( -#if FEATURE_COM - value => value == null || (typeof(TValue).IsAssignableFrom(value.GetType()) || IsComObject(value)), -#else - value => value == null || typeof(TValue).IsAssignableFrom(value.GetType()), -#endif + value => value is TValue || value == null, () => It.IsAny()); } @@ -71,11 +64,7 @@ internal static MethodCallExpression IsAny(Type genericArgument) public static TValue IsNotNull() { return Match.Create( -#if FEATURE_COM - value => value != null && (typeof(TValue).IsAssignableFrom(value.GetType()) || IsComObject(value)), -#else - value => value != null && typeof(TValue).IsAssignableFrom(value.GetType()), -#endif + value => value is TValue, () => It.IsNotNull()); } diff --git a/tests/Moq.Tests/Matchers/IsAssignableFromVsIsOperatorFixture.cs b/tests/Moq.Tests/Matchers/IsAssignableFromVsIsOperatorFixture.cs new file mode 100644 index 000000000..9fae4f318 --- /dev/null +++ b/tests/Moq.Tests/Matchers/IsAssignableFromVsIsOperatorFixture.cs @@ -0,0 +1,163 @@ +// Copyright (c) 2007, Clarius Consulting, Manas Technology Solutions, InSTEDD. +// All rights reserved. Licensed under the BSD 3-Clause License; see License.txt. + +using System; +using System.Collections.Generic; + +using Xunit; + +namespace Moq.Tests.Matchers +{ + // The purpose behind these tests is to find cases where `value is T` and + // `typeof(T).IsAssignableFrom(value.GetType())` disagree (i.e. yield different + // results). This is to check the validity of simplifying the implementation + // of `It.IsAny`, which used to run both checks (except when `value` == null; + // see commit history). If both expressions generally yield the same results + // (and it looks like that's the case) we can simplify and just run `value is T`. + public class IsAssignableFromVsIsOperatorFixture + { + [Fact] + public void Nullable_value_type_is_value_type() + { + int? value = 42; + Assert.True(value is int); + Assert.True(typeof(int).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_is_nullable_value_type() + { + int value = 42; + Assert.True(value is int?); + Assert.True(typeof(int?).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_is_object() + { + int value = 42; + Assert.True(value is object); + Assert.True(typeof(object).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_is_not_wider_value_type() + { + int value = 42; + Assert.False(value is long); + Assert.False(typeof(long).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_is_not_narrower_value_type() + { + long value = 42L; + Assert.False(value is int); + Assert.False(typeof(int).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_is_not_value_type_of_opposite_signedness() + { + int value = 42; + Assert.False(value is uint); + Assert.False(typeof(uint).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_as_object_is_not_wider_value_type() + { + int value = 42; + Assert.False((object)value is long); + Assert.False(typeof(long).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_array_is_not_nullable_value_type_array() + { + int[] value = new[] { 42 }; + Assert.False(value is int?[]); + Assert.False(typeof(int?[]).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_array_as_object_is_not_nullable_value_type_array() + { + int[] value = new[] { 42 }; + Assert.False((object)value is int?[]); + Assert.False(typeof(int?[]).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_array_is_not_object_array() + { + int[] value = new[] { 42 }; + Assert.False(value is object[]); + Assert.False(typeof(object[]).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_array_is_wider_value_type_array_but_is_operator_says_no() + { + int[] value = new[] { 42 }; + Assert.False(value is uint[]); + Assert.True(typeof(uint[]).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_array_is_wider_value_type_array_and_is_operator_agrees_when_value_as_object() + { + int[] value = new[] { 42 }; + Assert.True((object)value is uint[]); + Assert.True(typeof(uint[]).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Value_type_is_interface() + { + int value = 42; + Assert.True(value is IComparable); + Assert.True(typeof(IComparable).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Reference_type_is_object() + { + string value = "42"; + Assert.True(value is object); + Assert.True(typeof(object).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Reference_type_is_interface() + { + string value = "42"; + Assert.True(value is IComparable); + Assert.True(typeof(IComparable).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Reference_type_is_super_class() + { + ArgumentNullException value = new ArgumentNullException(); + Assert.True(value is Exception); + Assert.True(typeof(Exception).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Generic_type_is_not_more_general_generic_type() + { + IEnumerable value = new List(); + Assert.False(value is IEnumerable); + Assert.False(typeof(IEnumerable).IsAssignableFrom(value.GetType())); + } + + [Fact] + public void Generic_type_is_not_generic_type_definition() + { + List value = new List(); + //Assert.True(value is List<>); + Assert.False(typeof(List<>).IsAssignableFrom(value.GetType())); + } + } +}