Delete unused variable (#14)

.envrc

@@ -1 +1,23 @@
 use flake
+DOTNET_PATH=$(readlink "$(which dotnet)")
+SETTINGS_FILE=$(find . -maxdepth 1 -type f -name '*.sln.DotSettings.user')
+MSBUILD=$(realpath "$(find "$(dirname "$DOTNET_PATH")/../share/dotnet/sdk" -maxdepth 2 -type f -name MSBuild.dll)")
+if [ -f "$SETTINGS_FILE" ] ; then
+    xmlstarlet ed --inplace \
+    -N wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation" \
+    -N x="http://schemas.microsoft.com/winfx/2006/xaml" \
+    -N s="clr-namespace:System;assembly=mscorlib" \
+    -N ss="urn:shemas-jetbrains-com:settings-storage-xaml" \
+    --update "//s:String[@x:Key='/Default/Environment/Hierarchy/Build/BuildTool/DotNetCliExePath/@EntryValue']" \
+    --value "$(realpath "$(dirname "$DOTNET_PATH")/../share/dotnet/dotnet")" \
+    "$SETTINGS_FILE"
+
+    xmlstarlet ed --inplace \
+    -N wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation" \
+    -N x="http://schemas.microsoft.com/winfx/2006/xaml" \
+    -N s="clr-namespace:System;assembly=mscorlib" \
+    -N ss="urn:shemas-jetbrains-com:settings-storage-xaml" \
+    --update "//s:String[@x:Key='/Default/Environment/Hierarchy/Build/BuildTool/CustomBuildToolPath/@EntryValue']" \
+    --value "$MSBUILD" \
+    "$SETTINGS_FILE"
+fi

README.md

@@ -17,7 +17,7 @@ An [expect-testing](https://blog.janestreet.com/the-joy-of-expect-tests/) librar
 
 The basic mechanism works.
 Snapshot updating is vibe-coded with Opus 4 and is purely text-based; I didn't want to use the F# compiler services because that's a pretty heavyweight dependency which should be confined to a separate test runner entity.
-It's not very well tested, and I expect it to be kind of brittle.
+It's fairly well tested, but you will certainly be able to find ways to break it; try not to be too fancy with your syntax around the `snapshot` statement.
 
 # How to use
 

WoofWare.Expect/Builder.fs

@@ -23,8 +23,6 @@ type Mode =
 /// <param name="applyChanges">When running the tests, instead of throwing an exception on failure, update the snapshot.</param>
 /// <param name="sourceOverride">Override the file path and line numbers reported in snapshots, so that your tests can be fully stable even on failure. (You almost certainly don't want to set this.)</param>
 type ExpectBuilder (mode : Mode) =
-    member private this.Mode = Unchecked.defaultof<Mode>
-
     new (sourceOverride : string * int) = ExpectBuilder (Mode.AssertMockingSource sourceOverride)
 
     new (update : bool)
@@ -254,7 +252,7 @@ type ExpectBuilder (mode : Mode) =
                 |> ExpectException
                 |> raise
             | Mode.Assert ->
-                if GlobalBuilderConfig.bulkUpdate.Value > 0 then
+                if GlobalBuilderConfig.isBulkUpdateMode () then
                     GlobalBuilderConfig.registerTest state
                 else
                     sprintf
@@ -270,14 +268,14 @@ type ExpectBuilder (mode : Mode) =
                 let lines = File.ReadAllLines state.Caller.FilePath
                 let oldContents = String.concat "\n" lines
                 let lines = SnapshotUpdate.updateSnapshotAtLine lines state.Caller.LineNumber actual
-                File.WriteAllLines (state.Caller.FilePath, lines)
+                File.writeAllLines lines state.Caller.FilePath
                 failwith ("Snapshot successfully updated. Previous contents:\n" + oldContents)
 
         match CompletedSnapshotGeneric.passesAssertion state with
         | None ->
-            match mode, GlobalBuilderConfig.bulkUpdate.Value with
+            match mode, GlobalBuilderConfig.isBulkUpdateMode () with
             | Mode.Update, _
-            | _, 1 ->
+            | _, true ->
                 failwith
                     "Snapshot assertion passed, but we are in snapshot-updating mode. Use the `expect` builder instead of `expect'` to assert the contents of a single snapshot; disable `GlobalBuilderConfig.bulkUpdate` to move back to assertion-checking mode."
             | _ -> ()

WoofWare.Expect/Config.fs

@@ -1,12 +1,19 @@
 namespace WoofWare.Expect
 
-open System.Threading
-
 /// Module holding global mutable state controlling the behaviour of WoofWare.Expect
 /// when running in bulk-update mode.
 [<RequireQualifiedAccess>]
 module GlobalBuilderConfig =
-    let internal bulkUpdate = ref 0
+    /// All access to the global mutable state locks on this.
+    let private locker = obj ()
+
+    // Global mutable state ensuring there is at most one `enterBulkUpdateMode`/`updateAllSnapshots` pair running at once.
+    let private bulkUpdate = ref 0
+
+    let private allTests : ResizeArray<CompletedSnapshot> = ResizeArray ()
+
+    let internal isBulkUpdateMode () : bool =
+        lock locker (fun () -> bulkUpdate.Value > 0)
 
     /// <summary>
     /// Call this to make the <c>expect</c> builder register all tests for bulk update as it runs.
@@ -16,11 +23,15 @@ module GlobalBuilderConfig =
     /// The implied global mutable state is liable to interfere with other expect builders in other fixtures otherwise.
     /// </remarks>
     let enterBulkUpdateMode () =
-        if Interlocked.Increment bulkUpdate <> 1 then
-            failwith
-                "WoofWare.Expect requires bulk updates to happen serially: for example, make the test fixture `[<NonParallelizable>]` if you're using NUnit."
+        lock
+            locker
+            (fun () ->
+                if bulkUpdate.Value <> 0 then
+                    failwith
+                        "WoofWare.Expect requires bulk updates to happen serially: for example, make the test fixture `[<NonParallelizable>]` if you're using NUnit."
 
-    let private allTests : ResizeArray<CompletedSnapshot> = ResizeArray ()
+                bulkUpdate.Value <- bulkUpdate.Value + 1
+            )
 
     /// <summary>
     /// Clear the set of failing tests registered by any previous bulk-update runs.
@@ -30,23 +41,31 @@ module GlobalBuilderConfig =
     /// You probably don't need to do this, because your test runner is probably tearing down
     /// anyway after the tests have failed; this is mainly here for WoofWare.Expect's own internal testing.
     /// </remarks>
-    let clearTests () = lock allTests allTests.Clear
+    let clearTests () = lock locker allTests.Clear
 
     let internal registerTest (s : CompletedSnapshotGeneric<'T>) : unit =
         let toAdd = s |> CompletedSnapshot.make
-        lock allTests (fun () -> allTests.Add toAdd)
+        lock locker (fun () -> allTests.Add toAdd)
 
     /// <summary>
     /// For all tests whose failures have already been registered,
     /// transform the files on disk so that the failing snapshots now pass.
     /// </summary>
     let updateAllSnapshots () =
-        let bulkUpdate' = Interlocked.Decrement bulkUpdate
+        // It's OK for this to be called when `enterBulkUpdateMode` has not been called, i.e. when `bulkUpdate` has
+        // value 0. That just means we aren't in bulk-update mode, so we expect the following simply to do nothing.
+        // (This is an expected workflow: we expect users to run `updateAllSnapshots` unconditionally in a
+        // one-time tear-down of the test suite, and they use the one-time setup to control whether any work is actually
+        // performed here.)
+        lock
+            locker
+            (fun () ->
+                let allTests = Seq.toArray allTests
 
-        try
-            if bulkUpdate' = 0 then
-                let allTests = lock allTests (fun () -> Seq.toArray allTests)
-                SnapshotUpdate.updateAll allTests
-
-        finally
-            clearTests ()
+                try
+                    SnapshotUpdate.updateAll allTests
+                finally
+                    // double acquiring of reentrant lock is OK, we're not switching threads
+                    clearTests ()
+                    bulkUpdate.Value <- 0
+            )

WoofWare.Expect/File.fs

@@ -0,0 +1,32 @@
+namespace WoofWare.Expect
+
+open System
+open System.IO
+
+[<RequireQualifiedAccess>]
+module internal File =
+
+    /// Standard attempt at an atomic file write.
+    /// It may fail to be atomic if the working directory somehow spans multiple volumes,
+    /// and of course with external network storage all bets are off.
+    let writeAllLines (lines : string[]) (path : string) : unit =
+        let file = FileInfo path
+
+        let tempFile =
+            Path.Combine (file.Directory.FullName, file.Name + "." + Guid.NewGuid().ToString () + ".tmp")
+
+        try
+            File.WriteAllLines (tempFile, lines)
+            // Atomicity guarantees are undocumented, but on Unix this is an atomic `rename` call
+            // https://github.com/dotnet/runtime/blob/9a4be5b56d81aa04c7ea687c02b3f4e64c83761b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs#L181
+            // and on Windows this is an atomic ReplaceFile:
+            // https://github.com/dotnet/runtime/blob/9a4be5b56d81aa04c7ea687c02b3f4e64c83761b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs#L92
+            // calls https://github.com/dotnet/runtime/blob/9a4be5b56d81aa04c7ea687c02b3f4e64c83761b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReplaceFile.cs#L12
+            // which calls ReplaceFileW, whose atomicity guarantees are again apparently undocumented,
+            // but 4o-turbo, Opus 4, and Gemini 2.5 Flash all think it's atomic.
+            File.Replace (tempFile, path, null)
+        finally
+            try
+                File.Delete tempFile
+            with _ ->
+                ()

WoofWare.Expect/SnapshotUpdate.fs

@@ -300,5 +300,5 @@ module internal SnapshotUpdate =
 
             let newContents = updateAllLines contents sources
 
-            System.IO.File.WriteAllLines (callerFile, newContents)
+            File.writeAllLines newContents callerFile
         )

WoofWare.Expect/WoofWare.Expect.fsproj

@@ -18,6 +18,7 @@
     <ItemGroup>
         <Compile Include="AssemblyInfo.fs" />
         <Compile Include="Text.fs" />
+        <Compile Include="File.fs" />
         <Compile Include="Domain.fs" />
         <Compile Include="SnapshotUpdate.fs" />
         <Compile Include="Config.fs" />

flake.nix

@@ -65,6 +65,7 @@
           pkgs.alejandra
           pkgs.nodePackages.markdown-link-check
           pkgs.shellcheck
+          pkgs.xmlstarlet
         ];
       };
     });