Don't return null target methods in GetInterfaceMap for abstract classes, or classes using DIMs #53972
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @CoffeeFlux Issue DetailsThe only case where we should be getting nulls back is for reabstracted DIMs. Otherwise we should be returning either the abstract method from the abstract class (or its ancestor classes) or a default interface method. Fixes #53933
|
Regression tests for dotnet#53933
|
Once it's reviewed, we need to sync this back to mono/mono |
|
Failures are related. Needs more work |
If the iterface method is reabstracted, and either the found implementation method is abstract, or the found implementation method is from another DIM (meaning neither klass nor any of its ancestor classes implemented the method), then say the target method is null. Otherwise return the found implementation method, even if it is abstract, unless we found a reabstracted method in a non-abstract class Fixes dotnet#53933
|
Ready for review |
|
@vargaz please review |
vargaz
reviewed
Jun 15, 2021
| /* only valid on interface methods*/ | ||
| /* method is marked "virtual" but not "virtual abstract" */ | ||
| return method->flags & method->flags & METHOD_ATTRIBUTE_VIRTUAL && !(method->flags & METHOD_ATTRIBUTE_ABSTRACT); | ||
| } |
There was a problem hiding this comment.
There are some accessors like m_method_is_virtual () etc.
There was a problem hiding this comment.
This looks wrong.
@vargaz do you mean not using the accessor is wrong, or checking for a default interface method using the method flags is wrong?
There was a problem hiding this comment.
This part:
method->flags & method->flags & METHOD_ATTRIBUTE_VIRTUAL
vargaz
approved these changes
Jun 15, 2021
|
/azp run sync-runtime-to-mono |
|
Azure Pipelines successfully started running 1 pipeline(s). |
lambdageek
added a commit
to lambdageek/mono
that referenced
this pull request
Jun 15, 2021
…ses, or classes using DIMs If the interface method is reabstracted, and either the found implementation method is abstract, or the found implementation method is from another DIM (meaning neither klass nor any of its ancestor classes implemented the method), then say the target method is null. Otherwise return the found implementation method, even if it is abstract. Corresponding dotnet/runtime PR is dotnet/runtime#53972 Fixes dotnet/runtime#53933
lambdageek
added a commit
to mono/mono
that referenced
this pull request
Jun 21, 2021
…ses, or classes using DIMs (#21112) * Don't return null target methods in GetInterfaceMap for abstract classes, or classes using DIMs If the interface method is reabstracted, and either the found implementation method is abstract, or the found implementation method is from another DIM (meaning neither klass nor any of its ancestor classes implemented the method), then say the target method is null. Otherwise return the found implementation method, even if it is abstract. Corresponding dotnet/runtime PR is dotnet/runtime#53972 Fixes dotnet/runtime#53933 * Add method flags accessors; cleanup interface map code Ported from dotnet/runtime#54271
bholmes
added a commit
to Unity-Technologies/mono
that referenced
this pull request
Jul 1, 2021
…ses, or classes using DIMs (mono#21112) * Don't return null target methods in GetInterfaceMap for abstract classes, or classes using DIMs If the interface method is reabstracted, and either the found implementation method is abstract, or the found implementation method is from another DIM (meaning neither klass nor any of its ancestor classes implemented the method), then say the target method is null. Otherwise return the found implementation method, even if it is abstract. Corresponding dotnet/runtime PR is dotnet/runtime#53972 Fixes dotnet/runtime#53933 * Add method flags accessors; cleanup interface map code Ported from dotnet/runtime#54271
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the interface method is reabstracted, and either the found implementation method is abstract, or the found implementation method is from another DIM (meaning neither klass nor any of its ancestor classes implemented the
method), then say the target method is null. Otherwise return the found implementation method, even if it is abstract.
Fixes #53933