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

C# should allow implicit Nullable<T> conversion if data-flow proves not-null #3317

Closed
jods4 opened this issue Mar 27, 2020 · 10 comments
Closed

Comments

@jods4
Copy link

jods4 commented Mar 27, 2020

Version Used: Visual Studio, C# Tools 3.5-beta4

Steps to Reproduce:
Implicit conversion of Nullable<T> to T has always been forbidden by C#, because this conversion can fail if the nullable is actually null.

It can be explicitely done by either casting y = (T)x or using the value property y = x.Value. Both throw if x doesn't hold a value.

Nowadays C# 8 has nullable data-flow analysis (for nullable reference types) and we become used to going from string? to string simply by having a if guard (or otherwise).

It feels weird and is a missed productivity enhancement that the data-flow analysis is not applied to nullable value types.

I understand that the underlying type of a nullable value is different and it won't magically change thanks to a if, but implicit conversion should be allowed.

Examples:

int? x = 0;
int y = x;

x = random;
if (x != null) 
  y = x;

Expected Behavior:
These implicit conversions should be allowed by the compiler to happen, as if I wrote y = (int)x, as the compiler can prove they won't fail.

Actual Behavior:
CS0266: Cannot implicitly convert type 'int?' to 'int'.


Bonus round: If we're being very bold, I suggest that methods of T be lifted to Nullable<T> whenever the data-flow indicates it's not null. That would be a nice improvement and unify the way references and value types feel when it comes to nullability.

DateTime? dt = DateTime.Now;
dt.ToShortDateString();

This would be ambiguous for members that are shared between Nullable<T> and T. Most likely none but a struct could have a Value of its own.
In this case I suggest the nullable takes precedence (you'll have to do x.Value.Value).

@jcouv
Copy link
Member

jcouv commented Mar 27, 2020

In the current design of the nullability feature, nullability analysis only affects warnings, it doesn't affect the semantics of the language.
Your proposal would cause a problem as currently the understanding of the language comes before nullability analysis, but now nullability analysis would affect the understanding of the language, thus causing a circular dependency.

The roslyn repository is for compiler and IDE bugs. Language discussions should be moved to csharplang repository. Thanks

@jods4
Copy link
Author

jods4 commented Mar 27, 2020

Sorry, I figured it could apply to VB and C# equally. Can you move it?

The lifting of methods is rather radical, esp. with the chicken and egg problem you mention. Let's forget that.

The first part is doable, though. If you think about it, it's only affecting warnings/errors not the understanding of the language. This time it would be about removing an error rather than adding one.

During the first pass, the compiler sees an implicit conversion from Nullable<T> to T. That's what it is and there's no semantic to change here.

Later after data-flow it can decide whether:

  1. that's an error (as it always is today).
  2. that's ok and can proceed.

I'm sure the error is emitted rather early in current code, but that's an implementation detail: maybe you can move it to a later stage?

@jcouv jcouv transferred this issue from dotnet/roslyn Mar 27, 2020
@HaloFour
Copy link
Contributor

Some of this is discussed here: #1865

@CyrusNajmabadi
Copy link
Member

Note: i would only want this if provably safe (since it changes codegen). if this was done, i should never get a null-ref exception with the assignment/use of the variable.

@333fred
Copy link
Member

333fred commented Mar 27, 2020

As has mentioned in #1865, we considered this and explicitly decided against it. Now that C# 8 has shipped, changing this would be a breaking change (and would have been a breaking change at the time, as it could affect overload resolution).

@jods4
Copy link
Author

jods4 commented Mar 28, 2020

@333fred How is allowing an implicit cast that was previously forbidden a breaking change?

It doesn't change the behavior of any program that was valid before the change.
It only allows previously invalid programs to compile, and that's not a breaking change AFAIK.

@333fred
Copy link
Member

333fred commented Mar 28, 2020

public class C {
    public void M(long? l) {}
    public void M(int i) {}
    void M2(int? i) 
    {
        if (i != null)
            M(i);
    } 
}

This program is valid today. It would become ambiguous with this change and fail to compile.

@jods4
Copy link
Author

jods4 commented Mar 28, 2020

@333fred It's a good point.
It can be fixed by specifying that during overload resolution, implicit non-null casts are worse than other implicit casts, can't it?

@333fred
Copy link
Member

333fred commented Mar 28, 2020

No. Overload resolution does not try to guess which implicit conversion would be better, it only attempts to find the most specific method. This is only the tip of the iceberg here, as well. There are scenarios where this can change lambda inference, type parameter inference, and others. This is really not a change that we can consider.

@jods4
Copy link
Author

jods4 commented Mar 28, 2020

Well if it gets so complex I guess it's not really worth it, feel free to close this.
It would be a small quality-of-life improvement but there are other things I'd rather have in C# 9.

@333fred 333fred closed this as completed Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants