-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 part1 #43576
Covariant returns part1 #43576
Conversation
…e language version. Note that we now consider a get accessor with the wrong return type to still be an override. Issue language version suggestion when covariant returns feature would permit the override.
@dotnet/roslyn-compiler May I please have a couple of reviews of this first step in the implementation of covariant returns for overrides? |
…t/roslyn into covariant-returns-part1
@@ -155,12 +155,11 @@ internal class MemberSignatureComparer : IEqualityComparer<Symbol> | |||
/// <summary> | |||
/// This instance is used to check whether one property or event overrides another, according to the C# definition. | |||
/// <para>NOTE: C# ignores accessor member names.</para> | |||
/// <para>CAVEAT: considers return types so that getters and setters will be treated the same.</para> | |||
/// </summary> | |||
public static readonly MemberSignatureComparer CSharpAccessorOverrideComparer = new MemberSignatureComparer( |
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.
CSharpAccessorOverrideComparer [](start = 55, length = 30)
Looking at the call sites that use this comparer, I see that they also use RuntimeSignatureComparer
(for example in MakePropertyAccessorOverriddenOrHiddenMembers
depending on accessorIsFromSomeCompilation
).
Should RuntimeSignatureComparer
also be adjusted?
Also, I notice that this PR hasn't updated MakePropertyAccessorOverriddenOrHiddenMembers
. That means it is using a relaxed comparison, which is not constrained by new covariance rules.
Oh, I see that's what you meant by "Note that we now consider a get accessor with the wrong return type to still be an override.". Should probably be marked as PROTOTYPE #Closed
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.
We are not changing the runtime's rules for implicit overriding. It includes the return type. The language's concept doesn't. So no change to the other comparer is needed.
The "relaxed" comparison is the C# language rules. This is not a temporary PROTOTYPE. It is the new rules.
In reply to: 413964273 [](ancestors = 413964273)
/// </summary> | ||
public static readonly MemberSignatureComparer CSharpAccessorOverrideComparer = new MemberSignatureComparer( | ||
considerName: false, | ||
considerExplicitlyImplementedInterfaces: false, //Bug: DevDiv #15775 | ||
considerReturnType: true, | ||
considerReturnType: 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.
I see that this comparer is used for properties/indexers and also events. Can you add tests for event accessors? #Closed
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.
Sure, but events have an add accessor which will prevent overriding with a different return type.
In reply to: 413965442 [](ancestors = 413965442)
CreateCompilation(source).VerifyDiagnostics( | ||
// (9,23): error CS8652: The feature 'covariant returns' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. | ||
// public override T M() => null; | ||
Diagnostic(ErrorCode.ERR_FeatureInPreview, "M").WithArguments("covariant returns").WithLocation(9, 23) |
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.
ERR_FeatureInPreview [](start = 37, length = 20)
Please add LangVer tests for other overrides scenarios (not just methods). #Closed
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.
This already tests methods, properties and indexers. Will add events (for which no change of permitted behavior is planned).
In reply to: 413972645 [](ancestors = 413972645)
} | ||
class Derived : Base | ||
{ | ||
public override string M => null; |
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.
(All test suggestions below can be tracked/PROTOTYPE as needed)
- What is expected for
public Derived : Base { public new string M => null; }
scenario? - What is expected for
public Derived : Base { public string M => null; }
scenario? (I assume we still produce a warning about hiding) - What is expected for
public Derived2: Derived { public new object/string M => null; }
scenarios? - Please add a test where
Base
has a property,Derived
hides it with a different return type, andDerived2
tries to override with either return type. - These are also applicable to virtual methods.
- I assume that an abstract method can be implemented with a covariant return. Please add a test.
- Are interface implementations in scope for this feature and/or PR?
- Please add a test with nested variance involved (returning
CIn<object>
vs.CIn<string>
, orCOut<object> vs.
COut). Also consider nullability variance (
COut<object?>vs.
COut<string!>` and some permutations). - Test with an override that doesn't have an implicit reference conversion from base. For instance, numeric types, types convertible via user-defined operators, etc.
- Test with wrong variance in override (
Base.Method()
returnsstring
butoverride Derived.Method()
returnsobject
) - I assume that we can't emit or run yet. If that's not the case, then let's add some emit and execution tests
- low-pri: Test other implicit reference conversion scenarios, such as
Interface Base.Method()
andTypeThatImplementsInterface Derived.Method()
, to lock-in the proper check - test some DIM scenarios #Closed
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.
(7): This is for override
only, thereby excluding interfaces.
Will add comments for suggested tests. Many of these are existing tests elsewhere in our test suite, and since the diagnostics have not changed (i.e. not suggesting upgrade to language version that supports covariant returns) that is evidence that the behavior has not changed.
In reply to: 413973483 [](ancestors = 413973483)
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.
We should also have a test with at least three levels:
class A
{
public virtual object M() => null!;
}
class B : A
{
public override string M() => null!;
}
class C : B
{
public override object M() => null!; //error
}
class D : C
{
public override string M() => null!; // probably fine
}
``` #Resolved
@@ -701,8 +701,11 @@ private void CheckNewModifier(Symbol symbol, bool isNew, DiagnosticBag diagnosti | |||
} | |||
} | |||
|
|||
private static void CheckOverrideMember(Symbol overridingMember, OverriddenOrHiddenMembersResult overriddenOrHiddenMembers, | |||
DiagnosticBag diagnostics, out bool suppressAccessors) | |||
private void CheckOverrideMember( |
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.
Should this PR target the features/records
branch?
FYI, I am doing a refactoring of this method (simple breaking out of this massive and deeply nested method) in the records branch 68426e7
If covariant returns needs a dedicated feature branch, then consider cherry-picking that refactoring. #Closed
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.
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.
From standup meeting today, seems like folks are leaning towards using a separate branch for now. #Closed
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.
Your delta is in a private branch and not yet integrated. I don't see an advantage of merging now and hoping you don't change things further. I know I will be changing things further. Will deal with the conflicts later.
In reply to: 414044555 [](ancestors = 414044555,413984024)
/// <summary> | ||
/// Return true if <paramref name="overridingReturnType"/> is valid for the return type of an override method when the overridden method's return type is <paramref name="overriddenReturnType"/>. | ||
/// </summary> | ||
private bool IsValidOverrideReturnType(Symbol overridingSymbol, TypeWithAnnotations overridingReturnType, TypeWithAnnotations overriddenReturnType, DiagnosticBag diagnostics) |
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.
nit: could be a local function #Closed
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.
if (useSiteDiagnostics != null) | ||
{ | ||
Location symbolLocation = overridingSymbol.Locations.FirstOrDefault(); | ||
diagnostics.Add(symbolLocation, useSiteDiagnostics); |
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.
Please add a test for this code path (ie. with a use-site diagnostic from conversion) #Closed
/// <summary> | ||
/// Return true if <paramref name="overridingReturnType"/> is valid for the return type of an override method when the overridden method's return type is <paramref name="overriddenReturnType"/>. | ||
/// </summary> | ||
private bool IsValidOverrideReturnType(Symbol overridingSymbol, TypeWithAnnotations overridingReturnType, TypeWithAnnotations overriddenReturnType, DiagnosticBag diagnostics) |
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.
IsValidOverrideReturnType [](start = 21, length = 25)
I'm curious why you structured this method this way. I would have expected:
- check if types are equal, if so, then return true
- check if the types are convertible, if so do the version check and report LangVer error here (so it doesn't have to be duplicated at various call sites)
The benefit of this approach is that it keeps the binding from depending on LangVer, and we don't have to duplicate the HasIdentityOrImplicitReferenceConversion
check. We can verify that in testing the semantic model in error cases. #Closed
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.
Not all places where this is called deserve a language version diagnostic. For example, that probably isn't apropos for events.
I'm not concerned with an extra check in the case of an error, since errors such as this occur only rarely as an intermediate state during source development.
In reply to: 414000465 [](ancestors = 414000465)
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.
Okay.
I'm still concerned that we're binding differently based on LangVer and that will be visible via symbols from semantic model.
In reply to: 414047588 [](ancestors = 414047588,414000465)
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.
Verifying the symbol and semantic models is a future work item.
In reply to: 414049357 [](ancestors = 414049357,414047588,414000465)
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.
Sorry to insist, but I don't feel good about punting because we can see there is an issue here (should not bind differently depending on LangVer). Let's avoid introducing this problem, rather than come back and fix it later.
In reply to: 414052536 [](ancestors = 414052536,414049357,414047588,414000465)
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.
This doesn't affect binding. It only affects what diagnostics are produced. An override method with the wrong return type has always been an override of the base method. That is not changing. What is changing is when an error is produced.
In reply to: 414106079 [](ancestors = 414106079,414052536,414049357,414047588,414000465)
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.
Ah, thanks! :-) #Closed
@@ -27,6 +27,8 @@ public static class TestOptions | |||
public static readonly CSharpParseOptions RegularWithDocumentationComments = Regular.WithDocumentationMode(DocumentationMode.Diagnose); | |||
public static readonly CSharpParseOptions RegularWithLegacyStrongName = Regular.WithFeature("UseLegacyStrongNameProvider"); | |||
public static readonly CSharpParseOptions WithoutImprovedOverloadCandidates = Regular.WithLanguageVersion(MessageID.IDS_FeatureImprovedOverloadCandidates.RequiredVersion() - 1); | |||
public static readonly CSharpParseOptions WithCovariantReturns = Regular.WithLanguageVersion(MessageID.IDS_FeatureCovariantReturnsForOverrides.RequiredVersion()); | |||
public static readonly CSharpParseOptions WithoutCovariantReturns = Regular.WithLanguageVersion(LanguageVersion.CSharp8); |
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.
WithoutCovariantReturns [](start = 50, length = 23)
seems unused #Closed
public override string M() => null; | ||
} | ||
"; | ||
CreateCompilation(source).VerifyDiagnostics( |
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.
CreateCompilation [](start = 12, length = 17)
Would be safer to specify LangVersion 8, rather than rely on default #Closed
/// </summary> | ||
private bool IsValidOverrideReturnType(Symbol overridingSymbol, TypeWithAnnotations overridingReturnType, TypeWithAnnotations overriddenReturnType, DiagnosticBag diagnostics) | ||
{ | ||
if (DeclaringCompilation.LanguageVersion >= MessageID.IDS_FeatureCovariantReturnsForOverrides.RequiredVersion()) |
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.
IDS_FeatureCovariantReturnsForOverrides [](start = 66, length = 39)
Just to confirm, I think we can assume that language version implies newer platform (ie. one that supports covariant returns). If not, we should add a platform check as well #Closed
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.
We do not support emitting metadata (targeting any plaform) yet. When we do, I'll add a check if apropos.
In reply to: 414007929 [](ancestors = 414007929)
if (DeclaringCompilation.LanguageVersion >= MessageID.IDS_FeatureCovariantReturnsForOverrides.RequiredVersion()) | ||
{ | ||
HashSet<DiagnosticInfo> useSiteDiagnostics = null; | ||
var result = DeclaringCompilation.Conversions.HasIdentityOrImplicitReferenceConversion(overridingReturnType.Type, overriddenReturnType.Type, ref useSiteDiagnostics); |
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.
HasIdentityOrImplicitReferenceConversion [](start = 62, length = 40)
This would allow overriding with a derived interface, no?
From the runtime PR, I see that's not going to be allowed: "Return types in covariant return methods can only be reference types: covariant interface return types are not supported." #Resolved
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.
It can be any implicit reference conversion. What is not supported is covariant overrides of methods declared in interfaces.
In reply to: 414047290 [](ancestors = 414047290)
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.
Should the description in runtime PR be corrected then?
In reply to: 414049729 [](ancestors = 414049729,414047290)
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.
I've started an email thread about that question.
In reply to: 414050605 [](ancestors = 414050605,414049729,414047290)
diagnostics.Add(ErrorCode.ERR_CantChangeReturnTypeOnOverride, overridingMemberLocation, overridingMember, overriddenMember, overriddenMethod.ReturnType); | ||
// If the return type would be a valid covariant return, suggest using covariant return feature. | ||
HashSet<DiagnosticInfo> discardedUseSiteDiagnostics = null; | ||
if (DeclaringCompilation.Conversions.HasIdentityOrImplicitReferenceConversion(overridingMethod.ReturnTypeWithAnnotations.Type, overriddenMethod.ReturnTypeWithAnnotations.Type, ref discardedUseSiteDiagnostics)) |
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.
This seems strange. Shouldn't we check if it's valid, including a covariant return, then check if the feature is enabled and, if it isn't and the two types are not equal, produce an error? #ByDesign
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.
I'd rather keep the error case separate. rather than blend the code for the correct case and the error case.
In reply to: 414069899 [](ancestors = 414069899)
else | ||
{ | ||
// error CS0508: return type must be 'C<V>' to match overridden member 'M<T>()' | ||
diagnostics.Add(ErrorCode.ERR_CantChangeReturnTypeOnOverride, overridingMemberLocation, overridingMember, overriddenMember, overriddenMethod.ReturnType); |
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.
We should either never produce this error message any more, or we should change the text to match the new language design #Resolved
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.
This diagnostic is apropos for non-reference types or sealed reference types or in older language versions. I will add a prototype comment to adjust for inheritable reference types.
In reply to: 414071529 [](ancestors = 414071529)
} | ||
class Derived : Base | ||
{ | ||
public override string M => null; |
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.
We should also have a test with at least three levels:
class A
{
public virtual object M() => null!;
}
class B : A
{
public override string M() => null!;
}
class C : B
{
public override object M() => null!; //error
}
class D : C
{
public override string M() => null!; // probably fine
}
``` #Resolved
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.
LGTM Thanks (iteration 5).
Please check the doc in runtime PR, as it says a "Return types in covariant return methods can only be reference types: covariant interface return types are not supported." but this PR allows overriding with a derived interface as return type.
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.
LGTM
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.
Auto-approval
…t/roslyn into covariant-returns-part1
@gafter I am planning to take a look at this PR today #Closed |
@AlekseyTs Your comments are most welcome. Please record test suggestions in #43188. I'm going to integrate this PR so I can submit for review other PRs that build upon it. #Resolved |
diagnostics.Add(ErrorCode.ERR_CantChangeReturnTypeOnOverride, overridingMemberLocation, overridingMember, overriddenMember, overriddenMethod.ReturnType); | ||
// If the return type would be a valid covariant return, suggest using covariant return feature. | ||
HashSet<DiagnosticInfo> discardedUseSiteDiagnostics = null; | ||
if (DeclaringCompilation.Conversions.HasIdentityOrImplicitReferenceConversion(overridingMethod.ReturnTypeWithAnnotations.Type, overriddenMethod.ReturnTypeWithAnnotations.Type, ref discardedUseSiteDiagnostics)) |
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.
discardedUseSiteDiagnostics [](start = 216, length = 27)
I do not think we can discard use-site diagnostics for this scenario. It is quite possible that a missing reference is the root cause of the failure and we should report that. Also, we should track dependencies for success cases, this feature is still in a separate feature brunch, but each feature should be developed with it in mind so that merge goes as smooth as possible later. #Closed
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.
I see that this check duplicates similar check in IsValidOverrideReturnType
and in there we do not discard use-site diagnostics. So, reporting it here would be redundant.
In reply to: 416151997 [](ancestors = 416151997)
@@ -877,7 +880,7 @@ private void CheckNewModifier(Symbol symbol, bool isNew, DiagnosticBag diagnosti | |||
diagnostics.Add(ErrorCode.ERR_CantChangeRefReturnOnOverride, overridingMemberLocation, overridingMember, overriddenMember); | |||
suppressAccessors = true; //we get really unhelpful errors from the accessor if the ref kind is mismatched | |||
} | |||
else if (!overridingMemberType.Equals(overriddenMemberType, TypeCompareKind.AllIgnoreOptions)) | |||
else if (!IsValidOverrideReturnType(overridingProperty, overridingMemberType, overriddenMemberType, diagnostics)) | |||
{ | |||
// if the type is or contains an error type, the type must be fixed before the override can be found, so suppress error | |||
if (!isOrContainsErrorType(overridingMemberType.Type)) |
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.
if (!isOrContainsErrorType(overridingMemberType.Type)) [](start = 32, length = 54)
It looks like we are not reporting language version mismatch on this code path. Is the assumption that this would be covered by the getter? Consider adding a comment about that. #Closed
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.
} | ||
"; | ||
CreateCompilation(source, parseOptions: TestOptions.WithoutCovariantReturns).VerifyDiagnostics( | ||
// (8,28): error CS1715: 'Derived.M': type must be 'object' to match overridden member 'Base.M' |
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.
// (8,28): error CS1715: 'Derived.M': type must be 'object' to match overridden member 'Base.M' [](start = 16, length = 95)
Are we not supporting covariant returns for properties? Otherwise why there is a difference by comparison to CovariantReturns_01 scenario? #Closed
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.
This actually looks like a bug since we report no error for the ```TestOptions.WithCovariantReturns```` scenario. We want to have a hint that switching to a different language version is a way out.
In reply to: 416167876 [](ancestors = 416167876)
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.
var source = @" | ||
class Base | ||
{ | ||
public virtual object M => null; |
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.
object M => null; [](start = 19, length = 17)
I think it would be interesting to test the following scenarios:
- an attempt to override a get/set property with a get-set property
- an attempt to override a get/set property with a get-only property
- similar as the second case only with next level derived type attempting to override with get/set or set-only property
- similar as the second case only with next level derived type attempting to override with more specific get-only property
#Resolved
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.
I ended up providing comments on this PR, even though it has been merged. This makes it easier to have a discussion and the context for it. I think the comments are actionable, please respond. #Closed |
Relates to #43188 (test plan for covariant returns)