Non-substitutable (non-virtual) member are sometimes still substitutable in extension methods

[rklec]

NS1000 and NS1001 sometimes lie.

At least, sometimes NSubstitute can actually substitute the methods correctly.
I don't quite get/know why this works, because I agree with the warning that it should not work, but it seems that it can successfully work on Extension methods (which by definiton cannot be virtual or so of course) if they are on an interface.

Code to reproduce that runs through without any error, only NSubstitute.Analyzers complains:

using NSubstitute;
using NUnit.Framework;

namespace Test
{
    class TestClass
    {
        [Test]
        public void NSubstituteNonVirtualTest_NS1001()
        {
            var thing = Substitute.For<ISomeThing>();

            thing.DoSomething();

            thing.Received(1).DoSomething(); // NS1001 error here
        }

        [Test]
        public void NSubstituteNonVirtualTest_NS1000()
        {
            var thing = Substitute.For<ISomeThing>();
            var otherThing = new Somethingelse();
            thing.GetModel<Somethingelse>(thing).Returns(otherThing); // NS1000 error here

            var result = thing.GetModel<Somethingelse>(thing);

            Assert.AreSame(result, otherThing);
        }
    }

    public class Something : ISomeThing
    {
        public Somethingelse Model { get; set; }
    }


    public class Somethingelse
    {
    }

    public interface ISomeThing
    {
        public Somethingelse Model { get; set; }
    }

    public interface ISomeThingelse
    {
    }
    
    public static class TestExtension
    {

        public static T DoSomething<T>(this T ownThing) where T : class, ISomeThing =>
            ownThing;

        public static Somethingelse GetModel<T>(this ISomeThing _, ISomeThing someThing)
        {
            var model = someThing.Model;

            return model;
        }
    }
}

Warnings

System

DotNet Core 3.1

<TargetFramework>netcoreapp3.1</TargetFramework>
<!-- ... -->
<Nullable>enable</Nullable>

Dependencies:

		<PackageReference Include="NSubstitute" Version="4.2.2" />
		<PackageReference Include="NSubstitute.Analyzers.CSharp" Version="1.0.14" />

[tpodolak]

Yes, this is limitation of NSubstitute. Analyzers. We dont even try to detect such cases, as this kind of analysis will be simply too expensive both in terms of memory/cpu usage and implementation time/complexity. For advanced users who "know what they are doing", we allow to configure analyzers to ignore certain rules for specific members. See more details in
#11 and in docs https://github.com/nsubstitute/NSubstitute.Analyzers/blob/dev/documentation/Configuration.md
@dtchepak do we have some definitive list of when NSubstitute works with non-virtual members? If so, I can try to analyze it and check again if it is feesable to handle cases described in this issue

[dtchepak]

Hi @tpodolak !

do we have some definitive list of when NSubstitute works with non-virtual members?

No. Whether it works or not depends on how the extension method delegates to a substitute, and whether the extension return matches the value returned from the call to the substitute. Whenever I've done this in the past I've considered it a bit of a hack based on knowing the internals of how NSubstitute works. The same trick can work with non-virtual calls to class members too -- if they forward to a virtual protected member then we can sometimes mock that protected member via the non-virtual call.

I think I'd prefer to discourage this practice via Analyzers as we can't reliably mock static members, and for people that are confident a particular case will work then they can suppress the error. Maybe we can make it easier to do this by having a separate error for not being able to reliably substitute for extension methods, so that can be suppressed separately from standard non-virtual calls?

[dtchepak]

Hi @rklec ,

don't quite get/know why this works

It sort of works by accident. For a contrived example:

public static int AddOne(this ICalculator c, int value) => c.Add(value, 1);

If we test this like:

var c = Substitute.For<ICalculator>();
c.AddOne(42).Returns(100);

NSubstitute does not see the AddOne call (non-virtual extension), but the substitute c receives a call to Add(42, 1). It then gets the Returns(100) call, so it now knows that whenever c gets an Add(42, 1) call it should return 100. The AddOne extension returns this value, so it seems like everything is working ok. There's a lot of scope for this trick not to work depending on the implementation of the static member. It is also very susceptible to breaking if we change the extension method. Because it relies on a combination of knowing the details of the extension method implementation I think it's a good idea to avoid this hack for all but the simplest cases. :)

[rklec]

Thanks a lot for all your detailed answers!

Personally, I'd also say you could adjust the doc/(new) error message or so, to state that in some circumstances it may work or so.

[tpodolak]

Maybe we can make it easier to do this by having a separate error for not being able to reliably substitute for extension methods, so that can be suppressed separately from standard non-virtual calls?

@dtchepak we have 3 different warnings for non-virtual member usages
https://github.com/nsubstitute/NSubstitute.Analyzers/blob/master/documentation/rules/NS1000.md
https://github.com/nsubstitute/NSubstitute.Analyzers/blob/master/documentation/rules/NS1001.md
https://github.com/nsubstitute/NSubstitute.Analyzers/blob/master/documentation/rules/NS1002.md

Should we add 3 additional warnings for extension method usage to align with that convention (one for Returns one for Received and one for When calls) ?

[dtchepak]

@tpodolak I guess it makes sense to follow that convention. Alternatively maybe we can merge NS1000 and NS1002?

[tpodolak]

Alternatively maybe we can merge NS1000 and NS1002?

I think this should be possible(will double check just in case) and merge them once I start working on this one.