Add ReturnValue to IInvocation

[MaStr11]

Fixes #920

[stakx]

Thanks for the PR. I've been thinking about doing the same, too, but then didn't because I didn't have a clean solution for the case where an invocation throws an exception (i.e. doesn't return anything). It strikes me as wrong that one should be able to query ReturnValue and get a null value.

Do you have a suggestion for that case?

[MaStr11]

As far as I can see, any exception raised is not yet recorded in the Invocation class. Besides that, the question is how to communicate that an exception was thrown. For the second part, I see several possibilities but don't have a favorite:

Add an Exception property to the interface

	/// <summary>
	/// Provides information about an invocation of a mock object.
	/// </summary>
	public interface IInvocation
	{
		/// <summary>
		/// Gets the method of the invocation.
		/// </summary>
		MethodInfo Method { get; }

		/// <summary>
		/// Gets the arguments of the invocation.
		/// </summary>
		IReadOnlyList<object> Arguments { get; }

		/// <summary>
		/// Gets the return value of the invocation.
		/// </summary>
		object ReturnValue { get; }

		/// <summary>
		/// Gets the exception thrown during the invocation.
		/// </summary>
		Exception Exception { get; }
	}

Use an Either type

e.g. https://github.com/nlkl/Optional#options-with-exceptional-values
or any other of the Either monad implementations out there:

	/// <summary>
	/// Provides information about an invocation of a mock object.
	/// </summary>
	public interface IInvocation
	{
		/// <summary>
		/// Gets the method of the invocation.
		/// </summary>
		MethodInfo Method { get; }

		/// <summary>
		/// Gets the arguments of the invocation.
		/// </summary>
		IReadOnlyList<object> Arguments { get; }

		/// <summary>
		/// Either the return value or the exception thrown during the invocation.
		/// </summary>
		Option<Object, Exception> InvocationResult { get; }
	}

Create a custom type

	/// <summary>
	/// Provides information about an invocation of a mock object.
	/// </summary>
	public interface IInvocation
	{
		/// <summary>
		/// Gets the method of the invocation.
		/// </summary>
		MethodInfo Method { get; }

		/// <summary>
		/// Gets the arguments of the invocation.
		/// </summary>
		IReadOnlyList<object> Arguments { get; }

		/// <summary>
		/// Either the return value or the exception thrown during the invocation.
		/// </summary>
		InvocationResult InvocationResult { get; }
	}

enum InvocationResultType
{
  Void, // for successfull invocations without return type
  ReturnValue,
  Exception,
}

abstract class InvocationResult
{
  public abstract InvocationResultType ResultType { get; }
}

sealed class VoidInvocationResult: InvocationResult
{
  public override InvocationResultType ResultType { get => InvocationResultType.Void }
}

sealed class ReturnValueInvocationResult: InvocationResult
{
  public override InvocationResultType ResultType { get => InvocationResultType.ReturnValue }
  public object ReturnValue { get; }
}

sealed class ExceptionInvocationResult: InvocationResult
{
  public override InvocationResultType ResultType { get => InvocationResultType.Exception }
  public Exception Exception { get; }
}

[MaStr11]

A fourth option would be to keep the interface as is and return either the returned value or the exception when object ReturnValue { get; } is accessed.

[stakx]

Thanks for these four options. FWIW, Moq v5 appears to have gone with a combination of the 1st and 3rd option.

A fifth option I have been considering is this one:

partial interface IInvocation
{
    bool TryGetReturnValue(out object returnValue);
    bool TryGetReturnValue(out object returnValue, out Exception exception);
}

What I like about a Try pattern based approach is that it doesn't require the introduction of further types.

The second overload might have to wait until Moq is actually recording thrown exceptions, too. (What's your use case? Would you want to know about thrown exceptions, if that information were also available in the records?) Also, I'm not sure the semantics of two out parameters are self-explanatory.

The first overload would be quite neat (IMHO) but we'd actually have to add some kind of flag to Invocation to track whether the invocation has a return value set or not. Which could be accomplished by turning Invocation.VerificationState into a more general flags enum and adding a Returned flag to it.

[MaStr11]

What I like about bool TryGetReturnValue(out object returnValue); is the evolvability of it (in contrast to most of my solutions, there is no need to implement the recording of exceptions now).
What is not clear to me is, how you intend to work with void methods. I see these options:

1. Return false, null
2. Return true, null
3. Return true, new Void()
4. Raise a new InvalidOperationException("The invoked method does not have a return value")

In the first case, it is ambiguous whether false means 'Invoked but finished with an exception' or 'Invoked but does not return values'.

The second option is indicating that the method returned null, but it actually returned nothing.

The third option is semantically more correct but technically wrong and is hard to discover (needs documentation).

The fourth option violates the "Try" pattern.

Besides this problem, I don't like how you would use the TryGet methods:

  if (invocation.TryGetReturnValue(out var returnValue, out var exception)
  {
    // Was the invocation successfull?
    if (exception == null) // Checking returnValue != null would be a bug!
    {
       // Now you also need to make sure that the returnValue is really what the method returned:
      if (returnValue != new System.Void()) // or alternatively 
      if (invocation.Method.ReturnType != typeof(System.Void))
      {
         // returnValue is really the returned value.
      } else {
         // the method returned nothing.
      }
    } else {
      // the method raised an exception
    }
  } else {
    // TryGetReturnValue(returnValue, exception) returned false. What does this mean?
  }

My use case is fairly simple. I just need the return value of a particular invocation to do some assertations about it. I mock the data layer. In tests of the business layer, I inspect the object returned from datalayer.Insert(...) to make sure the object stored is what it is supposed to be. Looks like this: mock.Invocations[0].GetReturnValue<Person>(). (GetReturnValue<T> is my extension method for IInvocation).

But I could imagine other use cases where you would query the mock.Invocations collection.

[stakx]

how you intend to work with void methods

I'd go with your option (4). Throwing InvalidOperationException from a Try method is unusual but I don't think it violates the Try pattern. Citing from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern:

When using this pattern, it is important to define the try functionality in strict terms. If the member fails for any reason other than the well-defined try, the member must still throw a corresponding exception.

My interpretation of this is that yes, a Try method may still throw exceptions if e. g. some well-defined precondition isn't met. Which would be the case here, TryGetReturnValue never makes sense for a void method, and the caller can easily and definitely recognize that case before the call.

Besides this problem, I don't like how you would use the TryGet methods: [...]

I feared that the semantics with two out parameters aren't immediately obvious (but could be easily documented). That's what I had in mind:

Debug.Assert(invocation.Method.ReturnType != typeof(void));
if (invocation.TryGetReturnValue(out var returnValue, out var exception))
{
    Debug.Assert(exception == null);
    // do something with `returnValue`
}
else
{
    Debug.Assert(exception != null);
    // do something with `exception`
}

[MaStr11]

Throwing an exception is bad when a user just wants to query the return values of all invocations of a mock. The user would be forced to filter the invocation list before. This filter is hard to discover: invocation.Method.ReturnType != typeof(void).

That TryGetReturnValue returns false in case of an exception is indeed not obvious and can be solved by documenting. It also seems that C#8 nullable reference types allow specifying 'exception will not be null if TryGetReturnValue returned false' (see https://devblogs.microsoft.com/dotnet/try-out-nullable-reference-types/ section Conditional postconditions).

And another thing: I just read that Callback and Returns accept InvocationFunc and InvocationAction which both work with IInvocation. In that context, the invocation is about to happen while on mock.Invocations the invocation already took place.

So everything related to RetrunValue only makes sense in the latter context. How do you want to solve that problem?

[stakx]

when a user just wants to query the return values of all invocations of a mock

you say that an exception would be "bad" in that case. I disagree. Without the exception, you're silently going to assume that a null (or Missing.Value, or any other magic value) was returned by some method when it wasn't. The exception will stop you from making that easy mistake.

The user would be forced to filter the invocation list before. This filter is hard to discover: invocation.Method.ReturnType != typeof(void)

I agree that it's not as easy as other parts of Moq's API... but does it really have to be easy? I would expect that the majority of users won't ever need to look at mock.Invocations. In my opinion, this is a case of "make the common things easy, and the rare things possible".

In that context, the invocation is about to happen while on mock.Invocations the invocation already took place. [...] How do you want to solve that problem?

Good point. My proposal would still work reliably for the out object returnValue part, but if the method returns false you might get null for the out Exception exception parameter.

I could argue for common sense (why would you query the return value in a Callback or Returns where you know whether it has been set or not, and you possibly know, or can easily reproduce, what it has been set to?), but I agree that the API should work reliably no matter when and where it's used.

So we're at a point where we could implement bool TryGetReturnValue(out object returnValue), but the overload with an additional out Exception parameter is problematic in several ways.

How about...

1. We later add a second method bool TryGetException(out Exception exception) that one would have to call independently of the former method (usually when the first one returned false)?

   if (invocation.TryGetReturnValue(out var returnValue))
   {
       // do something with `returnValue`
   }
   else if (invocation.TryGetException(out var exception))
   {
       Debug.Assert(exception != null);
       // do something with `exception`
   }
   else
   {
       // We either have an invocation to a `void` method,
       // or the invocation is still happening. For invocations
       // to non-`void` methods, we can distinguish between
       // the two; for `void` methods, we cannot.
   }

2. Keep things simple and add two properties to IInvocation: object ReturnValue and Exception Exception? It's very close to having two methods TryGetReturnValue and TryGetException; the former solution is far more simple, while the latter is more accurate because it allows one to distinguish between a "null return value", null := "return value not yet set", and null := "no return value".

(As before, I think we could wait with the exception property / method until it's actually needed.)

[MaStr11]

The exception will stop you from making that easy mistake

I'm fine with the exception approach. I just wanted to point out use cases where this behavior makes things more difficult (but also forces the user to do things right).

My proposal would still work reliably for the out object returnValue part, but if the method returns false you might get null for the out Exception exception parameter.

In my opinion, throwing a new InvalidOperationException("Can't provide a return value before an invocation took place") here would be better. I would even argue that this situation is a better fit for an exception than the void methods we discussed before.

I like the simple properties you proposed in 2. with the following semantics:

• ReturnValue
  • In the InvocationFunc and InvocationAction context: throw new InvalidOperationException("Can't provide a return value before an invocation took place")
  • For void returning methods: new InvalidOperationException("The invoked method does not have a return value")
  • If an exception was thrown: return null (or throw new InvalidOperationException("The return value can't be accessed because the invocation caused an exception.")?)
• Exception:
  • In the InvocationFunc and InvocationAction context: throw new InvalidOperationException("Can't provide a return value before an invocation took place")
  • If an exception was thrown return the exception
  • Otherwise return null

Implementing this sematic might not be possible without tracking exceptions.

What do you think?

[stakx]

In the InvocationFunc and InvocationAction context: throw new InvalidOperationException[...]

I don't think we'd have to be quite that strict: IIRC it should be easily possible to track whether or not an invocation has returned. If it has returned, there's no strict reason to unconditionally throw.

That being said, my gut feeling tells me that having properties that throw for a variety of reasons might be somewhat undesirable. I'm getting the impression one will have to tiptoe around those properties very carefully in order to not provoke an exception. For example:

If an exception was thrown: return null (or throw new InvalidOperationException("The return value can't be accessed because the invocation caused an exception.")?)

The latter (throwing) case implies that one will have to always query Exception first to discover whether ReturnValue may be safely queried.

If we get to that point, we could just as well use the Try pattern, which provides some additional safety and guidance.

I think that if we do go with properties, they probably shouldn't throw at all... you simply get the data that's available, and it'll be up to you to interpret the result (e.g. the meaning of ReturnValue == null).

All things considered, my vote tends to be for a TryGetReturnValue method (and possibly TryGetException, now or later). I'm OK with plain properties, too, but I'm not so excited by the idea of them throwing exceptions.

[stakx]

Hi @MaStr11, it's been a while. After having worked on getting Mock.Setups exposed, and designing the public ISetup type in several iterations, I am now much more inclined to go along with your PR. When it comes to the design of ISetup and IInvocation, easy introspection is perhaps more important than an overly perfect API design, e.g. a simple nullable object ReturnValue { get; } property (as you have proposed it) will work much better in an IDE's Watch window than a method with an out parameter (as I previously proposed).

Please give me some more time to take another close look at how Moq 5 does this.

[stakx]

@MaStr11, sorry once again for the long silence. I've mostly let Moq rest for a bit during the summer and am only now getting back to work on it.

I'd be happy to merge your PR! If you're still interested, could you please rebase this to current master, and add a entry to CHANGELOG.md, for example:

 ## Unreleased

 #### Added

 * New method overloads for `It.Is`, `It.IsIn`, and `It.IsNotIn` that compare values using a custom `IEqualityComparer<T>` (@weitzhandler, #1064)
Implement It.Is, It.IsIn, It.IsNotIn with a comparer overload (#1059)
+* New property `IInvocation.ReturnValue` to query recorded invocations' return values (@MaStr11, #921)

 #### Fixed

[MaStr11]

I'd be happy to merge your PR! If you're still interested, could you please rebase this to current master, and add an entry to CHANGELOG.md

Done. Thanks for accepting the change.

[stakx]

@MaStr11 – done. 🚀 Thanks for your contribution, and also for your patience; I know that I have been super-slow on this one.

[stakx]

P.S. @MaStr11: In case you were wondering where we stand regarding thrown exceptions, I didn't forget about it, but thought it would be cleaner to do that in a separate PR, to keep this one as straightforward as it was.

If you would like to submit a PR that adds an Exception Exception { get; } property to IInvocation (as you described in (1) above), please feel free to do so, and I'll merge that, too.

It shouldn't be too hard to do, setting the Invocation.Exception property would likely happen in a catch block being added there. (Ideally, we would make this addition without adding a new field to Invocation—to keep it as lightweight as possible–but that's not essential, it'd be more of an optimization. The existing returnValue field could serve double duty since an invocation cannot both return a value and throw. We'd only have to make sure that returned Exception objects won't leak out through the Exception property; this could be done by wrapping thrown exceptions in an internal type that only we can check for.)