Runtime: 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 https://github.com/dotnet/coreclr/pull/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

[danmosemsft]

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.

[danmosemsft]

Not required to ship 5.0 product. Setting milestone.