Missing nullable warning in generic methods

[jairbubbles]

Version Used:

[email protected]

Steps to Reproduce:

1. Unzip TestNullable.zip
2. Build

Expected Behavior:

• Have a possible null reference warning with all method calls using possibleNullValue.
• Have a warning on all methods that returns possibleNullValue asking to return a nullable.

Actual Behavior:

I only have a "possible null reference" warning on the last Check(value) (in the generic method). In fact I don't think the compiler really knows possibleNullValue can really be null so it looks like a bug to me (even if the warning is totally valid).

Adding the code error here so that people don't have to open the .zip:

class Program
{
    static void Main(string[] args)
    {
        Get();
        GetGeneric<string>();
    }

    static void Check(object value) { }

    static string Get()
    {
        var possibleNullValue = JsonConvert.DeserializeObject<string>("null");

        if (possibleNullValue == null)
        {
            Console.WriteLine("null value");
        }

        Check(possibleNullValue);

        return possibleNullValue;
    }

    static T GetGeneric<T>()
    {
        var possibleNullValue = JsonConvert.DeserializeObject<T>("null");

        if (possibleNullValue == null)
        {
            Console.WriteLine("null value");
        }

        Check(possibleNullValue); // "Possible null reference" warning here

        return possibleNullValue;
    }
}

---

[jcouv updated:]
I think we've narrowed down to the most questionable/suspicious scenario:

public static class JsonConvert
{
    public static T DeserializeObject<T>() => default;
}

#nullable safeonly

class NullTestWithoutContraint
{
    static T GetGeneric<T>()
    {
        var possibleNullValue = JsonConvert.DeserializeObject<T>();

       if (possibleNullValue is null) {} 
  
       return possibleNullValue;  // expecting a warning here, if T=string for example
    }
}

(see on sharplab)

[jcouv]

Can you tweak your code sample to remove the JsonConvert dependency by any chance? A standalone repro would be ideal. For instance, on sharplab.

[jcouv]

Hum, I tweaked your sample (replacing calls to JsonConvert with default values) and I think the problem may already have been fixed. Example

[jairbubbles]

The JsonConvert calls were in my original code base. When I stumbeld into it I wondered how the compiler knew it could return a null value.

Is it normal that Roslyn doesn't care at all if we check if a reference is null and then return it after? Only the declared type is taken in account?

[jcouv]

If JsonConvert was compiled without the nullability feature, then the API is "oblivious" which means the compiler will assume the best (and produce fewer warnings).
If it is compiler with the nullability feature on, then we'd have to look at that API (how was it annotated?).

We're working on making this information more readily available in IDE/QuickInfo (likely for 16.2 release).

[jairbubbles]

the compiler will assume the best

Of topic but: is it possible to make it assume the worst on some external assemblies or classes? (all references can be null)

If it is compiled with the nullability feature on

The code was using an old version of Newtonsoft.Json so it was very unlikely. That's why I wondered what kind a black magic was in place here.

We're working on making this information more readily available in IDE/QuickInfo (likely for 16.2 release).

Very cool! This is indeed a must have. Can we get that information with other tools for now? (like dotPeek)

[jairbubbles]

I modified the example to avoid the references to Newtonsoft.json but it does work as expected. So I'm not sure what triggers the issue.

[jairbubbles]

I couldn't reproduce my original problem but I tweaked it to highlight some issues explained in comments (see on sharplab):

public static class JsonConvert
{
    public static T DeserializeObject<T>() => default;
}

#nullable safeonly

// This class does get the two warnings correctly
class NullTestWithContraints
{
    static void Print(object value) => value.ToString();

    static T GetGeneric<T>() where T : class, new()
    {
        var possibleNullValue = JsonConvert.DeserializeObject<T>();

        // Remark: If I comment this check I don't get anymore warnings => looks ok to me
        if (possibleNullValue == null) {}

        // Remark: If I replace this by possibleNullValue.ToString(); I don't get the next warning
        // => doesn't look ok to me
        Print(possibleNullValue);

        return possibleNullValue;
    }
}

// This class only gets one warning
class NullTestWithoutContraint
{
    static void Print(object value) => value.ToString();

    static T GetGeneric<T>()
    {
        var possibleNullValue = JsonConvert.DeserializeObject<T>();

        // Remark: If I comment I still get the warning
        // => doesn't look ok to me
        if (possibleNullValue == null) {} 

        Print(possibleNullValue);

        // This is the main issue. No warning here ?
        // The only difference here is that in this case T can be a non reference-type
        return possibleNullValue; 
    }
}

[jcouv]

If you have a non-null value, but do a "pure" null test on it (such as if (nonNullValue == null) ... or if (nonNullValue is null) ...) the compiler takes this test seriously.
Practically, if (nonNullValue == null) {} tells the compiler that nonNullValue is maybe-null after the conditional, but if (nonNullValue == null) throw new Exception(); doesn't.

That said, maybe I misunderstood something. It would be easier if you distilled one or two scenarios that are unexpected for you, rather than mixing potentially well-behaved and misbehaved scenarios. Also, the name of your locals seem incorrect for current design (oblivious APIs produce non-null values).

is it possible to make it assume the worst on some external assemblies or classes?

It is not possible at the moment. This feature has many trade-offs and we're trying to find a good middle that allows existing projects to adopt the feature, as well as new projects.

[jairbubbles]

Sure I can detail the 3 weird behaviors I noticed in my example:

• Even if it's true, compiler should not infer value can be null (this is my original issue):

public static class JsonConvert
{
    public static T DeserializeObject<T>() => default;
}

#nullable safeonly

class NullTestWithoutContraint
{
    static void GetGeneric<T>()
    {
        var nonNullValue = JsonConvert.DeserializeObject<T>();

        nonNullValue.ToString(); // Why do I a get a warning here?
    }
}

(see on sharplab)

• No warning on return value if no constraint on T

public static class JsonConvert
{
    public static T DeserializeObject<T>() => default;
}

#nullable safeonly

class NullTestWithoutContraint
{
    static T GetGeneric<T>()
    {
        var possibleNullValue = JsonConvert.DeserializeObject<T>();

       if (possibleNullValue is null) {} 
  
       return possibleNullValue;  // No warning here
    }
}

(see on sharplab)

• No warning on the return value I f I unwrap the content of the Print function (no need to have generic class to see this behavior):

#nullable safeonly
class NullTest
{
    static void Print(object value) => value.ToString();
    
    static string Test()
    {
        var possibleNullValue = default(string);

        if (possibleNullValue == null) {}

        possibleNullValue.ToString();

        return possibleNullValue; // <= No warning here
    }
}

(see on sharplab)

[jcouv]

1. consider T being substituted with string?, then the warning makes sense.
2. I agree there is a problem here. For example, if T were string, then the return statement should warn
3. this scenario is correct because a warning is produced on possible null dereference on possibleNullValue.ToString(); so we consider possibleNullValue non-null from there on (the code would have thrown an NRE already if it was null)

[jairbubbles]

1. Okay but it's very hard to guess by looking at the code. I don't expect an average programmer to understand this. Do you plan to improve diagnostic messages when dealing with generics?
2. Yeah I found a real bug! :-p
3. Okay that's what I though but was not sure. I with you could make at least a simple analysis of called methods to have more consistency. But in agree that in this case this a minor issue.

[jairbubbles]

Also is there a way to add a constraint to say that T is a nullable reference?

[jcouv]

Do you plan to improve diagnostic messages when dealing with generics?

I'm sure we'll adjust diagnostics based on preview feedback and dogfooding experience. This is useful feedback.

is there a way to add a constraint to say that T is a nullable reference?

You can already do it with where T : class then using T? to say it's a nullable reference.
We're still exploring how to expression something similar when T isn't constrained to be a reference type.

[jcouv]

Note, I updated the original post with the salient scenario (2).