Big update to prettier-extension-monkeyc

I forgot to relate to the nested inlining:

first thought:

I think the only reasonable thing to do here is to count the number of times it is inlined because of the original code. Or in other words: remove the irrelevant code (excludeAnnotations, constant conditions) and then count how many times my code (as is) calls the inlined function.

I think this should be sufficient, relatively easy to implement, and any other solution is more complex and I don't see how they would make sense (but I bet you'll have examples

When a developer writes code like:

(:inline) function f() {
  ...
}

then he considers this as one building block of the whole program. When he looks at it (at the "..." part) then he knows what it does. And then he decides to call it from one or more parts of the code. For the purpose of the counting of how many times some function is inlined I would consider only this: how many times he explicitly calls it from his code.

So if there is:
(:inline_multi) function f() {
   g();
}

and this is the only place g() is called, then that's OK IMHO to call f() any number of times, and that shouldn't count as multiple inlining of g() (and it doesn't even matter if f doesn't have any annotation or it has inline_once or inline_multi IMHO)

However on 2nd thought.... (15 minutes later)

If we have:

------ B.mc ----------
// 2020-01-01: this is a big method, it's inlined only because depending on foo we have different implementation, but it's OK, because it's only used in 1 place in C.mc:23
(:inline_once, :foo) function big_method(str) {...}
(:inline_once, :no_foo) function big_method(str) {...}

------------- C.mc --------------
// 2020-01-01 can be inlined because it's only used in 1 place...
(:inline, :bar) function small_method() {
 big_method("bar");
}
(:inline, :no_bar) function small_method() {
 big_method("no_bar");
}

but then he forgot about the comment in B.mc and in 2022 he uses foo() in another place (who can guess where the example comes from? :) So now if we go by what I wrote above we would be OK, because big_method() is only called from 1 place. But the decision to call the "small_method" twice ("I tested it in the sim, and it only adds 16 bytes..." - just forgot to test it with prettier optimizer ;) didn't take into consideration that when big_method is inlined then small_method becomes very big method... So here it would make sense to give a warning.

So maybe this is what you have to do: count how many times each function is inlined at the final stage.

BTW I thought one more reason why having inline_once, inline_many is good: if someone inlines not because of technical issue (need to have different code in 1 place depending on annotations) or in order to save code size, but optimize performance (want to have clean separated code, and only write it once, but be able to inline it multiple times in order to save on the call (add parameters to the stack, call, return, get the result from the stack)) then it's good to have inline_multi and not get warnings, even if the size gets bigger. 

flocsy🤠 said:

wow. big chaos

I've found another edge case that can trigger the behavior you saw. Keeping your B.mc, I've made this change to C.mc

import Toybox.Ant;
import Toybox.Lang;
import Toybox.Application;

const K = 42;
class C {
    private var mB as B = new B();

    public function c(x as Number or Float) as Number {
        return d(42);
    }

    (:inline)
    hidden function d(beats as Number) as Number {
        var b = mB;
        var s = b.K;
        if (beats > 1) {
            b.remove(beats);
        }
        return s;
    }
}

ERROR> fenix5xplus: source/C.mc:18:13: $.Toybox.PersistedContent.remove expects 0 arguments, but got 1
ERROR> fenix5xplus: source/C.mc:18:13: $.Toybox.PersistedContent.Waypoint.remove expects 0 arguments, but got 1
ERROR> fenix5xplus: source/C.mc:18:13: $.Toybox.PersistedContent.Course.remove expects 0 arguments, but got 1
ERROR> fenix5xplus: source/C.mc:18:13: $.Toybox.PersistedContent.Workout.remove expects 0 arguments, but got 1
ERROR> fenix5xplus: source/C.mc:18:13: $.Toybox.PersistedContent.Route.remove expects 0 arguments, but got 1
ERROR> fenix5xplus: source/C.mc:18:13: $.Toybox.PersistedContent.Track.remove expects 0 arguments, but got 1

basically, I've added a reference to b.K (note that any global namespace name would do). Normally, when you access a member variable, the type analyzer will refine the type of the object based on the property it sees. In this case, thats the wrong thing to do, and end up deducing that b has the "empty" type, and then have no idea how to lookup b.reduce, so assume it could be any of the reduce functions, and then warn because most of them need different arguments.

I suspect that this isn't your problem though, because Garmin's type checker will complain that K doesn't exist on B (although if you disable Garmin's type checker, the code runs correctly).

Yeah, I don't think this is my case.

I found another bug:

class D extends DataField {
    function onUpdate(dc as Graphics.Dc) {
        var backgroundColor = getBackgroundColor();
        var foregroundColor = backgroundColor == Graphics.COLOR_BLACK ? Graphics.COLOR_WHITE : Graphics.COLOR_BLACK;
        dc.setColor(foregroundColor, backgroundColor);
        dc.setColor(Graphics.COLOR_WHITE, Graphics.COLOR_BLACK);
    }
}

The editor shows backgroundColor in setColor() as red:
Argument 2 to $.Toybox.Graphics.Dc.setColor expected to be Number or Number as Toybox.Graphics.ColorValue but got Number or Enum[pmc-analysis]

another one:

    var cond1 as Boolean = true;
    var cond2 as Boolean = true;
    var mA as Number = 0;
    function a(dc as Graphics.Dc) as Void {
        var zero = 0;
        var hJ = Graphics.TEXT_JUSTIFY_CENTER;
        var vJ = Graphics.TEXT_JUSTIFY_VCENTER;

        if (cond1) {
            hJ = Graphics.TEXT_JUSTIFY_RIGHT;
        } else {
            hJ = Graphics.TEXT_JUSTIFY_LEFT;
        }
        if (cond1) {
        } else {
            vJ = zero;
        }
        mA = hJ;
        mA = vJ; // Invalid assignment to mA. Expected Number but got Number or Enum[pmc-analysis]
        mA = hJ | vJ; // Invalid assignment to mA. Expected Number but got Boolean[pmc-analysis]
    }

flocsy🤠 said:

Yeah, I don't think this is my case.

What about this one then... could it be the same "const" bug from earlier?

If I change your original example to 

class C {
    private const mB = new B();

i.e a const without a declared type (as opposed to the var with a declared type) then the bug repros before my const fix, and not after...

    const Z = -1.0;
    var mZ as Float = 0.0;
    hidden function f() as Void {
        var fZ = Z;
        mZ = fZ; // Invalid assignment to mZ. Expected Float but got Any[pmc-analysis]
    }

it's not a const in my real code, but I'll try to change it to a const, cause it could be, I never change. Is there any difference "nowadays" between a var and a const in the class level? I think a few compiler versions earlier I saw that using const makes the code bigger so I stopped using consts in classes.

This one I'm not sure if it's a bug or a problem in the Monkey C language:

    function x() {
        Ant.GenericChannel.initialize(
            method(:onMessage), // here 2 warnings
            new Ant.ChannelAssignment(Ant.CHANNEL_TYPE_RX_NOT_TX, Ant.NETWORK_PLUS)
        );
    }
    public function onMessage(msg as Message) as Void {
    }

flocsy🤠 said:

Do I need to add: "as Method<(Toybox.Ant.Message) as Null>"

That's not the correct syntax - I should fix the error message.

I think it would be "as Method(m as Toybox.Ant.Message) as Null". But that suggests that Garmin's compiler is inferring the type by looking up the method (since presumably it doesn't give an error). I can certainly do the same...