-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Covariant Returns Feature #35308
Covariant Returns Feature #35308
Changes from 12 commits
bf7501f
fef49b2
f10119d
8d5d87e
a14af6f
0e4e472
698eafb
6848c40
5a85da8
89bb5c3
cbecf78
1154b90
f97b9de
1329fea
997074f
ed9fb4c
6eaaa1a
688c263
3636d03
c05f075
7ccd3ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
# Covariant Return Methods | ||
|
||
Covariant return methods is a runtime feature designed to support the [covariant return types](https://github.com/dotnet/csharplang/blob/master/proposals/covariant-returns.md) and [records](https://github.com/dotnet/csharplang/blob/master/proposals/records.md) C# language features posed for C# 9.0. | ||
|
||
This feature allows an overriding method to have a return type that is different than the one on the method it overrides, but compatible with it. The type compability rules are defined in ECMA I.8.7.1. Example: using a more derived return type. | ||
|
||
Covariant return methods can only be described through MethodImpl records, and as an initial implementation will only be applicable to methods on reference types. Methods on interfaces and value types will not be supported (may be supported later in the future). | ||
|
||
MethodImpl checking will allow a return type to vary as long as the override is compatible with the return type of the method overriden (ECMA I.8.7.1). | ||
|
||
If a language wishes for the override to be semantically visible such that users of the more derived type may rely on the covariant return type it shall make the override a newslot method with appropriate visibility AND name to be used outside of the class. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This statement sounds like newslot is not required if the method is not semantically visible (BTW, what exactly does it mean to be semantically visible). Is this correct? |
||
|
||
For virtual method slot MethodImpl overrides, each slot shall be checked for compatible signature on type load. (Implementation note: This behavior can be triggered only if the type has a covariant return type override in its hierarchy, so as to make this pay for play.) | ||
|
||
A new `RequireMethodImplToRemainInEffectAttribute` shall be added. The presence of this attribute is to require the type loader to ensure that the MethodImpl records specified on the method have not lost their slot unifying behavior due to other actions. This is used to allow the C# language to require that overrides have the consistent behaviors expected. The expectation is that C# would place this attribute on covariant override methods in classes. | ||
|
||
## Implementation Notes | ||
|
||
### Return Type Checking | ||
|
||
During enumeration of MethodImpls on a type (`MethodTableBuilder::EnumerateMethodImpls()`), if the signatures of the MethodImpl and the MethodDecl do not match: | ||
1. We repeat the signature comparison a second time, but skip the comparison of the return type signatures. If the signatures for the rest of the method arguments match, we will conditionally treat that MethodImpl as a valid one, but flag it for a closer examination of the return type compatibility at a later stage of type loading (end of `CLASS_LOAD_EXACTPARENTS` stage). | ||
2. At the end of the `CLASS_LOAD_EXACTPARENTS` type loading stage, examing each virtual method on the type, and if it has been flagged for further return type checking: | ||
+ Load the `TypeHandle` of the return type of the method on base type. | ||
+ Load the `TypeHandle` of the return type of the method on the current type being validated. | ||
+ Verify that the second `TypeHandle` is compatible with the first `TypeHandle` using the `MethodTable::CanCastTo()` API. If they are not compatible, a TypeLoadException is thrown. | ||
|
||
The only exception where `CanCastTo()` will return true for an incompatible type according to the ECMA rules is for structs implementing interfaces, so we explicitly check for that case and throw a TypeLoadException if we hit it. | ||
|
||
Once a method is flagged for return type checking, every time the vtable slot containing that method gets overridden on a derived type, the new override will also be checked for compatiblity. This is to ensure that no derived type can implicitly override some virtual method that has already been overridden by some MethodImpl with a covariant return type. | ||
|
||
### VTable Slot Unification | ||
|
||
If a MethodImpl has the `RequireMethodImplToRemainInEffectAttribute` attribute, it needs to propagate all applicable vtable slots on the type. This is to ensure that if we use the signature of one of the base type methods to call the overriding method, we still execute the overriding method. | ||
|
||
Consider this case: | ||
``` C# | ||
class A { | ||
RetType VirtualFunction() { } | ||
} | ||
class B : A { | ||
[RequireMethodImplToRemainInEffect] | ||
DerivedRetType VirtualFunction() { .override A.VirtualFuncion } | ||
} | ||
class C : B { | ||
[RequireMethodImplToRemainInEffect] | ||
MoreDerivedRetType VirtualFunction() { .override A.VirtualFunction } | ||
} | ||
``` | ||
|
||
Given an object of type `C`, the attribute will ensure that: | ||
``` C# | ||
callvirt RetType A::VirtualFunc() -> executes the MethodImpl on C | ||
callvirt DerivedRetType B::VirtualFunc() -> executes the MethodImpl on C | ||
callvirt MoreDerivedRetType C::VirtualFunc() -> executes the MethodImpl on C | ||
``` | ||
|
||
Without the attribute, the second callvirt would normally execute the MethodImpl on `B` (the MethodImpl on `C` does not override the vtable slot of `B`'s MethodImpl, but only overrides the declaring method's vtable slot. | ||
|
||
This slot unification step will also take place during the last step of type loading (end of `CLASS_LOAD_EXACTPARENTS` stage). | ||
|
||
### [Future] Interface Support | ||
|
||
An interface method may be both non-final and have a MethodImpl that declares that it overrides another interface method. If it does, NO other interface method may .override it. Instead further overrides must override the method that it overrode. Also the overriding method may only override 1 method. | ||
|
||
The default interface method resolution algorithm shall change from: | ||
|
||
``` console | ||
Given interface method M and type T. | ||
Let MSearch = M | ||
Let MFound = Most specific implementation within the interfaces for MSearch within type T. If multiple implementations are found, throw Ambiguous match exception. | ||
Return MFound | ||
``` | ||
|
||
To: | ||
|
||
``` console | ||
Given interface method M and type T. | ||
Let MSearch = M | ||
|
||
If (MSearch overrides another method MBase) | ||
Let MSearch = MBase | ||
|
||
Let MFound = Most specific implementation within the interfaces for MSearch within type T. If multiple implementations are found, throw Ambiguous match exception. | ||
Let M Code = NULL | ||
|
||
If ((MFound != Msearch) and (MFound is not final)) | ||
Let M ClassVirtual = ResolveInterfaceMethod for MFound to virtual override on class T without using Default interface method implementation or return NULL if not found. | ||
If (M ClassVirtual != NULL) | ||
Let M Code= ResolveVirtualMethod for MFound on class T to implementation method | ||
|
||
If (M Code != NULL) | ||
Let M Code = MFound | ||
|
||
Check M Code For signature <compatible-with> interface method M. | ||
|
||
Return M Code | ||
``` |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -45,6 +45,9 @@ private class UnificationGroup | |||||
private MethodDesc[] _members = MethodDesc.EmptyMethods; | ||||||
private int _memberCount = 0; | ||||||
|
||||||
private MethodDesc[] _methodsRequiringSlotUnification = MethodDesc.EmptyMethods; | ||||||
private int _methodsRequiringSlotUnificationCount = 0; | ||||||
|
||||||
/// <summary> | ||||||
/// Custom enumerator struct for Unification group. Makes enumeration require 0 allocations. | ||||||
/// </summary> | ||||||
|
@@ -99,6 +102,42 @@ public Enumerator GetEnumerator() | |||||
return new Enumerator(_members); | ||||||
} | ||||||
|
||||||
public IEnumerable<MethodDesc> MethodsRequiringSlotUnification | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this no allocation by doing this:
Suggested change
With a matching: public readonly struct Enumerable
{
private readonly MethodDesc[] _arrayToEnumerate;
public Enumerable(MethodDesc[] arrayToEnumerate)
{
_arrayToEnumerate = arrayToEnumerate;
}
public Enumerator GetEnumerator()
{
return new Enumerator(_arrayToEnumerate);
}
} |
||||||
{ | ||||||
get | ||||||
{ | ||||||
foreach (var method in _methodsRequiringSlotUnification) | ||||||
{ | ||||||
if (method == null) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How could There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's Maybe this could use an |
||||||
yield break; | ||||||
yield return method; | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
public void AddMethodRequiringSlotUnification(MethodDesc method) | ||||||
{ | ||||||
if (RequiresSlotUnification(method)) | ||||||
return; | ||||||
|
||||||
_methodsRequiringSlotUnificationCount++; | ||||||
if (_methodsRequiringSlotUnificationCount >= _methodsRequiringSlotUnification.Length) | ||||||
{ | ||||||
Array.Resize(ref _methodsRequiringSlotUnification, Math.Max(_methodsRequiringSlotUnification.Length * 2, 2)); | ||||||
} | ||||||
_methodsRequiringSlotUnification[_methodsRequiringSlotUnificationCount - 1] = method; | ||||||
} | ||||||
|
||||||
public bool RequiresSlotUnification(MethodDesc method) | ||||||
{ | ||||||
for(int i = 0; i < _methodsRequiringSlotUnificationCount; i++) | ||||||
{ | ||||||
if (_methodsRequiringSlotUnification[i] == method) | ||||||
return true; | ||||||
} | ||||||
return false; | ||||||
} | ||||||
|
||||||
public void SetDefiningMethod(MethodDesc newDefiningMethod) | ||||||
{ | ||||||
// Do not change the defining method if its the same as | ||||||
|
@@ -395,6 +434,11 @@ private static void FindBaseUnificationGroup(MetadataType currentType, Unificati | |||||
MethodDesc methodImpl = FindImplFromDeclFromMethodImpls(currentType, unificationGroup.DefiningMethod); | ||||||
if (methodImpl != null) | ||||||
{ | ||||||
if(methodImpl.RequiresSlotUnification()) | ||||||
{ | ||||||
unificationGroup.AddMethodRequiringSlotUnification(unificationGroup.DefiningMethod); | ||||||
unificationGroup.AddMethodRequiringSlotUnification(methodImpl); | ||||||
} | ||||||
unificationGroup.SetDefiningMethod(methodImpl); | ||||||
} | ||||||
|
||||||
|
@@ -448,29 +492,68 @@ private static void FindBaseUnificationGroup(MetadataType currentType, Unificati | |||||
if (separatedMethods == null) | ||||||
separatedMethods = new MethodDescHashtable(); | ||||||
separatedMethods.AddOrGetExisting(declSlot); | ||||||
|
||||||
if (unificationGroup.RequiresSlotUnification(declSlot) || implSlot.RequiresSlotUnification()) | ||||||
{ | ||||||
if (implSlot.Signature.Equals(unificationGroup.DefiningMethod.Signature, true)) | ||||||
{ | ||||||
unificationGroup.AddMethodRequiringSlotUnification(declSlot); | ||||||
unificationGroup.AddMethodRequiringSlotUnification(implSlot); | ||||||
unificationGroup.SetDefiningMethod(implSlot); | ||||||
} | ||||||
} | ||||||
|
||||||
continue; | ||||||
} | ||||||
if (!unificationGroup.IsInGroupOrIsDefiningSlot(declSlot) && unificationGroup.IsInGroupOrIsDefiningSlot(implSlot)) | ||||||
if (!unificationGroup.IsInGroupOrIsDefiningSlot(declSlot)) | ||||||
{ | ||||||
// Add decl to group. | ||||||
if (unificationGroup.IsInGroupOrIsDefiningSlot(implSlot)) | ||||||
{ | ||||||
// Add decl to group. | ||||||
|
||||||
// To do so, we need to have the Unification Group of the decl slot, as it may have multiple members itself | ||||||
UnificationGroup addDeclGroup = new UnificationGroup(declSlot); | ||||||
FindBaseUnificationGroup(baseType, addDeclGroup); | ||||||
Debug.Assert(addDeclGroup.IsInGroupOrIsDefiningSlot(declSlot)); | ||||||
// To do so, we need to have the Unification Group of the decl slot, as it may have multiple members itself | ||||||
UnificationGroup addDeclGroup = new UnificationGroup(declSlot); | ||||||
FindBaseUnificationGroup(baseType, addDeclGroup); | ||||||
Debug.Assert( | ||||||
addDeclGroup.IsInGroupOrIsDefiningSlot(declSlot) || | ||||||
(addDeclGroup.RequiresSlotUnification(declSlot) && addDeclGroup.DefiningMethod.Signature.Equals(declSlot.Signature, true))); | ||||||
|
||||||
// Add all members from the decl's unification group except for ones that have been seperated by name/sig matches | ||||||
// or previously processed methodimpls. NOTE: This implies that method impls are order dependent. | ||||||
if (separatedMethods == null || !separatedMethods.Contains(addDeclGroup.DefiningMethod)) | ||||||
{ | ||||||
unificationGroup.AddToGroup(addDeclGroup.DefiningMethod); | ||||||
} | ||||||
foreach(MethodDesc methodImplRequiredToRemainInEffect in addDeclGroup.MethodsRequiringSlotUnification) | ||||||
{ | ||||||
unificationGroup.AddMethodRequiringSlotUnification(methodImplRequiredToRemainInEffect); | ||||||
} | ||||||
|
||||||
foreach (MethodDesc addDeclGroupMemberMethod in addDeclGroup) | ||||||
// Add all members from the decl's unification group except for ones that have been seperated by name/sig matches | ||||||
// or previously processed methodimpls. NOTE: This implies that method impls are order dependent. | ||||||
if (separatedMethods == null || !separatedMethods.Contains(addDeclGroup.DefiningMethod)) | ||||||
{ | ||||||
unificationGroup.AddToGroup(addDeclGroup.DefiningMethod); | ||||||
} | ||||||
|
||||||
foreach (MethodDesc addDeclGroupMemberMethod in addDeclGroup) | ||||||
{ | ||||||
if (separatedMethods == null || !separatedMethods.Contains(addDeclGroupMemberMethod)) | ||||||
{ | ||||||
unificationGroup.AddToGroup(addDeclGroupMemberMethod); | ||||||
} | ||||||
} | ||||||
|
||||||
if(unificationGroup.RequiresSlotUnification(declSlot)) | ||||||
{ | ||||||
unificationGroup.AddMethodRequiringSlotUnification(implSlot); | ||||||
} | ||||||
else if (implSlot == unificationGroup.DefiningMethod && implSlot.RequiresSlotUnification()) | ||||||
{ | ||||||
unificationGroup.AddMethodRequiringSlotUnification(declSlot); | ||||||
unificationGroup.AddMethodRequiringSlotUnification(implSlot); | ||||||
} | ||||||
} | ||||||
else if (unificationGroup.RequiresSlotUnification(declSlot)) | ||||||
{ | ||||||
if (separatedMethods == null || !separatedMethods.Contains(addDeclGroupMemberMethod)) | ||||||
if (implSlot.Signature.Equals(unificationGroup.DefiningMethod.Signature, true)) | ||||||
{ | ||||||
unificationGroup.AddToGroup(addDeclGroupMemberMethod); | ||||||
unificationGroup.AddMethodRequiringSlotUnification(implSlot); | ||||||
unificationGroup.SetDefiningMethod(implSlot); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,9 @@ public override string ToString() | |
{ | ||
var sb = new StringBuilder(); | ||
|
||
// (Skipping return type to keep things short) | ||
sb.Append(Signature.ReturnType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is left out on purpose because it's noisy and in the 99% case we don't care about the return type (same way it's left out on purpose in e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I can revert that. I was hoping this would provide better diagnostics, not just for covariant return methods, but also for cases where we could have methods with the same name/sig, but different return types. |
||
sb.Append(" "); | ||
|
||
sb.Append(OwningType); | ||
sb.Append('.'); | ||
sb.Append(DiagnosticName); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,7 +114,7 @@ public int Length | |
} | ||
} | ||
|
||
public bool Equals(MethodSignature otherSignature) | ||
public bool Equals(MethodSignature otherSignature, bool allowCovariantReturn = false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would kind of prefer a separate method for this ( It can pipe to a common private implementation, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with Michal. Equality is a symmetric relation ( In type theory, the relation between two types (in this case two function signatures) where one is more general than the other (for example because the return type of one function is a supertype of the return type of the other) is called "subsumption". That might be a good name to use here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds reasonable yes. |
||
{ | ||
// TODO: Generics, etc. | ||
if (this._flags != otherSignature._flags) | ||
|
@@ -124,7 +124,18 @@ public bool Equals(MethodSignature otherSignature) | |
return false; | ||
|
||
if (this._returnType != otherSignature._returnType) | ||
return false; | ||
{ | ||
if (!allowCovariantReturn) | ||
return false; | ||
|
||
// Casting a struct to an interface it implements is allowed, but these two types are not compatible and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The It also has interesting behaviors for
How does this feature work with methods returning pointers, byrefs, or function pointers? Can I e.g. override a method returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The feature should be able to extend to something like pointers (see ECMA 8.7.2), but these are all interesting corner cases that we should probably call out and clarify what is supported/not-supported, and see if we need to special case some of them from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add the ref return/pointer return cases to the TODO above so that it doesn't get lost? |
||
// can't be considered a valid covariant return type | ||
if (this._returnType.IsValueType && !otherSignature._returnType.IsValueType) | ||
return false; | ||
|
||
if (!this._returnType.CanCastTo(otherSignature._returnType)) | ||
return false; | ||
} | ||
|
||
if (this._parameters.Length != otherSignature._parameters.Length) | ||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -411,5 +411,29 @@ public static bool ContainsSignatureVariables(this TypeDesc thisType) | |
return false; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Check if MethodImpl requires slot unification. | ||
/// </summary> | ||
/// <param name="method">Method to check</param> | ||
/// <returns>True when the method is a MethodImpl marked with the RequireMethodImplToRemainInEffect custom attribute, false otherwise.</returns> | ||
public static bool RequiresSlotUnification(this MethodDesc method) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any perf numbers for this? I think this is going to make virtual method resolution even slower than it already is. I think we we'll need to burn a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have not measured perf |
||
{ | ||
if (!method.HasCustomAttribute("System.Runtime.CompilerServices", "RequireMethodImplToRemainInEffectAttribute")) | ||
return false; | ||
|
||
MetadataType mdType = method.OwningType as MetadataType; | ||
if (mdType != null) | ||
{ | ||
foreach (MethodImplRecord methodImplRecord in mdType.VirtualMethodImplsForType) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would Roslyn emit this attribute on something that is not a MethodImpl? Why do we need to filter those out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roslyn will not, but this is about being on par with some of the verification on the coreclr Typesystem side. However, the virtual resolution algorithm should not invoke this for non-MethodImpls, so we can just convert this into a debug-only check/assert, or completely remove it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My vote would be to remove it then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of removing it, but let's do that closer to merging. It's useful to have this check for now during development |
||
{ | ||
// Method is a MethodImpl body record with the attribute | ||
if (method == methodImplRecord.Body) | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a chapter on this to https://github.com/dotnet/runtime/blob/master/docs/design/specs/Ecma-335-Augments.md ? Try to match the style and language used by ECMA-335 (e.g. in the ideal case - we would just copy&paste it over once we figure out how to make edits to ECMA-335).