From 0f67fdf9c74bef8ae208b88613a1417d0ee1098f Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 15 Oct 2026 23:30:32 +0900 Subject: [PATCH 1/2] feat: compile-check records whether a compile may still apply while references compile A later change lets an assembly start compiling before the dependents it references finish, reading Unity's reference assembly for them. Such a compile overwrites the assembly's record, so it must not start when the record could still be replayed once those references finish; otherwise a run over the same inputs compiles it twice every time. - The input key now also reports a shape (every part of the key except the references to project assemblies) and the identity of each project assembly reference. Which references are split out depends on whether they name a project assembly, not on whether this run compiles them, so a reference entering or leaving the plan moves neither. The key itself is unchanged. - Compile records keep both, in format version 3. - MayStillApply answers whether a record can still apply with some references unknown. It compares the references first and reads the sources for the shape only when they agree. No behavior change: nothing consults the new check yet. --- .../internal/compilecheck/fast_path_test.go | 4 +- .../internal/compilecheck/input_key.go | 132 ++++++++++++++++-- .../internal/compilecheck/input_key_test.go | 131 +++++++++++++++++ .../internal/compilecheck/result_cache.go | 27 +++- .../compilecheck/result_cache_applies_test.go | 128 +++++++++++++++++ .../resultstore/baseline_contract_test.go | 2 +- .../compilecheck/resultstore/baseline_test.go | 2 +- .../resultstore/compile_result.go | 113 ++++++++++++--- .../resultstore/compile_result_test.go | 122 +++++++++++++++- .../resultstore/record_format_test.go | 6 +- .../compilecheck/resultstore/store.go | 3 +- 11 files changed, 626 insertions(+), 44 deletions(-) create mode 100644 cli/dispatcher/internal/compilecheck/result_cache_applies_test.go diff --git a/cli/dispatcher/internal/compilecheck/fast_path_test.go b/cli/dispatcher/internal/compilecheck/fast_path_test.go index 87b524a3e..b4bdd277d 100644 --- a/cli/dispatcher/internal/compilecheck/fast_path_test.go +++ b/cli/dispatcher/internal/compilecheck/fast_path_test.go @@ -124,7 +124,7 @@ func TestHandCorruptedFailedManifestFallsBack(t *testing.T) { project.recordBaseline(t) project.editBody(t, 0, "edited") baseline := project.baseline(t) - if err := project.records().RecordCompile("Alpha", baseline.InputKey, + if err := project.records().RecordCompile("Alpha", resultstore.KeyIdentity{Key: baseline.InputKey}, resultstore.CompileResult[Diagnostic]{Succeeded: false, Diagnostics: baseline.Diagnostics}, unitOutputNames(project.plan.Units[0])); err != nil { t.Fatalf("failed to record the failed compile: %v", err) } @@ -477,7 +477,7 @@ func TestZeroChangedFilesDropsBaselineAndFallsBack(t *testing.T) { project.recordBaseline(t) baseline := project.baseline(t) records := project.records() - if err := records.RecordCompile("Alpha", "tampered", + if err := records.RecordCompile("Alpha", resultstore.KeyIdentity{Key: "tampered"}, resultstore.CompileResult[Diagnostic]{Succeeded: true, Diagnostics: baseline.Diagnostics}, unitOutputNames(project.plan.Units[0])); err != nil { t.Fatalf("failed to record the tampered compile: %v", err) } diff --git a/cli/dispatcher/internal/compilecheck/input_key.go b/cli/dispatcher/internal/compilecheck/input_key.go index 0ab2617a3..6d7f060f4 100644 --- a/cli/dispatcher/internal/compilecheck/input_key.go +++ b/cli/dispatcher/internal/compilecheck/input_key.go @@ -1,6 +1,7 @@ package compilecheck import ( + "bytes" "crypto/sha256" "encoding/hex" "fmt" @@ -10,6 +11,8 @@ import ( "path/filepath" "sort" "strings" + + "github.com/hatayama/unity-cli-loop/dispatcher/internal/compilecheck/resultstore" ) // pathValuedFlagNames are the flags whose value names a file the project edits and csc reads: they @@ -32,6 +35,22 @@ type inputKeyResult struct { // SourceHashes maps each source, spelled as unit.Sources spells it, to the sha256 of the bytes // the key read from it. SourceHashes map[string]string + // Shape digests the same inputs as Key, in the same order, except the references to project + // assemblies, which ProjectReferences carries one by one instead. + // Why they are split out: a reference still compiling is read from Unity's build when a unit + // starts before it finishes, and from this run's output when it changed. Comparing the rest + // apart from it tells whether a recorded compile can still be replayed once it finishes. + Shape string + // ProjectReferences maps each project assembly the unit references, in the plan or not, to the + // sha256 of the line Key holds for it. + // Why the plan does not decide which references go here: a reference entering or leaving the + // plan leaves the bytes of Key as they were, so it must leave these as they were too. + ProjectReferences map[string]string +} + +// identity is what a record of a compile under this key is kept with. +func (result inputKeyResult) identity() resultstore.KeyIdentity { + return resultstore.KeyIdentity{Key: result.Key, Shape: result.Shape, ProjectReferences: result.ProjectReferences} } // computeInputKey digests everything that decides what csc reports for one assembly, so that two @@ -52,8 +71,54 @@ func computeInputKey( if err != nil { return inputKeyResult{}, err } + shape, projectReferences, err := computeShape(projectRoot, paths, plan, unit, sourceHashes) + if err != nil { + return inputKeyResult{}, err + } + + return inputKeyResult{ + Key: key, SourceHashes: sourceHashes, Shape: shape, ProjectReferences: projectReferences, + }, nil +} + +// computeShape digests the parts of the input key in the same order, writing the sources from the +// hashes the key read and leaving each project assembly reference out of the digest, and reports +// the identity of each of those references apart. +// Why the sources are not read again: the shape must describe the very bytes the key does. +func computeShape( + projectRoot string, paths EditorCompilerPaths, plan BuildPlan, unit CompileUnit, + sourceHashes map[string]string, +) (string, map[string]string, error) { + projectReferences := map[string]string{} + parts := keyParts(projectRoot, paths, plan, unit, func(digest hash.Hash) error { + writeHashedSources(digest, sourceHashes) + + return nil + }) + parts[referencesPartIndex].write = func(digest hash.Hash) error { + return writeShapeReferenceIdentity(digest, projectRoot, plan, unit, projectReferences) + } + digest := sha256.New() + for _, part := range parts { + if err := part.write(digest); err != nil { + return "", nil, err + } + } + + return hex.EncodeToString(digest.Sum(nil)), projectReferences, nil +} - return inputKeyResult{Key: key, SourceHashes: sourceHashes}, nil +// computeProjectReferences reports the identity of each project assembly the unit references, +// reading those references alone. +// Why apart from the key: it answers whether a record can still apply without reading the sources, +// and differs in most cases where the answer is no. +func computeProjectReferences( + projectRoot string, plan BuildPlan, unit CompileUnit, +) (map[string]string, error) { + projectReferences := map[string]string{} + err := writeShapeReferenceIdentity(io.Discard, projectRoot, plan, unit, projectReferences) + + return projectReferences, err } // keyStillHolds reports whether the unit's inputs still give the key the run started from. @@ -122,6 +187,9 @@ var keyPartNames = []string{ "compiler", "response file", "source list", "references", "analyzers", "analyzer inputs", "output name", } +// referencesPartIndex is where keyParts digests the references. +const referencesPartIndex = 3 + // keyParts lists every kind of input of one unit, letting the caller decide how the sources go in. func keyParts( projectRoot string, paths EditorCompilerPaths, plan BuildPlan, unit CompileUnit, @@ -131,7 +199,7 @@ func keyParts( {keyPartNames[0], func(digest hash.Hash) error { return writeCompilerIdentity(digest, paths) }}, {keyPartNames[1], func(digest hash.Hash) error { return writeResponseFileIdentity(digest, unit.Assembly) }}, {keyPartNames[2], sourceWriter}, - {keyPartNames[3], func(digest hash.Hash) error { + {keyPartNames[referencesPartIndex], func(digest hash.Hash) error { return writeReferenceIdentity(digest, projectRoot, plan, unit) }}, {keyPartNames[4], func(digest hash.Hash) error { @@ -223,6 +291,18 @@ func writeSourceIdentity( return nil } +// writeHashedSources writes the same lines writeSourceIdentity writes, from hashes it already read. +func writeHashedSources(digest io.Writer, hashes map[string]string) { + ordered := make([]string, 0, len(hashes)) + for source := range hashes { + ordered = append(ordered, source) + } + sort.Strings(ordered) + for _, source := range ordered { + _, _ = fmt.Fprintf(digest, "source\x00%s\x00%s\n", filepath.ToSlash(source), hashes[source]) + } +} + // writeSourceNames records which sources the assembly compiles, without what they hold. func writeSourceNames(digest hash.Hash, sources []string) { ordered := append([]string{}, sources...) @@ -240,26 +320,58 @@ func writeSourceNames(digest hash.Hash, sources []string) { // is read by timestamp, which moves whenever Unity rebuilt it, and a compile that did not need it // costs less than a missed error. func writeReferenceIdentity( - digest hash.Hash, projectRoot string, plan BuildPlan, unit CompileUnit, + digest io.Writer, projectRoot string, plan BuildPlan, unit CompileUnit, ) error { for _, reference := range unit.Assembly.References { - resolved := rewriteReference(projectRoot, plan, unit, reference) - path := projectPath(projectRoot, resolved) - if isUnderDirectory(resolved, plan.OutputDir) { - if err := writeFileContentEntry(digest, "produced-reference", resolved, path); err != nil { + if err := writeReferenceLine(digest, projectRoot, plan, unit, reference); err != nil { + return err + } + } + + return nil +} + +// writeShapeReferenceIdentity writes the lines writeReferenceIdentity writes for every reference +// that is not a project assembly, and puts the sha256 of the line of each one that is into +// projectReferences under its name. +// Why a name seen twice is simply overwritten: the same reference resolves to the same line. +func writeShapeReferenceIdentity( + digest io.Writer, projectRoot string, plan BuildPlan, unit CompileUnit, + projectReferences map[string]string, +) error { + for _, reference := range unit.Assembly.References { + name, isProjectAssembly := projectAssemblyReferenceName(reference, plan.DagDir) + if !isProjectAssembly { + if err := writeReferenceLine(digest, projectRoot, plan, unit, reference); err != nil { return err } continue } - if err := writeStatEntry(digest, "reference", resolved, path); err != nil { + var line bytes.Buffer + if err := writeReferenceLine(&line, projectRoot, plan, unit, reference); err != nil { return err } + sum := sha256.Sum256(line.Bytes()) + projectReferences[name] = hex.EncodeToString(sum[:]) } return nil } +// writeReferenceLine writes the line one reference contributes to the key. +func writeReferenceLine( + digest io.Writer, projectRoot string, plan BuildPlan, unit CompileUnit, reference string, +) error { + resolved := rewriteReference(projectRoot, plan, unit, reference) + path := projectPath(projectRoot, resolved) + if isUnderDirectory(resolved, plan.OutputDir) { + return writeFileContentEntry(digest, "produced-reference", resolved, path) + } + + return writeStatEntry(digest, "reference", resolved, path) +} + // writeAnalyzerIdentity records the analyzer assemblies csc loads, which are shipped rather than // edited, so their timestamps answer whether they moved. func writeAnalyzerIdentity(digest hash.Hash, projectRoot string, analyzers []string) error { @@ -360,7 +472,7 @@ func splitFlag(line string) (string, string, bool) { } // writeStatEntry records a file by where it is, how big it is and when it last changed. -func writeStatEntry(digest hash.Hash, kind string, name string, path string) error { +func writeStatEntry(digest io.Writer, kind string, name string, path string) error { info, err := os.Stat(path) if err != nil { return fmt.Errorf("failed to read %s: %w", path, err) @@ -372,7 +484,7 @@ func writeStatEntry(digest hash.Hash, kind string, name string, path string) err } // writeFileContentEntry records a file by where it is and what it holds. -func writeFileContentEntry(digest hash.Hash, kind string, name string, path string) error { +func writeFileContentEntry(digest io.Writer, kind string, name string, path string) error { file, err := os.Open(path) if err != nil { return fmt.Errorf("failed to read %s: %w", path, err) diff --git a/cli/dispatcher/internal/compilecheck/input_key_test.go b/cli/dispatcher/internal/compilecheck/input_key_test.go index 6c1d1b218..1cfa5d48b 100644 --- a/cli/dispatcher/internal/compilecheck/input_key_test.go +++ b/cli/dispatcher/internal/compilecheck/input_key_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "reflect" + "sort" "testing" "time" ) @@ -471,3 +472,133 @@ func TestInputKeyReturnsHashesOfContentItRead(t *testing.T) { t.Errorf("expected source hashes %v, got %v", expected, result.SourceHashes) } } + +// identity computes the whole input key result of the unit under test. +func (project *inputKeyProject) identity(t *testing.T) inputKeyResult { + t.Helper() + keyResult, err := computeInputKey(project.root, project.paths, project.plan, project.plan.Units[0]) + if err != nil { + t.Fatalf("expected a key, got error: %v", err) + } + + return keyResult +} + +// assertShapeKept fails when a change that should only move one project reference moved the shape, +// or did not move the key and that reference's identity. +func assertShapeKept(t *testing.T, before inputKeyResult, after inputKeyResult, reference string) { + t.Helper() + if after.Key == before.Key { + t.Error("the change should move the key") + } + if after.ProjectReferences[reference] == before.ProjectReferences[reference] { + t.Errorf("the change should move the identity of %s", reference) + } + if after.Shape != before.Shape { + t.Error("the change should leave the shape alone") + } +} + +// Verifies the shape leaves out the content of a reference this run produced, which the identity of +// that reference carries instead: a record must stay comparable while that reference is unknown. +func TestComputeInputKeyShapeIgnoresTheIdentityOfAProducedReference(t *testing.T) { + project := newInputKeyProject(t) + before := project.identity(t) + + writeProjectFile(t, project.root, + filepath.Join(project.plan.OutputDir, "Beta"+referenceAssemblyExtension), "beta surface with more") + + assertShapeKept(t, before, project.identity(t), "Beta") +} + +// Verifies the shape leaves out the timestamp of Unity's file for a reference this run compiles but +// reads from Unity's build, which the identity of that reference carries instead. +func TestComputeInputKeyShapeIgnoresTheIdentityOfAKeptReference(t *testing.T) { + project := newInputKeyProject(t) + unityReference := filepath.Join(planDagDirectory, "Beta"+referenceAssemblyExtension) + writeProjectFile(t, project.root, unityReference, "unity beta") + project.plan.Units[0].KeptSurfaceReferences = map[string]bool{"Beta": true} + before := project.identity(t) + + touchProjectFile(t, project.root, unityReference) + + assertShapeKept(t, before, project.identity(t), "Beta") +} + +// Verifies the key, the shape and the project reference identities do not depend on whether a +// referenced assembly is in the plan: when it is and kept Unity's surface, the compile reads the +// same file as when it is not, so a record must stay comparable as the reference enters or leaves. +func TestComputeInputKeyShapeDoesNotDependOnThePlan(t *testing.T) { + project := newInputKeyProject(t) + outsidePlan := project.identity(t) + + project.plan.Units = append(project.plan.Units, CompileUnit{Assembly: ResponseFile{AssemblyName: "Gamma"}}) + project.plan.Units[0].KeptSurfaceReferences = map[string]bool{"Gamma": true} + insidePlan := project.identity(t) + + if insidePlan.Key != outsidePlan.Key { + t.Error("the key should not depend on whether a kept reference is in the plan") + } + if insidePlan.Shape != outsidePlan.Shape { + t.Error("the shape should not depend on whether a kept reference is in the plan") + } + if !reflect.DeepEqual(insidePlan.ProjectReferences, outsidePlan.ProjectReferences) { + t.Errorf("project references = %v, want %v", insidePlan.ProjectReferences, outsidePlan.ProjectReferences) + } +} + +// Verifies a reference that is not a project assembly - a package or system dll - stays in the +// shape: it is never unknown while the run compiles, so the shape is what follows it. +func TestComputeInputKeyShapeFollowsAReferenceOutsideTheProject(t *testing.T) { + project := newInputKeyProject(t) + library := filepath.Join("Packages", "Library.dll") + writeProjectFile(t, project.root, library, "library") + project.plan.Units[0].Assembly.References = append(project.plan.Units[0].Assembly.References, library) + before := project.identity(t) + + touchProjectFile(t, project.root, library) + after := project.identity(t) + + if after.Shape == before.Shape { + t.Error("a reference outside the project should move the shape") + } + if !reflect.DeepEqual(after.ProjectReferences, before.ProjectReferences) { + t.Error("a reference outside the project should leave the project reference identities alone") + } +} + +// Verifies the shape follows the content of the sources and the project reference identities do not. +func TestComputeInputKeyShapeFollowsTheSources(t *testing.T) { + project := newInputKeyProject(t) + before := project.identity(t) + + writeProjectFile(t, project.root, filepath.Join("Assets", "Alpha", "Alpha.cs"), "class Alpha { int Added; }") + after := project.identity(t) + + if after.Shape == before.Shape { + t.Error("an edited source should move the shape") + } + if !reflect.DeepEqual(after.ProjectReferences, before.ProjectReferences) { + t.Error("an edited source should leave the project reference identities alone") + } +} + +// Verifies the project reference identities name every project assembly the unit references, in the +// plan or not, and nothing else. +func TestComputeInputKeyNamesEveryProjectReference(t *testing.T) { + project := newInputKeyProject(t) + library := filepath.Join("Packages", "Library.dll") + writeProjectFile(t, project.root, library, "library") + project.plan.Units[0].Assembly.References = append(project.plan.Units[0].Assembly.References, library) + + identity := project.identity(t) + + names := []string{} + for name := range identity.ProjectReferences { + names = append(names, name) + } + sort.Strings(names) + if !reflect.DeepEqual(names, []string{"Beta", "Gamma"}) { + t.Errorf("project references = %v, want [Beta Gamma]", names) + } +} diff --git a/cli/dispatcher/internal/compilecheck/result_cache.go b/cli/dispatcher/internal/compilecheck/result_cache.go index 4592285d0..73302e16c 100644 --- a/cli/dispatcher/internal/compilecheck/result_cache.go +++ b/cli/dispatcher/internal/compilecheck/result_cache.go @@ -63,7 +63,7 @@ func (c Compiler) reuseOrCompile( // putting the file back would replay what the edited file reported. CompileUnit has already // removed the previous record, so leaving this one out makes the next run compile again. if c.keyStillHolds(plan, unit, key) { - if writeErr := records.RecordCompile(unit.Assembly.AssemblyName, key, + if writeErr := records.RecordCompile(unit.Assembly.AssemblyName, keyResult.identity(), resultstore.CompileResult[Diagnostic]{Succeeded: result.Succeeded, Diagnostics: result.Diagnostics}, unitOutputNames(unit)); writeErr != nil { return UnitResult{}, writeErr } @@ -82,6 +82,31 @@ func (c Compiler) reuseOrCompile( return result, nil } +// recordMayStillApply reports whether the unit's recorded compile may be replayed once the +// references named in unknown finish compiling. unit is shaped the way it would start now, reading +// Unity's reference assembly for each of them; what it reads for them is not compared. +// Why the project references are compared before the shape: the shape reads every source of the +// unit, and in the common case where a reference that already finished changed again, the +// references alone answer no. +// Why --all and an unreadable input answer no: --all never replays a record, and a unit whose key +// cannot be computed cannot replay one either, so waiting for either would gain nothing. +func (c Compiler) recordMayStillApply(plan BuildPlan, unit CompileUnit, unknown map[string]bool) bool { + if plan.All { + return false + } + projectReferences, err := computeProjectReferences(c.ProjectRoot, plan, unit) + if err != nil { + return false + } + + return c.records(plan).MayStillApply(unit.Assembly.AssemblyName, projectReferences, unknown, + func() (string, bool) { + keyResult, keyErr := computeInputKey(c.ProjectRoot, c.Paths, plan, unit) + + return keyResult.Shape, keyErr == nil + }) +} + // records opens the store of what earlier runs of the check recorded in the plan's output directory. func (c Compiler) records(plan BuildPlan) resultstore.Store[Diagnostic] { return resultstore.Open[Diagnostic](c.ProjectRoot, plan.OutputDir) diff --git a/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go b/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go new file mode 100644 index 000000000..7a1eb1c0d --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go @@ -0,0 +1,128 @@ +package compilecheck + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// recordMayStillApply asks whether the named unit's record may still apply under plan, with the +// named references still compiling: the unit is shaped the way it would start then, reading Unity's +// reference assembly for each of them. +func (project *cachedSchedulerProject) recordMayStillApply( + t *testing.T, plan BuildPlan, name string, unknownNames ...string, +) bool { + t.Helper() + unknown := map[string]bool{} + for _, unknownName := range unknownNames { + unknown[unknownName] = true + } + for _, unit := range plan.Units { + if unit.Assembly.AssemblyName != name { + continue + } + unit.KeptSurfaceReferences = unknown + compiler := Compiler{Paths: project.paths, ProjectRoot: project.root, Timeout: time.Minute} + + return compiler.recordMayStillApply(plan, unit, unknown) + } + t.Fatalf("the plan has no unit %s", name) + + return false +} + +// newChangedReferenceProject compiles [X, D], D referencing X, where X changed its surface, so D's +// record holds the identity of the reference assembly this run produced for X. +func newChangedReferenceProject(t *testing.T) *cachedSchedulerProject { + t.Helper() + project := newCachedSchedulerProject(t, map[string][]string{"D": {"X"}}, "X", "D") + project.compileOrFail(t, newCachedSchedulerRecorder("X", "D")) + + return project +} + +// Verifies a record holding the produced identity of a reference still may apply while that +// reference is unknown - the next start reads Unity's file for it - even after the produced file +// changed, and stops applying once the unit's own sources change. +func TestCompileUnitsRecordsAKeyAUnitCanStillApplyLater(t *testing.T) { + project := newChangedReferenceProject(t) + + if !project.recordMayStillApply(t, project.plan, "D", "X") { + t.Fatal("the record should still apply while X is unknown") + } + + writeOutputFile(t, project.outputDirectory(), "X"+referenceAssemblyExtension, "another surface X") + if !project.recordMayStillApply(t, project.plan, "D", "X") { + t.Error("the record should still apply while X is unknown, whatever X's output holds") + } + + project.editSource(t, "D", "class D { int Added; }") + if project.recordMayStillApply(t, project.plan, "D", "X") { + t.Error("the record should not apply once D's own sources changed") + } +} + +// Verifies a record does not apply when a reference that is not unknown has another identity, and +// that the answer does not wait on the sources: it stays the same with the unit's source unreadable. +func TestRecordMayStillApplyRefusesAChangedProjectReferenceWithoutReadingSources(t *testing.T) { + project := newChangedReferenceProject(t) + writeOutputFile(t, project.outputDirectory(), "X"+referenceAssemblyExtension, "another surface X") + + if project.recordMayStillApply(t, project.plan, "D") { + t.Fatal("the record should not apply once X's produced reference changed") + } + + if err := os.Remove(filepath.Join(project.root, "Assets", "D", "D.cs")); err != nil { + t.Fatalf("failed to remove D's source: %v", err) + } + if project.recordMayStillApply(t, project.plan, "D") { + t.Error("the record should not apply once X's produced reference changed, with D's source gone") + } +} + +// Verifies a record still may apply after a reference that kept Unity's surface left the plan: the +// compile reads the same file for it either way, so neither the key nor its parts move. +func TestMayStillApplyWhenAReferenceLeftThePlan(t *testing.T) { + project := newCachedSchedulerProject(t, map[string][]string{ + "D1": {"X"}, "D2": {"X", "D1", "W"}, + }, "X", "D1", "D2", "W") + recorder := newCachedSchedulerRecorder("X", "D1", "D2", "W") + recorder.surfaces["W"] = keptSurfaceOf("W") + project.compileOrFail(t, recorder) + + if !project.recordMayStillApply(t, project.withOnlyUnits("X", "D1", "D2"), "D2", "D1") { + t.Error("the record should still apply after the kept reference W left the plan") + } +} + +// Verifies --all never waits on a record: it does not replay one. +func TestRecordMayStillApplyIsFalseForAll(t *testing.T) { + project := newChangedReferenceProject(t) + if !project.recordMayStillApply(t, project.plan, "D", "X") { + t.Fatal("the record should still apply in an ordinary run") + } + + all := project.plan + all.All = true + + if project.recordMayStillApply(t, all, "D", "X") { + t.Error("a run with --all should not wait on a record") + } +} + +// Verifies a unit whose key cannot be computed never waits on a record: it could not replay one. +func TestRecordMayStillApplyIsFalseWhenTheKeyCannotBeComputed(t *testing.T) { + project := newChangedReferenceProject(t) + if !project.recordMayStillApply(t, project.plan, "D", "X") { + t.Fatal("the record should still apply while every input can be read") + } + + if err := os.Remove(filepath.Join(project.root, "Assets", "D", "D.cs")); err != nil { + t.Fatalf("failed to remove D's source: %v", err) + } + + if project.recordMayStillApply(t, project.plan, "D", "X") { + t.Error("a unit whose source is gone should not wait on a record") + } +} diff --git a/cli/dispatcher/internal/compilecheck/resultstore/baseline_contract_test.go b/cli/dispatcher/internal/compilecheck/resultstore/baseline_contract_test.go index 1d925134f..b170c5572 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/baseline_contract_test.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/baseline_contract_test.go @@ -45,7 +45,7 @@ func (project projectStore) recordCompile(t *testing.T, key string, succeeded bo Succeeded: succeeded, Diagnostics: []testDiagnostic{{Severity: "warning", Code: "CS0168", File: "Assets/A.cs", Line: 1, Column: 1}}, } - if err := project.store.RecordCompile("Alpha", key, result, alphaOutputs()); err != nil { + if err := project.store.RecordCompile("Alpha", KeyIdentity{Key: key}, result, alphaOutputs()); err != nil { t.Fatalf("failed to record the compile: %v", err) } } diff --git a/cli/dispatcher/internal/compilecheck/resultstore/baseline_test.go b/cli/dispatcher/internal/compilecheck/resultstore/baseline_test.go index 8557a2d41..acd1d2200 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/baseline_test.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/baseline_test.go @@ -30,7 +30,7 @@ func TestHashGeneratedFilesRecordsOnlyCSharpFiles(t *testing.T) { func TestLoadBaselineReportsABaselineOfAnotherFormat(t *testing.T) { directoryPath := newRecordedOutputs(t) store := directoryStore(directoryPath) - if err := store.RecordCompile("Alpha", "input", CompileResult[testDiagnostic]{Succeeded: true}, alphaOutputs()); err != nil { + if err := store.RecordCompile("Alpha", KeyIdentity{Key: "input"}, CompileResult[testDiagnostic]{Succeeded: true}, alphaOutputs()); err != nil { t.Fatalf("failed to write the manifest: %v", err) } if err := writeBaselineRecord(directoryPath, "Alpha", baselineRecord{ diff --git a/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go b/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go index cd240cff1..19828a31b 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go @@ -12,8 +12,9 @@ const ( // what belongs in the key is part of this check's code, so a manifest from another version says // nothing about whether this one would compile the same thing. So is the order the sources are // handed to csc in, which decides the bytes of what it writes without being in the key: version 1 - // compiled them in ordinal order rather than Unity's. - resultManifestFormatVersion = 2 + // compiled them in ordinal order rather than Unity's. Version 2 had no shape, so it could not say + // whether it may still apply while some of its references are compiling. + resultManifestFormatVersion = 3 resultManifestExtension = ".result.json" ) @@ -24,13 +25,26 @@ type CompileResult[D any] struct { Diagnostics []D } +// KeyIdentity is the input key a compile ran on, together with the parts of it that tell whether the +// compile may still apply while some of the project assemblies it references are compiling. +type KeyIdentity struct { + Key string + // Shape digests every input of the key except the references to project assemblies. + Shape string + // ProjectReferences maps each project assembly the unit references to the identity the key holds + // for it. + ProjectReferences map[string]string +} + // unitResultManifest is what one finished compile left behind for the next run: the inputs it ran // on, what it reported, and the outputs it wrote. type unitResultManifest[D any] struct { - FormatVersion int - InputKey string - Succeeded bool - Diagnostics []D + FormatVersion int + InputKey string + Shape string + ProjectReferences map[string]string + Succeeded bool + Diagnostics []D // Outputs maps the name of each file the compile wrote in the check's output directory to its // content hash, so a run that finds them changed underneath compiles again. Outputs map[string]string @@ -46,16 +60,8 @@ func manifestPath(directoryPath string, assembly string) string { // Why anything unexpected simply means no: the caller compiles the unit, which is what it would // have done anyway. Nothing here is worth failing a run over. func (s Store[D]) Reusable(assembly string, key string) (CompileResult[D], bool) { - content, err := os.ReadFile(manifestPath(s.directoryPath(), assembly)) - if err != nil { - return CompileResult[D]{}, false - } - - var manifest unitResultManifest[D] - if err := json.Unmarshal(content, &manifest); err != nil { - return CompileResult[D]{}, false - } - if manifest.FormatVersion != resultManifestFormatVersion || manifest.InputKey != key || key == "" { + manifest, found := s.loadManifest(assembly) + if !found || manifest.InputKey != key || key == "" { return CompileResult[D]{}, false } if !outputsStillMatch(s.directoryPath(), manifest.Outputs) { @@ -68,6 +74,63 @@ func (s Store[D]) Reusable(assembly string, key string) (CompileResult[D], bool) return CompileResult[D]{Succeeded: manifest.Succeeded, Diagnostics: manifest.Diagnostics}, true } +// MayStillApply reports whether the assembly's recorded compile may become reusable once the +// references named in unknown finish compiling: every project reference outside unknown has the +// identity it was recorded with, the same project assemblies are referenced, the outputs still hold +// what that compile wrote, and shape gives the recorded shape. +// Why shape is a function: it reads every source of the assembly, and is asked for only when every +// other check has passed, which in most cases where the answer is no it has not. +// Why anything unexpected means no: the caller then compiles without waiting, which is what it did +// before this check existed. +func (s Store[D]) MayStillApply( + assembly string, projectReferences map[string]string, unknown map[string]bool, shape func() (string, bool), +) bool { + manifest, found := s.loadManifest(assembly) + if !found || !sameKnownReferences(manifest.ProjectReferences, projectReferences, unknown) { + return false + } + if !outputsStillMatch(s.directoryPath(), manifest.Outputs) { + return false + } + current, computed := shape() + + return computed && current == manifest.Shape +} + +// sameKnownReferences reports whether two sets of project reference identities name the same +// assemblies and agree on every one outside unknown. +func sameKnownReferences(recorded map[string]string, current map[string]string, unknown map[string]bool) bool { + if len(recorded) != len(current) { + return false + } + for name, identity := range current { + recordedIdentity, found := recorded[name] + if !found { + return false + } + if !unknown[name] && recordedIdentity != identity { + return false + } + } + + return true +} + +// loadManifest reads the assembly's manifest when one of the current format is on disk. +func (s Store[D]) loadManifest(assembly string) (unitResultManifest[D], bool) { + content, err := os.ReadFile(manifestPath(s.directoryPath(), assembly)) + if err != nil { + return unitResultManifest[D]{}, false + } + + var manifest unitResultManifest[D] + if err := json.Unmarshal(content, &manifest); err != nil { + return unitResultManifest[D]{}, false + } + + return manifest, manifest.FormatVersion == resultManifestFormatVersion +} + // outputsStillMatch reports whether every file a compile wrote is still there and still holds what // it wrote. func outputsStillMatch(directoryPath string, outputs map[string]string) bool { @@ -81,12 +144,14 @@ func outputsStillMatch(directoryPath string, outputs map[string]string) bool { return true } -// RecordCompile records what a compile of the assembly ran on (key) and what it reported, together +// RecordCompile records what a compile of the assembly ran on (identity) and what it reported, together // with the outputs it wrote, named in the output directory. It records nothing when that compile // claimed success without producing every one of those outputs. // Why it is written through a temporary file: a run interrupted while writing would otherwise leave // half a manifest behind, and the next run would read it as a description of outputs it never saw. -func (s Store[D]) RecordCompile(assembly string, key string, result CompileResult[D], outputs []string) error { +func (s Store[D]) RecordCompile( + assembly string, identity KeyIdentity, result CompileResult[D], outputs []string, +) error { hashes, complete, err := hashOutputs(s.directoryPath(), outputs, result.Succeeded) if err != nil { return err @@ -96,11 +161,13 @@ func (s Store[D]) RecordCompile(assembly string, key string, result CompileResul } content, err := json.Marshal(unitResultManifest[D]{ - FormatVersion: resultManifestFormatVersion, - InputKey: key, - Succeeded: result.Succeeded, - Diagnostics: result.Diagnostics, - Outputs: hashes, + FormatVersion: resultManifestFormatVersion, + InputKey: identity.Key, + Shape: identity.Shape, + ProjectReferences: identity.ProjectReferences, + Succeeded: result.Succeeded, + Diagnostics: result.Diagnostics, + Outputs: hashes, }) if err != nil { return fmt.Errorf("failed to record the result of %s: %w", assembly, err) diff --git a/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go b/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go index 97c95ed20..95b169ddd 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go @@ -46,7 +46,7 @@ func storedResult() CompileResult[testDiagnostic] { // recordResult stores one compile of Alpha's outcome under a key. func recordResult(t *testing.T, directoryPath string, key string) { t.Helper() - if err := directoryStore(directoryPath).RecordCompile("Alpha", key, storedResult(), alphaOutputs()); err != nil { + if err := directoryStore(directoryPath).RecordCompile("Alpha", KeyIdentity{Key: key}, storedResult(), alphaOutputs()); err != nil { t.Fatalf("failed to write the manifest: %v", err) } } @@ -197,7 +197,7 @@ func TestRecordCompileReportsAFailedRename(t *testing.T) { directoryPath := newRecordedOutputs(t) blockManifestPath(t, manifestPath(directoryPath, "Alpha")) - err := directoryStore(directoryPath).RecordCompile("Alpha", "key", storedResult(), alphaOutputs()) + err := directoryStore(directoryPath).RecordCompile("Alpha", KeyIdentity{Key: "key"}, storedResult(), alphaOutputs()) if err == nil || !strings.HasPrefix(err.Error(), "failed to write "+manifestPath(directoryPath, "Alpha")+":") { t.Fatalf("err = %v", err) @@ -213,7 +213,7 @@ func TestRecordCompileReportsAFailedRename(t *testing.T) { func TestRecordCompileReportsAMissingOutputDirectory(t *testing.T) { directoryPath := filepath.Join(t.TempDir(), "missing") - err := directoryStore(directoryPath).RecordCompile("Alpha", "key", storedResult(), alphaOutputs()) + err := directoryStore(directoryPath).RecordCompile("Alpha", KeyIdentity{Key: "key"}, storedResult(), alphaOutputs()) if err == nil || !strings.HasPrefix(err.Error(), "failed to write "+manifestPath(directoryPath, "Alpha")+":") { t.Fatalf("err = %v", err) @@ -271,3 +271,119 @@ func TestForgetRemovesTheManifestTheBaselineAndTheGeneratorOutput(t *testing.T) } } } + +// recordIdentity stores one compile of Alpha's outcome under the shape and project reference +// identities of the key it ran on. +func recordIdentity(t *testing.T, directoryPath string, shape string, projectReferences map[string]string) { + t.Helper() + identity := KeyIdentity{Key: "key", Shape: shape, ProjectReferences: projectReferences} + if err := directoryStore(directoryPath).RecordCompile("Alpha", identity, storedResult(), alphaOutputs()); err != nil { + t.Fatalf("failed to write the manifest: %v", err) + } +} + +// shapeOf hands MayStillApply a shape, counting how many times it was asked for one. +func shapeOf(shape string, asked *int) func() (string, bool) { + return func() (string, bool) { + *asked++ + + return shape, true + } +} + +// Verifies a record still may apply when only the identities of the unknown references differ: +// those references are still compiling, so the key they will end up giving is not known yet. +func TestMayStillApplyWhenOnlyUnknownReferencesDiffer(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "produced beta", "Gamma": "gamma"}) + asked := 0 + + mayApply := directoryStore(directoryPath).MayStillApply("Alpha", + map[string]string{"Beta": "unity beta", "Gamma": "gamma"}, map[string]bool{"Beta": true}, shapeOf("shape", &asked)) + + if !mayApply { + t.Error("a record differing only in an unknown reference should still apply") + } +} + +// Verifies a record no longer applies when a reference that is not unknown has another identity, +// and that the shape, which reads every source, is not asked for then. +func TestMayStillApplyRefusesAChangedKnownReference(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta", "Gamma": "gamma"}) + asked := 0 + + mayApply := directoryStore(directoryPath).MayStillApply("Alpha", + map[string]string{"Beta": "beta", "Gamma": "another gamma"}, map[string]bool{"Beta": true}, shapeOf("shape", &asked)) + + if mayApply { + t.Error("a record whose known reference changed should not apply") + } + if asked != 0 { + t.Errorf("the shape was asked for %d times, want none once a known reference differs", asked) + } +} + +// Verifies a record no longer applies when the inputs other than the project references differ. +func TestMayStillApplyRefusesAnotherShape(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta"}) + asked := 0 + + mayApply := directoryStore(directoryPath).MayStillApply("Alpha", + map[string]string{"Beta": "beta"}, map[string]bool{"Beta": true}, shapeOf("another shape", &asked)) + + if mayApply { + t.Error("a record of another shape should not apply") + } +} + +// Verifies a record no longer applies when the unit references another set of project assemblies, +// whether one was added or one was dropped, even when the difference is an unknown reference. +func TestMayStillApplyRefusesAnotherSetOfReferences(t *testing.T) { + cases := map[string]map[string]string{ + "a reference the record lacks": {"Beta": "beta", "Gamma": "gamma", "Delta": "delta"}, + "a reference no longer there": {"Beta": "beta"}, + } + for name, current := range cases { + t.Run(name, func(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta", "Gamma": "gamma"}) + asked := 0 + unknown := map[string]bool{"Gamma": true, "Delta": true} + + if directoryStore(directoryPath).MayStillApply("Alpha", current, unknown, shapeOf("shape", &asked)) { + t.Error("a record over another set of project references should not apply") + } + }) + } +} + +// Verifies a record no longer applies once an output it hashed holds other bytes. +func TestMayStillApplyRefusesChangedOutputs(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta"}) + writeOutputFile(t, directoryPath, "Alpha"+referenceAssemblyExtension, "another surface") + asked := 0 + + if directoryStore(directoryPath).MayStillApply("Alpha", + map[string]string{"Beta": "beta"}, map[string]bool{}, shapeOf("shape", &asked)) { + t.Error("a record whose outputs changed should not apply") + } +} + +// Verifies a record of format version 2, which kept no shape, neither may apply nor is reused. +func TestMayStillApplyRefusesAnotherFormatVersion(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta"}) + setManifestFormatVersion(t, directoryPath, "Alpha", 2) + asked := 0 + + if directoryStore(directoryPath).MayStillApply("Alpha", + map[string]string{"Beta": "beta"}, map[string]bool{}, shapeOf("shape", &asked)) { + t.Error("a record of format version 2 should not apply") + } + if _, reusable := directoryStore(directoryPath).Reusable("Alpha", "key"); reusable { + t.Error("a record of format version 2 should not be reused") + } +} diff --git a/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go b/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go index e4e61f11d..84efd6a86 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go @@ -38,11 +38,13 @@ func TestWriteUnitResultManifestWritesTheRecordedFormat(t *testing.T) { }}, } - if err := directoryStore(directoryPath).RecordCompile("Alpha", "key", result, alphaOutputs()); err != nil { + if err := directoryStore(directoryPath).RecordCompile("Alpha", KeyIdentity{ + Key: "key", Shape: "shape", ProjectReferences: map[string]string{"Beta": "beta"}, + }, result, alphaOutputs()); err != nil { t.Fatalf("failed to write the manifest: %v", err) } - assertFileContent(t, manifestPath(directoryPath, "Alpha"), `{"FormatVersion":2,"InputKey":"key","Succeeded":false,`+ + assertFileContent(t, manifestPath(directoryPath, "Alpha"), `{"FormatVersion":3,"InputKey":"key","Shape":"shape","ProjectReferences":{"Beta":"beta"},"Succeeded":false,`+ `"Diagnostics":[{"Severity":"error","Code":"CS0103","Message":"The name 'Missing' does not exist in the current context",`+ `"File":"Assets/Alpha/Alpha.cs","Line":3,"Column":9}],`+ `"Outputs":{"Alpha.dll":"e020bdf568793dee747a220086c5e53c8b3af5abd959383a31297f303577efe6",`+ diff --git a/cli/dispatcher/internal/compilecheck/resultstore/store.go b/cli/dispatcher/internal/compilecheck/resultstore/store.go index 2de3bd030..ddef83305 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/store.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/store.go @@ -14,7 +14,8 @@ // - Records are written through a temporary file and renamed into place, so a run stopped while // writing never leaves half a record behind. // - A compile result is reusable only under the key it was recorded with, and only while every -// output it hashed still holds those bytes. +// output it hashed still holds those bytes. It may still apply while some project references are +// unknown only when every other part of that key, and those outputs, are as recorded. // - A baseline is handed out only when the compile result recorded with it is still on disk: the // same key, a successful compile, and outputs that still match. Otherwise LoadBaseline says why // there is none, in one of these reasons: From c980c3a7a33afb4f4a9b5425d1efb140eb9bfec7 Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 15 Oct 2026 23:49:48 +0900 Subject: [PATCH 2/2] fix: compile-check no longer waits on a record a rebuilt Unity reference ruled out A record that read a still-compiling reference from Unity's build was said to still apply after Unity rebuilt that reference, although no way the reference could finish would give the recorded key back. A unit would then wait for its references instead of starting. - Each project reference identity now says whether its line read this run's output or Unity's build. An unknown reference the record read from Unity's build must still match; only one it read from this run's output is left uncompared. A known reference must match in kind as well as hash. - MayStillApply takes the shape as a function returning an error and asks for it last, after the references and the outputs, so the store's own tests can see that a difference in the references answers without the shape. The compiler-level test that claimed the same could not tell, and is removed. --- .../internal/compilecheck/input_key.go | 40 +++--- .../internal/compilecheck/input_key_test.go | 10 +- .../internal/compilecheck/result_cache.go | 6 +- .../compilecheck/result_cache_applies_test.go | 29 ++-- .../resultstore/compile_result.go | 60 ++++++--- .../resultstore/compile_result_test.go | 124 ++++++++++++++---- .../resultstore/record_format_test.go | 4 +- 7 files changed, 199 insertions(+), 74 deletions(-) diff --git a/cli/dispatcher/internal/compilecheck/input_key.go b/cli/dispatcher/internal/compilecheck/input_key.go index 6d7f060f4..f01c39a88 100644 --- a/cli/dispatcher/internal/compilecheck/input_key.go +++ b/cli/dispatcher/internal/compilecheck/input_key.go @@ -42,10 +42,10 @@ type inputKeyResult struct { // apart from it tells whether a recorded compile can still be replayed once it finishes. Shape string // ProjectReferences maps each project assembly the unit references, in the plan or not, to the - // sha256 of the line Key holds for it. + // sha256 of the line Key holds for it and whether that line reads this run's output. // Why the plan does not decide which references go here: a reference entering or leaving the // plan leaves the bytes of Key as they were, so it must leave these as they were too. - ProjectReferences map[string]string + ProjectReferences map[string]resultstore.ReferenceIdentity } // identity is what a record of a compile under this key is kept with. @@ -88,8 +88,8 @@ func computeInputKey( func computeShape( projectRoot string, paths EditorCompilerPaths, plan BuildPlan, unit CompileUnit, sourceHashes map[string]string, -) (string, map[string]string, error) { - projectReferences := map[string]string{} +) (string, map[string]resultstore.ReferenceIdentity, error) { + projectReferences := map[string]resultstore.ReferenceIdentity{} parts := keyParts(projectRoot, paths, plan, unit, func(digest hash.Hash) error { writeHashedSources(digest, sourceHashes) @@ -114,8 +114,8 @@ func computeShape( // and differs in most cases where the answer is no. func computeProjectReferences( projectRoot string, plan BuildPlan, unit CompileUnit, -) (map[string]string, error) { - projectReferences := map[string]string{} +) (map[string]resultstore.ReferenceIdentity, error) { + projectReferences := map[string]resultstore.ReferenceIdentity{} err := writeShapeReferenceIdentity(io.Discard, projectRoot, plan, unit, projectReferences) return projectReferences, err @@ -323,7 +323,7 @@ func writeReferenceIdentity( digest io.Writer, projectRoot string, plan BuildPlan, unit CompileUnit, ) error { for _, reference := range unit.Assembly.References { - if err := writeReferenceLine(digest, projectRoot, plan, unit, reference); err != nil { + if _, err := writeReferenceLine(digest, projectRoot, plan, unit, reference); err != nil { return err } } @@ -332,44 +332,50 @@ func writeReferenceIdentity( } // writeShapeReferenceIdentity writes the lines writeReferenceIdentity writes for every reference -// that is not a project assembly, and puts the sha256 of the line of each one that is into -// projectReferences under its name. +// that is not a project assembly, and puts the sha256 of the line of each one that is, with which +// file that line reads, into projectReferences under its name. // Why a name seen twice is simply overwritten: the same reference resolves to the same line. func writeShapeReferenceIdentity( digest io.Writer, projectRoot string, plan BuildPlan, unit CompileUnit, - projectReferences map[string]string, + projectReferences map[string]resultstore.ReferenceIdentity, ) error { for _, reference := range unit.Assembly.References { name, isProjectAssembly := projectAssemblyReferenceName(reference, plan.DagDir) if !isProjectAssembly { - if err := writeReferenceLine(digest, projectRoot, plan, unit, reference); err != nil { + if _, err := writeReferenceLine(digest, projectRoot, plan, unit, reference); err != nil { return err } continue } var line bytes.Buffer - if err := writeReferenceLine(&line, projectRoot, plan, unit, reference); err != nil { + produced, err := writeReferenceLine(&line, projectRoot, plan, unit, reference) + if err != nil { return err } sum := sha256.Sum256(line.Bytes()) - projectReferences[name] = hex.EncodeToString(sum[:]) + kind := resultstore.UnityReference + if produced { + kind = resultstore.ProducedReference + } + projectReferences[name] = resultstore.ReferenceIdentity{Kind: kind, Hash: hex.EncodeToString(sum[:])} } return nil } -// writeReferenceLine writes the line one reference contributes to the key. +// writeReferenceLine writes the line one reference contributes to the key, and reports whether that +// line reads this run's output rather than Unity's build. func writeReferenceLine( digest io.Writer, projectRoot string, plan BuildPlan, unit CompileUnit, reference string, -) error { +) (bool, error) { resolved := rewriteReference(projectRoot, plan, unit, reference) path := projectPath(projectRoot, resolved) if isUnderDirectory(resolved, plan.OutputDir) { - return writeFileContentEntry(digest, "produced-reference", resolved, path) + return true, writeFileContentEntry(digest, "produced-reference", resolved, path) } - return writeStatEntry(digest, "reference", resolved, path) + return false, writeStatEntry(digest, "reference", resolved, path) } // writeAnalyzerIdentity records the analyzer assemblies csc loads, which are shipped rather than diff --git a/cli/dispatcher/internal/compilecheck/input_key_test.go b/cli/dispatcher/internal/compilecheck/input_key_test.go index 1cfa5d48b..d9f874b49 100644 --- a/cli/dispatcher/internal/compilecheck/input_key_test.go +++ b/cli/dispatcher/internal/compilecheck/input_key_test.go @@ -9,6 +9,8 @@ import ( "sort" "testing" "time" + + "github.com/hatayama/unity-cli-loop/dispatcher/internal/compilecheck/resultstore" ) // inputKeyProject is a project tree holding every kind of input one unit's key is built from, so a @@ -584,7 +586,7 @@ func TestComputeInputKeyShapeFollowsTheSources(t *testing.T) { } // Verifies the project reference identities name every project assembly the unit references, in the -// plan or not, and nothing else. +// plan or not, and nothing else, each with the kind of file its line reads. func TestComputeInputKeyNamesEveryProjectReference(t *testing.T) { project := newInputKeyProject(t) library := filepath.Join("Packages", "Library.dll") @@ -601,4 +603,10 @@ func TestComputeInputKeyNamesEveryProjectReference(t *testing.T) { if !reflect.DeepEqual(names, []string{"Beta", "Gamma"}) { t.Errorf("project references = %v, want [Beta Gamma]", names) } + if kind := identity.ProjectReferences["Beta"].Kind; kind != resultstore.ProducedReference { + t.Errorf("Beta, read from this run's output, is of kind %q", kind) + } + if kind := identity.ProjectReferences["Gamma"].Kind; kind != resultstore.UnityReference { + t.Errorf("Gamma, read from Unity's build, is of kind %q", kind) + } } diff --git a/cli/dispatcher/internal/compilecheck/result_cache.go b/cli/dispatcher/internal/compilecheck/result_cache.go index 73302e16c..7453b858e 100644 --- a/cli/dispatcher/internal/compilecheck/result_cache.go +++ b/cli/dispatcher/internal/compilecheck/result_cache.go @@ -99,11 +99,13 @@ func (c Compiler) recordMayStillApply(plan BuildPlan, unit CompileUnit, unknown return false } + // Why the sources are hashed inside the function only: the store asks for the shape last, and + // most answers of no come from the references before it. return c.records(plan).MayStillApply(unit.Assembly.AssemblyName, projectReferences, unknown, - func() (string, bool) { + func() (string, error) { keyResult, keyErr := computeInputKey(c.ProjectRoot, c.Paths, plan, unit) - return keyResult.Shape, keyErr == nil + return keyResult.Shape, keyErr }) } diff --git a/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go b/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go index 7a1eb1c0d..db3351f39 100644 --- a/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go +++ b/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go @@ -63,21 +63,26 @@ func TestCompileUnitsRecordsAKeyAUnitCanStillApplyLater(t *testing.T) { } } -// Verifies a record does not apply when a reference that is not unknown has another identity, and -// that the answer does not wait on the sources: it stays the same with the unit's source unreadable. -func TestRecordMayStillApplyRefusesAChangedProjectReferenceWithoutReadingSources(t *testing.T) { - project := newChangedReferenceProject(t) - writeOutputFile(t, project.outputDirectory(), "X"+referenceAssemblyExtension, "another surface X") - - if project.recordMayStillApply(t, project.plan, "D") { - t.Fatal("the record should not apply once X's produced reference changed") +// Verifies a record that read an unknown reference from Unity's build stops applying once Unity +// rebuilt that file: whatever the reference finishes as, the key can never read the recorded line +// again. +func TestRecordMayStillApplyRefusesAnUnknownReferenceUnityRebuilt(t *testing.T) { + project := newCachedSchedulerProject(t, map[string][]string{"D": {"X"}}, "X", "D") + recorder := newCachedSchedulerRecorder("X", "D") + recorder.surfaces["X"] = keptSurfaceOf("X") + project.compileOrFail(t, recorder) + if !project.recordMayStillApply(t, project.plan, "D", "X") { + t.Fatal("the record should still apply while Unity's file for X is as it read it") } - if err := os.Remove(filepath.Join(project.root, "Assets", "D", "D.cs")); err != nil { - t.Fatalf("failed to remove D's source: %v", err) + rebuilt := time.Now().Add(time.Hour) + unityReference := filepath.Join(project.root, planDagDirectory, "X"+referenceAssemblyExtension) + if err := os.Chtimes(unityReference, rebuilt, rebuilt); err != nil { + t.Fatalf("failed to move Unity's reference assembly of X: %v", err) } - if project.recordMayStillApply(t, project.plan, "D") { - t.Error("the record should not apply once X's produced reference changed, with D's source gone") + + if project.recordMayStillApply(t, project.plan, "D", "X") { + t.Error("the record should not apply once Unity rebuilt the reference assembly it read for X") } } diff --git a/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go b/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go index 19828a31b..8d39cfa3a 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go @@ -33,7 +33,23 @@ type KeyIdentity struct { Shape string // ProjectReferences maps each project assembly the unit references to the identity the key holds // for it. - ProjectReferences map[string]string + ProjectReferences map[string]ReferenceIdentity +} + +const ( + // ProducedReference is the kind of a reference the compile read from this run's own output. + ProducedReference = "produced" + // UnityReference is the kind of a reference the compile read from Unity's build. + UnityReference = "unity" +) + +// ReferenceIdentity is what an input key holds for one project assembly reference: which file the +// compile read it from, and a hash of the line the key holds for it. +// Why the kind is kept with the hash: a reference still compiling is read from Unity's build, and +// whether a record can still apply once it finishes depends on which file the record read. +type ReferenceIdentity struct { + Kind string + Hash string } // unitResultManifest is what one finished compile left behind for the next run: the inputs it ran @@ -42,7 +58,7 @@ type unitResultManifest[D any] struct { FormatVersion int InputKey string Shape string - ProjectReferences map[string]string + ProjectReferences map[string]ReferenceIdentity Succeeded bool Diagnostics []D // Outputs maps the name of each file the compile wrote in the check's output directory to its @@ -75,31 +91,42 @@ func (s Store[D]) Reusable(assembly string, key string) (CompileResult[D], bool) } // MayStillApply reports whether the assembly's recorded compile may become reusable once the -// references named in unknown finish compiling: every project reference outside unknown has the -// identity it was recorded with, the same project assemblies are referenced, the outputs still hold -// what that compile wrote, and shape gives the recorded shape. -// Why shape is a function: it reads every source of the assembly, and is asked for only when every -// other check has passed, which in most cases where the answer is no it has not. +// references named in unknown finish compiling. projectReferences is what the key holds for each +// project assembly reference now, with the unknown ones read from Unity's build. +// It checks, in this order, and stops at the first that fails: a record of the current format, the +// same project assemblies referenced, every reference outside unknown as recorded, every unknown one +// the record read from Unity's build still as recorded, the outputs still holding what that compile +// wrote, and shape giving the recorded shape. +// Why an unknown reference the record read from Unity's build must still match: it finishes either +// keeping Unity's surface, read from that same file, or changed, read from this run's output; when +// Unity's file moved, neither gives the recorded line back. One the record read from this run's +// output is not compared, because what it finishes as decides that. +// Why shape is a function asked last: it reads every source of the assembly, and the cheaper checks +// before it answer most of the cases where the answer is no. // Why anything unexpected means no: the caller then compiles without waiting, which is what it did // before this check existed. func (s Store[D]) MayStillApply( - assembly string, projectReferences map[string]string, unknown map[string]bool, shape func() (string, bool), + assembly string, projectReferences map[string]ReferenceIdentity, unknown map[string]bool, + shape func() (string, error), ) bool { manifest, found := s.loadManifest(assembly) - if !found || !sameKnownReferences(manifest.ProjectReferences, projectReferences, unknown) { + if !found || !referencesMayStillMatch(manifest.ProjectReferences, projectReferences, unknown) { return false } if !outputsStillMatch(s.directoryPath(), manifest.Outputs) { return false } - current, computed := shape() + current, err := shape() - return computed && current == manifest.Shape + return err == nil && current == manifest.Shape } -// sameKnownReferences reports whether two sets of project reference identities name the same -// assemblies and agree on every one outside unknown. -func sameKnownReferences(recorded map[string]string, current map[string]string, unknown map[string]bool) bool { +// referencesMayStillMatch reports whether two sets of project reference identities name the same +// assemblies, agree on every one outside unknown, and agree on every unknown one the record read from +// Unity's build. +func referencesMayStillMatch( + recorded map[string]ReferenceIdentity, current map[string]ReferenceIdentity, unknown map[string]bool, +) bool { if len(recorded) != len(current) { return false } @@ -108,7 +135,10 @@ func sameKnownReferences(recorded map[string]string, current map[string]string, if !found { return false } - if !unknown[name] && recordedIdentity != identity { + if unknown[name] && recordedIdentity.Kind == ProducedReference { + continue + } + if recordedIdentity != identity { return false } } diff --git a/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go b/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go index 95b169ddd..8e9aed7b8 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/compile_result_test.go @@ -274,7 +274,9 @@ func TestForgetRemovesTheManifestTheBaselineAndTheGeneratorOutput(t *testing.T) // recordIdentity stores one compile of Alpha's outcome under the shape and project reference // identities of the key it ran on. -func recordIdentity(t *testing.T, directoryPath string, shape string, projectReferences map[string]string) { +func recordIdentity( + t *testing.T, directoryPath string, shape string, projectReferences map[string]ReferenceIdentity, +) { t.Helper() identity := KeyIdentity{Key: "key", Shape: shape, ProjectReferences: projectReferences} if err := directoryStore(directoryPath).RecordCompile("Alpha", identity, storedResult(), alphaOutputs()); err != nil { @@ -282,27 +284,59 @@ func recordIdentity(t *testing.T, directoryPath string, shape string, projectRef } } +// unityLine is the identity of a reference read from Unity's build. +func unityLine(hash string) ReferenceIdentity { + return ReferenceIdentity{Kind: UnityReference, Hash: hash} +} + +// producedLine is the identity of a reference read from this run's output. +func producedLine(hash string) ReferenceIdentity { + return ReferenceIdentity{Kind: ProducedReference, Hash: hash} +} + // shapeOf hands MayStillApply a shape, counting how many times it was asked for one. -func shapeOf(shape string, asked *int) func() (string, bool) { - return func() (string, bool) { +func shapeOf(shape string, asked *int) func() (string, error) { + return func() (string, error) { *asked++ - return shape, true + return shape, nil } } -// Verifies a record still may apply when only the identities of the unknown references differ: -// those references are still compiling, so the key they will end up giving is not known yet. +// Verifies a record still may apply when the only difference is an unknown reference it read from +// this run's output: what that reference finishes as decides the key, and it has not finished. func TestMayStillApplyWhenOnlyUnknownReferencesDiffer(t *testing.T) { directoryPath := newRecordedOutputs(t) - recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "produced beta", "Gamma": "gamma"}) + recordIdentity(t, directoryPath, "shape", + map[string]ReferenceIdentity{"Beta": producedLine("produced beta"), "Gamma": unityLine("gamma")}) asked := 0 mayApply := directoryStore(directoryPath).MayStillApply("Alpha", - map[string]string{"Beta": "unity beta", "Gamma": "gamma"}, map[string]bool{"Beta": true}, shapeOf("shape", &asked)) + map[string]ReferenceIdentity{"Beta": unityLine("unity beta"), "Gamma": unityLine("gamma")}, + map[string]bool{"Beta": true}, shapeOf("shape", &asked)) if !mayApply { - t.Error("a record differing only in an unknown reference should still apply") + t.Error("a record differing only in an unknown reference read from this run's output should still apply") + } +} + +// Verifies an unknown reference the record read from Unity's build must still match Unity's file: +// whatever it finishes as, a moved Unity file can never give the recorded line back. The same +// record with Unity's file as recorded still applies. +func TestMayStillApplyRefusesAnUnknownReferenceWhoseUnityFileMoved(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]ReferenceIdentity{"Beta": unityLine("unity beta")}) + asked := 0 + store := directoryStore(directoryPath) + unknown := map[string]bool{"Beta": true} + + if !store.MayStillApply("Alpha", + map[string]ReferenceIdentity{"Beta": unityLine("unity beta")}, unknown, shapeOf("shape", &asked)) { + t.Fatal("a record whose unknown reference is still Unity's file as recorded should still apply") + } + if store.MayStillApply("Alpha", + map[string]ReferenceIdentity{"Beta": unityLine("rebuilt unity beta")}, unknown, shapeOf("shape", &asked)) { + t.Error("a record whose unknown reference read a Unity file that has moved should not apply") } } @@ -310,11 +344,13 @@ func TestMayStillApplyWhenOnlyUnknownReferencesDiffer(t *testing.T) { // and that the shape, which reads every source, is not asked for then. func TestMayStillApplyRefusesAChangedKnownReference(t *testing.T) { directoryPath := newRecordedOutputs(t) - recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta", "Gamma": "gamma"}) + recordIdentity(t, directoryPath, "shape", + map[string]ReferenceIdentity{"Beta": producedLine("beta"), "Gamma": unityLine("gamma")}) asked := 0 mayApply := directoryStore(directoryPath).MayStillApply("Alpha", - map[string]string{"Beta": "beta", "Gamma": "another gamma"}, map[string]bool{"Beta": true}, shapeOf("shape", &asked)) + map[string]ReferenceIdentity{"Beta": producedLine("beta"), "Gamma": unityLine("another gamma")}, + map[string]bool{"Beta": true}, shapeOf("shape", &asked)) if mayApply { t.Error("a record whose known reference changed should not apply") @@ -324,31 +360,66 @@ func TestMayStillApplyRefusesAChangedKnownReference(t *testing.T) { } } -// Verifies a record no longer applies when the inputs other than the project references differ. -func TestMayStillApplyRefusesAnotherShape(t *testing.T) { +// Verifies a known reference must match in kind as well as in hash: a line read from Unity's build +// never matches one read from this run's output. +func TestMayStillApplyRefusesAKnownReferenceOfAnotherKind(t *testing.T) { directoryPath := newRecordedOutputs(t) - recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta"}) + recordIdentity(t, directoryPath, "shape", map[string]ReferenceIdentity{"Beta": producedLine("beta")}) asked := 0 - mayApply := directoryStore(directoryPath).MayStillApply("Alpha", - map[string]string{"Beta": "beta"}, map[string]bool{"Beta": true}, shapeOf("another shape", &asked)) + if directoryStore(directoryPath).MayStillApply("Alpha", + map[string]ReferenceIdentity{"Beta": unityLine("beta")}, map[string]bool{}, shapeOf("shape", &asked)) { + t.Error("a known reference read from another file should not apply") + } +} - if mayApply { +// Verifies the shape is asked for exactly once when everything before it matches, and decides the +// answer then. +func TestMayStillApplyAsksForTheShapeOnceEverythingElseMatches(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]ReferenceIdentity{"Beta": producedLine("beta")}) + store := directoryStore(directoryPath) + current := map[string]ReferenceIdentity{"Beta": producedLine("beta")} + + sameAsked := 0 + if !store.MayStillApply("Alpha", current, map[string]bool{}, shapeOf("shape", &sameAsked)) { + t.Error("a record matching in everything should still apply") + } + otherAsked := 0 + if store.MayStillApply("Alpha", current, map[string]bool{}, shapeOf("another shape", &otherAsked)) { t.Error("a record of another shape should not apply") } + if sameAsked != 1 || otherAsked != 1 { + t.Errorf("the shape was asked for %d and %d times, want once each", sameAsked, otherAsked) + } +} + +// Verifies a shape that cannot be computed means the record does not apply. +func TestMayStillApplyRefusesAShapeThatCannotBeComputed(t *testing.T) { + directoryPath := newRecordedOutputs(t) + recordIdentity(t, directoryPath, "shape", map[string]ReferenceIdentity{"Beta": producedLine("beta")}) + + if directoryStore(directoryPath).MayStillApply("Alpha", + map[string]ReferenceIdentity{"Beta": producedLine("beta")}, map[string]bool{}, + func() (string, error) { return "shape", os.ErrNotExist }) { + t.Error("a record whose shape cannot be computed should not apply") + } } // Verifies a record no longer applies when the unit references another set of project assemblies, // whether one was added or one was dropped, even when the difference is an unknown reference. func TestMayStillApplyRefusesAnotherSetOfReferences(t *testing.T) { - cases := map[string]map[string]string{ - "a reference the record lacks": {"Beta": "beta", "Gamma": "gamma", "Delta": "delta"}, - "a reference no longer there": {"Beta": "beta"}, + cases := map[string]map[string]ReferenceIdentity{ + "a reference the record lacks": { + "Beta": producedLine("beta"), "Gamma": producedLine("gamma"), "Delta": producedLine("delta"), + }, + "a reference no longer there": {"Beta": producedLine("beta")}, } for name, current := range cases { t.Run(name, func(t *testing.T) { directoryPath := newRecordedOutputs(t) - recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta", "Gamma": "gamma"}) + recordIdentity(t, directoryPath, "shape", + map[string]ReferenceIdentity{"Beta": producedLine("beta"), "Gamma": producedLine("gamma")}) asked := 0 unknown := map[string]bool{"Gamma": true, "Delta": true} @@ -362,25 +433,28 @@ func TestMayStillApplyRefusesAnotherSetOfReferences(t *testing.T) { // Verifies a record no longer applies once an output it hashed holds other bytes. func TestMayStillApplyRefusesChangedOutputs(t *testing.T) { directoryPath := newRecordedOutputs(t) - recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta"}) + recordIdentity(t, directoryPath, "shape", map[string]ReferenceIdentity{"Beta": producedLine("beta")}) writeOutputFile(t, directoryPath, "Alpha"+referenceAssemblyExtension, "another surface") asked := 0 if directoryStore(directoryPath).MayStillApply("Alpha", - map[string]string{"Beta": "beta"}, map[string]bool{}, shapeOf("shape", &asked)) { + map[string]ReferenceIdentity{"Beta": producedLine("beta")}, map[string]bool{}, shapeOf("shape", &asked)) { t.Error("a record whose outputs changed should not apply") } + if asked != 0 { + t.Errorf("the shape was asked for %d times, want none once an output differs", asked) + } } // Verifies a record of format version 2, which kept no shape, neither may apply nor is reused. func TestMayStillApplyRefusesAnotherFormatVersion(t *testing.T) { directoryPath := newRecordedOutputs(t) - recordIdentity(t, directoryPath, "shape", map[string]string{"Beta": "beta"}) + recordIdentity(t, directoryPath, "shape", map[string]ReferenceIdentity{"Beta": producedLine("beta")}) setManifestFormatVersion(t, directoryPath, "Alpha", 2) asked := 0 if directoryStore(directoryPath).MayStillApply("Alpha", - map[string]string{"Beta": "beta"}, map[string]bool{}, shapeOf("shape", &asked)) { + map[string]ReferenceIdentity{"Beta": producedLine("beta")}, map[string]bool{}, shapeOf("shape", &asked)) { t.Error("a record of format version 2 should not apply") } if _, reusable := directoryStore(directoryPath).Reusable("Alpha", "key"); reusable { diff --git a/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go b/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go index 84efd6a86..892075a00 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/record_format_test.go @@ -39,12 +39,12 @@ func TestWriteUnitResultManifestWritesTheRecordedFormat(t *testing.T) { } if err := directoryStore(directoryPath).RecordCompile("Alpha", KeyIdentity{ - Key: "key", Shape: "shape", ProjectReferences: map[string]string{"Beta": "beta"}, + Key: "key", Shape: "shape", ProjectReferences: map[string]ReferenceIdentity{"Beta": {Kind: ProducedReference, Hash: "beta"}}, }, result, alphaOutputs()); err != nil { t.Fatalf("failed to write the manifest: %v", err) } - assertFileContent(t, manifestPath(directoryPath, "Alpha"), `{"FormatVersion":3,"InputKey":"key","Shape":"shape","ProjectReferences":{"Beta":"beta"},"Succeeded":false,`+ + assertFileContent(t, manifestPath(directoryPath, "Alpha"), `{"FormatVersion":3,"InputKey":"key","Shape":"shape","ProjectReferences":{"Beta":{"Kind":"produced","Hash":"beta"}},"Succeeded":false,`+ `"Diagnostics":[{"Severity":"error","Code":"CS0103","Message":"The name 'Missing' does not exist in the current context",`+ `"File":"Assets/Alpha/Alpha.cs","Line":3,"Column":9}],`+ `"Outputs":{"Alpha.dll":"e020bdf568793dee747a220086c5e53c8b3af5abd959383a31297f303577efe6",`+