Make CapturingDiagnosticsLogger safe for concurrent writers

docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@@ -2,6 +2,7 @@
 
 * Fix internal error FS0192 "Iterate2D" when a `[<ReflectedDefinition(true)>]` parameter auto-quotes an argument that captures a not-yet-generalized use of an inferred generically-recursive function. The auto-quoted (`Expr.WithValue`) copy now keeps a fresh link to the recursive-value use so it receives the same inferred type arguments as the executable expression at the letrec point. ([Issue #20379](https://github.com/dotnet/fsharp/issues/20379))
 * Fix `NativePtr.stackalloc` nested in a larger expression (e.g. a call argument or the right of an assignment) producing an assembly that throws `InvalidProgramException` at load. ([Issue #8083](https://github.com/dotnet/fsharp/issues/8083), [PR #20302](https://github.com/dotnet/fsharp/pull/20302))
+* Fix `CapturingDiagnosticsLogger` losing diagnostics or throwing `ArgumentException` ("Source array was not long enough") on every later request for a cached transparent-compiler result, after parallel graph type-checking nodes wrote into it concurrently. ([PR #20553](https://github.com/dotnet/fsharp/pull/20553))
 * Fix internal error "Unexpected generalized type variables when compiling an active pattern" when an active pattern is used in a `let` binding whose right-hand side is a generic value, e.g. `let (T) = id`. Such a binding is now checked like the equivalent `match` and is not generalized. ([Issue #16856](https://github.com/dotnet/fsharp/issues/16856), [PR #20383](https://github.com/dotnet/fsharp/pull/20383))
 * Fix Release-only (`--optimize+`) `System.InvalidProgramException` from `Seq.collect` / `yield!` over a value-type (struct) collection implementing `seq<'T>` (e.g. `ImmutableArray<_>`) when materialised with `List.ofSeq` / `Seq.toList` / `Seq.toArray` or a list/array comprehension. The collector lowering now boxes a struct sub-collection to `seq<'T>` before calling `AddMany`/`AddManyAndClose` (matching the coercion the type checker already inserts for `yield!`), and uses `unit` as the try/finally result type instead of the body type (removing a spurious `ldnull` store). ([Issue #20203](https://github.com/dotnet/fsharp/issues/20203))
 * Fix recursive inline SRTP resolution being truncated by one currying level (e.g. FSharpPlus `memoizeN`), a regression from the function-domain unification order change in [PR #15181](https://github.com/dotnet/fsharp/pull/15181); the contravariant domain now keeps the inference variable that still carries the pending member constraint. ([PR #20247](https://github.com/dotnet/fsharp/pull/20247))

src/Compiler/Facilities/DiagnosticsLogger.fs

@@ -399,18 +399,19 @@ type CapturingDiagnosticsLogger(nm, ?eagerFormat) =
             | None -> diagnostic
             | Some f -> f diagnostic
 
-        if diagnostic.Severity = FSharpDiagnosticSeverity.Error then
-            errorCount <- errorCount + 1
+        lock diagnostics (fun () ->
+            if diagnostic.Severity = FSharpDiagnosticSeverity.Error then
+                errorCount <- errorCount + 1
 
-        diagnostics.Add(diagnostic)
+            diagnostics.Add(diagnostic))
 
     override _.ErrorCount = errorCount
 
-    member _.Diagnostics = diagnostics |> Seq.toList
+    member _.Diagnostics = lock diagnostics (fun () -> List.ofSeq diagnostics)
 
     member _.CommitDelayedDiagnostics(diagnosticsLogger: DiagnosticsLogger) =
         // Eagerly grab all the errors and warnings from the mutable collection
-        let errors = diagnostics.ToArray()
+        let errors = lock diagnostics diagnostics.ToArray
         errors |> Array.iter diagnosticsLogger.DiagnosticSink
 
 let buildPhase = AsyncLocal<BuildPhase voption>()

tests/FSharp.Compiler.Service.Tests/BuildGraphTests.fs

@@ -338,6 +338,22 @@ module BuildGraphTests =
 
         Parallel.Invoke(task1, task2)
 
+    [<Fact>]
+    let ``CapturingDiagnosticsLogger keeps every diagnostic reported concurrently`` () =
+        let count = 100_000
+        let logger = CapturingDiagnosticsLogger "concurrent writers"
+
+        do
+            use _ = UseDiagnosticsLogger logger
+            Parallel.For(0, count, fun _ -> errorR TestException) |> ignore
+
+        logger.ErrorCount |> Assert.shouldBe count
+        logger.Diagnostics.Length |> Assert.shouldBe count
+
+        let target = CapturingDiagnosticsLogger "commit target"
+        logger.CommitDelayedDiagnostics target
+        target.ErrorCount |> Assert.shouldBe count
+
 
     type internal DiagnosticsLoggerWithCallback(callback) =
         inherit CapturingDiagnosticsLogger("test")