don't split expressions referencing the target variable wrongly

compiler/src/prog8/ast/expressions/AstExpressions.kt

@@ -23,7 +23,7 @@ sealed class Expression: Node {
     abstract fun constValue(program: Program): NumericLiteralValue?
     abstract fun accept(visitor: IAstVisitor)
     abstract fun accept(visitor: AstWalker, parent: Node)
-    abstract fun referencesIdentifiers(vararg name: String): Boolean
+    abstract fun referencesIdentifier(vararg scopedName: String): Boolean
     abstract fun inferType(program: Program): InferredTypes.InferredType
 
     infix fun isSameAs(assigntarget: AssignTarget) = assigntarget.isSameAs(this)
@@ -85,7 +85,7 @@ class PrefixExpression(val operator: String, var expression: Expression, overrid
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String) = expression.referencesIdentifiers(*name)
+    override fun referencesIdentifier(vararg scopedName: String) = expression.referencesIdentifier(*scopedName)
     override fun inferType(program: Program): InferredTypes.InferredType {
         val inferred = expression.inferType(program)
         return when(operator) {
@@ -142,7 +142,7 @@ class BinaryExpression(var left: Expression, var operator: String, var right: Ex
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String) = left.referencesIdentifiers(*name) || right.referencesIdentifiers(*name)
+    override fun referencesIdentifier(vararg scopedName: String) = left.referencesIdentifier(*scopedName) || right.referencesIdentifier(*scopedName)
     override fun inferType(program: Program): InferredTypes.InferredType {
         val leftDt = left.inferType(program)
         val rightDt = right.inferType(program)
@@ -255,7 +255,7 @@ class ArrayIndexedExpression(var identifier: IdentifierReference,
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String) = identifier.referencesIdentifiers(*name)
+    override fun referencesIdentifier(vararg scopedName: String) = identifier.referencesIdentifier(*scopedName)
 
     override fun inferType(program: Program): InferredTypes.InferredType {
         val target = identifier.targetStatement(program.namespace)
@@ -291,7 +291,7 @@ class TypecastExpression(var expression: Expression, var type: DataType, val imp
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String) = expression.referencesIdentifiers(*name)
+    override fun referencesIdentifier(vararg scopedName: String) = expression.referencesIdentifier(*scopedName)
     override fun inferType(program: Program): InferredTypes.InferredType = InferredTypes.knownFor(type)
     override fun constValue(program: Program): NumericLiteralValue? {
         val cv = expression.constValue(program) ?: return null
@@ -322,7 +322,7 @@ data class AddressOf(var identifier: IdentifierReference, override val position:
     }
 
     override fun constValue(program: Program): NumericLiteralValue? = null
-    override fun referencesIdentifiers(vararg name: String) = false
+    override fun referencesIdentifier(vararg scopedName: String) = false
     override fun inferType(program: Program): InferredTypes.InferredType = InferredTypes.knownFor(DataType.UWORD)
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
@@ -345,7 +345,7 @@ class DirectMemoryRead(var addressExpression: Expression, override val position:
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String) = false
+    override fun referencesIdentifier(vararg scopedName: String) = false
     override fun inferType(program: Program): InferredTypes.InferredType = InferredTypes.knownFor(DataType.UBYTE)
     override fun constValue(program: Program): NumericLiteralValue? = null
 
@@ -398,7 +398,7 @@ class NumericLiteralValue(val type: DataType,    // only numerical types allowed
         throw FatalAstException("can't replace here")
     }
 
-    override fun referencesIdentifiers(vararg name: String) = false
+    override fun referencesIdentifier(vararg scopedName: String) = false
     override fun constValue(program: Program) = this
 
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
@@ -498,7 +498,7 @@ class StringLiteralValue(val value: String,
         throw FatalAstException("can't replace here")
     }
 
-    override fun referencesIdentifiers(vararg name: String) = false
+    override fun referencesIdentifier(vararg scopedName: String) = false
     override fun constValue(program: Program): NumericLiteralValue? = null
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
@@ -533,7 +533,7 @@ class ArrayLiteralValue(val type: InferredTypes.InferredType,     // inferred be
         replacement.parent = this
     }
 
-    override fun referencesIdentifiers(vararg name: String) = value.any { it.referencesIdentifiers(*name) }
+    override fun referencesIdentifier(vararg scopedName: String) = value.any { it.referencesIdentifier(*scopedName) }
     override fun constValue(program: Program): NumericLiteralValue? = null
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
@@ -638,7 +638,7 @@ class RangeExpr(var from: Expression,
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String): Boolean  = from.referencesIdentifiers(*name) || to.referencesIdentifiers(*name)
+    override fun referencesIdentifier(vararg scopedName: String): Boolean  = from.referencesIdentifier(*scopedName) || to.referencesIdentifier(*scopedName)
     override fun inferType(program: Program): InferredTypes.InferredType {
         val fromDt=from.inferType(program)
         val toDt=to.inferType(program)
@@ -751,8 +751,8 @@ data class IdentifierReference(val nameInSource: List<String>, override val posi
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String): Boolean =
+    override fun referencesIdentifier(vararg scopedName: String): Boolean =
-            nameInSource.size==name.size && nameInSource.toTypedArray().contentEquals(name)
+            nameInSource.size==scopedName.size && nameInSource.toTypedArray().contentEquals(scopedName)
 
     override fun inferType(program: Program): InferredTypes.InferredType {
         return when (val targetStmt = targetStatement(program.namespace)) {
@@ -840,7 +840,7 @@ class FunctionCall(override var target: IdentifierReference,
     override fun accept(visitor: IAstVisitor) = visitor.visit(this)
     override fun accept(visitor: AstWalker, parent: Node)= visitor.visit(this, parent)
 
-    override fun referencesIdentifiers(vararg name: String): Boolean = target.referencesIdentifiers(*name) || args.any{it.referencesIdentifiers(*name)}
+    override fun referencesIdentifier(vararg scopedName: String): Boolean = target.referencesIdentifier(*scopedName) || args.any{it.referencesIdentifier(*scopedName)}
 
     override fun inferType(program: Program): InferredTypes.InferredType {
         val constVal = constValue(program ,false)

compiler/src/prog8/ast/processing/AstChecker.kt

@@ -476,7 +476,7 @@ internal class AstChecker(private val program: Program,
         fun err(msg: String, position: Position?=null) = errors.err(msg, position ?: decl.position)
 
         // the initializer value can't refer to the variable itself (recursive definition)
-        if(decl.value?.referencesIdentifiers(decl.name) == true || decl.arraysize?.index?.referencesIdentifiers(decl.name) == true)
+        if(decl.value?.referencesIdentifier(decl.name) == true || decl.arraysize?.index?.referencesIdentifier(decl.name) == true)
             err("recursive var declaration")
 
         // CONST can only occur on simple types (byte, word, float)

compiler/src/prog8/compiler/BeforeAsmGenerationAstChanger.kt

@@ -26,16 +26,31 @@ internal class BeforeAsmGenerationAstChanger(val program: Program, val errors: E
     override fun after(assignment: Assignment, parent: Node): Iterable<IAstModification> {
         // Try to replace A = B <operator> Something  by A= B, A = A <operator> Something
         // this triggers the more efficent augmented assignment code generation more often.
+        // But it can only be done if the target variable IS NOT OCCURRING AS AN OPERAND ITSELF.
         if(!assignment.isAugmentable
                 && assignment.target.identifier != null
                 && assignment.target.isNotMemory(program.namespace)) {
             val binExpr = assignment.value as? BinaryExpression
-            if(binExpr!=null && binExpr.operator !in comparisonOperators) {
+            if (binExpr != null && binExpr.operator !in comparisonOperators) {
-                if(binExpr.left !is BinaryExpression) {
+                if (binExpr.left !is BinaryExpression) {
-                    val assignLeft = Assignment(assignment.target, binExpr.left, assignment.position)
+                    if (binExpr.right.referencesIdentifier(*assignment.target.identifier!!.nameInSource.toTypedArray())) {
-                    return listOf(
+                        // the right part of the expression contains the target variable itself.
-                            IAstModification.InsertBefore(assignment, assignLeft, parent),
+                        // we can't 'split' it trivially because the variable will be changed halfway through.
-                            IAstModification.ReplaceNode(binExpr.left, assignment.target.toExpression(), binExpr))
+                        if(binExpr.operator in associativeOperators) {
+                            // A = <something-without-A>  <associativeoperator>  <otherthing-with-A>
+                            // use the other part of the expression to split.
+                            val assignRight = Assignment(assignment.target, binExpr.right, assignment.position)
+                            return listOf(
+                                    IAstModification.InsertBefore(assignment, assignRight, parent),
+                                    IAstModification.ReplaceNode(binExpr.right, binExpr.left, binExpr),
+                                    IAstModification.ReplaceNode(binExpr.left, assignment.target.toExpression(), binExpr))
+                        }
+                    } else {
+                        val assignLeft = Assignment(assignment.target, binExpr.left, assignment.position)
+                        return listOf(
+                                IAstModification.InsertBefore(assignment, assignLeft, parent),
+                                IAstModification.ReplaceNode(binExpr.left, assignment.target.toExpression(), binExpr))
+                    }
                 }
             }
         }

compiler/src/prog8/optimizer/ConstantIdentifierReplacer.kt

@@ -58,7 +58,7 @@ internal class ConstantIdentifierReplacer(private val program: Program, private
     override fun before(decl: VarDecl, parent: Node): Iterable<IAstModification> {
         // the initializer value can't refer to the variable itself (recursive definition)
         // TODO: use call graph for this?
-        if(decl.value?.referencesIdentifiers(decl.name) == true || decl.arraysize?.index?.referencesIdentifiers(decl.name) == true) {
+        if(decl.value?.referencesIdentifier(decl.name) == true || decl.arraysize?.index?.referencesIdentifier(decl.name) == true) {
             errors.err("recursive var declaration", decl.position)
             return noModifications
         }

examples/test.p8

@@ -37,31 +37,13 @@ _saveX   .byte 0
     sub start() {
 ;        byte bb = 100
 ;        word ww = 30000
-        float ff1 = 12345
+        float ff1 = 1000
-        float ff2 = -99999
+        float ff2 = -1000
 
-        ;ff = 1+((-ff) *3)       ; TODO fix invalid splitting (can't split because it references ff itself)
+        ff1 = 1+((-ff1) *3)       ; TODO why is splitting undone when OPTIMIZATION is ON?
-        ;ff = 1+((-ff2) *3)       ; TODO splitting should be okay here
-
-        testX()
-        floats.print_f(ff1)     ; TODO if we remove this, the following calcuation is wrong
-        testX()
-        txt.chrout('\n')
-        testX()
-        ff1 = -ff2 * 3
-        testX()
         floats.print_f(ff1)
         testX()
-        txt.chrout('\n')
+        ff1 = 1+((-ff2) *3)       ; TODO why is splitting undone when OPTIMIZATION is ON?
-        testX()
-
-        ff1 = -ff1 * 3
-        testX()
-        floats.print_f(ff1)
-        testX()
-        txt.chrout('\n')
-
-        ff1 = abs(ff2)
         floats.print_f(ff1)
         txt.chrout('\n')
         testX()