Pattern matching with null leads to runtime error

[frabbit]

The following code fails at runtime with:

Uncaught exception - Invalid field access : index

enum T {
    T1(x:Null<T>); // same problem for T1(?x:T)
    T2(x:Int);
}
class Test  {

  public static function main () {
    switch (T1(null)) {
        // here is the problem, i guess a null check is needed here 
        // if the parameter is optional.
        case T1(z = T2(1)): trace(z); 
        case _ : trace("else");
    }
  }
}

[Simn]

I'm not sure if this isn't working as expected. You can add a T1(null) pattern to manually deal with this case, which is consistent with toplevel matching. That is, the following crashes without the null pattern:

enum T {
    T1(x:Null<T>); // same problem for T1(?x:T)
    T2(x:Int);
}
class Main  {

    public static function main () {
        var t:Null<T> = null;
        switch(t) {
            case null:
            case T1(_):
            case _:
        }
    }
}

We don't want to add hidden null checks everywhere, so it seems fair to leave this responsibility to the user as with all other null scenarios. What do you think?

[ncannasse]

I agree we shouldn't add null checks when the user hasn't been explicitly saying it can be Null (or optional). But when that's the case we should add null checks to prevent an error occurring on pattern matching, which is unexpected.

[frabbit]

i actually run into this problem while writing the python backend, see https://github.com/frabbit/hx2python/blob/development/src/python/gen/PythonTransformer.hx#L384.

Actually i would prefer null checks if you deal with Null<T>-types but not with regular T-types.

[delahee]

@ncannasse proposal seems fair.

2014-01-29 frabbit [email protected]

i actually run into this problem while writing the python backend, see
https://github.com/frabbit/hx2python/blob/development/src/python/gen/PythonTransformer.hx#L384
.

Actually i would prefer null checks if you deal with Null types but not
with regular T types.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2580#issuecomment-33570174
.

David Elahee

[Simn]

I don't know, this could potentially add many null checks in places where they aren't required. Consider code like this:

if (enumValue != null) {
    switch(enumValue) { ... }
}

This is good practice, but adding implicit null checks would cause this to be detrimental to performance. We cannot properly check the redundancy of the null check without some notion of static analysis.

I also don't quite see why pattern matching should be special with regards to null checks. We don't add them upon possiblyNull.field access even for explicit Null<T> types, so why should pattern matching be different?

[frabbit]

i guess it's only required for nested parameters (inside of patterns) not at the top level of the switch.

[ncannasse]

Agreed with @frabbit

[Simn]

This is a bit messy to implement, I'll move it to 3.2.

[Simn]

We cannot add this without adding null as a required value for exhaustiveness. Consider this:

enum T {
    T1(x:Null<T>);
    T2(x:Int);
}

class Main  {
    public static function main () {
        switch (T1(null)) {
            case T1(T2(_)):
            case T1(T1(_)): trace("ok");
            case T2(_):
        }
    }
}

There is no case which covers T1(null) here.

I don't remember why null patterns are not part of the exhaustiveness check, but the relevant code is commented out so I assume there was some reason.

[Simn]

I have added this. We will have to deal with some pseudo-regressions now such as HaxeFoundation/format#13

[frabbit]

i have to say, i quite like the required toplevel check against null, because it's a source of bugs.

[Simn]

My take is that if you remember to type nullable types as Null<T>, you also remember to check their null value.

[frabbit]

yes, it's good if Null<T> has a meaning instead of just being "decoration".

[frabbit]

you added an implicit null case, which means that this example compiles fine (no exhaustiveness check). Is that expected?

class Test {

  public static function main() {

    var o:Null<Int> = 0;
    var x = switch o {
      case x if (x > 1): x;
    }
    trace(x); // null

  }

}

[Simn]

I don't think that's related to this change. It should give an error regardless.

[frabbit]

ok, i added an issue #2914