fix import order problem related to %option merge

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,
+    // Move everything into the first other block with the same name that does not have %option merge
-    // will be joined into a block with the same name, coming from a library.
+    // (if there is no block without that option, just select the first occurrence)
-    // (or a normal block if no library block with that name was found)
+    // 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<Block>()        // 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()) {
+            if("merge" in block.options() && block.isNotEmpty()) {
-                // move everything into the first other block with the same name
+                val libraryBlockCandidates =
-                // and remove the source block altogether
+                    allBlocks.filter { it !== block && it.isInLibrary && it.name == block.name }
-                val libraryBlock = allBlocks.firstOrNull { it !== block && it.isInLibrary && it.name==block.name }
+                val (withMerge, withoutMerge) = libraryBlockCandidates.partition { "merge" in it.options() }
-                if(libraryBlock!=null) {
+                if (withoutMerge.isNotEmpty() && withoutMerge.any { it !in mergedBlocks}) {
-                    merge(block, libraryBlock)
+                    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 }
+                    val regularBlockCandidates = allBlocks.filter { it !== block && it.name == block.name }
-                    if(regularBlock!=null) {
+                    val (withMerge, withoutMerge) = regularBlockCandidates.partition { "merge" in it.options() }
-                        merge(block, regularBlock)
+                    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 })
                     }
                 }
             }

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
+    }
 })

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 prog8tests.helpers.DummyFunctions
-import prog8.code.target.VMTarget
+import prog8tests.helpers.DummyMemsizer
-import prog8tests.helpers.*
+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

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 (implicit && targettype in IntegerDatatypes && type==DataType.BOOL)
-            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(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))
             }

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.

docs/source/todo.rst

@@ -1,13 +1,9 @@
 TODO
 ====
 
-import is now order dependent due to merges, you must use this order:
+- fix the aggregate any all errors
-%import textio
+- remove this warning INFO  library:/prog8lib/shared_floats_functions.p8:9:1: removing unused block 'txt'
-%import floats
+- compiler is particularly slow (>2 sec) for compiler/test/comparisons/test_word_splitw_lte.p8
-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
-
 
 ...
 

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()
     }
 }