Attempt to fix nullability for e.g. #973

[jdb0123]

No description provided.

[dtchepak]

Thanks for picking this up @jdb0123 . Would it be simplest to revert #856 ?

Let me know when you're ready for a review of this. 🙇

[jdb0123]

@dtchepak no problem, happy to help.

I believe the current changes should be sufficient to fix the issues. However, I only detected and fixed things which showed up in our own usage of nsubstitute so I might have missed things.

I think reverting the changes would be a bit of waste, given that the direction of enabling nullable is a good idea.

Anyhow, these changes are ready for review.

[dtchepak]

Testing this out now. Is the sub.DoSomethingNullable(Arg.Is<string?>(x => x == null)).Returns(456); behaviour expected?

using NUnit.Framework;

namespace NSubstitute.Acceptance.Specs.FieldReports;

public class Issue973_MatchingWithNullability
{

    #nullable enable

    public interface ISomething
    {
        int DoSomething(string s);
        int DoSomethingNullable(string? s);
    }

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

        sub.DoSomething(Arg.Is<string>(x => x.StartsWith("12"))).Returns(42);

        Assert.That(sub.DoSomething("123"), Is.EqualTo(42));
        Assert.That(sub.DoSomething("abc"), Is.EqualTo(0));
    }

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

        sub.DoSomethingNullable(Arg.Is<string>(x => x.StartsWith("12"))).Returns(42);
        sub.DoSomethingNullable(Arg.Is<string?>(x => x == null)).Returns(456);

        Assert.That(sub.DoSomethingNullable("123"), Is.EqualTo(42));
        Assert.That(sub.DoSomethingNullable("hi"), Is.EqualTo(0));
        Assert.That(sub.DoSomethingNullable(null), Is.EqualTo(456));
        /*
failed Match_nullable 
    Assert.That(sub.DoSomethingNullable(null), Is.EqualTo(456))
  Expected: 456
  But was:  0
*/

    #nullable restore
}

[Romfos]

what is purpose of GenericTypeArgumentsAreCompatible. Is it related to #974 ? Maybe better to revert old PR until author will propose fixed version?

[Romfos]

Other changes looks good

[Romfos]

Testing this out now. Is the sub.DoSomethingNullable(Arg.Is<string?>(x => x == null)).Returns(456); behaviour expected?

using NUnit.Framework;

namespace NSubstitute.Acceptance.Specs.FieldReports;

public class Issue973_MatchingWithNullability
{

    #nullable enable

    public interface ISomething
    {
        int DoSomething(string s);
        int DoSomethingNullable(string? s);
    }

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

        sub.DoSomething(Arg.Is<string>(x => x.StartsWith("12"))).Returns(42);

        Assert.That(sub.DoSomething("123"), Is.EqualTo(42));
        Assert.That(sub.DoSomething("abc"), Is.EqualTo(0));
    }

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

        sub.DoSomethingNullable(Arg.Is<string>(x => x.StartsWith("12"))).Returns(42);
        sub.DoSomethingNullable(Arg.Is<string?>(x => x == null)).Returns(456);

        Assert.That(sub.DoSomethingNullable("123"), Is.EqualTo(42));
        Assert.That(sub.DoSomethingNullable("hi"), Is.EqualTo(0));
        Assert.That(sub.DoSomethingNullable(null), Is.EqualTo(456));
        /*
failed Match_nullable 
    Assert.That(sub.DoSomethingNullable(null), Is.EqualTo(456))
  Expected: 456
  But was:  0
*/

    #nullable restore
}

For me looks correct

[cremor]

For me looks correct

How is it correct that it doesn't return 456? How else would I setup an Arg.Is check for null?

[jdb0123]

@dtchepak sub.DoSomethingNullable(Arg.Is<string?>(x => x == null)).Returns(456); No i believe that to be incorrect. Added a fix to the review

[jdb0123]

what is purpose of GenericTypeArgumentsAreCompatible. Is it related to #974 ? Maybe better to revert old PR until author will propose fixed version?

No it was not related to #974
I just put all fixes we required to get our tests running / compiling again in this PR.

GenericTypeArgumentsAreCompatible was required to not use a derived setup when generic functions were called with a base type. I've added a test for this, see Stub_for_derived_type_argument_is_not_returned_for_base_type_argument

[dtchepak]

Hi @zvirja, was hoping to get your thoughts here based on your initial concern about enabling nullability. I'm a bit torn between reverting the initial nullability change vs. proceeding with @jdb0123's fix. @jdb0123's fix addresses the issues found i their code base, but we're not sure what other cases will emerge. Any thoughts on whether to press on with this, or revert and maybe take another look at this later?

[Romfos]

@dtchepak

I propose following:

• merge this
• merge Fix return type equivalency checking #977

and release as 6.0.1

and then lets process next feedback if any