dotnet / dotnet/fsharp

Graph-based type-checking reports a finisher's diagnostic once per dependent file, and races on the shared logger in FCS

Open
#20,516 0 comments 0 reactions 0 assignees View on GitHub
Needs-Triage
Dominant language
F#
Stars
4.3k
Forks
876
Avg merge
4d 22h
Merged PRs (30d)
144

Description

Graph-based type-checking folds a file's *finisher* once per dependent file, so any diagnostic raised
inside a finisher is reported once per dependent instead of once. In FCS the same replay happens on
several threads at once under one shared diagnostics logger, and there it does not merely duplicate the
message — it corrupts the list behind the sink and takes the check down with an
`IndexOutOfRangeException`.

## Repro

The project as a zip:
**[FS0239Duplicated.zip](https://github.com/xperiandri/fsharp/releases/download/repro-fs0239-20516/FS0239Duplicated.zip)**
(3 KB, fourteen source files and an `.fsproj`). It is also written out in full below, so nothing here
depends on the download.

Two files declare the same module, which is one mistake; twelve further files merely depend on it.

`Dup1.fs`
```fsharp
module Repro.Dup

let a = 1
```

`Dup2.fs`
```fsharp
module Repro.Dup

let b = 2
```

`Leaf1.fs` … `Leaf12.fs`
```fsharp
module Repro.Leaf1

let v = Repro.Dup.a + 1
```

with all fourteen files in one `.fsproj`, in that order.

```
dotnet build --no-incremental
```

> `FS0239: An implementation of the file or module 'Repro.Dup' has already been given` — **13 times**,
> every one of them at `Dup2.fs(1,1)`

```
dotnet build --no-incremental -p:OtherFlags=--parallelcompilation-
```

> the same error **once**

(MSBuild echoes each error into its summary, so the raw line counts are 26 and 2; `fsc` invoked
directly with `--parallelcompilation+` / `--parallelcompilation-` gives 13 and 1.)

The count tracks the number of dependents, which is what identifies the mechanism:

| Files depending on `Repro.Dup` | Graph (default) | Sequential |
|---|---|---|
| 1 | 2 | 1 |
| 3 | 4 | 1 |
| 12 | 13 | 1 |

`FS0248 "Two modules named 'Repro.Dup' occur in two parts of this assembly"` is duplicated in exactly
the same way and to the same counts, so this is not specific to one diagnostic — it is anything raised
from a finisher.

Measured on SDK `11.0.100-rc.1.26420.103` and on a local build of `main`. Since
`TypeCheckingMode.Graph` is the default (`CompilerConfig.fs`, `typeCheckingConfig`), this is what
users get out of the box.

## Where it comes from

`TypeCheckingGraphProcessing.combineResults` (`src/Compiler/Service/TransparentCompiler.fs`) builds the
state a node is checked against by folding the finishers of that node's transitive dependencies. Every
dependent does this, so a given file's finisher runs once per dependent — and again in the final
sequential fold after `processGraphAsync` returns, which is the one that produces the project's state.

A finisher is not a pure state transition. `CheckOneInputWithCallback` raises FS0239 from it:

```fsharp
if Zset.contains qualNameOfFile tcState.tcsRootImpls then
errorR (Error(FSComp.SR.buildImplementationAlreadyGiven ..., m))
```

and `AddCheckResultsToTcState`, which every finisher calls, reaches `CombineCcuContentFragments` and its
`errorR`s. So the replay re-reports.

## The same replay crashes FCS

Reported to me as "Peek Definition does nothing"; the debugger showed the check had died. Caught live in
a Visual Studio experimental instance checking a large solution:

```
System.IndexOutOfRangeException: Index was outside the bounds of the array.
at System.Collections.Generic.List`1.Add(T item)
at FSharp.Compiler.ParseAndCheckInputs.CheckOneInputWithCallback@1543-13.Invoke(TcState tcState)
in ParseAndCheckInputs.fs:line 1546
at TransparentCompiler.clo@1483-104.Invoke(TcInfo tcInfo) in TransparentCompiler.fs:1485
at TypeCheckingGraphProcessing.folder@229TT.Invoke(TcInfo, Finisher) in TransparentCompiler.fs:229
at Microsoft.FSharp.Collections.ArrayModule.Fold[T,TState](...)
at TypeCheckingGraphProcessing.combineResults(...) in TransparentCompiler.fs:209
at TypeCheckingGraphProcessing.workWrapper@229-1.Invoke(Unit) in TransparentCompiler.fs:237
```

`ParseAndCheckInputs.fs:1546` is the FS0239 above, and `qualNameOfFile.Text` in that frame was the name
of a module declared twice in the project. `List.Add` cannot throw `IndexOutOfRangeException` on one
thread, so two are appending at once, and the ambient logger explains how they come to share one:

- `diagnosticsLogger` is an `AsyncLocal` (`Facilities/DiagnosticsLogger.fs`), so it flows *into*
everything started from a context;
- `processGraphAsync` dispatches each node with `Async.Start`, so every node inherits the same instance;
- `ComputeTcIntermediate` installs a logger of its own (`TransparentCompiler.fs`, `CompilationGlobalsScope`)
but only for the duration of the check — the finisher it returns is folded later, outside that scope.

The exception leaves `workWrapper`, so the project loses its check rather than reporting an error.
Parsing already avoids this shape by running its parallel work through
`MultipleDiagnosticsLoggers.Parallel`; graph type-checking does not.

## What I think the fix is, and the part I could not settle

The preparatory folds are not authoritative — the final fold after `processGraphAsync` applies every
`PhysicalFile` finisher once, in order, and that is the state the project gets. So silencing diagnostics
during the preparatory folds would remove the duplication and the race together.

The part I could not convince myself of: the final fold keeps only `NodeToTypeCheck.PhysicalFile` nodes,
so an `ArtificialImplFile` finisher — which also reaches `CombineCcuContentFragments` through
`AddSignatureResultToTcImplEnv` — is folded *only* in the preparatory passes. Silencing them would drop
whatever it raises, unless the physical files' folds re-raise the same clashes, which I have not
verified. Happy to be told which way that goes.

@T-Gro

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Contributor guide

Open the contributing guide

Research direction

Reproduce the issue with the supplied fourteen-file project, then start in TypeCheckingGraphProcessing.combineResults in src/Compiler/Service/TransparentCompiler.fs and trace the finisher folds and diagnosticsLogger flow from Facilities/DiagnosticsLogger.fs. Compare the graph and sequential paths, including the final physical-file fold and ArtificialImplFile handling. Done means diagnostics are reported once without the parallel logger crash, while relevant finisher diagnostics remain covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
fsharp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.