diff --git a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md index b14092db3..059d47371 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 000000000..8dc0ed1ce --- /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 dc651ba5b..452382cea 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 4364b3f37..32ae1e41e 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 8050e6f87..0bc5124dc 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 d285fc3a0..a562f9659 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 fe3eb77b8..7f9895350 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.