Big update to prettier-extension-monkeyc

Andreas Huggel said:

I see lots of warnings from the pmc live code analyzer like the following. They don't seem to be correct, and I wonder if there is a way to silence them?

I'm confused about how your project is configured.

Clearly, live analysis is reading all the files under source-legacy/, source-modern/ and source-amoled/. But it only reads files that it finds through monkey.jungle - so (unless I have a bug, which isn't out of the question), you have a monkey.jungle that lists all those directories (or that doesn't mention sourcePath at all, so that it picks up all the .mc files under the root directory).

But if you have such a monkey.jungle, Garmin's compiler is also going to pick up all the source files, and you'll end up with compile time errors, because you have multiple definitions for things.

So, can you clarify how things are setup?

I'll just add that if I had a project like this I would create 3 separate monkey.jungle files (one for each build), in three separate directories. I would then add each of those directories to a vscode workspace. The analyzer would then analyze each "project" separately. A small downside is that every diagnostic for the "common" files would then appear 3 times - once for each project - but that's just extra incentive to clean up any diagnostics.

Sorry, I think I'm being a bit slow here. Clearly you have a monkey.jungle that chooses which files to use based on the device you're compiling for.

So yes, this is a problem with the live analysis. It finds all source files referenced by all devices. That's usually ok, but if you have multiple definitions of a type, or if types vary between different source sets, it can cause issues like this.

I don't really have a solution at the moment, I'm afraid.

I could add an option to limit the analysis to a particular device, or maybe device family. That would avoid this type of warning, but would mean that only one set of sources was being analyzed at any given time...

 > Clearly you have a monkey.jungle that chooses which files to use based on the device you're compiling for.

Yes, with base source path set to source and device specific additions (one of the three other folder) and some exclude annotations for each device. Nothing unusual, I think. I'm actually a bit puzzled that this issue seems to be new.

> I could add an option to limit the analysis to a particular device...

What about an option to limit the analysis to the code for the device last built? For the way I typically work, that would solve my issue. If not ticked it would still analyze everything. If it's an extension option to a particular device, I'd probably forget to go to the extension settings and change it most of the time when I switch and work on a different architecture.

Andreas Huggel said:

I'm actually a bit puzzled that this issue seems to be new.

There was a report of a similar issue a few days ago. In that case he had functions that returned different types (Number vs Array<Number>) depending on release vs debug builds. In his case, he was using exclude annotations, and the caller and callee would always be in sync.

So it's not quite new, but it does seem to be fairly unusual. It's also something I've given quite a lot of thought to, and full blown solutions seem prohibitively expensive (potentially running analysis once for each device listed in the manifest). And in most cases I've come across, there seem to be better ways to organize the code.

eg in the release vs debug case, why not give the functions distinct names. if release foo always calls release bar, and debug foo always calls debug bar, why not have foo, bar and fooDebug and barDebug?

But yours does seem different. If I understand correctly, you have three enum definitions, and three functions for handling the corresponding values - but there are some "shared" enum values that are present in all three enums, and your shared code might call enumHandler(commonValue) - so having three separate enumHandler functions would mess up the shared code.

One possible solution I've thought about is keeping a set of possible devices for each definition. ie figure out up front for each module, class, function and enum, the set of devices for which *this* definition is live. Then if a function foo calls bar, and there are 3 possible definitions for bar, I can check the intersection of the live set for foo with bar, and eliminate some of the bars. That might be feasible performance wise - but there are a lot of special cases to handle. I'm not sure when, but I might give this a try...

Andreas Huggel said:

What about an option to limit the analysis to the code for the device last built?

That would work, I guess. What about before you build anything though?

Meanwhile, there is an option that should help, although you lose some checking. Setting prettierMonkeyC.strictTypeCheck to Off should eliminate these warnings.

The difference between strict and non-strict type checking is that with strict it will complain unless it can prove the type is correct; with non-strict it won't complain if the type might be correct.

So eg passing something that is known to be "String or Number" to a function that expects "String" would warn in strict mode, but not non-strict. Otoh passing an Array to the same function would warn in both cases.

For you this should suppress the diagnostics because it knows that the value being passed is one of three types, and each of the possible callees will accept one of those types. Otoh, you're getting less precise diagnostics overall. But you'd still get Garmin's strict diagnostics at build time. You could even setup build/launch tasks which explicitly set prettierMonkeyC.strictTypeCheck to "On" so that you also got my strict diagnostics at build time.

markw65 said:

There was a report of a similar issue a few days ago. In that case he had functions that returned different types (Number vs Array<Number>) depending on release vs debug builds. In his case, he was using exclude annotations, and the caller and callee would always be in sync.

After going through all my warnings, here's another example for the list of cases: A class in my common code has two different constructors, which have different numbers of arguments. Exclude annotations and monkey.jungle instructions make sure everything works fine, but the analyzer sees all the code and gets confused. Mentioning this just because renaming a constructor isn't an option, and I also can't immediately see a pragmatic solution that I would want to implement for my particular scenario.

markw65 said:

If I understand correctly, you have three enum definitions, and three functions for handling the corresponding values - but there are some "shared" enum values that are present in all three enums, and your shared code might call enumHandler(commonValue)

Yes, that description is accurate.

markw65 said:

Andreas Huggel said:

What about an option to limit the analysis to the code for the device last built?

That would work, I guess. What about before you build anything though?

Either don't run the analyzer at all until then, or still let it see all the code, I guess. But yes, this suggestion is inferior to the comprehensive approach you described, where the analyzer is aware of the monkey.jungle build instructions in totality. Implementing that does sound like a good challenge though.

markw65 said:

Meanwhile, there is an option that should help, although you lose some checking. Setting prettierMonkeyC.strictTypeCheck to Off should eliminate these warnings.

Yes, that works, and thanks for your explanation of what it actually changes. I'm quite happy with this now.