From ff1294207e90d0718656f45d957e6518c5e803a9 Mon Sep 17 00:00:00 2001
From: Irmen de Jong <irmen@razorvine.net>
Date: Tue, 2 Jul 2019 00:06:40 +0200
Subject: [PATCH] improved parameter name shadowing check

---
 compiler/src/prog8/ast/AST.kt                 | 15 ++++-
 .../src/prog8/ast/AstIdentifiersChecker.kt    | 18 ++++--
 compiler/src/prog8/ast/StmtReorderer.kt       |  4 +-
 examples/test.p8                              | 64 ++++---------------
 4 files changed, 38 insertions(+), 63 deletions(-)

diff --git a/compiler/src/prog8/ast/AST.kt b/compiler/src/prog8/ast/AST.kt
index 8dee3526e..dab8bd641 100644
--- a/compiler/src/prog8/ast/AST.kt
+++ b/compiler/src/prog8/ast/AST.kt
@@ -429,7 +429,7 @@ interface INameScope {
     }
 
     fun getLabelOrVariable(name: String): IStatement? {
-        // this is called A LOT and could perhaps be optimized a bit more, but adding a cache didn't make much of a practical runtime difference
+        // TODO this is called A LOT and could perhaps be optimized a bit more, but adding a cache didn't make much of a practical runtime difference
         for (stmt in statements) {
             if (stmt is VarDecl && stmt.name==name) return stmt
             if (stmt is Label && stmt.name==name) return stmt
@@ -437,8 +437,17 @@ interface INameScope {
         return null
     }
 
-    fun allDefinedNames(): Set<String> =
-            statements.filterIsInstance<Label>().map { it.name }.toSet() + statements.filterIsInstance<VarDecl>().map { it.name }.toSet()
+    fun allDefinedSymbols(): List<Pair<String, IStatement>>  {
+        return statements.mapNotNull {
+            when (it) {
+                is Label -> it.name to it
+                is VarDecl -> it.name to it
+                is Subroutine -> it.name to it
+                is Block -> it.name to it
+                else -> null
+            }
+        }
+    }
 
     fun lookup(scopedName: List<String>, localContext: Node) : IStatement? {
         if(scopedName.size>1) {
diff --git a/compiler/src/prog8/ast/AstIdentifiersChecker.kt b/compiler/src/prog8/ast/AstIdentifiersChecker.kt
index 20fff6ed8..6b1bad34b 100644
--- a/compiler/src/prog8/ast/AstIdentifiersChecker.kt
+++ b/compiler/src/prog8/ast/AstIdentifiersChecker.kt
@@ -114,14 +114,18 @@ private class AstIdentifiersChecker(private val namespace: INameScope) : IAstPro
             if (existing != null && existing !== subroutine)
                 nameError(subroutine.name, subroutine.position, existing)
 
-            // check that there are no local variables that redefine the subroutine's parameters
-            val allDefinedNames = subroutine.allDefinedNames()
+            // check that there are no local variables, labels, or other subs that redefine the subroutine's parameters
+            val symbolsInSub = subroutine.allDefinedSymbols()
+            val namesInSub = symbolsInSub.map{ it.first }.toSet()
             val paramNames = subroutine.parameters.map { it.name }.toSet()
-            val paramsToCheck = paramNames.intersect(allDefinedNames)
+            val paramsToCheck = paramNames.intersect(namesInSub)
             for(name in paramsToCheck) {
-                val thing = subroutine.getLabelOrVariable(name)!!
-                if(thing.position != subroutine.position)
-                    nameError(name, thing.position, subroutine)
+                val labelOrVar = subroutine.getLabelOrVariable(name)
+                if(labelOrVar!=null && labelOrVar.position != subroutine.position)
+                    nameError(name, labelOrVar.position, subroutine)
+                val sub = subroutine.statements.singleOrNull { it is Subroutine && it.name==name}
+                if(sub!=null)
+                    nameError(name, sub.position, subroutine)
             }
 
             // inject subroutine params as local variables (if they're not there yet) (for non-kernel subroutines and non-asm parameters)
@@ -132,7 +136,7 @@ private class AstIdentifiersChecker(private val namespace: INameScope) : IAstPro
             if(subroutine.asmAddress==null && !subroutine.canBeAsmSubroutine) {
                 if(subroutine.asmParameterRegisters.isEmpty()) {
                     subroutine.parameters
-                            .filter { it.name !in allDefinedNames }
+                            .filter { it.name !in namesInSub }
                             .forEach {
                                 val vardecl = VarDecl(VarDeclType.VAR, it.type, false, null, it.name, null, false, subroutine.position)
                                 vardecl.linkParents(subroutine)
diff --git a/compiler/src/prog8/ast/StmtReorderer.kt b/compiler/src/prog8/ast/StmtReorderer.kt
index dd53c92a5..595d0e90f 100644
--- a/compiler/src/prog8/ast/StmtReorderer.kt
+++ b/compiler/src/prog8/ast/StmtReorderer.kt
@@ -315,7 +315,9 @@ private class StatementReorderer(private val program: Program): IAstProcessor {
 
 
 private class VarInitValueAndAddressOfCreator(private val namespace: INameScope): IAstProcessor {
-    // Replace the var decl with an assignment and add a new vardecl with the default constant value.
+    // For VarDecls that declare an initialization value:
+    // Replace the vardecl with an assignment (to set the initial value),
+    // and add a new vardecl with the default constant value of that type (usually zero) to the scope.
     // This makes sure the variables get reset to the intended value on a next run of the program.
     // Variable decls without a value don't get this treatment, which means they retain the last
     // value they had when restarting the program.
diff --git a/examples/test.p8 b/examples/test.p8
index 86473fd40..805fe1b21 100644
--- a/examples/test.p8
+++ b/examples/test.p8
@@ -5,59 +5,21 @@
 ~ main {
 
     sub start() {
-        c64scr.setcc(0,0,160,0)
-        c64scr.setcc(0,1,160,1)
-        c64scr.setcc(0,2,160,2)
-        c64scr.setcc(0,3,160,3)
-        c64scr.setcc(0,4,160,4)
-        c64scr.setcc(0,5,160,5)
-        c64scr.setcc(0,6,160,6)
-        c64scr.setcc(0,7,160,7)
-        c64scr.setcc(0,8,160,8)
-        c64scr.setcc(0,9,160,9)
-        c64scr.setcc(0,10,160,10)
-        c64scr.setcc(0,11,160,11)
-        c64scr.setcc(0,12,160,12)
-        c64scr.setcc(0,13,160,13)
-        c64scr.setcc(0,14,160,14)
-        c64scr.setcc(0,15,160,15)
 
-        c64scr.setcc(1,0,160,0)
-        c64scr.setcc(1,1,160,1)
-        c64scr.setcc(1,2,160,2)
-        c64scr.setcc(1,3,160,3)
-        c64scr.setcc(1,4,160,4)
-        c64scr.setcc(1,5,160,5)
-        c64scr.setcc(1,6,160,6)
-        c64scr.setcc(1,7,160,7)
-        c64scr.setcc(1,8,160,8)
-        c64scr.setcc(1,9,160,9)
-        c64scr.setcc(1,10,160,10)
-        c64scr.setcc(1,11,160,11)
-        c64scr.setcc(1,12,160,12)
-        c64scr.setcc(1,13,160,13)
-        c64scr.setcc(1,14,160,14)
-        c64scr.setcc(1,15,160,15)
+        foo(1)
+    }
+
+    sub foo(ubyte param1)  {
+
+        sub subsub() {
+
+        }
+
+        sub param1() {
+        }
+    }
 
-        c64scr.setcc(2,0,160,0)
-        c64scr.setcc(2,1,160,1)
-        c64scr.setcc(2,2,160,2)
-        c64scr.setcc(2,3,160,3)
-        c64scr.setcc(2,4,160,4)
-        c64scr.setcc(2,5,160,5)
-        c64scr.setcc(2,6,160,6)
-        c64scr.setcc(2,7,160,7)
-        c64scr.setcc(2,8,160,8)
-        c64scr.setcc(2,9,160,9)
-        c64scr.setcc(2,10,160,10)
-        c64scr.setcc(2,11,160,11)
-        c64scr.setcc(2,12,160,12)
-        c64scr.setcc(2,13,160,13)
-        c64scr.setcc(2,14,160,14)
-        c64scr.setcc(2,15,160,15)
 
-        _x:
-            goto _x
 
 ;        for ubyte y in 0 to 3 {
 ;            for ubyte x in 0 to 10 {
@@ -94,6 +56,4 @@
 ;        c64flt.print_f(xcoor[2])
 ;        c64.CHROUT('\n')
 
-
-    }
 }