Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullable warning for cast through object? #34976

Closed
stephentoub opened this issue Apr 13, 2019 · 19 comments
Closed

Nullable warning for cast through object? #34976

stephentoub opened this issue Apr 13, 2019 · 19 comments
Labels
Area-Compilers Feature - Nullable Reference Types Nullable Reference Types Resolution-By Design The behavior reported in the issue matches the current design

Comments

@stephentoub
Copy link
Member

stephentoub commented Apr 13, 2019

Version used:
Today's master

Repro:

#nullable enable
using System;
class Program
{
    public static void Main()
    {
        bool b1 = true;
        bool b2 = (bool)(object?)b1; // CS8605: Unboxing a possibly null value
        Console.WriteLine(b2);
    }
}

cc: @dotnet/nullablefc

@stephentoub
Copy link
Member Author

stephentoub commented Apr 13, 2019

Note that the above case is a simplified version of a more common pattern. Because C# doesn't let you cast from a generic T to a specific value type, code optimizing for specific types often casts through object, e.g.

 private static bool Method<T>(T value)
 {
    if (typeof(T) == typeof(bool))
        return (bool)(object?)value;
    return false;
}

This similarly warns, even though there's no way that value can be null.

@safern
Copy link
Member

safern commented Apr 13, 2019

cc: @jaredpar @cston

@cston
Copy link
Member

cston commented Apr 13, 2019

The warning in the simplified example is actually by design:

        bool b1 = true;
        bool b2 = (bool)(object?)b1; // CS8605: Unboxing a possibly null value

The result of an explicit cast to a nullable reference type is treated as nullable. Although clearly, when casting from a value type, the value is not null.

Unfortunately, the explicit cast behavior makes it difficult to handle the generic example which requires a cast to object:

        if (typeof(T) == typeof(bool))
            return (bool)(object?)value; // CS8605: Unboxing a possibly null value

Of course, ! can be used to suppress the warning:

        if (typeof(T) == typeof(bool))
            return (bool)(object)value!;

An alternative is to change the type test:

        if (value is bool b)
            return b;

@stephentoub
Copy link
Member Author

The result of an explicit cast to a nullable reference type is treated as a nullable

But it can never be null... why does the flow analysis consider it possibly null? I'm struggling to understand "The warning in the simplified example is actually by design" contrasted with "Although clearly, when casting from a value type, the value is not null.".

@gafter
Copy link
Member

gafter commented Apr 13, 2019

The ability to override the compiler's analysis by explicitly casting to a nullable reference type permits the programmer to provide information (that a value might be null) where the compiler's analysis may not be precise. The idea is that if you don't think the value can be null, don't cast to a nullable reference type.

@gafter gafter added the Resolution-By Design The behavior reported in the issue matches the current design label Apr 13, 2019
@stephentoub
Copy link
Member Author

permits the programmer to provide information (that a value might be null) where the compiler's analysis may not be precise. The idea is that if you don't think the value can be null, don't cast to a nullable reference type.

By that logic, shouldn't this warn on dereferencing o1? It doesn't:

using System;
#nullable enable
public class C {
    public void M(string s1)
    {
        if (s1 != null)
        {
            s1.GetHashCode();
            object? o1 = s1;
            o1.GetHashCode();
        }
    }
}

To be clear, I don't think either case should warn. But in both I'm casting a known-non-null to object?... I don't see how they're fundamentally different in this regard.

@gafter
Copy link
Member

gafter commented Apr 13, 2019

There is no cast in this.

@stephentoub
Copy link
Member Author

There is no cast in this.

Oh you literally meant an explicit cast. Ok, it's then really wierd to me that this warns for both:

#nullable enable
public class C {
    public void M(bool b, string s)
    {
        object? o1 = (object?)b;
        object? o2 = (object?)s;
        o1.GetHashCode();
        o2.GetHashCode();
    }
 }

but this doesn't for either:

#nullable enable
public class C {
    public void M(bool b, string s)
    {
        object? o1 = b;
        object? o2 = s;
        o1.GetHashCode();
        o2.GetHashCode();
    }
 }

I don't see why an explicit vs implicit casts should impact the warnings issued.

@gafter
Copy link
Member

gafter commented Apr 14, 2019

We could have added a new syntax or operator for the programmer to say "this expression might be null", but the explicit cast seems to fit the bill. If you believe the expression may be null, cast to T?. If you believe it cannot, cast to T. In the latter case we'll warn if the compiler disagrees (the ! operator is used to override the compiler's judgment to say that a value should be considered not null). Since the syntax of casting to a nullable reference type is new, it seems useful to use it to express the programmer's intent.

Implicit conversions, on the other hand, have no syntax. For an implicit conversion, the compiler tracks the type as a nullable type but with a known non-null value at that point in the program.

@stephentoub
Copy link
Member Author

If you believe the expression may be null, cast to T?. If you believe it cannot, cast to T.

I would if I could, but in the second example the compiler warns if I try to use (bool)(object)value, because it's not factoring in the above type check that means value can't be null, so I end up needing a !, regardless.

Since the syntax of casting to a nullable reference type is new, it seems useful to use it to express the programmer's intent.

Can you share an example where you'd want a warning in an example like the first, where it's impossible for the value to be null and the compiler can see that? I'm really curious. We're spending a lot of time !'ing away warnings, and I've yet to think to myself "man I wish I had more warnings for things that didn't need them" 😄

@gafter
Copy link
Member

gafter commented Apr 15, 2019

Can you share an example where you'd want a warning in an example like the first, where it's impossible for the value to be null and the compiler can see that?

The compiler doesn't distinguish values that are "believed" to be non-null from those that are "impossible" to be non-null. They tracked states are (1) null is not expected, and (2) null is expected. There is no tracked state of "null is actually impossible".

If you'd prefer that the compiler refine the set of tracked states, that would be a language change suitable for consideration in csharplang.

@stephentoub
Copy link
Member Author

stephentoub commented Apr 15, 2019

The compiler doesn't distinguish values that are "believed" to be non-null from those that are "impossible" to be non-null.

Let me try again then: can you share an example where you'd want a warning in an example like the first, where the value is "believed to be non-null"? e.g.

#nullable enable
using System;
class Program
{
    public static void Main()
    {
        bool b1 = true;
        object o = b1;
        bool b2 = (bool)(object?)o; // warns
        Console.WriteLine(b2);
    }
}

What's an example of a situation where I as a developer benefit from that (object?) generating a warning? Incidentally, not only does this warn that I may be unboxing a null, it also tells me that the cast is redundant and offers to remove it for me, so it seems the compiler doesn't agree with itself.
image

@gafter
Copy link
Member

gafter commented Apr 15, 2019

I don't believe "examples like the first" are intended use cases for casting to a nullable type. I don't know why that example casts to a nullable type. It seems like a bad idea.

@stephentoub
Copy link
Member Author

I don't know why that example casts to a nullable type. It seems like a bad idea.

It was meant to highlight a core piece from the second example, where if I don't cast to nullable, the compiler warns me to do so:
image
and then when I try to fix that by casting through nullable, the latter issue still remains:
image

Ultimately I'm simply trying to figure out how we can avoid the need for what is from my perspective unnecessary usage of !, and I'm trying to understand in what situations a developer would actually want (object?) to cause additional warnings.

@CyrusNajmabadi
Copy link
Member

Should the language recognize typeof(T) == typeof(StructType) to mean that instances of T within this scope cannot be null?

@terrajobst
Copy link
Member

terrajobst commented Apr 15, 2019

Should the language recognize typeof(T) == typeof(StructType) to mean that instances of T within this scope cannot be null?

Hmm, that only works for cases where the T is a known type. It seems generic code would want to write code that differentiates between reference types and value types. So if (typeof(T).IsValueType) would seem more useful.

@gafter
Copy link
Member

gafter commented Apr 16, 2019

Ultimately I'm simply trying to figure out how we can avoid the need for what is from my perspective unnecessary usage of !

The compiler doesn't use the fact that typeof(T) == typeof(bool) to reason about subsequent code. Consequently ! is actually necessary here.

@gafter
Copy link
Member

gafter commented Apr 16, 2019

This would something that could reasonably done by a theorem-prover-styly analysis (dotnet/csharplang#2388)

@gafter
Copy link
Member

gafter commented Apr 16, 2019

Closing and tracking as part of dotnet/csharplang#2388

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Nullable Reference Types Nullable Reference Types Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

6 participants