Covariant returns part 2 by gafter · Pull Request #43673 · dotnet/roslyn

gafter · 2020-04-25T01:20:31Z

Emit language version suggestion for property overrides when applicable.
Add test for override properties with setters.
Test relaxed rules for override in source vs a metadata base type.
Add a number of tests requested in Covariant returns part1 #43576
Add testing for symbol API (when override is in source)

…able. - Add test for override properties with setters. - Test relaxed rules for override in source vs a metadata base type.

AlekseyTs · 2020-04-28T20:41:19Z

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

-                            else if (!IsValidOverrideReturnType(overridingProperty, overridingMemberType, overriddenMemberType, diagnostics))
+                            else if (overridingProperty.SetMethod is null ?
+                                !IsValidOverrideReturnType(overridingProperty, overridingMemberType, overriddenMemberType, diagnostics) :
+                                !overridingMemberType.Equals(overriddenMemberType, TypeCompareKind.AllIgnoreOptions))


overriddenMemberType [](start = 61, length = 20)

It feels like this isn't always the right type to use. For example:

class A { public virtual A P {get; set;} } class B : A { public override B P {get => null;} } class C : B { public override B P {get; set;} }

It looks like the check will succeed for C.P, but the override is not valid because we cannot override the setter this way. #ByDesign

There is a separate check for the accessors which will fail. This is the existing code for this scenario.

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

AlekseyTs · 2020-04-28T20:55:03Z

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

+        {
+            var method = comp.GlobalNamespace.GetMember(methodName);
+            var overridden = method.GetOverriddenMember();
+            Assert.Equal(overriddenMethodName, overridden.ToTestDisplayString());


Assert.Equal(overriddenMethodName, overridden.ToTestDisplayString()); [](start = 12, length = 69)

Consider also asserting the same for method. Makes it easier to see what type did we assume for the overriding method, strengthens the tests. #Closed

Did you mean adding a parameter so we can assert the display name of the method? The syntax we use for looking up a member is not compatible with the syntax generated for a display string.

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

Did you mean adding a parameter so we can assert the display name of the method?

Yes

In reply to: 416985050 [](ancestors = 416985050,416916024)

AlekseyTs · 2020-04-28T21:17:14Z

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

+}
+public class Derived : Base
+{
+    public override string M1 => null;


public override string M1 => null; [](start = 4, length = 34)

When there are multiple equal lines, we usually add some distinct comments to them to make it easier to navigate the base-line #Closed

AlekseyTs · 2020-04-28T21:59:19Z

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

+            VerifyOverride(comp, "Derived2.get_P", "System.String Derived.P.get");
+            VerifyNoOverride(comp, "Derived2.set_P");
+            comp = CreateCompilation(source, parseOptions: TestOptions.WithCovariantReturns).VerifyDiagnostics(
+                // (12,53): error CS0546: 'Derived2.P.set': cannot override because 'Derived.P' does not have an overridable set accessor


// (12,53): error CS0546: 'Derived2.P.set': cannot override because 'Derived.P' does not have an overridable set accessor [](start = 16, length = 121)

I think this diagnostics is confusing because it implies that either there is no setter, or it is not virtual/overridable. I think this is because we are detecting this problem in the "wrong" place. #Closed

This is an existing diagnostic for this identical scenario. I am not attempting to improve it in the context of this set of changes.

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

This is an existing diagnostic for this identical scenario. I am not attempting to improve it in the context of this set of changes.

I think we should improve that because before one could almost never run into this existing diagnostics and it would most likely mean what it says. For this scenario, the diagnostic should be improved to explain what is wrong in a way the people can understand. Please, either ad a PROTOTYPE comment or open an issue for this.

In reply to: 416983969 [](ancestors = 416983969,416949513)

Recorded as #43794

In reply to: 417027167 [](ancestors = 417027167,416983969,416949513)

AlekseyTs · 2020-04-28T22:01:25Z

Done with review pass (iteration 5) #Closed

…riding method. Ensure that the overriden-symbol API behaves the same with or without the featue.

gafter · 2020-04-29T01:08:34Z

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

@agocke Can you please review this?

gafter · 2020-04-29T17:24:50Z

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

@agocke Can you please review this?

AlekseyTs

LGTM (iteration 6)

agocke

LGTM aside from question about combining checks

agocke · 2020-05-04T20:28:10Z

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

                                suppressAccessors = true; //we get really unhelpful errors from the accessor if the ref kind is mismatched
                            }
-                            else if (!IsValidOverrideReturnType(overridingProperty, overridingMemberType, overriddenMemberType, diagnostics))
+                            else if (overridingProperty.SetMethod is null ?


Would it be easier to simply call GetOwnOrInheritedSetMethod here? Like we do below for doing the nullable checking? #Resolved

I will respond to this comment in #43797 #Resolved

No, because we need to treat the presence of a set method declared on the property differently from the absence of one, even if one is inherited.

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

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

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

gafter self-assigned this Apr 25, 2020

Neal Gafter added 2 commits April 27, 2020 12:12

- Emit language version suggestion for property overrides when applic…

5210155

…able. - Add test for override properties with setters. - Test relaxed rules for override in source vs a metadata base type.

Add a number of tests requested in dotnet#43576

51e69fb

gafter force-pushed the covariant-returns-part2 branch from 2f9edfa to 51e69fb Compare April 27, 2020 19:12

gafter marked this pull request as ready for review April 27, 2020 19:14

gafter requested a review from a team as a code owner April 27, 2020 19:14

gafter requested review from AlekseyTs and agocke April 27, 2020 19:14

This was referenced Apr 27, 2020

Covariant returns part 3 #43720

Closed

Covariant returns part 3 #43721

Closed

Covariant returns part 3 #43722

Closed

Add testing for symbol API (when override is in source)

72ea185

gafter closed this Apr 27, 2020

gafter reopened this Apr 27, 2020

Neal Gafter added 2 commits April 27, 2020 19:37

Fix small typo with big impact.

4b4104b

Add tests from test plan.

3693059

AlekseyTs reviewed Apr 28, 2020

View reviewed changes

Enhance tests to include verification of the display name of the over…

96aca5f

…riding method. Ensure that the overriden-symbol API behaves the same with or without the featue.

gafter mentioned this pull request Apr 29, 2020

Possible diagnostic improvement in covariant returns with set accessor #43794

Open

AlekseyTs approved these changes Apr 29, 2020

View reviewed changes

agocke approved these changes May 4, 2020

View reviewed changes

gafter merged commit 60f7f3c into dotnet:features/covariant-returns May 4, 2020

Conversation

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

Uh oh!

Uh oh!

AlekseyTs Apr 28, 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!

gafter Apr 28, 2020 • edited 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 Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 28, 2020 • edited 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 Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

gafter commented Apr 29, 2020

Uh oh!

gafter commented Apr 29, 2020

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

agocke May 4, 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 4, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gafter commented Apr 25, 2020 •

edited

Loading

AlekseyTs Apr 28, 2020 •

edited by gafter

Loading

gafter Apr 28, 2020 •

edited

Loading

AlekseyTs Apr 28, 2020 •

edited

Loading

AlekseyTs Apr 28, 2020 •

edited

Loading

AlekseyTs Apr 28, 2020 •

edited

Loading

AlekseyTs Apr 29, 2020 •

edited

Loading

AlekseyTs commented Apr 28, 2020 •

edited

Loading

agocke May 4, 2020 •

edited by gafter

Loading

gafter May 4, 2020 •

edited

Loading