Remove Interceptor.calls, ExpressionKey, and SetupKind
#509
Conversation
The `calls` dictionary in `Interceptor` was used, up until now, to de- tect equivalent unconditional setups such that for any two such set- ups, the one added later would replace the former. Unfortunately, maintaining `calls` comes at a cost: * There are two data structures keeping track of registered setups: `calls` (a partial view), and `InterceptorContext.OrderedCalls` (the full view). It's often difficult to remember which one is used for what purpose(s). * For every setup being registered, the setup expression is converted into a lookup key. This involves formatting the expression tree to a string representaion, which is fairly costly. It turns out that if we just use `InterceptorContext.OrderedCalls` in- stead of `calls`, and if we always insert new setups at the *front* of that collection, then it doesn't matter much if it contains duplicate setups; when finding a matching setup for an invocation, the setup first encountered will win over those at a later (earlier) position. This means that the invocation->setup matching operation becomes more efficient, too. One thing however becomes less efficient: mass verification triggered via `mock.Verify[All]`. While we no longer care about duplicate setups during setup, we still need to ensure that verification does not in- clude duplicate setups, so the lookup key generation is still needed here.
`ExpressionKey` was used, up until now, as a representation for an expression tree so that those could be compared quickly for equality. Such comparisons are needed to detect duplicate / overridden setups. Turns out that just comparing expression trees directly, using `ExpressionComparer` (which is already in Moq's codebase) instead of first turning them into strings, is actually approx. 1.33x faster. This commit therefore removes `ExpressionKey` in favor of `Expression- Comparer`.
|
|
||
| namespace Moq.Matchers | ||
| namespace Moq |
There was a problem hiding this comment.
We no longer use ExpressionComparer just together with matchers, so it's moved into the main namespace.
| @@ -142,7 +138,7 @@ internal IEnumerable<IProxyCall> OrderedCalls | |||
| { | |||
| lock (orderedCalls) | |||
| { | |||
| return orderedCalls.ToList(); | |||
| return orderedCalls.ToArray(); | |||
There was a problem hiding this comment.
Micro-optimization. Since callers are only going to read the returned collection, choose a type with as little overhead as possible.
| { | ||
| throw new MockVerificationException(failures.ToArray()); | ||
| continue; |
There was a problem hiding this comment.
Conditional setups were already excluded from Mock.Verify[All] prior to this PR, but that wasn't as easy to see as it is now (because previously, conditional setups only got added to OrderedCalls, but not to calls, and mass verification happened against the latter).
| { | ||
| verifiedSetupsForMethod = new List<Expression>(); | ||
| verifiedSetupsPerMethod.Add(setup.Method, verifiedSetupsForMethod); | ||
| } |
There was a problem hiding this comment.
There's another occurrence of this code pattern somewhere else in Moq's codebase, perhaps consider creating some kind of MultiMap / MultiDictionary ADT so this can be abstracted away.
| @@ -69,22 +69,26 @@ public InterceptionAction HandleIntercept(ICallContext invocation, InterceptorCo | |||
| return InterceptionAction.Continue; | |||
| } | |||
|
|
|||
| IProxyCall lastMatchingSetup = null; | |||
| IProxyCall lastMatchingSetupForSameMethod = null; | |||
| IProxyCall matchedSetup = null; | |||
| foreach (var setup in ctx.OrderedCalls) | |||
| { | |||
There was a problem hiding this comment.
Since InterceptorContext.OrderedCalls, now being a Stack<IProxyCall> instead of a List<IProxyCall>, is now in reverse order (last setup added gets enumerated first), we no longer search for the last matching setup, but for the first. This makes it possible to break out of the loop earlier.
| @@ -406,6 +406,7 @@ private static bool AreSameMethod(Expression left, Expression right) | |||
| private static string FormatSetupsInfo(IEnumerable<IProxyCall> setups) | |||
| { | |||
| var expressionSetups = setups | |||
| .Reverse() | |||
There was a problem hiding this comment.
This is needed for continuity (so setups are shown in exception messages in the order in which they were registered). Minor performance penalty here, but since it only happens in the error case, that's OK.
|
|
||
| public MockVerificationException(IProxyCall[] failedSetups) | ||
| public MockVerificationException(IEnumerable<IProxyCall> failedSetups) |
There was a problem hiding this comment.
This change is to avoid an unnecessary call to .ToArray() in the calling code.
All of these were used so far to detect when one setup overrides an earlier, equivalent setup. This PR proposes their removal.
Instead of performing the checks against
Interceptor.calls, they can run directly againstInterceptorContext.OrderedCalls(of which the former is a partial view).Instead of comparing setups via their string representation (built for
ExpressionKey), they can be compared by directly comparing their expression trees using the already existingExpressionComparer.SetupKindbecomes redundant after (2).