Improve output for expected argument for custom argument matchers on non-match or make it customizable

[rbeurskens]

When implementing a custom argument matcher, it would be nice that the output of what is expected would be the same of the default matchers (or is customizable):
Example:
sut.Received().MyMethod(Arg.Is<MyType>(p => p.Property == 42));
output:

NSubstitute.Exceptions.ReceivedCallsException
Expected to receive a call matching:
MyMethod(p => (p.Property == 42))
Actually received no matching calls.
Received 1 non-matching call (non-matching arguments indicated with '*' characters):
MyMethod(*MyType*)

at NSubstitute.Core.ReceivedCallsExceptionThrower.Throw(ICallSpecification callSpecification, IEnumerable`1 matchingCalls, IEnumerable`1 nonMatchingCalls, Quantity requiredQuantity)
at NSubstitute.Routing.Handlers.CheckReceivedCallsHandler.Handle(ICall call)
at NSubstitute.Routing.Route.Handle(ICall call)
at NSubstitute.Proxies.CastleDynamicProxy.CastleForwardingInterceptor.Intercept(IInvocation invocation)
at Castle.DynamicProxy.AbstractInvocation.Proceed()
at Castle.DynamicProxy.AbstractInvocation.Proceed()
at Castle.Proxies.ObjectProxy.MyMethod(MyType p)

With a custom argument matcher: (note the name of an internal proxy is used to describe the expected match)

NSubstitute.Exceptions.ReceivedCallsException
Expected to receive a call matching:
MyMethod(NSubstitute.Core.Arguments.ArgumentMatcher+GenericToNonGenericMatcherProxy`1[MyType])
Actually received no matching calls.
Received 1 non-matching call (non-matching arguments indicated with '*' characters):
MyMethod(*MyType*)
(...)

.. or (of it implements IDescribeNonMatches):

NSubstitute.Exceptions.ReceivedCallsException
Expected to receive a call matching:
MyMethod(NSubstitute.Core.Arguments.ArgumentMatcher+GenericToNonGenericMatcherProxyWithDescribe`1[MyType])
Actually received no matching calls.
Received 1 non-matching call (non-matching arguments indicated with '*' characters):
MyMethod(*MyType*)
(...)

Even if my custom matcher overrides .ToString() (which Arg.Is(Expression<Predicate<T>>)) seems to use to get the string of what is expected, the output does not change.

[rbeurskens]

It might be adding this to the proxy classes solves this: (looking at code of ArgumentFormatter, which checks if object.ToString is overridden.)

public override string ToString() => this._matcher.ToString();

As an alternative, a new interface IDescribeMatches (?) could be added to test on and determine if an argument matcher provides a custom implementation to describe the expected argument. (instead of testing if object.ToString() is overridden and calling that for this purpose)

[dtchepak]

Thanks for raising this @rbeurskens . This definitely needs to be improved.

ToString() seemed to work if enqueued directly (without being wrapped by a GenericToNonGenericMatcherProxy):

class CustomMatcher : IArgumentMatcher, IDescribeNonMatches, IArgumentMatcher<int>
{
    public string DescribeFor(object argument) => "failed";
    public bool IsSatisfiedBy(object argument) => false;
    public bool IsSatisfiedBy(int argument) => false;
    public override string ToString() => "Custom match";
}

[Test]
public void CustomArgMatch() {
    //ArgumentMatcher.Enqueue<int>(new CustomMatcher());
    var spec = new ArgumentSpecification(typeof(int), new CustomMatcher());
    SubstitutionContext.Current.ThreadContext.EnqueueArgumentSpecification(spec);
    _something.Received().Add(23, 0);
}
/*
  Failed CustomArgMatch [52 ms]
  Error Message:
   NSubstitute.Exceptions.ReceivedCallsException : Expected to receive a call matching:
        Add(23, Custom match)
Actually received no matching calls.
*/

I think we can just delegate to the matcher for these proxies?

    private class GenericToNonGenericMatcherProxy<T> : IArgumentMatcher
    {
        protected readonly IArgumentMatcher<T> _matcher;

        public GenericToNonGenericMatcherProxy(IArgumentMatcher<T> matcher)
        {
            _matcher = matcher;
        }

        public bool IsSatisfiedBy(object? argument) => _matcher.IsSatisfiedBy((T?)argument!);

        public override string ToString() => _matcher?.ToString() ?? ""; // TODO: fix nullability stuff here
    }

    private class GenericToNonGenericMatcherProxyWithDescribe<T> : GenericToNonGenericMatcherProxy<T>, IDescribeNonMatches
    {
        public GenericToNonGenericMatcherProxyWithDescribe(IArgumentMatcher<T> matcher) : base(matcher)
        {
            if (matcher is not IDescribeNonMatches) throw new SubstituteInternalException("Should implement IDescribeNonMatches type.");
        }

        public string DescribeFor(object? argument) => ((IDescribeNonMatches)_matcher).DescribeFor(argument);

        public override string ToString() => _matcher?.ToString() ?? "";    // TODO: fix nullability stuff here
    }

[rbeurskens]

I think we can just delegate to the matcher for these proxies?

This would not work as like there would be no proxy if the custom matcher does not override .ToString()

The problem with using .ToString() for this and reflection to get the type name of the matcher if it is not overridden makes this difficult to have the output the same as if there is no proxy in between because the formatter cannot access the matcher being proxied. (and the proxy does not know of the used formatter, unless it would be included/injected in its state).
It would be easier if there was an interface for this purpose, but that could mean a breaking change in the API.

public class ArgumentFormatter : IArgumentFormatter
{
    internal static IArgumentFormatter Default { get; } = new ArgumentFormatter();
    public string Format(object? argument, bool highlight)
    {
        var formatted = Format(argument);
        return highlight ? "*" + formatted + "*" : formatted;
    }

    private string Format(object? arg)
    {
        return arg switch
        {
            null => "<null>",
            string str => $"\"{str}\"",
            { } obj when HasDefaultToString(obj) => arg.GetType().GetNonMangledTypeName(), // <===
            _ => arg.ToString() ?? string.Empty
        };

        static bool HasDefaultToString(object obj)
            => obj.GetType().GetMethod(nameof(ToString), Type.EmptyTypes)!.DeclaringType == typeof(object); // <=== if obj is a proxy, we can't tell if it's overridden on the matcher itself
    }
}

Maybe this would be an option:

public interface IFormatable
{
    string Format(Func<object? ,string> format);
    // Alternative if exposing the proxied object through this property is no problem:
    // object? Arg {get;}
}
public class ArgumentFormatter : IArgumentFormatter
{
    private string Format(object? arg)
    {
        return arg switch
        {
            IFormattable f => f.Format(Format),
            // IFormattable f => Format(f.Arg),
            null => "<null>",
            string str => $"\"{str}\"",
            { } obj when HasDefaultToString(obj) => arg.GetType().GetNonMangledTypeName(), // <===
            _ => arg.ToString() ?? string.Empty
        };
        // (...)
    }
}
private class GenericToNonGenericMatcherProxy<T> : IArgumentMatcher, IFormatable
{
    public string Format(Func<object? ,string> format) => format(_matcher);
    // public object? Arg => _matcher;
    // (...)
}

It would still require custom IArgumentFormatter implementations to add support for the new interface, though just forwarding the .ToString() calls in the proxy may not return desired default output if the custom IArgumentMatcher<T> does not override it as the default string is created in the IArgumentFormatter implementation. (doing so would return the default object.String() output instead in that case)

[rbeurskens]

On a second thought and looking a bit further, I see how the code ends up running ArgumentFormatter.Format, as it is invoked from ArgumentSpecification like this, and I think I was making it more complicated than it has to be.
First I was wondering why those proxies are needed at all, but I see the generic IArgumentMatcher<T> (which could be changed to be contravariant, btw) is not used internally, where parameter values are typed/boxed as object. So they just exist to avoid forcing custom matchers to implement both generic and non-generic versions of the interface.

public class ArgumentSpecification : IArgumentSpecification
{
    public string FormatArgument(object? argument)
    {
        var isSatisfiedByArg = IsSatisfiedBy(argument);

        return _matcher is IArgumentFormatter matcherFormatter
            ? matcherFormatter.Format(argument, highlight: !isSatisfiedByArg)
            : ArgumentFormatter.Default.Format(argument, highlight: !isSatisfiedByArg);
    }
    // (...)
}

This means that we can just simply have the proxies implement IArgumentFormatter instead of adding a new interface or overriding .ToString().
I think this would keep it relatively clean and simple, not have any unwanted side effects or API changes and also work as desired if the custom matcher does not override .ToString().

private class GenericToNonGenericMatcherProxy<T> : IArgumentMatcher, IArgumentFormatter
{
    string Format(object? arg, bool highlight) =>
        _matcher is IArgumentFormatter matcherFormatter
            ? matcherFormatter.Format(argument, highlight)
            : ArgumentFormatter.Default.Format(argument, highlight);
    // (...)
}

(Since GenericToNonGenericMatcherProxyWithDescribe<T> inherits GenericToNonGenericMatcherProxy<T>, it only has to be added to the latter.)

[dtchepak]

One problem is we have two formatting concepts here: formatting matching/non-matching calls, and formatting specifications.

The latter is the one causing a problem here. It currently works off CallSpecification.ToString from ReceivedCallsExceptionThrower, which also relies on arg specification ToString. So if we are keeping that approach implementing ToString on the wrappers should be sufficient (and probably document that ToString should be overridden to describe custom arg matchers on IArgumentMatcher or similar).

Alternatively we could add an IArgumentSpecFormatter or similar to reflect this. I don't feel strongly about this but am leaning toward the simpler option to avoid an API change. 🤔

[rbeurskens]

So if I understand correctly, the 'problematic' output comes from the ArgumentSpecification.ToString() call (which always calls .ToString() on the matcher no matter if it is overridden or not) and not from ArgumentSpecification.FormatArgument(object?) ?
If so, it would currently also display undesired output (from default object.ToString() implementation) if .ToString() is not overridden by the matcher even if it is not wrapped in a proxy and the reason why default matchers have proper output is that all override .ToString() (it would indeed make sense the proxies would follow that convention)
I agree that if this would be a new/revised API, it would be better to have a separate interface for this, but as it is not, it would be better to document that custom matchers should override .ToString() to control what is displayed for "expected".
Maybe also add .ToString() to the IArgumentMatcher and IArgumentMatcher<T> interfaces (with xmldocs) to make it clear (even though it does not enforce overriding because it is implemented on object) and/or provide an optional base class similar to

public abstract class ArgumentMatcher<T> : IArgumentMatcher<T>, IArgumentMatcher
{
    /// <inheritdoc cref="IArgumentMatcher.ToString" />
    public abstract override string ToString();
    public abstract bool IsSatisfiedBy(T? argument);
    bool IArgumentMatcher.IsSatisfiedBy(object? argument) =>IsSatisfiedBy((T?)argument);
}

By the way, it would be handy for custom argument matchers to have access to ArgumentFormatter.Default.

.. and maybe also change code to only use proxies if they are needed:

    /// <summary>
    /// Enqueues a matcher for the method argument in current position and returns the value which should be
    /// passed back to the method you invoke.
    /// </summary>
    public static ref T? Enqueue<T>(IArgumentMatcher<T> argumentMatcher)
    {
        var matcher = switch argumentMatcher
        {
            null => throw new ArgumentNullException(nameof(argumentMatcher)),
            IArgumentMatcher m => m,
            IDescribeNonMatches => new ArgumentMatcher.GenericToNonGenericMatcherProxyWithDescribe<T>(argumentMatcher),
            _ => new ArgumentMatcher.GenericToNonGenericMatcherProxy<T>(argumentMatcher)
        }
        return ref ArgumentMatcher.EnqueueArgSpecification<T>(new ArgumentSpecification(typeof (T), matcher));
    }

[dtchepak]

I've created #806 to try to address this. Please take a look at let me know if you feel it is sufficient. 🙇

[rbeurskens]

Looks good to me on a first glance. Best of both worlds: An interface with a clear purpose for it without breaking code that uses the existing mechanism.
I added some review comments about some technical details in code that may or may not be relevant..