Type flow analysis issues with nullable class variables

[m4tiz5zktonj]

import Toybox.Lang;

//

class C1 {
    function f1() as Void {}
}

class C2 extends C1 {
    private var x as Number?;
    private var y as Number?;

    function initialize() {
        C1.initialize();
    }

    function f2() as Number {
        if (x != null && y != null) {
            f1();

            return x + y; // Unexpected types for operator '+': [Number vs Null], [Null vs Null or Number]
        } else {
            return 0;
        }
    }
}

//

class C3 {
    private var z as Array<Number>?;

    function f3() as Array<Number> {
        if (z != null) {
            (z as Array<Number>)[0] = 0;

            return z; // Expected $.C3.f3 to return Array<Number> but got Null or Array<Number>
        } else {
            return [] as Array<Number>;
        }
    }
}

Plugin version is 2.0.121. Builds without errors with SDK 8.4.0 and strict type checking.

[markw65]

For now, the C2 case is "as expected". Without a lot more analysis, it's quite possible that f1 modifies x or y (even though they're private, f1 could call public methods of C2 that modify them).

Of course, in this case, f1 does nothing, and even if it did, there are no public methods in C2 that modify x or y, and some fairly simple analysis could determine that. But I don't think it's worth doing - in any real code, they would be set somewhere (otherwise they'd just be constants of type Null), so optimizing for an unrealistic case seems pointless.

For C3, (z as Array<Number>)[0] = 0 is treated as an assignment that can't be analyzed, which throws away all derived information (like z != null). This one should be easier to deal with; with suitable local checks, it's easy to see that the assignment can't affect z, so it should be fixable. I'll see what needs to be done...

Builds without errors with SDK 8.4.0 and strict type checking

The problem is that Garmin tends to just ignore issues like this. I haven't checked, but I'm pretty sure that if you added a public method to the first C2 that set x and y to null, and then called it from f1, their compiler would still be happy. I will go test it :-)

[markw65]

The problem is that Garmin tends to just ignore issues like this

It's even worse than I thought. Even without the if 8.4.0 doesn't report an error (or even warning) - even though in the example both x and y are guaranteed to be Null.

On the other hand if I change the type of x to Number or Boolean or Null, it correctly reports that you can't add a Boolean and a Number. So it is checking the operands, but it completely ignores Null.

[edit: I had to go all the way back to 4.1.5 to find a version that noticed the Null issue. That one doesn't report a problem if you check for null immediately before the addition, but it does if you call f1() - ie the behavior is pretty much the same as mine]

[markw65]

These are both sort of fixed in v2.0.125.

It turns out I wrote some analysis code a long time ago that tries to summarize the possible side effects of each function. But it was only populated when running the optimizer.

I've moved that into the analysis phase, and that actually fixes your first example - but It really needs more work; at the moment it's order dependent - for example, if the callee is analyzed after the caller, you won't get any benefit (I can fix that by iterating to convergence - but that will come later), but for your case, the callee is analyzed first. The summary information also isn't very precise, so you can't do much in the callee without it breaking the null analysis. But calling a pure function should be safe.

I also fixed the assignment through a "cast" array issue, so both examples work now.

[m4tiz5zktonj]

Thanks, this fixed it for the examples and for my C3.f3-like code, but not for my C2.f2 (no problem - it is much more complicated, and probably just doesn't satisfy the conditions you mentioned).

Unfortunately, the fix introduces a new issue. It seems the analyzer is confused by seeing a local variable and a function with the same name.

import Toybox.Time;

function m() as Moment {
    return new Moment(0);
}

function f(d as Duration?) as Moment? {
    var m = m();
    // Argument 1 to $.Toybox.Time.Moment.add expected to be Toybox.Time.Duration but got Boolean or Number or Long or Float or Double or Char or String or Array or Dictionary or Method or Module or Function or Class or Object or Enum or Symbol or Typedef
    return d != null ? m.add(d) : null;
}

[markw65]

Unfortunately, the fix introduces a new issue.

Good catch. Mostly I've been pretty careful about locals vs calls, but this particular place in type analysis has apparently always got this wrong. What happens here is that it treats the call to m() as an unknown call for "interference" purposes, so assumes it could affect anything "global" (note that it correctly identified the call when analyzing it's return type).

My (partial) fix for #66 introduced an incorrect short-circuit which inadvertently marked function parameters as possibly modified by unknown calls. There was an easy fix, and your example stopped giving an error.

But that still left the long-standing problem; ie the call to m() is actually a known call.

That was also an easy fix - but I'd probably never have noticed the problem without your example. After fixing that, this example also works:

function m() as Moment {
    return new Moment(0);
}

var z as Array<Number>?;

function f(d as Duration?) as Moment? {
    if (z != null) {
        var m = m();
        z[0] = 0;
        return d != null ? m.add(d) : null;
    }
}

i.e it now knows that the call to m() can't change z, so the assignment to z is safe.

[markw65]

The regression is fixed in v2.0.126.

[m4tiz5zktonj]

One more example:

import Toybox.Lang;

class C {
    private var x as Number?;

    function f() as Void {
        if (x != null) {
            for (var i = 0; i < 1; i++) {
                g(x); // Argument 1 to $.C.g expected to be Number but got Null or Number
            }

            x = null;
        }
    }

    function g(y as Number) as Void {}
}

[markw65]

One more example:

This is because of the order dependency I noted above. If you move the definition of g above f it should work (when I get time, I will definitely fix the ordering issue though).