Revert "Revert "Require partial method signatures to match" (47576) (#47879)" by Youssef1313 · Pull Request #53352 · dotnet/roslyn

Youssef1313 · 2021-05-12T06:53:54Z

This PR brings back #47576, and changes the warning level to 6, and also extend the warning for parameter name differences.

Fixes #47838
Fixes #52520

…otnet#47879)" This reverts commit c785833.

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

AlekseyTs · 2021-05-12T15:13:11Z

@Youssef1313 Could you provide details about what was the problem with the original change that got reverted?

Youssef1313 · 2021-05-12T15:26:37Z

@Youssef1313 Could you provide details about what was the problem with the original change that got reverted?

@AlekseyTs, After @cston initially implemented it, it got temporarily reverted to be added later into a warning wave. This PR gets @cston work again by reverting the revert. And on top of that, extend the warning per #47838 (this was confirmed in LDM - see #47841 (comment))

cston · 2021-05-12T16:17:01Z

what was the problem with the original change that got reverted?

#47576 was reverted because it was late in the 16.8 release cycle to take a change that affects warnings. The expectation was we'd re-introduce the change in a later warning wave.

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

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

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

RikkiGibson · 2021-05-20T22:26:49Z

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

                new FormattedSymbol(overridingParameter, SymbolDisplayFormat.ShortFormat));

-        internal static void CheckValidNullableMethodOverride<TArg>(
+        internal static bool CheckValidNullableMethodOverride<TArg>(


Consider adding a doc comment for the return value.

Doc'ed. Would like @cston to confirm the correctness of the doc comment.

RikkiGibson · 2021-05-20T22:28:33Z

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

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


Does this introduce new errors when one of the partial declarations has an error type for its return type?

Does this introduce new errors when one of the partial declarations has an error type for its return type?

Yes, I believe so, but if the return types are distinct, arguably we should report a warning, even if one or both of those return types is an error type.

RikkiGibson · 2021-05-20T22:33:38Z

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

                var typeParameter1 = typeParameters1[i];
                var typeParameter2 = typeParameters2[i];

+                // TODO: Should report on type parameter name differences?


Sure, why not? However, it feels reasonable to do this at the same time as we check the parameter name difference. i.e. if any type parameter names or parameter names or parameter nullabilities differ, for example, report a single diagnostic on the implementation.

@RikkiGibson I didn't do that because LDM notes didn't mention this at all. So I wasn't sure if it needs to be discussed in a later LDM first?

If it's okay to go ahead and do that directly in that PR, I'll.

I feel that this language supports us reporting warnings on type parameter name differences:

We will add a new warning wave warning for all other syntactic differences between partial method implementations.

I will go ahead and ping language design internally to see if there is any objection, but I'm fairly confident that this will be accepted.

@RikkiGibson I did it in the last commit (modulo tests - will fix the failing tests after you confirm). In case there are any concerns not to do this, I'll revert.

LDM has confirmed that type parameter name differences should also cause the warning.

Thanks for confirming.

cston · 2021-06-01T15:52:05Z

public partial V M1<V>() => default;

It looks like this should be U M1<U>() => default;. Otherwise the test appears identical to M3 below.

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs:2807 in 4adfba6. [](commit_id = 4adfba6, deletion_comment = False)

cston · 2021-06-01T15:53:50Z

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

+            var type1 = ns.GetTypeMembers("MyClass").Single() as NamedTypeSymbol;
+            Assert.Equal(0, type1.TypeParameters.Length);
+            var f = type1.GetMembers("F").Single() as MethodSymbol;
+            Assert.Equal(2, f.TypeParameters.Length);


TypeParameters

Should these checks be using Parameters instead?

@cston The number of parameters is 1, not 2. So this change will fail the test.

The number of parameters is 1, not 2. So this change will fail the test.

I meant that any asserts should probably check Parameters rather than TypeParameters since, unlike the following test, the parameter names are distinct not the type parameter names.

src/Compilers/CSharp/Portable/CSharpResources.resx

Co-authored-by: Rikki Gibson <rigibson@microsoft.com>

src/Compilers/CSharp/Portable/CSharpResources.resx

RikkiGibson

Sorry for delay on this. It looks very close now. Just had some minor suggestions on wording/formatting.

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

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

Co-authored-by: Rikki Gibson <rigibson@microsoft.com>

cston · 2021-06-10T04:56:39Z

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

+            Assert.Equal(0, type1.TypeParameters.Length);
+            var f = type1.GetMembers("F").Single() as MethodSymbol;
+            Assert.Equal(2, f.TypeParameters.Length);
+            var param1 = f.TypeParameters[0];
+            var param2 = f.TypeParameters[1];
+            Assert.Equal("T", param1.Name);
+            Assert.Equal("U", param2.Name);


Consider checking the parameter name rather than type parameter names since it is the parameter name that is distinct.

Suggested change

Assert.Equal(0, type1.TypeParameters.Length);

var f = type1.GetMembers("F").Single() as MethodSymbol;

Assert.Equal(2, f.TypeParameters.Length);

var param1 = f.TypeParameters[0];

var param2 = f.TypeParameters[1];

Assert.Equal("T", param1.Name);

Assert.Equal("U", param2.Name);

Assert.Equal("T t", f.Parameters[0].ToTestDisplayString());

cston

LGTM, thanks @Youssef1313.

RikkiGibson · 2021-06-10T05:08:49Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+    <value>Partial method declarations '{0}' and '{1}' have signature differences.</value>
+  </data>
+  <data name="WRN_PartialMethodTypeDifference_Title" xml:space="preserve">
+    <value>Partial method declarations have differences in parameter names, parameter types, or return types.</value>


Ah, hate to do this last minute, but this message should be changed to be in sync with the other message.

@RikkiGibson My bad! Fixing now.

RikkiGibson · 2021-06-10T17:52:16Z

Thanks for the contribution @Youssef1313!

…ures/interpolated-string * upstream/main: (95 commits) Update official build number in separate job Update Language Feature Status.md (#54015) Remove IRazorDocumentOptionsService inheritance interface (#54047) Fix comment Simplify Do not create a cache field for lambda if it depends on caller's type argument (#44939) Documentation Documentation Documentation Update test impls Just pass null Pull diagnostics should just request from the doc, not the whole project. Add test plan for file-scoped namespace (#54003) Add source build to official build Improved nullable 'is' analysis (#53311) Multi session service (#53762) Resolve Versions.props conflicts Revert "Revert "Require partial method signatures to match" (47576) (#47879)" (#53352) Broaden enforcement on prototype marker (#53886) Update Language Feature Status.md (#53926) ...

Youssef1313 added 3 commits May 12, 2021 08:39

Revert "Revert "Require partial method signatures to match" (47576) (d…

0c63203

…otnet#47879)" This reverts commit c785833.

Fix conflicts

0e366d9

Move to warning level 6

e01db55

ghost added the Area-Compilers label May 12, 2021

Report warning on different parameter names

41d05ee

Youssef1313 marked this pull request as ready for review May 12, 2021 07:22

Youssef1313 requested a review from a team as a code owner May 12, 2021 07:22

Youssef1313 changed the base branch from main to features/compiler May 12, 2021 08:00

Update tests

528d481

Youssef1313 mentioned this pull request May 12, 2021

Report warning for differences in parameter names in partial method declaration and implementation #47841

Closed

Fix few tests

10bebc9

Youssef1313 commented May 12, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits May 12, 2021 12:30

Fix nullability mismatch check

1342efe

Fix more tests

8d0573f

AlekseyTs added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 12, 2021

runfoapp bot mentioned this pull request May 12, 2021

SimpleCovariantReturnEndToEndTest is stack overflowing in CI #53361

Closed