Big update to prettier-extension-monkeyc

andyj713 said:

It looks like it's expecting new values being added to a dictionary to be the same type as other items already in the dictionary

Yes. I'm attempting to fix a problem with Garmin's type checker, but it does introduce other issues, like this one.

With Garmin's type checker, [0, 1, 2] has type Array, not type Array<Number>, similarly with Dictionaries, and as a result 

mArray as Array<Number> = [1, 2, 3];
mDict as Dictionary<Symbol, String> = { :foo => "bar" };

are both errors (according to Garmin), because you're trying to assign untyped Arrays and Dictionaries to typed ones. So to make this work you have to write 

mArray as Array<Number> = [1, 2, 3] as Array<Number>;
mDict as Dictionary<Symbol, String> = { :foo => "bar" } as Dictionary<Symbol, String>;

I don't like that for two reasons. First, you have to redundantly repeat the type; and second, you're hiding errors (because if you write mArray as Array<Number> = ["foo"] as Array<number>; Garmin's typechecker won't notice).

So I explicitly type literal arrays and dictionaries - which can result in the issue you're noticing. To get back to Garmin's behavior you can just write:

  var cPicBufOpts = {:width=>picX, :height=>picY, :bitmapResource=>_centerPic} as Dictionary;

or you could be more specific: Dictionary<Symbol, Object>, or you could be as precise as you want with the type.

andyj713 said:

Toybox.WatchUi.BitmapResource<{getWidth: Function<Toybox.WatchUi.BitmapResource.getWidth>, getHeight: Function<Toybox.WatchUi.BitmapResource.getHeight>}>

I learned from this that  when you create a new BufferedBitmap with a bitmapResource, the bitmap will take the dimensions from the bitmapResource, so :width and :height are not only redundant but not necessary and it will compile and run without them:

var cPicBufOpts = {:bitmapResource=>_centerPic};

But this just makes things worse:

WARNING> fenix7x: Argument 1 to $.Toybox.Lang.Dictionary.put expected to be :bitmapResource but got :colorDepth
WARNING> fenix7x: Argument 2 to $.Toybox.Lang.Dictionary.put expected to be Toybox.WatchUi.BitmapResource<{getWidth: Function<Toybox.WatchUi.BitmapResource.getWidth>, getHeight: Function<Toybox.WatchUi.BitmapResource.getHeight>}> but got Number<4>
WARNING> fenix7x: Argument 1 to $.Toybox.Lang.Dictionary.put expected to be :bitmapResource but got :palette
WARNING> fenix7x: Argument 2 to $.Toybox.Lang.Dictionary.put expected to be Toybox.WatchUi.BitmapResource<{getWidth: Function<Toybox.WatchUi.BitmapResource.getWidth>, getHeight: Function<Toybox.WatchUi.BitmapResource.getHeight>}> but got Array<Number as Toybox.Graphics.ColorValue>

Let me know if and how I can help.

markw65 said:

var cPicBufOpts = {:bitmapResource=>_centerPic} as Dictionary; will fix it.

Unfortunately when I try that I get this:

ERROR: fenix7x: Cannot initialize dictionary with type '$.Toybox.Lang.Dictionary'.

Without the Dictionary type it compiles and runs, I just get the warnings from before.

andyj713 said:

ERROR: fenix7x: Cannot initialize dictionary with type '$.Toybox.Lang.Dictionary'.

Oh, That looks like a garmin parser bug:

var cPicBufOpts1 = {:bitmapResource=>_centerPic} as Dictionary; // error
var cPicBufOpts2 = ({:bitmapResource=>_centerPic}) as Dictionary; // no problem

I'll file a bug report.

But there's definitely a problem I need to fix here, and I think I see a reasonable way to handle this that should keep both Garmin's type checker and mine happy (ie your existing code should be fine without the cast once I fix things).

Meanwhile as a workaround, it turns out that Dictionary probably isn't the correct type for the cast. eg

        var options =
            ({
                :width => 42,
                :height => 42,
            }) as Dictionary;

        options.put(:pallette, [
            Graphics.COLOR_DK_GRAY,
            Graphics.COLOR_LT_GRAY,
            Graphics.COLOR_BLACK,
            Graphics.COLOR_WHITE,
        ]);

        var bits = new Graphics.BufferedBitmap(options);

This now fails to compile at the new Graphics.BufferedBitmap line, because Dictionary isn't specific enough. So, for now, as a workaround, the best cast seems to be a subset of the expected type. So something like { :width as Number }

It actually doesn't matter what field(s) you specify, as long as you don't mis-declare something. So { :height as String } is a problem, because :height is required to be a Number, but { :foo as Dictionary } is fine, because :foo isn't included in the expected type at all.

So I'd suggest:

var cPicBufOpts = ({:bitmapResource=>_centerPic}) as { :width as Number };

And again - this is just a temporary workaround until I fix my type checker to do the right thing. I was planning a release soon anyway, so unless this turns out to be harder than I expect, it should be ready in a few days.

While you have the hood up, I have a few warnings about unused variables that I can clearly see are being used, so I suspect variable substitution is being used and it's reporting the now orphaned variables. On the plus side it gives me some hints about how I can reduce some variables on my own.

andyj713 said:

I have a few warnings about unused variables that I can clearly see are being used

You're saying that the monkeyc compiler reports unused variables, when they're clearly used in the original source? I've not seen that across all the projects I test with. In fact, if you start with a project that *does* report unused variables, and build it with my optimizer, the optimizer will just remove the unused variables, and then the monkeyc compiler doesn't get a chance to report them.

So to get anywhere with this I'd need to see an example. Can you create and post a small test case? Or at least show a few lines of the original, and optimized source?

One thought - try turning on the "Prettier MonkeyC: Iterate Optimizer" option. Its just possible that could fix the issue...

Original code:

	private var _moonShine as oneDim?;
	
		orbR = Astro.v_moonRiseR + V_faceAngle;
		_moonShine = Astro.v_moonShine as oneDim;
		var moonAngle = _moonShine[2] - _moonShine[1] + PI_2 + V_faceAngle as Float;
		var moonScale = V_littleR * 0.92;
		_moonDarkOrb = Orb.drawMoonRise(A_bigR, orbR, moonScale, moonAngle, 0.0);
		_moonFaceOrb = Orb.drawMoonRise(A_bigR, orbR, moonScale, moonAngle, _moonShine[3] + Math.PI);
		_moonDayOrb = Astro.v_moonRiseDay ? Orb.drawRegPoly(A_bigR, orbR, dayScale, orbR + Math.PI, 3) : null;

Optimized:

	private var _moonShine as oneDim?;
	
		orbR = pre_Astro.v_moonRiseR + pre_V_faceAngle;
		pre_battStat /*>pre__moonShine<*/ =
			pre_Astro.v_moonShine as oneDim;
		_moonShine = pre_battStat /*>pre__moonShine<*/;
		var moonAngle =
			pre_battStat /*>pre__moonShine<*/[2] -
			pre_battStat /*>pre__moonShine<*/[1] +
			1.5707964 +
			(pre_V_faceAngle as Float);
		var moonScale = pre_V_littleR * 0.92;
		_moonDarkOrb = pre_Orb.drawMoonRise(
			pre_A_bigR,
			orbR,
			moonScale,
			moonAngle,
			0.0
		);
		_moonFaceOrb = pre_Orb.drawMoonRise(
			pre_A_bigR,
			orbR,
			moonScale,
			moonAngle,
			pre_battStat /*>pre__moonShine<*/[pre_3] + pre_3_1415927
		);
		_moonDayOrb = pre_Astro.v_moonRiseDay
			? pre_Orb.drawRegPoly(
					pre_A_bigR,
					orbR,
					dayScale,
					orbR + pre_3_1415927,
					pre_3
			  )
			: null;

WARNING: fenix7x:  Member variable '_moonShine' is not used.

One thing I see is that the optimized code creates a local variable for everything not local like class and global variables and constants. Does this save space, execution time or both?

andyj713 said:

Does this save space, execution time or both?

When the optimizer does it, it is guaranteed to save space (basically there have to be at least two uses of a non-local for it to be worth putting it in a local). Generally, it will also make the code faster - but thats not actually guaranteed (in this case, it definitely does).

The issue here is that you're using a non-local variable, but you always assign to it before reading it. So it really ought to be a local.

My optimizer tries to minimize the number of reads and writes to non-locals by holding their values in locals; but currently, it always guarantees that prior to leaving a function (ie on return, or when calling another function that might read them), the non-locals have the correct values. So in this case, it manages to remove all the reads from _moonShine, but keeps the write, just in case.

But it doesn't notice that after all the transformations, _moonShine is written, but never read, so could be removed entirely (but Garmin's compiler does notice). I guess thats something I should check for too.

Feature request: When substituting numbers for color values, hex codes would be easier to interpret.