Skip to content
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

Merged
merged 9 commits into from
Jun 19, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -5389,6 +5389,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_NullabilityMismatchInParameterTypeOnPartial_Title" xml:space="preserve">
<value>Nullability of reference types in type of parameter doesn't match partial method declaration.</value>
</data>
<data name="WRN_NullabilityMismatchInReturnTypeOnPartial" xml:space="preserve">
<value>Nullability of reference types in return type doesn't match partial method declaration.</value>
</data>
<data name="WRN_NullabilityMismatchInReturnTypeOnPartial_Title" xml:space="preserve">
<value>Nullability of reference types in return type doesn't match partial method declaration.</value>
</data>
<data name="WRN_NullabilityMismatchInTypeOnImplicitImplementation" xml:space="preserve">
<value>Nullability of reference types in type of '{0}' doesn't match implicitly implemented member '{1}'.</value>
</data>
Expand Down Expand Up @@ -6172,6 +6178,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_PartialMethodExtendedModDifference" xml:space="preserve">
<value>Both partial method declarations must have identical combinations of 'virtual', 'override', 'sealed', and 'new' modifiers.</value>
</data>
<data name="ERR_PartialMethodReturnTypeDifference" xml:space="preserve">
<value>Both partial method declarations must have the same return type.</value>
</data>
<data name="ERR_PartialMethodRefReturnDifference" xml:space="preserve">
<value>Both partial method declarations must return by reference or neither may return by reference.</value>
</data>
<data name="IDS_TopLevelStatements" xml:space="preserve">
<value>top-level statements</value>
</data>
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1818,6 +1818,14 @@ internal enum ErrorCode
ERR_CannotConvertAddressOfToDelegate = 8811,
ERR_AddressOfToNonFunctionPointer = 8812,

// Codes 8813, 8814, 8815, 8816 used by features/module-initializers

ERR_PartialMethodReturnTypeDifference = 8817,
ERR_PartialMethodRefReturnDifference = 8818,
WRN_NullabilityMismatchInReturnTypeOnPartial = 8819,

// Codes 8820, 8821 used by features/static-lambdas

ERR_ExpressionTreeContainsWithExpression = 8849,
ERR_BadRecordDeclaration = 8850,
ERR_DuplicateRecordConstructor = 8851,
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ static ErrorFacts()

nullableWarnings.Add(getId(ErrorCode.WRN_NullabilityMismatchInTypeOnOverride));
nullableWarnings.Add(getId(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnOverride));
nullableWarnings.Add(getId(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnPartial));
nullableWarnings.Add(getId(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnOverride));
nullableWarnings.Add(getId(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnPartial));
nullableWarnings.Add(getId(ErrorCode.WRN_NullabilityMismatchInTypeOnImplicitImplementation));
Expand Down Expand Up @@ -399,6 +400,7 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_NullReferenceArgument:
case ErrorCode.WRN_NullabilityMismatchInTypeOnOverride:
case ErrorCode.WRN_NullabilityMismatchInReturnTypeOnOverride:
case ErrorCode.WRN_NullabilityMismatchInReturnTypeOnPartial:
case ErrorCode.WRN_NullabilityMismatchInParameterTypeOnOverride:
case ErrorCode.WRN_NullabilityMismatchInParameterTypeOnPartial:
case ErrorCode.WRN_NullabilityMismatchInConstraintsOnPartialImplementation:
Expand Down

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
Expand Up @@ -1079,11 +1079,11 @@ static void checkValidNullableMethodOverride(
checkParameters ? ReportBadParameter : null,
overridingMemberLocation);
}
}

static bool IsOrContainsErrorType(TypeSymbol typeSymbol)
{
return (object)typeSymbol.VisitType((currentTypeSymbol, unused1, unused2) => currentTypeSymbol.IsErrorType(), (object)null) != null;
}
internal static bool IsOrContainsErrorType(TypeSymbol typeSymbol)
{
return (object)typeSymbol.VisitType((currentTypeSymbol, unused1, unused2) => currentTypeSymbol.IsErrorType(), (object)null) != null;
}

static readonly ReportMismatchInReturnType<Location> ReportBadReturn =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1149,6 +1149,25 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S
{
Debug.Assert(!ReferenceEquals(definition, implementation));

MethodSymbol constructedDefinition = definition;
if (implementation.IsGenericMethod)
{
constructedDefinition = definition.Construct(implementation.TypeArgumentsWithAnnotations);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= definition.ConstructIfGeneric(...)


bool returnTypesEqual = constructedDefinition.ReturnTypeWithAnnotations.Equals(implementation.ReturnTypeWithAnnotations, TypeCompareKind.AllIgnoreOptions);
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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:

Copy link
Member

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:

  1. 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 for IntPtr vs. nint.
  2. 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 case object was the more natural choice. Forcing them to use object 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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

if (!returnTypesEqual
&& !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(implementation.ReturnType)
&& !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(definition.ReturnType))
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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)

{
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]);
Expand Down Expand Up @@ -1195,12 +1214,19 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S
definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations),
Copy link
Member

@cston cston Jun 18, 2020

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

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)
Expand Down
20 changes: 20 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,16 @@
<target state="translated">Obě deklarace částečné metody musí mít modifikátor readonly, nebo nesmí mít modifikátor readonly žádná z nich.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodRefReturnDifference">
<source>Both partial method declarations must return by reference or neither may return by reference.</source>
<target state="new">Both partial method declarations must return by reference or neither may return by reference.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodReturnTypeDifference">
<source>Both partial method declarations must have the same return type.</source>
<target state="new">Both partial method declarations must have the same return type.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodWithAccessibilityModsMustHaveImplementation">
<source>Partial method '{0}' must have an implementation part because it has accessibility modifiers.</source>
<target state="new">Partial method '{0}' must have an implementation part because it has accessibility modifiers.</target>
Expand Down Expand Up @@ -2164,6 +2174,16 @@
<target state="translated">Typ odkazu s možnou hodnotou null ve vráceném typu neodpovídá přepsanému členu.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInTypeOnExplicitImplementation">
<source>Nullability of reference types in type doesn't match implemented member '{0}'.</source>
<target state="translated">Typ odkazu s možnou hodnotou null v typu neodpovídá implementovanému členu {0}.</target>
Expand Down
20 changes: 20 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,16 @@
<target state="translated">Entweder beide oder keine der partiellen Methodendeklarationen müssen als "readonly" festgelegt werden.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodRefReturnDifference">
<source>Both partial method declarations must return by reference or neither may return by reference.</source>
<target state="new">Both partial method declarations must return by reference or neither may return by reference.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodReturnTypeDifference">
<source>Both partial method declarations must have the same return type.</source>
<target state="new">Both partial method declarations must have the same return type.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodWithAccessibilityModsMustHaveImplementation">
<source>Partial method '{0}' must have an implementation part because it has accessibility modifiers.</source>
<target state="new">Partial method '{0}' must have an implementation part because it has accessibility modifiers.</target>
Expand Down Expand Up @@ -2164,6 +2174,16 @@
<target state="translated">Die NULL-Zulässigkeit von Verweistypen im Rückgabetyp entspricht nicht dem außer Kraft gesetzten Member.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInTypeOnExplicitImplementation">
<source>Nullability of reference types in type doesn't match implemented member '{0}'.</source>
<target state="translated">Die NULL-Zulässigkeit von Verweistypen im Typ entspricht nicht dem implementierten Member "{0}".</target>
Expand Down
20 changes: 20 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,16 @@
<target state="translated">Ambas declaraciones de métodos parciales deben ser de solo lectura o ninguna de ellas puede ser de solo lectura</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodRefReturnDifference">
<source>Both partial method declarations must return by reference or neither may return by reference.</source>
<target state="new">Both partial method declarations must return by reference or neither may return by reference.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodReturnTypeDifference">
<source>Both partial method declarations must have the same return type.</source>
<target state="new">Both partial method declarations must have the same return type.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodWithAccessibilityModsMustHaveImplementation">
<source>Partial method '{0}' must have an implementation part because it has accessibility modifiers.</source>
<target state="new">Partial method '{0}' must have an implementation part because it has accessibility modifiers.</target>
Expand Down Expand Up @@ -2164,6 +2174,16 @@
<target state="translated">La nulabilidad de los tipos de referencia en el tipo de valor devuelto no coincide con el miembro reemplazado</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInTypeOnExplicitImplementation">
<source>Nullability of reference types in type doesn't match implemented member '{0}'.</source>
<target state="translated">La nulabilidad de los tipos de referencia del tipo no coincide con el miembro implementado "{0}".</target>
Expand Down
20 changes: 20 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,16 @@
<target state="translated">Soit les deux déclarations de méthodes partielles sont readonly, soit aucune ne l'est</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodRefReturnDifference">
<source>Both partial method declarations must return by reference or neither may return by reference.</source>
<target state="new">Both partial method declarations must return by reference or neither may return by reference.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodReturnTypeDifference">
<source>Both partial method declarations must have the same return type.</source>
<target state="new">Both partial method declarations must have the same return type.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodWithAccessibilityModsMustHaveImplementation">
<source>Partial method '{0}' must have an implementation part because it has accessibility modifiers.</source>
<target state="new">Partial method '{0}' must have an implementation part because it has accessibility modifiers.</target>
Expand Down Expand Up @@ -2164,6 +2174,16 @@
<target state="translated">La nullabilité des types référence dans le type de retour ne correspond pas au membre substitué.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInTypeOnExplicitImplementation">
<source>Nullability of reference types in type doesn't match implemented member '{0}'.</source>
<target state="translated">La nullabilité des types référence dans le type ne correspond pas au membre implémenté '{0}'.</target>
Expand Down
20 changes: 20 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,16 @@
<target state="translated">Nessuna o entrambe le dichiarazioni di metodi parziali devono essere di tipo readonly</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodRefReturnDifference">
<source>Both partial method declarations must return by reference or neither may return by reference.</source>
<target state="new">Both partial method declarations must return by reference or neither may return by reference.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodReturnTypeDifference">
<source>Both partial method declarations must have the same return type.</source>
<target state="new">Both partial method declarations must have the same return type.</target>
<note />
</trans-unit>
<trans-unit id="ERR_PartialMethodWithAccessibilityModsMustHaveImplementation">
<source>Partial method '{0}' must have an implementation part because it has accessibility modifiers.</source>
<target state="new">Partial method '{0}' must have an implementation part because it has accessibility modifiers.</target>
Expand Down Expand Up @@ -2164,6 +2174,16 @@
<target state="translated">Il supporto dei valori Null dei tipi riferimento nel tipo restituito non corrisponde al membro di cui è stato eseguito l'override.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInTypeOnExplicitImplementation">
<source>Nullability of reference types in type doesn't match implemented member '{0}'.</source>
<target state="translated">Il supporto dei valori Null dei tipi riferimento nel tipo non corrisponde al membro implementato '{0}'.</target>
Expand Down
Loading