dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.05k stars 4.04k forks source link

When inheriting constraints from enable context to disable context non-null types should be changed to oblivious #29979

Closed AlekseyTs closed 5 years ago

AlekseyTs commented 6 years ago
        [Fact]
        public void Constraints_21()
        {
            var source1 =
@"
public class A<T>
{
    public virtual void F1<T1>() where T1 : class
    {
    }

    public virtual void F2<T2>() where T2 : class?
    {
    }
}
";
            var comp1 = CreateCompilation(new[] { source1, NonNullTypesTrue, NonNullTypesAttributesDefinition });

            var source2 =
@"
class B : A<int>
{
    public override void F1<T11>()
    {
    }

    public override void F2<T22>()
    {
    }
}
";

            var comp2 = CreateCompilation(new[] { source2 }, references: new[] { comp1.EmitToImageReference() });

            comp2.VerifyDiagnostics();

            CompileAndVerify(comp2, sourceSymbolValidator: symbolValidator, symbolValidator: symbolValidator);
            void symbolValidator(ModuleSymbol m)
            {
                bool isSource = !(m is PEModuleSymbol);

                var bf1 = (MethodSymbol)m.GlobalNamespace.GetMember("B.F1");
                Assert.Equal("void B.F1<T11>() where T11 : class", bf1.ToDisplayString(SymbolDisplayFormat.TestFormatWithConstraints));
                TypeParameterSymbol t11 = bf1.TypeParameters[0];

                // PROTOTYPE(NullableReferenceTypes): It is probably wrong to have this difference between source symbol
                //                                    and emitted-then-imported symbol. What are the rules for inheriting constraints
                //                                    across different non-null contexts?
                if (isSource)
                {
                    Assert.False(t11.ReferenceTypeConstraintIsNullable);
                }
                else
                {
                    Assert.Null(t11.ReferenceTypeConstraintIsNullable);
                }

                Assert.Empty(t11.GetAttributes());

                var bf2 = (MethodSymbol)m.GlobalNamespace.GetMember("B.F2");
                Assert.Equal("void B.F2<T22>() where T22 : class?", bf2.ToDisplayString(SymbolDisplayFormat.TestFormatWithConstraints));

                TypeParameterSymbol t22 = bf2.TypeParameters[0];
                Assert.True(t22.ReferenceTypeConstraintIsNullable);
                if (isSource)
                {
                    Assert.Empty(t22.GetAttributes());
                }
                else
                {
                    Assert.Equal("System.Runtime.CompilerServices.NullableAttribute", t22.GetAttributes().Single().ToString());
                }
            }
        }
AlekseyTs commented 6 years ago

Also see Constraints_27 unit-test

jaredpar commented 5 years ago

This test behavior has already been updated. Both Constraints_21 and Constraints_27 were updated as a part of 19b50a45ec09d2ac4253d7375f34a03362d4c2a5

jaredpar commented 5 years ago

Chatted with @AlekseyTs. The specific code was indeed changed but it didn't end up testing the intended behavior here. A test which accurately verifies that constraints are flipped to oblivious in this case can be tested as follows:

                Assert.Null(bf1.TypeParameters[0].ReferenceTypeConstraintIsNullable);

That test indeed still fails.

AlekseyTs commented 5 years ago

Here is a scenario that reflects the current behavior:

        [Fact]
        public void ConstraintsChecks_32()
        {
            var source =
@"
#nullable enable
public class A
{
    public virtual void F2<T2>(T2 y) where T2 : class?
    {
    }

    public void M3<T3>(T3 z) where T3 : class { }
}

class B : A
{
#nullable disable
    public override void F2<T22>(T22 y)
    {
#nullable enable
        M3<T22>(y); // 3
        M3(y); // 4
    }

#nullable disable
    void F22<T22>(T22 y) where T22 : class
    {
#nullable enable
        M3<T22>(y); // 5
        M3(y); // 6
    }
}
";
            var comp1 = CreateCompilation(source);

            comp1.VerifyDiagnostics(
                // (18,9): warning CS8634: The type 'T22' cannot be used as type parameter 'T3' in the generic type or method 'A.M3<T3>(T3)'. Nullability of type argument 'T22' doesn't match 'class' constraint.
                //         M3<T22>(y); // 3
                Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterReferenceTypeConstraint, "M3<T22>").WithArguments("A.M3<T3>(T3)", "T3", "T22").WithLocation(18, 9),
                // (19,9): warning CS8634: The type 'T22' cannot be used as type parameter 'T3' in the generic type or method 'A.M3<T3>(T3)'. Nullability of type argument 'T22' doesn't match 'class' constraint.
                //         M3(y); // 4
                Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterReferenceTypeConstraint, "M3").WithArguments("A.M3<T3>(T3)", "T3", "T22").WithLocation(19, 9)
                );
        }

Note, that we get two warnings inside overriding method and none in F22. The difference is caused by the fact that the overriding method is inheriting class? constraint, where as F22 gets class~ constraint. Both methods are declared in nullable disable context.

AlekseyTs commented 5 years ago

On Apr. 15, 2019, C# LDM decided to keep the current behavior: "The constraint is inherited from the definition."

gafter commented 5 years ago

See https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-04-15.md#inheriting-constraints-in-nullable-disabled-code