dotnet/roslyn

Improve NRT warning CS8604 in cases where implicit conversions cause the warning

Open

#65,369 opened on Nov 11, 2022

View on GitHub
 (1 comment) (4 reactions) (0 assignees)C# (4,257 forks)batch import
Area-CompilersConcept-Diagnostic Clarityhelp wanted

Repository metrics

Stars
 (20,414 stars)
PR merge metrics
 (Avg merge 6d 17h) (256 merged PRs in 30d)

Description

Background and Considerations

The NRT warning CS8604 is lacking in information when it is caused by an implicit conversion.

Example

public struct C
{
    public static implicit operator string?[]? (C value)
    {
        return null;
    }
}

public static class A
{
    public static void B(params object?[] d)
    {
    }

    public static void E()
    {
        var a = new C();
        B(a);
    }
}

sharplab.io

In this code B(a); will throw warning CS8604: Possible null reference argument for parameter 'd' in 'void A.B(params object?[] d)'. Due to the implicit conversion to C -> string?[]? . However this failure reason is not reflected in the error message. Requiring internal knowledge about the type C to discern the real cause.

Take a rather nasty real world use case in Asp.Net on .NET 7

            if (_httpContextAccessor.HttpContext!.Request.Headers.TryGetValue("Origin", out var origin))
            {
                _logger.LogInformation("Site User Request Initialized from Url: {Origin}", origin);
            }

in the _logger.LogInformation call origin will throw the warning CS8604: Possible null reference argument for parameter 'args' in 'Microsoft.Extensions.Logging.LoggerExtensions.LogInformation'.

This is due to origin having the type StringValues having an implicit conversion to string?[]? (Related issue here https://github.com/dotnet/runtime/issues/78241)

Choosing to implicitly convert in LogInformation() due to LogInformation taking a type of params object?[] as an argument. However this failure reason is not immediately obvious and made worse by the fact that normal methods of checking for null to resolve this type of NRT error wont work as the implicit conversion is in the Log call itself.

E.G this will not work:

            if (_httpContextAccessor.HttpContext!.Request.Headers.TryGetValue("Origin", out var origin))
            {
                if (origin is null)
                {
                    return;
                }
                _logger.LogInformation("Site User Request Initialized from Url: {Origin}", origin);
            }

As origin is a non nullable value type, creating an incredibly confusing set of error messages

CS0037: Cannot convert 'null' to 'Microsoft.Extensions.Primitives.StringValues' because it is a non-nullable value type

followed immediately by

CS8604: Possible null reference argument for parameter 'args' in 'Microsoft.Extensions.Logging.LoggerExtensions.LogInformation'

These two errors make seemingly no sense together as they are referring the exact same name.

Proposal / Discussion

Improve the error message of CS8604 to state that an implicit conversion is causing the NRT warning and ideally the type it is attempting to convert to that is failing.

E.G: CS8604: Possible null reference argument for parameter 'args' in 'Microsoft.Extensions.Logging.LoggerExtensions.LogInformation' due to implicit conversion in 'StringValues' type

or

CS8604: Possible null reference argument for parameter 'args' in 'Microsoft.Extensions.Logging.LoggerExtensions.LogInformation' due to implicit 'StringValues' conversion for params object[] from StringsValues to string?[]?

Benefits

Improved developer experience so developers do not have to dive into a types source code to find the cause of the error.

Contributor guide