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

Records: Incorrect nullable annotations for generated Equals override #47627

Open Youssef1313 opened 4 years ago

Youssef1313 commented 4 years ago

Version Used:

master

Steps to Reproduce:

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+BiAdgVwBtCJhC4ACOXU8gWAChGABAZgoAcIEYBLCQhQRwAxgHsEKCgGFGAb0YBfIA==

Expected Behavior:

public override bool Equals(object? obj)
{
    return Equals(obj as C);
}

Actual Behavior:

public override bool Equals(object obj)
{
    return Equals(obj as C);
}

[jcouv update:] more broadly we should emit nullability expectations for bool Equals(R? other) and Type EqualityContract { get; } and R? <Clone>$() and void Deconstruct(out string? NotNullableStringProperty). And we should warn on bad user-provided methods. Relates to https://github.com/dotnet/roslyn/issues/44763 (Record constructor arguments should propagate nullability to properties)

RikkiGibson commented 4 years ago

Is it possible that this is inconsequential in practice due to these methods being synthesized overrides? For example when I bring up the completion list for Equals on a record, I see:

image

Youssef1313 commented 4 years ago

I hadn't looked at the completion list when I opened this. But it seems okay as long as they show as nullable. The only concern is what happens if reflection is used to retrieve the parameter nullability?

If it will work correctly then I can't think of a scenario which will have problems.

jcouv commented 3 years ago

@RikkiGibson I assigned this to you since in same broad area (nullability analysis of records) as another issue already on your plate (https://github.com/dotnet/roslyn/issues/44763). I hope that's okay. Also added some details to OP.