Big update to prettier-extension-monkeyc

flocsy🤠 said:

Both give the same warning in the editor

This was just an oversight. When a const doesn't have a declared type (as this one doesn't), I would deduce the type from the initializer, but only for certain simple expressions - and arrays were not included.

note that

const FOO as Number = 42;

has a declared type, while

const FOO = BAR as Number;

does not.

In any case, I've fixed it to always deduce the type from the initializer, and the problem is gone.

Btw, as with the other issues, this just makes the optimizer more conservative; it knows less about the type than it should, so may miss some optimization opportunities, but won't make incorrect decisions (at least not without some other bug).

Strangely, Garmin's type checker doesn't like your second declaration for some reason (but was happy with the first):

ERROR: fenix5xplus: source/C.mc:21,6: Cannot assign value '$.Toybox.Lang.Array<$.Toybox.Lang.Dictionary<$.Toybox.Lang.Boolean or $.Toybox.Lang.Char or $.Toybox.Lang.Double or $.Toybox.Lang.Float or $.Toybox.Lang.Long or $.Toybox.Lang.Number or $.Toybox.Lang.String,Null or $.Toybox.Application.PropertyKeyType or $.Toybox.Lang.Array<$.Toybox.Application.PropertyValueType> or $.Toybox.Lang.Dictionary<$.Toybox.Application.PropertyKeyType,$.Toybox.Application.PropertyValueType> or $.Toybox.WatchUi.BitmapResource>>' to member ':DEFAULT_USERS_PROPERTIES2'.

If I give it a declared type (matching the cast) then Garmin's type checker is happy.

flocsy🤠 said:

Unfortunately I don't see any change at all

I wasn't really expecting code size differences... out of 150 or so projects I compile, only 4 changed size.

I fixed one issue that could cause the type checker to lose track of the type of a member expression (a.b.c etc). And when it loses track, it assumes "Any" for the type, so again, it just makes it more conservative than it needs to be. But can also result in strange warnings. Obviously, you've found another way for this to happen...

The reason you see the strange warnings about seemingly unrelated classes, is that when its looking up remove in b, it tries to find every possible definition of remove that's compatible with the known type of b. If the "known" type of b is "Any" its going to find every method/constant/enum/class/module in the program (including the Toybox) thats named remove. It *has* to do that to ensure it's conservative about any optimizations it performs.

So the warnings are understandable, and not an indication of an error per se (or rather, the error is that it failed to determine the type of b when it really should have known it).

I'm still inclined to think this is a temporary glitch, where right after inlining the function, the inlined parts have no type information (because type propagation hasn't run on them yet).

What I try to do is skip over the just-inlined body, process the rest of the caller, and then re-run type propagation on the caller, and re-optimize it (possibly doing more inlining along the way). But in your case, I think something causes it to re-opimize the inlined code before type propagation has run again. So it generates the warnings, but then goes back and optimizes again, this time with the correct type information. But so far, I can't figure out how its happening.

ok, so even after your fix it's better to have the ugly:
const DEFAULT_USERS_PROPERTIES as PropertyDictionaryArray = [] as PropertyDictionaryArray;

> Strangely, Garmin's type checker doesn't like your second declaration for some reason (but was happy with the first):

I guess it's a bug that they only know it's an Array in certain context, but not sure.

would it help if I send you the logs from 2.0.42 in email?

Can you add a warning when I accidentally inline a "big" function more than once? At least for the huge ones that are obviously a mistake :) 

It happened again. I hade the (:inline) there for a while, and I think it was only used once when I added it, but then later I used it somewhere else, and didn't notice for a few months, and now that I start to get Out Of Memory Errors in ERA I noticed this, but I believe the optimizer could warn about this.

Or I can again drop in the old idea to have at least something like :inline_once.
Or maybe it would make more sense (even though it would not be backward compatible) to make :inline only work if it's only used once and add a new annotation: :inline_any or something like that that will inline multiple times (this would be used for things like 2 simple functions that either call App.setProperty or Properties.setValue for example)

flocsy🤠 said:

ok, so even after your fix it's better to have the ugly

For my optimizer? No. After the fix it will make no difference. The issue was that I simply punted on "complex" looking initializers because the code to get the type of a constant could be called many times; but I've fixed that by caching the result.

On the other hand, it looks like you *have* to add the declaration in the second case to get Garmin's type checker to accept it.

So the only reasons I see for declaring the type of a constant would be if Garmin requires it, or if you want the type to be wider than what would be deduced (eg Array<Number or String> when Array<Number> would be deduced). But I can't really think of a good reason for wanting to do that right now.

Btw, my type checker should deduce the type of a literal array. So eg

  var x as Array<Number> = [1,2,3];

works. But Garmin's type checker will reject that...

flocsy🤠 said:

Or I can again drop in the old idea to have at least something like :inline_once

I guess I need a better definition of what that means.

If foo is inline_once, and its only used once from bar, but bar is inline, do I inline foo into bar and then prevent bar from being inlined at all; or do I not inline foo into bar and then inline bar wherever it likes?

But also, currently all the inlining decisions are made on the fly. I find a call, see that inlining was requested, and try to inline it - which may fail for any number of reasons, and then continue on to the next call... 

In the "inline_once" world, I'd have to find every call to an inline_once function, determine which ones were only called once, determine which of those I could actually inline, then for each one that I did inline determine whether it changed the number of calls to any of the other inline_once functions (which it might, because an if-block might be eliminated after inlining), and continue.

Its doable, and I'm heading towards a more global optimization framework, so doing it may become a lot easier, but for now its not trivial.

Warning about inlining "big" functions more than once is probably more doable - I just need to add some kind of size calculation (which is going to be somewhat guesswork).

OK, let's start from the end, because that's more or less what I was hoping for. I knew it will be some guesswork, that's why I asked for "big" because I think that's something that at least in the "average" case you'll easily spot. The edge cases where the only real way to know if inlining is better or not (for short functions) is to compare it with and without (and here I'm not even considering the prettier optimizer that makes the guesswork even harder). You could even do a "lower" level warning when it's maybe not good to inline it more than once (because the guesstimates size is too close to be sure) but there's a "higher" level warning when it's clear that it shouldn't be inlined more than once.

I think the above detailed approach is a good one, because it probably will catch similar mistakes to mine for others.

I can even think of another approach. Do the above and keep :inline work that way, but add 2 new annotations for the new functionality: :inline_once, :inline_multi. When inline_multi is used then you can skip all the above (no need to warn) but when inline_once is used then instead of trying to figure out every time if there will be another inlining of the same function later, you could have a counter (just a map is enough) increased every time it's inlined, and if you see that you're inlining now a function that was already inlined before then you warn. In other words you never warn for the 1st inline, but you always warn on 2nd,... Actually instead of a counter put the location of the 1st inline to the map and then you can even give a warning:
warn: in C.mc:33 you're trying to inline_once foo, but it was already inlined in B.mc:53

To me this would be perfect. I would take the time to change every :inline to either :inline_once or :inline_multi. Maybe others don't care and they could keep the :inline.