Implement plan for testability

[meisl]

This is my take on implementing the plan laid out in #50 ("CompilerDevelopment.md").

As discussed, commits will be added to this draft in bunches, allowing for comments and/or review in between.

The first bunch covers steps 1, 2 and 3 of the plan, in a total of 14 commits (not counting merges from v7.1 and what's already in #50), and about triples the nr of tests in compilerAst.
These first 3 steps were inseparable from each other, and only after the 14 commits is a state established where everything that was working before is working again, and all tests pass - plus of course improvements wrt. testability, readability and actual functionality such as resolving #46.

[meisl]

Since this was rebased on v7.1 (again), the change re Module.isLibrary() (a7736d8) is incorporated.

• The main things added are Prog8Parser, SourceCode and ParseError.
• ModuleImporter is adjusted just enough to work with these, but not any further.
• The last commit above, 44da7a3, derives a Module name, similar to what was recently added on v7.1: Module.pathForResource(resourcePath: String) (which is also incorporated). However, here it is
  • done in Prog8Parser.parseModule, hence only for modules that the parser creates (as opposed to e.g. "prog8_interned_strings")
  • using SourceCode, not a String
  • using a different way of indicating that the module came from resources: <res:/path/inside/resources> instead of @embedded@/path/inside/resources

The next BOC "Bunch'O'Commits" (TM) - to this PR draft will address the last point s.t. it's using the same @embedded@ convention as on v7.1 atm.

[meisl]

1. 137a89d is more or less preparation.
   • It mainly reorganizes Prog8Parser so that all the derivation of the redundant args for ctor Module are concentrated.
   • This is done via (private) subclass ParsedModule : Module, since I don't yet wanted to change the public Module ctor just yet (neither even so radically).
   • Note: the Module is not anymore instantiated via ext method .toAst (simply unnecessary) but directly through ctor. What's more important: the module is created first, and only then are statements added. This way we have access to the modules SourceCode beforehand (see also On making an AST #52)
   • Plus some more tests
2. b0073ac actually changes "<res:...>" to "@Embedded@...". I, personally, liked the former better
   • because it yields NOT a valid path name (sic!)
   • it at least hints at what it's really about: resources
   • it resembles a URL - the canonical way of expressing something like this (which you were mentioning somewhere [TODO]); even though with a made-up scheme "res:", admittedly.
   • @Embedded@ is rendered strangely in markdown (you can see this also from the commit messages - I did use lowercase but it's rendered capitalized)
   • but anyways, and in case we want to go back: that's no problem at all - whereas this way revealed quite something, as the following nr 3 shows:
3. 19bb56d was initially imagined to be a rather simple cleanup step which supposed to reduce the occurrences of the magic "@Embedded@" to just a few in one place. That it does, but it also turned up quite some more:
   • First off: Module.source is now of type SourceCode?. We must be honest in the code: either a module does come from actual source code - or it does not. No more "dummy" stuff, that's nothing but cheating and I just cannot see any benefit from doing that all over the place.
   • Then I introduced SourceCode.isFromResources (had to, don't really like it). Module.isLibrary is now pretty self-explanatory: true if there's no SourceCode at all or else also true if source.isFromResources. That is the exact same logic, but way more clear for someone who's wondering what .isLibrary is meant to be and looking.
   • This turned up quite some more unexpected, and hitherto silent problems:
     • Compiler.kt / parseImports(..): again, same functionality but now more expressive -> question: variable importedFiles - should this be only those that are really coming from the file system, maybe?
     • AstChecker.kt / checkFileExists(d: Directive, fileName: String): there I found another magic thing, .startsWith("library:"). I figured this was old and simply forgotten when "@Embedded@" was introduced. So can now be changed into definingModule.isLibrary(). Was that ok?
     • Also there: code that's doing what I think Module.definingModule() is doing. But I didn't dare to replace that.
     • This "library:" prefix is used in Prog8 source code inside res/prog8lib/, with %asminclude directives. For example math.p8: %asminclude "library:math.asm"
     • AsmGen.kt / translate(Directive), cases %asminclude and %asmbinary: same functionality but should be consolidated into SourceCode

Remark

I am trying hard to leave out opinion. And I'll stick with it - unless you ask :)

[meisl]

Addendum: problems present in this PR up to this point

• No tests for the isLibrary stuff etc:
  • making up a SourceCode that only pretends to "come from resources" makes no sense, since it would mean to open up the actual class under test. That is: I think so..?
  • in tests there are no resources available like in the final .jar
  • ...unless we make one as a test fixture. That's pretty weird, but the best I can think of atm
• The .position of AST nodes below Module is generally broken, i.e. not always well-defined / consistent with the one (wrt field .file) of the defining module

Please note that there is more in the pipeline already. Even more than one variation in cases. It really depends on your feedback now.

[meisl]

Nice first overview: https://github.com/irmen/prog8/network
[just in the very improbable case you didn't check]

[meisl]

These five commits are adding and improving tests. Worth mentioning are:

• Nr. 2, c3e9d4a: this solves the problem of not having resources available in tests. It is changes /compilerAst/build.gradle and compilerAst/compilerAst.iml (IDEA file); you may have to re-import the gradle project into IDEA.
• Nr. 3, ddaef3e, which uses Nr 2 and adds the missing tests for SourceCode.fromResources(..)
• Nr. 5, 39d5b7e: some more of the end-to-end tests with examples, in particular now a) for both target platforms and b) including the two largest examples, tehtriz.p8 and textelite.p8

So, my "Addendum re current problems" above is now outdated for the most part.

In fact, it was rather misleading to begin with (because I was misled):

• It is possible to test resources, as shown.
• Hence neither do we need to "make up" something, nor have a .jar. That was just stupid me.
• Re .position of AST nodes below Module:
  • There are tests for that in compilerAst/test/TestProg8Parser.kt, namely testInnerNodePositionsForSourceFromPath and testInnerNodePositionsForSourceFromString
  • Only the latter is failing atm. That is: only if the source is NOT coming from resources or a file is when the .position.file == "<unknown>". That's exactly the status as of the current v7.1 HEAD, only that here we're testing it.
  • I've tried a few things, and thought about it. The real problem is in fact the extension method approach, see On making an AST #52. So step 8, "refactor AST nodes and grammar" would solve this anyway. Therefore I will leave it at that until then.

I would really like to move on to step 4 now.

As said, I've got almost all of the plan already. Plus variations.
Btw.: the "previews" on my fork have not produced any feedback so far. So no more of that.

[meisl]

So now we've reached step 7, move ModuleImporter to module compiler, but the rewrite part is yet to be done.
Once again, the moving as such isn't too exciting, but the preparation is important: improving and adding tests.

In doing so I found yet another bug: the -libdirs option didn't work.
Of course this is tied to ModuleImporter, and since this will be done over anyways I didn't file an extra bug report this time.
Also: I was able to do a quick fix, at least as far as the primary module file is concerned (probably still not quite working for imported stuff).

While looking into this I decided that access to a Program's module should be more restricted, so that

• we can make a few guarantees like no module can be added more than once, no two of the same name, and: parent-child relationship is always set up correctly
• instantiation and setup of a program is more concise at the call site

The question came up as I realized that in case of parse errors, both the ModuleImporter and associated Program instance are left in an inconsistent state:

• the module (file) that caused the error is not added to the program
• but the one which imported that is
• therefore, if we call importModule or importLibraryModule on the importer instance again - which we can - things look like normal

One thing that really puzzled me (and still does) is this: how to correctly "linkParents" when adding a module to a program?

• Both, Node and INameScope define .parent and linkParents(Node)
• Program implements Node but not INameScope
• Instead, it has namespace: GlobalNamespace - which implements both, Node AND INameScope!
• I found all of these variations:
  • module.linkParents(program); module.program = program
  • module.linkParents(ParentSentinel); module.program = program (AsmgenTests.kt)
  • module.linkParents(program.namespace); module.program = program
    I'm guessing the last one is correct, and that's how I implemented Program.addModule(Module). See eb46852 for what I did exactly.

To wrap up:

• The whole ModuleImporter story is unsatisfactory.
• Atm, both, the importer and the associated program hold information about the state of incremental parsing.
• But it should only be one thing that does that, really.
• In order to fix that, plus collecting together similar related stuff (e.g. asminclude, asmbinary, tryGetEmbeddedResource in Compiler.kt) - a few more lower level improvements such as the one re Program are in order.
• And: more and better tests.
• At this point I think it's time to at least introduce kotest, or, even better: commit to it. I'd like to avoid writing lots of stuff in the old style(s) and then having to rewrite them rather sooner than later.
• So, let's next look at how a common new style could look like, by means of a few step-by-step examples.

[meisl]

First, here's something we should definitely do - whether or not we'll switch to kotest:
Structuring the tests by means of

• @Nested annotated inner classes (c2986ea)
• and `backtick`-syntax for test method names (f0c150d, bd6c60c)

There are a few restrictions re the characters that can be used (with kotest: none), but still absolutely worth it.
I mean, just compare before (left) and after (right):

[irmen]

@meisl I may be able to start spending some time again on prog8 in the coming weeks. Any quick hint on where to start reviewing the things again concerning this topic?

[irmen]

@meisl I tried to rebase this on the current 7.1 but that is a bit of work. Merging the branches however is a lot simpler - so if you don't particularly care about having all your commits in this branch on top, I'd say forget rebasing

Also merging this branch back into 7.1 is rather uneventful, the few commits mentioned above can be resolved easily.

That said I still haven't properly reviewed all of this PR, so that's still a task for me before we can hit the merge button.

[irmen]

Why would you want this?
For included assembly code I could think of only one reason which is that prog8 itself is going to analyze the included assembly. But for binary blobs? What's wrong with just letting the assembler take care of this

[irmen]

I'm going to go ahead with the proposed changes.

[irmen]

@meisl for posterity, I've created a branch testability_123_before_merge_71_by_irmen in your repo that represents the state of the testability_steps_1_2_3_again branch before I merged 7.1 into it to resolve the handful of conflicts there - and merging it all back into 7.1.

Yay!

Thanks again for your work on this so far. Haven't heard from you for a while I hope you haven't lost interest in the project