Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roslyn fails to correctly infer nullability of foreach variables' types if deconstruction is used #56352

Open
TessenR opened this issue Sep 13, 2021 · 2 comments
Labels
Area-Compilers Bug Concept-Null Annotations The issue involves annotating an API for nullable reference types
Milestone

Comments

@TessenR
Copy link

TessenR commented Sep 13, 2021

Version Used:

Branch main (4 Sep 2021)
Latest commit 9525fa5 by Tomáš Matoušek:
Options refactoring (#55686)

Steps to Reproduce:

Compile and run the following code

#nullable enable
class C<T>
{
  public T field;
  
  public C(T t) => field = t;
}

class Program
{
  public static void Main()
  {
    var x = M1("");
    x.field.ToString();
  }
    
  static C<string> M1(string? s)
  {
    var c = Infer(s);
    foreach (var (x, _) in new[] { (c, c) })
    {
      x.ToString();
      x.field.ToString();
      
      var z = x;
      z.field.ToString();
      z.field = null;
      return z;
    }

    return new("");
  }
  
  public static C<T> Infer<T>(T t) => new(t);
}

Expected Behavior:
Possible null dereference warning for x.field.ToString() in M1
Nullability of reference types in value of type 'C<string?>' doesn't match target type 'C<string>' warning for return z in M1

Actual Behavior:
No warnings at all in the program above.
It crashes with a NullReferenceException if you run it due to the missing warning for x.field.ToString();
If you comment the x.field.ToString() line in M1 it crashes at x.field.ToString() in Main due to the missing warning on return z

Notes:
It looks like Roslyn fails to infer any nullability of type arguments at all for all variables in M1 so it allows both dereferencing .field and assigning it with null values without any warnings as if it thinks their type arguments have unknown nullability.

Interestingly, you'll correctly get both warnings if you change the cycle to not use deconstruction i.e. change the foreach cycle header to the following:

    foreach (var x in new[] { c })
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 13, 2021
@RikkiGibson
Copy link
Contributor

Is it possible that this issue is fixed by #52722?

@jaredpar jaredpar added Bug Concept-Null Annotations The issue involves annotating an API for nullable reference types and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 13, 2021
@jaredpar jaredpar added this to the Compiler.Next milestone Sep 13, 2021
@TessenR
Copy link
Author

TessenR commented Sep 13, 2021

@RikkiGibson It looks quite similar and it might indeed also solve this issue. However, it looks like all tests and fixed issues in #52722 mention only top-level nullability whereas this issue has a problem with nullability of type arguments of foreach variables. I'd suggest adding tests to verify whether the changes in #52722 are enough to fix this as well.

@jaredpar jaredpar modified the milestones: Compiler.Next, Backlog Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Concept-Null Annotations The issue involves annotating an API for nullable reference types
Projects
Nullable Board
Awaiting triage
Development

No branches or pull requests

3 participants