Added: LDM notes for Dec. 18, 2019

[agocke]

https://github.com/dotnet/csharplang/blob/e4b05fb2cad47ea67c5b4dcb46bf4143895d571f/meetings/2019/LDM-2019-12-18.md

Agenda:

1. Pure null checks
2. var?

[qrli]

Glad to see that var? is not necessary and it is solved by flow analysis, otherwise the meaning of var would be confusing and may need another var!.

[vladd]

I've got mixed feelings about var being nullable.
The syntax for non-nullable types (without !) was chosen because the typical variable/field type is non-nullable. Now choosing var to be always nullable means that users who want to declare the type as non-nullable and let the assignments of nullable values produce warnings would have to type the explicit type. (And this ought to be the typical scenario.)
Moreover, we are invalidating the well-know rule "you may freely replace var with the exact type of the RHS without any effect".

[alrz]

you may freely replace var with the exact type of the RHS without any effect

You can. Note that you will get a warning if you actually pass a possible-null value to a non-nullable location - the compiler still maintain the actual state of the variable.

Also, since that is only concerning reassignments, it would make sense to NOT do that for "readonly locals" and make them infer nullability from the init expression.

[vladd]

@alrz
Yes I can, but not without any effect. Example.

My point is that usually I want the LHS location to be a non-nullable one, and I want to get the warnings when I am assigning nullable values to that location, not when I am assigning the value from this location to a non-nullable location.

[vladd]

I'd humbly propose:

• var lhs = rhs; // lhs has exactly the same nullability as rhs
• var? lhs = rhs; // lhs is nullable
• var! lhs = rhs; // lhs is non-nullable; if rhs is nullable, this produces a warning which can be silenced with !

[mikernet]

Where/when is the concluded var? resolution expected to be implemented? As in, will it be with C# 9, or a VS update, or a .NET Core patch release, etc?

[mikernet]

The var / var? situation made me realize something - would it not have been better to just treat all locals as nullable and apply the appropriate NRT flow analysis to determine safe usage?

The point at which nullability really matters is when a method call is made on the variable, when it is passed into another method or when it is returned, at which point flow analysis would determine whether it was safe or not. If you don't care to make a distinction between var and var? because flow analysis is good enough then why on any other type?

To be clear, I agree with your position on var?, I just think that the idea should be taken further. I don't know how much value is added by forcing the extra work of explicitly declaring locals as nullable or non-nullable. If there was a ton of value in that then I presume you would have decided to include a var? identifier that would be required anywhere an implicitly typed nullable value is needed.

Here is another similar situation aside from var/var? where this would be helpful:

public void SomeMethod(object p1, object p2)
{
    if (p1 is string name) {
        // ...
    }
    else {
        if ((name = p2 as string) == null) { // error converting null value to non-nullable type
            throw ...
        }
    }
}

The syntax if (p1 is string? name) is not allowed, so now I have to refactor a bunch of stuff instead.

[mikernet]

The other sensible option would have been to make var? mandatory for all implicitly typed nullable values. At some point, a judgement must have been made that explicitly specifying local variable nullability had value, so I find it odd that var got left out and we have to pick between implicit typing and specifying nullability but can't get both.

[333fred]

Soon. @jcouv?

[jcouv]

@mikernet We had the discussion about whether to let maybe-null values into locals, and decided against it. However, those produce a distinct warning (CS8600) which can easily be suppressed for those who don't care about the discipline/readability/intent this provides.

We decided against var? because it does not provide the benefit of readability/safety one might hope for. For example, var x = (int?)null; would allow a null into a var that was not a var?. So a reviewer could not rely on the var/var? to figure out nullability... :-(

As for the type inferred for var to be nullable (if possible), the fix following this LDM meeting (2019-12-18) will first be available in VS 16.6 preview 1.

[DavidArno]

@jcouv,

I do not think your argument against var? stands up to scrutiny. var just means "infer the type from the RHS". That's what it's always meant. With the introduction of NRTs it could therefore infer T or T?, depending on that RHS. A reviewer cannot rely on var to figure out anything without referring to the RHS. var? simply guarantees that the type will be T?`.

Changing var from "infer the type from the RHS" to "infer the type from the RHS, then force it to be nullable" is a mistake in my view. But if you must make this change, please at least consider this code example a bug:

#nullable enable

class C 
{
    string NotNullable()
    {
        var s = (string)"";  // <- ignores the explicit cast and makes it string? regardless
        s = Nullable();
        return s;
    }
    
    string? Nullable() => "";
}

At least that way I can work around this unfortunate change for the worse by overriding the new default behaviour with a cast.

[jcouv]

@DavidArno

As a quick recap we considered the following options (at length), and maybe some others that I forgot:

1. status quo (user must use explicit type with ?)
2. var?
3. suppress W-warnings related to locals with inferred type
4. infer a nullable type for var (plan of record)

You're promoting some flavor of option (2). I used to as well ;-)
So I started an internal thread promoting that option at the time. After discussions, I was convinced otherwise and summarized my thoughts. I'm copying them below. I hope that'll help.

In option 2, we’re not offering proper protection for people who care about W-warnings.
Looking at some code var x = SomeMethod(); it is not clear whether I intend this variable to be nullable or not. And the intent would be muddled by changes to the SomeMethod signature.
So while var? would clearly indicate a nullable type, var would still be a mixed bag of nullable and non-nullable.

We could conceive another option, 2B, where var means non-nullable and var? means nullable.
But this option as a few problems:

1. It would likely make adoption more annoying (some data could help this assumption, if other problems were solved)
2. If var means non-nullable, then it is inconsistent with the behavior of the var pattern that shipped (is var x) which does not perform a null check.
3. We would have to figure out the behavior of var x = GetNullableInt(); // return int?:
   a. If it is allowed, then var doesn’t mean “non-nullable”
   b. If it is not allowed then we’re introducing a bunch of W-warnings for code which doesn’t need it and has no safety issue downstream (because Nullable<int> is a different type than int).

This sinks option 2 in my mind. But I’m convinced that we have a problem (seeing our adoption PRs).
And since I’m not satisfied with option 1 (do nothing), that leaves me to prefer option 4 (plan of record) :-)

---

s = Nullable();

Yes, there is a downside of the approach we selected (we don't warn on this assignment, and only warn later, with a safety diagnostic). If I could, I would prefer to avoid it, but the trade-off was judged better than other options.

var? simply guarantees that the type will be T?

Sure, we could do that, but to what end or benefit?

[mikernet]

@jcouv

However, those produce a distinct warning (CS8600) which can easily be suppressed

That's fantastic. I've been playing with silenced CS8600 and I must say that I really like the result...inferred/dynamic local variable nullability based on flow analysis is very nice and fits well with the var situation. I don't really feel like I've lost anything - I have gotten into the habit of mousing-over values to check their nullability state when it matters anyway since the declaration often doesn't tell you much, especially when using var.

The only thing that I think would really nicely polish off this feature is a very subtle visual indicator for the null flow state of your values, which would make local nullability inferred/dynamic but readily visible...IMO the best of both worlds, so looks like I'm doing up an extension for that 😃

[HaloFour]

This behavior feels like a regression with var:

    public void MyMethod()
    {
        var callLog = new List<string>(3);
        // do stuff with with NestedMethod etc.,
        void NestedMethod(string s) => callLog.Add(s);
    }

Previously the callLog.Add call did not warn, but now it does, despite callLog not possibly being null.

Looks like that was the behavior as well if you declared callLog to explicitly be List<string>?, but that also doesn't seem right.

[agocke]

@HaloFour I don't see any warning https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxASwDYB8ACAmARgFgAoHABgAIcCAWAblNJwGZq9KBhSgb1MoHU2OWpQCyATzFwYACwD2AEwAUASn6C+JQTsoA3CAkoBjCBgwAZeQHNKAXkoA7OAHdKFtLAA8NcgD5lFlVGbV0BAHpwykV5SlgAVwAzRMoXNDlU9NlKADk4WDhFaTklShljADokDTC8gqKZBRUAImbgmt0RXPz4BpKVXzjVez8TM0sbCoBBRRUodtCBAF9SJaA=

[HaloFour]

@agocke

Curious, you get the warning if you comment out the call to the local function.

[Joe4evr]

@agocke In addition to what Halo said, you also forgot to place #nullable enable.

[agocke]

Yeah, even with #nullable enable, no warning. This is probably because the local function is unreachable and the starting state of unreachable local functions is the annotation, not the flow state. I think that should be changed, but we never had confirmation on that design change

[agocke]

That said, if the local function is unreachable you can also delete it :)

[egil]

@agocke, this is the more complex example that case of what @HaloFour (which I originally asked about on Gitter):

[Fact]
public async Task Test001()
{
    var callLog = new List<string>(3);
    var cut = RenderComponent<FixtureComponent>(builder => builder
        .Add(p => p.Setup, MyLocalMethod)
        .AddChildContent("FOO")
    );

    // callLog assertion

    void MyLocalMethod(FixtureComponent fixture) => callLog.Add(nameof(MyLocalMethod));
}

The complete test is over in my repo.

So in this case where I get the warning, I pass MyLocalMethod to a another type, that will call it when the test is running.