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..f01c39a88 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 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]resultstore.ReferenceIdentity +} + +// 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]resultstore.ReferenceIdentity, error) { + projectReferences := map[string]resultstore.ReferenceIdentity{} + 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]resultstore.ReferenceIdentity, error) { + projectReferences := map[string]resultstore.ReferenceIdentity{} + 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,64 @@ 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 { + 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, 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]resultstore.ReferenceIdentity, ) 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 { + 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 + produced, err := writeReferenceLine(&line, projectRoot, plan, unit, reference) + if err != nil { return err } + sum := sha256.Sum256(line.Bytes()) + 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, 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, +) (bool, error) { + resolved := rewriteReference(projectRoot, plan, unit, reference) + path := projectPath(projectRoot, resolved) + if isUnderDirectory(resolved, plan.OutputDir) { + return true, writeFileContentEntry(digest, "produced-reference", resolved, path) + } + + return false, 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 +478,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 +490,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..d9f874b49 100644 --- a/cli/dispatcher/internal/compilecheck/input_key_test.go +++ b/cli/dispatcher/internal/compilecheck/input_key_test.go @@ -6,8 +6,11 @@ import ( "os" "path/filepath" "reflect" + "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 @@ -471,3 +474,139 @@ 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, each with the kind of file its line reads. +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) + } + 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 4592285d0..7453b858e 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,33 @@ 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 + } + + // 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, error) { + keyResult, keyErr := computeInputKey(c.ProjectRoot, c.Paths, plan, unit) + + return keyResult.Shape, keyErr + }) +} + // 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..db3351f39 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/result_cache_applies_test.go @@ -0,0 +1,133 @@ +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 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") + } + + 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", "X") { + t.Error("the record should not apply once Unity rebuilt the reference assembly it read for X") + } +} + +// 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..8d39cfa3a 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,42 @@ 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]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 // 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]ReferenceIdentity + 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 +76,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 +90,77 @@ 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. 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]ReferenceIdentity, unknown map[string]bool, + shape func() (string, error), +) bool { + manifest, found := s.loadManifest(assembly) + if !found || !referencesMayStillMatch(manifest.ProjectReferences, projectReferences, unknown) { + return false + } + if !outputsStillMatch(s.directoryPath(), manifest.Outputs) { + return false + } + current, err := shape() + + return err == nil && current == manifest.Shape +} + +// 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 + } + for name, identity := range current { + recordedIdentity, found := recorded[name] + if !found { + return false + } + if unknown[name] && recordedIdentity.Kind == ProducedReference { + continue + } + if 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 +174,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 +191,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..8e9aed7b8 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,193 @@ 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]ReferenceIdentity, +) { + 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) + } +} + +// 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, error) { + return func() (string, error) { + *asked++ + + return shape, nil + } +} + +// 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]ReferenceIdentity{"Beta": producedLine("produced beta"), "Gamma": unityLine("gamma")}) + asked := 0 + + mayApply := directoryStore(directoryPath).MayStillApply("Alpha", + 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 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") + } +} + +// 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]ReferenceIdentity{"Beta": producedLine("beta"), "Gamma": unityLine("gamma")}) + asked := 0 + + mayApply := directoryStore(directoryPath).MayStillApply("Alpha", + 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") + } + if asked != 0 { + t.Errorf("the shape was asked for %d times, want none once a known reference differs", asked) + } +} + +// 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]ReferenceIdentity{"Beta": producedLine("beta")}) + asked := 0 + + 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") + } +} + +// 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]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]ReferenceIdentity{"Beta": producedLine("beta"), "Gamma": producedLine("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]ReferenceIdentity{"Beta": producedLine("beta")}) + writeOutputFile(t, directoryPath, "Alpha"+referenceAssemblyExtension, "another surface") + asked := 0 + + if directoryStore(directoryPath).MayStillApply("Alpha", + 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]ReferenceIdentity{"Beta": producedLine("beta")}) + setManifestFormatVersion(t, directoryPath, "Alpha", 2) + asked := 0 + + if directoryStore(directoryPath).MayStillApply("Alpha", + 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 { + 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..892075a00 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]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":2,"InputKey":"key","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",`+ 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: