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.06k stars 4.04k forks source link

NotNull annotation warning for parameter even though it's checked for null #65684

Closed RobSiklos closed 1 year ago

RobSiklos commented 1 year ago

Version Used: .NET 7.0

Steps to Reproduce:

Compile the following code:

public static void RequireValidString([NotNull] string? s)
{
    bool isNullOrEmpty = string.IsNullOrEmpty(s);
    ThrowIfTrue(isNullOrEmpty);
} // CS8777: Parameter 's' must have a non-null value when exiting.

public static void ThrowIfTrue([DoesNotReturnIf(true)] bool isTrue)
{
    if (isTrue) { throw new ArgumentException(); }
}

Diagnostic ID: CS8777

Expected Behavior: No warning, because the ThrowIfTrue method checks the condition.

Actual Behavior: CS8777: Parameter 's' must have a non-null value when exiting. on the last line of the RequireValidString method.

If there is a way to avoid the warning with correct code, while still keeping a similar structure, please let me know.

Youssef1313 commented 1 year ago

Following will prevent the warning:

public static void RequireValidString([NotNull] string? s)
{
    ThrowIfTrue(string.IsNullOrEmpty(s));
}

public static void ThrowIfTrue([DoesNotReturnIf(true)] bool isTrue)
{
    if (isTrue) { throw new ArgumentException(); }
}

@RikkiGibson Is this by-design?

RobSiklos commented 1 year ago

@Youssef1313 Yes, that will prevent the warning, but the point isn't really about the nullable check, it's about the [NotNull] attribute on the return value causing a warning even when the method won't return.

Maybe here's a better example:

public static void RequireValidString([NotNull] string? s)
{
    bool isBad = true;
    ThrowIfTrue(isBad);
} // CS8777: Parameter 's' must have a non-null value when exiting.

public static void ThrowIfTrue([DoesNotReturnIf(true)] bool isTrue)
{
    if (isTrue) { throw new ArgumentException(); }
}
RikkiGibson commented 1 year ago

The compiler doesn't track what possible values a bool variable could have at a given point. Therefore, we do expect a warning for the ThrowIfTrue(isBad) case.

RobSiklos commented 1 year ago

Thanks @RikkiGibson , that makes sense.

So what's the best way to get around this warning? Should I just suppress it locally by enclosing in a #pragma warning disable/restore pair?

In this case, I know for a fact that the RequireValidString method cannot exit unless s is non-null. Is there any way for me to tell this to the compiler so it doesn't give me the warning, since it can't discover this for itself?

RikkiGibson commented 1 year ago

Is there any way for me to tell this to the compiler so it doesn't give me the warning, since it can't discover this for itself?

Debug.Assert(s is not null) is commonly used for this.