From 48712c74630c0581566ca37f5f1eef4473447a14 Mon Sep 17 00:00:00 2001
From: Joel Martin <github@martintribe.org>
Date: Mon, 19 Aug 2024 08:47:04 -0500
Subject: [PATCH] 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.
---
 impls/java/src/main/java/mal/step4_if_fn_do.java | 2 +-
 impls/java/src/main/java/mal/step5_tco.java      | 2 +-
 impls/java/src/main/java/mal/step6_file.java     | 2 +-
 impls/java/src/main/java/mal/step7_quote.java    | 2 +-
 impls/java/src/main/java/mal/step8_macros.java   | 2 +-
 impls/java/src/main/java/mal/step9_try.java      | 2 +-
 impls/java/src/main/java/mal/stepA_mal.java      | 2 +-
 impls/kotlin/src/mal/step4_if_fn_do.kt           | 2 +-
 impls/kotlin/src/mal/step5_tco.kt                | 2 +-
 impls/kotlin/src/mal/step6_file.kt               | 4 ++--
 impls/kotlin/src/mal/step7_quote.kt              | 4 ++--
 impls/kotlin/src/mal/step8_macros.kt             | 2 +-
 impls/kotlin/src/mal/step9_try.kt                | 2 +-
 impls/kotlin/src/mal/stepA_mal.kt                | 2 +-
 impls/tests/step6_file.mal                       | 6 ++++++
 15 files changed, 22 insertions(+), 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
index 5f8f92916e..d0979fbc8c 100644
--- 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
index b5a176683f..9fffee39c3 100644
--- 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
index 96d259aa5e..dc7f681f59 100644
--- 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
index 7a91ddab83..ed9a10c04d 100644
--- 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
index 7133ab5f76..1210ae19d5 100644
--- 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
index 7db20aaa76..2e10bfbed6 100644
--- 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
index 8927528a38..1064248a7e 100644
--- 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
index 9ff50cc2d9..d8be4eb259 100644
--- 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
index aaad5a5bd5..6aa739d8d6 100644
--- 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
index 21c972d724..b049c9ad6d 100644
--- 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
index 669d63b055..a32b2c69fa 100644
--- 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
index 2ba8704e78..b2d5f8b8c7 100644
--- 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
index 69263f4185..7185a9c00e 100644
--- 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
index cccd0a614f..93d9a9f7f1 100644
--- 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
index e8a3ae881f..45ea84cf76 100644
--- 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))