diff --git a/.gitignore b/.gitignore index 58da32ae096..ae438fb2b11 100644 --- a/.gitignore +++ b/.gitignore @@ -161,3 +161,4 @@ tests/projects/CompilerCompat/local-nuget-packages/ tests/projects/CompilerCompat/lib-output-*/ tests/projects/CompilerCompat/**/bin/ tests/projects/CompilerCompat/**/obj/ +.scratch/ diff --git a/azure-pipelines-PR.yml b/azure-pipelines-PR.yml index b627fab985c..714dc54bbec 100644 --- a/azure-pipelines-PR.yml +++ b/azure-pipelines-PR.yml @@ -100,7 +100,7 @@ stages: helixRepo: dotnet/fsharp jobs: # Determinism, we want to run it only in PR builds - - job: Determinism_Debug + - job: Determinism_Release condition: eq(variables['Build.Reason'], 'PullRequest') variables: - name: _SignType @@ -109,13 +109,6 @@ stages: name: $(DncEngPublicBuildPool) demands: ImageOverride -equals $(_WindowsMachineQueueName) timeoutInMinutes: 90 - strategy: - maxParallel: 2 - matrix: - regular: - _experimental_flag: '' - experimental_features: - _experimental_flag: '' steps: - checkout: self clean: true @@ -129,15 +122,15 @@ stages: workingDirectory: $(Build.SourcesDirectory) installationPath: $(Build.SourcesDirectory)/.dotnet - script: .\eng\common\dotnet.cmd - - script: .\eng\test-determinism.cmd -configuration Debug - env: - FSHARP_EXPERIMENTAL_FEATURES: $(_experimental_flag) - displayName: Determinism tests with Debug configuration + - script: .\eng\test-determinism.cmd -configuration Release + displayName: Determinism tests (race detector — same flags both builds) + - script: .\eng\test-determinism.cmd -configuration Release -mode seq-vs-par + displayName: Determinism tests (1-shot diff — sequential vs parallel) - task: PublishPipelineArtifact@1 displayName: Publish Determinism Logs inputs: - targetPath: '$(Build.SourcesDirectory)/artifacts/log/Debug' - artifactName: 'Determinism_Debug Attempt $(System.JobAttempt) Logs' + targetPath: '$(Build.SourcesDirectory)/artifacts/log/Release' + artifactName: 'Determinism_Release Attempt $(System.JobAttempt) Logs' continueOnError: true condition: not(succeeded()) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index ff1c9aae2dd..fabeb1acea3 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -1,7 +1,10 @@ ### Fixed +* Stabilize codegen order under `--parallelcompilation+` so `--deterministic` Release builds produce byte-identical IL across rebuilds: optimizer Val iteration, IlxGen type/method/field/event emit order, anonymous-record extra-binding drain, and `FileIndex` assignment now follow source position rather than thread-scheduling order. ([Issue #19732](https://github.com/dotnet/fsharp/issues/19732), [PR #19810](https://github.com/dotnet/fsharp/pull/19810)) * Reject non-function bindings for single-case and partial active pattern names with FS1209, matching the existing multi-case behavior. ([PR #19763](https://github.com/dotnet/fsharp/pull/19763)) * Fix FS0421 "The address of the variable cannot be used at this point" incorrectly raised for the discard pattern `let _ = &expr` when `let x = &expr` compiles. ([Issue #18841](https://github.com/dotnet/fsharp/issues/18841), [PR #19811](https://github.com/dotnet/fsharp/pull/19811)) +* Stabilize codegen order under `--parallelcompilation+`: optimizer Val iteration (`DetupleArgs`, `InnerLambdasToTopLevelFuncs`), `IlxGen.TypeDefsBuilder` type-emit order, per-`TypeDefBuilder` method/field/event order, anonymous-record extra-binding drain order, and `FileIndex` assignment during parallel parsing now follow source position rather than thread-scheduling order. The Release `Determinism` CI job (which builds the compiler twice and compares MD5 hashes) covers this end-to-end; a new in-process differential test asserts that `--parallelcompilation+` and `--parallelcompilation-` produce byte-identical IL. ([Issue #19732](https://github.com/dotnet/fsharp/issues/19732), [PR #19810](https://github.com/dotnet/fsharp/pull/19810)) +* Improve determinism under parallel optimization and code generation: optimizer Val iteration, IlxGen TypeDefsBuilder emit order, anonymous-record extra-binding drain order, and FileIndex assignment during parallel parsing are all now stable across builds. This makes Release MVID reproducible. ([Issue #19732](https://github.com/dotnet/fsharp/issues/19732), [PR #19810](https://github.com/dotnet/fsharp/pull/19810)) * Honor `--nowarn` and `--warnaserror` for warnings emitted during command-line option parsing ([Issue #19576](https://github.com/dotnet/fsharp/issues/19576), [PR #19776](https://github.com/dotnet/fsharp/pull/19776)) * Fix `[]` prefix attributes being silently dropped on class members, and fix false-positive `AllowMultiple=false` errors when `[]` and `[]` are applied to the same binding. ([Issue #17904](https://github.com/dotnet/fsharp/issues/17904), [Issue #19020](https://github.com/dotnet/fsharp/issues/19020), [PR #19738](https://github.com/dotnet/fsharp/pull/19738)) * Fix attributes on return type of unparenthesized tuple methods being silently dropped from IL. ([Issue #462](https://github.com/dotnet/fsharp/issues/462), [PR #19714](https://github.com/dotnet/fsharp/pull/19714)) diff --git a/eng/test-determinism.ps1 b/eng/test-determinism.ps1 index a69c677644d..b7c65bffc2c 100644 --- a/eng/test-determinism.ps1 +++ b/eng/test-determinism.ps1 @@ -2,6 +2,8 @@ param([string]$configuration = "Debug", [string]$msbuildEngine = "vs", [string]$altRootDrive = "q:", + [ValidateSet("same", "seq-vs-par")] + [string]$mode = "same", [switch]$help, [switch]$norestore, [switch]$rebuild) @@ -15,6 +17,8 @@ function Print-Usage() { Write-Host " -msbuildEngine Msbuild engine to use to run build ('dotnet', 'vs', or unspecified)." Write-Host " -bootstrapDir Directory containing the bootstrap compiler" Write-Host " -altRootDrive The drive we build on (via subst) for verifying pathmap implementation" + Write-Host " -mode 'same' (default): build twice with identical flags (race-detector)" + Write-Host " 'seq-vs-par': first build sequential, second build parallel (deterministic 1-shot diff)" } if ($help) { @@ -25,7 +29,7 @@ if ($help) { # List of binary names that should be skipped because they have a known issue that # makes them non-deterministic. $script:skipList = @() -function Run-Build([string]$rootDir, [string]$increment) { +function Run-Build([string]$rootDir, [string]$increment, [string]$additionalFscFlags = "") { $logFileName = $increment @@ -62,6 +66,9 @@ function Run-Build([string]$rootDir, [string]$increment) { Stop-Processes Write-Host "Building $solution using $bootstrapDir into '$increment' $incrementDir" + if ($additionalFscFlags -ne "") { + Write-Host " AdditionalFscCmdFlags = '$additionalFscFlags'" + } MSBuild $toolsetBuildProj ` /p:Configuration=$configuration ` /p:Projects=$solution ` @@ -86,6 +93,7 @@ function Run-Build([string]$rootDir, [string]$increment) { /p:RunAnalyzers=false ` /p:RunAnalyzersDuringBuild=false ` /p:BUILDING_USING_DOTNET=false ` + /p:AdditionalFscCmdFlags="$additionalFscFlags" ` /bl:$logFilePath Write-Host "Copy-Item -Path $binDir -Destination $incrementDir -ErrorAction SilentlyContinue -Recurse" @@ -202,9 +210,9 @@ function Test-MapContents($dataMap) { } } -function Test-Build([string]$rootDir, $dataMap, [string]$increment) { +function Test-Build([string]$rootDir, $dataMap, [string]$increment, [string]$additionalFscFlags = "") { $logFileName = $increment - Run-Build $rootDir -increment $increment + Run-Build $rootDir -increment $increment -additionalFscFlags $additionalFscFlags $errorList = @() $allGood = $true @@ -273,14 +281,31 @@ function Test-Build([string]$rootDir, $dataMap, [string]$increment) { } function Run-Test() { + $seqFlags = "" + $parFlags = "" + if ($mode -eq "seq-vs-par") { + # First build: sequential (force single-threaded, disable any parallel test paths) + $seqFlags = "--parallelcompilation- --test:ParallelOff --nowarn:75" + # Second build: parallel (default in modern fsc, made explicit for clarity) + $parFlags = "--parallelcompilation+" + Write-Host "Determinism mode: seq-vs-par" + Write-Host " Initial (seq): $seqFlags" + Write-Host " Test1 (par): $parFlags" + } + else { + Write-Host "Determinism mode: same (race-detector; both builds identical)" + } + # Run the initial build so that we can populate the maps - Run-Build $RepoRoot -increment "Initial" -useBootstrap + Run-Build $RepoRoot -increment "Initial" -additionalFscFlags $seqFlags $dataMap = Record-Binaries $RepoRoot "Initial" Test-MapContents $dataMap - # Run a test against the source in the same directory location - Test-Build -rootDir $RepoRoot -dataMap $dataMap -increment "Test1" + # Run a test against the source in the same directory location. + # In 'same' mode: same flags as Initial (probabilistic race detector). + # In 'seq-vs-par' mode: parallel flags, contrasting against the sequential Initial. + Test-Build -rootDir $RepoRoot -dataMap $dataMap -increment "Test1" -additionalFscFlags $parFlags # Run another build in a different source location and verify that path mapping # allows the build to be identical. To do this we'll copy the entire source diff --git a/src/Compiler/Driver/OptimizeInputs.fs b/src/Compiler/Driver/OptimizeInputs.fs index 78bca4bf979..db69e6fd85c 100644 --- a/src/Compiler/Driver/OptimizeInputs.fs +++ b/src/Compiler/Driver/OptimizeInputs.fs @@ -437,6 +437,14 @@ let ApplyAllOptimizations if tcConfig.extraOptimizationIterations > 0 then addPhase "ExtraLoop" extraLoop + // A per-file naming scope is created at this per-file optimization boundary so that + // compiler-generated names from the Detuple and TLR passes are bucketed by the consumer + // file currently being optimized, rather than by the (possibly inlined) source range of + // each value. This keeps those names deterministic under parallel optimization. + // See https://github.com/dotnet/fsharp/issues/19732. + let mkFileNamingScope (file: CheckedImplFile) = + tcGlobals.CompilerGlobalState.Value.NiceNameGenerator.NewFileScope(file.QualifiedNameOfFile.Range) + let detuple ({ File = file @@ -444,7 +452,8 @@ let ApplyAllOptimizations PrevFile = _prevFile }: PhaseInputs) : PhaseRes = - let file = file |> Detuple.DetupleImplFile ccu tcGlobals + let scope = mkFileNamingScope file + let file = file |> Detuple.DetupleImplFile scope ccu tcGlobals file, prevPhase if tcConfig.doDetuple then @@ -457,9 +466,11 @@ let ApplyAllOptimizations PrevFile = _prevFile }: PhaseInputs) : PhaseRes = + let scope = mkFileNamingScope file + let file = file - |> InnerLambdasToTopLevelFuncs.MakeTopLevelRepresentationDecisions ccu tcGlobals + |> InnerLambdasToTopLevelFuncs.MakeTopLevelRepresentationDecisions scope ccu tcGlobals file, prevPhase @@ -511,8 +522,12 @@ let ApplyAllOptimizations let results, optEnvFirstLoop = match tcConfig.optSettings.processingMode with - // Parallel optimization breaks determinism - turn it off in deterministic builds. | Optimizer.OptimizationProcessingMode.Parallel -> + // Determinism under Parallel mode relies on the per-pass sorts in + // DetupleArgs.determineTransforms and InnerLambdasToTopLevelFuncs.CreateNewValuesForTLR + // (via valSourceOrderKey). Any new pass calling NiceNameGenerator from a + // parallel optimizer phase must sort its Val collection the same way. + // See https://github.com/dotnet/fsharp/issues/19732. let results, optEnvFirstPhase = ParallelOptimization.optimizeFilesInParallel optEnv phases implFiles diff --git a/src/Compiler/Driver/ParseAndCheckInputs.fs b/src/Compiler/Driver/ParseAndCheckInputs.fs index 6c53e11ab14..399656d9ada 100644 --- a/src/Compiler/Driver/ParseAndCheckInputs.fs +++ b/src/Compiler/Driver/ParseAndCheckInputs.fs @@ -736,6 +736,13 @@ let ParseInputFilesInParallel (tcConfig: TcConfig, lexResourceManager, sourceFil for fileName in sourceFiles do checkInputFile tcConfig fileName + // Pre-register FileIndex values in source-file order. Without this, parallel + // parsing races for indices via fileIndexOfFile -> FileIndexTable lock, + // producing non-deterministic FileIndex assignments that leak into IL + // (via debug info, NiceNameGenerator keys, and sort orders downstream). + for fileName in sourceFiles do + FileIndex.fileIndexOfFile fileName |> ignore + let sourceFiles = List.zip sourceFiles isLastCompiland UseMultipleDiagnosticLoggers (sourceFiles, delayLogger, None) (fun sourceFilesWithDelayLoggers -> diff --git a/src/Compiler/Optimize/DetupleArgs.fs b/src/Compiler/Optimize/DetupleArgs.fs index b0dd2d62835..4155484d379 100644 --- a/src/Compiler/Optimize/DetupleArgs.fs +++ b/src/Compiler/Optimize/DetupleArgs.fs @@ -5,6 +5,7 @@ module internal FSharp.Compiler.Detuple open Internal.Utilities.Collections open Internal.Utilities.Library open FSharp.Compiler.DiagnosticsLogger +open FSharp.Compiler.CompilerGlobalState open FSharp.Compiler.Syntax open FSharp.Compiler.TcGlobals open FSharp.Compiler.Text @@ -496,7 +497,7 @@ type Transform = // transform - mkTransform - decided, create necessary stuff //------------------------------------------------------------------------- -let mkTransform g (f: Val) m tps x1Ntys retTy (callPattern, tyfringes: (TType list * Val list) list) = +let mkTransform (scope: PerFileNamingScope) g (f: Val) m tps x1Ntys retTy (callPattern, tyfringes: (TType list * Val list) list) = // Create formal choices for x1...xp under callPattern let transformedFormals = (callPattern, tyfringes) @@ -547,12 +548,12 @@ let mkTransform g (f: Val) m tps x1Ntys retTy (callPattern, tyfringes: (TType li let fCty = mkLambdaTy g tps argTys retTy let transformedVal = - // Ensure that we have an g.CompilerGlobalState - assert (g.CompilerGlobalState |> Option.isSome) - + // Names are bucketed by the per-file optimization scope (not by f.Range, which may point at + // inlined source from another file) to keep compiler-generated names deterministic under + // parallel optimization. f.Range is still used as the Val's source location below. mkLocalVal f.Range - (g.CompilerGlobalState.Value.NiceNameGenerator.FreshCompilerGeneratedName(f.LogicalName, f.Range)) + (scope.Fresh(f.LogicalName, f.Range)) fCty valReprInfo @@ -638,7 +639,7 @@ let decideFormalSuggestedCP g z tys vss = // transform - decideTransform //------------------------------------------------------------------------- -let decideTransform g z v callPatterns (m, tps, vss: Val list list, retTy) = +let decideTransform (scope: PerFileNamingScope) g z v callPatterns (m, tps, vss: Val list list, retTy) = let tys = List.map (typeOfLambdaArg m) vss // NOTE: 'a in arg types may have been instanced at different tuples... @@ -664,7 +665,7 @@ let decideTransform g z v callPatterns (m, tps, vss: Val list list, retTy) = if isTrivialCP callPattern then None // no transform else - Some(v, mkTransform g v m tps tys retTy (callPattern, tyfringes)) + Some(v, mkTransform scope g v m tps tys retTy (callPattern, tyfringes)) //------------------------------------------------------------------------- @@ -686,7 +687,7 @@ let eligibleVal g m (v: Val) = && not // .IsCompiledAsTopLevel && v.IsCompiledAsTopLevel -let determineTransforms g (z: Results) = +let determineTransforms (scope: PerFileNamingScope) g (z: Results) = let selectTransform (f: Val) sites = if not (eligibleVal g f.Range f) then None @@ -702,9 +703,13 @@ let determineTransforms g (z: Results) = | arg1 :: _ -> // consider f let m = arg1.Range // mark of first arg, mostly for error reporting let callPatterns = sitesCPs sites // callPatterns from sites - decideTransform g z f callPatterns (m, tps, vss, retTy) // make transform (if required) + decideTransform scope g z f callPatterns (m, tps, vss, retTy) // make transform (if required) - let vtransforms = Zmap.chooseL selectTransform z.Uses + // See https://github.com/dotnet/fsharp/issues/19732 for why we sort here. + let vtransforms = + Zmap.toList z.Uses + |> List.sortWith (fun (v1, _) (v2, _) -> compare (valSourceOrderKey v1) (valSourceOrderKey v2)) + |> List.choose (fun (f, sites) -> selectTransform f sites) let vtransforms = Zmap.ofList valOrder vtransforms vtransforms @@ -948,12 +953,12 @@ let passImplFile penv assembly = // entry point //------------------------------------------------------------------------- -let DetupleImplFile ccu g expr = +let DetupleImplFile (scope: PerFileNamingScope) ccu g expr = // Collect expr info - wanting usage contexts and bindings let z = GetUsageInfoOfImplFile g expr // For each Val, decide Some "transform", or None if not changing - let vtrans = determineTransforms g z + let vtrans = determineTransforms scope g z // Pass over term, rewriting bindings and fixing up call sites, under penv let penv = diff --git a/src/Compiler/Optimize/DetupleArgs.fsi b/src/Compiler/Optimize/DetupleArgs.fsi index 4dc7c1ac487..787a3cfb688 100644 --- a/src/Compiler/Optimize/DetupleArgs.fsi +++ b/src/Compiler/Optimize/DetupleArgs.fsi @@ -3,10 +3,11 @@ module internal FSharp.Compiler.Detuple open Internal.Utilities.Collections +open FSharp.Compiler.CompilerGlobalState open FSharp.Compiler.TcGlobals open FSharp.Compiler.TypedTree -val DetupleImplFile: CcuThunk -> TcGlobals -> CheckedImplFile -> CheckedImplFile +val DetupleImplFile: PerFileNamingScope -> CcuThunk -> TcGlobals -> CheckedImplFile -> CheckedImplFile module GlobalUsageAnalysis = val GetValsBoundInExpr: Expr -> Zset diff --git a/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fs b/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fs index 885917fee37..4156adbab60 100644 --- a/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fs +++ b/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fs @@ -818,7 +818,7 @@ let ChooseReqdItemPackings g fclassM topValS declist reqdItemsMap = // REVIEW: could do better here by preserving names let MakeSimpleArityInfo tps n = ValReprInfo (ValReprInfo.InferTyparInfo tps, List.replicate n ValReprInfo.unnamedTopArg, ValReprInfo.unnamedRetVal) -let CreateNewValuesForTLR g tlrS arityM fclassM envPackM = +let CreateNewValuesForTLR (scope: PerFileNamingScope) g tlrS arityM fclassM envPackM = let createFHat (f: Val) = let wf = Zmap.force f arityM ("createFHat - wf", (valL >> showL)) @@ -837,14 +837,18 @@ let CreateNewValuesForTLR g tlrS arityM fclassM envPackM = let fHatArity = MakeSimpleArityInfo newTps (envp.ep_aenvs.Length + wf) let fHatName = - // Ensure that we have an g.CompilerGlobalState - assert(g.CompilerGlobalState |> Option.isSome) - g.CompilerGlobalState.Value.NiceNameGenerator.FreshCompilerGeneratedName(name, m) + // Names are bucketed by the per-file optimization scope (not by m, which may point at + // inlined source from another file) to keep compiler-generated names deterministic under + // parallel optimization. m is still used as the new Val's source location below. + scope.Fresh(name, m) let fHat = mkLocalNameTypeArity f.IsCompilerGenerated m fHatName fHatTy (Some fHatArity) fHat - let fs = Zset.elements tlrS + // See https://github.com/dotnet/fsharp/issues/19732 for why we sort here. + let fs = + Zset.elements tlrS + |> List.sortWith (fun v1 v2 -> compare (valSourceOrderKey v1) (valSourceOrderKey v2)) let ffHats = List.map (fun f -> f, createFHat f) fs let fHatM = Zmap.ofList valOrder ffHats fHatM @@ -1345,7 +1349,7 @@ let RecreateUniqueBounds g expr = // entry point //------------------------------------------------------------------------- -let MakeTopLevelRepresentationDecisions ccu g expr = +let MakeTopLevelRepresentationDecisions (scope: PerFileNamingScope) ccu g expr = try // pass1: choose the f to be TLR with arity(f) let tlrS, topValS, arityM = Pass1_DetermineTLRAndArities.DetermineTLRAndArities g expr @@ -1355,7 +1359,7 @@ let MakeTopLevelRepresentationDecisions ccu g expr = // pass3 let envPackM = ChooseReqdItemPackings g fclassM topValS declist reqdItemsMap - let fHatM = CreateNewValuesForTLR g tlrS arityM fclassM envPackM + let fHatM = CreateNewValuesForTLR scope g tlrS arityM fclassM envPackM // pass4: rewrite if verboseTLR then dprintf "TransExpr(rw)------\n" diff --git a/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fsi b/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fsi index 5a745306764..e563469cc16 100644 --- a/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fsi +++ b/src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fsi @@ -2,7 +2,9 @@ module internal FSharp.Compiler.InnerLambdasToTopLevelFuncs +open FSharp.Compiler.CompilerGlobalState open FSharp.Compiler.TypedTree open FSharp.Compiler.TcGlobals -val MakeTopLevelRepresentationDecisions: CcuThunk -> TcGlobals -> CheckedImplFile -> CheckedImplFile +val MakeTopLevelRepresentationDecisions: + PerFileNamingScope -> CcuThunk -> TcGlobals -> CheckedImplFile -> CheckedImplFile diff --git a/src/Compiler/TypedTree/CompilerGlobalState.fs b/src/Compiler/TypedTree/CompilerGlobalState.fs index ab1dde178f0..74389b6a71c 100644 --- a/src/Compiler/TypedTree/CompilerGlobalState.fs +++ b/src/Compiler/TypedTree/CompilerGlobalState.fs @@ -22,20 +22,56 @@ type NiceNameGenerator() = // Cache this as a delegate. let basicNameCountsAddDelegate = Func(fun _ -> ref 0) - let increment basicName (m: range) = - let key = struct (basicName, m.FileIndex) + let incrementBucket basicName (fileIndex: int) = + let key = struct (basicName, fileIndex) let countCell = basicNameCounts.GetOrAdd(key, basicNameCountsAddDelegate) Interlocked.Increment(countCell) - + + let increment basicName (m: range) = incrementBucket basicName m.FileIndex + + let mkName basicName (m: range) count = + CompilerGeneratedNameSuffix basicName (string m.StartLine + (match (count - 1) with 0 -> "" | n -> "-" + string n)) + member _.FreshCompilerGeneratedNameOfBasicName (basicName, m: range) = let count = increment basicName m - CompilerGeneratedNameSuffix basicName (string m.StartLine + (match (count - 1) with 0 -> "" | n -> "-" + string n)) + mkName basicName m count member this.FreshCompilerGeneratedName (name, m: range) = this.FreshCompilerGeneratedNameOfBasicName (GetBasicNameOfPossibleCompilerGeneratedName name, m) member _.IncrementOnly(name: string, m: range) = increment name m + /// Allocate a fresh compiler-generated name whose uniqueness counter is bucketed by an + /// explicit per-file scope (see PerFileNamingScope) rather than by the file index of 'm'. + /// 'm' is used only for the human-readable start-line marker baked into the generated name, + /// so passing a range that points at inlined source code can no longer make compiler-generated + /// names non-deterministic under parallel optimization. See + /// https://github.com/dotnet/fsharp/issues/19732. + member _.FreshCompilerGeneratedNameInScope (scopeFileIndex: int, name: string, m: range) = + let basicName = GetBasicNameOfPossibleCompilerGeneratedName name + let count = incrementBucket basicName scopeFileIndex + mkName basicName m count + + /// Create a naming scope tied to a single ImplFile, identified by 'fileRange' (whose FileIndex + /// is the consumer file currently being optimized). Names allocated through the returned scope + /// are bucketed by that file, so parallel optimization of different files cannot race on a + /// shared name-counter bucket. + member this.NewFileScope (fileRange: range) = PerFileNamingScope(this, fileRange.FileIndex) + +/// A compiler-generated-name allocation scope bound to a single ImplFile being optimized. +/// +/// The constructor is intentionally not part of the public signature: a scope can only be obtained +/// from NiceNameGenerator.NewFileScope at the per-file boundary of the parallel optimizer. This makes +/// it impossible for a call site to accidentally bucket names by the wrong (e.g. inlined-source) file +/// and thereby reintroduce the non-determinism fixed by https://github.com/dotnet/fsharp/issues/19732. +and [] PerFileNamingScope internal (nng: NiceNameGenerator, fileIndex: int) = + + /// Allocate a fresh compiler-generated name within this file's scope. 'm' contributes only the + /// source-location marker in the generated name; the determinism-critical uniqueness bucket is + /// fixed by this scope's file and never by 'm'. + member _.Fresh (name: string, m: range) = + nng.FreshCompilerGeneratedNameInScope(fileIndex, name, m) + /// Generates compiler-generated names marked up with a source code location, but if given the same unique value then /// return precisely the same name. Each name generated also includes the StartLine number of the range passed in /// at the point of first generation. diff --git a/src/Compiler/TypedTree/CompilerGlobalState.fsi b/src/Compiler/TypedTree/CompilerGlobalState.fsi index b308cbe25a7..ee3c6061466 100644 --- a/src/Compiler/TypedTree/CompilerGlobalState.fsi +++ b/src/Compiler/TypedTree/CompilerGlobalState.fsi @@ -19,6 +19,24 @@ type NiceNameGenerator = member FreshCompilerGeneratedName: name: string * m: range -> string member IncrementOnly: name: string * m: range -> int + /// Create a per-file naming scope for the ImplFile identified by 'fileRange' (whose FileIndex is + /// the consumer file being optimized). All names allocated through the returned scope are bucketed + /// by that file, guaranteeing determinism under parallel optimization. + /// See https://github.com/dotnet/fsharp/issues/19732. + member NewFileScope: fileRange: range -> PerFileNamingScope + +/// A compiler-generated-name allocation scope bound to a single ImplFile being optimized. +/// +/// Instances can only be obtained from NiceNameGenerator.NewFileScope at the per-file boundary of the +/// parallel optimizer; the constructor is deliberately not exposed. This prevents a call site from +/// bucketing names by the wrong (e.g. inlined-source) file, which would reintroduce the non-determinism +/// fixed by https://github.com/dotnet/fsharp/issues/19732. +and [] PerFileNamingScope = + + /// Allocate a fresh compiler-generated name within this file's scope. 'm' contributes only the + /// source-location marker baked into the generated name; the uniqueness bucket is this scope's file. + member Fresh: name: string * m: range -> string + /// Generates compiler-generated names marked up with a source code location, but if given the same unique value then /// return precisely the same name. Each name generated also includes the StartLine number of the range passed in /// at the point of first generation. diff --git a/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fs b/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fs index 00761538123..83401b7a9ae 100644 --- a/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fs +++ b/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fs @@ -44,6 +44,15 @@ module internal ExprConstruction = member _.Compare(v1, v2) = compareBy v1 v2 _.Stamp } + // Source-position-derived order key for Vals. Used to walk Val collections + // in a stable, build-independent order before calling NiceNameGenerator + // from parallel optimizer passes. Stamp is the final tiebreaker for + // synthetic Vals at the same location; stamps are fixed within a single + // process so the order is total. See https://github.com/dotnet/fsharp/issues/19732. + let valSourceOrderKey (v: Val) = + let r = v.Range + struct (r.FileIndex, r.StartLine, r.StartColumn, v.LogicalName, v.Stamp) + let tyconOrder = { new IComparer with member _.Compare(tycon1, tycon2) = compareBy tycon1 tycon2 _.Stamp diff --git a/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fsi b/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fsi index 36942be52f1..09a00276dfe 100644 --- a/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fsi +++ b/src/Compiler/TypedTree/TypedTreeOps.ExprConstruction.fsi @@ -22,6 +22,12 @@ module internal ExprConstruction = /// An ordering for value definitions, based on stamp val valOrder: IComparer + /// Stable, source-position-derived key for ordering Vals. + /// Use this before calling NiceNameGenerator from parallel optimizer passes + /// so the generated names do not depend on Val.Stamp assignment race. + /// See https://github.com/dotnet/fsharp/issues/19732. + val valSourceOrderKey: Val -> struct (int * int * int * string * int64) + /// An ordering for type definitions, based on stamp val tyconOrder: IComparer diff --git a/tests/fsharp/Compiler/CodeGen/EmittedIL/DeterministicTests.fs b/tests/fsharp/Compiler/CodeGen/EmittedIL/DeterministicTests.fs index ad43e5b7bf0..0d678b442d1 100644 --- a/tests/fsharp/Compiler/CodeGen/EmittedIL/DeterministicTests.fs +++ b/tests/fsharp/Compiler/CodeGen/EmittedIL/DeterministicTests.fs @@ -334,6 +334,72 @@ let inline myFunc x y = x - y""" else Assert.NotEqual(mvid1,mvid2) + // https://github.com/dotnet/fsharp/issues/19732 + // Multi-file optimized compilation exercises DetupleArgs and TLR (tuple-arg + // functions + nested lambdas). These passes iterate Val sets whose order + // depends on Val.Stamp, which is racy under parallel optimization. + // The fix sorts by source position (valSourceOrderKey) before iterating. + // Note: this in-process test is a regression guard; the full race requires + // large-scale parallel compilation tested by eng/test-determinism.ps1 in Release. + [] + let ``Optimized multi-file assembly should be deterministic`` () = + let outputDir = DirectoryInfo(Path.Combine(Path.GetTempPath(), "fsharp-determinism-test")) + if outputDir.Exists then outputDir.Delete(true) + outputDir.Create() + + let makeFile i = + FsSourceWithFileName + $"File%d{i}.fs" + $""" +module File%d{i} + +let processTuple%d{i} (a: int, b: string) = + let inner x = x + a + (inner 1, b.Length) + +let callSite%d{i} () = + let r1 = processTuple%d{i} (42, "hello") + let r2 = processTuple%d{i} (99, "world") + let nested () = + let deep () = fst r1 + fst r2 + deep () + nested () +""" + + let additionalFiles = [ for i in 2..8 -> makeFile i ] + + let getMvid () = + FSharp + """ +module File1 + +let processTuple1 (a: int, b: string) = + let inner x = x + a + (inner 1, b.Length) + +let callSite1 () = + let r1 = processTuple1 (42, "hello") + let r2 = processTuple1 (99, "world") + let nested () = + let deep () = fst r1 + fst r2 + deep () + nested () +""" + |> withAdditionalSourceFiles additionalFiles + |> asLibrary + |> withOptimize + |> withName "DetTest" + |> withOutputDirectory (Some outputDir) + |> withOptions [ "--deterministic" ] + |> compileGuid + + let mvids = [| for _ in 1..10 -> getMvid () |] + + for i in 1 .. mvids.Length - 1 do + Assert.Equal(mvids.[0], mvids.[i]) + + outputDir.Delete(true) + [] let ``Reference assemblies MVID must change when literal constant value changes`` () = let codeWithLiteral42 = """