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

Runtime support for static interface methods #50129

Closed
39 of 53 tasks
trylek opened this issue Mar 23, 2021 · 7 comments
Closed
39 of 53 tasks

Runtime support for static interface methods #50129

trylek opened this issue Mar 23, 2021 · 7 comments
Assignees
Labels
area-TypeSystem-coreclr Bottom Up Work Not part of a theme, epic, or user story tracking This issue is tracking the completion of other related issues.
Milestone

Comments

@trylek
Copy link
Member

trylek commented Mar 23, 2021

This issue tracks the runtime core team part of work on static interface methods. The broader implementation of this feature spanning other parts of the stack (JIT, Roslyn, libraries) are tracked separately. (Reference needed)

  • Update the ECMA addendum to cover static interface methods, their behavior and resolution;
  • Define positive and negative testing scenarios for the feature to run as CoreCLR IL-based tests (Roslyn support doesn't yet exist);
  • Coordinate with the JIT team regarding the new behavior of .constrained call / .constrained ldftn instruction;
  • Implement runtime changes to the typesystem and JIT interface to support compilation and runtime resolution of static interface methods;
    • Basic Functionality
    • Adjust JIT processing for the ldftn and call instructions to permit them to work with a constrained. prefix and pass the constrained type to the getCallInfo function.
    • Write a function to perform a static resolution given an implementing type, and an interface static method. Place this method into the MethodTable codebase. Call it MethodTable::ResolveStaticVirtualMethod(MethodDesc interfaceMethod)
      • This function shall process MethodImpls walking the type hierarchy. Be sure to properly handle covariant returns.
    • Adjust getCallInfo to have special handling for constrained ldftn and constrained call instructions.
      • If the constrained type is sufficiently well known to resolve to exactly one target method using MethodTable::ResolveStaticVirtualMethod, resolve to that target, and return the appropriate details to call that method directly.
      • If the constrained type is a shared generic which is insufficiently precise to specify only 1 possible target method, follow the path that returns CORINFO_CALL_CODE_POINTER (R2R implementation may be slightly different)
    • Adjust the type loader to allow abstract static methods to exist on interface types. Set a flag (HasStaticVirtualMethods) on the interface type to indicate that these methods exist.
    • Adjust normal MethodImpl processing to ignore MethodImpls which refer to static methods
    • Negative checking
    • In the same place as covariant return processing return type validation, if the type is non-abstract loop through all the interfaces that have the HasStaticVirtualMethods flag set. For each static abstract method on those interfaces, make sure it is implemented. If the interface was implemented on a base type which is also concrete, then this logic may be skipped.
    • In the constraint checker, if the type is abstract verify that interface constraints which have the HasStaticVirtualMethods flag set are fully implemented.
    • Basic optimization
    • Add a hashtable of interface method and constraint type which is filled in as ResolveStaticVirtualMethod is run. Future executions with the same parameters should be able to simply use the value in the hash.
    • Add a hashtable of concrete type, interface type to improve the performance of the constraint checker extra logic.
    • Reflection support
    • The implementation of GetInterfaceMap() will need to handle these new static interface methods.
    • Fix issues around TypeBuild.CreateType so that these new overrides can be implemented
  • Implement changes to the Crossgen2 JIT interface to support CG2 compilation of static interface methods;
    • Crossgen2 constraint checker update
    • R2R format update to handle new case (I'm not sure if its necessary, but we may need a new R2R fixup)
    • Crossgen2 jit interface updates
  • Implement the proposed set of tests and use them to validate the expected compilation / runtime behavior.
    • Test calls between generic contexts (Implement as autogenerator)
      • Generic over reference type
      • Generic over value type
      • Generic over constrained type which is instantiated as a reference type
      • Generic over constrained type which is instantiated as a valuetype
      • Curiously recurring pattern various incarnations thereof
      • Shared generic where the exact constrained type and interface type of the call are both reference types.
    • Test override with various different type hierarchies (Implement as autogenerator)
      • Test MethodBody of MethodImpl on MethodImpl defining type
      • Test MethodBody of MethodImpl on non-generic base type of MethodImpl defining type
      • Test MethodBody of MethodImpl on generic base type of MethodImpl defining type where base type is exactly defined
      • Test MethodBody of MethodImpl on generic base type of MethodImpl defining type where base type is a generic type defined by the generic parameters of MethodImpl defining type
    • Test covariant override (share autogenerator with normal type hierarchy testing)
      • Test covariant override where override is implemented on MethodImpl defining type (non-generic)
      • Test covariant override where override is implemented on MethodImpl defining type (generic over reference type)
      • Test covariant override where override is implemented on MethodImpl defining type (generic over value type)
      • Test covariant override where override is implemented on MethodImpl base type (non-generic)
      • Test covariant override where override is implemented on MethodImpl base type (generic case)
    • Positive Type equivalence test case
    • Positive test case, variant use of interface static method
    • Positive test case, verify that name/sig match static method on derived type has no effect on method dispatch of interface MethodImpl defined on base type
    • ReadyToRun Version resilience test
      • Verify that change in override produces correct result
      • Verify that adding a new override on the most derived class (where there was none before and the MethodImpl was only present on the Base class) works
    • Negative test case MethodBody defined on unrelated type
    • Negative test case, multiple MethodImpl records with same MethodDecl
    • Negative test case, non-abstract static virtual method on interface type
    • Negative test case, abstract virtual method on class type
    • Negative test case, non-abstract virtual method on class type
    • Negative test case, abstract static interface method which violates variance safety rule
    • Negative test case, non-implemented static abstract interface method on non-abstract class
    • Negative test case, non-implemented static abstract interface method on abstract class used concrete instantiation
    • Reflection testing
      • Test behavior of Type.GetInterfaceMap()
      • Test MethodInfo.Invoke on abstract static method -> Should throw
      • Test MethodBase.IsVirtual and MethodBase.IsAbstract -> Should show presence of static interface methods
      • Test PropertyInfo.GetValue/SetValue -> Should throw in presence of static interface methods
    • Reflection.Emit testing
      • Test behavior of TypeBuilder.DefineMethod to allow definition of abstract virtual static method
      • Test behavior of TypeBuilder.DefineMethodOverride to ensure that it works for defining override for method defined on type
      • Test behavior of TypeBuilder.DefineMethodOverride to ensure that it works for defining override for method defined on base type
@trylek trylek added this to the 6.0.0 milestone Mar 23, 2021
@trylek trylek self-assigned this Mar 23, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 23, 2021
@trylek trylek added User Story A single user-facing feature. Can be grouped under an epic. Bottom Up Work Not part of a theme, epic, or user story and removed untriaged New issue has not been triaged by the area owner labels Mar 23, 2021
@trylek
Copy link
Member Author

trylek commented Mar 23, 2021

@davidwrighton, Manish asked me to put together a user story for my part of the static interface work that he can use to track progress at our backlog meetings. Could you please take a look whether you find it reasonable? Thanks Tomas

@mangod9
Copy link
Member

mangod9 commented Mar 24, 2021

+@MadsTorgersen as well to link it to the larger e2e user-story.

@MichalStrehovsky
Copy link
Member

This will probably affect similar areas that default interface methods did.

I would add:

  • Support in reflection emit
  • Support in reflection (Would we expect them to show up in e.g. Type.GetInterfaceMap? Do we need to think about how reflection invoke for these works or doesn't work?)

For default interface methods we also had some library work, e.g. in the DispatchProxy class.

@davidwrighton
Copy link
Member

The reflection plan is as follows

  • TypeBuilder.DefineMethod shall allow creation of virtual abstract static methods on interfaces.
  • TypeBuilder.DefineMethodOverride should be useable.
  • Type.GetInterfaceMap should return the implemented static methods
  • MethodInfo.Invoke on an abstract static interface method should fail with an exception just like other abstract interface methods. Similar details shall hold for PropertyInfo.Get/SetValue and EventInfo.Get/SetValue.
  • MethodBase.IsVirtual and MethodBase.IsAbstract shall work
  • There shall be no way to use reflection Invoke to perform a constrained invoke. If a customer truly needs to do that, the customer may use the interface map acquired through Type.GetInterfaceMap()

@davidwrighton davidwrighton self-assigned this Mar 25, 2021
@Joe-Sewell-PreEmptive
Copy link

Joe-Sewell-PreEmptive commented Mar 30, 2021

Based on the body of this issue, this is the runtime work for supporting the proposed "static abstract members in interfaces" C# feature. But the title for this issue says "static interface methods" - is that the correct title? I thought that term referred to ordinary static methods in interfaces, which have been supported in the runtime since .NET Framework 1.0 and in C# since 8.0. Or is this a different feature than the proposed C# one?

Disclaimer: Views expressed by the author do not necessarily represent those of PreEmptive [my employer].

@davidwrighton
Copy link
Member

@Joe-Sewell-PreEmptive you are correct, this is in support of the proposed static abstract members in interfaces feature. However, we've been referring to the feature as static interface methods internally on the team for many years now since our first experimentations with this sort of thing ~10 years ago and we don't really see a great need to correct that terminology here as it hasn't caused much confusion. Inevitably, the publicly used documentation will be driven by name of the feature at the C# language level.

@marek-safar marek-safar added tracking This issue is tracking the completion of other related issues. and removed User Story A single user-facing feature. Can be grouped under an epic. labels Mar 31, 2021
@mangod9
Copy link
Member

mangod9 commented Jul 19, 2021

I am closing this userstory since most of the work is completed. If there is any pending items to be done post .net 6 we could track them separately.

@mangod9 mangod9 closed this as completed Jul 19, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-TypeSystem-coreclr Bottom Up Work Not part of a theme, epic, or user story tracking This issue is tracking the completion of other related issues.
Projects
None yet
Development

No branches or pull requests

6 participants