Big update to prettier-extension-monkeyc

I hope you are wrong about the Storage_getValue, because I do the exact opposite, and I did that after comparing the code+data size after each change. So if you're right then it means that things changed in later compilers, which means that once in a while I'll need to reevaluate these "best practice"-s.

Maybe we're using them different amounts of times?

I use setValue ~55 times and save 166 bytes, and I use getValue ~80 times and save 234 bytes.

SirLancelot said:

you can save a ton by wrapping some system calls in MonkeyC functions

There's a bit of subtlety here, and how much you save depends on exactly how you imported Storage.

If you just did "import Toybox.Application;", then a call to Storage.getValue() will first lookup Application (6 bytes) then lookup Storage in Application (another 6 bytes) then lookup getValue in Storage (another 6 bytes).

But if you did "import Toybox.Application.Storage;", then it will lookup Storage directly, saving 6 bytes. So this is both smaller and faster. The plugin will do this for you - so if you're using that, you were already getting the smaller code, regardless of imports.

On the other hand a call to Storage_getValue will first push "self" (2 bytes) and then lookup Storage_getValue in self (6 bytes). So it's just 8 bytes - saving 4 over Storage.getValue with the correct imports.

But this is potentially much slower. If you're in module scope, looking up a symbol in self involves searching the current module, then searching each containing module until you reach the global scope. If you're in a class then its much worse; first it searches the current class, then it searches all parent classes down to and including Object, if those fail (which they will in your case) it then searches the module containing the class, and all containing modules. In your case, when it gets to the global scope it will find the function - but if it didn't it would then search the module scope for each parent class, again, all the way down to Object.

Of course, if you're calling it from a global scope function, self will already be a reference to the global scope, so the lookup will actually be faster than the alternative methods...

SirLancelot said:

automatically remove WatchUi.loadResource calls in system functions that don't need them

I was considering this when I first started writing the optimizer, but at the time a lot of those calls were incorrectly typed. In particular, ToggleMenuItem(Rez.Strings.DarkMode,...) would trigger a warning or error from the type checker. I reported some of them, and I'm pretty sure most were fixed.

But meanwhile, I found a better trick - a specialization of your global function suggestion;

(:typecheck(false))
function loadString(string_id as Symbol) as String {
    return WatchUi.loadResource(Rez.Strings[string_id] as Symbol) as String;
}

Note that garmin's type checker doesn't like the above function, hence the (:typecheck(false)) but the code works.

Now you can say: new WatchUi.ToggleMenuItem(loadString(:DarkMode),...) and the code for the call to loadString is actually shorter than the code for Rez.Strings.DarkMode.

You might think that you can do even better by "import Rez.Strings;" and then just using Strings.DarkMode - but I tried that with the optimizer and it revealed two separate bugs in the monkeyc compiler:

One issue was that Rez.Strings is lazily initialized, and it gets initialized the first time you "mention" Rez. But after "import Rez.Strings" you don't need to mention it (the import doesn't count), so you would have to ensure that *something* mentioned Rez before the first use of a string. The symptom of this issue was that Strings.DarkMode could evaluate to null.

The other issue was related to background resources. If you "import Rez.Strings" then any reference to Strings always references the foreground resources, and will crash if the code is run in the background (similarly if you "import Rez;" any mention of Rez always refers to the foreground resources. So don't import Rez if some of the uses are from background code).

I'll note that I've mentioned this trick before, and flocsy🤠 said he couldn't get a win out of it. We never established exactly why not, but it probably depends on use. In my case, I have global arrays holding string ids, and 

var arr = [:a, :b, :c];

produces much less code than

var arr = [Rez.Strings.a, Rez.Strings.b, Rez.Strings.c]

Yes, I use them only a handful times

I use SDK 6.4.2, prettier MC: v2.0.73

I had this code:

// MyApp.mc:
import Toybox.Application;
class MyApp extends Application.AppBase {...}

// Config.mc:
import Toybox.Application;
import Toybox.Application.Properties;
(:no_ciq_2_4_0, :inline)
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
    MyApp.setProperty(key, val);
}

When I change it to :no_inline to test if it's smaller, then I get these errors:
ERROR: fr235: bin/optimized/group064-release/source/source/Config.mc:11,2: Cannot find symbol ':setProperty' on class definition '$.MyApp'.
ERROR: fr235: bin/optimized/group064-release/source/source/Config.mc:11,2: Cannot determine type for method invocation.

So I change the line with setProperty to:

(MyApp as Application.AppBase).setProperty(key, val);

Which seems to work, but it gives me these optimization errors:

ERROR> fr235: source/Config.mc:12:6: The type Class<MyApp> cannot be converted to Toybox.Application.AppBase because they have nothing in common

flocsy🤠 said:

When I change it to :no_inline to test if it's smaller, then I get these errors:

This is a change that happened in the 6.2.0 sdk. They tightened up type checking to prevent calling non-static methods statically.

But it's subtle. For a given non-static method m in class C: 

• C.m() in a non-static method of C translates into a non-static call to m (meaning the caller's "self" is passed as m's self).
• C.m() anywhere else translates into a static call to m (meaning that C itself is passed as m's self).

Since the type checker now (post 6.2.0) rejects attempts to call m statically, this means that C.m() will generate a type checker error *unless you call it from within a non-static method of C*.

So I assume what's going on is that you're calling setConfig from within a non-static method of MyApp. If it gets inlined, the compiler is happy, but if the non-inlined version of setConfig is still in the code, you get the error, because you're trying to call setProperty (a non-static method of AppBase) statically.

As you noted, and somewhat bizarrely, you can fix the error via a cast. The compiler doesn't complain about (C as C).m() no matter where you call it. The reason is that "X as C" means "Treat X is an instance of C" - so now the type checker sees a call to m on an instance of C, which is ok. Note that there is no way in monkey C to cast something to "class C" - you can only cast to "instance of C".

flocsy🤠 said:

ERROR> fr235: source/Config.mc:12:6: The type Class<MyApp> cannot be converted to Toybox.Application.AppBase because they have nothing in common

So I thought at first that this was a bug. It seems that they obviously do have something in common, since MyApp extends AppBase.

But in fact, the error is correct. The class MyApp has nothing in common with an instance of AppBase, and so the cast should not be allowed.

As it turns out, for now, the runtime still behaves as expected (although I've noticed a few places where the runtime behavior was tightened up in other static-vs-non-static cases), so if you must do this, you can do ((MyApp as Object?) as Application.AppBase) or even ((MyApp as Object?) as MyApp), and keep both the type checker and my optimizer happy.

But if you only call this method from an instance method of MyApp, you're going to be better off (in terms of code size) replacing your call to setConfig with a call to self.setProperty (or just setProperty). Hmm... except I *think* my optimizer may make that transformation after inlining anyway (if it doesn't, it should).

markw65 said:

((MyApp as Object?) as MyApp)

Cool trick! This works indeed, and it enables me to use again my older code (that decreases code size) that stopped to work with one of the recent sdks.

FYI that my extension doesn't work with the 7.x beta. They've (finally) added support for "tuple" types, and my parser currently rejects the syntax - but it's an easy fix.

"tuple" types are arrays that have a fixed number of elements, with a particular type at each index - eg:

var myTuple = ["Hello", 42, true] as [String, Number, Boolean];

Currently, you would have to say:

var myTuple = ["Hello", 42, true] as Array<String or Number or Boolean>;

With the result that myTuple[0] has type String or Number or Boolean, rather than just String.

The reason it breaks the optimizer even if you don't use this syntax (which, of course you don't yet) is that they use it for things like:

function getCurrentView() as [Null or $.Toybox.WatchUi.View, Null or $.Toybox.WatchUi.InputDelegates] {}

So the optimizer falls over trying to parse api.mir. Note this means that getCurrentView is now properly typed - it returns an array of two elements, the first is a View (or null) and the second is an InputDelegate (or null). With earlier sdk's the return type was an array of arbitrary length, and each element was either a View, an InputDelegate or Null.

I hope to put out a fix shortly.