Fix nullability check in override/implementation #49723

jcouv · 2020-12-02T05:24:38Z

This PR focuses on user-facing issues related to type substitution affecting override/implementation checks, but without attempting to simplify type substitution at the same time.

Fixes #41368
Fixes #49131
Fixes #49071

Filed #49722 (type of this)

RikkiGibson · 2020-12-02T17:45:23Z

Does this also fix #49071? #Resolved

jcouv · 2020-12-02T18:20:23Z

@RikkiGibson Thanks! The partials scenario can indeed be fixed with same approach. I'll update this PR> #Resolved

jcouv · 2020-12-02T22:58:54Z

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

+        }
+
+        [Fact]
+        public void DefaultParameterValue()


📝 some of the tests that I added are not so relevant to this new PR, I just moved all the new tests from my earlier PR. Seems okay to keep them. #Resolved

AlekseyTs · 2020-12-03T18:26:40Z

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

@@ -479,6 +483,10 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
            {
                newAnnotation = NullableAnnotation;
            }
+            else if (newTypeWithModifiers.NullableAnnotation == NullableAnnotation.Ignored)


newTypeWithModifiers.NullableAnnotation == NullableAnnotation.Ignore [](start = 21, length = 68)

It looks like we already have a special handling for indexed type parameters that has the same effect. If that is the case, I think we should reuse this mechanism rather than going through the trouble of dealing with the new annotation. We might need to use ConstructedFrom in diagnostics in affected components though. #Closed

We can't use indexed type parameters because (1) they don't have names, and (2) they don't have constraints (so the logic in AreParameterAnnotationsCompatible which simulates an assignment produces wrong values/states in GetParameterState).
We could fix the first problem by introducing a new type parameter symbol. But the second problem is tougher, and I don't see a good way to solve it.
So using a new annotation seems a better fit to solve this problem. #Resolved

We can't use indexed type parameters because (1) they don't have names, and (2) they don't have constraints (so the logic in AreParameterAnnotationsCompatible which simulates an assignment produces wrong values/states in GetParameterState).

I would like to get a better understanding of these issues. Let's sync offline

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

Would it be possible to remove the special handling for indexed type parameters and use the new annotation instead?

In reply to: 535509257 [](ancestors = 535509257,535501474)

Would it be possible to remove the special handling for indexed type parameters and use the new annotation instead?

Interesting idea. Thanks. Pushed a commit for that change #Resolved

AlekseyTs · 2020-12-04T00:02:55Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

@@ -721,7 +721,7 @@ private static void PartialMethodConstraintsChecks(SourceOrdinaryMethodSymbol de
            }

            var typeParameters2 = implementation.TypeParameters;
-            var indexedTypeParameters = IndexedTypeParameterSymbol.Take(arity);
+            var indexedTypeParameters = TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(arity));


TypeParametersAsTypeSymbolsWithIgnoredAnnotations [](start = 48, length = 49)

It looks like we are missing a change in CompilationContext in EE #Closed

AlekseyTs · 2020-12-04T00:04:36Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

@@ -721,7 +721,7 @@ private static void PartialMethodConstraintsChecks(SourceOrdinaryMethodSymbol de
            }

            var typeParameters2 = implementation.TypeParameters;
-            var indexedTypeParameters = IndexedTypeParameterSymbol.Take(arity);
+            var indexedTypeParameters = TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(arity));


Take [](start = 125, length = 4)

Consider changing what this method returns instead. It looks like majority of call sites actually need TypeWithAnnotations. For the case of just type parameters, we can add a special helper. #Closed

I was intentionally suggesting to keep the same name. There are often overloads that can handle arrays of both types, however we want default case to be TypeWithAnnotations with an ignored annotation. This way, every consumer of this method, existing or added later will be automatically guided into the right direction and will not require changes. Changes will be required only for exceptions, which aren't that many.

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

AlekseyTs · 2020-12-04T15:52:35Z

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

@@ -32,5 +32,10 @@ internal enum NullableAnnotation : byte
        /// Type is annotated with '?' - string?, T?.
        /// </summary>
        Annotated,
+
+        /// <summary>
+        /// Used for indexed type parameters and used locally in override/implementation checks.


Used for indexed type parameters and used locally in override/implementation checks. [](start = 12, length = 84)

Should probably provide details about how exactly it is ignored. So that one could predict the impact of usage of this annotation #Closed

AlekseyTs · 2020-12-04T15:56:32Z

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

+                NullableAnnotation.Oblivious when type.IsValueType => CodeAnalysis.NullableAnnotation.NotAnnotated,
+                NullableAnnotation.Oblivious => CodeAnalysis.NullableAnnotation.None,
+
+                // May occur as type argument in symbols in diagnostic


// May occur as type argument in symbols in diagnostic [](start = 16, length = 54)

I think we should be able to avoid this if we use ConstructedFrom for diagnostic instead. #Closed

I think we should really make an effort to not leak a symbol with an annotation like that. Otherwise, equality is likely to misbehave, etc.

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

AlekseyTs · 2020-12-04T16:09:29Z

        switch (nullableAnnotation)

Can we assert that if annotation is Ignored then the type is a type parameter? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs:89 in 322f89a. [](commit_id = 322f89a, deletion_comment = False)

AlekseyTs · 2020-12-04T16:26:13Z

Done with review pass (commit 8) #Closed

AlekseyTs · 2020-12-04T18:45:32Z

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

@@ -1770,7 +1772,7 @@ internal static void CheckNullableReferenceTypeMismatchOnImplementingMember(Type

                            if (implementedMethod.IsGenericMethod)
                            {
-                                implementedMethod = implementedMethod.Construct(implementingMethod.TypeArgumentsWithAnnotations);
+                                implementedMethod = implementedMethod.Construct(TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(implementingMethod.TypeParameters));
                            }

                            SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride(


It feels like the code in this function should also dig to constructed from as appropriate. #Closed

The lambdas that are passed in do use ConstrcutedFrom (actually, they don't need to dig into derived/implementating method, I was hasty and will revert that part). The rest of the method is fine (see earlier comment).

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

AlekseyTs · 2020-12-04T18:52:25Z

                        checkValidNullableMethodOverride(

Please confirm that this method digs to ConstructedFrom as appropriate #Closed

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

AlekseyTs · 2020-12-04T18:54:37Z

        SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride(

Please confirm this method digs to ConstructedFrom as appropriate. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs:690 in dbfb16b. [](commit_id = dbfb16b, deletion_comment = False)

AlekseyTs · 2020-12-04T18:56:11Z

Done with review pass (commit 9) #Closed

jcouv · 2020-12-04T19:20:52Z

        SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride(

SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride is fine. It doesn't report on the base method, which is getting the substitution and neither do the lambdas that are passed in by various callers

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

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs:690 in dbfb16b. [](commit_id = dbfb16b, deletion_comment = False)

jcouv · 2020-12-04T19:21:58Z

                        checkValidNullableMethodOverride(

This local function only reports on return and parameter types, but does not use method symbols in diagnostics

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

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

AlekseyTs

LGTM (commit 10)

jcouv · 2020-12-04T20:44:28Z

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

+            var method3 = model.GetSymbolInfo(invocations[1]).Symbol;
+            Assert.True(method3.IsDefinition);
+
+            // Note: definitions and subsituted symbols are not equal even when ignoring nullability


// Note: definitions and subsituted symbols are not equal even when ignoring nullability [](start = 12, length = 88)

@AlekseyTs @cston FYI This behavior seems wrong, but I didn't file an issue for this because from our discussion, trying to adjust this seems very risky (likely performance impact and other unknowns). #Resolved

This behavior seems wrong, but I didn't file an issue for this because from our discussion, trying to adjust this seems very risky (likely performance impact and other unknowns).

I do not remember us discussing this, unless this somehow involves "Ignored" annotation. It doesn't look like it does. I think we should fix this. Not necessary in this PR, unless the behavior is related to the changes in it.

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

Okay. Thanks for the clarification. Filed #49798

Right, we discussed this for comparisons dealing with Ignored annotation, but we have the same problem for existing annotations (shortcuts we take when one of the symbols is a definition). #Resolved

AlekseyTs · 2020-12-04T22:35:10Z

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

+            Assert.True(method1.Equals(method3, SymbolEqualityComparer.IncludeNullability));
+            Assert.True(method3.Equals(method1, SymbolEqualityComparer.Default));
+            Assert.True(method3.Equals(method1, SymbolEqualityComparer.IncludeNullability));
+        }


For true cases, please assert GetHashCode equality #Resolved

RikkiGibson · 2020-12-04T23:25:40Z

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

+                NullableAnnotation.Oblivious when type.IsValueType => CodeAnalysis.NullableAnnotation.NotAnnotated,
+                NullableAnnotation.Oblivious => CodeAnalysis.NullableAnnotation.None,
+
+                NullableAnnotation.Ignored => CodeAnalysis.NullableAnnotation.None,


what motivated the choice here to not throw UnexpectedValue in retail builds? #Resolved

what motivated the choice here to not throw UnexpectedValue in retail builds?

Not worth crashing compiler over this

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

Returning None would be the right behavior if we expected symbols to leak through public APIs in some way, so doesn't seem harmful to handle (I don't think it's worth crashing if we have such a bug).
I don't feel very strongly about it

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

RikkiGibson · 2020-12-04T23:26:01Z

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

@@ -27,6 +27,11 @@ internal static ImmutableArray<TypeWithAnnotations> TypeParametersAsTypeSymbolsW
            return typeParameters.SelectAsArray((tp) => TypeWithAnnotations.Create(tp));
        }

+        internal static ImmutableArray<TypeWithAnnotations> TypeParametersAsTypeSymbolsWithIgnoredAnnotations(ImmutableArray<TypeParameterSymbol> typeParameters)
+        {
+            return typeParameters.SelectAsArray((tp) => TypeWithAnnotations.Create(tp, NullableAnnotation.Ignored));


nit: static

RikkiGibson · 2020-12-04T23:32:27Z

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

@@ -37280,8 +37280,6 @@ public class Derived : Base
 }
 ";
            var comp = CreateNullableCompilation(new[] { NotNullWhenAttributeDefinition, source });
-            // Note: we're missing warnings on F1 because the overridden method with substituted T from Derived has an oblivious T
-            // https://github.com/dotnet/roslyn/issues/41368


It doesn't look like new diagnostics were added here. Was this issue already solved before this PR? #Resolved

The comment was incorrect or out-of-date (we already had the appropriate warnings on F1 for this scenario)

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

RikkiGibson · 2020-12-04T23:53:24Z

src/Compilers/Core/Portable/Symbols/ISymbol.cs

@@ -203,7 +203,7 @@ public interface ISymbol : IEquatable<ISymbol?>
        ISymbol OriginalDefinition { get; }

        void Accept(SymbolVisitor visitor);
-        TResult Accept<TResult>(SymbolVisitor<TResult> visitor);
+        TResult? Accept<TResult>(SymbolVisitor<TResult> visitor);


We may want to test insert this change before merging to roslyn master, so that we can prepare fixes for any VS-side build breaks.

Should be okay (I've run a test insertion on previous PR with similar effect), but it's good to be sure. Started https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=4276225&view=results

Succeeded. I'll go ahead and merge

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Dec 2, 2020

jcouv self-assigned this Dec 2, 2020

jcouv force-pushed the check-overrides branch 3 times, most recently from c34b684 to c5394af Compare December 2, 2020 16:04

Fix nullability check in override/implementation

c3db802

jcouv force-pushed the check-overrides branch from c5394af to c3db802 Compare December 2, 2020 16:55

This was referenced Dec 2, 2020

Adjust type substitution for oblivious type argument in not-annotated type parameter #48694

Closed

Implement proposal 1 for type substitution #49441

Closed

Fix partials scenario

62313f7

jcouv marked this pull request as ready for review December 2, 2020 22:55

jcouv requested review from a team as code owners December 2, 2020 22:55

jcouv commented Dec 2, 2020

View reviewed changes

RikkiGibson self-assigned this Dec 2, 2020

AlekseyTs reviewed Dec 3, 2020

View reviewed changes

jcouv added 3 commits December 3, 2020 14:00

Use Ignored for indexed type parameters

8940d30

comment

d5617ae

assertion

e71faba

AlekseyTs reviewed Dec 4, 2020

View reviewed changes

jcouv added 3 commits December 3, 2020 18:46

Fix CompilationContext

3bd5caa

rename

147c3e0

comments

322f89a

runfoapp bot mentioned this pull request Dec 4, 2020

Roslyn failed to fetch git #49142

Closed

jcouv requested a review from RikkiGibson December 4, 2020 15:32

AlekseyTs reviewed Dec 4, 2020

View reviewed changes

Use ConstructedFrom for diagnostics with Ignored annotations in symbols

dbfb16b

AlekseyTs reviewed Dec 4, 2020

View reviewed changes

Remove unnecessary ConstructedFrom

df7ea75

AlekseyTs approved these changes Dec 4, 2020

View reviewed changes

jcouv commented Dec 4, 2020

View reviewed changes

AlekseyTs reviewed Dec 4, 2020

View reviewed changes

Check GetHashCode

b71ca77

RikkiGibson reviewed Dec 4, 2020

View reviewed changes

static

28608b6

RikkiGibson reviewed Dec 4, 2020

View reviewed changes

RikkiGibson approved these changes Dec 4, 2020

View reviewed changes

jcouv merged commit abe0db9 into dotnet:master Dec 5, 2020

ghost added this to the Next milestone Dec 5, 2020

runfoapp bot mentioned this pull request Dec 8, 2020

Roslyn infrastructure failures cause CI to fail #49023

Open

dibarbet modified the milestones: Next, 16.9.P3 Dec 19, 2020

Youssef1313 mentioned this pull request May 12, 2021

Revert "Revert "Require partial method signatures to match" (47576) (#47879)" #53352

Merged

Fix nullability check in override/implementation #49723

Fix nullability check in override/implementation #49723

Conversation

jcouv commented Dec 2, 2020 • edited Loading

RikkiGibson commented Dec 2, 2020 • edited by jcouv Loading

jcouv commented Dec 2, 2020 • edited Loading

jcouv Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 4, 2020 • edited Loading

AlekseyTs commented Dec 4, 2020 • edited Loading

AlekseyTs Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 4, 2020 • edited Loading

AlekseyTs commented Dec 4, 2020 • edited Loading

AlekseyTs commented Dec 4, 2020 • edited Loading

jcouv commented Dec 4, 2020

jcouv commented Dec 4, 2020

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

RikkiGibson Dec 4, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Dec 4, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Dec 2, 2020 •

edited

Loading

RikkiGibson commented Dec 2, 2020 •

edited by jcouv

Loading

jcouv commented Dec 2, 2020 •

edited

Loading

jcouv Dec 2, 2020 •

edited

Loading

AlekseyTs Dec 3, 2020 •

edited

Loading

jcouv Dec 3, 2020 •

edited

Loading

jcouv Dec 3, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs commented Dec 4, 2020 •

edited

Loading

AlekseyTs commented Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited

Loading

AlekseyTs commented Dec 4, 2020 •

edited

Loading

AlekseyTs commented Dec 4, 2020 •

edited

Loading

AlekseyTs commented Dec 4, 2020 •

edited

Loading

jcouv Dec 4, 2020 •

edited

Loading

jcouv Dec 4, 2020 •

edited

Loading

AlekseyTs Dec 4, 2020 •

edited by jcouv

Loading

RikkiGibson Dec 4, 2020 •

edited by jcouv

Loading

RikkiGibson Dec 4, 2020 •

edited by jcouv

Loading