Big update to prettier-extension-monkeyc

flocsy said:

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

I don't know - but if it ever did that, it was a bug. What it's doing now is correct.

function foo() as String {
  System.println("foo");
  return "bar";
}

(:inline)
function nothing(msg as String) as Void {
}

function something() {
  nothing(foo());
}

Think about it. If that program doesn't print "foo" on the console, there's something wrong.

The optimizer *does* know that certain system functions don't have side effects, and so it will remove those - and I intend to keep expanding the list. But anything I've not explicitly told it about, it assumes it can't drop the call.

flocsy said:

UPDATE: renaming makeWebRequest to makeWebRequest2 fixes it

This isn't actually the optimizer, it's the parser/formatter. If you just format your test case, it will break it in exactly the same way. I think the parser must be caching something from the first makeWebRequest and using a shared copy in the second. Just not sure exactly what yet...

Actually, it *is* the formatter, but I just misread the original code. It looks like the formatter is doing two things wrong. First, its added a ";" after the second Method. That's just wrong, but I see where it's coming from. The other is that it's adding parentheses around the whole thing, which Garmin's parser doesn't like.

Both of these happen deep inside some Prettier code for formatting javascript that I hijacked - which makes it harder to tweak. But I think I can fix them without too much trouble...

flocsy said:

UPDATE: renaming makeWebRequest to makeWebRequest2 fixes it

So I found the problem, but the *real* issue is that you didn't write what you thought you did.

This is what you wrote (I've changed the formatting to match what I think you meant).

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

ie what you want is that responseCallback is a method with one of two possible signatures. But thats not how it's parsed.

Its parsed as (note that the parentheses on the second line aren't valid syntax - I'm putting them in to show how things are grouped):

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)

ie its a method which returns Void or a Method. And given this, the formatter gets confused (so yes, you found a real bug). But given what you *intended* it works fine. ie if you write it like this (and this time the parentheses are valid):

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)

It means what you want it to mean, and it doesn't break the formatter...

[EDIT: Sorry, I'm wrong. If you replace Void with Null (or any other type), then what I wrote is correct; but for Void, Garmin's compiler parses it the way you wanted it. So I need to change the parser to match the way that Garmin's parser works AND fix the formatter to output valid code when you have a method that returns a method)]

When you inline functions that have :typecheck, then you should inline the annotation as well. I think there was some talk about it, now I have a specific problem with :typecheck(disableBackgroundCheck:

(:foreground, :background)
class MyApp extends Application.AppBase {
    (:background, :foreground, :background_app)
    var isForeground as Boolean;

    (:foreground) private var mField as MyField?;

    (:background, :foreground)
    function initialize() {
        var isForeground = !isRunningInBackground();
        self.isForeground = isForeground;
        if (isForeground) {
            initForeground();
        }
    }

    (:foreground, :inline, :typecheck(disableBackgroundCheck))
    hidden function initForeground() as Void {
        mField = new MyField();
    }
}

This compiles and runs without the optimizer, but fails when compiled with the optimizer:

ERROR: fr255: bin/optimized/group002-debug/source/source/MyApp.mc:48,8: Value 'ExtHRMField' not available in all function scopes.
ERROR: fr255: bin/optimized/group002-debug/source/source/MyApp.mc:48,8: Value 'initialize' not available in all function scopes.

line 48 in the generated code is: mField = new MyField();

The "way I want it" is copy & pasted from the documentation of Communications.makeJsonRequest and Communications.makeWebRequest ;) the only thing I changed IMHO was to remove "Lang."

flocsy said:

The "way I want it" is copy & pasted from the documentation

Right - I already acknowledged the mistake. And its due to Garmin's bizarre parsing rules.

• "Method() as Null or String" is a method that either returns Null or it returns String
• "Method() as Null or Method() as Whatever" is a method that either returns Null, or it returns a Method that returns Whatever
• "(Method() as Null) or Method() as Whatever" is either a method returning null, or its a method returning Whatever.

but

• "Method() as Void or String" is either a method returning Void, or its a String
• "Method() as Void or Method() as Whatever" is a either method that returns Void, or its a Method that returns Whatever"

(note that Void is the exception - any type other than Void parses the same as Null does)

I guess I see how this happened. It makes no sense to return "Void or String", so they disallow including Void in a union of types. Sure - I get it. But most languages would parse consistently, and then report the error, rather than avoiding the error at parse time. Because otherwise you end up with stupid parsing rules that give completely different meanings to essentially the same syntax...

Especially given that Void methods in monkeyc actually return null, just like Null methods do...

flocsy said:

When you inline functions that have :typecheck, then you should inline the annotation as well

I do propagate :typecheck(false), but not other variants - mainly because the optimizer only understands :typecheck(false). But yes, I agree it needs to copy the others too.

UPDATE:

Actually, I remember why I did that. If you put more than one :typecheck directive on a function, they're all ignored. Also, you can't (as far as I can see) combine "false" with "disableBackgroundCheck", even though you can combine disableBackgroundCheck and disableGlanceCheck by putting them in square brackets.

So I'll change it so that any :typecheck annotations from the inlined function get propagated to the caller, unless the caller already has annotations. And then I'll do nothing.

flocsy said:

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

So I got to the bottom of this. It first tries to inline timeF (twice) as part of the parameter to l(...), and (correctly) fails to do so, so generates the diagnostic and puts it in a table for later. Then it inlines l, and as part of that it puts a copy of the parameter in a variable initializer, something like:

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

and now, since msg is unused, it drops the side-effect free parts of that, leaving both calls to timeF as statements. So now they do get inlined. But it lost track of the fact that they're the same calls that failed to inline earlier, and so the original diagnostics remain. I've fixed the code so that it does keep track of them, and now the two diagnostics corresponding to those two calls are gone.

But the third one comes from logR. In logR, the result is used, so it never ends up in statement context, and it never gets inlined. So the warning (correctly) remains.

But finally, at the end, it notices that nothing can call logR, so it removes that entirely - and now there are no calls left to timeF, but the optimizer only removes unused calls once (ie it removes unused calls, and doesn't go back to see whether thats resulted in anything else becoming unused).

At some point I'm going to change the dead code removal to start from the entry points, and figure out everything thats reachable - rather than just looking at all the code, and seeing what isn't referenced from anywhere. Once I do that, problems like this will go away. But there's a few more things I want to do first.

But the real issue is that when logR *is* used, timeF won't be inlined there. So the diagnostic (for that one call) is correct.

I've also added lots more to the list of side-effect free functions, and now all the "junk" code is gone...

v2.0.48 is out.

This fixes a few minor bugs and missed optimization opportunities.

It also adds a new optimizer pass to do limited copy propagation. If an expression is assigned to a local, and then the local's value is only used once, and the expression's value hasn't changed between the assignment, and the use, the assignment will be deleted, and the original expression will be substituted for the use of the variable.

function foo(a, b) {
  var x = a + b;
  ... // nothing modifies a or b, or uses x
  return x;
}

becomes

function foo(a, b) {
  ... // nothing modifies a or b, or uses x
  return a + b;
}