Require partial method signatures to match #47576
Conversation
src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs
Outdated
Show resolved
Hide resolved
| // (24,28): error CS8824: Partial method declarations 'string C.M10()' and 'string? C.M10()' must have identical parameter types and identical return types. | ||
| // public partial string? M10() => null; | ||
| Diagnostic(ErrorCode.ERR_PartialMethodSignatureDifference, "M10").WithArguments("string C.M10()", "string? C.M10()").WithLocation(24, 28), | ||
| // (29,27): error CS8824: Partial method declarations 'string C.M5()' and 'string C.M5()' must have identical parameter types and identical return types. |
There was a problem hiding this comment.
As a user I would be very confused if I saw this diagnostic. Since many code generators have nullability disabled, this may be a scenario that matters. It's not clear to me if code generators which adopt extended partial methods are also expected to adopt nullability in sync with their users.
I think we may want oblivious nullability to compare "equal" to annotated and to not-annotated nullability for purposes of partial method checks. One thing that may lack tests here currently is when the declaration part is oblivious and the implementation part is nullable-enabled. (sorry, I misread the test, all is well.)
#Resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
We decided in LDM to keep most warnings/errors the way they are, and to add a warning wave warning to "any difference between partial methods, except for nullable/oblivious differences". https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-09-14.md#partial-method-signature-matching
|
Done with review pass (iteration 3) |
| diagnostics.Add(ErrorCode.ERR_PartialMethodReturnTypeDifference, implementation.Locations[0]); | ||
| } | ||
|
|
||
| if (definition.RefKind != implementation.RefKind) | ||
| else if (definition.RefKind != implementation.RefKind) |
There was a problem hiding this comment.
else [](start = 12, length = 4)
nit: I think it was okay to cascade reporting an error on the return type and the ref return differences. The user needs to fix both... #Resolved
| ErrorCode.ERR_PartialMethodNullabilityDifference : | ||
| ErrorCode.ERR_PartialMethodTypeDifference; | ||
| diagnostics.Add(errorCode, implementation.Locations[0], getFormattedSymbol(definition), getFormattedSymbol(implementation)); | ||
| static FormattedSymbol getFormattedSymbol(Symbol symbol) => new FormattedSymbol(symbol, SymbolDisplayFormat.MinimallyQualifiedFormat); |
There was a problem hiding this comment.
nit: consider inlining to avoid a local function in middle of method. Also doesn't save on characters. #Resolved
| else | ||
| { | ||
| var errorCode = definition.HasExplicitAccessModifier && MemberSignatureComparer.ExtendedPartialMethodsStrictIgnoreNullabilityComparer.Equals(definition, implementation) ? | ||
| ErrorCode.ERR_PartialMethodNullabilityDifference : |
There was a problem hiding this comment.
ERR_PartialMethodNullabilityDifference [](start = 34, length = 38)
From our discussion, I think that nullability differences should be a warning at most, and we should ignore differences due to oblivious. #Resolved
| @@ -127,6 +126,45 @@ internal class MemberSignatureComparer : IEqualityComparer<Symbol> | |||
| considerRefKindDifferences: true, | |||
| typeComparison: TypeCompareKind.AllIgnoreOptions); | |||
|
|
|||
| /// <summary> | |||
| /// This instance is used to determine if a partial method implementation matches the definition | |||
| /// including differences ignored by the runtime other than dynamic/object and nullability. | |||
There was a problem hiding this comment.
including differences ignored [](start = 12, length = 29)
nit: It may be good to include an example of such difference (native integers) #Resolved
| diagnostics, | ||
| (diagnostics, implementedMethod, implementingMethod, topLevel, arg) => | ||
| { | ||
| hasTypeDifferences = true; |
There was a problem hiding this comment.
This capture/allocation would have happen for every partial method in the compilation. Consider modifying CheckValidNullableOverride to return a value indicating whether a diagnostic was reported, and making these lambdas 'static' to make it harder for future changes to this method to inadvertently introduce allocating captures. #Resolved
There was a problem hiding this comment.
| @@ -2688,6 +2694,15 @@ partial class C | |||
| }"; | |||
| var comp = CreateCompilation(source, parseOptions: TestOptions.RegularWithExtendedPartialMethods); | |||
| comp.VerifyDiagnostics(); | |||
|
|
|||
| comp = CreateCompilation(source, options: TestOptions.ReleaseDllWithWarningLevel5, parseOptions: TestOptions.RegularWithExtendedPartialMethods); | |||
There was a problem hiding this comment.
FYI @Youssef1313 I think that the work to enable warning waves by default in tests will need to be done by just grinding out all the breaks quickly to avoid breaking people. Perhaps after 16.8 is shipped.
There was a problem hiding this comment.
Oh sorry for missing completing #47077
I think there are only a few tests that need to be fixed. I'll complete it ASAP.
There was a problem hiding this comment.
@RikkiGibson Could you clarify what that means "to enable warning waves by default in tests"?
Are we talking about changing the default options used in CreateCompilation, or something else?
src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs
Outdated
Show resolved
Hide resolved
| considerTypeConstraints: false, | ||
| considerCallingConvention: false, | ||
| considerRefKindDifferences: true, | ||
| typeComparison: TypeCompareKind.ConsiderEverything); |
There was a problem hiding this comment.
ConsiderEverything [](start = 44, length = 18)
I expected we'd ignore oblivious differences. #Closed
There was a problem hiding this comment.
Ah, yes, this should change. Sorry for overlooking this in my review 😓 The DifferentReturnTypes_17 baseline should change as a result of this. #Closed
There was a problem hiding this comment.
| @@ -378,8 +378,10 @@ public override string ToString() | |||
| sb.Append('"'); | |||
| } | |||
|
|
|||
| sb.Append(", isSuppressed: "); | |||
| sb.Append(_isSuppressed ? "true" : "false"); | |||
| if (_isSuppressed) | |||
There was a problem hiding this comment.
was there something about the new diagnostic baselines that made this change necessary? #ByDesign
|
Also: it occurs to me that LDM suggested that warnings be reported if parameter names differ between partial declarations. Could you please either open a follow-up issue or implement that aspect of the warning wave in this PR? I think that particular aspect of the change is somewhat riskier (at least for .NET 5 WPF/Winforms projects). So feel free to use your discretion in deciding when/how to implement it and whether to check if partner teams break (or how badly they break 😉) when consuming such a change. |
ghost
added this to the 
Report a warning if a partial method declaration and implementation have type differences that are significant in C# but ignored by the runtime. The warning is added to the C#9 warning wave.
See C# LDM 2020-09-14.
Fixes #45519.
Fixes #30145.
Test plan #38821