Exception message for AmbiguousMatchException should include the name

[bradwilson]

Fundamentally, this is an indication of a bad argument, without telling you what the bad value is, which usually demands that you fire up the debugger to figure it out. It would be helpful to know what the ambiguous name actually was.

System.Reflection.AmbiguousMatchException : Ambiguous match found.
Stack Trace:
    at System.RuntimeType.GetMethodImpl(String name, BindingFlags bindingAttr, Binder binder, CallingConventions callConv, Type[] types, ParameterModifier[] modifiers)
    at System.Type.GetMethod(String name)

[terrajobst]

That makes sense to me.

@weshaggard @jkotas, any thoughts on this?

[jkotas]

indication of a bad argument, without telling you what the bad value is

This is true for majority of the exception messages. It is pretty rare for exception messages to include the bad argument value ... you have to pretty much always fire up the debugger if you need it (or get it in some other way).

If there is something particular about this one to make it really worth it to include the bad argument value, I do not see a problem with it. But we need to think about where reasonable balance is - I do not think we want to be adding the bad argument values to exception messages everywhere.

[bradwilson]

I do not think we want to be adding the bad argument values to exception messages everywhere.

Not to be simplistic, but: why not?

Allow me to draw the equivalent to a unit testing framework (since that's something I know a little bit about): Your argument would be the equivalent of saying "You don't need to add expected and actual values into your unit test messages, because they could find that out in a debugger". While that's true, it also diminishes the value, and at what cost?

[jkotas]

Allow me to draw the equivalent to a unit testing framework

The difference is in cost/benefit. Adding actual/expected values in unit test messages can be done in central place. It is worth it to add a few more lines in central place for this.

Adding bad argument values to exception messages everywhere would change like 10000's places just for corefx, bloat the code quite a bit. I do not think it is worth it.

[svick]

A similar change to ArgumentException thrown from Dictionary<K, V>.Add() was proposed in https://github.com/dotnet/corefx/issues/1187 and merged in dotnet/coreclr#1452. Is this any different?

Personally, I think clearer exceptions messages are worth some code bloat.

[justinvp]

It doesn't look like AmbiguousMatchException is created in too many places:

• CoreCLR
• CoreRT
• CoreFX

[MichalStrehovsky]

GetMethodImpl is just one of the places where the exception gets thrown. In order to get a consistent behavior, you would also need to pipe it through to Binder because that's what GetMethodImpl calls. But Binder.SelectMethod is not name based. So now you need to also do breaking API changes to the binder, or have a GetMethodImpl that only sometimes gives you the name in the exception message...

[karelz]

Next steps: Find the places which throw it - try to implement it consistently.
Complexity: Medium

[GrabYourPitchforks]

Let's tentatively try to make this a little better in 5.0 for debuggability. Though it's low-priority compared to other reflection work so it may still slip below the cut line.

Edit: We shouldn't strive for 100% coverage. Just target scenarios where we don't have to contort the code to make the exception message a little better.

[terrajobst]

Like #34703

[danmoseley]

My 2c. Many times issues first show up in logs. A debugger may not be available - nor dump file - nor even a repro. If by changing some localized places to add some value that we have easily available into that message, we can make some of these diagnosable without further work. It seems a place where relatively small work in a library is highly leveraged. This is why we added keys to key not found exceptions, paths to file not found exceptions. Of course there are cases where it is not worth it but there are still plenty of cases we we do not have a good reason to not do it.

GetMethodImpl is just one of the places where the exception gets thrown. In order to get a consistent behavior,

I don't think consistency for its own sake is important in these messages. In making changes of this sort in the past, I just skipped cases where it is difficult/impossible to supply the value. Eg., some of the IO exception paths. Most of our exception types were originally designed to have an optional "parameter" -- they already come in helpful and less helpful forms. We just prefer the more helpful form if we can.

[danmoseley]

Not required to ship 5.0 product. Setting milestone.

[steveharter]

Still seems like a valid request; there are ~24 cases currently: https://github.com/dotnet/runtime/search?q=%22new+AmbiguousMatchException%22&type=code

[IDisposable]

If I were to take this on, would it be reasonable to add a format string to Strings.resx for the new formatted versions of the error messages?
Something like these these new ones paralleling the current messages that don't name the ambiguity:

<data name="Arg_AmbiguousMatchException_Detailed" xml:space="preserve">
  <value>Ambiguous match found for: '{0}'.</value>
</data>
<data name="AmbiguousImplementationException_Detailed" xml:space="preserve">
  <value>Ambiguous implementation found: '{0} {1}'.</value>
</data>

Then in the throw-sites, would grab the argument string of whatever we're ambiguous about and format away.

For the throw-sites where where we HAVE a MemberInfo, we could use the MemberInfo.DeclaringType.Name and MemberInfo.Name or something like that (and emit it is "type.member" in the exception).

We could just use .ToString() but that's not great

var foo = "Hello World";
var footype = foo.GetType();
Console.WriteLine(footype.ToString());
var data = footype.GetProperty("Length");
Console.WriteLine(data.ToString());

System.String
Int32 Length

Would that case warrant two new format strings or is there a standard helper I would use to gen up a runtime-safe human-readable name of the declaring type + member (e.g. "String.Length")?

[IDisposable]

Maybe Console.WriteLine("Ambiguous implementation found for: '{0} {1}'", data.DeclaringType.ToString(), data.ToString()); which results in Ambiguous implementation found for: 'System.String Int32 Length'?

[IDisposable]

Additionally, it's plausible in cases like DefaultBinder.cs we actually have a lot more than just one method... is there value in having this exposed in more detail?

[danmoseley]

re parallel strings, we do this for IO, eg.,

  <data name="UnauthorizedAccess_IODenied_NoPathName" xml:space="preserve">
    <value>Access to the path is denied.</value>
  </data>
  <data name="UnauthorizedAccess_IODenied_Path" xml:space="preserve">
    <value>Access to the path '{0}' is denied.</value>
  </data>

and of course try our best to only use the one with the more info, but they are both there.

Edit: BTW, we try to put single quotes around replacement markers unless they're numbers. Not start with a replacement marker. and end with a period.

[danmoseley]

I don't own this area but I think you'd be welcome to take it @IDisposable. do you want me to assign to you?

[IDisposable]

Cool, thoughts on formatting and if emitting details where we have multiple ambiguous possibilities?

I'm thinking that (for example in DefaultBinder), we could just emit the CurMin's details... otherwise we would have to add some state to hold the information... seem like too much work and would change the public signature of the AmbiguousMatchException or force introduction of a derived child class for that state.

[IDisposable]

Updated example format strings per replacement standard mentioned above

[danmoseley]

I think an area owner like @steveharter should advise.

[IDisposable]

Draft PR #84512 for commentary as to my approach and to let the build bots test it :)

This take leverages the places where we have MemberInfo of the "first" match of the ambiguity and also emits the information about Assembly names for that ambiguous match.

I did NOT address dumping all the ambiguous matches because that would incur not only a huge change to the AmbiguousMatchException to have a place to hang the information, but it would also force us to carry the complete list of candidates when many of the current places we simply early-fail-out as soon as we know there's an ambiguity.

[buyaa-n]

I think the issue is asking for adding a member info in case no member info is provided. i.e:
instead of just Ambiguous match found. add member info: Ambiguous match found for: {0}

I don't feel we really need to change Ambiguous match found for: '{0}'. with "Ambiguous implementation found for: '{0} {1}'. Not sure if adding a memberInfo.DeclaringType info adds much clarification here:

    internal static AmbiguousMatchException GetAmbiguousMatchException(MemberInfo memberInfo)
    {
        Type? declaringType = memberInfo.DeclaringType;
            return (declaringType is not null)
                ? new AmbiguousMatchException(SR.Format(SR.Arg_AmbiguousMatchException_Detailed, declaringType, memberInfo))
                : new AmbiguousMatchException(SR.Format(SR.Arg_AmbiguousMatchException_Simple, memberInfo));
    }