From 5ffab8d307ff4be1f457bc85992b86bb49fec15a Mon Sep 17 00:00:00 2001 From: Alfonso Garcia-Caro Date: Sun, 2 Feb 2025 13:25:53 +0900 Subject: [PATCH] Fix #4031: Hoist vars locally in for/while loops --- src/Fable.Cli/CHANGELOG.md | 4 ++ src/Fable.Compiler/CHANGELOG.md | 4 ++ src/Fable.Transforms/Fable2Babel.fs | 56 +++++++++++-------- .../Library.js.expected | 5 +- tests/Js/Main/MiscTests.fs | 44 +++++++++++++++ 5 files changed, 87 insertions(+), 26 deletions(-) diff --git a/src/Fable.Cli/CHANGELOG.md b/src/Fable.Cli/CHANGELOG.md index f8d7f1c807..a406224cae 100644 --- a/src/Fable.Cli/CHANGELOG.md +++ b/src/Fable.Cli/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [Python] - Print root module and module function comments (by @alfonsogarciacaro) +### Fixed + +* [JS/TS] Fix #4031: Hoist vars locally in for and while loops (@alfonsogarciacaro) + ## 5.0.0-alpha.9 - 2025-01-28 ### Fixed diff --git a/src/Fable.Compiler/CHANGELOG.md b/src/Fable.Compiler/CHANGELOG.md index d61905d0c9..2ff5150338 100644 --- a/src/Fable.Compiler/CHANGELOG.md +++ b/src/Fable.Compiler/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [Python] - Print root module and module function comments (by @alfonsogarciacaro) +### Fixed + +* [JS/TS] Fix #4031: Hoist vars locally in for and while loops (@alfonsogarciacaro) + ## 5.0.0-alpha.9 - 2025-01-28 ### Fixed diff --git a/src/Fable.Transforms/Fable2Babel.fs b/src/Fable.Transforms/Fable2Babel.fs index aa705ea114..fc1bba82c8 100644 --- a/src/Fable.Transforms/Fable2Babel.fs +++ b/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) = diff --git a/tests/Integration/Integration/data/signatureHidesFunction/Library.js.expected b/tests/Integration/Integration/data/signatureHidesFunction/Library.js.expected index 7d28d9ce6c..214f6ff6b2 100644 --- a/tests/Integration/Integration/data/signatureHidesFunction/Library.js.expected +++ b/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; } - diff --git a/tests/Js/Main/MiscTests.fs b/tests/Js/Main/MiscTests.fs index 587cc43552..c6e3fb951e 100644 --- a/tests/Js/Main/MiscTests.fs +++ b/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