-
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
Conversation
PTAL @cston @dotnet/roslyn-compiler |
constructedDefinition = definition.Construct(implementation.TypeArgumentsWithAnnotations); | ||
} | ||
|
||
bool returnTypesEqual = constructedDefinition.ReturnTypeWithAnnotations.Equals(implementation.ReturnTypeWithAnnotations, TypeCompareKind.AllIgnoreOptions); |
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:
Line 939 in 85b65a4
if (overridingMethod.IsGenericMethod) |
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:
- It's historically what we did so pretty much had to maintain it for consistency when adding new features. Basically it would be weird if we didn't error for
dynamic
vs.object
but did error forIntPtr
vs.nint
. - It's expected that the developer doesn't control the signature of the base member. Hence we should give them the flexbility to use
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
or IntPtr
/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 for CLRSignatureCompareOptions
?
It's expected that the developer doesn't control the signature of the base member.
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.
I may have misunderstood your comment, but it looks like CLRSignatureCompareOptions does not detect a difference between object/dynamic or IntPtr/nint.
The suggestion of CLRSignatureCmopareOptions
was just an example, didn't mean to suggest it should be used.
It feels like this is also the case to a significant degree when one of the 'partial' declarations comes from a code generator.
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.
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.
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.
public partial string M1() => ""hello""; | ||
|
||
public partial IEnumerable<string?> M2(); | ||
public partial IEnumerable<string> M2() => 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.
Shouldn't the difference in nullability be an error? Perhaps we could allow differences when one annotated partial is defined in a #nullable enable
context and the other unannotated partial is defined in #nullable disable
but allowing other cases seems incorrect to me. #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.
The partial method checks on parameters allow for "safe" nullable variance.
partial void M(string s);
partial void M(string? s) { } // ok
partial void M(string? s);
partial void M(string s) { } // warning
partial void M(IEnumerable<string> s);
partial void M(IEnumerable<string?> s) { } // ok
partial void M(IEnumerable<string?> s);
partial void M(IEnumerable<string> s) { } // warning
I chose an analogous behavior on return types in order to be consistent with this.
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 consider testing all pairs of:
#nullable enable
partial object F();
partial object? F();
#nullable disable
partial object F();
partial object? F();
In reply to: 441084726 [](ancestors = 441084726)
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'm not sure I understand this suggestion. There are only defining declarations in this sample. #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.
Sorry for the confusion. I was suggesting pairs of those signatures where one is a partial method declaration and the other is a partial method implementation.
In reply to: 441120309 [](ancestors = 441120309)
}"; | ||
var comp = CreateCompilation(source, parseOptions: TestOptions.RegularWithExtendedPartialMethods); | ||
comp.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.
} [](start = 8, length = 1)
Is there a reason to support differences in dynamic
/ object
, or nint
/ System.IntPtr
?
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 think we will try to answer this question here: #45128 (comment)
Yeah it looks like I picked the exact wrong example to test out if that was the right value or not 😦 |
} | ||
|
||
[Fact, WorkItem(44930, "https://github.com/dotnet/roslyn/issues/44930")] | ||
public void DifferentReturnTypes_15() |
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 blocking but I think a lot of these tests wheree there is no difference could be more succinctly expressed as a [Theory]
with the equivalent types expressed as arguments.
|
||
public partial nint M2(); | ||
public partial IntPtr M2() => default; | ||
}"; |
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.
} [](start = 0, length = 1)
I think we should report an error for differences in native integers and underlying types.
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.
Filed a follow-up issue.
src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
Show resolved
Hide resolved
@@ -1195,12 +1214,19 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S | |||
definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations), |
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.
definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations) [](start = 16, length = 74)
constructedDefinition
? #Closed
if (implementation.IsGenericMethod) | ||
{ | ||
constructedDefinition = definition.Construct(implementation.TypeArgumentsWithAnnotations); | ||
} |
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.
= definition.ConstructIfGeneric(...)
}"; | ||
var comp = CreateCompilation(source, parseOptions: TestOptions.RegularWithExtendedPartialMethods); | ||
comp.VerifyDiagnostics( | ||
// (8,28): error CS8818: Nullability of reference types in return type doesn't match partial method declaration. |
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.
error [](start = 27, length = 5)
Consider updating comments so it's clear this is a "warning". #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.
funny, I guess I created these baselines before filling out some of the boilerplate to make the diagnostic a warning.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on patterns I saw in CheckOverrideMember
, it seemed like reducing the cascading errors was best.
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 seemed like reducing the cascading errors was best.
Ok, but it doesn't seem like a cascading error to me though because the types are different.
In reply to: 442526990 [](ancestors = 442526990)
}"; | ||
var comp = CreateCompilation(source, parseOptions: TestOptions.RegularWithExtendedPartialMethods); | ||
comp.VerifyDiagnostics( | ||
// (5,37): error CS8818: Both partial method declarations must return by reference or neither may return by reference. |
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.
Both partial method declarations must return by reference or neither may return by reference. [](start = 41, length = 93)
The message seems a little misleading since both are returning by reference. #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.
True however I struggled with coming up with a higher quality message. Maybe "Both partial method declarations must have equivalent 'ref' or 'ref readonly' modifiers"..? #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.
Maybe "Both partial method declarations must have equivalent 'ref' or 'ref readonly' modifiers"
Sounds good. Perhaps strengthen it to "... must have matching 'ref' or 'ref readonly' modifiers."
In reply to: 442527921 [](ancestors = 442527921)
Closes #44930
Related to #43795