Incorrect warnings in class initialization scope with enabled type check

[m4tiz5zktonj]

import Toybox.Lang;

class Test {
    static const CONST_1 = (1).toNumber();
    // Unexpected types for operator '+': [Number vs Null]
    static const CONST_2 = 1 + (0.5 * 2).toNumber();

    var a as Number;
    var b as Number;
    var c as Number;

    function initialize(x as Number) {
        a = x;
        b = getValue(a);
        // Argument 1 to $.Test.getValue expected to be Number but got Null or Number
        c = getValue(a);
    }

    function getValue(x as Number) as Number {
        // Unexpected types for operator '*': [Number vs Null]
        return x * CONST_1;
    }
}

[markw65]

Argument 1 to $.Test.getValue expected to be Number but got Null or Number

I noticed this in one of my own projects. It's an unintended consequence of my fix for the missing assignment to VALUE discussed here.

That fix just added Null to the declared types of member variables inside the initialize method. But what it should do is only add it to uses that are not dominated by a store to that member. It's a fairly straightforward fix, but it may be a while before I get to it.

There's one other issue I see though. The type checker tries to keep track of the actual types of member and global variables, which is why it doesn't complain about b = getValue(a). At that point, it knows that x has been assigned to a.

Typically, after a function call, information about globals and member variables is reset to the declared type (or in this case due to the bug, declared type+Null). But the type checker tries to keep track of which functions might modify which variables - and in this case it should be clear that getValue doesn't modify any. So it looks like there's actually a second bug here.

[markw65]

For the CONST_1 and CONST_2 issues, I'm not sure what's going on. Some flavors of toNumber include Null in their return types, but the type checker should see that that's not the case here.

For the x * CONST_1 in getValue, I need to check exactly what the rules are these days; garmin has changed the rules a few times regarding lookups of statics from non-static methods and vice versa (both the actual runtime behavior, and the behavior of their type checker, which haven't always been in sync). I'm pretty sure there have been releases where getValue really would crash at runtime.

I'll investigate when I get some time.

[m4tiz5zktonj]

There's one more example, where 2.0.96 with enabled type check gives such warning:

import Toybox.Lang;
import Toybox.Math;

class Test {
    const CONST = Math.PI.toFloat();

    function getValue(value as Number) as Numeric {
        // Unexpected types for operator '*': [Number vs Null] [pmc-analysis]
        return value * CONST;
    }
}

---

Funny fact: I was going to add another method to the class above to illustrate different case I have, that gives the same or similar warning, but I realized there's actual error in my code, that was found by Prettier Monkey C and was completely ignored by Garmin's compiler with strict checking:

    typedef ValueType as Number or Float or String or Null;

    function getAnotherValue(value1 as ValueType, value2 as ValueType) as Float {
        // Unexpected types for operator '/': [Float vs Null], [Null vs Null or Float] [pmc-analysis]
        return value1 != null && value2 != null && value2 != 0 ? value1.toFloat() / value2.toFloat() : 0.0;
        //                                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    }

At first ValueType didn't contain String (whose toFloat can return null), and later I expanded it, and forgot to check getAnotherValue method's code. I don't actually call this method with string arguments, thus there's no runtime error. It's a shame Garmin misses this. Keep it up, PMC!

[markw65]

I tried your example, and it didn't report a problem - and then I realized I have another bug. If you set prettierMonkeyC.typeCheckLevel to anything other than strict, my type analyzer doesn't do strict type checking. That wasn't what I intended. Setting prettierMonkeyC.typeCheckLevel should only affect the final build by Garmin's compiler.

prettierMonkeyC.checkTypes should determine whether or not to do type checking at all (and whether to report errors or warnings); but we still need to differentiate between strict and non-strict type checking - so I'm adding a new option prettierMonkeyC.strictTypeCheck which can be "On", "Off", or "Default". When it's set to "Default" (the default!) it will do strict checking if either monkeyC.typeCheckLevel or prettierMonkeyC.typeCheckLevel is "Strict".

Having fixed that, my test project now reports the error - so I'll see what needs fixing there...

[m4tiz5zktonj]

One more in 2.0.97, sorry.

import Toybox.Lang;
import Toybox.Math;

class Test {
    const CONST = Math.sin(0).toFloat();

    function getValue(value as Number) as Numeric {
        return value * CONST;
    }
}

[markw65]

One more in 2.0.97, sorry

I guess I should just fix this properly, or you'll just keep extending the test case :-)

The issue is that I designed type propagation to work function-at-a-time, and I don't have a function in these cases, but I figured I could get away with ad hoc analysis because the expressions would be simple.

I should just synthesize a method for all the const/var initializers in any given scope, and then run the normal type propagation on that. Which is pretty much what Garmin does in the runtime anyway - it puts all those initializers into the <init> method.

[markw65]

Should be fixed in v1.1.71 (v2.0.100 of the extension).