Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error reporting for dynamic method invocation via reflection #97022

Closed
Nefcanto opened this issue Jan 16, 2024 · 15 comments · Fixed by #98129
Closed

Improve error reporting for dynamic method invocation via reflection #97022

Nefcanto opened this issue Jan 16, 2024 · 15 comments · Fixed by #98129
Labels
area-System.Reflection good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@Nefcanto
Copy link

When we invoke a method via reflection, in case something goes wrong with type matching we get this error:

Object does not match target type.

This is a very poor message for debugging. Please give us more information. Something like:

You wanted to invoke SomeMethod and you passed X of type XType with value XValue and Y with type YType. These types do not match with what SomeMethod expects as its parameters A of type AType and B of type BType.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 16, 2024
@ghost
Copy link

ghost commented Jan 16, 2024

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

Issue Details

When we invoke a method via reflection, in case something goes wrong with type matching we get this error:

Object does not match target type.

This is a very poor message for debugging. Please give us more information. Something like:

You wanted to invoke SomeMethod and you passed X of type XType with value XValue and Y with type YType. These types do not match with what SomeMethod expects as its parameters A of type AType and B of type BType.

Author: Nefcanto
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

@steveharter steveharter added help wanted [up-for-grabs] Good issue for external contributors good first issue Issue should be easy to implement, good for first-time contributors and removed untriaged New issue has not been triaged by the area owner labels Jan 17, 2024
@steveharter steveharter added this to the 9.0.0 milestone Jan 17, 2024
@steveharter
Copy link
Member

The request sounds reasonable; the proposed wording seems a little verbose. Maybe shorten to something like:

SomeMethod exists on type X but an object of type Y was passed.

It looks like the existing resource string "RFLCT_Targ_ITargMismatch" is used by the managed code in System.Reflection.MethodInvokerCommon and by runtime code in interoputil.cpp. To make this easier, we would likely create a new resource string for the managed code and leave the runtime\native code as-is.

@steveharter steveharter changed the title Improve error reporting for dynamic method invokation via reflection Improve error reporting for dynamic method invocation via reflection Jan 17, 2024
@Nefcanto
Copy link
Author

@steveharter, thank you so much.

@GustavoAdami
Copy link

@steveharter, I would like to take on this issue.

@RutulPatel8
Copy link

@Nefcanto or @steveharter
Can you please suggest how Can I debug runtime > Reflection code so I can fix this issue.
I tried lot but I didn't found the way.

@Nefcanto
Copy link
Author

Nefcanto commented Feb 4, 2024

@RutulPatel8, create a simple class with a single method that takes one parameter which is a string.

Now reflect it and call that method, but pass it a list of integers. That's how.

@RutulPatel8
Copy link

@RutulPatel8, create a simple class with a single method that takes one parameter which is a string.

Now reflect it and call that method, but pass it a list of integers. That's how.

@Nefcanto Yes I know. how to reproduce this issue.
I'm talking about dotnet > Runtime > libraries > System.Reflection debugging.
Currently I clone runtime repository but I don't know which solution file needs to execute(debug) in runtime project.
In libraries, I found System.Reflection csProj but I think it is not right way to technique.

@Nefcanto
Copy link
Author

Nefcanto commented Feb 4, 2024

@RutulPatel8, I apologize for my misunderstanding. I don't know about that part.

@RutulPatel8
Copy link

@RutulPatel8, I apologize for my misunderstanding. I don't know about that part.

Oh okay no issue Thanks for the help!

@wcsanders1
Copy link
Contributor

@RutulPatel8, create a simple class with a single method that takes one parameter which is a string.
Now reflect it and call that method, but pass it a list of integers. That's how.

@Nefcanto Yes I know. how to reproduce this issue. I'm talking about dotnet > Runtime > libraries > System.Reflection debugging. Currently I clone runtime repository but I don't know which solution file needs to execute(debug) in runtime project. In libraries, I found System.Reflection csProj but I think it is not right way to technique.

I believe the solution file you're looking for is this one: https://github.com/dotnet/runtime/blob/main/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln

@AndyBevan
Copy link
Contributor

I was taking a look at this and the error message gets thrown within class MethodInvokerCommon method ValidateInvokeTarget
I was unable to find any pre-existing tests for this class - am I missing them? I grepped the repo for the class name and did not find it used in anything test related.

I do find tests within \src\libraries\System.Private.CoreLib\tests\IntrinsicsInSystemPrivatecoreLibAnalyzer.Tests so would assume that somewhere under \tests\ would need a new test class adding for MethodInvokerCommon.

I tried to run the existing tests following the instructions from here -
https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/libraries/testing.md#running-tests-for-a-single-library

But this doesn't appear to work for src\libraries\System.Private.CoreLib\tests\ because there is no solution like there is for something like System.Collections.Immutable which has a solution within the libraries folder.

@steveharter - are you able to give some pointers on what I might be missing in terms of how we can run the tests within visual studio for System.Private.CoreLib?

@RutulPatel8
Copy link

I was taking a look at this and the error message gets thrown within class MethodInvokerCommon method ValidateInvokeTarget
I was unable to find any pre-existing tests for this class - am I missing them? I grepped the repo for the class name and did not find it used in anything test related.

I do find tests within \src\libraries\System.Private.CoreLib\tests\IntrinsicsInSystemPrivatecoreLibAnalyzer.Tests so would assume that somewhere under \tests\ would need a new test class adding for MethodInvokerCommon.

I tried to run the existing tests following the instructions from here -
https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/libraries/testing.md#running-tests-for-a-single-library

But this doesn't appear to work for src\libraries\System.Private.CoreLib\tests\ because there is no solution like there is for something like System.Collections.Immutable which has a solution within the libraries folder.

@steveharter - are you able to give some pointers on what I might be missing in terms of how we can run the tests within visual studio for System.Private.CoreLib?

Which solution did you execute for debugging?

@AndyBevan
Copy link
Contributor

AndyBevan commented Feb 5, 2024

Which solution did you execute for debugging?

At this point I have not found any tests to execute. I'm hoping that @steveharter will give us some pointers on how to approach this because there is a solution file that contains libraries\System.Private.CoreLib but it's in coreclr (/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.sln) and does not contain the tests (Maybe it should?)

Rest of my questions are in this post - #97022 (comment)

@AndyBevan
Copy link
Contributor

I think I figured it out - It looks like you need to use the System.Runtime solution which includes the public methods that allow access to System.Private.CoreLib.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2024
@AndyBevan
Copy link
Contributor

AndyBevan commented Feb 7, 2024

I've been looking at this and have submitted a PR - #98129

@steveharter - I do have a few questions that I'm hoping you can help me with.

  1. I added a test to DefaultBinderTests to get things working because there was a good class in the abstract to re-use but this really isn't testing DefaultBinding behavior. The other location I considered was MethodInvokerTests, those tests all relate to static method testing but maybe that is a better place. Thoughts?
  2. RFLCT_Targ_ITargMismatch is the current resource string, I added a new one in \src\libraries\System.Private.CoreLib\src\Resources\Strings.resx - is that the correct location?
  3. String Name RFLCT_Targ_ITargMismatch_WithType - Does this resource string name correct? I wasn't sure how to differentiate this new string from the original one.
  4. I used message Object of type {0} does not match target type {1}. This is different from the proposed message of SomeMethod exists on type X but an object of type Y was passed.. Because I'm not sure we know easily that the target type has SomeMethod. The problem here relates to the incorrect type being passed in so I think the error message I used adds the additional clarity we are looking for.
  5. I also have a general question about building the changes. Some of the changes are to MethodInvokerCommon which is not included in the solution System.Runtime solution where the managed code reflection tests are, it's class is located in the library folder but the class is not included in the csProj (\System.Private.CoreLib\src\System\Reflection\MethodInvokerCommon.cs). To modify MethodInvokerCommon I needed to load System.Private.CoreLib.sln, what's the easiest way to rebuild this library so that it gets pulled into for use within projects contained in the System.Runtime.sln solution. I ended up rerunning build.cmd clr+libs -rc Release but that's a big hammer for a small nut (hopefully). Is there a more efficient way to work to rebuild Private.CoreLib and then have the managed code libraries pull the new version? I hope this question makes sense. I tried just building System.Private.CoreLib.sln and that wasn't sufficient to for the projects contained in System.Runtime solution to have the new assembly.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2024
AndyBevan added a commit to AndyBevan/runtime that referenced this issue Feb 7, 2024
… and target type does not match passed type (fixes dotnet#97022)
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2024
buyaa-n pushed a commit that referenced this issue Feb 16, 2024
… and target type does not match passed type (#98129)

* Added more verbose error message when calling method using reflection and target type does not match passed type (fixes #97022)

* Fixed resource string as per PR comment

* Fixed the test related to a resource string change

* Modified test to have a contains check rather than validating the English string.

* Modifications to AOT implementation for validating target type when a method is invoked using reflection, inline with other PR changes
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 16, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
6 participants