Covariant returns part1 #43576
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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> | ||
| /// </summary> | ||
| public static readonly MemberSignatureComparer CSharpAccessorOverrideComparer = new MemberSignatureComparer( | ||
| considerName: false, | ||
| considerExplicitlyImplementedInterfaces: false, //Bug: DevDiv #15775 | ||
| considerReturnType: false, | ||
|
There was a problem hiding this comment. 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. Sure, but events have an add accessor which will prevent overriding with a different return type. In reply to: 413965442 [](ancestors = 413965442) |
||
| considerTypeConstraints: false, | ||
| considerCallingConvention: false, //ignore static-ness | ||
| considerRefKindDifferences: true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -701,8 +701,11 @@ private void CheckNewModifier(Symbol symbol, bool isNew, DiagnosticBag diagnosti | |
| } | ||
| } | ||
|
|
||
| private void CheckOverrideMember( | ||
|
There was a problem hiding this comment. Should this PR target the There was a problem hiding this comment. There was a problem hiding this comment. From standup meeting today, seems like folks are leaning towards using a separate branch for now. #Closed There was a problem hiding this comment. 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) |
||
| Symbol overridingMember, | ||
| OverriddenOrHiddenMembersResult overriddenOrHiddenMembers, | ||
| DiagnosticBag diagnostics, | ||
| out bool suppressAccessors) | ||
| { | ||
| Debug.Assert((object)overridingMember != null); | ||
| Debug.Assert(overriddenOrHiddenMembers != null); | ||
|
|
@@ -877,7 +880,7 @@ private static void CheckOverrideMember(Symbol overridingMember, OverriddenOrHid | |
| 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 (!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.
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 |
||
|
|
@@ -981,13 +984,24 @@ private static void CheckOverrideMember(Symbol overridingMember, OverriddenOrHid | |
| { | ||
| diagnostics.Add(ErrorCode.ERR_CantChangeRefReturnOnOverride, overridingMemberLocation, overridingMember, overriddenMember); | ||
| } | ||
| else if (!IsValidOverrideReturnType(overridingMethod, overridingMethod.ReturnTypeWithAnnotations, overriddenMethod.ReturnTypeWithAnnotations, diagnostics)) | ||
| { | ||
| // if the Return type is or contains an error type, the return type must be fixed before the override can be found, so suppress error | ||
| if (!isOrContainsErrorType(overridingMethod.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. 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. 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) There was a problem hiding this comment.
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. I see that this check duplicates similar check in In reply to: 416151997 [](ancestors = 416151997) |
||
| { | ||
| var diagnosticInfo = MessageID.IDS_FeatureCovariantReturnsForOverrides.GetFeatureAvailabilityDiagnosticInfo(this.DeclaringCompilation); | ||
| Debug.Assert(diagnosticInfo is { }); | ||
| diagnostics.Add(diagnosticInfo, overridingMemberLocation); | ||
| } | ||
| 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. 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. 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) |
||
| } | ||
| } | ||
| } | ||
| else if (overriddenMethod.IsRuntimeFinalizer()) | ||
|
|
@@ -1058,6 +1072,29 @@ static void checkValidNullableMethodOverride( | |
| } | ||
| } | ||
|
|
||
| /// <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. nit: could be a local function #Closed There was a problem hiding this comment. There was a problem hiding this comment.
I'm curious why you structured this method this way. I would have expected:
The benefit of this approach is that it keeps the binding from depending on LangVer, and we don't have to duplicate the There was a problem hiding this comment. 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. Okay. In reply to: 414047588 [](ancestors = 414047588,414000465) There was a problem hiding this comment. 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. 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. 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) |
||
| { | ||
| if (DeclaringCompilation.LanguageVersion >= MessageID.IDS_FeatureCovariantReturnsForOverrides.RequiredVersion()) | ||
|
There was a problem hiding this comment.
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. 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) |
||
| { | ||
| HashSet<DiagnosticInfo> useSiteDiagnostics = null; | ||
| var result = DeclaringCompilation.Conversions.HasIdentityOrImplicitReferenceConversion(overridingReturnType.Type, overriddenReturnType.Type, ref useSiteDiagnostics); | ||
|
There was a problem hiding this comment.
This would allow overriding with a derived interface, no? There was a problem hiding this comment. 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. Should the description in runtime PR be corrected then? In reply to: 414049729 [](ancestors = 414049729,414047290) There was a problem hiding this comment. I've started an email thread about that question. In reply to: 414050605 [](ancestors = 414050605,414049729,414047290) |
||
| if (useSiteDiagnostics != null) | ||
| { | ||
| Location symbolLocation = overridingSymbol.Locations.FirstOrDefault(); | ||
| diagnostics.Add(symbolLocation, useSiteDiagnostics); | ||
|
There was a problem hiding this comment. Please add a test for this code path (ie. with a use-site diagnostic from conversion) #Closed |
||
| } | ||
|
|
||
| return result; | ||
| } | ||
| else | ||
| { | ||
| return overridingReturnType.Equals(overriddenReturnType, TypeCompareKind.AllIgnoreOptions); | ||
| } | ||
| } | ||
|
|
||
| static readonly ReportMismatchInReturnType<Location> ReportBadReturn = | ||
| (DiagnosticBag diagnostics, MethodSymbol overriddenMethod, MethodSymbol overridingMethod, bool topLevel, Location location) | ||
| => diagnostics.Add(topLevel ? | ||
|
|
||
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.
Looking at the call sites that use this comparer, I see that they also use
RuntimeSignatureComparer(for example inMakePropertyAccessorOverriddenOrHiddenMembersdepending onaccessorIsFromSomeCompilation).Should
RuntimeSignatureCompareralso 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)