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
18.92k stars 4.02k forks source link

Apply NotNullIfNotNull with multiple arguments #43906

Open alrz opened 4 years ago

alrz commented 4 years ago

Currently multiple NotNullIfNotNull behaves as if they are being OR'ed

using System;
using System.Diagnostics.CodeAnalysis;
#nullable enable
public class C {
    [return: NotNullIfNotNull("a")]
    [return: NotNullIfNotNull("b")]
    public object? M(object? a, object? b) {
        if (a != null && b != null)
          return new {a, b};
        return null;
    }

    void Test()
    {
        M(null, new()).ToString();   // no warnings (one expected)
        M(new(), null).ToString();   // no warnings (one expected)
        M(null, null).ToString();    // warns (ok)
        M(new(), new()).ToString();  // no warnings (ok)
    }
}

If this is the intended behavior, a possible solution is to permit multiple arguments on the attribute

[return: NotNullIfNotNull("a", "b")]
public object? M(object? a, object? b) 

So that we get a warning if either of arguments are possible nulls.

jnm2 commented 4 years ago

Had a situation like this last week, but I didn't write it down. :-(

wilhelmzapiain commented 4 years ago

I have a use case:

public static T Min<T>([AllowNull] T left, [AllowNull] T right)
    where T : IComparable<T>
    => left == null ? (right == null ? default : (right.CompareTo(left) > 0 ? left : right)) : (left.CompareTo(right) < 0 ? left : right);

If either left or right is null the return value may be null, depending on the implementation of IComparable<T> (although the spec for IComparable<T> says that if either one is null the return value is null.)

jnm2 commented 4 years ago

@wilhelmzapiain That method would confuse me. I'd expect Enumerable.Min semantics: "the min of everything that isn't null"

mikernet commented 1 year ago

Alternate option: https://github.com/dotnet/csharplang/discussions/6981

csharper2010 commented 8 months ago

@wilhelmzapiain That method would confuse me. I'd expect Enumerable.Min semantics: "the min of everything that isn't null"

Maybe but if you want to write a null-safe Plus operator in analogy to SQL semantics, you have a perfectly valid example for this issue.

Either a solution with...

[return: MayBeNullIfNull(nameof(left)), MayBeNullIfNull(nameof(right))] // with OR semantics
public static Value operator +(Value? left, Value right) {}

... or ...

[return: NotNullIfNotNull(nameof(left), nameof(right))] // with AND semantics
public static Value? operator +(Value? left, Value right) {}

... would be highly appreciated also for our project where we have such operators used in really many places and refactoring all of them out to fully apply nullable reference types would be hell.

PleasantD commented 2 months ago

I just encountered this when trying to update some operators.

public static Measure? operator *(Measure? left, Measure? right)
{
    if (left == null || right == null)
    {
        return null;
    }
    return new Measure(/*multiply left and right*/);
}

IMO MayBeNullIfNull isn't entirely clear. There's no concrete guarantee that the return will or won't be null based on the input nullability. It would be clearer as [return: NullIfAnyNull(nameof(left), nameof(right))] -- this says that the return WILL BE null if any are null.

AND semantics are already established for this,, but could use a clearer name: [return: NotNullIfAllNotNull(nameof(left), nameof(right))]

csharper2010 commented 2 months ago

IMO MayBeNullIfNull isn't entirely clear. There's no concrete guarantee that the return will or won't be null based on the input nullability. It would be clearer as [return: NullIfAnyNull(nameof(left), nameof(right))] -- this says that the return WILL BE null if any are null.

You usually don't want to know when exactly a return value WILL be null but when the return value will surely be NOT null as only then, null checks in calling code can be omitted.