New nullability warnings regarding lambda conversions are added in perfectly valid code upon updating to the latest compiler

[TessenR]

Version Used:

Branch main (26 Oct 2021)
Latest commit a4ec465 by Gen Lu:
Merge pull request #57338 from genlu/FixWarning

Fix 'AssemblyVersion overridden by auto-generated version' warning in build

Steps to Reproduce:

Compile the following code:

#nullable enable
using System.Diagnostics.CodeAnalysis;

delegate void D1<T>([NotNull] ref T? s);
[return: NotNull] delegate T D2<T>();

class C<T> where T : new()
{
  void Method()
  {
    D1<T> d1 = (ref T? s) => s = new T();
    D2<T> d2 = () => new T();
  }
}

Expected Behavior: No warnings. The code doesn't have any warnings in C# 9 in the latest release version of VS. The code doesn't violate any nullability contracts set by the delegate itself as lambda expressions return non-nulalble values as required by the delegate attributes. Updating the compiler to the latest version shouldn't add new warnings since there are no actual problems in the code to begin with.

Actual Behavior: Nullability mismatch warnings are reported for both lambda expression in the program above:

    warning CS8622: Nullability of reference types in type of parameter 's' of 'lambda expression' doesn't match the target delegate 'D1<T>' (possibly because of nullability attributes).
    warning CS8621: Nullability of reference types in return type of 'lambda expression' doesn't match the target delegate 'D2<T>' (possibly because of nullability attributes).

Notes: It looks like Roslyn didn't verify nullability declared by delegate attributes at all and now it does, so sometimes the new warnings might be correct where there was no warning at all previously.

Therefore, I'm not sure whether it's a bug or intended behavior but please consider the following:

• the attributes required to fix this warning without suppression simply aren't allowed in C# 9
• code written in C# 9 will have to deal with these new warnings even if it doesn't have any nullability problems upon updating a compiler version because the correct annotation simply wasn't possible in C# 9. E.g. an example above doesn't have any actual nullability issues
• in C# 9 you still cannot add the nullable attributes required by the warnings so if you cannot migrate to the latest C# as well you are required to either rewrite your code in a different way (e.g. replace lambda expression with method groups) or suppress the warnings

While it's useful to verify such contracts, perhaps it could be done by using target delegates' attributes instead of lambda expression attributes if there are none on the lambda expression? It would make code without nullability problems compile without warnings; it will not miss any nullability problems even if you don't annotate the lambda expression and even with C# 10 it will save some code duplication where you currently have to duplicate every attribute on the lambda expression to make it compatible with the target delegate which feels redundant.

[RikkiGibson]

Related to #56792. cc @jcouv.

[TessenR]

Actually, it seems that some form of support was there for attributes from target delegates and is now lost.

I've investigated the new warnings and came across this code sample. It didn't warn before and it shouldn't because the target delegate has the [MaybeNull] annotation but now the warning requires me to annotate it on the lambda expression as well (the code was written in C# 8 so the delegate couldn't utilize the TF? syntax)

Both warnings seem wrong

using System.Diagnostics.CodeAnalysis;

#nullable enable
class C
{
  [return: System.Diagnostics.CodeAnalysis.MaybeNull]
  public delegate TF MightReturnNull<out TF>();

  public static void Boo<T>(MightReturnNull<T> getValue)
  {
  }
  public void Test()
  {
    Boo<string>(() => null); // warning CS8603: Possible null reference return.
                             // warning CS8621: Nullability of reference types in return type of 'lambda expression' doesn't match the target delegate 'C.MightReturnNull<string>' (possibly because of nullability attributes).
  }
}

[RikkiGibson]

I think I agree with what you're saying. We need to re-evaluate how we choose a "signature" for the lambda being analyzed and rationalize the fact that nullability attributes can now appear on either the target delegate or the lambda itself.

[jcouv]

We'd had a conversation along these lines with Rikki at the time of https://github.com/dotnet/roslyn/pull/56792. I'm not yet convinced, but we can discuss. Now that attributes are allowed in lambdas, their presence/absence should be honored. The language doesn't have the concept of target-attributing. It's unfortunate that it can lead to redundancy in some scenarios, but the behavior falls out.