Missing warnings on overriding a method with different nullability attributes

[jcouv]

Relates to PR #41336
My attempts to fixing this resulted in a difficult cycle to fix, so the problem was separated into an issue.
Should take a look at this again after we investigate a solution for constraint binding cycles (#48154).

        [Fact]
        public void MaybeNullWhenTrue_OHI_Parameter_Generic_OnOverrides()
        {
            var source =
@"using System.Diagnostics.CodeAnalysis;
public class Base
{
    public virtual bool F1<T>(T  t1, out T  t2, ref T  t3, in T  t4)                  => throw null!;
}
public class Derived : Base
{
    public override bool F1<T>([MaybeNullWhen(true)]T  t1, [MaybeNullWhen(true)] out T  t2, [MaybeNullWhen(true)] ref T  t3, [MaybeNullWhen(true)] in T  t4)                  => throw null!; // t2, t3
}
";
            var comp = CreateNullableCompilation(new[] { MaybeNullWhenAttributeDefinition, source });
            // Note: we're missing warnings on F1 because the overridden method with substituted T from Derived has an oblivious T 
            comp.VerifyDiagnostics(
                // TODO we should have warnings on t2 and t3
                );
        }

The proposed fix was:

diff --git a/src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs b/src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs
index 76acfc3..d268af3 100644
--- a/src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs
+++ b/src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs
@@ -486,7 +486,7 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
             }
             else if (NullableAnnotation != NullableAnnotation.Oblivious)
             {
-                if (!typeSymbol.IsTypeParameterDisallowingAnnotation())
+                if (!typeSymbol.IsTypeParameterDisallowingAnnotation() || newTypeWithModifiers.Type.IsTypeParameterDisallowingAnnotation())
                 {
                     newAnnotation = NullableAnnotation;
                 }

But that caused a cycle with

        [Fact]
        public void CircularConstraints()
        {
            var source =
@"class A<T> where T : B<T>.I
{
    internal interface I { }
}
class B<T> : A<T> where T : A<T>.I
{
}";
            var comp = CreateCompilation(new[] { source }, parseOptions: TestOptions.Regular7);
            comp.VerifyDiagnostics();

            comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue(), parseOptions: TestOptions.Regular7, skipUsesIsNullable: true);
            comp.VerifyDiagnostics(
                // error CS8630: Invalid 'NullableContextOptions' value: 'Enable' for C# 7.0. Please use language version '8.0' or greater.
                Diagnostic(ErrorCode.ERR_NullableOptionNotAvailable).WithArguments("NullableContextOptions", "Enable", "7.0", "8.0").WithLocation(1, 1)
                );

            comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue());
            comp.VerifyDiagnostics();
        }

with the following stack trace:

>	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeParameterSymbol.IsValueType.get() Line 566	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbolExtensions.IsTypeParameterDisallowingAnnotation(Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol type) Line 63	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeWithAnnotations.SubstituteTypeCore(Microsoft.CodeAnalysis.CSharp.Symbols.AbstractTypeMap typeMap) Line 487	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeWithAnnotations.NonLazyType.SubstituteType(Microsoft.CodeAnalysis.CSharp.Symbols.TypeWithAnnotations type, Microsoft.CodeAnalysis.CSharp.Symbols.AbstractTypeMap typeMap) Line 888	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeWithAnnotations.SubstituteType(Microsoft.CodeAnalysis.CSharp.Symbols.AbstractTypeMap typeMap) Line 436	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.AbstractTypeMap.SubstituteNamedType(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol previous) Line 64	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.SubstitutedNamedTypeSymbol.GetDeclaredBaseType(Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved) Line 142	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbolExtensions.GetNextDeclaredBase(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol type, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.CSharpCompilation compilation, ref Microsoft.CodeAnalysis.PooledObjects.PooledHashSet<Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol> visited) Line 191	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbolExtensions.GetNextBaseTypeNoUseSiteDiagnostics(Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol type, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.CSharpCompilation compilation, ref Microsoft.CodeAnalysis.PooledObjects.PooledHashSet<Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol> visited) Line 165	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.LookupMembersInClass(Microsoft.CodeAnalysis.CSharp.LookupResult result, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol type, string name, int arity, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.LookupOptions options, Microsoft.CodeAnalysis.CSharp.Binder originalBinder, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol accessThroughType, bool diagnose, ref System.Collections.Generic.HashSet<Microsoft.CodeAnalysis.DiagnosticInfo> useSiteDiagnostics) Line 750	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.LookupMembersInClass(Microsoft.CodeAnalysis.CSharp.LookupResult result, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol type, string name, int arity, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.LookupOptions options, Microsoft.CodeAnalysis.CSharp.Binder originalBinder, bool diagnose, ref System.Collections.Generic.HashSet<Microsoft.CodeAnalysis.DiagnosticInfo> useSiteDiagnostics) Line 687	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.LookupMembersInType(Microsoft.CodeAnalysis.CSharp.LookupResult result, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol type, string name, int arity, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.LookupOptions options, Microsoft.CodeAnalysis.CSharp.Binder originalBinder, bool diagnose, ref System.Collections.Generic.HashSet<Microsoft.CodeAnalysis.DiagnosticInfo> useSiteDiagnostics) Line 187	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.LookupMembersInternal(Microsoft.CodeAnalysis.CSharp.LookupResult result, Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceOrTypeSymbol nsOrType, string name, int arity, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.LookupOptions options, Microsoft.CodeAnalysis.CSharp.Binder originalBinder, bool diagnose, ref System.Collections.Generic.HashSet<Microsoft.CodeAnalysis.DiagnosticInfo> useSiteDiagnostics) Line 164	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.LookupSymbolsOrMembersInternal(Microsoft.CodeAnalysis.CSharp.LookupResult result, Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceOrTypeSymbol qualifierOpt, string name, int arity, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.LookupOptions options, bool diagnose, ref System.Collections.Generic.HashSet<Microsoft.CodeAnalysis.DiagnosticInfo> useSiteDiagnostics) Line 130	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.LookupSymbolsSimpleName(Microsoft.CodeAnalysis.CSharp.LookupResult result, Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceOrTypeSymbol qualifierOpt, string plainName, int arity, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, Microsoft.CodeAnalysis.CSharp.LookupOptions options, bool diagnose, ref System.Collections.Generic.HashSet<Microsoft.CodeAnalysis.DiagnosticInfo> useSiteDiagnostics) Line 38	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindNonGenericSimpleNamespaceOrTypeOrAliasSymbol(Microsoft.CodeAnalysis.CSharp.Syntax.IdentifierNameSyntax node, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, bool suppressUseSiteDiagnostics, Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceOrTypeSymbol qualifierOpt) Line 805	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindSimpleNamespaceOrTypeOrAliasSymbol(Microsoft.CodeAnalysis.CSharp.Syntax.SimpleNameSyntax syntax, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, bool suppressUseSiteDiagnostics, Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceOrTypeSymbol qualifierOpt) Line 744	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindQualifiedName(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax leftName, Microsoft.CodeAnalysis.CSharp.Syntax.SimpleNameSyntax rightName, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, bool suppressUseSiteDiagnostics) Line 1306	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindNamespaceOrTypeOrAliasSymbol(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax syntax, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved, bool suppressUseSiteDiagnostics) Line 417	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindTypeOrAlias(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax syntax, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol> basesBeingResolved) Line 312	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindTypeOrAliasOrConstraintKeyword(Microsoft.CodeAnalysis.CSharp.Syntax.TypeSyntax syntax, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, out Microsoft.CodeAnalysis.CSharp.Binder.ConstraintContextualKeyword keyword) Line 172	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindTypeOrConstraintKeyword(Microsoft.CodeAnalysis.CSharp.Syntax.TypeSyntax syntax, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, out Microsoft.CodeAnalysis.CSharp.Binder.ConstraintContextualKeyword keyword) Line 54	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindTypeParameterConstraints(Microsoft.CodeAnalysis.CSharp.Syntax.TypeParameterSyntax typeParameterSyntax, Microsoft.CodeAnalysis.CSharp.Syntax.TypeParameterConstraintClauseSyntax constraintClauseSyntax, bool isForOverride, Microsoft.CodeAnalysis.DiagnosticBag diagnostics) Line 225	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Binder.BindTypeParameterConstraintClauses(Microsoft.CodeAnalysis.CSharp.Symbol containingSymbol, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.CSharp.Symbols.TypeParameterSymbol> typeParameters, Microsoft.CodeAnalysis.CSharp.Syntax.TypeParameterListSyntax typeParameterList, Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.TypeParameterConstraintClauseSyntax> clauses, ref System.Collections.Generic.IReadOnlyDictionary<Microsoft.CodeAnalysis.CSharp.Symbols.TypeParameterSymbol, bool> isValueTypeOverride, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, bool isForOverride) Line 65	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.SourceNamedTypeSymbol.MakeTypeParameterConstraints(Microsoft.CodeAnalysis.DiagnosticBag diagnostics) Line 308	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.SourceNamedTypeSymbol.GetTypeParameterConstraintClause(int ordinal) Line 246	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.SourceTypeParameterSymbol.GetTypeParameterConstraintClause() Line 554	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.SourceTypeParameterSymbol.GetDeclaredConstraints() Line 571	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.SourceTypeParameterSymbol.HasValueTypeConstraint.get() Line 494	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Symbols.TypeParameterSymbol.IsValueType.get() Line 566	C#

---

I think a more common motivating scenario is:

        [Fact]
        public void Implement_GenericMethodWithNullabilityDifference()
        {
            var source =
@"#nullable enable

public interface I
{
    T M<T>();
}
public class C : I
{
    public T? M<T>() => default;
}";
            var comp = CreateCompilation(source);
            comp.VerifyEmitDiagnostics(
// We fail to warn
                );
        }

[cston]

@jcouv, does that cycle still occur now that #47968 has been merged?

[jcouv]

That cycle would occur if we applied the desired fix (see above diff) using the above test.
I didn't make the change described in the diff because I didn't have solution for cycle (would require separating figuring out if reference type without doing full binding).

[jcouv]

@cston Yes, the cycle still occurs after #47968 was merged.

[jcouv]

From discussion with @cston the proposed design would be an extension of #47968 (we can leverage the flags he put in place). To compute IsValueType we need to do "less work". We think we can reduce the work further than today by not binding type arguments in type constraints.

---

From discussion with Aleksey, we think we can fix this specific issue by adjusting the type arguments made from type parameters:

        private void CheckOverrideMember(
            Symbol overridingMember,
            OverriddenOrHiddenMembersResult overriddenOrHiddenMembers,
            DiagnosticBag diagnostics,
            out bool suppressAccessors)
        {
...
                        if (overridingMethod.IsGenericMethod)
                        {
                            overriddenMethod = overriddenMethod.Construct(overridingMethod.TypeArgumentsWithAnnotations); // TODO2
                        }

                        // Check for mismatched byref returns and return type. Ignore custom modifiers, because this diagnostic is based on the C# semantics.
                        if (overridingMethod.RefKind != overriddenMethod.RefKind)
                        {
                            diagnostics.Add(ErrorCode.ERR_CantChangeRefReturnOnOverride, overridingMemberLocation, overridingMember, overriddenMember);
                        }
                        else if (!IsValidOverrideReturnType(overridingMethod, overridingMethod.ReturnTypeWithAnnotations, overriddenMethod.ReturnTypeWithAnnotations, diagnostics))
                        {

and

        internal static void CheckNullableReferenceTypeMismatchOnImplementingMember(TypeSymbol implementingType, Symbol implementingMember, Symbol interfaceMember, bool isExplicit, DiagnosticBag diagnostics)
...
                            if (implementedMethod.IsGenericMethod)
                            {
                                implementedMethod = implementedMethod.Construct(implementingMethod.TypeArgumentsWithAnnotations); // TODO2
                            }

                            SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride(
                                compilation,
                                implementedMethod,
                                implementingMethod,
                                diagnostics,
                                reportMismatchInReturnType,
                                reportMismatchInParameterType,
                                (implementingType, isExplicit));