Support nullable variance in interface and partial method implementation. #36663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

AlekseyTs merged 3 commits into dotnet:master from AlekseyTs:Issue35227

Jun 24, 2019

Contributor

AlekseyTs commented Jun 21, 2019

Closes #35227.


          Support nullable variance in interface and partial method implementat…

d2fbca4

…ion.

Closes dotnet#35227.

AlekseyTs added Area-Compilers Feature - Nullable Reference Types labels

AlekseyTs requested a review from agocke

June 21, 2019 17:32

AlekseyTs requested a review from a team as a code owner

June 21, 2019 17:32

Contributor Author

AlekseyTs commented Jun 21, 2019 •

edited

Loading

@agocke, @dotnet/roslyn-compiler Please review. #Closed

jcouv added this to the 16.2.P4 milestone


          Add more tests

aff03fd

333fred reviewed

View reviewed changes

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

    
                              {

                                  implementedMember = interfaceMember;

                              }

                                  Action<DiagnosticBag, MethodSymbol, MethodSymbol, (TypeSymbol implementingType, bool isExplicit)> reportMismatchInReturnType =

Member

333fred Jun 21, 2019 •

edited by AlekseyTs

Loading

Consider making these local functions so we can make them static and easily verify that they are not capturing. #ByDesign

Member

jcouv Jun 21, 2019

+1 on that

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

Contributor Author

AlekseyTs Jun 21, 2019

Delegates for lambdas are cached automatically.

In reply to: 296389943 [](ancestors = 296389943,296380417)

Member

jcouv Jun 21, 2019

FWIW, for me it's less about caching and more about readability.

In reply to: 296390411 [](ancestors = 296390411,296389943,296380417)

333fred reviewed

View reviewed changes

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

    
                                                      checkParameters: overridingProperty.SetMethod is null);

                                                      checkParameters: overridingProperty.SetMethod is null ||

                                                                       overriddenGetMethod?.AssociatedSymbol != overriddenProperty ||

                                                                       overriddenProperty.GetOwnOrInheritedSetMethod()?.AssociatedSymbol != overriddenProperty);

Member

333fred Jun 21, 2019 •

edited by AlekseyTs

Loading

Could consider extracting this check to a helper, as we use it in multiple places. #WontFix

Contributor Author

AlekseyTs Jun 21, 2019

Could consider extracting this check to a helper, as we use it in multiple places.

I do not believe it is worth doing just for portion of the condition, the entire condition is different.

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

333fred reviewed

View reviewed changes

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

    
                              }

                              CheckValidNullableMethodOverride(overridingMethod.DeclaringCompilation, overriddenMethod, overridingMethod, diagnostics,

                                                               checkReturnType ?

                                                                  (diagnostics, overriddenMethod, overridingMethod, location) => diagnostics.Add(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnOverride, location) :

Member

333fred Jun 21, 2019 •

edited by AlekseyTs

Loading

overriddenMethod, overridingMethod [](start = 66, length = 34)

Consider using _ for ignored parameter. #WontFix

Contributor Author

AlekseyTs Jun 21, 2019

Consider using _ for ignored parameter.

I see no point in doing this, it is not hard for me to type the names and it is better for readability in my opinion. Also, I do not think both parameters can be named _.

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

jcouv reviewed

View reviewed changes

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

    
                                                                  (diagnostics, overriddenMethod, overridingMethod, location) => diagnostics.Add(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnOverride, location) :

                                                                  (Action<DiagnosticBag, MethodSymbol, MethodSymbol, Location>)null,

                                                               checkParameters ?

                                                                   (diagnostics, overriddenMethod, overridingMethod, overridingParameter, location) =>

Member

jcouv Jun 21, 2019 •

edited by AlekseyTs

Loading

(diagnostics, overriddenMethod, overridingMethod, overridingParameter, location) => [](start = 53, length = 83)

nit: consider using a local function for those funcs in various invocations of CheckValidNullableEventOverride #ByDesign

Contributor Author

AlekseyTs Jun 21, 2019

nit: consider using a local function for those funcs in various invocations of CheckValidNullableEventOverride

Lambdas are used intentionally to take advantage of automatic caching.

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

333fred reviewed

View reviewed changes

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

    
                              return;

                          }

                          if (reportMismatchInParameterType == null)

Member

333fred Jun 21, 2019 •

edited

Loading

Consider moving this if before the previous one, and removing the reportMismatchInParameterType != null check from that one. #Closed

Member

333fred Jun 21, 2019

Wait, ignore. Misread parameter for return.

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

jcouv reviewed

View reviewed changes

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

    
                          }

                      }

                      private static bool CheckValidNullableOverride(

Member

jcouv Jun 21, 2019 •

edited by AlekseyTs

Loading

CheckValidNullableOverride [](start = 28, length = 26)

nit: name of the method is a bit misleading as it doesn't actually perform a check. Maybe ShouldCheck...? #Resolved

333fred reviewed

View reviewed changes

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

    
                          Symbol overriddenMember,

                          Symbol overridingMember)

                      {

                          if (overriddenMember is null ||

Member

333fred Jun 21, 2019 •

edited by AlekseyTs

Loading

Nit: you could just return ! this condition. #Resolved

Member

333fred commented Jun 21, 2019 •

edited by AlekseyTs

Loading

Done review pass (commit 2). Mostly looks good, just a couple of small formatting suggestions. #Resolved

Member

jcouv commented Jun 21, 2019 •

edited by AlekseyTs

Loading

public abstract event System.Action<string> E1;

If not covered elsewhere, consider extending this test to add event Action<string?> in the base which is overridden with event Action<string>. Should produce no warning. #Resolved

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

Contributor Author

AlekseyTs commented Jun 21, 2019 •

edited

Loading

public abstract event System.Action<string> E1;

If not covered elsewhere, consider extending this test to add event Action<string?> in the base which is overridden with event Action. Should produce no warning.

I find existing coverage sufficient.

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

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

jcouv approved these changes

View reviewed changes

Member

jcouv left a comment

LGTM Thanks (iteration 2)


          PR feedback

ca801b8

333fred approved these changes

View reviewed changes

Member

333fred left a comment

LGTM (commit 3)

AlekseyTs merged commit d16d3a5 into dotnet:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Nullable Reference Types