Fix nullability check in override/implementation (#49723)

src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs

@@ -750,7 +750,7 @@ private static ImmutableArray<Symbol> PerformCrefOverloadResolution(ArrayBuilder
                             // CONSIDER: we might want to reuse this method symbol (as long as the MethodKind and Vararg-ness match).
                             signatureMember = new SignatureOnlyMethodSymbol(
                                 methodKind: candidateMethodKind,
-                                typeParameters: IndexedTypeParameterSymbol.Take(signatureMemberArity),
+                                typeParameters: IndexedTypeParameterSymbol.TakeSymbols(signatureMemberArity),
                                 parameters: parameterSymbols,
                                 // This specific comparer only looks for varargs.
                                 callingConvention: candidateMethodIsVararg ? Microsoft.Cci.CallingConvention.ExtraArguments : Microsoft.Cci.CallingConvention.HasThis,

src/Compilers/CSharp/Portable/Emitter/EditAndContinue/CSharpSymbolMatcher.cs

@@ -743,7 +743,7 @@ private static MethodSymbol SubstituteTypeParameters(MethodSymbol method)
                     return method;
                 }
 
-                return method.Construct(IndexedTypeParameterSymbol.Take(n).Cast<TypeParameterSymbol, TypeSymbol>());
+                return method.Construct(IndexedTypeParameterSymbol.Take(n));
             }
 
             private bool AreNamedTypesEqual(NamedTypeSymbol type, NamedTypeSymbol other)

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

@@ -32,5 +32,16 @@ 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.
+        /// When substituting a type parameter with Ignored annotation into some original type parameter
+        /// with some other annotation, the result is the annotation from the original symbol.
+        ///
+        /// T annotated + (T -> U ignored) = U annotated
+        /// T oblivious + (T -> U ignored) = U oblivious
+        /// T not-annotated + (T -> U ignored) = U not-annotated
+        /// </summary>
+        Ignored,
     }
 }

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

@@ -5,6 +5,7 @@
 #nullable disable
 
 using System.Collections.Immutable;
+using System.Diagnostics;
 using Microsoft.CodeAnalysis.CSharp.Symbols;
 using Roslyn.Utilities;
 
@@ -25,37 +26,55 @@ internal static class NullableAnnotationExtensions
         /// Join nullable annotations from the set of lower bounds for fixing a type parameter.
         /// This uses the covariant merging rules. (Annotated wins over Oblivious which wins over NotAnnotated)
         /// </summary>
-        public static NullableAnnotation Join(this NullableAnnotation a, NullableAnnotation b) => (a < b) ? b : a;
+        public static NullableAnnotation Join(this NullableAnnotation a, NullableAnnotation b)
+        {
+            Debug.Assert(a != NullableAnnotation.Ignored);
+            Debug.Assert(b != NullableAnnotation.Ignored);
+            return (a < b) ? b : a;
+        }
 
         /// <summary>
         /// Meet two nullable annotations for computing the nullable annotation of a type parameter from upper bounds.
         /// This uses the contravariant merging rules. (NotAnnotated wins over Oblivious which wins over Annotated)
         /// </summary>
-        public static NullableAnnotation Meet(this NullableAnnotation a, NullableAnnotation b) => (a < b) ? a : b;
+        public static NullableAnnotation Meet(this NullableAnnotation a, NullableAnnotation b)
+        {
+            Debug.Assert(a != NullableAnnotation.Ignored);
+            Debug.Assert(b != NullableAnnotation.Ignored);
+            return (a < b) ? a : b;
+        }
 
         /// <summary>
         /// Return the nullable annotation to use when two annotations are expected to be "compatible", which means
         /// they could be the same. These are the "invariant" merging rules. (NotAnnotated wins over Annotated which wins over Oblivious)
         /// </summary>
-        public static NullableAnnotation EnsureCompatible(this NullableAnnotation a, NullableAnnotation b) =>
-            (a, b) switch
+        public static NullableAnnotation EnsureCompatible(this NullableAnnotation a, NullableAnnotation b)
+        {
+            Debug.Assert(a != NullableAnnotation.Ignored);
+            Debug.Assert(b != NullableAnnotation.Ignored);
+            return (a, b) switch
             {
                 (NullableAnnotation.Oblivious, _) => b,
                 (_, NullableAnnotation.Oblivious) => a,
                 _ => a < b ? a : b,
             };
+        }
 
         /// <summary>
         /// Merges nullability.
         /// </summary>
-        public static NullableAnnotation MergeNullableAnnotation(this NullableAnnotation a, NullableAnnotation b, VarianceKind variance) =>
-            variance switch
+        public static NullableAnnotation MergeNullableAnnotation(this NullableAnnotation a, NullableAnnotation b, VarianceKind variance)
+        {
+            Debug.Assert(a != NullableAnnotation.Ignored);
+            Debug.Assert(b != NullableAnnotation.Ignored);
+            return variance switch
             {
                 VarianceKind.In => a.Meet(b),
                 VarianceKind.Out => a.Join(b),
                 VarianceKind.None => a.EnsureCompatible(b),
                 _ => throw ExceptionUtilities.UnexpectedValue(variance)
             };
+        }
 
         /// <summary>
         /// The attribute (metadata) representation of <see cref="NullableAnnotation.NotAnnotated"/>.
@@ -105,18 +124,25 @@ internal static CodeAnalysis.NullableAnnotation ToPublicAnnotation(this TypeWith
         internal static ImmutableArray<CodeAnalysis.NullableAnnotation> ToPublicAnnotations(this ImmutableArray<TypeWithAnnotations> types) =>
             types.SelectAsArray(t => t.ToPublicAnnotation());
 
-        internal static CodeAnalysis.NullableAnnotation ToPublicAnnotation(TypeSymbol type, NullableAnnotation annotation) =>
-            annotation switch
+        internal static CodeAnalysis.NullableAnnotation ToPublicAnnotation(TypeSymbol type, NullableAnnotation annotation)
+        {
+            Debug.Assert(annotation != NullableAnnotation.Ignored);
+            return annotation switch
             {
-                CSharp.NullableAnnotation.Annotated => CodeAnalysis.NullableAnnotation.Annotated,
-                CSharp.NullableAnnotation.NotAnnotated => CodeAnalysis.NullableAnnotation.NotAnnotated,
+                NullableAnnotation.Annotated => CodeAnalysis.NullableAnnotation.Annotated,
+                NullableAnnotation.NotAnnotated => CodeAnalysis.NullableAnnotation.NotAnnotated,
+
                 // A value type may be oblivious or not annotated depending on whether the type reference
                 // is from source or metadata. (Binding using the #nullable context only when setting the annotation
                 // to avoid checking IsValueType early.) The annotation is normalized here in the public API.
-                CSharp.NullableAnnotation.Oblivious when type.IsValueType => CodeAnalysis.NullableAnnotation.NotAnnotated,
-                CSharp.NullableAnnotation.Oblivious => CodeAnalysis.NullableAnnotation.None,
+                NullableAnnotation.Oblivious when type.IsValueType => CodeAnalysis.NullableAnnotation.NotAnnotated,
+                NullableAnnotation.Oblivious => CodeAnalysis.NullableAnnotation.None,
+
+                NullableAnnotation.Ignored => CodeAnalysis.NullableAnnotation.None,
+
                 _ => throw ExceptionUtilities.UnexpectedValue(annotation)
             };
+        }
 
         internal static CSharp.NullableAnnotation ToInternalAnnotation(this CodeAnalysis.NullableAnnotation annotation) =>
             annotation switch

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs

@@ -989,7 +989,7 @@ IEqualityComparer<MethodSymbol> retargetedMethodComparer
                         retargetedType,
                         method.MethodKind,
                         method.CallingConvention,
-                        IndexedTypeParameterSymbol.Take(method.Arity),
+                        IndexedTypeParameterSymbol.TakeSymbols(method.Arity),
                         targetParamsBuilder.ToImmutableAndFree(),
                         method.RefKind,
                         method.IsInitOnly,

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

@@ -82,7 +82,7 @@ private static void GrowPool(int count)
         /// </summary>
         /// <param name="count"></param>
         /// <returns></returns>
-        internal static ImmutableArray<TypeParameterSymbol> Take(int count)
+        internal static ImmutableArray<TypeParameterSymbol> TakeSymbols(int count)
         {
             if (count > s_parameterPool.Length)
             {
@@ -99,6 +99,23 @@ internal static ImmutableArray<TypeParameterSymbol> Take(int count)
             return builder.ToImmutableAndFree();
         }
 
+        internal static ImmutableArray<TypeWithAnnotations> Take(int count)
+        {
+            if (count > s_parameterPool.Length)
+            {
+                GrowPool(count);
+            }
+
+            var builder = ArrayBuilder<TypeWithAnnotations>.GetInstance();
+
+            for (int i = 0; i < count; i++)
+            {
+                builder.Add(TypeWithAnnotations.Create(GetTypeParameter(i), NullableAnnotation.Ignored));
+            }
+
+            return builder.ToImmutableAndFree();
+        }
+
         public override int Ordinal
         {
             get { return _index; }

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

@@ -944,7 +944,7 @@ void checkSingleOverriddenMember(Symbol overridingMember, Symbol overriddenMembe
 
                         if (overridingMethod.IsGenericMethod)
                         {
-                            overriddenMethod = overriddenMethod.Construct(overridingMethod.TypeArgumentsWithAnnotations);
+                            overriddenMethod = overriddenMethod.Construct(TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(overridingMethod.TypeParameters));
                         }
 
                         // Check for mismatched byref returns and return type. Ignore custom modifiers, because this diagnostic is based on the C# semantics.

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

@@ -632,7 +632,7 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S
         {
             Debug.Assert(!ReferenceEquals(definition, implementation));
 
-            MethodSymbol constructedDefinition = definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations);
+            MethodSymbol constructedDefinition = definition.ConstructIfGeneric(TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(implementation.TypeParameters));
             bool returnTypesEqual = constructedDefinition.ReturnTypeWithAnnotations.Equals(implementation.ReturnTypeWithAnnotations, TypeCompareKind.AllIgnoreOptions);
             if (!returnTypesEqual
                 && !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(implementation.ReturnType)

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

@@ -20,11 +20,16 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
     /// </summary>
     internal sealed class TypeMap : AbstractTypeParameterMap
     {
-        public static readonly System.Func<TypeWithAnnotations, TypeSymbol> AsTypeSymbol = t => t.Type;
+        public static readonly Func<TypeWithAnnotations, TypeSymbol> AsTypeSymbol = t => t.Type;
 
         internal static ImmutableArray<TypeWithAnnotations> TypeParametersAsTypeSymbolsWithAnnotations(ImmutableArray<TypeParameterSymbol> typeParameters)
         {
-            return typeParameters.SelectAsArray((tp) => TypeWithAnnotations.Create(tp));
+            return typeParameters.SelectAsArray(static (tp) => TypeWithAnnotations.Create(tp));
+        }
+
+        internal static ImmutableArray<TypeWithAnnotations> TypeParametersAsTypeSymbolsWithIgnoredAnnotations(ImmutableArray<TypeParameterSymbol> typeParameters)
+        {
+            return typeParameters.SelectAsArray(static (tp) => TypeWithAnnotations.Create(tp, NullableAnnotation.Ignored));
         }
 
         internal static ImmutableArray<TypeSymbol> AsTypeSymbols(ImmutableArray<TypeWithAnnotations> typesOpt)

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

@@ -1690,10 +1690,12 @@ internal static void CheckNullableReferenceTypeMismatchOnImplementingMember(Type
                         {
                             if (arg.isExplicit)
                             {
+                                // We use ConstructedFrom symbols here and below to not leak methods with Ignored annotations in type arguments
+                                // into diagnostics
                                 diagnostics.Add(topLevel ?
                                                     ErrorCode.WRN_TopLevelNullabilityMismatchInReturnTypeOnExplicitImplementation :
                                                     ErrorCode.WRN_NullabilityMismatchInReturnTypeOnExplicitImplementation,
-                                                implementingMethod.Locations[0], new FormattedSymbol(implementedMethod, SymbolDisplayFormat.MinimallyQualifiedFormat));
+                                                implementingMethod.Locations[0], new FormattedSymbol(implementedMethod.ConstructedFrom, SymbolDisplayFormat.MinimallyQualifiedFormat));
                             }
                             else
                             {
@@ -1702,7 +1704,7 @@ internal static void CheckNullableReferenceTypeMismatchOnImplementingMember(Type
                                                     ErrorCode.WRN_NullabilityMismatchInReturnTypeOnImplicitImplementation,
                                                 GetImplicitImplementationDiagnosticLocation(implementedMethod, arg.implementingType, implementingMethod),
                                                 new FormattedSymbol(implementingMethod, SymbolDisplayFormat.MinimallyQualifiedFormat),
-                                                new FormattedSymbol(implementedMethod, SymbolDisplayFormat.MinimallyQualifiedFormat));
+                                                new FormattedSymbol(implementedMethod.ConstructedFrom, SymbolDisplayFormat.MinimallyQualifiedFormat));
                             }
                         };
 
@@ -1716,7 +1718,7 @@ internal static void CheckNullableReferenceTypeMismatchOnImplementingMember(Type
                                                     ErrorCode.WRN_NullabilityMismatchInParameterTypeOnExplicitImplementation,
                                                 implementingMethod.Locations[0],
                                                 new FormattedSymbol(implementingParameter, SymbolDisplayFormat.ShortFormat),
-                                                new FormattedSymbol(implementedMethod, SymbolDisplayFormat.MinimallyQualifiedFormat));
+                                                new FormattedSymbol(implementedMethod.ConstructedFrom, SymbolDisplayFormat.MinimallyQualifiedFormat));
                             }
                             else
                             {
@@ -1726,7 +1728,7 @@ internal static void CheckNullableReferenceTypeMismatchOnImplementingMember(Type
                                                 GetImplicitImplementationDiagnosticLocation(implementedMethod, arg.implementingType, implementingMethod),
                                                 new FormattedSymbol(implementingParameter, SymbolDisplayFormat.ShortFormat),
                                                 new FormattedSymbol(implementingMethod, SymbolDisplayFormat.MinimallyQualifiedFormat),
-                                                new FormattedSymbol(implementedMethod, SymbolDisplayFormat.MinimallyQualifiedFormat));
+                                                new FormattedSymbol(implementedMethod.ConstructedFrom, SymbolDisplayFormat.MinimallyQualifiedFormat));
                             }
                         };
 
@@ -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(

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

@@ -86,6 +86,7 @@ internal static TypeWithAnnotations Create(TypeSymbol typeSymbol, NullableAnnota
                 return default;
             }
 
+            Debug.Assert(nullableAnnotation != NullableAnnotation.Ignored || typeSymbol.IsTypeParameter());
             switch (nullableAnnotation)
             {
                 case NullableAnnotation.Oblivious:
@@ -161,11 +162,6 @@ internal bool CanBeAssignedNull
             }
         }
 
-        private static bool IsIndexedTypeParameter(TypeSymbol typeSymbol)
-        {
-            return typeSymbol is IndexedTypeParameterSymbol;
-        }
-
         private static TypeWithAnnotations CreateNonLazyType(TypeSymbol typeSymbol, NullableAnnotation nullableAnnotation, ImmutableArray<CustomModifier> customModifiers)
         {
             return new TypeWithAnnotations(typeSymbol, nullableAnnotation, Extensions.Create(customModifiers));
@@ -429,6 +425,9 @@ public TypeWithAnnotations SubstituteType(AbstractTypeMap typeMap) =>
 
         internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
         {
+            // Ignored may only appear on a replacement type and will not survive the substitution (ie. the original annotation wins over Ignored)
+            Debug.Assert(NullableAnnotation != NullableAnnotation.Ignored);
+
             var newCustomModifiers = typeMap.SubstituteCustomModifiers(this.CustomModifiers);
             TypeSymbol typeSymbol = this.Type;
             var newTypeWithModifiers = typeMap.SubstituteType(typeSymbol);
@@ -437,6 +436,7 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
             {
                 Debug.Assert(newTypeWithModifiers.NullableAnnotation.IsOblivious() || (typeSymbol.IsNullableType() && newTypeWithModifiers.NullableAnnotation.IsAnnotated()));
                 Debug.Assert(newTypeWithModifiers.CustomModifiers.IsEmpty);
+                Debug.Assert(NullableAnnotation != NullableAnnotation.Ignored);
 
                 if (typeSymbol.Equals(newTypeWithModifiers.Type, TypeCompareKind.ConsiderEverything) &&
                     newCustomModifiers == CustomModifiers)
@@ -457,7 +457,7 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
             {
                 return this; // substitution had no effect on the type or modifiers
             }
-            else if (Is((TypeParameterSymbol)typeSymbol))
+            else if (Is((TypeParameterSymbol)typeSymbol) && newTypeWithModifiers.NullableAnnotation != NullableAnnotation.Ignored)
             {
                 return newTypeWithModifiers;
             }
@@ -468,14 +468,13 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
             }
 
             NullableAnnotation newAnnotation;
-
-            Debug.Assert(!IsIndexedTypeParameter(newTypeWithModifiers.Type) || newTypeWithModifiers.NullableAnnotation.IsOblivious());
+            Debug.Assert(newTypeWithModifiers.Type is not IndexedTypeParameterSymbol || newTypeWithModifiers.NullableAnnotation == NullableAnnotation.Ignored);
 
             if (NullableAnnotation.IsAnnotated() || newTypeWithModifiers.NullableAnnotation.IsAnnotated())
             {
                 newAnnotation = NullableAnnotation.Annotated;
             }
-            else if (IsIndexedTypeParameter(newTypeWithModifiers.Type))
+            else if (newTypeWithModifiers.NullableAnnotation == NullableAnnotation.Ignored)
             {
                 newAnnotation = NullableAnnotation;
             }