From bc89306dc156d49dc0cf83096ffbf6b06d9c0528 Mon Sep 17 00:00:00 2001
From: Irmen de Jong <irmen@razorvine.net>
Date: Sat, 19 Sep 2020 15:46:51 +0200
Subject: [PATCH] better detection of duplicate variable definitions

---
 .../compiler/BeforeAsmGenerationAstChanger.kt | 52 ++++++++++++-------
 examples/cube3d-gfx.p8                        |  2 +-
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/compiler/src/prog8/compiler/BeforeAsmGenerationAstChanger.kt b/compiler/src/prog8/compiler/BeforeAsmGenerationAstChanger.kt
index fee195676..fe19849fd 100644
--- a/compiler/src/prog8/compiler/BeforeAsmGenerationAstChanger.kt
+++ b/compiler/src/prog8/compiler/BeforeAsmGenerationAstChanger.kt
@@ -15,6 +15,7 @@ internal class BeforeAsmGenerationAstChanger(val program: Program, val errors: E
     private val noModifications = emptyList<IAstModification>()
 
     override fun after(decl: VarDecl, parent: Node): Iterable<IAstModification> {
+        subroutineVariables.add(Pair(decl.name, decl))
         if (decl.value == null && !decl.autogeneratedDontRemove && decl.type == VarDeclType.VAR && decl.datatype in NumericDatatypes) {
             // a numeric vardecl without an initial value is initialized with zero.
             decl.value = decl.zeroElementValue()
@@ -41,36 +42,47 @@ internal class BeforeAsmGenerationAstChanger(val program: Program, val errors: E
         return noModifications
     }
 
+    private val subroutineVariables = mutableListOf<Pair<String, VarDecl>>()
+
+    override fun before(subroutine: Subroutine, parent: Node): Iterable<IAstModification> {
+        subroutineVariables.clear()
+        return noModifications
+    }
+
     override fun after(scope: AnonymousScope, parent: Node): Iterable<IAstModification> {
         val decls = scope.statements.filterIsInstance<VarDecl>()
+        subroutineVariables.addAll(decls.map { Pair(it.name, it) })
+
         val sub = scope.definingSubroutine()
         if (sub != null) {
-            val existingVariables = sub.statements.filterIsInstance<VarDecl>().associateBy { it.name }
-            var conflicts = false
-            decls.forEach {
-                val existing = existingVariables[it.name]
-                if (existing != null) {
-                    errors.err("variable ${it.name} already defined in subroutine ${sub.name} at ${existing.position}", it.position)
-                    conflicts = true
-                }
-            }
-            if (!conflicts) {
-                // move vardecls of the scope into the upper scope. Make sure the position remains the same!
-                val numericVarsWithValue = decls.filter { it.value != null && it.datatype in NumericDatatypes }
-                return numericVarsWithValue.map {
-                    val initValue = it.value!!  // assume here that value has always been set by now
-                    it.value = null     // make sure no value init assignment for this vardecl will be created later (would be superfluous)
-                    val target = AssignTarget(IdentifierReference(listOf(it.name), it.position), null, null, it.position)
-                    val assign = Assignment(target, initValue, it.position)
-                    initValue.parent = assign
-                    IAstModification.ReplaceNode(it, assign, scope)
-                } + decls.map { IAstModification.InsertFirst(it, sub) }    // move it up to the subroutine
+            // move vardecls of the scope into the upper scope. Make sure the position remains the same!
+            val numericVarsWithValue = decls.filter { it.value != null && it.datatype in NumericDatatypes }
+            val replaceVardecls =numericVarsWithValue.map {
+                val initValue = it.value!!  // assume here that value has always been set by now
+                it.value = null     // make sure no value init assignment for this vardecl will be created later (would be superfluous)
+                val target = AssignTarget(IdentifierReference(listOf(it.name), it.position), null, null, it.position)
+                val assign = Assignment(target, initValue, it.position)
+                initValue.parent = assign
+                IAstModification.ReplaceNode(it, assign, scope)
             }
+            val moveVardeclsUp = decls.map { IAstModification.InsertFirst(it, sub) }
+            return replaceVardecls + moveVardeclsUp
         }
         return noModifications
     }
 
     override fun after(subroutine: Subroutine, parent: Node): Iterable<IAstModification> {
+        val firstDeclarations = mutableMapOf<String, VarDecl>()
+        for(decl in subroutineVariables) {
+            val existing = firstDeclarations[decl.first]
+            if(existing!=null && existing !== decl.second) {
+                errors.err("variable ${decl.first} already defined in subroutine ${subroutine.name} at ${existing.position}", decl.second.position)
+            } else {
+                firstDeclarations[decl.first] = decl.second
+            }
+        }
+
+
         // add the implicit return statement at the end (if it's not there yet), but only if it's not a kernel routine.
         // and if an assembly block doesn't contain a rts/rti, and some other situations.
         val mods = mutableListOf<IAstModification>()
diff --git a/examples/cube3d-gfx.p8 b/examples/cube3d-gfx.p8
index 3704f89d5..1498aa5cd 100644
--- a/examples/cube3d-gfx.p8
+++ b/examples/cube3d-gfx.p8
@@ -82,7 +82,7 @@ main {
         ubyte @zp i
         for i in len(edgesFrom) -1 downto 0 {
             ubyte @zp vFrom = edgesFrom[i]
-            ubyte @zp vTo = edgesTo[i]              ; TODO need compiler error for double declaration if also declared outside the for loop!
+            ubyte @zp vTo = edgesTo[i]
             word @zp persp1 = 256 + rotatedz[vFrom]/256
             word @zp persp2 = 256 + rotatedz[vTo]/256
             graphics.line(rotatedx[vFrom] / persp1 + screen_width/2 as uword,