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

Handle generics in methodimpls for default interface methods #20404

Merged
merged 6 commits into from Nov 13, 2018

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Oct 12, 2018

The existing logic looking at MethodImpls to find the default interface implementation was not correct in the presence of generics. The MethodImpl records we build during type load only contain inexact MethodDescs for the declMethod that are not suitable for comparisons.

The fix is to pipe through the token and resolve the declaring method from it at the time of dispatch.

Fixes #16355.
Fixes #20281.
Fixes #16354.

The existing logic looking at MethodImpls to find the default interface implementation was not correct in the presence of generics. The MethodImpl record that we build during type load only contain inexact MethodDescs for the declMethod that are not suitable for comparisons.

The fix is to pipe through the token and resolve the declaring method from it at the time of dispatch.

Fixes #16355.
@MichalStrehovsky MichalStrehovsky added this to the 3.0 milestone Oct 12, 2018

// We do CanCastToInterface to also cover variance.
// We already know this is a method on the same type definition as the (generic)
// interface but we need to make sure the instantiations match.
Copy link
Member

@davidwrighton davidwrighton Oct 16, 2018

Choose a reason for hiding this comment

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

Variant dispatch is typically done via a two pass technique. Wherein we first iterate to see if there is an exact match, and then a second iteration through the interfaces (of a particular type) to attempt to do a variant dispatch. Looking at the logic in this function, it appears that the variant matching rules here may not be followed, and the variance test cases added do not appear to cover the exact/inexact matching issue.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Oct 16, 2018

Choose a reason for hiding this comment

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

The issue of it ignoring variant interface dispatch rules and not doing two passes is orthogonal to the thing I'm fixing here.

I would prefer to track that in a separate issue/pull request for accounting purposes.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Oct 16, 2018

Choose a reason for hiding this comment

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

#20452

Copy link
Member

@davidwrighton davidwrighton left a comment

🕐

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Nov 12, 2018

@dotnet-bot test this please

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Nov 13, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Nov 13, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

Timed out...

@MichalStrehovsky MichalStrehovsky merged commit e95c1f8 into dotnet:master Nov 13, 2018
31 checks passed
@MichalStrehovsky MichalStrehovsky deleted the fix16355 branch Nov 13, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this issue Feb 18, 2022
…coreclr#20404)

The existing logic looking at MethodImpls to find the default interface implementation was not correct in the presence of generics. The MethodImpl records we build during type load only contain inexact MethodDescs for the declMethod that are not suitable for comparisons.

The fix is to pipe through the token and resolve the declaring method from it at the time of dispatch.

Commit migrated from dotnet/coreclr@e95c1f8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants