Simplify event subscription and remove a remaining inconsistency

CHANGELOG.md

@@ -14,7 +14,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 
 #### Changed
 
-* Event accessor calls (`+=` and `-=`) now get consistently recorded in `Mock.Invocations`. This previously wasn't the case for backwards compatibility with `VerifyNoOtherCalls` (which got implemented before it was possible to check them using `Verify{Add,Remove}`). You now need to explicitly verify expected calls to event accessors prior to `VerifyNoOtherCalls`. Verification of `+=` and `-=` now works regardless of whether or not you set those up (which makes it consistent with how verification usually works). (@80O, @stakx, #1058)
+* Event accessor calls (`+=` and `-=`) now get consistently recorded in `Mock.Invocations`. This previously wasn't the case for backwards compatibility with `VerifyNoOtherCalls` (which got implemented before it was possible to check them using `Verify{Add,Remove}`). You now need to explicitly verify expected calls to event accessors prior to `VerifyNoOtherCalls`. Verification of `+=` and `-=` now works regardless of whether or not you set those up (which makes it consistent with how verification usually works). (@80O, @stakx, #1058, #1084)
 
 #### Fixed
 

src/Moq/Interception/InterceptionAspects.cs

@@ -132,21 +132,16 @@ public static bool Handle(Invocation invocation, Mock mock)
 					var @event = implementingMethod.DeclaringType.GetEvents(bindingFlags).SingleOrDefault(e => e.GetAddMethod(true) == implementingMethod);
 					if (@event != null)
 					{
-						bool doesntHaveEventSetup = !mock.MutableSetups.HasEventSetup;
-
 						if (mock.CallBase && !invocation.Method.IsAbstract)
 						{
-							if (doesntHaveEventSetup)
-							{
-								invocation.ReturnValue = invocation.CallBase();
-							}
+							invocation.ReturnValue = invocation.CallBase();
+							return true;
 						}
 						else if (invocation.Arguments.Length > 0 && invocation.Arguments[0] is Delegate delegateInstance)
 						{
 							mock.EventHandlers.Add(@event, delegateInstance);
+							return true;
 						}
-
-						return doesntHaveEventSetup;
 					}
 				}
 				else if (methodName[0] == 'r' && methodName.Length > 7 && methodName[6] == '_' && invocation.Method.IsEventRemoveAccessor())
@@ -155,21 +150,16 @@ public static bool Handle(Invocation invocation, Mock mock)
 					var @event = implementingMethod.DeclaringType.GetEvents(bindingFlags).SingleOrDefault(e => e.GetRemoveMethod(true) == implementingMethod);
 					if (@event != null)
 					{
-						bool doesntHaveEventSetup = !mock.MutableSetups.HasEventSetup;
-
 						if (mock.CallBase && !invocation.Method.IsAbstract)
 						{
-							if (doesntHaveEventSetup)
-							{
-								invocation.ReturnValue = invocation.CallBase();
-							}
+							invocation.ReturnValue = invocation.CallBase();
+							return true;
 						}
 						else if (invocation.Arguments.Length > 0 && invocation.Arguments[0] is Delegate delegateInstance)
 						{
 							mock.EventHandlers.Remove(@event, delegateInstance);
+							return true;
 						}
-
-						return doesntHaveEventSetup;
 					}
 				}
 			}

src/Moq/Interception/Mock.cs

@@ -14,11 +14,6 @@ void IInterceptor.Intercept(Invocation invocation)
 
 			RecordInvocation.Handle(invocation, this);
 
-			if (HandleEventSubscription.Handle(invocation, this))
-			{
-				return;
-			}
-
 			if (FindAndExecuteMatchingSetup.Handle(invocation, this))
 			{
 				return;
@@ -29,6 +24,11 @@ void IInterceptor.Intercept(Invocation invocation)
 				return;
 			}
 
+			if (HandleEventSubscription.Handle(invocation, this))
+			{
+				return;
+			}
+
 			FailForStrictMock.Handle(invocation, this);
 
 			Return.Handle(invocation, this);

src/Moq/MethodCall.cs

@@ -105,6 +105,10 @@ protected override void ExecuteCore(Invocation invocation)
 					new ReturnBaseOrDefaultValue(this.Mock).Execute(invocation);
 				}
 			}
+			else
+			{
+				HandleEventSubscription.Handle(invocation, this.Mock);  // no-op for everything other than event accessors
+			}
 
 			this.afterReturnCallback?.Execute(invocation);
 		}

src/Moq/SetupCollection.cs

@@ -11,12 +11,10 @@ namespace Moq
 	internal sealed class SetupCollection : ISetupList
 	{
 		private List<Setup> setups;
-		private volatile bool hasEventSetup;
 
 		public SetupCollection()
 		{
 			this.setups = new List<Setup>();
-			this.hasEventSetup = false;
 		}
 
 		public int Count
@@ -30,14 +28,6 @@ public int Count
 			}
 		}
 
-		public bool HasEventSetup
-		{
-			get
-			{
-				return this.hasEventSetup;
-			}
-		}
-
 		public ISetup this[int index]
 		{
 			get
@@ -53,11 +43,6 @@ public void Add(Setup setup)
 		{
 			lock (this.setups)
 			{
-				if (setup.Method.IsEventAddAccessor() || setup.Method.IsEventRemoveAccessor())
-				{
-					this.hasEventSetup = true;
-				}
-
 				this.setups.Add(setup);
 
 				this.MarkOverriddenSetups();
@@ -116,7 +101,6 @@ public void Clear()
 			lock (this.setups)
 			{
 				this.setups.Clear();
-				this.hasEventSetup = false;
 			}
 		}
 

tests/Moq.Tests/EventHandlersFixture.cs

@@ -82,15 +82,15 @@ public void Raising_event__using_Raise__triggers_handler__if_setup_without_CallB
 			Assert.True(handled);
 		}
 
-		[Fact]  // this is inconsistent with the above test `Raising_event__using_Raise__does_not_trigger_handler__if_CallBase_true`
-		public void Raising_event__using_Raise__triggers_handler__if_setup_with_CallBase_present()
+		[Fact]
+		public void Raising_event__using_Raise__does_not_trigger_handler__if_setup_with_CallBase_present()
 		{
 			var handled = false;
 			var mock = new Mock<HasEvent>();
 			mock.SetupAdd(m => m.Event += It.IsAny<Action>()).CallBase();
 			mock.Object.Event += () => handled = true;
 			mock.Raise(m => m.Event += null);
-			Assert.True(handled);
+			Assert.False(handled);
 		}
 
 		[Fact]

tests/Moq.Tests/MockFixture.cs

@@ -1279,20 +1279,6 @@ public void Reset_clears_configured_default_return_values()
 			Assert.Empty(mock.ConfiguredDefaultValues);
 		}
 
-		[Fact]
-		public void Reset_clears_event_setup_flag()
-		{
-			var mock = new Mock<IFoo>();
-			mock.SetupAdd(m => m.EventHandler += It.IsAny<EventHandler>());
-
-			var before = mock.MutableSetups.HasEventSetup;
-			mock.Reset();
-			var after = mock.MutableSetups.HasEventSetup;
-
-			Assert.True(before, "Before reset");
-			Assert.False(after, "After reset");
-		}
-
 #if FEATURE_DYNAMICPROXY_SERIALIZABLE_PROXIES
 		[Serializable]
 		public class BadSerializable : ISerializable