-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Check partial method return type compatibility #45128
Changes from all commits
b66f235
550f8c5
e513785
608ed59
a98d022
3aea65a
3015415
debcedf
7be99fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1149,6 +1149,20 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S | |
{ | ||
Debug.Assert(!ReferenceEquals(definition, implementation)); | ||
|
||
MethodSymbol constructedDefinition = definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations); | ||
bool returnTypesEqual = constructedDefinition.ReturnTypeWithAnnotations.Equals(implementation.ReturnTypeWithAnnotations, TypeCompareKind.AllIgnoreOptions); | ||
if (!returnTypesEqual | ||
&& !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(implementation.ReturnType) | ||
&& !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(definition.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. If the types are distinct, why not report that, regardless of whether the types contain errors? 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 based on patterns I saw in 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, but it doesn't seem like a cascading error to me though because the types are different. In reply to: 442526990 [](ancestors = 442526990) |
||
{ | ||
diagnostics.Add(ErrorCode.ERR_PartialMethodReturnTypeDifference, implementation.Locations[0]); | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (definition.RefKind != implementation.RefKind) | ||
{ | ||
diagnostics.Add(ErrorCode.ERR_PartialMethodRefReturnDifference, implementation.Locations[0]); | ||
} | ||
|
||
if (definition.IsStatic != implementation.IsStatic) | ||
{ | ||
diagnostics.Add(ErrorCode.ERR_PartialMethodStaticDifference, implementation.Locations[0]); | ||
|
@@ -1192,15 +1206,22 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S | |
|
||
SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride( | ||
implementation.DeclaringCompilation, | ||
definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations), | ||
constructedDefinition, | ||
implementation, | ||
diagnostics, | ||
reportMismatchInReturnType: null, | ||
(diagnostics, implementedMethod, implementingMethod, topLevel, returnTypesEqual) => | ||
{ | ||
if (returnTypesEqual) | ||
{ | ||
// report only if this is an unsafe *nullability* difference | ||
diagnostics.Add(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnPartial, implementingMethod.Locations[0]); | ||
} | ||
}, | ||
(diagnostics, implementedMethod, implementingMethod, implementingParameter, blameAttributes, arg) => | ||
{ | ||
diagnostics.Add(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnPartial, implementingMethod.Locations[0], new FormattedSymbol(implementingParameter, SymbolDisplayFormat.ShortFormat)); | ||
}, | ||
extraArgument: (object)null); | ||
extraArgument: returnTypesEqual); | ||
} | ||
|
||
private static void PartialMethodConstraintsChecks(SourceOrdinaryMethodSymbol definition, SourceOrdinaryMethodSymbol implementation, 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.
Why did you choose this over
CLRSignatureCompareOptions
?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.
Hmm the
TypeCompareKind
here appears to be different than how we verify parameters. For example tuple element names matter in parameter names. Unless there is a good reason otherwise think we should use the same comparisons for return type and parameters.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 used
SourceMemberContainerSymbol.CheckOverrideMember
as a basis for this code:roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs
Line 939 in 85b65a4
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.
Think that makes sense in overrides because:
dynamic
vs.object
but did error forIntPtr
vs.nint
.dynamic
in their code base because it makes sense while in the base caseobject
was the more natural choice. Forcing them to useobject
in the override would just result in them adding a bunch of unnecessary casts.For this feature neither is a concern. This is a new feature so compat doesn't come into play and the developer controls both definitions.
Happy to hear if others disagree but my instinct is to say that parameters and return types should have the same comparisons done here.
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 may have misunderstood your comment, but it looks like CLRSignatureCompareOptions does not detect a difference between
object
/dynamic
orIntPtr
/nint
. It looks like constructing a scenario which requires CLRSignatureCompareOptions involves custom modifiers, array sizes or lower bounds, which I am not sure how to do in source. Do you have any ideas for scenarios which illustrate the need forCLRSignatureCompareOptions
?It feels like this is also the case to a significant degree when one of the 'partial' declarations comes from a code generator.
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.
The suggestion of
CLRSignatureCmopareOptions
was just an example, didn't mean to suggest it should be used.True but it is also something that it's reasonable for the source generator to replicate. Consider that if they don't replicate what the user wrote, particularly the tuple element names, then they're subverting the will of the developer and making their generated code much harder to consume.
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 applies when the source generator produces an implementing declaration: a well-behaved generator should just copy the signature of the user-authored defining declaration. But in the case where the generator produces a defining declaration and the user authors an implementing declaration, the user has to align with a generator that they likely do not control. Still, it's hard to picture a situation where a user is inconvenienced by being unable to substitute 'dynamic' for 'object' or 'IntPtr' for 'nint' or vice versa.
Probably the main reason to allow such differences for return types is probably that 'object'/'dynamic' etc. differences are currently allowed for parameters, so to only disallow them for returns is inconsistent.