GoSub no longer inherits from Jump node, fixes subtle ast/codegen bugs related to jsrs

codeGeneration/src/prog8/compiler/target/cpu6502/codegen/AsmGen.kt

@@ -775,7 +775,8 @@ class AsmGen(private val program: Program,
                 }
             }
             is Assignment -> assignmentAsmGen.translate(stmt)
-            is Jump -> translate(stmt)
+            is Jump -> jmp(getJumpTarget(stmt))
+            is GoSub -> translate(stmt)
             is PostIncrDecr -> postincrdecrAsmGen.translate(stmt)
             is Label -> translate(stmt)
             is BranchStatement -> translate(stmt)
@@ -1099,7 +1100,7 @@ class AsmGen(private val program: Program,
 
         if (stmt.elsepart.isEmpty()) {
             val jump = stmt.truepart.statements.singleOrNull()
-            if(jump is Jump && !jump.isGosub) {
+            if(jump is Jump) {
                 translateCompareAndJumpIfTrue(booleanCondition, jump)
             } else {
                 val endLabel = makeLabel("if_end")
@@ -1332,7 +1333,7 @@ $repeatLabel    lda  $counterVar
             throw AssemblyError("only else part contains code, shoud have been switched already")
 
         val jump = stmt.truepart.statements.first() as? Jump
-        if(jump!=null && !jump.isGosub) {
+        if(jump!=null) {
             // branch with only a jump (goto)
             val instruction = branchInstruction(stmt.condition, false)
             out("  $instruction  ${getJumpTarget(jump)}")
@@ -1400,21 +1401,16 @@ $label              nop""")
         }
     }
 
-    private fun translate(jump: Jump) {
+    private fun translate(gosub: GoSub) {
-        if(jump.isGosub) {
+        val tgt = gosub.identifier!!.targetSubroutine(program)
-            jump as GoSub
+        if(tgt!=null && tgt.isAsmSubroutine) {
-            val tgt = jump.identifier!!.targetSubroutine(program)
+            // no need to rescue X , this has been taken care of already
-            if(tgt!=null && tgt.isAsmSubroutine) {
+            out("  jsr  ${getJumpTarget(gosub)}")
-                // no need to rescue X , this has been taken care of already
+        } else {
-                out("  jsr  ${getJumpTarget(jump)}")
+            saveXbeforeCall(gosub)
-            } else {
+            out("  jsr  ${getJumpTarget(gosub)}")
-                saveXbeforeCall(jump)
+            restoreXafterCall(gosub)
-                out("  jsr  ${getJumpTarget(jump)}")
-                restoreXafterCall(jump)
-            }
         }
-        else
-            jmp(getJumpTarget(jump))
     }
 
     private fun getJumpTarget(jump: Jump): String {
@@ -1429,6 +1425,18 @@ $label              nop""")
         }
     }
 
+    private fun getJumpTarget(gosub: GoSub): String {
+        val ident = gosub.identifier
+        val label = gosub.generatedLabel
+        val addr = gosub.address
+        return when {
+            ident!=null -> asmSymbolName(ident)
+            label!=null -> label
+            addr!=null -> addr.toHex()
+            else -> "????"
+        }
+    }
+
     private fun translate(ret: Return, withRts: Boolean=true) {
         ret.value?.let { returnvalue ->
             val sub = ret.definingSubroutine!!
@@ -1613,8 +1621,6 @@ $label              nop""")
     }
 
     private fun translateCompareAndJumpIfTrue(expr: BinaryExpression, jump: Jump) {
-        require(!jump.isGosub)
-
         if(expr.operator !in ComparisonOperators)
             throw AssemblyError("must be comparison expression")
 

codeOptimizers/src/prog8/optimizer/StatementOptimizer.kt

@@ -277,24 +277,25 @@ class StatementOptimizer(private val program: Program,
     }
 
     override fun after(jump: Jump, parent: Node): Iterable<IAstModification> {
-        if(jump.isGosub) {
+        // if the jump is to the next statement, remove the jump
-            // if the next statement is return with no returnvalue, change into a regular jump if there are no parameters as well.
+        val scope = jump.parent as IStatementContainer
-            val subroutineParams = jump.identifier?.targetSubroutine(program)?.parameters
+        val label = jump.identifier?.targetStatement(program)
-            if(subroutineParams!=null && subroutineParams.isEmpty()) {
+        if (label != null && scope.statements.indexOf(label) == scope.statements.indexOf(jump) + 1)
-                val returnstmt = jump.nextSibling() as? Return
+            return listOf(IAstModification.Remove(jump, scope))
-                if(returnstmt!=null && returnstmt.value==null) {
+        return noModifications
-                    return listOf(
+    }
-                        IAstModification.Remove(returnstmt, parent as IStatementContainer),
+
-                        IAstModification.ReplaceNode(jump, Jump(jump.address, jump.identifier, jump.generatedLabel, jump.position), parent)
+    override fun after(gosub: GoSub, parent: Node): Iterable<IAstModification> {
-                    )
+        // if the next statement is return with no returnvalue, change into a regular jump if there are no parameters as well.
-                }
+        val subroutineParams = gosub.identifier?.targetSubroutine(program)?.parameters
+        if(subroutineParams!=null && subroutineParams.isEmpty()) {
+            val returnstmt = gosub.nextSibling() as? Return
+            if(returnstmt!=null && returnstmt.value==null) {
+                return listOf(
+                    IAstModification.Remove(returnstmt, parent as IStatementContainer),
+                    IAstModification.ReplaceNode(gosub, Jump(gosub.address, gosub.identifier, gosub.generatedLabel, gosub.position), parent)
+                )
             }
-        } else {
-            // if the jump is to the next statement, remove the jump
-            val scope = jump.parent as IStatementContainer
-            val label = jump.identifier?.targetStatement(program)
-            if (label != null && scope.statements.indexOf(label) == scope.statements.indexOf(jump) + 1)
-                return listOf(IAstModification.Remove(jump, scope))
         }
         return noModifications
     }

codeOptimizers/src/prog8/optimizer/UnusedCodeRemover.kt

@@ -33,8 +33,7 @@ class UnusedCodeRemover(private val program: Program,
     }
 
     override fun before(jump: Jump, parent: Node): Iterable<IAstModification> {
-        if(!jump.isGosub)
+        reportUnreachable(jump)
-            reportUnreachable(jump)
         return emptyList()
     }
 

compiler/src/prog8/compiler/BeforeAsmGenerationAstChanger.kt

@@ -139,7 +139,7 @@ internal class BeforeAsmGenerationAstChanger(val program: Program, private val o
         if (subroutineStmtIdx > 0) {
             val prevStmt = outerStatements[subroutineStmtIdx-1]
                 if(outerScope !is Block
-                    && (prevStmt !is Jump || prevStmt.isGosub)
+                    && (prevStmt !is Jump)
                     && prevStmt !is Subroutine
                     && prevStmt !is Return) {
                 mods += IAstModification.InsertAfter(outerStatements[subroutineStmtIdx - 1], returnStmt, outerScope)

compiler/src/prog8/compiler/astprocessing/AstChecker.kt

@@ -168,7 +168,7 @@ internal class AstChecker(private val program: Program,
                 if(targetStatement is BuiltinFunctionStatementPlaceholder)
                     errors.err("can't jump to a builtin function", jump.position)
             }
-            if(!jump.isGosub && targetStatement is Subroutine && targetStatement.parameters.any()) {
+            if(targetStatement is Subroutine && targetStatement.parameters.any()) {
                 errors.err("can't jump to a subroutine that takes parameters", jump.position)
             }
         }
@@ -179,6 +179,22 @@ internal class AstChecker(private val program: Program,
         super.visit(jump)
     }
 
+    override fun visit(gosub: GoSub) {
+        val ident = gosub.identifier
+        if(ident!=null) {
+            val targetStatement = checkFunctionOrLabelExists(ident, gosub)
+            if(targetStatement!=null) {
+                if(targetStatement is BuiltinFunctionStatementPlaceholder)
+                    errors.err("can't gosub to a builtin function", gosub.position)
+            }
+        }
+
+        val addr = gosub.address
+        if(addr!=null && addr > 65535u)
+            errors.err("gosub address must be valid integer 0..\$ffff", gosub.position)
+        super.visit(gosub)
+    }
+
     override fun visit(block: Block) {
         val addr = block.address
         if(addr!=null && addr>65535u) {
@@ -226,8 +242,7 @@ internal class AstChecker(private val program: Program,
                 count++
             }
             override fun visit(jump: Jump) {
-                if(!jump.isGosub)
+                count++
-                    count++
             }
 
             override fun visit(inlineAssembly: InlineAssembly) {

compiler/src/prog8/compiler/astprocessing/ParentNodeChecker.kt

@@ -119,6 +119,12 @@ internal class ParentNodeChecker: AstWalker() {
         return noModifications
     }
 
+    override fun before(gosub: GoSub, parent: Node): Iterable<IAstModification> {
+        if(gosub.parent!==parent)
+            throw FatalAstException("parent node mismatch at $gosub")
+        return noModifications
+    }
+
     override fun before(label: Label, parent: Node): Iterable<IAstModification> {
         if(label.parent!==parent)
             throw FatalAstException("parent node mismatch at $label")

compiler/test/TestSubroutines.kt

@@ -337,6 +337,6 @@ class TestSubroutines: FunSpec({
 
         stmts.last() shouldBe instanceOf<Subroutine>()
         stmts.dropLast(1).last() shouldBe instanceOf<Return>()  // this prevents the fallthrough
-        stmts.dropLast(2).last() shouldBe instanceOf<Jump>()
+        stmts.dropLast(2).last() shouldBe instanceOf<GoSub>()
     }
 })

compilerAst/src/prog8/ast/AstToSourceTextConverter.kt

@@ -219,10 +219,7 @@ class AstToSourceTextConverter(val output: (text: String) -> Unit, val program:
     }
 
     override fun visit(jump: Jump) {
-        if(jump.isGosub)
+        output("goto ")
-            output("gosub ")
-        else
-            output("goto ")
         when {
             jump.address!=null -> output(jump.address.toHex())
             jump.generatedLabel!=null -> output(jump.generatedLabel)
@@ -230,6 +227,15 @@ class AstToSourceTextConverter(val output: (text: String) -> Unit, val program:
         }
     }
 
+    override fun visit(gosub: GoSub) {
+        output("gosub ")
+        when {
+            gosub.address!=null -> output(gosub.address.toHex())
+            gosub.generatedLabel!=null -> output(gosub.generatedLabel)
+            gosub.identifier!=null -> gosub.identifier.accept(this)
+        }
+    }
+
     override fun visit(ifStatement: IfStatement) {
         output("if ")
         ifStatement.condition.accept(this)

compilerAst/src/prog8/ast/statements/AstStatements.kt

@@ -499,12 +499,11 @@ class PostIncrDecr(var target: AssignTarget, val operator: String, override val
     override fun toString() = "PostIncrDecr(op: $operator, target: $target, pos=$position)"
 }
 
-open class Jump(val address: UInt?,
+class Jump(val address: UInt?,
-                val identifier: IdentifierReference?,
+           val identifier: IdentifierReference?,
-                val generatedLabel: String?,             // can be used in code generation scenarios
+           val generatedLabel: String?,             // can be used in code generation scenarios
-                override val position: Position) : Statement() {
+           override val position: Position) : Statement() {
     override lateinit var parent: Node
-    open val isGosub = false
 
     override fun linkParents(parent: Node) {
         this.parent = parent
@@ -521,11 +520,22 @@ open class Jump(val address: UInt?,
 }
 
 // a GoSub is ONLY created internally for calling subroutines
-class GoSub(address: UInt?, identifier: IdentifierReference?, generatedLabel: String?, position: Position) :
+class GoSub(val address: UInt?,
-    Jump(address, identifier, generatedLabel, position) {
+            val identifier: IdentifierReference?,
+            val generatedLabel: String?,             // can be used in code generation scenarios
+            override val position: Position) : Statement() {
+    override lateinit var parent: Node
 
-    override val isGosub = true
+    override fun linkParents(parent: Node) {
+        this.parent = parent
+        identifier?.linkParents(this)
+    }
+
+    override fun replaceChildNode(node: Node, replacement: Node) = throw FatalAstException("can't replace here")
     override fun copy() = GoSub(address, identifier?.copy(), generatedLabel, position)
+    override fun accept(visitor: IAstVisitor) = visitor.visit(this)
+    override fun accept(visitor: AstWalker, parent: Node) = visitor.visit(this, parent)
+
     override fun toString() =
         "GoSub(addr: $address, identifier: $identifier, label: $generatedLabel;  pos=$position)"
 }

compilerAst/src/prog8/ast/walk/AstWalker.kt

@@ -99,6 +99,7 @@ abstract class AstWalker {
     open fun before(ifStatement: IfStatement, parent: Node): Iterable<IAstModification> = noModifications
     open fun before(inlineAssembly: InlineAssembly, parent: Node): Iterable<IAstModification> = noModifications
     open fun before(jump: Jump, parent: Node): Iterable<IAstModification> = noModifications
+    open fun before(gosub: GoSub, parent: Node): Iterable<IAstModification> = noModifications
     open fun before(label: Label, parent: Node): Iterable<IAstModification> = noModifications
     open fun before(memread: DirectMemoryRead, parent: Node): Iterable<IAstModification> = noModifications
     open fun before(memwrite: DirectMemoryWrite, parent: Node): Iterable<IAstModification> = noModifications
@@ -140,6 +141,7 @@ abstract class AstWalker {
     open fun after(ifStatement: IfStatement, parent: Node): Iterable<IAstModification> = noModifications
     open fun after(inlineAssembly: InlineAssembly, parent: Node): Iterable<IAstModification> = noModifications
     open fun after(jump: Jump, parent: Node): Iterable<IAstModification> = noModifications
+    open fun after(gosub: GoSub, parent: Node): Iterable<IAstModification> = noModifications
     open fun after(label: Label, parent: Node): Iterable<IAstModification> = noModifications
     open fun after(memread: DirectMemoryRead, parent: Node): Iterable<IAstModification> = noModifications
     open fun after(memwrite: DirectMemoryWrite, parent: Node): Iterable<IAstModification> = noModifications
@@ -270,6 +272,12 @@ abstract class AstWalker {
         track(after(jump, parent), jump, parent)
     }
 
+    fun visit(gosub: GoSub, parent: Node) {
+        track(before(gosub, parent), gosub, parent)
+        gosub.identifier?.accept(this, gosub)
+        track(after(gosub, parent), gosub, parent)
+    }
+
     fun visit(ifStatement: IfStatement, parent: Node) {
         track(before(ifStatement, parent), ifStatement, parent)
         ifStatement.condition.accept(this, ifStatement)

compilerAst/src/prog8/ast/walk/IAstVisitor.kt

@@ -56,6 +56,10 @@ interface IAstVisitor {
         jump.identifier?.accept(this)
     }
 
+    fun visit(gosub: GoSub) {
+        gosub.identifier?.accept(this)
+    }
+
     fun visit(ifStatement: IfStatement) {
         ifStatement.condition.accept(this)
         ifStatement.truepart.accept(this)

compilerInterfaces/src/prog8/compilerinterface/CallGraph.kt

@@ -103,6 +103,17 @@ class CallGraph(private val program: Program) : IAstVisitor {
         super.visit(jump)
     }
 
+    override fun visit(gosub: GoSub) {
+        val otherSub = gosub.identifier?.targetSubroutine(program)
+        if (otherSub != null) {
+            gosub.definingSubroutine?.let { thisSub ->
+                calls[thisSub] = calls.getValue(thisSub) + otherSub
+                calledBy[otherSub] = calledBy.getValue(otherSub) + gosub
+            }
+        }
+        super.visit(gosub)
+    }
+
     override fun visit(identifier: IdentifierReference) {
         allIdentifiersAndTargets[Pair(identifier, identifier.position)] = identifier.targetStatement(program)!!
     }

docs/source/todo.rst

@@ -3,9 +3,7 @@ TODO
 
 For next compiler release (7.6)
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-fix imageviewer crashes...
+...
-also textelite: after showing map, it continues to show the market without keyboard entry...
-
 
 Blocked by an official Commander-x16 v39 release
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^