From 1ccbf3ddea7cd863cfad66b0902a5ce184ba01bd Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 15 Oct 2026 14:56:10 +0900 Subject: [PATCH] fix: compile-check keeps the records of assemblies it blocks A run where an assembly has errors blocks everything below it. It used to remove the blocked assemblies' outputs and records as well, so the first successful run after the error compiled every dependent again, even when the assembly with errors came back with the same reference assembly. Nothing in the blocking run reads those outputs, since everything that references a blocked assembly is blocked too, and a later run uses them only after checking its input key, output hashes, baseline and non-source key. A reference that comes back the same now leaves the dependents reused or checked on their changed files; one that comes back changed still compiles them. --- .../CliOnlyTools~/CompileCheck/Skill/SKILL.md | 2 +- .../compilecheck/blocked_records_test.go | 172 ++++++++++++++++++ .../internal/compilecheck/fast_path_test.go | 35 ---- .../compilecheck/resultstore/baseline.go | 2 +- .../internal/compilecheck/scheduler.go | 24 ++- .../internal/compilecheck/scheduler_test.go | 6 +- docs/compile-check.md | 10 +- 7 files changed, 195 insertions(+), 56 deletions(-) create mode 100644 cli/dispatcher/internal/compilecheck/blocked_records_test.go diff --git a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md index b14092db3a..059d473714 100644 --- a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md +++ b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md @@ -56,7 +56,7 @@ everything from scratch, which is the way out if a reused result ever looks wron In a large assembly whose last compile-check compile succeeded, an edit that keeps every declaration is checked on the changed files only, in a few seconds. Such a check skips a few assembly-wide rules, so run once with `--full` before finishing. The first body edit after a Unity -build, after a `--full` that had errors, after a run that skipped or blocked the assembly, or after +build, after a `--full` that had errors, after a run that skipped the assembly, or after `--all` in an assembly that references another of the project is compiled in full and records what the next check compares against; `FastPathFallbacks` says why. diff --git a/cli/dispatcher/internal/compilecheck/blocked_records_test.go b/cli/dispatcher/internal/compilecheck/blocked_records_test.go new file mode 100644 index 0000000000..8dc0ed1ce7 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/blocked_records_test.go @@ -0,0 +1,172 @@ +package compilecheck + +import ( + "fmt" + "testing" + + "github.com/hatayama/unity-cli-loop/dispatcher/internal/compilecheck/fastcheck" +) + +// blockAlphaOnce makes Alpha fail to compile, which blocks Beta, and checks that it did. +// Why Alpha's source is edited first: an untouched Alpha would be replayed from the previous run's +// record, and the failure set up here would never be read. +func blockAlphaOnce(t *testing.T, project *cachedSchedulerProject) { + t.Helper() + project.editSource(t, "Alpha", "class Alpha { broken }") + failing := newCachedSchedulerRecorder("Alpha", "Beta") + failing.output["Alpha"] = failingCompilerOutput("Alpha") + // Why the failing compile writes no reference assembly: csc writes none when it reports errors. + delete(failing.surfaces, "Alpha") + outcome, err := project.compile(t.Context(), failing) + if err != nil { + t.Fatalf("expected the failing run to finish, got error: %v", err) + } + assertStrings(t, "blocked", outcome.Blocked, []string{"Beta"}) +} + +// newBlockableProject lays out Alpha and Beta, Beta referencing Alpha and picked only as its +// dependent, and compiles both once. Why Alpha's surface differs from Unity's: when it matches, +// Beta is skipped for that reason and the run never asks whether Beta's records can be reused. +func newBlockableProject(t *testing.T) *cachedSchedulerProject { + t.Helper() + project := newCachedSchedulerProject(t, map[string][]string{"Beta": {"Alpha"}}, "Alpha", "Beta") + markSelectedAsDependent(t, project.plan, "Beta") + project.compileOrFail(t, newCachedSchedulerRecorder("Alpha", "Beta")) + + return project +} + +// Verifies a dependent blocked by one failed run is reused, without a compile, once the assembly it +// references comes back with the same reference assembly it was compiled against. +func TestCompileUnitsReusesABlockedDependentWhenItsReferenceComesBackTheSame(t *testing.T) { + project := newBlockableProject(t) + blockAlphaOnce(t, project) + + project.editSource(t, "Alpha", "class Alpha { }") + recorder := newCachedSchedulerRecorder("Alpha", "Beta") + results := project.compileOrFail(t, recorder) + + if recorder.runs["Alpha"] != 1 { + t.Errorf("Alpha should have compiled once, got %d", recorder.runs["Alpha"]) + } + if recorder.runs["Beta"] != 0 { + t.Errorf("Beta should not have compiled, got %d runs", recorder.runs["Beta"]) + } + if !resultOf(t, results, "Beta").Reused { + t.Errorf("Beta should have been reused, got %+v", resultOf(t, results, "Beta")) + } +} + +// Verifies a dependent blocked by one failed run is compiled again when the assembly it references +// comes back with a different reference assembly, since its record was made against the old one. +func TestCompileUnitsRecompilesABlockedDependentWhenItsReferenceComesBackChanged(t *testing.T) { + project := newBlockableProject(t) + blockAlphaOnce(t, project) + + project.editSource(t, "Alpha", "class Alpha { public void Added() { } }") + recorder := newCachedSchedulerRecorder("Alpha", "Beta") + recorder.surfaces["Alpha"] = "changed surface Alpha" + results := project.compileOrFail(t, recorder) + + if recorder.runs["Beta"] != 1 { + t.Errorf("Beta should have compiled once, got %d runs", recorder.runs["Beta"]) + } + if resultOf(t, results, "Beta").Reused { + t.Error("Beta should not have been reused against a changed reference assembly") + } +} + +// failChangedFilesCheckOfAlpha edits one of Alpha's bodies into an error the changed-files check +// reports, runs the plan, and checks that the error blocked Beta. +func failChangedFilesCheckOfAlpha(t *testing.T, project *fastPathProject) { + t.Helper() + project.editBody(t, 0, "int x = \"s\";") + project.worker.diagnostics = []fastcheck.WorkerDiagnostic{{ + File: project.absoluteSource(0), Line: 1, Column: 20, Severity: severityError, Code: "CS0029", Message: "cannot convert", + }} + outcome := project.run(t, newCachedSchedulerRecorder("Alpha", "Beta"), false) + if result := resultOf(t, outcome.Units, "Alpha"); !result.FastChecked || result.Succeeded { + t.Fatalf("expected a failed changed-files check, got %+v", result) + } + assertStrings(t, "blocked", outcome.Blocked, []string{"Beta"}) + project.worker.diagnostics = nil +} + +// newBlockableFastPathProject lays out a fast path project whose Beta is picked only as Alpha's +// dependent, and records Alpha's baseline. Alpha's surface differs from Unity's for the reason +// newBlockableProject gives. +func newBlockableFastPathProject(t *testing.T) *fastPathProject { + t.Helper() + project := newFastPathProject(t, fastPathMinimumSources) + markSelectedAsDependent(t, project.plan, "Beta") + project.recordBaseline(t) + + return project +} + +// Verifies fixing a typo the changed-files check reported leaves the dependent it blocked reused: +// the check writes no output, so Beta reads the reference assembly it was compiled against. +func TestFastPathFixAfterAnErrorReusesTheBlockedDependents(t *testing.T) { + project := newBlockableFastPathProject(t) + failChangedFilesCheckOfAlpha(t, project) + + project.editBody(t, 0, "fixed") + recorder := newCachedSchedulerRecorder("Alpha", "Beta") + outcome := project.run(t, recorder, false) + + if result := resultOf(t, outcome.Units, "Alpha"); !result.FastChecked || !result.Succeeded { + t.Fatalf("expected a clean changed-files check of Alpha, got %+v", result) + } + if recorder.runs["Beta"] != 0 || !resultOf(t, outcome.Units, "Beta").Reused { + t.Errorf("expected Beta to be reused, got %d runs and %+v", recorder.runs["Beta"], resultOf(t, outcome.Units, "Beta")) + } +} + +// Verifies undoing a typo the changed-files check reported, so the body is back to the baseline, +// leaves both Alpha and the dependent it blocked reused. +func TestFastPathRevertAfterAnErrorReusesTheBlockedDependents(t *testing.T) { + project := newBlockableFastPathProject(t) + failChangedFilesCheckOfAlpha(t, project) + + project.editBody(t, 0, "body") + recorder := newCachedSchedulerRecorder("Alpha", "Beta") + outcome := project.run(t, recorder, false) + + if result := resultOf(t, outcome.Units, "Alpha"); !result.Reused { + t.Fatalf("expected Alpha to be reused, got %+v", result) + } + if recorder.runs["Beta"] != 0 || !resultOf(t, outcome.Units, "Beta").Reused { + t.Errorf("expected Beta to be reused, got %d runs and %+v", recorder.runs["Beta"], resultOf(t, outcome.Units, "Beta")) + } +} + +// Verifies a run that blocks an assembly keeps its baseline, so once the failed reference comes +// back with the same reference assembly, a body edit in the blocked assembly is answered by the +// changed-files check instead of a full compile. +func TestBlockedRunKeepsTheBaselineOfTheBlockedAssembly(t *testing.T) { + project := &fastPathProject{ + cachedSchedulerProject: newCachedSchedulerProject(t, map[string][]string{"Beta": {"Alpha"}}, "Alpha", "Beta"), + worker: newFakeFastCheckWorker(), + } + sources := make([]string, 0, fastPathMinimumSources) + for index := 0; index < fastPathMinimumSources; index++ { + source := fmt.Sprintf("Assets/Beta/File%03d.cs", index) + project.writeFile(t, source, fmt.Sprintf("class File%03d %s body", index, fakeDeclarationSeparator)) + sources = append(sources, source) + } + project.plan.Units[1].Sources = sources + project.run(t, newCachedSchedulerRecorder("Alpha", "Beta"), false) + if !project.hasBaselineOf("Beta") { + t.Fatal("expected the first run to record Beta's baseline") + } + blockAlphaOnce(t, project.cachedSchedulerProject) + + project.editSource(t, "Alpha", "class Alpha { }") + project.writeFile(t, sources[0], fmt.Sprintf("class File000 %s edited", fakeDeclarationSeparator)) + recorder := newCachedSchedulerRecorder("Alpha", "Beta") + outcome := project.run(t, recorder, false) + + if result := resultOf(t, outcome.Units, "Beta"); !result.FastChecked || recorder.runs["Beta"] != 0 { + t.Errorf("expected Beta to be answered by the changed-files check, got %d runs and %+v", recorder.runs["Beta"], result) + } +} diff --git a/cli/dispatcher/internal/compilecheck/fast_path_test.go b/cli/dispatcher/internal/compilecheck/fast_path_test.go index dc651ba5b1..452382cea1 100644 --- a/cli/dispatcher/internal/compilecheck/fast_path_test.go +++ b/cli/dispatcher/internal/compilecheck/fast_path_test.go @@ -3,7 +3,6 @@ package compilecheck import ( "context" "errors" - "fmt" "os" "path/filepath" "reflect" @@ -502,40 +501,6 @@ func TestFallbackReasonHasNoAbsolutePath(t *testing.T) { assertFallback(t, outcome, recorder, "Unsupported: changed file is not a source: "+project.sourceName(0)+" near Library") } -// Verifies that an assembly whose baseline was removed because a reference failed and blocked it -// falls back without claiming it was never compiled in full, since nothing on disk tells the two apart. -func TestBaselineRemovedByBlockedRunIsNotCalledNeverCompiled(t *testing.T) { - project := &fastPathProject{ - cachedSchedulerProject: newCachedSchedulerProject(t, map[string][]string{"Beta": {"Alpha"}}, "Alpha", "Beta"), - worker: newFakeFastCheckWorker(), - } - sources := make([]string, 0, fastPathMinimumSources) - for index := 0; index < fastPathMinimumSources; index++ { - source := fmt.Sprintf("Assets/Beta/File%03d.cs", index) - project.writeFile(t, source, fmt.Sprintf("class File%03d %s body", index, fakeDeclarationSeparator)) - sources = append(sources, source) - } - project.plan.Units[1].Sources = sources - project.run(t, newCachedSchedulerRecorder("Alpha", "Beta"), false) - if !project.hasBaselineOf("Beta") { - t.Fatal("expected the first run to record Beta's baseline") - } - - project.editSource(t, "Alpha", "class Alpha { broken }") - failing := newCachedSchedulerRecorder("Alpha", "Beta") - failing.output["Alpha"] = failingCompilerOutput("Alpha") - assertStrings(t, "blocked", project.run(t, failing, false).Blocked, []string{"Beta"}) - - project.editSource(t, "Alpha", "class Alpha { }") - project.writeFile(t, sources[0], fmt.Sprintf("class File000 %s edited", fakeDeclarationSeparator)) - recorder := newCachedSchedulerRecorder("Alpha", "Beta") - outcome := project.run(t, recorder, false) - - if reason := resultOf(t, outcome.Units, "Beta").FastPathFallbackReason; reason != "no baseline: no full compile of this assembly is on record" { - t.Errorf("Beta fallback reason = %q", reason) - } -} - // Verifies that a unit whose non-source key could not be read before the run falls back with a // reason of its own rather than listing every kind of input as changed. func TestUnreadableNonSourceKeyFallsBack(t *testing.T) { diff --git a/cli/dispatcher/internal/compilecheck/resultstore/baseline.go b/cli/dispatcher/internal/compilecheck/resultstore/baseline.go index 4364b3f370..32ae1e41ee 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/baseline.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/baseline.go @@ -116,7 +116,7 @@ func (s Store[D]) LoadBaseline(assembly string) (Baseline[D], string) { // compile. // Why the reasons are told apart: a full compile with errors drops the baseline an earlier compile // left, and a reason that read as "never compiled" sent users looking for a compile that had run. -// Why a missing manifest says nothing about what happened: a run that skips or blocks the unit +// Why a missing manifest says nothing about what happened: a run that skips the unit // removes the manifest and the baseline alike, so nothing on disk tells that apart from no compile. func (s Store[D]) missingBaselineReason(assembly string) string { const notOnRecord = "no baseline: no full compile of this assembly is on record" diff --git a/cli/dispatcher/internal/compilecheck/scheduler.go b/cli/dispatcher/internal/compilecheck/scheduler.go index 8050e6f874..0bc5124dcd 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler.go +++ b/cli/dispatcher/internal/compilecheck/scheduler.go @@ -140,11 +140,7 @@ func (run *schedulerRun) startReadyUnits(jobs int) error { run.state.ready = run.state.ready[1:] // Why the block is read before the skip: a blocked unit has no result to compare against // anything, so asking whether its references kept their surface answers nothing. - blocked, err := run.skipBlockedUnit(index) - if err != nil { - return err - } - if blocked { + if run.skipBlockedUnit(index) { continue } skipped, err := run.skipUnchangedDependent(index) @@ -165,19 +161,21 @@ func (run *schedulerRun) startReadyUnits(jobs int) error { // to what waits on this one, which is where Unity's own build stops as well: the reference assembly // this unit compiles against was never produced, so all it could report is the metadata it cannot // find rather than anything about itself. -// Why its earlier outputs are removed, as for a skipped dependent: a reference assembly an earlier -// run wrote would otherwise be picked up as this run's own output. -func (run *schedulerRun) skipBlockedUnit(index int) (bool, error) { +// Why its earlier outputs and records are kept: nothing in this run reads them, since every unit +// that references this one is blocked too, and a later run uses them only when the records still +// match: a replayed compile needs its input key, which reads every reference this run rebuilds by +// content and the others by timestamp, and outputs that still hash as recorded; a changed-files +// check needs the baseline's compile result on disk and the same non-source key. +// Removing them made the first successful run after an error compile every dependent again even +// when the assembly with errors came back with the same reference assembly. +func (run *schedulerRun) skipBlockedUnit(index int) bool { if !run.state.blocked[index] { - return false, nil - } - if err := run.compiler.removeUnitOutputs(run.plan, run.plan.Units[index]); err != nil { - return false, err + return false } run.state.blockDependents(index) run.state.release(index, false) - return true, nil + return true } // skipUnchangedDependent leaves out a unit this run picked up only because something it references diff --git a/cli/dispatcher/internal/compilecheck/scheduler_test.go b/cli/dispatcher/internal/compilecheck/scheduler_test.go index d285fc3a01..a562f96599 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_test.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_test.go @@ -494,8 +494,10 @@ func TestCompileUnitsBlocksTheDependentsOfAnAssemblyThatFailedToCompile(t *testi t.Errorf("%s should not have compiled against an assembly that failed", name) } } - if _, statErr := os.Stat(staleOutput); !os.IsNotExist(statErr) { - t.Error("the blocked unit's earlier output should have been removed") + // Why the earlier output stays: nothing in this run reads it, and a later run checks it against + // the unit's record before using it. + if _, statErr := os.Stat(staleOutput); statErr != nil { + t.Errorf("the blocked unit's earlier output should have been kept, got %v", statErr) } } diff --git a/docs/compile-check.md b/docs/compile-check.md index fe3eb77b80..7f9895350b 100644 --- a/docs/compile-check.md +++ b/docs/compile-check.md @@ -153,7 +153,7 @@ build - does not call the worker again: it reports what the previous check repor Otherwise the assembly is compiled in full as before, and `FastPathFallbacks` says why: - `no baseline: no full compile of this assembly is on record` (none ran yet, a later run - skipped or blocked the assembly, which removes its result and baseline, or an input changed while + skipped the assembly, which removes its result and baseline, or an input changed while the last full compile ran, so it left no record; `FastPathBaselineNotes` of that run says which) - `no baseline: the last full compile had errors` (a full compile that fails drops the baseline an earlier one left) @@ -191,9 +191,11 @@ So the first body edit is compiled in full more often than it looks: that references another assembly of the project, the first body edit after `--all` falls back with `inputs other than source contents changed: references`; an assembly that references none is checked as usual. -- A run that skips an assembly because nothing it compiles from changed, or blocks it because an - assembly it references has errors, removes its result and baseline, so its next body edit is - compiled in full. +- A run that skips an assembly because nothing it compiles from changed removes its result and + baseline, so its next body edit is compiled in full. A run that blocks it because an assembly it + references has errors keeps them: the next run checks them against the inputs and output hashes + they were recorded with before using them, so a reference that comes back the same leaves the + assembly reused or checked on its changed files, and one that comes back changed compiles it. - `--full` while the assembly still has errors drops its baseline, so the run after the fix is compiled in full. Running `--full` once the errors are fixed keeps it.