Allow setting of out parameters

[samcragg]

I'm loving the work done under #401 to make the syntax for ref/out parameters much better, i.e. we can now say:

var lookup = Substitute.For<ILookup>();
lookup.TryLookup("hello", out Arg.Any<string>())
    .Returns(true);

However, the syntax to set the argument is still a bit clunky:

lookup.TryLookup("hello", out Arg.Any<string>())
    .Returns(x => { 
        x[1] = "world!";
        return true;
    });

I was wondering if you would be happy for me to submit a PR to take it one step further and let you change the argument, so I'm thinking of something like this:

lookup.TryLookup("hello", out Arg.Set("world"))
    .Returns(true);

Arg.Set (happy for you to pick another name 🙂) would have the same match semantics as Arg.Any - if you wanted the ability to selectively set the argument I guess we could have an Arg.SetWhen(condition, value) method perhaps, though personally I've never had a need to do argument matching on out parameters as you have to set them in the method anyway plus if the user did want this they could switch back to the more verbose syntax.

Thoughts?

[dtchepak]

Thanks for the suggestion!

I wonder if this would be possible to do by modifying Arg.Do:

// Proposed:
lookup.TryLookup("hello", out Arg.Do(ref string x => x = "world"))
    .Returns(true);

I haven't had a chance to look much into ref/out changes in C#7 so not sure how feasible this is. And Arg.Set might be easier than juggling multiple out/ref modifiers.

@zvirja: Any ideas about this?

[zvirja]

Hi David,

My initial thought was also to re-use the existing Arg.Do() instead of creating a new one 😉

However, after giving it more precise look, I found myself to be reluctant to add this feature. The main concern is that Arg.XXX() matcher becomes unsynchronized with Returns() callback, which could lead to confusion. Consider following sample:

lookup.TryLookup("hello", ref Arg.Set(42)).Returns(true, false);

How should we expect Arg.Set() to behave - assign only once, or for each call? Probably, when we return false, we don't want value to be assigned.

Other sample:

lookup.TryLookup("hello", ref Arg.Set(42))
      .Returns(x => true, x => { x[1] = 44; return true; }, x => false)

Should Returns() override the value set by Arg.Set()?

Probably, if we look even more, we could see other scenarios where feature isn't that obvious.

Given that ref and out scenarios are not that popular, I'd keep the existing way. Yes, it doesn't provide us with the compile-time safety, but it doesn't cause the confusion we might have due to Arg.XXX() and Returns() synchronization.

Instead, we might solve this by analyzers from @tpodolak, so they catch when we assign value incorrectly (if we don't do that already...).

[samcragg]

Thanks for the comments guys, much appreciated.

With regards to using Arg.Do - I think that can work if we change the Argument class to have the argument array+index passed to it so that the Value property can be a ref return. Doing a quick test, though, and the syntax to invoke it would be nearly as described by @dtchepak (notice the extra parenthesis and also you can't write Arg.Do<string>((ref x) …)):

lookup.TryLookup("hello", out Arg.Do((ref string x) => x = "world"))
    .Returns(true);

The points about Returns are valid - my expectation would be that it does always overwrite the value for the first scenario but in the second scenario the value set in Returns would be the winning value.

The main improvement I see for the feature was working with out parameters, not necessarily the compile time enforcement but making it more concise and it just working. I think it's already been pointed out that the feature won't be that widely used, as out parameters are not that common, but when you do need to use them I think having a discoverable short cut that works for the common scenario of setting the value can make the tests look cleaner, though appreciate it's adding features to the library to cater for something not commonly done (but then again, the whole ref support seems to have been added to better handle ref/out parameters)

Happy to close the request if you think it doesn't add any merit, however, equally as happy to do the work to implement the specification you come up with 🙂

[alexandrnikitin]

I would vote for do nothing for now. While I don't like the current syntax with the array of objects, I don't see a nice and consistent way to set and check refs/outs. (Maybe the FakeItEasy approach is a bit nicer, see below). @zvirja already mentioned a few edge cases and there are more (ref/out can be used in arg matchers, multiple return specs, partial mocks)

I reviewed what other frameworks have for ref/out.

Moq has Callback() method

// callbacks for methods with `ref` / `out` parameters are possible but require some work (and Moq 4.8 or later):
delegate void SubmitCallback(ref Bar bar);

mock.Setup(foo => foo.Submit(ref It.Ref<Bar>.IsAny)
    .Callback(new SubmitCallback((ref Bar bar) => bar = new Bar());

FakeItEasy has a dedicated method to set refs and outs (looks neat for me):

A.CallTo(() => aFake.AMethod(anInt, ref aRef, out anOut))
 .Returns(true)
 .AssignsOutAndRefParameters("new aRef value", "new anOut value");

It seems that Typemock and JustMock just pass argument values. I believe they should have "callback" feature but unfortunately I couldn't find any example in docs.

[dtchepak]

Thanks for all the good points everyone!

Given that ref and out scenarios are not that popular, ...

I've been doing some non-.NET work for a while now, so I'm not sure about this, but from recent C# changes like the ref-returns stuff I wonder if these are going to become more commonplace?

Happy to close the request if you think it doesn't add any merit, however, equally as happy to do the work to implement the specification you come up with

It definitely has merit @samcragg! It's just whether we can get it to slot in with other features. Thanks for raising it, and your generous offer to help implement! 😄

FakeItEasy has a dedicated method to set refs and outs (looks neat for me)

Looks neat to me too! We can already do something similar with .AndDoes:

            sub.Lookup("blah", out Arg.Any<int>())
               .Returns(true)
               .AndDoes(x => x[1] = 45);

We could possibly have another dedicated method on ConfiguredCall and implement AssignsOutAndRefParameters using the existing .AndDoes code. I'm not sure it offers enough over .AndDoes to be worthwhile though?

While trying out this example, I realised we already have the problem Alex mentioned with synchronising with Returns:

        public interface ILookup { bool Lookup(string key, out int i); }

        [Fact]
        public void Test() {
            var sub = Substitute.For<ILookup>();

            sub.Lookup("blah", out Arg.Any<int>())
               .Returns(x => {
                   x[1] = 42;
                   return true;
                })
               .AndDoes(x => x[1] = 45);

            sub.Lookup("blah", out var value);

            Assert.Equal(??, value);
        }

In this test, value will actually be 42 (the .Returns set wins). I've raised nsubstitute/NSubstitute.Analyzers#62 to try to detect this case; maybe we could do similarly with Arg.Set or Arg.Do? Following the current convention, we could define that .Returns code always wins (which sort of makes sense; getting the return value is the last thing that happens when a call runs).

I'm pretty torn about this. The edge cases raised are very valid and good illustrations of the confusion this can cause; but on the other hand the Arg.Set syntax is quite a bit neater (especially for setting multiple args), and can be avoided if people don't want to use it. There is a similar precedent for Arg.Invoke (which I was also hesitant to include!), that optimises for the case where we want to execute callbacks passed to a subbed method, at the expense of making things a bit more confusing.

So I guess I'm on the fence on this one (as usual 😂). Happy to accept your "hold" votes on this @alexandrnikitin and @zvirja until we get more demand for the feature. Or if my rambling here has changed your minds I'd also be happy to get this added to main Arg class, or even as an experimental extension/class so we're not cluttering the main API, and people can opt in if they are really keen.