version 12.3.3

Also error on a variable declaration directly followed by another statement on the same line.

fixes #235

codeGenCpu6502/src/prog8/codegen/cpu6502/AsmGen.kt

@@ -2028,7 +2028,11 @@ $repeatLabel""")
         if(asm.isIR)
             throw AssemblyError("%asm containing IR code cannot be translated to 6502 assembly")
         else
-            assembly.add(asm.assembly.trimEnd().trimStart('\r', '\n'))
+            // User-supplied assembly should be passed through verbatim. It may contain
+            // characters such as '|' (64tass bitwise OR) that out() would otherwise
+            // misinterpret as a statement separator, and the user's indentation/spacing
+            // should be preserved rather than re-tabbed.
+            assembly.add(asm.assembly.trim('\r', '\n'))
     }
 
     private fun translate(incbin: PtIncludeBinary) {

codeGenCpu6502/src/prog8/codegen/cpu6502/AsmOptimizer.kt

@@ -169,6 +169,18 @@ internal fun String.hasInstr(mnemonic: String): Boolean {
     return false
 }
 
+private fun replaceMnemonic(raw: String, oldMnemonic: String, newMnemonic: String): String {
+    val trimmed = raw.trimStart()
+    val instruction = trimmed.instructionPart()
+    if(!instruction.startsWith(oldMnemonic))
+        return raw.replaceFirst(oldMnemonic, newMnemonic)
+    val instrStartInTrimmed = trimmed.indexOf(instruction)
+    if(instrStartInTrimmed<0) return raw.replaceFirst(oldMnemonic, newMnemonic)
+    val leadingLen = raw.length - trimmed.length
+    val idxInRaw = leadingLen + instrStartInTrimmed
+    return raw.substring(0, idxInRaw) + newMnemonic + raw.substring(idxInRaw + oldMnemonic.length)
+}
+
 /** Extracts the instruction part of a trimmed assembly line, skipping any label prefix.
  *  For "mylabel: lda  #1" returns "lda  #1". For "lda  #1" returns "lda  #1". */
 private fun String.instructionPart(): String {
@@ -673,7 +685,8 @@ private fun optimizeJsrRtsAndOtherCombinations(linesByFour: Sequence<List<Trimme
             }
             else if (tfirst.hasInstr("jsr") && tsecond.hasInstr("rts")) {
                 if(!tfirst.contains("floats.pushFAC") && !tfirst.contains("floats.popFAC")) {       // these 2 routines depend on being called with JSR!!
-                    mods += Modification(lines[0].index, false, lines[0].value.replace("jsr", "jmp"))
+                    val replacement = replaceMnemonic(lines[0].value, "jsr", "jmp")
+                    mods += Modification(lines[0].index, false, replacement)
                     mods += Modification(lines[1].index, true, null)
                 }
             }
@@ -750,7 +763,7 @@ private fun optimizeJsrRtsAndOtherCombinations(linesByFour: Sequence<List<Trimme
             val firstMnemonic = tfirst.take(3)
             branchInversions[firstMnemonic]?.let { invertedBranch ->
                 if (sameLabel(tfirst, tsecond, tthird)) {
-                    val branch = lines[1].value.replace("jmp", invertedBranch)
+                    val branch = replaceMnemonic(lines[1].value, "jmp", invertedBranch)
                     mods.add(Modification(lines[0].index, true, null))
                     mods.add(Modification(lines[1].index, false, branch))
                 }

codeGenCpu6502/test/TestAsmOptimizer.kt

@@ -160,6 +160,20 @@ class TestAsmOptimizer: FunSpec({
         lines shouldBe listOf("  jsr  floats.pushFAC", "  rts", "  nop", "  nop")
     }
 
+    test("optimizeJsrRts: three consecutive jsr only tail jsr+rts becomes jmp") {
+        val lines = mutableListOf("  jsr  \$4000", "  jsr  \$5000", "  jsr  \$6000", "  rts", "  nop", "  nop")
+        optimize(lines)
+        lines shouldBe listOf("  jsr  \$4000", "  jsr  \$5000", "  jmp  \$6000", "  nop", "  nop")
+    }
+
+    test("optimizeJsrRts: single line with embedded newlines only tail converted defensively") {
+        val lines = mutableListOf("  jsr  \$4000\n  jsr  \$5000\n  jsr  \$6000", "  rts", "  nop", "  nop")
+        optimize(lines)
+        // Before fix this became jmp/jmp/jmp via replace("jsr","jmp"); after fix only one jmp via replaceMnemonic
+        val jmpCount = lines.joinToString("\n").split("jmp").size - 1
+        jmpCount shouldBe 1
+    }
+
     // --- optimizeUselessPushPopStack ---
 
     test("optimizeUselessPushPop: removes phy+ldy+pla") {

codeOptimizers/src/prog8/optimizer/Inliner.kt

@@ -11,6 +11,8 @@ import prog8.code.target.VMTarget
 
 private  fun isEmptyReturn(stmt: Statement): Boolean = stmt is Return && stmt.values.isEmpty()
 
+private class CannotInlineException : Exception()
+
 /**
  * The Inliner performs subroutine inlining on the AST.
  *
@@ -87,6 +89,13 @@ class Inliner(private val program: Program, private val options: CompilationOpti
             }
 
             fun isBodyInlineable(stmt: Assignment): Boolean {
+                // Never auto-inline a sub that assigns directly to one of its parameters.
+                // Substituting the call argument into the assignment target would either
+                // crash (literal argument has no identifier to assign to) or change semantics
+                // (assigning to the caller's variable instead of the local parameter copy).
+                val targetName = stmt.target.identifier?.nameInSource?.lastOrNull()
+                if (targetName != null && subroutine.parameters.any { it.name == targetName })
+                    return false
                 return if (stmt.value.isSimple) {
                     val targetInline =
                         if (stmt.target.identifier != null) {
@@ -139,7 +148,7 @@ class Inliner(private val program: Program, private val options: CompilationOpti
                     stmt.target is NumericLiteral
             }
 
-            if (!subroutine.isAsmSubroutine && subroutine.parameters.size <= 1) {
+            if (!subroutine.isAsmSubroutine && subroutine.parameters.size <= 1 && subroutine.parameters.all { it.registerOrPair == null }) {
                 val containsSubsOrVariables = subroutine.statements.any { it is Subroutine || (it is VarDecl && it.origin != VarDeclOrigin.SUBROUTINEPARAM) }
                 if (!containsSubsOrVariables) {
                     val functionalStatements = subroutine.statements.filter { it !is VarDecl || it.origin != VarDeclOrigin.SUBROUTINEPARAM }
@@ -254,11 +263,23 @@ class Inliner(private val program: Program, private val options: CompilationOpti
             return false to "subroutines with more than 1 parameter cannot be inlined in this compiler version"
         }
 
+        if (sub.parameters.any { it.registerOrPair != null }) {
+            return false to "subroutines with register-mapped parameters cannot be inlined"
+        }
+
         if (sub.parameters.size == 1) {
             val arg = fcall.args.first()
             if (!arg.isSimple) {
                 return false to "argument is too complex to inline (might have side effects)"
             }
+            // Refuse to inline a sub that assigns directly to its parameter.
+            // Substituting the argument into the target would crash for a literal
+            // argument, or wrongly modify the caller's variable for a variable argument
+            // (parameters are passed by value, so the sub only modifies its local copy).
+            val paramName = sub.parameters[0].name
+            val bodyStmt = sub.statements.firstOrNull { it !is VarDecl || it.origin != VarDeclOrigin.SUBROUTINEPARAM }
+            if ((bodyStmt as? Assignment)?.target?.identifier?.nameInSource?.lastOrNull() == paramName)
+                return false to "cannot inline subroutine that assigns to its parameter"
         }
         
         if (options.compTarget.name != VMTarget.NAME) {
@@ -304,8 +325,9 @@ class Inliner(private val program: Program, private val options: CompilationOpti
         fun inlineFunctionBody(toInline: Return): Iterable<AstModification> {
             // call site is an expression, so we have to have a Return here in the inlined sub to provide the values
             return if (toInline.values.size == 1 && functionCallExpr !== toInline.values[0]) {
+                val substitutedReturn = substituteParameters(sub, functionCallExpr, toInline.values[0]) as? Expression
+                    ?: return noModifications
                 sub.hasBeenInlined = true
-                val substitutedReturn = substituteParameters(sub, functionCallExpr, toInline.values[0]) as Expression
                 listOf(AstReplaceNode(functionCallExpr, substitutedReturn, parent))
             } else
                 noModifications
@@ -326,7 +348,8 @@ class Inliner(private val program: Program, private val options: CompilationOpti
 
         fun possiblyShortCircuitFunctionCall(toInline: Return): Iterable<AstModification> {
             // Substitute parameters in the return values first
-            val substitutedReturn = substituteParameters(sub, origNode as IFunctionCall, toInline) as Return
+            val substitutedReturn = substituteParameters(sub, origNode as IFunctionCall, toInline) as? Return
+                ?: return noModifications
             val functionCalls = substitutedReturn.values.filterIsInstance<FunctionCallExpression>()
 
             if (functionCalls.isEmpty()) {
@@ -353,7 +376,8 @@ class Inliner(private val program: Program, private val options: CompilationOpti
         }
 
         fun possiblyInlineFunctionBody(toInline: Statement): Iterable<AstModification> {
-            val inlinedStatement = substituteParameters(sub, origNode as IFunctionCall, toInline) as Statement
+            val inlinedStatement = substituteParameters(sub, origNode as IFunctionCall, toInline) as? Statement
+                ?: return noModifications
 
             return if (origNode !== toInline) {
                 sub.hasBeenInlined = true
@@ -425,7 +449,8 @@ class Inliner(private val program: Program, private val options: CompilationOpti
             return noModifications
 
         // Substitute parameters in all return values
-        val substitutedReturn = substituteParameters(sub, fcall, toInline) as Return
+        val substitutedReturn = substituteParameters(sub, fcall, toInline) as? Return
+            ?: return noModifications
 
         // Create multiple single assignments, skipping void targets (they discard the value)
         val newAssignments = multiTargets.zip(substitutedReturn.values)
@@ -452,7 +477,7 @@ class Inliner(private val program: Program, private val options: CompilationOpti
         return listOf(AstReplaceNode(assignment, scope, parent))
     }
 
-    private fun substituteParameters(sub: Subroutine, fcall: IFunctionCall, node: Node): Node {
+    private fun substituteParameters(sub: Subroutine, fcall: IFunctionCall, node: Node): Node? {
         val paramVarDecls = sub.parameters.map { param ->
             sub.statements.filterIsInstance<VarDecl>().find { it.origin == VarDeclOrigin.SUBROUTINEPARAM && it.name == param.name }
                 ?: throw FatalAstException("parameter ${param.name} not found in subroutine ${sub.name}")
@@ -475,7 +500,7 @@ class Inliner(private val program: Program, private val options: CompilationOpti
                 is ArrayIndexedExpression -> ArrayIndexedExpression(
                     n.plainarrayvar?.let {
                         val subId = substitute(it)
-                        if (subId !is IdentifierReference) throw FatalAstException("parameter substituted by ${subId::class.simpleName} but an identifier reference was expected")
+                        if (subId !is IdentifierReference) throw CannotInlineException()
                         subId
                     },
                     n.nestedArray?.let { substitute(it) as ArrayIndexedExpression },
@@ -489,7 +514,7 @@ class Inliner(private val program: Program, private val options: CompilationOpti
                     val newTarget = n.target.copy()
                     newTarget.identifier = n.target.identifier?.let {
                         val subId = substitute(it)
-                        if (subId !is IdentifierReference) throw FatalAstException("parameter substituted by ${subId::class.simpleName} but an identifier reference was expected")
+                        if (subId !is IdentifierReference) throw CannotInlineException()
                         subId
                     }
                     newTarget.memoryAddress = n.target.memoryAddress?.let { DirectMemoryWrite(substitute(it.addressExpression) as Expression, it.position) }
@@ -512,7 +537,7 @@ class Inliner(private val program: Program, private val options: CompilationOpti
                 }
                 is FunctionCallExpression -> FunctionCallExpression(
                     substitute(n.target).let {
-                        if (it !is IdentifierReference) throw FatalAstException("parameter substituted by ${it::class.simpleName} but an identifier reference was expected")
+                        if (it !is IdentifierReference) throw CannotInlineException()
                         it
                     },
                     n.args.map { substitute(it) as Expression }.toMutableList(),
@@ -520,7 +545,7 @@ class Inliner(private val program: Program, private val options: CompilationOpti
                 )
                 is FunctionCallStatement -> FunctionCallStatement(
                     substitute(n.target).let {
-                        if (it !is IdentifierReference) throw FatalAstException("parameter substituted by ${it::class.simpleName} but an identifier reference was expected")
+                        if (it !is IdentifierReference) throw CannotInlineException()
                         it
                     },
                     n.args.map { substitute(it) as Expression }.toMutableList(),
@@ -536,7 +561,7 @@ class Inliner(private val program: Program, private val options: CompilationOpti
                 is AddressOf -> AddressOf(
                     n.identifier?.let {
                         val subId = substitute(it)
-                        if (subId !is IdentifierReference) throw FatalAstException("parameter substituted by ${subId::class.simpleName} but an identifier reference was expected")
+                        if (subId !is IdentifierReference) throw CannotInlineException()
                         subId
                     },
                     n.arrayIndex?.let { substitute(it) as ArrayIndex },
@@ -549,6 +574,10 @@ class Inliner(private val program: Program, private val options: CompilationOpti
             }
         }
 
-        return substitute(node)
+        return try {
+            substitute(node)
+        } catch (_: CannotInlineException) {
+            null
+        }
     }
 }

compiler/src/prog8/compiler/Compiler.kt

@@ -538,6 +538,8 @@ internal fun determineCompilationOptions(program: Program, compTarget: ICompilat
 }
 
 private fun processAst(program: Program, errors: IErrorReporter, compilerOptions: CompilationOptions) {
+    program.checkVarDeclsOnOwnLine(errors)
+    errors.report()
     program.preprocessAst(errors, compilerOptions)
     if(errors.noErrors() && compilerOptions.dumpSymbols) {
         printSymbols(program)

compiler/src/prog8/compiler/astprocessing/AstExtensions.kt

@@ -5,10 +5,7 @@ import prog8.ast.Node
 import prog8.ast.Program
 import prog8.ast.expressions.*
 import prog8.ast.statements.*
-import prog8.ast.walk.AstModification
-import prog8.ast.walk.AstRemove
-import prog8.ast.walk.AstReplaceNode
-import prog8.ast.walk.AstWalker
+import prog8.ast.walk.*
 import prog8.code.core.*
 import java.io.CharConversionException
 
@@ -24,8 +21,47 @@ internal fun invalidNumberOfArgsError(errors: IErrorReporter, pos: Position, num
         errors.err("invalid number of arguments: expected $expected but got $numArgs", pos)
 }
 
-internal fun Program.checkValid(errors: IErrorReporter, compilerOptions: CompilationOptions) {
-    // semantic analysis to see if the program is valid.
+internal fun Program.checkVarDeclsOnOwnLine(errors: IErrorReporter) {
+    // A variable declaration directly followed by another statement on the same
+    // source line (e.g. `ubyte length foo(42)`) parses as two statements, but it is
+    // almost certainly a missing '=' or newline and very confusing, so report it.
+    // This must run on the freshly parsed tree: later passes split off initializer
+    // assignments and remove unused variables, which destroys the original adjacency.
+    fun checkStatements(statements: List<Statement>) {
+        for (i in 0 until statements.size - 1) {
+            val cur = statements[i] as? VarDecl ?: continue
+            if (cur.origin != VarDeclOrigin.USERCODE) continue
+            val next = statements[i + 1]
+            val curPos = cur.position
+            val nextPos = next.position
+            if (curPos.file == nextPos.file && curPos.line != 0 && curPos.line == nextPos.line && nextPos.startCol > curPos.endCol) {
+                errors.err("variable declaration must be on its own line (did you forget '=' or a newline?)", nextPos)
+            }
+        }
+    }
+
+    val checker = object : IAstVisitor {
+        override fun visit(block: Block) {
+            checkStatements(block.statements)
+            super.visit(block)
+        }
+        override fun visit(subroutine: Subroutine) {
+            checkStatements(subroutine.statements)
+            super.visit(subroutine)
+        }
+        override fun visit(scope: AnonymousScope) {
+            checkStatements(scope.statements)
+            super.visit(scope)
+        }
+        override fun visit(decl: VarDecl) {
+            // Do not traverse into the declaration: its value can still be an
+            // ExpressionTuple at this stage, which cannot be visited.
+        }
+    }
+    checker.visit(this)
+}
+
+internal fun Program.checkValid(errors: IErrorReporter, compilerOptions: CompilationOptions) {    // semantic analysis to see if the program is valid.
     val parentChecker = ParentNodeChecker()
     parentChecker.visit(this)
     val checker = AstChecker(this, errors, compilerOptions)

compiler/test/ast/TestAstChecks.kt

@@ -694,4 +694,55 @@ main {
         compileText(VMTarget(), false, src, outputDir, errors) shouldBe null
         errors.printedErrors.any { it.contains("2D arrays are not allowed as struct fields") } shouldBe true
     }
+
+    test("variable declaration followed by statement on same line should give error") {
+        val src = """
+            main {
+                sub start() {
+                    ubyte length foo(42)
+                }
+                sub foo(ubyte x) {
+                    x++
+                }
+            }
+        """.trimIndent()
+        val errors = ErrorReporterForTests()
+        compileText(VMTarget(), false, src, outputDir, errors) shouldBe null
+        errors.errors.size shouldBe 1
+        errors.errors[0] shouldContain "must be on its own line"
+    }
+
+    test("variable declaration followed by statement on next line is fine") {
+        val src = """
+            main {
+                sub start() {
+                    ubyte length
+                    length = 42
+                    foo(length)
+                }
+                sub foo(ubyte x) {
+                    x++
+                }
+            }
+        """.trimIndent()
+        val errors = ErrorReporterForTests()
+        compileText(VMTarget(), false, src, outputDir, errors, false) shouldNotBe null
+        errors.errors.size shouldBe 0
+    }
+
+    test("two statements on same line is fine if first one is not a variable declaration") {
+        val src = """
+            main {
+                sub start() {
+                    foo(1) foo(2)
+                }
+                sub foo(ubyte x) {
+                    cx16.r0 = x
+                }
+            }
+        """.trimIndent()
+        val errors = ErrorReporterForTests()
+        compileText(VMTarget(), false, src, outputDir, errors, false) shouldNotBe null
+        errors.errors.size shouldBe 0
+    }
 })

compiler/test/codegeneration/TestVariousCodeGen.kt

@@ -643,6 +643,54 @@ main {
         lines.any { it.startsWith("jsr") && it.contains("p8s_test") } shouldBe true
     }
 
+    test("inline asm pipe operator is preserved as bitwise OR") {
+        val src="""
+%zeropage basicsafe
+%option no_sysinit
+main {
+    sub start() {
+        %asm {{
+            lda  #%00010000 | $1234>>16
+        }}
+    }
+}"""
+        val result = compileText(C64Target(), false, src, outputDir, writeAssembly = true)!!
+        val asm = result.compilationOptions.outputDir.resolve(result.compilerAst.name + ".asm").readText()
+        val lines = asm.lines().map { it.trim() }
+        // the '|' must remain on the same line as the lda, not be split into a second line
+        lines.any { it == "lda  #%00010000 | $1234>>16" } shouldBe true
+        lines.any { it == "$1234>>16" } shouldBe false
+    }
+
+    test("subroutine with register-mapped parameter is not inlined") {
+        val src= $$"""
+%zeropage basicsafe
+%option no_sysinit
+main {
+    sub start() {
+        PlayVideo_STD(mkword($10, $20))
+    }
+
+    sub PlayVideo_STD(uword params @R0) {
+        TrampolineCallSTD($a000)
+    }
+
+    asmsub TrampolineCallSTD(uword addr @AY) clobbers(A, Y) {
+        %asm {{
+            rts
+        }}
+    }
+}"""
+        val result = compileText(C64Target(), false, src, outputDir, writeAssembly = true)!!
+        val asm = result.compilationOptions.outputDir.resolve(result.compilerAst.name + ".asm").readText()
+        val lines = asm.lines().map { it.trim() }
+        // the R0 argument setup must be preserved because the inlined body doesn't
+        // explicitly reference the register-mapped parameter
+        lines.any { it == "sta  cx16.r0" } shouldBe true
+        lines.any { it == "sty  cx16.r0+1" } shouldBe true
+        lines.any { it.startsWith("jsr") && it.contains("p8s_PlayVideo_STD") } shouldBe true
+    }
+
     test("if not without else is not swapped") {
         val src="""
 main {

gradle.properties

@@ -4,4 +4,4 @@ org.gradle.parallel=true
 org.gradle.daemon=true
 org.gradle.configuration-cache=false
 kotlin.code.style=official
-version=12.3.2
+version=12.3.3