Covariant returns part 3 #43797

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

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

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


            static void verify(CSharpCompilation comp)
            {
                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


            static void verify(CSharpCompilation comp)
            {
                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


            static void verify(CSharpCompilation comp)
            {
                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

@@ -387,6 +652,7 @@ static void verify(CSharpCompilation comp)
                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


            static void verify(CSharpCompilation comp)
            {
                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


            static void verify(CSharpCompilation comp)
            {
                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

@@ -544,6 +864,7 @@ public class Derived : Base
            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

@@ -605,6 +938,7 @@ static void verify(CSharpCompilation comp)
                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.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.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.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


            static void verify(CSharpCompilation comp)
            {
                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

@@ -883,6 +1304,7 @@ static void verify(CSharpCompilation comp)
                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


            static void verify(CSharpCompilation comp)
            {
                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

@@ -956,6 +1398,7 @@ static void verify(CSharpCompilation comp)
                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

@@ -998,6 +1450,7 @@ static void verify(CSharpCompilation comp)
                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


            static void verify(CSharpCompilation comp)
            {
                VerifyOverride(comp, "Derived.P", "System.IComparable Derived.P { get; }", "System.Object Base.P { get; set; }");
                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 AlekseyTs, RikkiGibson and a team 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

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

Covariant returns part 3 #43797

Covariant returns part 3 #43797

Conversation

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

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs May 8, 2020 • edited Loading

Choose a reason for hiding this comment

gafter May 8, 2020

Choose a reason for hiding this comment

AlekseyTs commented May 8, 2020 • edited Loading

AlekseyTs May 8, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter May 8, 2020

Choose a reason for hiding this comment

gafter commented May 8, 2020

AlekseyTs left a comment

Choose a reason for hiding this comment

gafter commented May 8, 2020

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

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