From cc13a5149370b8e9cde85b8c47be086b818d7a28 Mon Sep 17 00:00:00 2001 From: Irmen de Jong Date: Fri, 22 Nov 2024 23:21:23 +0100 Subject: [PATCH] fix import order problem related to %option merge --- .../compiler/astprocessing/BlockMerger.kt | 45 ++++-- .../TestImportedModulesOrderAndOptions.kt | 149 ++++++++++++++++- compiler/test/ast/TestProgram.kt | 152 +----------------- .../prog8/ast/expressions/AstExpressions.kt | 24 ++- docs/source/syntaxreference.rst | 2 + docs/source/todo.rst | 10 +- examples/test.p8 | 33 ---- 7 files changed, 200 insertions(+), 215 deletions(-) diff --git a/compiler/src/prog8/compiler/astprocessing/BlockMerger.kt b/compiler/src/prog8/compiler/astprocessing/BlockMerger.kt index 9dd19771a..f2d7c0581 100644 --- a/compiler/src/prog8/compiler/astprocessing/BlockMerger.kt +++ b/compiler/src/prog8/compiler/astprocessing/BlockMerger.kt @@ -7,25 +7,46 @@ import prog8.ast.statements.Subroutine import prog8.code.core.IErrorReporter class BlockMerger(val errors: IErrorReporter) { - // All blocks having a 'merge' option, - // will be joined into a block with the same name, coming from a library. - // (or a normal block if no library block with that name was found) + // Move everything into the first other block with the same name that does not have %option merge + // (if there is no block without that option, just select the first occurrence) + // Libraries are considered first. Don't merge into blocks that have already been merged elsewhere. + // If a merge is done, remove the source block altogether. private val mergedBlocks = mutableSetOf() // to make sure blocks aren't merged more than once fun visit(program: Program) { val allBlocks = program.allBlocks + + // It should be an error for the same block to be declared twice without either declaration having %option merge + // If all occurrences of the block have %option merge, the first in the list is chosen as the 'main' occurrence. + val multiples = allBlocks.groupBy { it.name }.filter { it.value.size > 1 } + for((name, blocks) in multiples) { + val withoutMerge = blocks.filter { "merge" !in it.options() } + if(withoutMerge.size>1) { + val positions = withoutMerge.joinToString(", ") { it.position.toString() } + errors.err("block '$name' is declared multiple times without %option merge: $positions", withoutMerge[0].position) + } + if(withoutMerge.isEmpty()) { + errors.err("all declarations of block '$name' have %option merge", blocks[0].position) + } + } + for(block in allBlocks) { - if("merge" in block.options()) { - // move everything into the first other block with the same name - // and remove the source block altogether - val libraryBlock = allBlocks.firstOrNull { it !== block && it.isInLibrary && it.name==block.name } - if(libraryBlock!=null) { - merge(block, libraryBlock) + if("merge" in block.options() && block.isNotEmpty()) { + val libraryBlockCandidates = + allBlocks.filter { it !== block && it.isInLibrary && it.name == block.name } + val (withMerge, withoutMerge) = libraryBlockCandidates.partition { "merge" in it.options() } + if (withoutMerge.isNotEmpty() && withoutMerge.any { it !in mergedBlocks}) { + merge(block, withoutMerge.first { it !in mergedBlocks }) + } else if (withMerge.isNotEmpty() && withMerge.any { it !in mergedBlocks}) { + merge(block, withMerge.first { it !in mergedBlocks }) } else { - val regularBlock = allBlocks.firstOrNull { it !== block && it.name==block.name } - if(regularBlock!=null) { - merge(block, regularBlock) + val regularBlockCandidates = allBlocks.filter { it !== block && it.name == block.name } + val (withMerge, withoutMerge) = regularBlockCandidates.partition { "merge" in it.options() } + if (withoutMerge.isNotEmpty() && withoutMerge.any { it !in mergedBlocks}) { + merge(block, withoutMerge.first { it !in mergedBlocks }) + } else if (withMerge.isNotEmpty() && withMerge.any { it !in mergedBlocks}) { + merge(block, withMerge.first { it !in mergedBlocks }) } } } diff --git a/compiler/test/TestImportedModulesOrderAndOptions.kt b/compiler/test/TestImportedModulesOrderAndOptions.kt index fe86aa6f3..3f9420c47 100644 --- a/compiler/test/TestImportedModulesOrderAndOptions.kt +++ b/compiler/test/TestImportedModulesOrderAndOptions.kt @@ -4,10 +4,12 @@ import io.kotest.assertions.withClue import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.shouldBe import io.kotest.matchers.shouldNotBe +import io.kotest.matchers.string.shouldContain import io.kotest.matchers.string.shouldStartWith import prog8.code.core.ZeropageType import prog8.code.core.internedStringsModuleName import prog8.code.target.C64Target +import prog8.code.target.Cx16Target import prog8.code.target.VMTarget import prog8.compiler.determineCompilationOptions import prog8.compiler.parseMainModule @@ -135,7 +137,7 @@ main { compileText(VMTarget(), optimize = false, src) shouldNotBe null } - test("double merge works") { + test("double merge is invalid") { val src=""" main { @@ -161,8 +163,151 @@ block1 { cx16.r2++ } }""" - compileText(VMTarget(), optimize = false, src) shouldNotBe null + val errors=ErrorReporterForTests() + compileText(VMTarget(), optimize = false, src, errors=errors) shouldBe null + errors.errors.size shouldBe 1 + errors.errors[0] shouldContain "all declarations of block 'block1' have %option merge" } + test("merge works") { + val src = """ +%import textio +main { + +sub start() { + blah.test() +} +} + +txt { +; merges this block into the txt block coming from the textio library +%option merge + +sub schrijf(str arg) { + print(arg) +} +} + +blah { +; merges this block into the other 'blah' one +%option merge + +sub test() { + printit("test merge") +} +} + +blah { +sub printit(str arg) { + txt.schrijf(arg) +} +}""" + compileText(C64Target(), optimize=false, src, writeAssembly=false) shouldNotBe null + } + + test("merge override existing subroutine") { + val src=""" +%import textio + +main { + +sub start() { + txt.print("sdfdsf") +} +} + +txt { +%option merge + +sub print(str text) { + cx16.r0++ + ; just some dummy implementation to replace existing print +} +}""" + + val result = compileText(VMTarget(), optimize=false, src, writeAssembly=false) + result shouldNotBe null + } + + test("merge doesn't override existing subroutine if signature differs") { + val src=""" +%import textio + +main { + +sub start() { + txt.print("sdfdsf") +} +} + +txt { +%option merge + +sub print(str anotherparamname) { + cx16.r0++ + ; just some dummy implementation to replace existing print +} +}""" + val errors = ErrorReporterForTests() + compileText(VMTarget(), optimize=false, src, writeAssembly=false, errors = errors) shouldBe null + errors.errors.size shouldBe 1 + errors.errors[0] shouldContain "name conflict" + } + + test("merge of float stuff into sys and txt - import order 1") { + val src=""" +%import textio +%import floats + +main { +sub start() { + txt.print_b(sys.MIN_BYTE) + txt.print_b(sys.MAX_BYTE) + txt.print_ub(sys.MIN_UBYTE) + txt.print_ub(sys.MAX_UBYTE) + txt.print_w(sys.MIN_WORD) + txt.print_w(sys.MAX_WORD) + txt.print_uw(sys.MIN_UWORD) + txt.print_uw(sys.MAX_UWORD) + + txt.print_f(floats.EPSILON) + txt.print_f(sys.MIN_FLOAT) + txt.print_f(sys.MAX_FLOAT) + txt.print_f(floats.E) + txt.print_ub(sys.SIZEOF_FLOAT) +} +}""" + + compileText(VMTarget(), optimize=false, src, writeAssembly=false) shouldNotBe null + compileText(Cx16Target(), optimize=false, src, writeAssembly=false) shouldNotBe null + } + + test("merge of float stuff into sys and txt - import order 2") { + val src=""" +%import floats +%import textio + +main { +sub start() { + txt.print_b(sys.MIN_BYTE) + txt.print_b(sys.MAX_BYTE) + txt.print_ub(sys.MIN_UBYTE) + txt.print_ub(sys.MAX_UBYTE) + txt.print_w(sys.MIN_WORD) + txt.print_w(sys.MAX_WORD) + txt.print_uw(sys.MIN_UWORD) + txt.print_uw(sys.MAX_UWORD) + + txt.print_f(floats.EPSILON) + txt.print_f(sys.MIN_FLOAT) + txt.print_f(sys.MAX_FLOAT) + txt.print_f(floats.E) + txt.print_ub(sys.SIZEOF_FLOAT) +} +}""" + + compileText(VMTarget(), optimize=false, src, writeAssembly=false) shouldNotBe null + compileText(Cx16Target(), optimize=false, src, writeAssembly=false) shouldNotBe null + } }) diff --git a/compiler/test/ast/TestProgram.kt b/compiler/test/ast/TestProgram.kt index aafdd6d24..0a309eeca 100644 --- a/compiler/test/ast/TestProgram.kt +++ b/compiler/test/ast/TestProgram.kt @@ -6,7 +6,6 @@ import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.collections.shouldBeIn import io.kotest.matchers.comparables.shouldBeGreaterThan import io.kotest.matchers.shouldBe -import io.kotest.matchers.shouldNotBe import io.kotest.matchers.string.shouldContain import io.kotest.matchers.string.shouldStartWith import io.kotest.matchers.types.shouldBeSameInstanceAs @@ -18,9 +17,10 @@ import prog8.code.core.Position import prog8.code.core.SourceCode import prog8.code.core.internedStringsModuleName import prog8.code.target.C64Target -import prog8.code.target.Cx16Target -import prog8.code.target.VMTarget -import prog8tests.helpers.* +import prog8tests.helpers.DummyFunctions +import prog8tests.helpers.DummyMemsizer +import prog8tests.helpers.DummyStringEncoder +import prog8tests.helpers.compileText class TestProgram: FunSpec({ @@ -113,150 +113,6 @@ class TestProgram: FunSpec({ } } - context("block merge") { - test("merge works") { - val src = """ -%import textio - -main { - - sub start() { - blah.test() - } -} - -txt { - ; merges this block into the txt block coming from the textio library - %option merge - - sub schrijf(str arg) { - print(arg) - } -} - -blah { - ; merges this block into the other 'blah' one - %option merge - - sub test() { - printit("test merge") - } -} - -blah { - sub printit(str arg) { - txt.schrijf(arg) - } -}""" - compileText(C64Target(), optimize=false, src, writeAssembly=false) shouldNotBe null - } - - test("merge override existing subroutine") { - val src=""" -%import textio - -main { - - sub start() { - txt.print("sdfdsf") - } -} - -txt { - %option merge - - sub print(str text) { - cx16.r0++ - ; just some dummy implementation to replace existing print - } -}""" - - val result = compileText(VMTarget(), optimize=false, src, writeAssembly=false) - result shouldNotBe null - } - - test("merge doesn't override existing subroutine if signature differs") { - val src=""" -%import textio - -main { - - sub start() { - txt.print("sdfdsf") - } -} - -txt { - %option merge - - sub print(str anotherparamname) { - cx16.r0++ - ; just some dummy implementation to replace existing print - } -}""" - val errors = ErrorReporterForTests() - compileText(VMTarget(), optimize=false, src, writeAssembly=false, errors = errors) shouldBe null - errors.errors.size shouldBe 1 - errors.errors[0] shouldContain "name conflict" - } - - test("merge of float stuff into sys and txt - import order 1") { - val src=""" -%import textio -%import floats - -main { - sub start() { - txt.print_b(sys.MIN_BYTE) - txt.print_b(sys.MAX_BYTE) - txt.print_ub(sys.MIN_UBYTE) - txt.print_ub(sys.MAX_UBYTE) - txt.print_w(sys.MIN_WORD) - txt.print_w(sys.MAX_WORD) - txt.print_uw(sys.MIN_UWORD) - txt.print_uw(sys.MAX_UWORD) - - txt.print_f(floats.EPSILON) - txt.print_f(sys.MIN_FLOAT) - txt.print_f(sys.MAX_FLOAT) - txt.print_f(floats.E) - txt.print_ub(sys.SIZEOF_FLOAT) - } -}""" - - compileText(VMTarget(), optimize=false, src, writeAssembly=false) shouldNotBe null - compileText(Cx16Target(), optimize=false, src, writeAssembly=false) shouldNotBe null - } - - test("merge of float stuff into sys and txt - import order 2") { - val src=""" -%import floats -%import textio - -main { - sub start() { - txt.print_b(sys.MIN_BYTE) - txt.print_b(sys.MAX_BYTE) - txt.print_ub(sys.MIN_UBYTE) - txt.print_ub(sys.MAX_UBYTE) - txt.print_w(sys.MIN_WORD) - txt.print_w(sys.MAX_WORD) - txt.print_uw(sys.MIN_UWORD) - txt.print_uw(sys.MAX_UWORD) - - txt.print_f(floats.EPSILON) - txt.print_f(sys.MIN_FLOAT) - txt.print_f(sys.MAX_FLOAT) - txt.print_f(floats.E) - txt.print_ub(sys.SIZEOF_FLOAT) - } -}""" - - compileText(VMTarget(), optimize=false, src, writeAssembly=false) shouldNotBe null - compileText(Cx16Target(), optimize=false, src, writeAssembly=false) shouldNotBe null - } - } - test("block sort order") { val src=""" %import textio diff --git a/compilerAst/src/prog8/ast/expressions/AstExpressions.kt b/compilerAst/src/prog8/ast/expressions/AstExpressions.kt index 1882a7b89..4a44709f2 100644 --- a/compilerAst/src/prog8/ast/expressions/AstExpressions.kt +++ b/compilerAst/src/prog8/ast/expressions/AstExpressions.kt @@ -620,13 +620,19 @@ class NumericLiteral(val type: DataType, // only numerical types allowed if(type==targettype) return ValueAfterCast(true, null, this) - if (implicit) { - if (targettype == DataType.BOOL) - return ValueAfterCast(false, "no implicit cast to boolean allowed", this) - if (targettype in IntegerDatatypes && type==DataType.BOOL) - return ValueAfterCast(false, "no implicit cast from boolean to integer allowed", this) + if (implicit && targettype in IntegerDatatypes && type==DataType.BOOL) + return ValueAfterCast(false, "no implicit cast from boolean to integer allowed", this) + + if(targettype == DataType.BOOL) { + return if(implicit) + ValueAfterCast(false, "no implicit cast to boolean allowed", this) + else if(type in NumericDatatypes) + ValueAfterCast(true, null, fromBoolean(number!=0.0, position)) + else + ValueAfterCast(false, "no cast available from $type to BOOL", null) } + when(type) { DataType.UBYTE -> { if(targettype==DataType.BYTE && (number in 0.0..127.0 || !implicit)) @@ -637,8 +643,6 @@ class NumericLiteral(val type: DataType, // only numerical types allowed return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) if(targettype==DataType.LONG) return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) - if(targettype==DataType.BOOL) - return ValueAfterCast(true, null, fromBoolean(number!=0.0, position)) } DataType.BYTE -> { if(targettype==DataType.UBYTE) { @@ -657,8 +661,6 @@ class NumericLiteral(val type: DataType, // only numerical types allowed return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) if(targettype==DataType.FLOAT) return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) - if(targettype==DataType.BOOL) - return ValueAfterCast(true, null, fromBoolean(number!=0.0, position)) if(targettype==DataType.LONG) return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) } @@ -671,8 +673,6 @@ class NumericLiteral(val type: DataType, // only numerical types allowed return ValueAfterCast(true, null, NumericLiteral(targettype, number.toInt().toShort().toDouble(), position)) if(targettype==DataType.FLOAT) return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) - if(targettype==DataType.BOOL) - return ValueAfterCast(true, null, fromBoolean(number!=0.0, position)) if(targettype==DataType.LONG) return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) } @@ -693,8 +693,6 @@ class NumericLiteral(val type: DataType, // only numerical types allowed } if(targettype==DataType.FLOAT) return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) - if(targettype==DataType.BOOL) - return ValueAfterCast(true, null, fromBoolean(number!=0.0, position)) if(targettype==DataType.LONG) return ValueAfterCast(true, null, NumericLiteral(targettype, number, position)) } diff --git a/docs/source/syntaxreference.rst b/docs/source/syntaxreference.rst index 97a9afb0c..dc5616f2b 100644 --- a/docs/source/syntaxreference.rst +++ b/docs/source/syntaxreference.rst @@ -213,6 +213,8 @@ Directives - ``merge`` (in a block) will merge this block's contents into an already existing block with the same name. Can be used to add or override subroutines to an existing library block, for instance. Overriding (monkeypatching) happens only if the signature of the subroutine exactly matches the original subroutine, including the exact names and types of the parameters. + Where blocks with this option are merged into is intricate: it looks for the first other block with the same name that does not have %option merge, + if that can't be found, select the first occurrence regardless. If no other blocks are found, no merge is done. Blocks in libraries are considered first to merge into. - ``splitarrays`` (block or module) makes all word-arrays in this scope lsb/msb split arrays (as if they all have the @split tag). See Arrays. - ``no_symbol_prefixing`` (block or module) makes the compiler *not* use symbol-prefixing when translating prog8 code into assembly. Only use this if you know what you're doing because it could result in invalid assembly code being generated. diff --git a/docs/source/todo.rst b/docs/source/todo.rst index b0ee2da40..4b0efbdca 100644 --- a/docs/source/todo.rst +++ b/docs/source/todo.rst @@ -1,13 +1,9 @@ TODO ==== -import is now order dependent due to merges, you must use this order: -%import textio -%import floats -needs to be fixed. It should be an error for the same block to be declared twice without either -declaration having %option merge, but as long as at least all but one declaration includes the option, -it shouldn't matter where in the list the one without it falls. See unit test breaking in TestProgram - +- fix the aggregate any all errors +- remove this warning INFO library:/prog8lib/shared_floats_functions.p8:9:1: removing unused block 'txt' +- compiler is particularly slow (>2 sec) for compiler/test/comparisons/test_word_splitw_lte.p8 ... diff --git a/examples/test.p8 b/examples/test.p8 index 5df5800ea..fb81add50 100644 --- a/examples/test.p8 +++ b/examples/test.p8 @@ -1,37 +1,4 @@ -%import textio -%import floats -%option no_sysinit -%zeropage basicsafe - main { sub start() { - txt.print_b(sys.MIN_BYTE) - txt.spc() - txt.print_b(sys.MAX_BYTE) - txt.nl() - txt.print_ub(sys.MIN_UBYTE) - txt.spc() - txt.print_ub(sys.MAX_UBYTE) - txt.nl() - txt.print_w(sys.MIN_WORD) - txt.spc() - txt.print_w(sys.MAX_WORD) - txt.nl() - txt.print_uw(sys.MIN_UWORD) - txt.spc() - txt.print_uw(sys.MAX_UWORD) - txt.nl() - txt.nl() - - txt.print_f(floats.EPSILON) - txt.nl() - txt.print_f(sys.MIN_FLOAT) - txt.nl() - txt.print_f(sys.MAX_FLOAT) - txt.nl() - txt.print_f(floats.E) - txt.nl() - txt.print_ub(sys.SIZEOF_FLOAT) - txt.nl() } }