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

Fix privatescope method resolution bug #916

Merged

Conversation

mrvoorhe
Copy link
Contributor

When resolving a GenericInstanceMethod for a privatescope method that has the same signature as other methods, MetadataResolver.GetMethod would incorrectly return the first method with the same name.

The fix for this seems to be an optimization opportunity as well. Although I have to admit it feels a little too easy. Please make sure I'm not overlooking some reason why this fix is not safe.

I added 2 variations of tests.

PrivateScope - These tests were fine and passed as is. This is because the instruction operands are MethodDefinitions and therefore didn't need to be resolved by MetadataResolver

PrivateScopeGeneric - This test triggered the bug.

When resolving a `GenericInstanceMethod` for a `privatescope` method that has the same signature as other methods, `MetadataResolver.GetMethod` would incorrectly return the first method with the same name.

The fix for this seems to be an optimization opportunity as well.  Although I have to admit it feels a little to easy.  Please make sure I'm not overlooking some reason why this fix is not safe.

A added 2 variations of tests.

`PrivateScope` - These tests were fine and passed as is.  This is because the instruction operands are MethodDefinitions and therefore didn't need to be resolved by `MetadataResolver`

`PrivateScopeGeneric` - This test triggered the bug.

// The equality failure isn't going to be very helpful since both methods will have the same ToString value,
// so before we assert equality, we'll assert that the method is compiler controlled because that is the key difference
Assert.IsTrue(second_call_resolved.IsCompilerControlled, "Expected the method reference to resolve to a compiler controlled method");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first assertion that would fail. It would fail because second_call_resolved incorrectly resolved to first_call_resolved

@jbevain jbevain merged commit 56d4409 into jbevain:master Jun 29, 2023
@jbevain
Copy link
Owner

jbevain commented Jun 29, 2023

We keep learning interesting things. Thank you for the PR!

mrvoorhe added a commit to Unity-Technologies/cecil that referenced this pull request Oct 11, 2023
When resolving a `GenericInstanceMethod` for a `privatescope` method that has the same signature as other methods, `MetadataResolver.GetMethod` would incorrectly return the first method with the same name.

The fix for this seems to be an optimization opportunity as well.  Although I have to admit it feels a little to easy.  Please make sure I'm not overlooking some reason why this fix is not safe.

A added 2 variations of tests.

`PrivateScope` - These tests were fine and passed as is.  This is because the instruction operands are MethodDefinitions and therefore didn't need to be resolved by `MetadataResolver`

`PrivateScopeGeneric` - This test triggered the bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants