Benign race condition in graph based type checking of type extensions but breaks determinism

[majocha]

This is a race condition in a very specific scenario when we have two or more type extensions of the same type in two independent (from graph checking pov) files:

fsharp/src/Compiler/Checking/CheckDeclarations.fs

Lines 4219 to 4228 in 12efe3b

	// Update resolved type parameters with the names from the source.
	let _, tcref, _ = res
	if tcref.TyparsNoRange.Length = synTypars.Length then
	(tcref.TyparsNoRange, synTypars)
	||> List.zip
	|> List.iter (fun (typar, SynTyparDecl.SynTyparDecl (typar = tp)) ->
	let (SynTypar(ident = untypedIdent; staticReq = sr)) = tp
	if typar.StaticReq = sr then
	typar.SetIdent(untypedIdent)
	)

If each extension updates the type var name, we have a race during parallel checking. In effect some of the extensions will end up with a randomly wrong typar name.

This happens when compiling FCS.dll using graph based type checking because LexBuffer<_> has such multiple independent extensions: #19028 (comment)

Repro steps

SetIdentRace repo or

SetIdentRace.zip

dotnet build --no-incremental a few times.

observe the dumped sigdata or inspect dll:

correct with sequential checking:

wrong with graph based (Print<'X> instead of Print<'U>):

This is not very severe and rather an edge case, apart from the fact that it prevents graph based type checking from being deterministic.

[majocha]

One solution I can think of is detecting such cases as dependencies and ordering affected sources properly in the graph to prevent race. But that is not feasible.

Maybe better: build a map of ident substitutions, hold it in cenv, instead of directly mutating the Typar.

[majocha]

Debugging this I can see that the correct type parameter name is threaded through the checking phase for quite some time. The correct data is there. It just gets erased at some later stage that I have yet to identify.

[majocha]

Copilot says:

Substitute enclosing typars in the member type before generalization
• Scope: build a Typar substitution from the entity’s typars to your copied declared typars (with 'E), apply it to the member Tau type before creating the ValScheme, and generalize over those declared typars.
• Effect: the member’s type becomes alpha-equivalent but uses the declared-copies for enclosing typars, so printers will show 'E. This avoids renaming anything in-place.
• Where: when assembling the member’s preliminary/generalized type in Phase2A/Phase2B (before we call GeneralizedType), detect ExtrinsicExtensionBinding and apply the substitution.

pseudocode

// Build map from tcref.Typars m -> declaredTyconTypars (with 'E)
let typarSubst = Map.ofList (List.zip (tcref.Typars m) declaredTyconTypars)
// Replace all occurrences of entity typars in memberTau with declared copies
let memberTau' = applyTyparSubstitution g typarSubst memberTau
// Then generalize using declaredTyconTypars as part of the scheme
let valscheme = GeneralizedType(declaredTyconTypars @ methodTypars, memberTau')

[majocha]

Relevant PRs and the functionality we'd want to preserve while fixing this:
#15287
#15845

Relevant test cases to quickly iterate on this:

Type extension with lowercase type parameters names from source
Type extension with constraints uses type parameters names from source
Type extension uses type parameters names from source

and also

Respect nowarn 957 for extension method

Possibly helpful read #16456