Detect non-virtual calls in Received.InOrder method

[tpodolak]

Originated from nsubstitute/NSubstitute#572

[tpodolak]

@dtchepak @zvirja @alexandrnikitin I was wondering how analyze the body of Received.InOrder so as it produces no false positives. From my experience I know that people usually use Received.InOrder basically in the way described in docs, so sth like

Received.InOrder(() => {
    connection.Open();
    connection.Closed();
//or
  connection.Closed(Arg.Is<int>());
  });
// etc

but I've seen usages which does sth like

Received.InOrder(() =>
            {
                // padding first
                _atomicLong.VolatileWrite(new IntPtr(tail),
                    RingBufferDescriptor.MakeHeader(RingBufferDescriptor.HeaderLength,
                        ManyToOneRingBuffer.PaddingMsgTypeId));
 
                // message then
                _atomicLong.VolatileWrite(new IntPtr(0), RingBufferDescriptor.MakeHeader(-recordLength, MessageTypeId));
                _buffer.Write(RingBufferDescriptor.EncodedMsgOffset(0), chunk);
                _atomicInt.VolatileWrite(new IntPtr(0), recordLength);
            });

note that

RingBufferDescriptor.MakeHeader(-recordLength, MessageTypeId)

is used not for order check but for providing argument value to the function. I assume that people might also do some more fancy stuff inside the lambda. So the question is, how should I distinguish
method being checked from other methods? I could ignore methods invoked as an arguments but I assume people might also do sth like

Received.InOrder(() =>
            {
               var x = RingBufferDescriptor.MakeHeader(RingBufferDescriptor.HeaderLength,
                        ManyToOneRingBuffer.PaddingMsgTypeId)
                _atomicLong.VolatileWrite(new IntPtr(tail),  x);
            });

so this approach is not bulletproof

[dtchepak]

Hi @tpodolak ,

I think this is a very hard problem. To solve the issue found in nsubstitute/NSubstitute#572, it might be sufficient to just ensure all Arg.xyz uses are with virtual members of substitutes? That smaller problem might be easier to handle well.

I was wondering how analyze the body of Received.InOrder so as it produces no false positives.

I don't think this is possible. :-\

Based on your very good analysis, we could try something like:

• Variable assignments can be non-checked method
• Arguments to checked methods can be non-checked method calls
• Other top-level calls in the lambda are all checked

I think this might be pretty inaccurate, but would hopefully catch simple misuses.

Another option is to demand every call be checked, with the error documentation making it clear to extract all real calls outside of the Received.InOrder. I'm not sure if there are cases where the argument will only become available part way through the sequence of calls though. Maybe it is reasonable even to enforce (or encourage; people can always disable the warning) that all objects receiving calls in an InOrder block are substitutes? I can imagine this being annoying for people that are used to pushing the boundaries of what they put in that lambda though.

[tpodolak]

Hi @dtchepak
Apologies for very late response. I finally had some time and prepared the PR for this issue. Is there any chance for you to build it locally and test on real project?
Regarding

Maybe it is reasonable even to enforce (or encourage; people can always disable the warning) that all objects receiving calls in an InOrder block are substitutes?

IMO I think that will be annoying for people who does not create substitutes directly via

var substitute = Substitute.For<IFoo>

for instance with automocks like AutoFixture or any other library. At this point I decided to implement the solution as per your comment

Based on your very good analysis, we could try something like:
Variable assignments can be non-checked method
Arguments to checked methods can be non-checked method calls
Other top-level calls in the lambda are all checked

[dtchepak]

Hi @tpodolak ,

I finally managed to test this (I have limited access to real .NET projects at the moment).

One case that was detected looked like this:

Received.InOrder(() => {
        ...
        Sut.ViewModel.Show(recipientViewModels);
    });

Here Sut.ViewModel is a real property on the class being tested, but the assertion is that the Show method is called on the interface. This can be extracted as per the rules we came up with, but if it can allow method chains that end in legitimate calls to substitutes then that would probably be better.

Another case found was:

Received.InOrder(() -> {
    ...
    foo.Received().SomeCall();
});

This received call triggered both a NS5001 (Received call in Received.InOrder) and NS1005 (Member Received can not be intercepted).

[tpodolak]

Thanks for checking @dtchepak. When it comes to

Received.InOrder(() => {
        ...
        Sut.ViewModel.Show(recipientViewModels);
    });

It if fixed and will not report diagnostics for method/properties inside the calling chain.
The second part

Received.InOrder(() -> {
    ...
    foo.Received().SomeCall();
});

will still report NS5001 (Received call in Received.InOrder) but NS1005 (Member Received can not be intercepted). will not be reported. Imo that is correct behaviour, are you OK with that?

[tpodolak]

@dtchepak I've merged corresponding PR with additional fix for your comments as described here.

[dtchepak]

Thanks @tpodolak 👍