From db9edb477e6f55b37261094447ea16ab69b924c7 Mon Sep 17 00:00:00 2001 From: meisl Date: Fri, 2 Jul 2021 12:38:24 +0200 Subject: [PATCH 1/5] * less confusing assertion messages (seemingly contradictory in case of failure) --- compiler/test/TestCompilerOnCharLit.kt | 6 +++--- compiler/test/TestCompilerOnExamples.kt | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/compiler/test/TestCompilerOnCharLit.kt b/compiler/test/TestCompilerOnCharLit.kt index 03a5b2a7e..b8cfc5252 100644 --- a/compiler/test/TestCompilerOnCharLit.kt +++ b/compiler/test/TestCompilerOnCharLit.kt @@ -51,7 +51,7 @@ class TestCompilerOnCharLit { libdirs = listOf(), outputDir ) - assertTrue(result.success, "compilation successful") + assertTrue(result.success, "compilation should succeed") val program = result.programAst val startSub = program.entrypoint() @@ -77,7 +77,7 @@ class TestCompilerOnCharLit { libdirs = listOf(), outputDir ) - assertTrue(result.success, "compilation successful") + assertTrue(result.success, "compilation should succeed") val program = result.programAst val startSub = program.entrypoint() val funCall = startSub.statements.filterIsInstance()[0] @@ -113,7 +113,7 @@ class TestCompilerOnCharLit { libdirs = listOf(), outputDir ) - assertTrue(result.success, "compilation successful") + assertTrue(result.success, "compilation should succeed") val program = result.programAst val startSub = program.entrypoint() val funCall = startSub.statements.filterIsInstance()[0] diff --git a/compiler/test/TestCompilerOnExamples.kt b/compiler/test/TestCompilerOnExamples.kt index effd4c23d..8e4fcb954 100644 --- a/compiler/test/TestCompilerOnExamples.kt +++ b/compiler/test/TestCompilerOnExamples.kt @@ -46,7 +46,8 @@ class TestCompilerOnExamples { libdirs = listOf(), outputDir ) - assertTrue(result.success, "${platform.name}, optimize=$optimize: \"$filepath\"") + assertTrue(result.success, + "compilation should succeed; ${platform.name}, optimize=$optimize: \"$filepath\"") } From 07d8052a579f7e3f858bd1fc02d59f2d2af82ad4 Mon Sep 17 00:00:00 2001 From: meisl Date: Fri, 2 Jul 2021 13:28:19 +0200 Subject: [PATCH 2/5] * fix comments: no more problem with exitProcess --- compiler/test/TestCompilerOnCharLit.kt | 3 --- compiler/test/TestCompilerOnExamples.kt | 3 --- 2 files changed, 6 deletions(-) diff --git a/compiler/test/TestCompilerOnCharLit.kt b/compiler/test/TestCompilerOnCharLit.kt index b8cfc5252..3c6fcb600 100644 --- a/compiler/test/TestCompilerOnCharLit.kt +++ b/compiler/test/TestCompilerOnCharLit.kt @@ -21,9 +21,6 @@ import kotlin.test.assertTrue * ATTENTION: this is just kludge! * They are not really unit tests, but rather tests of the whole process, * from source file loading all the way through to running 64tass. - * What's more: in case of failure (to compile and assemble) - which is when tests should help you - - * these tests will actually be ignored (ie. NOT fail), - * because in the code there are calls to Process.exit, making it essentially untestable. */ @TestInstance(TestInstance.Lifecycle.PER_CLASS) class TestCompilerOnCharLit { diff --git a/compiler/test/TestCompilerOnExamples.kt b/compiler/test/TestCompilerOnExamples.kt index 8e4fcb954..3c5737a59 100644 --- a/compiler/test/TestCompilerOnExamples.kt +++ b/compiler/test/TestCompilerOnExamples.kt @@ -16,9 +16,6 @@ import kotlin.test.assertTrue * ATTENTION: this is just kludge! * They are not really unit tests, but rather tests of the whole process, * from source file loading all the way through to running 64tass. - * What's more: in case of failure (to compile and assemble) - which is when tests should help you - - * these tests will actually be ignored (ie. NOT fail), - * because in the code there are calls to Process.exit, making it essentially untestable. */ @TestInstance(TestInstance.Lifecycle.PER_CLASS) class TestCompilerOnExamples { From c786acc39bdece74c523fa577489151e85627fc3 Mon Sep 17 00:00:00 2001 From: meisl Date: Fri, 2 Jul 2021 15:41:38 +0200 Subject: [PATCH 3/5] + CompilerDevelopment.md, outlining what to do to improve testability (atm only for the parsing stage) --- CompilerDevelopment.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 CompilerDevelopment.md diff --git a/CompilerDevelopment.md b/CompilerDevelopment.md new file mode 100644 index 000000000..c9829e6fb --- /dev/null +++ b/CompilerDevelopment.md @@ -0,0 +1,29 @@ +#### Just a few remarks upfront: +* There is the (gradle/IDEA) module `parser`: that's the parser generated by ANTLR4, in Java. The only file to be edited here is the grammar, `prog8.g4`. +* Then we have the module `compilerAst` - in Kotlin - which uses `parser` and adds AST nodes. Here we put our additions to the generated thing, *including any tests of the parsing stage*. + - the name is a bit misleading, as this module isn't (or, resp. shouldn't be; see below) about *compiling*, only the parsing stage + - also, the tree that comes out isn't much of an *abstraction*, but rather still more or less a parse tree (this might very well change). + - **However, let's not *yet* rename the module.** We'll find a good name during refactoring. + +#### Problems with `compilerAst`: +* `ModuleImporter.kt`, doing (Prog8-) module resolution. That's not the parser's job. +* During parsing, character literals are turned into UBYTEs (since there is no basic type e.g. CHAR). That's bad because it depends on a specific character encoding (`IStringEncoding` in `compilerAst/src/prog8/ast/AstToplevel.kt`) of/for some target platform. Note that *strings* are indeed encoded later, in the `compiler` module. +* The same argument applies to `IMemSizer`, and - not entirely sure about that - `IBuiltinFunctions`. + +#### Steps to take, in order: +1. introduce an abstraction `SourceCode` that encapsulates the origin and actual loading of Prog8 source code + - from the local file system (use case: user programs) + - from resources (prog8lib) + - from plain strings (for testing) +2. introduce a minimal interface to the outside, input: `SourceCode`, output: a tree with a `Module` node as the root + - this will be the Kotlin singleton `Prog8Parser` with the main method `parseModule` + - plus, optionally, method's for registering/unregistering a listener with the parser + - anything related to the lexer, error strategies, character/token streams is hidden from the outside + - to make a clear distinction between the *generated* parser (and lexer) vs. `Prog8Parser`, and to discourage directly using the generated stuff, we'll rename the existing `prog8Parser`/`prog8Lexer` to `Prog8ANTLRParser` and `Prog8ANTLRLexer` and move them to package `prog8.parser.generated` +3. introduce AST node `CharLiteral` and keep them until after identifier resolution and type checking; insert there an AST transformation step that turns them in UBYTE constants (literals) +4. remove uses of `IStringEncoding` from module `compilerAst` - none should be necessary anymore +5. move `IStringEncoding` to module `compiler` +6. same with `ModuleImporter`, then rewrite that (addressing #46) +7. refactor AST nodes and grammar: less generated parse tree nodes (`XyzContext`), less intermediary stuff (private classes in `Antr2Kotlin.kt` [sic]), more compact code. Also: nicer names such as simply `StringLiteral` instead of `StringLiteralValue` +8. re-think `IStringEncoding` to address #38 + From a598eb7e98f1df643c2b3e51e13f20d3247cd200 Mon Sep 17 00:00:00 2001 From: meisl Date: Fri, 2 Jul 2021 18:42:38 +0200 Subject: [PATCH 4/5] + add mention of `ParseError : ParsingFailedError` - particularly for testability this is something that needs to be done --- CompilerDevelopment.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/CompilerDevelopment.md b/CompilerDevelopment.md index c9829e6fb..2df63341f 100644 --- a/CompilerDevelopment.md +++ b/CompilerDevelopment.md @@ -7,6 +7,7 @@ #### Problems with `compilerAst`: * `ModuleImporter.kt`, doing (Prog8-) module resolution. That's not the parser's job. +* `ParsingFailedError` (in `ModuleParsing.kt`): this exception (it is actually *not* a `java.lang.Error`...) is thrown in a number of places, where other exceptions would make more sense. For example: not finding a file should just yield a `NoSuchFileException`, not this one. The other problem with it is that it does not provide any additional information about the source of parsing error, in particular a `Position`. * During parsing, character literals are turned into UBYTEs (since there is no basic type e.g. CHAR). That's bad because it depends on a specific character encoding (`IStringEncoding` in `compilerAst/src/prog8/ast/AstToplevel.kt`) of/for some target platform. Note that *strings* are indeed encoded later, in the `compiler` module. * The same argument applies to `IMemSizer`, and - not entirely sure about that - `IBuiltinFunctions`. @@ -15,15 +16,17 @@ - from the local file system (use case: user programs) - from resources (prog8lib) - from plain strings (for testing) -2. introduce a minimal interface to the outside, input: `SourceCode`, output: a tree with a `Module` node as the root +2. add subclass `ParseError : ParsingFailedError` which adds information about the *source of parsing error* (`SourceCode` and `Position`). We cannot just replace `ParsingFailedError` right away because it is so widely used (even in the `compiler` module). Therefore we'll just subclass for the time being, add more and more tests requiring the new one to be thrown (or, resp., NOT to be thrown), and gradually transition. +3. introduce a minimal interface to the outside, input: `SourceCode`, output: a tree with a `Module` node as the root - this will be the Kotlin singleton `Prog8Parser` with the main method `parseModule` - plus, optionally, method's for registering/unregistering a listener with the parser + - the *only* exception ever thrown / reported to listeners (TBD) will be `ParseError` - anything related to the lexer, error strategies, character/token streams is hidden from the outside - to make a clear distinction between the *generated* parser (and lexer) vs. `Prog8Parser`, and to discourage directly using the generated stuff, we'll rename the existing `prog8Parser`/`prog8Lexer` to `Prog8ANTLRParser` and `Prog8ANTLRLexer` and move them to package `prog8.parser.generated` -3. introduce AST node `CharLiteral` and keep them until after identifier resolution and type checking; insert there an AST transformation step that turns them in UBYTE constants (literals) -4. remove uses of `IStringEncoding` from module `compilerAst` - none should be necessary anymore -5. move `IStringEncoding` to module `compiler` -6. same with `ModuleImporter`, then rewrite that (addressing #46) -7. refactor AST nodes and grammar: less generated parse tree nodes (`XyzContext`), less intermediary stuff (private classes in `Antr2Kotlin.kt` [sic]), more compact code. Also: nicer names such as simply `StringLiteral` instead of `StringLiteralValue` -8. re-think `IStringEncoding` to address #38 +4. introduce AST node `CharLiteral` and keep them until after identifier resolution and type checking; insert there an AST transformation step that turns them in UBYTE constants (literals) +5. remove uses of `IStringEncoding` from module `compilerAst` - none should be necessary anymore +6. move `IStringEncoding` to module `compiler` +7. same with `ModuleImporter`, then rewrite that (addressing #46) +8. refactor AST nodes and grammar: less generated parse tree nodes (`XyzContext`), less intermediary stuff (private classes in `Antr2Kotlin.kt` [sic]), more compact code. Also: nicer names such as simply `StringLiteral` instead of `StringLiteralValue` +9. re-think `IStringEncoding` to address #38 From 4ac92caeb5bffef5c2234f0acb8ba724d6a72964 Mon Sep 17 00:00:00 2001 From: meisl Date: Sat, 3 Jul 2021 15:11:34 +0200 Subject: [PATCH 5/5] Update CompilerDevelopment.md --- CompilerDevelopment.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CompilerDevelopment.md b/CompilerDevelopment.md index 2df63341f..57ffcace9 100644 --- a/CompilerDevelopment.md +++ b/CompilerDevelopment.md @@ -11,7 +11,7 @@ * During parsing, character literals are turned into UBYTEs (since there is no basic type e.g. CHAR). That's bad because it depends on a specific character encoding (`IStringEncoding` in `compilerAst/src/prog8/ast/AstToplevel.kt`) of/for some target platform. Note that *strings* are indeed encoded later, in the `compiler` module. * The same argument applies to `IMemSizer`, and - not entirely sure about that - `IBuiltinFunctions`. -#### Steps to take, in order: +#### Steps to take, in conceptual (!) order: 1. introduce an abstraction `SourceCode` that encapsulates the origin and actual loading of Prog8 source code - from the local file system (use case: user programs) - from resources (prog8lib) @@ -27,6 +27,6 @@ 5. remove uses of `IStringEncoding` from module `compilerAst` - none should be necessary anymore 6. move `IStringEncoding` to module `compiler` 7. same with `ModuleImporter`, then rewrite that (addressing #46) -8. refactor AST nodes and grammar: less generated parse tree nodes (`XyzContext`), less intermediary stuff (private classes in `Antr2Kotlin.kt` [sic]), more compact code. Also: nicer names such as simply `StringLiteral` instead of `StringLiteralValue` +8. refactor AST nodes and grammar: less generated parse tree nodes (`XyzContext`), less intermediary stuff (private classes in `Antrl2Kotlin.kt`), more compact code. Also: nicer names such as simply `StringLiteral` instead of `StringLiteralValue` 9. re-think `IStringEncoding` to address #38