[mono][metadata] Fixes for static virtual methods with interface variance #91664
+68
−9
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
ghost
assigned
This was referenced Sep 6, 2023
This was referenced Sep 7, 2023
…rent class first When we lookup the vtable slot for a generic interface method, we first go through the set of implemented interfaces in the klass and look for an exact match. If the klass is not implementing this exact interface, we then look for any interface implementation that is variant compatible. Before this change we were iterating starting from the base implementations which is incorrect. We now iterate starting from the interfaces explicitly implemented by the most derived type (matching first interface in declaration order) and then continuing with the base types. Scenario covered in ComplexHierarchyPositive.cs
…aces
As an example from the now enabled test.
```
interface InterfaceScenario2<in T> {
static abstract int Method ();
}
class BaseScenario2 : InterfaceScenario2<string, InterfaceScenario2<object> {
public static int Method() {
// .override method int32 class InterfaceScenario2`1<string>::Method()
return 2;
}
public static int32 Method_Obj() {
// .override method int32 class InterfaceScenario2`1<object>::Method()
return 3;
}
}
class DerivedScenario2 : BaseScenario2 {
public static int Method () {
// .override method int32 class InterfaceScenario2`1<object>::Method()
return 6;
}
}
public static string Test_Scenario2_1<T, ImplType>() where ImplType : InterfaceScenario2<T> {
return ImplType.Method ().ToString ();
}
Test_Scenario2_1<string, DerivedScenario2> ());
```
This test calls InterfaceScenario2<string> on DerviedScenario2. This should call DerivedScenario2.Method (not BaseScenario2.Method) because this method overrides (in BaseScenario2) both the obvious slot for InterfaceScenario2<object> as well as the slot for variant compatible interface InterfaceScenario2<string>.
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This was referenced Sep 22, 2023
|
@lambdageek Could you take a look at this ? |
|
The actual algorithm and everything look right here, but I'm concerned that applying the variance at the vtable level is wrong as a general approach, since the spec seems to require the check to be done at call sites. Or does every specific generic instantiation of a given interface have its own slot in every class's vtable? I was under the impression that wasn't true, and that was part of the motivation for the IMT in minijit. On the other hand, for the specific scenario of static virtual methods, the vtable approach seems like it's probably safe. |
kg
approved these changes
Jul 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #88689
Contributes to #49904