Covariant returns part 3 by gafter · Pull Request #43797 · dotnet/roslyn

gafter · 2020-04-29T18:22:45Z

Test that covariant returns are recognized by the compiler from compilation references.
Test that covariant methods imply metadata newslot and a methodimpl table entry.
Test the type returned from calling a covariantly overridden method.
Test that the compiler APIs agree whether viewed from source or metadata.
Test VB consumption and overriding of covariant return methods declared in C#.
Read covariant return override information [methodimpl] from metadata.

…ata.

…able entry.

…ed in C#.

…e have a place to put necessary support library declarations.

…t/roslyn into covariant-returns-part3

gafter · 2020-05-05T20:00:04Z

@dotnet/roslyn-compiler May I please have a review of these changes for covariant returns? This set of changes focuses on getting things working the same in source, compilation references, and metadata. #Resolved

src/Compilers/CSharp/Portable/Symbols/OverriddenOrHiddenMembersHelpers.cs

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

AlekseyTs · 2020-05-07T15:31:55Z

It doesn't look like metadata encoding is specified in https://github.com/dotnet/csharplang/blob/master/proposals/covariant-returns.md. Is there a different specification for that? #Closed

gafter · 2020-05-07T16:16:07Z

@AlekseyTs https://github.com/dotnet/csharplang/blob/master/proposals/covariant-returns.md is a specification of the C# language change. The metadata specification is maintained by the runtime team, and you have been CC'ed on the relevant threads. A runtime design document is under review in dotnet/runtime#35308 and I expect that PR will be extended to include a draft augment for ECMA-335.

I think I've responded to all of your comments. Do you have any others? #Resolved

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

src/Compilers/CSharp/Portable/Symbols/OverriddenOrHiddenMembersHelpers.cs

AlekseyTs · 2020-05-07T19:26:06Z

Done reviewing compiler changes (iteration 7), tests are not looked at. #Closed

gafter · 2020-05-07T21:32:38Z

@AlekseyTs I believe I've responded to all of your comments. Do you have any others? #Resolved

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

gafter · 2020-05-08T00:43:09Z

@AlekseyTs I believe I've responded to all of your comments. Do you have any others? #Resolved

AlekseyTs · 2020-05-08T13:12:53Z

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

            {
                VerifyOverride(comp, "Derived.this[]", "System.String Derived.this[System.Int32 i] { get; }", "System.Object Base.this[System.Int32 i] { get; }");
+                VerifyOverride(comp, "Derived.get_Item", "System.String Derived.this[System.Int32 i].get", "System.Object Base.this[System.Int32 i].get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:13:06Z

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

            {
                VerifyOverride(comp, "Derived.this[]", "U Derived<T, U>.this[System.Int32 i] { get; }", "T Base<T>.this[System.Int32 i] { get; }");
+                VerifyOverride(comp, "Derived.get_Item", "U Derived<T, U>.this[System.Int32 i].get", "T Base<T>.this[System.Int32 i].get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:13:22Z

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

            {
                VerifyOverride(comp, "Derived.this[]", "T Derived<T>.this[System.Int32 i] { get; }", "N Base.this[System.Int32 i] { get; }");
+                VerifyOverride(comp, "Derived.get_Item", "T Derived<T>.this[System.Int32 i].get", "N Base.this[System.Int32 i].get");
+                VerifyAssignments(comp);


Same question. #Resolved

AlekseyTs · 2020-05-08T13:14:08Z

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

                VerifyOverride(comp, "Derived.P", "System.Func<System.String> Derived.P { get; set; }", "System.Func<System.Object> Base.P { get; set; }");
                VerifyNoOverride(comp, "Derived.set_P");
                VerifyOverride(comp, "Derived.get_P", "System.Func<System.String> Derived.P.get", "System.Func<System.Object> Base.P.get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:15:34Z

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

            static void verify(CSharpCompilation comp)
            {
                VerifyOverride(comp, "Derived.M", "System.String Derived.M()", "System.Object Base.M()");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:16:27Z

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

            {
                VerifyOverride(comp, "Derived.M", "System.String Derived.M { get; }", "System.Object Base.M { get; }");
+                VerifyOverride(comp, "Derived.get_M", "System.String Derived.M.get", "System.Object Base.M.get");
+                VerifyAssignments(comp);


Same question. #Resolved

AlekseyTs · 2020-05-08T13:16:42Z

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

            {
                VerifyOverride(comp, "Derived.this[]", "System.String Derived.this[System.Int32 i] { get; }", "System.Object Base.this[System.Int32 i] { get; }");
+                VerifyOverride(comp, "Derived.get_Item", "System.String Derived.this[System.Int32 i].get", "System.Object Base.this[System.Int32 i].get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:16:55Z

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

            static void verify(CSharpCompilation comp)
            {
                VerifyOverride(comp, "Derived.M", "System.String Derived.M()", "System.Object Base.M()");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:17:35Z

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

            static void verify(CSharpCompilation comp)
            {
                VerifyOverride(comp, "Derived.M", "System.Object Derived.M()", "System.String Base.M()");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:18:01Z

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

                VerifyNoOverride(comp, "Derived2.M2");
                VerifyNoOverride(comp, "Derived3.M1");
                VerifyNoOverride(comp, "Derived3.M2");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:18:44Z

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

                VerifyOverride(comp, "Derived2.M3", "Base Derived2.M3 { get; }", "System.String Derived.M3 { get; }");
+                VerifyOverride(comp, "Derived2.get_M3", "Base Derived2.M3.get", "System.String Derived.M3.get");
+
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:20:03Z

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

+                VerifyOverride(comp, "Derived.get_M1", "IIn<System.Object> Derived.M1.get", "IIn<System.String> Base.M1.get");
                VerifyOverride(comp, "Derived.M2", "IOut<System.String> Derived.M2 { get; }", "IOut<System.Object> Base.M2 { get; }");
+                VerifyOverride(comp, "Derived.get_M2", "IOut<System.String> Derived.M2.get", "IOut<System.Object> Base.M2.get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:21:04Z

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

+                VerifyOverride(comp, "Derived.get_M1", "IIn<System.String> Derived.M1.get", "IIn<System.Object> Base.M1.get");
                VerifyOverride(comp, "Derived.M2", "IOut<System.Object> Derived.M2 { get; }", "IOut<System.String> Base.M2 { get; }");
+                VerifyOverride(comp, "Derived.get_M2", "IOut<System.Object> Derived.M2.get", "IOut<System.String> Base.M2.get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:21:22Z

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

+                VerifyOverride(comp, "Derived.get_M1", "System.Int16 Derived.M1.get", "System.Int32 Base.M1.get");
                VerifyOverride(comp, "Derived.M2", "B Derived.M2 { get; }", "A Base.M2 { get; }");
+                VerifyOverride(comp, "Derived.get_M2", "B Derived.M2.get", "A Base.M2.get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:21:38Z

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

            {
                VerifyOverride(comp, "Derived.M", "System.String Derived.M { get; }", "System.IComparable Base.M { get; }");
+                VerifyOverride(comp, "Derived.get_M", "System.String Derived.M.get", "System.IComparable Base.M.get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:22:01Z

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

                VerifyNoOverride(comp, "Derived.Base.M2");
                VerifyNoOverride(comp, "C.Base.M1");
                VerifyNoOverride(comp, "C.Base.M2");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:22:20Z

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

            {
                VerifyOverride(comp, "Derived.P", "System.String Derived.P { get; }", "System.Object Base.P { get; set; }");
                VerifyOverride(comp, "Derived.get_P", "System.String Derived.P.get", "System.Object Base.P.get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:22:38Z

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

                VerifyOverride(comp, "Derived2.P", "System.String Derived2.P { get; set; }", "System.String Derived.P { get; }");
                VerifyOverride(comp, "Derived2.get_P", "System.String Derived2.P.get", "System.String Derived.P.get");
                VerifyNoOverride(comp, "Derived2.set_P");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:22:58Z

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

                VerifyOverride(comp, "Derived.get_P", "System.String Derived.P.get", "System.Object Base.P.get");
                VerifyOverride(comp, "Derived2.P", "System.String Derived2.P { set; }", "System.String Derived.P { get; }");
                VerifyNoOverride(comp, "Derived2.set_P");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:23:38Z

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

                VerifyOverride(comp, "Derived.get_P", "System.IComparable Derived.P.get", "System.Object Base.P.get");
                VerifyOverride(comp, "Derived2.P", "System.String Derived2.P { get; }", "System.IComparable Derived.P { get; }");
                VerifyOverride(comp, "Derived2.get_P", "System.String Derived2.P.get", "System.IComparable Derived.P.get");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:25:19Z

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

+            static void verify(CSharpCompilation comp)
+            {
+                VerifyOverride(comp, "Derived.M", "System.String Derived.M(System.String s)", "System.Object Base<System.String>.M(System.String s)");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:26:42Z

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

+            {
+                VerifyOverride(comp, "Derived.M", "System.IComparable Derived.M()", "System.Object Base.M()");
+                VerifyOverride(comp, "Derived2.M", "System.String Derived2.M()", "System.IComparable Derived.M()");
+                VerifyAssignments(comp);


VerifyAssignments [](start = 16, length = 17)

Same question. #Resolved

AlekseyTs · 2020-05-08T13:31:36Z

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

+                    count++;
+                    var initialValue = declarator.Initializer.Value;
+                    var typeInfo = model.GetTypeInfo(initialValue);
+                    Assert.True(typeInfo.Type.Equals(typeInfo.ConvertedType, TypeCompareKind.AllIgnoreOptions));


Equals [](start = 46, length = 6)

What API is called here? #Closed

This is calling Test.Utilities.Equals(this ITypeSymbol first, ITypeSymbol second, Microsoft.CodeAnalysis.TypeCompareKind typeCompareKind)
However, the ignore options aren't needed so this is changed to Assert.Equal(typeInfo.Type, typeInfo.ConvertedType) in the following PR.

In reply to: 422144339 [](ancestors = 422144339)

AlekseyTs · 2020-05-08T13:33:18Z

Done with review pass (iteration 7) #Closed

AlekseyTs · 2020-05-08T16:13:58Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

+            {
+                if (IsExplicitClassOverride)
+                {
+                    foreach (MethodSymbol method in ExplicitlyOverriddenMethods())


ExplicitlyOverriddenMethods [](start = 52, length = 27)

I think it would be good to test scenarios when we fail to successfully resolve a MethodImpl. For example, the method is not found in the type, or the type is not found. Cover scenarios when the type is a class and an interface. Also in presence of other resolvable MethodImpls, from classes and interfaces, all combinations. #Resolved

This suggestion is tracked at #44079

In reply to: 422233060 [](ancestors = 422233060)

gafter · 2020-05-08T16:43:03Z

@AlekseyTs I believe I've responded to all of your comments. Do you have any others?

AlekseyTs

Signing-off on iteration 7 with assumption that feedback is going to be addressed in the next PR, unless tracked by an issue.

gafter · 2020-05-08T18:27:03Z

You can see the issues already filed as noted and feedback already addressed in #44025

gafter added Area-Compilers Feature - Covariant Returns Permit an override to return a more specific reference type labels Apr 29, 2020

gafter added this to the Compiler.Net5 milestone Apr 29, 2020

gafter self-assigned this Apr 29, 2020

gafter mentioned this pull request May 4, 2020

Covariant returns part 2 #43673

Merged

Neal Gafter added 5 commits May 4, 2020 15:06

Test that covariant returns are recognized by the compiler from metad…

7439390

…ata.

Test that covariant methods imply metadata newslot and a methodimpl t…

1ae37dc

…able entry.

Test the type returned from calling a covariantly overridden method.

f4ceeee

Test VB consumption and overriding of covariant return methods declar…

045c671

…ed in C#.

Make test helper methods for with and without covariant returns, so w…

2052dca

…e have a place to put necessary support library declarations.

gafter force-pushed the covariant-returns-part3 branch from 0932ad2 to 2052dca Compare May 4, 2020 22:07

Neal Gafter added 2 commits May 5, 2020 12:55

Read covariant return override information [methodimpl] from metadata.

3ffbe67

Merge branch 'features/covariant-returns' of https://github.com/dotne…

f318e72

…t/roslyn into covariant-returns-part3

gafter requested review from a team, AlekseyTs and RikkiGibson May 5, 2020 19:58

gafter marked this pull request as ready for review May 5, 2020 19:58

RikkiGibson approved these changes May 6, 2020

View reviewed changes

gafter mentioned this pull request May 6, 2020

Covariant returns part 4 #44025

Merged

AlekseyTs reviewed May 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs Show resolved Hide resolved

AlekseyTs reviewed May 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs Show resolved Hide resolved

AlekseyTs reviewed May 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/OverriddenOrHiddenMembersHelpers.cs Show resolved Hide resolved

AlekseyTs reviewed May 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/OverriddenOrHiddenMembersHelpers.cs Show resolved Hide resolved

AlekseyTs reviewed May 8, 2020

View reviewed changes

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

AlekseyTs reviewed May 8, 2020

View reviewed changes

gafter mentioned this pull request May 8, 2020

Tests suggested for PEMthodSymbol.ExplicitlyOverriddenClassMethod #44079

Open

AlekseyTs approved these changes May 8, 2020

View reviewed changes

gafter merged commit 2167092 into dotnet:features/covariant-returns May 8, 2020

Conversation

gafter commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlekseyTs commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlekseyTs commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gafter commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

gafter commented Apr 29, 2020 •

edited

Loading

gafter commented May 5, 2020 •

edited

Loading

AlekseyTs commented May 7, 2020 •

edited

Loading

gafter commented May 7, 2020 •

edited

Loading

AlekseyTs commented May 7, 2020 •

edited

Loading

gafter commented May 7, 2020 •

edited

Loading

gafter commented May 8, 2020 •

edited

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading

AlekseyTs May 8, 2020 •

edited

Loading

AlekseyTs commented May 8, 2020 •

edited

Loading

AlekseyTs May 8, 2020 •

edited by gafter

Loading