Roslyn: Roslyn incorrectly updates variables' states to not nullable upon passing them to [AllowNull] parameters

[TessenR]

Version Used:

Branch master (14 Oct 2020)
Latest commit 3fca2e8 by msftbot[bot]:
Merge pull request #48555 from CyrusNajmabadi/splitFollowingComment

Do not start a new comment when hitting enter before an existing comment

Steps to Reproduce:

Compile and run the following code:

#nullable enable
using System.Diagnostics.CodeAnalysis;

string? s = null;
AcceptNullable(s);
s.ToString();

void AcceptNullable([AllowNull] string s) { }

Expected Behavior:
Warning for possible null dereference in s.ToString();

Actual Behavior:
No warnings. The program crashes at runtime with a NullReferenceException

[RikkiGibson]

This is a scenario we discussed at some length internally a few months ago. At the time we concluded that the flow state of the argument for such a parameter should be updated to not-null after the call, as part of our change to reduce redundant warnings for maybe-null arguments to not-null parameters (#43383).

(I feel the need to note that I didn't agree with this decision, but that my point of view did not win the day 😉)

Tagging @333fred.

[TessenR]

At the time we concluded that the flow state of the argument for such a parameter should be updated to not-null after the call,

TBH I fail to see why this would be considered logical behavior. I wonder if you remember any arguments for it

(I feel the need to note that I didn't agree with this decision, but that my point of view did not win the day 😉)

❤️

[333fred]

The arguments for this are that we made a change to update the flow state of a parameter to the declared state of a parameter after such a method is called. Generally speaking, we didn't consider this case to be realistic code, and weren't too concerned with the simpler rule (always update the state) being implemented. Can you share a real scenario that is actually hurt by this?

[Youssef1313]

While I can't come up with a real scenario here, I agree with @TessenR and @RikkiGibson that this should produce a warning.

[Youssef1313]

For a real scenario, imagine that I called WriteJson from Newtonsoft.Json library:

https://github.com/JamesNK/Newtonsoft.Json/blob/cdf10151d507d497a3f9a71d36d544b199f73435/Src/Newtonsoft.Json/JsonConverter.cs#L108

The parameter value is marked with [AllowNull]. After the call, if someone attempts to dereference the passed value, they will get no warnings.

[TessenR]

@333fred

weren't too concerned with the simpler rule (always update the state)

I'm not sure why "update the state to not nullable if parameter only allows not nullable values" isn't simple enough.

Can you share a real scenario that is actually hurt by this?

The original design of nullable reference types all but ensured that there's a ton of API out there using these attributes. One of the examples is linked above. The reasons for this are:

• you had to use [AllowNull] to annotate generics
• you had to preserve such attributes on implementation/overrides even if the requirement is already satisfied by the type system (in one of the compiler versions along the way, nullable reference types are updating fast so I can't remember the exact one)
• even the current compiler doesn't have any problems with overriding/implementing [AllowNull] T e.g. in IEqualityComparer.Equals with [AllowNull] NotNullable parameter, so it feels like a natural way of implementing such an interface

This leads to a situation where e.g. there's an implementation of the following interface from netcore3.1:

  public interface IEqualityComparer<in T>
  {
    bool Equals([AllowNull] T x, [AllowNull] T y);

    int GetHashCode([DisallowNull] T obj);
  }

With the following type:

class StringPalindromeComparer : IEqualityComparer<string>
{
  public bool Equals([AllowNull] string x, [AllowNull] string y) => StringUtils.IsPalindrome(x, y);

  public int GetHashCode([DisallowNull] string obj) => obj.Length;
}

And I'll miss warnings in the following scenario which will crash with an NRE

var comparer = new StringPalindromeComparer();
var s1 = GetMeAString(); // can it return null? well I hope the compiler will warn me if it is
var s2 = GetMeAString();
if (comparer.Equals(s1, s2))
{
  Console.WriteLine("Palindrome length: " + s1.Length); // no warnings, oops, runtime crash with a 'NullReferenceException'
  Console.WriteLine("Palindrome strings: {0}, {1}", s1, s2);
}

string? GetMeAString() => null;

Note that

• there's nothing wrong with the interface implementation in StringPalindromeComparer and it has no warnings regardless of the compiler's version
• at some point I _had_ to use the attributes instead of using the nullable types in arguments so I wrote the interface and it now just exists and causes missing warnings

So I'd consider this scenario a very real-world one.

While this all can be solved by tremendous effort of software engineers across the world e.g.

• find and review all existing usages of [AllowNull] attiributes in your code and all used third-party libraries
• fix all your usages to use nullable annotations instead (migrate to C#9 if needed for generics)
• ask all third party dependencies' maintainers to fix their API for you as well....

etc

I think the better solution would be to just support this attribute in the compiler especially given that this looks like the most obvious thing ever - the parameter allows null values, why would the compiler think it guarantees the argument won't be null after the call?

[TessenR]

flow state of a parameter to the declared state of a parameter after such a method is called

I think I've got a bit lost in the terminology here. What exactly do you mean by the declared state of a parameter?

Because it can't be neither parameter type nullability as the compiler does update the argument's state to not nullable upon passing it to [DisallowNull] string? s parameter. Nor the initial state of the parameter in the method since using [AllowNull] string s will warn on using it within a method so its state is nullable.

What I find especially confusing is that the compiler already understands [DisallowNull] and updates the argument' state in the following code:

#nullable enable
using System.Diagnostics.CodeAnalysis;

string? s = null;
L(s);
L(s); // no warnings
void L([DisallowNull] string? s) => s.ToString();

But for some reason just ignores its [AllowNull] counterpart. This seems completely illogical

[RikkiGibson]

Not yet certain how we are resolving this, but I am self-assigning in order to keep it on my radar.

[jaredpar]

Trying to get this on the LDM agenda for this / next week so we can get a resolution.

[RikkiGibson]

It looks like we will probably change analysis according to https://github.com/dotnet/csharplang/discussions/4102#discussioncomment-128346