Big update to prettier-extension-monkeyc

flocsy🤠 said:

If my original code was incorrect, then report it at the beginning

That would be ideal, I agree. But the fact is that it will miss things.

For example the result of a function may be Number or Null; but after inlining at a particular point in the program, the result is known to be 42. So at that point the optimizer can issue a "can't be null" warning, where it couldn't issue it earlier.

flocsy🤠 said:

Not only I don't want to get multiple errors of the same thing

I agree, and I go to great lengths to avoid it. There's a lot of code I could just throw away if I could just issue warnings at the start (or just at the end).

flocsy🤠 said:

Here it's very clear that it's the same line, same column, same parameter of the same function

But often there's more than one warning that applies at a particular point. I could make my diagnostic system even more sophisticated; in this case it would need to be aware that both warning.s are reporting type mismatches against the same expression. But right now, it relies on the message itself. And in this case it looks like it changed because it figured out that something couldn't actually be null.

flocsy🤠 said:

I'm ok getting errors about my code, but getting error about code that the optimizer transformed?

I agree. It shouldn't warn about things introduced by the optimizer (and in fact, the optimizer shouldn't introduce things that it would warn about). What I'm talking about here are things that were true but not obvious in your original code, but became clear after optimization. This often happens after eliminating one branch of an if-else, for example, or after inlining a function with constant arguments.

flocsy🤠 said:

I think I found a bug. I get this error message

This is a case where Garmin's type checker isn't as good at following types as mine. If you give mine the "optimized" version as input, it should correctly deduce that pmcr_alertView_0 is non-null at the showAlert call, even though alertView was tested; because at the point of the test, both variables held the same value. Anything you can deduce by testing pmcr_alertView, you can also deduce by testing alertView.

So the code isn't wrong, it's just not something that Garmin's type checker can handle.

But this is definitely sub-optimal output from the optimizer. There doesn't seem to be any point introducing the alternate variable in the first place [edit: it introduced it when it inlined the function, and presumably it couldn't get rid of it because the original alertView variable is used later in the function] - and even if it did, I would have expected the variable minimizer to get rid of it.

Yes, but even more interesting is that I tried for about an hour if not more to create some code I could post to reproduce it, and in each and every one of them the optimized code was the correct, when it uses pmcr_alertView_0. Only in my original code it didn't. 

No it actually is during the optimization, not the build.

One more "bug": because of this bug of the SDK 8 compiler: https://forums.garmin.com/developer/connect-iq/i/bug-reports/bug-nested-ternary-operator-returns-incorrect-value-in-sdk-8-0-0 even when I fixed my original code by adding the parentheses, the generated code still produces the wrong values, because it removed the parentheses.

I recommend not to remove parentheseses. It shouldn't cost anything to leave them, they might not be needed, or you (just as me until yesterday) might think they're not needed, but who knows... By keeping all of them you can prevent future similar issues (and if SDK 9 will introduce another bug that will break some apps, then at least when the developer understand that the fix is to add parentheses, then the optimized code will also be fixed automatically.

update: ok, so obviously when you remove code, then you'll sometimes remove parenteses as well, but that's fine. However in this case the original code was:

var labelFontHeight = showLabel ? getFontHeightOr0(dc, labelFont) : (displayGauge ? GAUGE_TRIANGLE_HEIGHT : 0);

    var labelFontHeight = showLabel
      ? labelFont == null
        ? pre_0
        : dc.getFontHeight(labelFont as Graphics.FontDefinition)
      : displayGauge
      ? pre_5
      : pre_0;

> I recommend not to remove parentheseses. It shouldn't cost anything to leave them

That's not how it works.

The parser produces an AST. There are no parentheses in the AST.

(a + b) + c =>      +

                   / \

                  +   c

                 / \

                a   b

While

a + (b + c) =>      +

                   / \

                  a   +   

                     / \

                    b   c

The formatter produces code that is supposed to be equivalent to the AST. I didn't write the formatter. Or at least, I didn't write most of it - I re-used a javascript formatter, with a few mods to make it work with monkey-c.

I can certainly force parentheses in certain contexts (and I had to in a few places where js and mc differ on precedence, for example), but I can't preserve them.

For now I'm going to assume this is a bug in the sdk-8-beta that won't make it into the real world. Releasing it with this change would be insane (but then again...)

If they do release it, I'll have to update the parser, as well as the formatter, since it's basically their parser that has changed, and mine will have to match it.

Do you have Config.I_DM_OFF defined in all 3 of the source folders? Can it be defined in source?

Yes, I have three enum Item and each of them happens to have I_DM_OFF. It's the list of menu items, and the lists are different for the three builds.

So they could probably be moved to source, together with some annotations, but that would be a hack. Logically and from a maintainability pov, they belong where they are. Also, another similar case happens with another such list, which is only part of two of the three builds.