fix: compile-check keeps the records of assemblies it blocks

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.
 

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)
+	}
+}

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) {

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"

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

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)
 	}
 }
 

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.