Check partial method return type compatibility #45128

RikkiGibson · 2020-06-12T23:06:50Z

Closes #44930
Related to #43795

RikkiGibson · 2020-06-16T17:31:42Z

PTAL @cston @dotnet/roslyn-compiler

jaredpar · 2020-06-16T18:29:16Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

+                constructedDefinition = definition.Construct(implementation.TypeArgumentsWithAnnotations);
+            }
+
+            bool returnTypesEqual = constructedDefinition.ReturnTypeWithAnnotations.Equals(implementation.ReturnTypeWithAnnotations, TypeCompareKind.AllIgnoreOptions);


Why did you choose this over CLRSignatureCompareOptions?

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.

I used SourceMemberContainerSymbol.CheckOverrideMember as a basis for this code:

roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs

Line 939 in 85b65a4

if (overridingMethod.IsGenericMethod)

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 for IntPtr 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 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.

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.

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.

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.

cston · 2020-06-16T19:15:51Z

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

+    public partial string M1() => ""hello"";
+
+    public partial IEnumerable<string?> M2();
+    public partial IEnumerable<string> M2() => null!;


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

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.

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)

I'm not sure I understand this suggestion. There are only defining declarations in this sample. #Closed

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)

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

cston · 2020-06-16T19:34:03Z

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

+}";
+            var comp = CreateCompilation(source, parseOptions: TestOptions.RegularWithExtendedPartialMethods);
+            comp.VerifyDiagnostics();
+        }


} [](start = 8, length = 1)

Is there a reason to support differences in dynamic / object, or nint / System.IntPtr?

I think we will try to answer this question here: #45128 (comment)

RikkiGibson · 2020-06-18T18:06:45Z

Please take another look @jaredpar @cston. I believe AllIgnoreOptions is a reasonable choice here as it makes the behavior consistent between partial method return types and parameter types.

jaredpar · 2020-06-18T19:09:57Z

I believe AllIgnoreOptions is a reasonable choice here as it makes the behavior consistent between partial method return types and parameter types.

Yeah it looks like I picked the exact wrong example to test out if that was the right value or not 😦

jaredpar · 2020-06-18T19:12:44Z

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

+        }
+
+        [Fact, WorkItem(44930, "https://github.com/dotnet/roslyn/issues/44930")]
+        public void DifferentReturnTypes_15()


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.

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

cston · 2020-06-18T21:31:31Z

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

+
+    public partial nint M2();
+    public partial IntPtr M2() => default;
+}";


} [](start = 0, length = 1)

I think we should report an error for differences in native integers and underlying types.

Filed a follow-up issue.

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

cston · 2020-06-18T21:40:52Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

@@ -1195,12 +1214,19 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S
                definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations),


definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations) [](start = 16, length = 74)

constructedDefinition? #Closed

cston · 2020-06-18T21:42:46Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

+            if (implementation.IsGenericMethod)
+            {
+                constructedDefinition = definition.Construct(implementation.TypeArgumentsWithAnnotations);
+            }


= definition.ConstructIfGeneric(...)

cston · 2020-06-18T21:48:21Z

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

+}";
+            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.


error [](start = 27, length = 5)

Consider updating comments so it's clear this is a "warning". #Closed

funny, I guess I created these baselines before filling out some of the boilerplate to make the diagnostic a warning.

cston · 2020-06-18T21:59:38Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

+            bool returnTypesEqual = constructedDefinition.ReturnTypeWithAnnotations.Equals(implementation.ReturnTypeWithAnnotations, TypeCompareKind.AllIgnoreOptions);
+            if (!returnTypesEqual
+                && !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(implementation.ReturnType)
+                && !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(definition.ReturnType))


If the types are distinct, why not report that, regardless of whether the types contain errors?

This is based on patterns I saw in CheckOverrideMember, it seemed like reducing the cascading errors was best.

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)

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

cston · 2020-06-18T22:04:32Z

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs

+}";
+            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.


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

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

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)

Check partial method return type compatibility

b66f235

RikkiGibson added the Area-Compilers label Jun 12, 2020

RikkiGibson requested a review from a team as a code owner June 12, 2020 23:06

Merge branch 'master' into partial-return-checks

550f8c5

jaredpar requested changes Jun 16, 2020

View reviewed changes

Add a pair of tuple tests

e513785

cston reviewed Jun 16, 2020

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs Show resolved Hide resolved

More tests

608ed59

cston reviewed Jun 16, 2020

View reviewed changes

Add test from feedback

a98d022

jaredpar approved these changes Jun 18, 2020

View reviewed changes

cston reviewed Jun 18, 2020

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs Show resolved Hide resolved

Add additional test from feedback

3aea65a

cston reviewed Jun 18, 2020

View reviewed changes

RikkiGibson mentioned this pull request Jun 18, 2020

nint/IntPtr differences should be disallowed in partial methods #45301

Closed

cston reviewed Jun 18, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs Show resolved Hide resolved

cston reviewed Jun 18, 2020

View reviewed changes

Address feedback

3015415

cston reviewed Jun 18, 2020

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs Show resolved Hide resolved

cston reviewed Jun 18, 2020

View reviewed changes

RikkiGibson added 2 commits June 18, 2020 15:18

More error type tests

debcedf

Update diagnostic message

7be99fd

cston approved these changes Jun 18, 2020

View reviewed changes

RikkiGibson merged commit 09d5725 into dotnet:master Jun 19, 2020

ghost added this to the Next milestone Jun 19, 2020

RikkiGibson deleted the partial-return-checks branch June 19, 2020 01:33

dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check partial method return type compatibility #45128

Check partial method return type compatibility #45128

RikkiGibson commented Jun 12, 2020 •

edited

Loading

RikkiGibson commented Jun 16, 2020

jaredpar Jun 16, 2020

jaredpar Jun 16, 2020

RikkiGibson Jun 16, 2020

jaredpar Jun 16, 2020

RikkiGibson Jun 16, 2020

jaredpar Jun 16, 2020

RikkiGibson Jun 16, 2020

cston Jun 16, 2020 •

edited

Loading

RikkiGibson Jun 16, 2020

cston Jun 16, 2020

RikkiGibson Jun 16, 2020 •

edited by cston

Loading

cston Jun 16, 2020

cston Jun 16, 2020

RikkiGibson Jun 16, 2020

RikkiGibson commented Jun 18, 2020

jaredpar commented Jun 18, 2020

jaredpar Jun 18, 2020

cston Jun 18, 2020

RikkiGibson Jun 18, 2020

cston Jun 18, 2020 •

edited

Loading

cston Jun 18, 2020

cston Jun 18, 2020 •

edited

Loading

RikkiGibson Jun 18, 2020

cston Jun 18, 2020

RikkiGibson Jun 18, 2020

cston Jun 18, 2020

cston Jun 18, 2020 •

edited

Loading

RikkiGibson Jun 18, 2020 •

edited by cston

Loading

cston Jun 18, 2020

		@@ -1195,12 +1214,19 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S
		definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations),

Check partial method return type compatibility #45128

Check partial method return type compatibility #45128

Conversation

RikkiGibson commented Jun 12, 2020 • edited Loading

RikkiGibson commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jun 16, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Jun 18, 2020

jaredpar commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Jun 18, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Jun 12, 2020 •

edited

Loading

cston Jun 16, 2020 •

edited

Loading

RikkiGibson Jun 16, 2020 •

edited by cston

Loading

cston Jun 18, 2020 •

edited

Loading

cston Jun 18, 2020 •

edited

Loading

cston Jun 18, 2020 •

edited

Loading

RikkiGibson Jun 18, 2020 •

edited by cston

Loading