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

Catch ambiguous interface method resolution exceptions #15978

Merged
merged 1 commit into from Jan 24, 2018

Conversation

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Jan 23, 2018

Default interface methods might end up being ambiguous. Resolution thows an exception we need to catch at devirtualization time. The exception will still happen at dispatch time.

Default interface methods might end up being ambiguous. This thows an exception we need to catch.
@MichalStrehovsky

This comment has been minimized.

Copy link
Member Author

MichalStrehovsky commented Jan 23, 2018

(Not a fan of the fix, but not a fan of the throwing behavior in the first place. I guess we could also pass a bool flag across half a dozen frames to the place where we throw and use the value of the flag to decide if we want to throw?)

@jkotas PTAL

Cc @sergiy-k

@MichalStrehovsky MichalStrehovsky added this to TODO in Default Interface Methods via automation Jan 23, 2018
@jkotas
jkotas approved these changes Jan 23, 2018
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 23, 2018

Do we need a test that hits this?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 23, 2018

@MichalStrehovsky

This comment has been minimized.

Copy link
Member Author

MichalStrehovsky commented Jan 23, 2018

Do we need a test that hits this?

This is getting hit by Loader\classloader\DefaultInterfaceMethods\diamondshape\diamondshape, but that one also hits #15979 so I'm not removing the test from issues.targets with this pull request.

@AndyAyersMS

This comment has been minimized.

Copy link
Member

AndyAyersMS commented Jan 23, 2018

Is there a link anywhere that discusses implementation decisions? Seems like it would be good to have something written up, even if just an overview, in the CoreCLR design docs.

In particular I'm wondering if this is something that we could catch when the class is loaded, or whether that is too expensive and/or undesirable.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 24, 2018

Is there a link anywhere that discusses implementation decisions?

Agree. Opened issue https://github.com/dotnet/coreclr/issues/15989

@jkotas jkotas merged commit 007fa55 into dotnet:master Jan 24, 2018
13 of 14 checks passed
13 of 14 checks passed
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Alpine.3.6 x64 Debug Build Build finished.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm64 Cross Debug Innerloop Build Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Innerloop Formatting Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Innerloop Formatting Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
license/cla All CLA requirements met.
Details
Default Interface Methods automation moved this from TODO to Done Jan 24, 2018
@MichalStrehovsky MichalStrehovsky deleted the MichalStrehovsky:fixDevirtException branch Jan 24, 2018
MichalStrehovsky added a commit to MichalStrehovsky/coreclr that referenced this pull request Jan 24, 2018
* The diamondshape test should work now that dotnet#15979 and dotnet#15978 are merged.
* Create debug and retail version of diamondshape and sharedgenerics tests so that we have retail coverage. These tests hit issues around devirtualization (that is only active when RyuJIT is optimizing).
MichalStrehovsky added a commit that referenced this pull request Jan 30, 2018
* The diamondshape test should work now that #15979 and #15978 are merged.
* Create debug and retail version of diamondshape and sharedgenerics tests so that we have retail coverage. These tests hit issues around devirtualization (that is only active when RyuJIT is optimizing).
* Add license headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.