Big update to prettier-extension-monkeyc

OK, I was able to "fix it": I realized that I had the following code:

if (maxHr != null && zero < (maxHr as Number)) {
    var z0 = (MAX_HR_2_Z1_MIN_PERCENT * maxHr).toNumber();
    var z1 = (MAX_HR_2_Z1_MAX_PERCENT * maxHr).toNumber();
    var z2 = (MAX_HR_2_Z2_MAX_PERCENT * maxHr).toNumber();
    var z3 = (MAX_HR_2_Z3_MAX_PERCENT * maxHr).toNumber();
    var z4 = (MAX_HR_2_Z4_MAX_PERCENT * maxHr).toNumber();
    var z5 = maxHr;
    heartRateZones = [z0 - one, z1, z2, z3, z4, z5] as Array<Number>;
}

and I just replaced it with:

if (maxHr != null && zero < (maxHr as Number)) {
    heartRateZones = [
        (MAX_HR_2_Z1_MIN_PERCENT * maxHr).toNumber() - one,
        (MAX_HR_2_Z1_MAX_PERCENT * maxHr).toNumber(),
        (MAX_HR_2_Z2_MAX_PERCENT * maxHr).toNumber(),
        (MAX_HR_2_Z3_MAX_PERCENT * maxHr).toNumber(),
        (MAX_HR_2_Z4_MAX_PERCENT * maxHr).toNumber(),
        maxHr] as Array<Number>;
}

Which makes me think that in this and similar cases the optimizer could give a warning that these variables are unnecessary and could be inlined by the developer (in this case it saves code, and stack). Maybe the optimizer could just do this replace automatically as well in some cases.

flocsy said:

you seem to remove public functions that are not called by my code.

Yes, as long as there's no way to call them.

There is a small hole in this, which is that I do rely on inheritance to tell me what the watch might call. eg if you write a class that extends DataField, I know onUpdate might be called, because DataField defines onUpdate. But it turns out that (at least some of) the inheritance described in the monkeyc documentation is not required. eg as long as it implements all the necessary methods, you can use any class as a Menu2InputDelegate; it doesn't actually have to extend Menu2InputDelegate. And if you do that, I'm going to remove your methods, and it's going to crash (this came up in a discussion several months ago if you remember). Note that Garmin's type checker will yell at you if you do this anyway, so you have to turn that off (or explicitly cast things) to do this in the first place.

So I added a (:keep) attribute. If you really must define Ui classes that don't extend the appropriate base class, then you'll have to manually mark the methods you want to keep. 

flocsy said:

If barrels can be distributed as bytecode

They aren't they're distributed as zipped source code. If you try to run my optimizer on a barrel project it will tell you to run it on the project that uses the barrel instead - for pretty much this reason.

flocsy said:

However the optimizer enters some loop and probably dies with a stackoverflow

Oh. I'll take a look. But you're saying this only happens with a program that was broken to start with?

flocsy said:

Anyway, maybe you should use instead of pre_foo just pre1,...preN and in every place where it's used add a comment with it's "current" meaning (just to make it easier to read it)

The plan is to make this generic. Rather than fixing pre to not create extra variables, have a final step that merges non-conflicting variables. That way it can reduce the number of variables even when it wasn't the optimizer that created them...

Yes, only with the broken code 

v2.0.45 is out.

• Fixes an incorrect type checker warning
• Adds a CompletionItemProvider. This provides completion suggestions as you type. Garmin's extension already does that, but it doesn't know about local variables, and it mostly doesn't know about context, so it suggests every name it knows about, rather than just the ones that apply at this point in the code. Unfortunately, there's no way to turn Garmin's off, so its suggestions get mixed in with mine. For the most part though, my suggestions should generally appear at or near the top.
• Adds a SignatureHelpProvider. This is triggered when you type the opening '(' for a call, or a ",", and it shows the arguments to the current function, and which one you're currently entering.

flocsy said:

However the optimizer enters some loop and probably dies with a stackoverflow

Doesn't repro for me with either 2.0.44 or the just released 2.0.45. Also, I assume you meant no_foo isn't set as an exclude annotation, and foo is set as an exclude annotation; since the other way around seems like it should compile. In any case, I tried it both ways, and didn't get an error.

In the last couple of upgrades of Prettier Monkey C I did not see a notification that there is a newer version (I think it was an Update button). This might be because there wasn't enough time between the release and when I checked. However what's even stranger is that after I click cogwheel > Install Another Version > 2.0.45 (or whatever the latest at this time), then I see Installing displayed for a second, and that's it. It doesn't display anything else, and I'm pretty sure in the past there was some "Reload required" that I had to click on. In fact if I restart VSC then it does display: Installing (for more than 5 seconds) and then Reload Required. So I'm not sure if this is related to your code or to VSC (1.74.3 (Universal) on Mac) but this worked better in the past than now.

Strange, I even moved the test to another small project, where there's almost no code next to it:

//monkey.jungle:

project.manifest = manifest.xml
project.typecheck = 3
project.optimization = 2z

base.sourcePath = source
base.excludeAnnotations = memory32K


// Test.mc:

import Toybox.Activity;
import Toybox.Lang;
import Toybox.WatchUi;

class Test extends WatchUi.DataField {
    (:memory16K) const FOO = false;
    (:memory32K) const FOO = true;

    (:memory32K)
    var mFoo as Number = 1;

    public function compute(info as Activity.Info) as Void {
        x();
    }

    (:inline)
    function x() as Void {
        var foo;
        if (FOO) {foo = mFoo;}
        else {foo = 0;}
    }
}

Settings (2.0.45): ERROR, ERROR, ERROR, DEFAULT, YES, "", "", "", bin/optimized, check, check, check, Strict, check, uncheck, check, uncheck

Thanks - I found the infinite recursion bug with that test case.

Simple fix. I'll ship it with the next release.

flocsy said:

Stack Overflow again

Ok, so between dead store elimination, and type propagation, I had all the information I needed to compute local variable conflicts. So adding a pass to merge local variables that don't conflict was fairly easy.

The one snag is that it makes such a huge difference to the generated code that a lot of my tests are broken by it (eg an inliner test that says "expect to see this call replaced by something that looks like...).

Anyway, I just need to decide whether to disable it for those tests, or to update the pattern matchers to recognize the transformed code.

But meanwhile, here's the results on a random piece of code I found on the internet.

Before: one (unused) parameter, and 3 locals

    function compute(info as Activity.Info) as Numeric or Duration or String or Null {
        var result;
        if (hasBodyBattery) {
            var iterator = Toybox.SensorHistory.getBodyBatteryHistory({:period => 1, :order => SensorHistory.ORDER_NEWEST_FIRST});
            var sample = iterator.next();
            if (sample != null && (sample as SensorSample).data != null) {
                result = ((sample as SensorSample).data as Number).toNumber();
            } else {
                result = "--";
            }
        } else {
            result = strNotSupported;
        }
        return result;
    }

After: one heavily reused parameter and no locals

    function compute(
        info as Activity.Info
    ) as Numeric or Duration or String or Null {
        if (hasBodyBattery) {
            info /*>iterator<*/ = Toybox.SensorHistory.getBodyBatteryHistory({
                :period => 1,
                :order => 0 as Toybox.SensorHistory.Order,
            });
            info /*>sample<*/ = info /*>iterator<*/.next();
            if (
                info /*>sample<*/ != null &&
                (info /*>sample<*/ as SensorSample).data != null
            ) {
                info /*>result<*/ = (
                    (info /*>sample<*/ as SensorSample).data as Number
                ).toNumber();
            } else {
                info /*>result<*/ = "--";
            }
        } else {
            info /*>result<*/ = strNotSupported;
        }
        return info /*>result<*/;
    }

By getting rid of the locals, this saves 3 stack slots, but also shaves 2 bytes off the code size because the "incsp" instruction at the head of the function is eliminated.

This doesn't address the single-use variable issue, but I'll probably go after that next.