fix(all): keep reads of imported mutable values in place

src/Fable.Transforms/FableTransforms.fs

@@ -409,11 +409,21 @@ module private Transforms =
             | _ -> ValueNone
 
     let tryInlineBinding (com: Compiler) (ident: Ident) value letBody =
+        let readsMutableValue callee =
+            match callee with
+            | IdentExpr id -> id.IsMutable
+            | Import({ Kind = MemberImport memberRef }, _, _) ->
+                com.TryGetMember(memberRef) |> Option.exists (fun memb -> memb.IsMutable)
+            | _ -> false
+
         let canInlineBinding =
             match value with
             | Import(i, _, _) -> i.IsCompilerGenerated
             | Call(callee, info, _, _) when List.isEmpty info.Args && List.contains "value" info.Tags ->
-                canInlineArg com ident.Name callee letBody
+                if readsMutableValue callee then
+                    canInlineArg com ident.Name value letBody
+                else
+                    canInlineArg com ident.Name callee letBody
             // Replace non-recursive lambda bindings
             | NestedLambda(_args, lambdaBody, _name) ->
                 match lambdaBody with

tests/Js/Main/MiscTests.fs

@@ -382,7 +382,7 @@
 
 let mutable mutableValue = 0
 
 let rec recursive1 = delay (fun () -> recursive2())
 and recursive2 =
     mutableValue <- 5
     fun () -> mutableValue <- mutableValue * 2
@@ -1314,6 +1314,11 @@
         Option.defaultWith (fun () -> invalidOp "This is invalid") (Some 3) |> equal 3
         throwsError "This is invalid" (fun () -> Option.defaultWith (fun () -> invalidOp "This is invalid") None)
 
+    testCase "A read of another file's mutable value is not moved past side effects" <| fun () -> // See #5156
+        let before = MiscTestsHelper.trackedCalls
+        MiscTestsHelper.track 1 |> ignore
+        MiscTestsHelper.trackedCalls - before |> equal 1
+
     testCase "Pattern matching optimization works (switch statement)" <| fun () ->
         let mutable x = ""
         let i = 4
@@ -1322,7 +1327,7 @@
         | 2 -> x <- "2"
         | 3 | 4 -> x <- "3" // Multiple cases are allowed
         // | 5 | 6 as j -> x <- string j // This prevents the optimization
         | 4 -> x <- "4" // Unreachable cases are removed
         | _ -> x <- "?"
         equal "3" x
 

tests/Js/Main/MiscTestsHelper.fs

@@ -31,3 +31,9 @@ type Vector2<[<Measure>] 'u> = Vector2 of x: float<'u> * y: float<'u> with
 
   static member inline ( + ) (Vector2(ax, ay), Vector2(bx, by)) = Vector2(ax + bx, ay + by)
   static member inline ( * ) (scalar, Vector2(x, y)) = Vector2(scalar * x, scalar * y)
+
+let mutable trackedCalls = 0
+
+let track x =
+    trackedCalls <- trackedCalls + 1
+    x

tests/Python/MiscTestsHelper.fs

@@ -42,3 +42,9 @@ type CrossModuleDemo =
 
     static member propDefault = A "prop"
     static member methDefault() = A "meth"
+
+let mutable trackedCalls = 0
+
+let track x =
+    trackedCalls <- trackedCalls + 1
+    x

tests/Python/TestMisc.fs

@@ -1336,7 +1336,7 @@
     | 2 -> x <- "2"
     | 3 | 4 -> x <- "3" // Multiple cases are allowed
     // | 5 | 6 as j -> x <- string j // This prevents the optimization
     | 4 -> x <- "4" // Unreachable cases are removed
     | _ -> x <- "?"
     equal "3" x
 
@@ -1833,3 +1833,9 @@
     with _ -> ()
     equal before storage.count
 #endif
+
+[<Fact>]
+let ``test A read of another file's mutable value is not moved past side effects`` () = // See #5156
+    let before = MiscTestsHelper.trackedCalls
+    MiscTestsHelper.track 1 |> ignore
+    MiscTestsHelper.trackedCalls - before |> equal 1