Big update to prettier-extension-monkeyc

flocsy🤠 said:

Why this can't be inlined

Actually, I was a bit surprised.

It's because it uses s.d.x twice, so it (thinks) it needs to add a local variable; and it can't do that in the middle of an expression. If you explicitly move s.d.x to a local, and pass *that* I think it should work.

The code that makes that decision was written before the pre pass existed, and it was reasonable then. s.d.x takes 14 bytes per use, so the inlined body would be huge. But with pre, thats no longer a consideration (the local will be added later, but it will be added just the same). I'll update the heuristics...

I have a new idea: we already mentioned inlining parent classes could be done:

(:inline) class A {...}
class B extends A {...}

There's another pattern (though probably less used), when there are different delegates/implementations for something and is used in this way:

class MyDelegate {
  private var priv as Number;
  protected var prot as Float;
  public var pub as Long;
  function onStart(state as Dictionary?) as Void {doSomething()}
  function onStop(state as Dictionary?) as Void {doElse()}
}

class MyApp extends AppBase {

  private var delegate = new MyDelegate();

  function onStart(state as Dictionary?) as Void {
    delegate.onStart(state);
  }
  function onStop(state as Dictionary?) as Void {
    delegate.onStop(state);
  }
}

In this case either adding (:inline) to class MyDelegate or to var delegate could tell the optimizer to inline everything in MyDelegate into MyApp. Obviously there will be some constrains, like for inlining functions.

found a nice bug. I changed a small function to be :inline. I got 3 warnings for all the 3 places it was used for: While inlining timeFormat: This function can only be inlined in statement, assignment, if or return contexts. Until now everything seems OK. And I assumed that when I see this warning then the function IS NOT INLINED. However then I noticed that when I have the :inline then the code size grew by 30 bytes. So I looked into the generated code. In 1 place it is not inlined, and the code with and without :inline is the same as expected. In the other 2 places however it's some hybrid mixture of half inlining:

my code:

import Toybox.Lang;
import Toybox.System;
import Toybox.Time;

const DO_LOG = false;

(:debug, :background, :inline)
function l(msg as String) as Void {
    if (DO_LOG) {
        logR(msg);
    }
}

(:foreground, :background)
function logR(msg as String) as Void {
    // System.println(Time.now().value() + " " + msg);
    System.println(timeF(Time.now()) + ' ' + msg);
}

(:foreground, :background, :inline)
function timeF(moment as Moment) as String {
    var time = Time.Gregorian.info(moment as Moment, Time.FORMAT_SHORT);
    return "" + time.hour + ':' + time.min + ':' + time.sec;
}

class A {
    (:background, :foreground)
    public function initialize() {
        if (true) {
            if (MyApp.isRunningInBackground()) {
                l("fetchStatus");
            } else {
                var lastTime = Background.getTemporalEventRegisteredTime() as Moment?;
                var nextTime = lastTime != null ? lastTime.add(new Time.Duration(300)) : Time.now();
                l("updateStatus: " + (lastTime == null ? null : timeF(lastTime)) + ", " + timeF(nextTime));
                Background.registerForTemporalEvent(nextTime);
            }
        }
    }
}

it got optimized into:

import Toybox.Lang;
import Toybox.System;
import Toybox.Time;

(:foreground,:background,:inline)
function timeF(moment as Moment) as String {
  moment /*>time<*/ = Time.Gregorian.info(
    moment as Moment,
    0 as Toybox.Time.DateFormat
  );
  return (
    "" +
    moment /*>time<*/.hour +
    ':' +
    moment /*>time<*/.min +
    ':' +
    moment /*>time<*/.sec
  );
}

class A {
  (:background,:foreground)
  public function initialize() {
    if (MyApp.isRunningInBackground()) {
    } else {
      var lastTime = Background.getTemporalEventRegisteredTime() as Moment?;
      var nextTime =
        lastTime != null ? lastTime.add(new Time.Duration(300)) : Time.now();
      if (lastTime == null) {
      } else {
        Time.Gregorian.info(lastTime as Moment, 0 as Toybox.Time.DateFormat);
      }
      Time.Gregorian.info(nextTime as Moment, 0 as Toybox.Time.DateFormat);
      Background.registerForTemporalEvent(nextTime);
    }
  }
}

1. t didn't remove timeF even though it is not called anywhere.

2. I'm not even sure what it was trying to do that left it with this:

      if (lastTime == null) {
      } else {
        Time.Gregorian.info(lastTime as Moment, 0 as Toybox.Time.DateFormat);
      }
      Time.Gregorian.info(nextTime as Moment, 0 as Toybox.Time.DateFormat);

This is all wrong:

A. since it warned that timeF can't be inlined it should not have tried to inline it. Just leave the calls as they were in the original code

B. what is this if/else with emtpy if block? I guess it's some way it tried to optimize the trinary operator, but how it got to an empty block? If shouldn't it be: if (lastTime != null) {Time....}?

C. it inlined timeF's 1st part, but without assigning the value to any variable or passing it to anything... very strange.

D. the return part of timeF disappeared

I think you should not optimize code inside functions that are annotated with (:test)

I have some problem, that seems to be another bug in the optimizer (will report soon, if indeed), and to check it I made a test and wanted to run the test with the optimizer. So I added this to launch.json:

{
    "type": "omonkeyc",
    "request": "launch",
    "name": "Run Optimized Tests",
    "stopAtLaunch": false,
    "runTests": true,
    "device": "${command:GetTargetDevice}",
    "typeCheckLevel": "Strict",
    "releaseBuild": false
}

And ran it. And then I got lots of errors from my real tests. Here's a minimal version to demonstrate the problem:

import Toybox.Lang;
import Toybox.Test;
import Toybox.Application;

(:inline)
function getBool1(key as PropertyKeyType, defaultValue as Boolean) as Boolean {
    return toBool1(getConfig(key), defaultValue);
}
(:inline)
function toBool1(value as PropertyValueType?, defaultValue as Boolean) as Boolean {
    return value instanceof Lang.Boolean
        ? value
        : (value != null && value has :toNumber ? value.toNumber() != 0 : defaultValue);
}
(:inline)
function getBool2(key as PropertyKeyType, defaultValue as Boolean) as Boolean {
    return toBool2(getConfig(key), defaultValue);
}
(:inline)
function toBool2(value as PropertyValueType?, defaultValue as Boolean) as Boolean {
    if (value instanceof Lang.Boolean) {
        return value;
    }
    if (value != null && value has :toNumber) {
        return value.toNumber() != 0;
    }
    return defaultValue;
}

(:test)
function toBool1_null(logger as Logger) as Boolean {
    return toBool1(null, true);
}
(:test)
function toBool2_null(logger as Logger) as Boolean {
    return toBool2(null, true);
}

Without the optimizer both tests pass, but with the optimizer both fail to compile. The optimized code:

import Toybox.Lang;
import Toybox.Test;
import Toybox.Application;

(:test)
function toBool1_null(logger as Logger) as Boolean {
  return null instanceof Lang.Boolean ? null : true;
}
(:test)
function toBool2_null(logger as Logger) as Boolean {
  if (null instanceof Lang.Boolean) {
    return null;
  }
  return true;
}

optimizer:
ERROR> fr255: Test.mc:32:5: Expected $.toBool1_null to return Boolean but got Null or True
ERROR> fr255: Test.mc:36:5: Expected $.toBool2_null to return Boolean but got Null
inlined from $.toBool2 in Test.mc:22:9

compiler:
ERROR: fr255: bin/optimized/group002-debug/source/source/test/Test.mc:7,2: Type 'Null' is not an instance of '$.Toybox.Lang.Boolean'.
ERROR: fr255: bin/optimized/group002-debug/source/source/test/Test.mc:7,2: Object of type 'PolyType<Null or $.Toybox.Lang.Boolean>' does not match return type '$.Toybox.Lang.Boolean'.
ERROR: fr255: bin/optimized/group002-debug/source/source/test/Test.mc:11,2: Type 'Null' is not an instance of '$.Toybox.Lang.Boolean'.
ERROR: fr255: bin/optimized/group002-debug/source/source/test/Test.mc:12,4: Object of type 'Null' does not match return type '$.Toybox.Lang.Boolean'.

flocsy🤠 said:

This is all wrong

Possibly the warning is wrong... the way I do inline warnings is that it tries first in "expression" context, and if that fails it puts the diagnostic in a list, and continues. It will then try to inline it in any other contexts that apply (eg assignment, return, if or statement), and if any of those succeed, it clears the original warning. But its possible that some other transformation between the initial warning, and the success changes the code enough that when it tries to clear the warning, it fails.

The rest of it looks like as intended behavior - although definitely sub-optimal.

It looks like, in the configuration you're building, "l" does nothing).

But you passed it

"updateStatus: " + (lastTime == null ? null : timeF(lastTime)) + ", " + timeF(nextTime)

as a parameter. So the inlined body of l looks like:

{
  var msg = "updateStatus: " + (lastTime == null ? null : timeF(lastTime)) + ", " + timeF(nextTime);
}

And now it tries to optimize that. Both timeF's got inlined so now we have some conditional code, and a couple of calls to Time.Gregorian.info. And here's where things are sub-optimal. There's nothing to tell the optimizer that Time.Gregorian.info has no side effects (I actually am building a table of system functions, and what side effects they have, but its *far* from complete, and doesn't currently include anything from Time.*).

So now it can either give up, or it can start dropping unused things. And it does the latter. It knows there's an unconditional call to Time.Gregorian.info, but there's also a conditional one.

Unfortunately, putting (lastTime == null ? null : Time.Gregorian.info(lastTime as Moment, 0 as Toybox.Time.DateFormat) as a top level statement in MonkeyC is an error; but (since it doesn't know its side effect free) it *must* only call info when lastTime== null.

So yes, it could be slightly smarter; it could say "if (lastTime != null) {Time.Gregorian.info(lastTime as Moment, 0 as Toybox.Time.DateFormat);}"

So thats it... it's not a bug (other than maybe incorrectly reporting that it failed to inline). Its just that it doesn't know that Gregorian.info has no side effects.

flocsy🤠 said:

I think you should not optimize code inside functions that are annotated with (:test)

except thats how I test the optimizer. 

return null instanceof Lang.Boolean ? null : true;

Ok, so this is just (another) case of Garmin's compiler refusing to compile something because the result is a compile time constant; which is really frustrating, but I already have to deal with a lot of this kind of thing.

That's just one more I need to deal with...

Meanwhile, if you want to run the un-optimized tests, you can always just not run the optimizer...

import Toybox.Communications;
import Toybox.Lang;

class C {
    function send(params as Dictionary) as Void {
        var url = "https://example.com/foo";               // set the url
        var options = {                                             // set the options
            :method => Communications.HTTP_REQUEST_METHOD_GET,     // set HTTP method
        };
        makeWebRequest(url, params, options, method(:onReceive));
    }
    (:no_api1_3, :inline)
    function makeWebRequest(url as String, params as Dictionary, options as { "Content-Type" as Communications.HttpRequestContentType } or Null, responseCallback as Method(responseCode as Number, data as Dictionary or String or Null) as Void) as Void {
        Communications.makeJsonRequest(url, params, options, method(:onReceive));
    }
    (:api1_3, :inline)
    function makeWebRequest(url as String, params as Dictionary?, options as { :method as Communications.HttpRequestMethod, :headers as Dictionary, :responseType as Communications.HttpResponseContentType, :context as Object?, :maxBandwidth as Number, :fileDownloadProgressCallback as Method(totalBytesTransferred as Number, fileSize as Number?) as Void } or Null, responseCallback as Method(responseCode as Number, data as Dictionary or String or Null) as Void or Method(responseCode as Number, data as Dictionary or String or Null, context as Object) as Void) as Void {
        Communications.makeWebRequest(url, params, options, method(:onReceive));
    }
    public function onReceive(responseCode as Number, data as Dictionary or String or Null) as Void {
    }
}

will generate:

import Toybox.Communications;
import Toybox.Lang;

class C {
  (:api1_3,:inline)
  function makeWebRequest(
    url as String,
    params as Dictionary?,
    options as
      {
        :method as Communications.HttpRequestMethod,
        :headers as Dictionary,
        :responseType as Communications.HttpResponseContentType,
        :context as Object?,
        :maxBandwidth as Number,
        :fileDownloadProgressCallback as
          (Method
            (totalBytesTransferred as Number, fileSize as Number?) as Void
          ),
      }?,
    responseCallback as
      (Method
        (
          responseCode as Number,
          data as Dictionary or String or Null
        ) as Void or
          (Method
          (
            responseCode as Number,
            data as Dictionary or String or Null,
            context as Object
          ) as Void
        );
      )
  ) as Void {
    Communications.makeWebRequest(url, params, options, method(:onReceive));
  }
  public function onReceive(
    responseCode as Number,
    data as Dictionary or String or Null
  ) as Void {}
}

ERROR: fenix3: bin/optimized/group004-debug/source/source/test/Test.mc:26,18: missing ')' at 'or'
ERROR: fenix3: bin/optimized/group004-debug/source/source/test/Test.mc:33,9: extraneous input ';' expecting {')', ','}
ERROR: fenix3: bin/optimized/group004-debug/source/source/test/Test.mc:35,2: mismatched input ')' expecting {'{', ';'}
ERROR: fenix3: bin/optimized/group004-debug/source/source/test/Test.mc:36,34: mismatched input 'url' expecting {')', ':'}
ERROR: fenix3: bin/optimized/group004-debug/source/source/test/Test.mc:36,74: mismatched input ')' expecting {'class', 'public', 'private', 'protected', 'hidden', 'static', 'const', 'native', 'var', 'enum', 'function'}
ERROR: fenix3: bin/optimized/group004-debug/source/source/test/Test.mc:42: extraneous input '}' expecting {<EOF>, 'class', 'module', 'using', 'import', 'alias', 'typedef', 'public', 'const', 'native', 'var', 'enum', 'function', '('}

UPDATE: renaming makeWebRequest to makeWebRequest2 fixes it.

until now I THINK (I'm pretty sure, I saw it multiple times. There might have been a bug that I didn't notice though) any time I compiled with DO_LOG = false all the

l(longCode + that(foo) + " ends up" + being("thrown away"));

were replaced with "{}". So what's different here?

It's hard to find these, the only way would be to read all the code in all the files. (If you would add a comment of the inlined function's name: /*>l<*/ then it would be easier to find if any code slipped into the generated code)

Obviously :) I never thought running them optimized until today, but now there was something that was strange in the real generated code and somehow I ended up trying to reproduce with some small example (still couldn't)