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.02k stars 4.03k forks source link

Type of `this` should probably depend on nullability context #49722

Open jcouv opened 3 years ago

jcouv commented 3 years ago

It's strange to end up with an oblivious in purely nullable-enabled code.

        [Fact]
        public void IgnoredNullability_ImplicitlyTypedArrayWithThis_DifferentNullability()
        {
            var src = @"
#nullable enable

internal class C<T>
{
    public void M(bool b)
    {
        _ = b ? this : new C<T?>();
    }
}
";
            var comp = CreateCompilation(src);
            // missing warning
            comp.VerifyDiagnostics();
        }
RikkiGibson commented 3 years ago

Does the type of this have oblivious type arguments here?

jcouv commented 3 years ago

@RikkiGibson Right. Currently this does use an oblivious type argument (ie. it's using the type definition). No warning is produced because we're taking a best type between C<T~> and C<T?>.

AlekseyTs commented 3 years ago

Type of this should probably depend on nullability context

This doesn't sound right to me. Or perhaps the proposal should provide more details.

jcouv commented 3 years ago

A proposal would be for this to have the same type as new C<T>() at that position in the source, in this example. I don't think it's worth spending much time to discuss or explore because it's such a minor issue (I put it in Backlog milestone and don't have intention to explore).

AlekseyTs commented 3 years ago

A proposal would be for this to have the same type as new C<T>() at that position in the source

At the position of this keyword? That would mean that the type of this is different throughout the method.

RikkiGibson commented 3 years ago

Perhaps it would make sense to use the nullability context from the point that the type parameter is declared?

TessenR commented 3 years ago

Shouldn't this issue be tagged as a bug? Because it leads to missing nullability warnings in code that actually crashes with a NullReferenceException

e.g. compile and run the following code

#nullable enable

var c = new C<string>("");
c.M(true);
c.field.ToString();

internal class C<T>
{
    public T field;
    public C(T t) => field = t;

    public void M(bool b)
    {
        var x = b ? this : new C<T?>(default);
        x.field = default;
    }
}

No warnings at all. The code crashes with a NullReferenceException