Allow overriding members with nullable variance by agocke · Pull Request #34718 · dotnet/roslyn

agocke · 2019-04-03T07:18:54Z

Implements a design change where overrides are allowed to change the
type of the member as long as there is an implicit nullable reference
conversion from the overriding type to the overridden type according to
the nullable variance rules.

Fixes #23268
Fixes #30958

agocke · 2019-04-03T21:56:25Z

@dotnet/roslyn-compiler for review #Closed

agocke · 2019-04-04T18:41:05Z

@dotnet/roslyn-compiler ping for review #Closed

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

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

gafter

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

agocke · 2019-04-05T18:28:06Z

@cston Did you have any more comments? #Closed

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

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

AlekseyTs · 2019-04-08T22:23:59Z

        var overriddenMembers = overriddenOrHiddenMembers.OverriddenMembers;

What is the overriddenMembers here? Is this a base override, if any, or always the initial declaration of the member in the top-most base type?

It looks like https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-01-07.md suggest that we would want to check nullability against base override.

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs:703 in d9ddb61. [](commit_id = d9ddb61, deletion_comment = False)

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

AlekseyTs · 2019-04-08T22:39:35Z

public override System.Action<string?> Oblivious2(System.Action<string?> x) => throw null!; // warn 3 and 4 // https://github.com/dotnet/roslyn/issues/29851: Should not warn

Can we remove this comment now and resolve the issue once this PR is merged? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:11206 in d9ddb61. [](commit_id = d9ddb61, deletion_comment = False)

AlekseyTs · 2019-04-08T22:40:55Z

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

)); [](start = 131, length = 3)

)); [](start = 131, length = 3)

Consider preserving original formatting #Closed

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

AlekseyTs · 2019-04-08T22:54:03Z

Done with review pass (iteration 6) #Closed

AlekseyTs · 2019-04-18T17:59:46Z

@agocke Could you please resolve merge conflicts and figure out what is wrong with CI failures? #Closed

Implements a design change where overrides are allowed to change the type of the member as long as there is an implicit nullable reference conversion from the overriding type to the overridden type according to the nullable variance rules. Fixes dotnet#23268

AlekseyTs

LGTM (iteration 5)

agocke · 2019-04-26T21:56:51Z

@gafter You looked at an earlier iteration but I've made substantial changes since then. Can you take another look? #Closed

agocke · 2019-04-29T19:31:46Z

ping @gafter #Closed

gafter · 2019-04-30T20:33:15Z

Looking #Closed

gafter · 2019-04-30T20:47:05Z

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

+                    overridingMethod is null ||
+                    compilation is null ||
+                    ((CSharpParseOptions)overridingMemberLocation.SourceTree?.Options)
+                        ?.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes) != true)


This can be compilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes) since all parse trees are checked that they have the same version and features. #Resolved

gafter · 2019-04-30T21:53:59Z

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

+                if (!isValidNullableConversion(
+                        conversions,
+                        overridingMethod.RefKind,
+                        overridingMethod.ReturnTypeWithAnnotations,


overridingMethod.ReturnTypeWithAnnotations [](start = 24, length = 42)

Has one of these been alpha-renamed to match the other, e.g. in case the method is generic and returns T?

gafter · 2019-04-30T21:54:28Z

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

+                for (int i = 0; i < overriddenMethod.ParameterCount; i++)
+                {
+                    var overriddenParameterType = overriddenParameters[i].TypeWithAnnotations;
+                    var overridingParameterType = overridingParameters[i].TypeWithAnnotations;


overridingParameters[i] [](start = 50, length = 23)

Same question: have the methods been alpha-renamed to match each other?

gafter

This needs tests showing the behavior for overriding generic methods, methods in generic classes.

Needs tests for overriding generic methods with variance.

AlekseyTs · 2019-05-01T17:15:30Z

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

-
-                            if (overriddenMember.Kind == SymbolKind.Method && (overriddenMethod = (MethodSymbol)overriddenMember).IsGenericMethod)
-                            {
-                                overriddenParameters = overriddenMethod.Construct(((MethodSymbol)overridingMember).TypeArgumentsWithAnnotations).Parameters;


overriddenParameters = overriddenMethod.Construct(((MethodSymbol)overridingMember).TypeArgumentsWithAnnotations).Parameters; [](start = 32, length = 124)

It looks like @gafter is correct, there is a problem with overriding generic methods and that is because this logic is not preserved.

As I mentioned offline, I think this logic is performed on line 955 above, but this code was using overriddenMember for checking, instead of the variable overriddenMethod, which contains the constructed member.

agocke · 2019-05-01T23:56:25Z

@gafter Anything else? #Closed

jcouv · 2019-05-03T21:55:25Z

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


+            return;
+
            bool isOrContainsErrorType(TypeSymbol typeSymbol)


bool [](start = 12, length = 4)

nit: not related to this PR, static too? #Closed

jcouv · 2019-05-03T22:05:02Z

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

+
+                return;
+
+                static bool isValidNullableConversion(


isValidNullableConversion [](start = 28, length = 25)

If you don't mind, could we move this local function out one level, to avoid nested local functions? #Closed

jcouv · 2019-05-03T22:20:55Z

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

+                // (17,29): warning CS8610: Nullability of reference types in type of parameter 't' doesn't match overridden member.
+                //     public override List<T> M<T>(List<T> t) => null!;
+                Diagnostic(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnOverride, "M").WithArguments("t").WithLocation(17, 29));
+        }


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

Some test ideas:

I think we should cover some generic cases as well:

class C : Base<object_> // where object_ is object? or object! or object~ { override object_ M() { ... } // ditto }

We should also cover some scenarios with multiple layers of overrides (with various combination of ?, ! and ~ for string_:

class A { virtual string_ Property { get; } } class B : A { override string_ Property { get; } } class C : B { override string_ Property { get; } }

Could you also add some tests verifying the behavior for callers?

new Derived().Property.ToString(); // warning depends on overriding member, not overridden member

It would also be good to cover a sublety in isValidNullableConversion: it compares ignoring all differences, except nullability.

class Base { virtual object? M() ... } class Derived : Base { override dynamic M() ... }

jcouv · 2019-05-03T22:27:03Z

public override event System.Action<string> E2; // 2

consider removing or updating the numbered comments

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:11156 in 0c532f5. [](commit_id = 0c532f5, deletion_comment = False)

jcouv · 2019-05-03T22:37:12Z

public override event System.Action<string?> E1 {add {} remove{}}

Feels like there should be a mismatch warning here.
There are multiple ways to interact with an event: add, remove, invocation and copy. The copy seems dangerous in override scenarios.

using System;
public class Derived : Base {
    override event Action<string?> Event;
    public void M() {
        Action<string?> x = Event; // is Event abiding by the contract of Base or of C? Is it safe to give it a null argument?
    }
}

#Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:11148 in 0c532f5. [](commit_id = 0c532f5, deletion_comment = False)

jcouv · 2019-05-03T22:46:27Z

    var e2 = E2;

It would be good to verify the result type that NullableWalker gets for E2 and similar override scenarios (also for properties, method groups, etc).
You could do _ = E2 /*T:System.Action<string>*/; and call comp.VerifyTypes();.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:11456 in 0c532f5. [](commit_id = 0c532f5, deletion_comment = False)

AlekseyTs · 2019-05-03T22:49:46Z

public override event System.Action<string?> E1 {add {} remove{}}

There are multiple ways to interact with an event: add, remove, invocation and copy. The copy seems dangerous in override scenarios.

There is no concept of copy for events. Also, it looks like there is a warning for this line:

                // (18,50): warning CS8610: Nullability of reference types in type of parameter 'value' doesn't match overridden member.
                //     public override event System.Action<string?> E1 {add {} remove{}}
                Diagnostic(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnOverride, "E1").WithArguments("value").WithLocation(18, 50),

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:11148 in 0c532f5. [](commit_id = 0c532f5, deletion_comment = False)

jcouv · 2019-05-03T23:09:41Z

public override event System.Action<string?> E1 {add {} remove{}}

There is no concept of copy for events.

I'm not sure what you're arguing (terminology or use case). Action<string> x = Event; is valid code.

it looks like there is a warning for this line

Ah, thanks.
I don't understand why that warning is produced by current implementation, then. override void Add(Action<string?>) seems a valid override for virtual void Add(Action<string>).

In reply to: 489263473 [](ancestors = 489263473,489261259)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:11148 in 0c532f5. [](commit_id = 0c532f5, deletion_comment = False)

jcouv

Done with review pass (iteration 6)

jcouv · 2019-05-03T23:24:36Z

public override event System.Action<string?> E1 {add {} remove{}}

Resolved offline.
I now understand why the warning is produced on the add method :-) Thanks!

In reply to: 489266871 [](ancestors = 489266871,489263473,489261259)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:11148 in 0c532f5. [](commit_id = 0c532f5, deletion_comment = False)

agocke · 2019-05-04T02:16:44Z

@jcouv Added tests for overriding a constructed type. The test for dynamic/object? is superseded by the test we have in the dynamic tests for object/dynamic. The multiple levels of overrides I have additional tests for.

jcouv

LGTM Thanks (iteration 8)

jinujoseph added the Area-Compilers label Apr 3, 2019

agocke force-pushed the override-signature-variance-classes branch from 959c234 to 418e01a Compare April 3, 2019 20:21

agocke changed the title ~~Allow overriding members with nullable conversions~~ Allow overriding members with nullable covariance Apr 3, 2019

agocke marked this pull request as ready for review April 3, 2019 20:23

agocke requested a review from a team as a code owner April 3, 2019 20:23

agocke force-pushed the override-signature-variance-classes branch from 418e01a to 6c4cee4 Compare April 3, 2019 21:53

agocke changed the title ~~Allow overriding members with nullable covariance~~ Allow overriding members with nullable variance Apr 3, 2019

CyrusNajmabadi mentioned this pull request Apr 4, 2019

Simplify handling of 'enter' in formatting. #33983

Merged

gafter reviewed Apr 5, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs Outdated Show resolved Hide resolved

gafter reviewed Apr 5, 2019

View reviewed changes

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs Outdated Show resolved Hide resolved

gafter previously approved these changes Apr 5, 2019

View reviewed changes

jaredpar reviewed Apr 5, 2019

View reviewed changes

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs Outdated Show resolved Hide resolved

jcouv reviewed Apr 5, 2019

View reviewed changes

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

jcouv reviewed Apr 8, 2019

View reviewed changes

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Apr 8, 2019

View reviewed changes

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Apr 8, 2019

View reviewed changes

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Apr 8, 2019

View reviewed changes

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

AlekseyTs reviewed Apr 8, 2019

View reviewed changes

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

AlekseyTs reviewed Apr 8, 2019

View reviewed changes

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

agocke force-pushed the override-signature-variance-classes branch from 2c79032 to 2c8a461 Compare April 15, 2019 21:21

jcouv added this to the 16.1.P3 milestone Apr 18, 2019

Respond to PR comments

799a6df

AlekseyTs approved these changes Apr 26, 2019

View reviewed changes

gafter reviewed Apr 30, 2019

View reviewed changes

AlekseyTs reviewed May 1, 2019

View reviewed changes

Respond to PR comments

0c532f5

jcouv reviewed May 3, 2019

View reviewed changes

Respond to PR comments

f15b333

Add test for consumer side

b7d4c51

jcouv approved these changes May 4, 2019

View reviewed changes

agocke merged commit 472276a into dotnet:master May 4, 2019

agocke deleted the override-signature-variance-classes branch May 4, 2019 07:17

RikkiGibson mentioned this pull request May 21, 2019

Allow nullable variance for in/out params when converting method group to delegate #35832

Merged

Conversation

agocke commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke commented Apr 3, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke commented Apr 4, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agocke commented Apr 5, 2019 • edited by jcouv 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 Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs Apr 8, 2019 • 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!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

agocke commented Apr 26, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

gafter Apr 30, 2019 • edited by jcouv 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 30, 2019

Choose a reason for hiding this comment

Uh oh!

gafter Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 1, 2019

Choose a reason for hiding this comment

Uh oh!

agocke May 1, 2019 • 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!

agocke commented May 1, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

agocke commented Apr 3, 2019 •

edited

Loading

agocke commented Apr 3, 2019 •

edited by jcouv

Loading

agocke commented Apr 4, 2019 •

edited by jcouv

Loading

agocke commented Apr 5, 2019 •

edited by jcouv

Loading

AlekseyTs commented Apr 8, 2019 •

edited

Loading

AlekseyTs commented Apr 8, 2019 •

edited

Loading

AlekseyTs Apr 8, 2019 •

edited

Loading

AlekseyTs commented Apr 8, 2019 •

edited

Loading

AlekseyTs commented Apr 18, 2019 •

edited

Loading

agocke commented Apr 26, 2019 •

edited by gafter

Loading

agocke commented Apr 29, 2019 •

edited by gafter

Loading

gafter commented Apr 30, 2019 •

edited

Loading

gafter Apr 30, 2019 •

edited by jcouv

Loading

agocke May 1, 2019 •

edited

Loading

agocke commented May 1, 2019 •

edited by jcouv

Loading

jcouv May 3, 2019 •

edited

Loading

jcouv May 3, 2019 •

edited

Loading

jcouv May 3, 2019 •

edited

Loading

jcouv commented May 3, 2019 •

edited

Loading