Make event accessor recognition logic more correct

Up until now, if Moq encounters a method named `add_X` or `remove_X` it will assume that it's found an specialname event accessor method without performing any further checks. This commits adds some trivial type checks and null reference checks to improve that logic somewhat. I hesitate to make this too perfect since that just might break things for F# or COM (see commit 44070a9).
devlooped · Jun 16, 2017 · 63750bb · 63750bb
1 parent 8299edc
commit 63750bb
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 27 deletions.
diff --git a/Source/InterceptorStrategies.cs b/Source/InterceptorStrategies.cs
@@ -34,7 +34,7 @@ internal class InvokeBase : IInterceptStrategy
 		public InterceptionAction HandleIntercept(ICallContext invocation, InterceptorContext ctx, CurrentInterceptContext localctx)
 		{
 			if (invocation.Method.DeclaringType == typeof(object) || // interface proxy
-				ctx.Mock.ImplementedInterfaces.Contains(invocation.Method.DeclaringType) && !invocation.Method.IsEventAttach() && !invocation.Method.IsEventDetach() && ctx.Mock.CallBase && !ctx.Mock.MockedType.GetTypeInfo().IsInterface || // class proxy with explicitly implemented interfaces. The method's declaring type is the interface and the method couldn't be abstract
+				ctx.Mock.ImplementedInterfaces.Contains(invocation.Method.DeclaringType) && !invocation.Method.LooksLikeEventAttach() && !invocation.Method.LooksLikeEventDetach() && ctx.Mock.CallBase && !ctx.Mock.MockedType.GetTypeInfo().IsInterface || // class proxy with explicitly implemented interfaces. The method's declaring type is the interface and the method couldn't be abstract
 				invocation.Method.DeclaringType.GetTypeInfo().IsClass && !invocation.Method.IsAbstract && ctx.Mock.CallBase // class proxy
 				)
 			{
@@ -263,39 +263,51 @@ public InterceptionAction HandleIntercept(ICallContext invocation, InterceptorCo
 			if (!FluentMockContext.IsActive)
 			{
 				//Special case for events
-				if (invocation.Method.IsEventAttach())
+				if (invocation.Method.LooksLikeEventAttach())
 				{
-					var delegateInstance = (Delegate)invocation.Arguments[0];
-					// TODO: validate we can get the event?
-					var eventInfo = this.GetEventFromName(invocation.Method.Name.Substring(4));
-
-					if (ctx.Mock.CallBase && !eventInfo.DeclaringType.GetTypeInfo().IsInterface)
-					{
-						invocation.InvokeBase();
-					}
-					else if (delegateInstance != null)
+					var eventInfo = this.GetEventFromName(invocation.Method.Name.Substring("add_".Length));
+					if (eventInfo != null)
 					{
-						ctx.AddEventHandler(eventInfo, (Delegate)invocation.Arguments[0]);
+						// TODO: We could compare `invocation.Method` and `eventInfo.GetAddMethod()` here.
+						// If they are equal, then `invocation.Method` is definitely an event `add` accessor.
+						// Not sure whether this would work with F# and COM; see commit 44070a9.
+						if (ctx.Mock.CallBase && !eventInfo.DeclaringType.GetTypeInfo().IsInterface)
+						{
+							invocation.InvokeBase();
+							return InterceptionAction.Stop;
+						}
+						else if (invocation.Arguments.Length > 0 && invocation.Arguments[0] is Delegate delegateInstance)
+						{
+							ctx.AddEventHandler(eventInfo, delegateInstance);
+							return InterceptionAction.Stop;
+						}
 					}
 
-					return InterceptionAction.Stop;
+					// wasn't an event attach accessor after all
+					return InterceptionAction.Continue;
 				}
-				else if (invocation.Method.IsEventDetach())
+				else if (invocation.Method.LooksLikeEventDetach())
 				{
-					var delegateInstance = (Delegate)invocation.Arguments[0];
-					// TODO: validate we can get the event?
-					var eventInfo = this.GetEventFromName(invocation.Method.Name.Substring(7));
-
-					if (ctx.Mock.CallBase && !eventInfo.DeclaringType.GetTypeInfo().IsInterface)
-					{
-						invocation.InvokeBase();
-					}
-					else if (delegateInstance != null)
+					var eventInfo = this.GetEventFromName(invocation.Method.Name.Substring("remove_".Length));
+					if (eventInfo != null)
 					{
-						ctx.RemoveEventHandler(eventInfo, (Delegate)invocation.Arguments[0]);
+						// TODO: We could compare `invocation.Method` and `eventInfo.GetRemoveMethod()` here.
+						// If they are equal, then `invocation.Method` is definitely an event `remove` accessor.
+						// Not sure whether this would work with F# and COM; see commit 44070a9.
+						if (ctx.Mock.CallBase && !eventInfo.DeclaringType.GetTypeInfo().IsInterface)
+						{
+							invocation.InvokeBase();
+							return InterceptionAction.Stop;
+						}
+						else if (invocation.Arguments.Length > 0 && invocation.Arguments[0] is Delegate delegateInstance)
+						{
+							ctx.RemoveEventHandler(eventInfo, delegateInstance);
+							return InterceptionAction.Stop;
+						}
 					}
 
-					return InterceptionAction.Stop;
+					// wasn't an event detach accessor after all
+					return InterceptionAction.Continue;
 				}
 
 				// Save to support Verify[expression] pattern.

diff --git a/Source/MemberInfoExtensions.cs b/Source/MemberInfoExtensions.cs
@@ -61,12 +61,12 @@ public static bool IsDestructor(this MethodInfo method)
 		}
 
 
-		public static bool IsEventAttach(this MethodBase method)
+		public static bool LooksLikeEventAttach(this MethodBase method)
 		{
 			return method.Name.StartsWith("add_", StringComparison.Ordinal);
 		}
 
-		public static bool IsEventDetach(this MethodBase method)
+		public static bool LooksLikeEventDetach(this MethodBase method)
 		{
 			return method.Name.StartsWith("remove_", StringComparison.Ordinal);
 		}

diff --git a/UnitTests/Regressions/IssueReportsFixture.cs b/UnitTests/Regressions/IssueReportsFixture.cs
@@ -170,6 +170,44 @@ public void DoTest()
 #endif
 		#endregion
 
+		#region 82
+
+		public class Issue82
+		{
+			public interface ILogger
+			{
+				event Action info;
+				void add_info(string info);
+				void Add_info(string info);
+				void remove_info(string info);
+				void Remove_info(string info);
+			}
+
+			[Fact]
+			public void MethodWithAddUnderscoreNamePrefixDoesNotGetMisrecognizedAsEventAccessor()
+			{
+				var mock = new Mock<ILogger>();
+				mock.Setup(x => x.add_info(It.IsAny<string>()));
+				mock.Setup(x => x.Add_info(It.IsAny<string>()));
+
+				mock.Object.add_info(string.Empty);
+				mock.Object.Add_info(string.Empty);
+			}
+
+			[Fact]
+			public void MethodWithRemoveUnderscoreNamePrefixDoesNotGetMisrecognizedAsEventAccessor()
+			{
+				var mock = new Mock<ILogger>();
+				mock.Setup(x => x.remove_info(It.IsAny<string>()));
+				mock.Setup(x => x.Remove_info(It.IsAny<string>()));
+
+				mock.Object.remove_info(string.Empty);
+				mock.Object.Remove_info(string.Empty);
+			}
+		}
+
+		#endregion
+
 		#region 163
 
 #if !NETCORE