[JS/TS] Fix #4031: Hoist vars locally in for and while loops (#4032)

* Fix #4031: Hoist vars locally in for/while loops

* update changlog

---------

Co-authored-by: Maxime Mangel <[email protected]>

Author: alfonsogarciacaro

src/Fable.Cli/CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * [JS/TS] - Fix anonymous record printing (#4029) (by @alfonsogarciacaro)
 * [Python] - Fix #3998: PhysicalEquality (by @alfonsogarciacaro)
+* [JS/TS] Fix #4031: Hoist vars locally in for and while loops (@alfonsogarciacaro)
 
 ## 5.0.0-alpha.9 - 2025-01-28
 

src/Fable.Compiler/CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * [JS/TS] - Fix anonymous record printing (#4029) (by @alfonsogarciacaro)
 * [Python] - Fix #3998: PhysicalEquality (by @alfonsogarciacaro)
+* [JS/TS] Fix #4031: Hoist vars locally in for and while loops (@alfonsogarciacaro)
 
 ## 5.0.0-alpha.9 - 2025-01-28
 

src/Fable.Transforms/Fable2Babel.fs

@@ -2208,6 +2208,27 @@ module Util =
     let transformBlock (com: IBabelCompiler) ctx ret expr : BlockStatement =
         com.TransformAsStatements(ctx, ret, expr) |> BlockStatement
 
+    let transformBlockWithVarHoisting (com: IBabelCompiler) ctx ret expr : BlockStatement =
+        let declaredVars = ResizeArray()
+
+        let ctx =
+            { ctx with
+                HoistVars =
+                    fun ids ->
+                        declaredVars.AddRange(ids)
+                        true
+            }
+
+        let body = transformBlock com ctx ret expr
+
+        if declaredVars.Count = 0 then
+            body
+        else
+            let varDeclStatement =
+                declaredVars |> Seq.map (fun v -> v, None) |> multiVarDeclaration com ctx Let
+
+            BlockStatement(Array.append [| varDeclStatement |] body.Body)
+
     let transformTryCatch com ctx r returnStrategy (body, catch, finalizer) =
         // try .. catch statements cannot be tail call optimized
         let ctx = { ctx with TailCallOpportunity = None }
@@ -3053,9 +3074,10 @@ module Util =
             transformDecisionTreeSuccessAsStatements com ctx returnStrategy idx boundValues
 
         | Fable.WhileLoop(TransformExpr com ctx guard, body, range) ->
-            [|
-                Statement.whileStatement (guard, transformBlock com ctx None body, ?loc = range)
-            |]
+            // Hoist vars within the loop so if a closure captures the variable
+            // it doesn't change in the next iteration, see #4031
+            let body = transformBlockWithVarHoisting com ctx None body
+            [| Statement.whileStatement (guard, body, ?loc = range) |]
 
         | Fable.ForLoop(var, TransformExpr com ctx start, TransformExpr com ctx limit, body, isUp, range) ->
             let op1, op2 =
@@ -3066,7 +3088,9 @@ module Util =
 
             [|
                 Statement.forStatement (
-                    transformBlock com ctx None body,
+                    // Hoist vars within the loop so if a closure captures the variable
+                    // it doesn't change in the next iteration, see #4031
+                    transformBlockWithVarHoisting com ctx None body,
                     VariableDeclaration.variableDeclaration (
                         Let,
                         var.Name,
@@ -3084,26 +3108,21 @@ module Util =
             Option.map (fun name -> NamedTailCallOpportunity(com, ctx, name, args) :> ITailCallOpportunity) name
 
         let args = FSharp2Fable.Util.discardUnitArg args
-        let declaredVars = ResizeArray()
         let mutable isTailCallOptimized = false
 
         let ctx =
             { ctx with
                 TailCallOpportunity = tailcallChance
-                HoistVars =
-                    fun ids ->
-                        declaredVars.AddRange(ids)
-                        true
                 OptimizeTailCall = fun () -> isTailCallOptimized <- true
             }
 
-        let body =
+        let returnStrategy =
             if body.Type = Fable.Unit then
-                transformBlock com ctx (Some ReturnUnit) body
-            elif isJsStatement ctx (Option.isSome tailcallChance) body then
-                transformBlock com ctx (Some Return) body
+                ReturnUnit
             else
-                transformAsExpr com ctx body |> wrapExprInBlockWithReturn
+                Return
+
+        let body = transformBlockWithVarHoisting com ctx (Some returnStrategy) body
 
         let args, body =
             match isTailCallOptimized, tailcallChance with
@@ -3143,15 +3162,6 @@ module Util =
                 ),
                 body
 
-        let body =
-            if declaredVars.Count = 0 then
-                body
-            else
-                let varDeclStatement =
-                    declaredVars |> Seq.map (fun v -> v, None) |> multiVarDeclaration com ctx Let
-
-                BlockStatement(Array.append [| varDeclStatement |] body.Body)
-
         args |> List.toArray, body
 
     let declareEntryPoint _com _ctx (funcExpr: Expression) =

tests/Integration/Integration/data/signatureHidesFunction/Library.js.expected

@@ -1,9 +1,8 @@
 
 function g(a) {
-    return a + 1;
+    return (a + 1) | 0;
 }
 
 export function f(b) {
-    return g(b);
+    return g(b) | 0;
 }
-

tests/Js/Main/MiscTests.fs

@@ -495,8 +495,52 @@ let dtest2 (f: MyIntDelegate -> int) =
 let dInvoke (d: MyIntDelegate) =
     d.Invoke ()
 
+let (>>*) (f: string -> unit) () (x: string) =
+    f x
+
+type RecordWithMutableFn = { mutable MutableFn: string -> unit }
+
+let buildRecordWithMutableFnWithWhile (xs: RecordWithMutableFn list) =
+    let mutable items = []
+    for x in xs do
+        items <- (x.MutableFn >>* ()) :: items
+    items
+
+let buildRecordWithMutableFnWithFor (xs: RecordWithMutableFn list) =
+    let mutable items = []
+    for i = 0 to xs.Length - 1 do
+        let x = xs[i]
+        items <- (x.MutableFn >>* ()) :: items
+    items
 let tests =
   testList "Miscellaneous" [
+    testCase "Hoisted vars don't conflict with while loop" <| fun () -> // see #4031
+        let mutable output = ""
+
+        let xs = [
+            { MutableFn = fun s -> output <- sprintf "%s\n%s %s" output "First" s }
+            { MutableFn = fun s -> output <- sprintf "%s\n%s %s" output "Second" s }
+        ]
+        let ys = buildRecordWithMutableFnWithWhile xs
+
+        xs |> List.iter (fun x -> x.MutableFn "output")
+        ys |> List.iter (fun y -> y "output")
+        System.Text.RegularExpressions.Regex.Replace(output.Trim(), @"\s+", " ")
+        |> equal "First output Second output Second output First output"
+
+    testCase "Hoisted vars don't conflict with for loop" <| fun () -> // see #4031
+        let mutable output = ""
+
+        let xs = [
+            { MutableFn = fun s -> output <- sprintf "%s\n%s %s" output "First" s }
+            { MutableFn = fun s -> output <- sprintf "%s\n%s %s" output "Second" s }
+        ]
+        let ys = buildRecordWithMutableFnWithFor xs
+
+        xs |> List.iter (fun x -> x.MutableFn "output")
+        ys |> List.iter (fun y -> y "output")
+        System.Text.RegularExpressions.Regex.Replace(output.Trim(), @"\s+", " ")
+        |> equal "First output Second output Second output First output"
 
     testCase "Passing delegate works" <| fun _ -> // #3862
         dtest1 dInvoke |> equal 42