Add message details for AmbiguousMatchException #84512

IDisposable · 2023-04-08T04:15:18Z

Add information to what is ambiguously matched when an AmbiguousMatchException or an AmbiguousInvovationException is thrown as we know what we found multiple matching items for.

Fixes #18255

Basic idea is to use the known-first match as a source for the declaring type name and member name to format into the Exception message value.

There's a bunch of similar/common/duplicated code all over, so not sure where (which subset) to apply so this PR attempts most of the uses of AmbigousMatchException and AmbiguousInvocationException (which has similar issues).

Need to know before this gets final which of the libraries/builds this needs to be done in:

src/coreclr/nativeaot/System.Private.CoreLib
src/libraries/System.Private.CoreLib
src/libraries/System.Reflection.MetadataLoadContext
src/mono/System.Private.CoreLib

I also noted that the same localizable string ends up in several assemblies the way things are now (e.g. there's a Strings.resx and matching ThrowHelper.cs for each of the target libraries above, with some overlapped strings). I am not sure there is anything to be done about that, but worth calling out in this PR

ghost · 2023-04-08T04:15:29Z

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Add information to what is ambiguously matched when an AmbiguousMatchException is thrown as we know what we found multiple matching items for.

This is a draft PR to fix #18255

Basic idea is to use the known-first match as a source for the declaring type name and member name to format into the Exception message value.

There's a bunch of similar/common/duplicated code all over, so not sure where (which subset) to apply so this PR attempts most of the uses of AmbigousMatchException and AmbiguousInvocationException (which has similar issues).

Need to know before this gets final which of the libraries/builds this needs to be done in:

src/coreclr/nativeaot/System.Private.CoreLib
src/libraries/System.Private.CoreLib
src/libraries/System.Reflection.MetadataLoadContext
src/mono/System.Prive.CoreLib

I also noted that the same localizable string ends up in several assemblies the way things are now (e.g. there's a Strings.resx and matching ThrowHelper.cs for each of the targes above, with some overlapped strings)... not sure there is anything to be done about that, but worth calling out in this Draft

Author:	IDisposable
Assignees:	-
Labels:	`area-System.Reflection`
Milestone:	-

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

IDisposable · 2023-04-08T04:23:43Z

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

@@ -2891,7 +2889,7 @@ public override InterfaceMapping GetInterfaceMap([DynamicallyAccessedMembers(Dyn
 {
 if (returnType is null)
 // if we are here we have no args or property type to select over and we have more than one property with that name
- throw new AmbiguousMatchException();
+ ThrowHelper.ThrowAmbiguousMatchException(firstCandidate);


As this is the first use of the helper in this PR, I thought I would mention the strategy. To resolve the original issue and add more information to figure out what was ambiguous, we need to know the name of the member that has ambiguity.

Since PropertyInfo et al all descend from MemberInfo, we can make a helper that formats messages from calling .ToString() on the info and it's DeclaringType. This yields, for example from String.Length the values System.String and Int32 Length as the insertions. See ThrowHelper.cs

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

IDisposable · 2023-04-08T04:27:04Z

...veaot/System.Private.CoreLib/src/System/Reflection/Runtime/BindingFlagSupport/QueryResult.cs

-
- if (!_policies.OkToIgnoreAmbiguity(match, challenger))
- throw new AmbiguousMatchException();
+ // If they're not the same method, we throw if the policy doesn't allow ambiguity.


Merged the two if statements as they are logically doing the full check, and in either case throwing with the same info.

...stem.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.BindingFlags.cs

src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs

IDisposable · 2023-04-08T05:06:13Z

If this seems like a reasonable approach, I will build out tests for all the paths

@steveharter, @marek-safar, @MichalStrehovsky I am not sure I actually need to handle the case of a null DeclaringType... is that really a possibility, because much of the code I did a surface scan of seems to think that never happens, in which case I can simplify a bit and eliminate the _Simple strings.

IDisposable · 2023-04-08T11:36:55Z

Check failures do not look to be related to anything I did.

IDisposable · 2023-04-09T06:12:57Z

Fleshed out the rest of the AmbiguousMatchExeception uses (and removed talk about doing the same for AmbiguousImplementationExceptio as there's no details available)

IDisposable

Cleanup and fleshed things out a bit more. Seems to build.

src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/DefaultBinder.cs

src/libraries/System.Reflection.MetadataLoadContext/src/System/ThrowHelper.cs

src/libraries/System.Runtime.InteropServices/src/Resources/Strings.resx

...aries/System.Runtime.InteropServices/src/System/Runtime/InteropServices/ComAwareEventInfo.cs

src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

...veaot/System.Private.CoreLib/src/System/Reflection/Runtime/BindingFlagSupport/QueryResult.cs

src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

src/libraries/System.Private.CoreLib/src/System/DefaultBinder.cs

src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

src/libraries/System.Reflection.Context/src/System/Reflection/Context/ThrowHelper.cs

IDisposable · 2023-05-10T19:02:46Z

Thanks TONS for the review @buyaa-n ! I just had eye surgery so I'm face down with minimal screen time for at least a week, will integrate your changes ASAP!

IDisposable · 2023-05-10T19:07:24Z

~~I merged the simple ones, but this is no an unstable revision until I loop through the rest... I'll resync with main and squash this all before making it ready again~~ Completed feedback comments and rebased and squashed

IDisposable · 2023-05-29T23:24:25Z

Addressed final feedback, rebased and squashed.

Clean build except:
Mono Product Build windows x64 release/Build Product has an unrelated error

D:\a_work\1\s\src\mono\mono\utils\options.c(327): fatal error C1051: program database file, 'D:\a_work\1\s\artifacts\obj\mono\windows.x64.Release\mono\utils\CMakeFiles\utils_objects.dir\vc140.pdb', has an obsolete format, delete it and recompile

Build Libraries Test Run release mono osx x64 Debug/Publish Logs has an unrelated error

ApplicationInsightsTelemetrySender correlated 2 events with X-TFS-Session ee700fc9-8f51-4fbc-9dfc-a87d4770ce7a
##[error]Artifact BuildLogs_Mono__windows_x64_release already exists for build 289357.

src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

...dataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/CustomAttributeHelpers.cs

IDisposable · 2023-05-31T00:06:44Z

Not sure why the Detailed version isn't useful, but if we're killing off the uses, I can remove the literal. I added these originally because what I was getting back from the various .ToString() calls was different information, so it seemed valuable.

If we're absolutely sure we should never emit the type's information, I'll make one more pass to neaten the .ResX files and rebase/squash again

buyaa-n · 2023-05-31T00:52:58Z

Not sure why the Detailed version isn't useful, but if we're killing off the uses, I can remove the literal. I added these originally because what I was getting back from the various .ToString() calls was different information, so it seemed valuable.

It is more about the extra conditional added for checking if the declaringType is not null, than it was not useful. At least throwing an exception is not a hot path that needs to be performant, so I am OK to add it if the declaringType info added is really useful, for which I am not that sure about. Let's take the example you used, for:
Ambiguous implementation found for 'System.String Int32 Length'
Ambiguous implementation found for 'Int32 Length'
How useful the System.String part for you, or is this could cause any confusion when you were actually looking for the Length property?

Also, another option could be just passing the declaring type without null check as string.Format accepts null. The message would just have and extra space when the declaring type is null Ambiguous implementation found for ' Int32 Length'. How this sound?

If you want to keep it all as is I am also OK with that

...dataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/CustomAttributeHelpers.cs

IDisposable · 2023-05-31T01:07:09Z

Let's take the example you used, for: Ambiguous implementation found for 'System.String Int32 Length'
Ambiguous implementation found for 'Int32 Length'

How useful the System.String part for you, or is this could cause any confusion when you were actually looking for the Length property?

When we're figuring out why the exception was thrown (at post-mortem debug), knowing the exact class that had ONE of the ambiguities lets us narrow things down. We wouldn't be doing this on core classes usually, so having the type seems very helpful for opening the right source file :)

Also, another option could be just passing the declaring type without null check as string.Format accepts null. The message would just have and extra space when the declaring type is null Ambiguous implementation found for ' Int32 Length'. How this sound?

I like that idea... lemme churn that a bit and commit.

IDisposable · 2023-05-31T02:08:56Z

Incorporated these last few discussions. Now always passes the (possibly null) DeclaringType when dumping a MemberInfo (or derived). The other ResX messages are now named according to the ambiguous thing, so we're now talking about AmbiguousMatchException_Assembly, Arg_AmbiguousMatchException_Attribute, Arg_AmbiguousMatchException_CustomAttributeData (was Simple), Arg_AmbiguousMatchException_MemberInfo (was Detailed), and Arg_AmbiguousMatchException_RoDefinitionType with matching ThrowHelper or inline versions (as needed)

IDisposable · 2023-05-31T03:08:18Z

~~Build errors, please hold~~ Fixed it... helps to ^S in VS Code ;)

Add information to what is ambiguously matched when an AmbiguousMatchException is thrown as we know what we found multiple matching items for. Addressed review feedback Renames of SR/RESX strings. Removal of `.ToString()` calls. Remove unused strings from RESX RESX string rewording. Change the exception format string to "found for" style. Cleanup of DefaultBinder.cs FindMostDerived* to use the local variable. Remove copy-pasta ThrowHelper.cs instance. Don't emit the CustomAtrributeData's type All MemberInfo cases pass a (possibly null) `DeclaringType`. Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>

buyaa-n

Thank you @IDisposable LGTM

buyaa-n · 2023-05-31T17:51:28Z

The failures unrelated and all known issues, merging

dotnet-issue-labeler bot added the area-System.Reflection label Apr 8, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 8, 2023

IDisposable commented Apr 8, 2023

View reviewed changes

IDisposable mentioned this pull request Apr 8, 2023

Exception message for AmbiguousMatchException should include the name #18255

Closed

IDisposable changed the title ~~Add message for AmbiguousMatchException~~ Add detailed message for AmbiguousMatchException and AmbiguousInvocationException Apr 8, 2023

IDisposable changed the title ~~Add detailed message for AmbiguousMatchException and AmbiguousInvocationException~~ Add message details for AmbiguousMatchException Apr 9, 2023

IDisposable force-pushed the better-ambiguity-messages branch 3 times, most recently from 011f8d2 to 7fcafef Compare April 9, 2023 06:47

IDisposable commented Apr 9, 2023

View reviewed changes

build-analysis bot mentioned this pull request Apr 9, 2023

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

IDisposable marked this pull request as ready for review April 18, 2023 01:55

IDisposable requested review from marek-safar and MichalStrehovsky as code owners April 18, 2023 01:55