java,kotlin: fix double eval of last do position.

These implementations were double evaluating the last position of a do form (off by one). Mostly this is just a performance issue until you have something with side-effect in the last position such as prn's, swap!'s or readline's. In self-hosted mode this become apparent with large chunks of mal code in the last position. The symptoms were repeated prns and multiple levels of readline being called, etc. Also, add a step6 test that uses atoms to test this (should be in step 4 using prn side-effects but it's difficult to match across multiple lines in a cross-platform way so we test using an atom in step6.
kanaka · Aug 19, 2024 · 48712c7 · 48712c7
1 parent 7b23c7e
commit 48712c7
Show file tree

Hide file tree

Showing 15 changed files with 22 additions and 16 deletions.
diff --git a/impls/java/src/main/java/mal/step4_if_fn_do.java b/impls/java/src/main/java/mal/step4_if_fn_do.java
@@ -73,7 +73,7 @@ public static MalVal EVAL(MalVal orig_ast, Env env) throws MalThrowable {
             }
             return EVAL(a2, let_env);
         case "do":
-            for (int i=1; i<ast.size(); i++)
+            for (int i=1; i<ast.size()-1; i++)
                 EVAL(ast.nth(i), env);
             return EVAL(ast.nth(ast.size() - 1), env);
         case "if":

diff --git a/impls/java/src/main/java/mal/step5_tco.java b/impls/java/src/main/java/mal/step5_tco.java
@@ -77,7 +77,7 @@ public static MalVal EVAL(MalVal orig_ast, Env env) throws MalThrowable {
             env = let_env;
             break;
         case "do":
-            for (int i=1; i<ast.size(); i++)
+            for (int i=1; i<ast.size()-1; i++)
                 EVAL(ast.nth(i), env);
             orig_ast = ast.nth(ast.size()-1);
             break;

diff --git a/impls/java/src/main/java/mal/step6_file.java b/impls/java/src/main/java/mal/step6_file.java
@@ -77,7 +77,7 @@ public static MalVal EVAL(MalVal orig_ast, Env env) throws MalThrowable {
             env = let_env;
             break;
         case "do":
-            for (int i=1; i<ast.size(); i++)
+            for (int i=1; i<ast.size()-1; i++)
                 EVAL(ast.nth(i), env);
             orig_ast = ast.nth(ast.size()-1);
             break;

diff --git a/impls/java/src/main/java/mal/step7_quote.java b/impls/java/src/main/java/mal/step7_quote.java
@@ -114,7 +114,7 @@ public static MalVal EVAL(MalVal orig_ast, Env env) throws MalThrowable {
             orig_ast = quasiquote(ast.nth(1));
             break;
         case "do":
-            for (int i=1; i<ast.size(); i++)
+            for (int i=1; i<ast.size()-1; i++)
                 EVAL(ast.nth(i), env);
             orig_ast = ast.nth(ast.size()-1);
             break;

diff --git a/impls/java/src/main/java/mal/step8_macros.java b/impls/java/src/main/java/mal/step8_macros.java
@@ -122,7 +122,7 @@ public static MalVal EVAL(MalVal orig_ast, Env env) throws MalThrowable {
             env.set((MalSymbol)a1, res);
             return res;
         case "do":
-            for (int i=1; i<ast.size(); i++)
+            for (int i=1; i<ast.size()-1; i++)
                 EVAL(ast.nth(i), env);
             orig_ast = ast.nth(ast.size()-1);
             break;

diff --git a/impls/java/src/main/java/mal/step9_try.java b/impls/java/src/main/java/mal/step9_try.java
@@ -148,7 +148,7 @@ public static MalVal EVAL(MalVal orig_ast, Env env) throws MalThrowable {
                 throw t;
             }
         case "do":
-            for (int i=1; i<ast.size(); i++)
+            for (int i=1; i<ast.size()-1; i++)
                 EVAL(ast.nth(i), env);
             orig_ast = ast.nth(ast.size()-1);
             break;

diff --git a/impls/java/src/main/java/mal/stepA_mal.java b/impls/java/src/main/java/mal/stepA_mal.java
@@ -148,7 +148,7 @@ public static MalVal EVAL(MalVal orig_ast, Env env) throws MalThrowable {
                 throw t;
             }
         case "do":
-            for (int i=1; i<ast.size(); i++)
+            for (int i=1; i<ast.size()-1; i++)
                 EVAL(ast.nth(i), env);
             orig_ast = ast.nth(ast.size()-1);
             break;

diff --git a/impls/kotlin/src/mal/step4_if_fn_do.kt b/impls/kotlin/src/mal/step4_if_fn_do.kt
@@ -61,7 +61,7 @@ private fun eval_fn_STAR(ast: ISeq, env: Env): MalType {
 }
 
 private fun eval_do(ast: ISeq, env: Env): MalType {
-    for (i in 1..ast.count() - 1) {
+    for (i in 1..ast.count() - 2) {
         eval(ast.nth(i), env)
     }
     return eval(ast.seq().last(), env)

diff --git a/impls/kotlin/src/mal/step5_tco.kt b/impls/kotlin/src/mal/step5_tco.kt
@@ -34,7 +34,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                 }
                 "fn*" -> return fn_STAR(ast, env)
                 "do" -> {
-                    for (i in 1..ast.count() - 1) {
+                    for (i in 1..ast.count() - 2) {
                         eval(ast.nth(i), env)
                     }
                     ast = ast.seq().last()

diff --git a/impls/kotlin/src/mal/step6_file.kt b/impls/kotlin/src/mal/step6_file.kt
@@ -36,7 +36,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                 }
                 "fn*" -> return fn_STAR(ast, env)
                 "do" -> {
-                    for (i in 1..ast.count() - 1) {
+                    for (i in 1..ast.count() - 2) {
                         eval(ast.nth(i), env)
                     }
                     ast = ast.seq().last()
@@ -51,7 +51,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                     } else return NIL
                 }
                 else -> {
-                    val evaluated = ast.elements.fold(MalList(), { a, b -> a.conj_BANG(eval(b, env)); a }) as ISeq
+                    val evaluated = ast.elements.fold(MalList(), { a, b -> a.conj_BANG(eval(b, env)); a })
                     val firstEval = evaluated.first()
 
                     when (firstEval) {

diff --git a/impls/kotlin/src/mal/step7_quote.kt b/impls/kotlin/src/mal/step7_quote.kt
@@ -36,7 +36,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                 }
                 "fn*" -> return fn_STAR(ast, env)
                 "do" -> {
-                    for (i in 1..ast.count() - 1) {
+                    for (i in 1..ast.count() - 2) {
                         eval(ast.nth(i), env)
                     }
                     ast = ast.seq().last()
@@ -53,7 +53,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                 "quote" -> return ast.nth(1)
                 "quasiquote" -> ast = quasiquote(ast.nth(1))
                 else -> {
-                    val evaluated = ast.elements.fold(MalList(), { a, b -> a.conj_BANG(eval(b, env)); a }) as ISeq
+                    val evaluated = ast.elements.fold(MalList(), { a, b -> a.conj_BANG(eval(b, env)); a })
                     val firstEval = evaluated.first()
 
                     when (firstEval) {

diff --git a/impls/kotlin/src/mal/step8_macros.kt b/impls/kotlin/src/mal/step8_macros.kt
@@ -36,7 +36,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                 }
                 "fn*" -> return fn_STAR(ast, env)
                 "do" -> {
-                    for (i in 1..ast.count() - 1) {
+                    for (i in 1..ast.count() - 2) {
                         eval(ast.nth(i), env)
                     }
                     ast = ast.seq().last()

diff --git a/impls/kotlin/src/mal/step9_try.kt b/impls/kotlin/src/mal/step9_try.kt
@@ -36,7 +36,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                 }
                 "fn*" -> return fn_STAR(ast, env)
                 "do" -> {
-                    for (i in 1..ast.count() - 1) {
+                    for (i in 1..ast.count() - 2) {
                         eval(ast.nth(i), env)
                     }
                     ast = ast.seq().last()

diff --git a/impls/kotlin/src/mal/stepA_mal.kt b/impls/kotlin/src/mal/stepA_mal.kt
@@ -36,7 +36,7 @@ fun eval(_ast: MalType, _env: Env): MalType {
                 }
                 "fn*" -> return fn_STAR(ast, env)
                 "do" -> {
-                    for (i in 1..ast.count() - 1) {
+                    for (i in 1..ast.count() - 2) {
                         eval(ast.nth(i), env)
                     }
                     ast = ast.seq().last()

diff --git a/impls/tests/step6_file.mal b/impls/tests/step6_file.mal
@@ -87,6 +87,12 @@
 (swap! a + 3)
 ;=>123
 
+;; Test that do only evals each slot once
+(def! b (atom 0))
+(do (swap! b + 1) (swap! b + 10) (swap! b + 100))
+(deref b)
+;=>111
+
 ;; Testing swap!/closure interaction
 (def! inc-it (fn* (a) (+ 1 a)))
 (def! atm (atom 7))