diff --git a/cli/dispatcher/internal/compilecheck/compiler.go b/cli/dispatcher/internal/compilecheck/compiler.go index 748ed6902..28182bba0 100644 --- a/cli/dispatcher/internal/compilecheck/compiler.go +++ b/cli/dispatcher/internal/compilecheck/compiler.go @@ -93,6 +93,9 @@ type UnitResult struct { // WaitedForRecord marks a unit that waited for the references still compiling rather than // starting before them, because its recorded result might be replayed once they finished. WaitedForRecord bool + // ChangedReferences names the referenced assemblies whose changed surface counted for this unit, + // in the plan's order, each with the first changed name the unit writes. + ChangedReferences []ChangedReference } // CommandRunner runs one compiler invocation, so tests can stand in for the real process. @@ -109,14 +112,14 @@ type Compiler struct { // compareSurface stands in for the comparison of a unit's reference assembly with Unity's; nil // compares the two. Why it can be replaced: the tests have to see which units the run compares at // all, and a byte comparison leaves nothing behind to count. - compareSurface func(projectRoot string, plan BuildPlan, unit CompileUnit) bool + compareSurface func(projectRoot string, plan BuildPlan, unit CompileUnit) SurfaceVerdict } // surfaceVerdict says whether compiling this unit left its reference assembly showing its dependents // the public surface the last Unity build showed them. func (c Compiler) surfaceVerdict(ctx context.Context, plan BuildPlan, unit CompileUnit) SurfaceVerdict { if c.compareSurface != nil { - return SurfaceVerdict{Kept: c.compareSurface(c.ProjectRoot, plan, unit)} + return c.compareSurface(c.ProjectRoot, plan, unit) } return c.referenceSurfaceVerdict(ctx, plan, unit) diff --git a/cli/dispatcher/internal/compilecheck/name_index.go b/cli/dispatcher/internal/compilecheck/name_index.go new file mode 100644 index 000000000..0595f9ab9 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/name_index.go @@ -0,0 +1,376 @@ +package compilecheck + +import ( + "bytes" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "slices" + "strings" + "time" + "unicode" + "unicode/utf8" +) + +// targetTypedNewToken stands for a target-typed new in a dependent's identifiers, the way the worker +// names a changed constructor. +const targetTypedNewToken = "new(" + +// ChangedReference names a referenced assembly whose changed surface made a unit compile, and the +// first of its changed names the unit writes; empty when the change could not be told by names. +type ChangedReference struct { + Assembly string + Name string +} + +// NameSkip names a unit left out although a reference it reads changed its surface, because the unit +// writes none of the names the change touches, and those references. +type NameSkip struct { + Assembly string + ChangedReferences []string +} + +// NameIndexReport says what reading the identifiers of the units with references in the plan took. +type NameIndexReport struct { + Duration time.Duration + // Units counts the units whose identifiers were read, WithGenerated those whose saved generator + // output was read too, and Unreadable those that could not be read and so compile on any change. + Units int + WithGenerated int + Unreadable int +} + +// identifierSet is the identifiers one unit writes. +type identifierSet map[string]struct{} + +// scanIdentifiers adds every identifier content holds to into, cut the way C# cuts one: a letter, a +// letter number or '_', followed by those, decimal digits, combining marks, connectors and formatting +// characters, the last left out of the name as the compiler leaves them out. Comments, strings and code a #if leaves out are read too, which can only make a +// unit compile more often. A "new" followed by '(' also adds the target-typed new token. +// Why \u escapes are not decoded: almost nobody writes one in an identifier, and the miss is named +// among the limits of the check. +func scanIdentifiers(content []byte, into identifierSet) { + position := 0 + for position < len(content) { + character, size := utf8.DecodeRune(content[position:]) + if !isIdentifierStart(character) { + position += size + + continue + } + start := position + position += size + for position < len(content) { + next, nextSize := utf8.DecodeRune(content[position:]) + if !isIdentifierPart(next) { + break + } + position += nextSize + } + identifier := identifierText(content[start:position]) + into[identifier] = struct{}{} + if identifier == "new" && opensParenthesis(content[position:]) { + into[targetTypedNewToken] = struct{}{} + } + } +} + +func isIdentifierStart(character rune) bool { + return unicode.IsLetter(character) || unicode.Is(unicode.Nl, character) || character == '_' +} + +func isIdentifierPart(character rune) bool { + return isIdentifierStart(character) || unicode.In(character, unicode.Nd, unicode.Mn, unicode.Mc, unicode.Pc, unicode.Cf) +} + +// identifierText is the name an identifier's text spells, without its formatting characters. +func identifierText(text []byte) string { + if !bytes.ContainsFunc(text, isFormatting) { + return string(text) + } + + return string(bytes.Map(func(character rune) rune { + if isFormatting(character) { + return -1 + } + + return character + }, text)) +} + +func isFormatting(character rune) bool { + return unicode.Is(unicode.Cf, character) +} + +// opensParenthesis reports whether rest starts with '(' after any white space. +// Why it walks the bytes in place: copying the rest of the file at every "new" makes a file full of +// them take time in the square of its length. +func opensParenthesis(rest []byte) bool { + trimmed := bytes.TrimLeftFunc(rest, unicode.IsSpace) + + return len(trimmed) > 0 && trimmed[0] == '(' +} + +// readSourceIdentifiers adds the identifiers of every source of a unit to into. +// Why a unit without sources is an error rather than an empty set: an empty set would read as a unit +// that writes no changed name, and skip it on every change. +func readSourceIdentifiers(projectRoot string, unit CompileUnit, into identifierSet) error { + if len(unit.Sources) == 0 { + return fmt.Errorf("%s has no sources to read names from", unit.Assembly.AssemblyName) + } + for _, source := range unit.Sources { + content, err := os.ReadFile(sourcePath(projectRoot, filepath.FromSlash(source))) + if err != nil { + return fmt.Errorf("failed to read the names of %s: %w", source, err) + } + scanIdentifiers(content, into) + } + + return nil +} + +// readGeneratedIdentifiers reads the identifiers of the generator output a unit's last full compile +// saved, and reports whether there was any. +// Why generator output is read: a generator can write a referenced name the unit's own sources never +// do, and that code compiles with the unit. +func readGeneratedIdentifiers(generatedDirectory string) (identifierSet, bool, error) { + identifiers := identifierSet{} + if _, err := os.Stat(generatedDirectory); errors.Is(err, fs.ErrNotExist) { + return identifiers, false, nil + } + err := filepath.WalkDir(generatedDirectory, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.IsDir() || !strings.EqualFold(filepath.Ext(path), ".cs") { + return nil + } + content, err := os.ReadFile(path) + if err != nil { + return err + } + scanIdentifiers(content, identifiers) + + return nil + }) + if err != nil { + return nil, false, fmt.Errorf("failed to read the generated names in %s: %w", generatedDirectory, err) + } + + return identifiers, true, nil +} + +// nameIndex reads, in the background, the identifiers of every unit that references another unit of +// the plan, so a reference that changed only names a unit never writes need not make it compile. +// Why every such unit and not only the dependents: a unit selected for its own sources compiles +// anyway, but one that started on the assumption that a reference keeps its surface need not compile +// again when that reference changed only names it never writes. +type nameIndex struct { + sets []identifierSet + errs []error + withGenerated []bool + indexed []bool + done chan struct{} + duration time.Duration + started bool +} + +// start reads the identifiers of the units marked in wanted, at most once per run. generated holds +// the generator output directory of each unit. The generator output is read before start returns and +// the sources in the background. +// Why the generator output is read first: a unit's compile removes and rewrites that directory, and a +// read racing it would lose the names only the generator wrote. Sources are never removed, and few +// units have saved generator output, so only the sources are worth reading while compiles run. +func (index *nameIndex) start(projectRoot string, units []CompileUnit, generated []string, wanted []bool) { + if index.started { + return + } + index.started = true + index.done = make(chan struct{}) + index.indexed = wanted + startedAt := time.Now() + type unitNames struct { + set identifierSet + withGenerated bool + err error + } + generatedNames := computeInParallel(len(units), func(position int) unitNames { + if !wanted[position] { + return unitNames{} + } + set, withGenerated, err := readGeneratedIdentifiers(generated[position]) + + return unitNames{set: set, withGenerated: withGenerated, err: err} + }) + go func() { + defer close(index.done) + results := computeInParallel(len(units), func(position int) unitNames { + names := generatedNames[position] + if !wanted[position] || names.err != nil { + return names + } + if err := readSourceIdentifiers(projectRoot, units[position], names.set); err != nil { + return unitNames{err: err} + } + + return names + }) + index.sets = make([]identifierSet, len(units)) + index.errs = make([]error, len(units)) + index.withGenerated = make([]bool, len(units)) + for position, result := range results { + index.sets[position] = result.set + index.errs[position] = result.err + index.withGenerated[position] = result.withGenerated + } + index.duration = time.Since(startedAt) + }() +} + +// lookup waits for the index and returns the identifiers of one unit, or an error when it has none. +func (index *nameIndex) lookup(position int) (identifierSet, error) { + if !index.started { + return nil, errors.New("the names of the plan were never read") + } + <-index.done + if !index.indexed[position] { + return nil, errors.New("the names of this unit were not read") + } + if index.errs[position] != nil { + return nil, index.errs[position] + } + + return index.sets[position], nil +} + +// wait returns once the background read is over, so nothing outlives the run that started it. +func (index *nameIndex) wait() { + if index.started { + <-index.done + } +} + +// report says what the read took; zero when nothing was read. +func (index *nameIndex) report() NameIndexReport { + if !index.started { + return NameIndexReport{} + } + <-index.done + report := NameIndexReport{Duration: index.duration} + for position, indexed := range index.indexed { + if !indexed { + continue + } + report.Units++ + if index.errs[position] != nil { + report.Unreadable++ + } else if index.withGenerated[position] { + report.WithGenerated++ + } + } + + return report +} + +// startNameIndex starts reading the identifiers of the units that reference another unit of the plan, +// when there is any. +func (run *schedulerRun) startNameIndex() { + wanted := make([]bool, len(run.plan.Units)) + any := false + for position := range run.plan.Units { + wanted[position] = len(run.state.references[position]) > 0 + any = any || wanted[position] + } + if !any { + return + } + records := run.compiler.records(run.plan) + generated := make([]string, len(run.plan.Units)) + for position, unit := range run.plan.Units { + generated[position] = filepath.Join(run.compiler.ProjectRoot, records.GeneratedDirectory(unit.Assembly.AssemblyName)) + } + run.names.start(run.compiler.ProjectRoot, run.plan.Units, generated, wanted) +} + +// referenceEffect is what one reference's final state means for one unit that reads it: whether it +// counts as a changed input, and the first of its changed names the unit writes. +type referenceEffect struct { + dependent int + touches bool + // named marks a reference whose change was told by names, so one that does not touch was + // changed rather than kept. + named bool + name string +} + +// changedReference is a reference that touched a unit, by its position in the plan, with the first +// changed name the unit writes; empty when no name told the change. +type changedReference struct { + reference int + name string +} + +// effectsOf decides, for every unit reading the unit at index, whether its verdict touches that unit: +// never when kept; always when the change cannot be told by names, when nothing compared it, or when +// the unit's names could not be read; and otherwise when the unit writes one of the changed names. +func (run *schedulerRun) effectsOf(index int, verdict SurfaceVerdict) []referenceEffect { + dependents := run.state.dependents[index] + effects := make([]referenceEffect, 0, len(dependents)) + for _, dependent := range dependents { + switch { + case verdict.Kept: + effects = append(effects, referenceEffect{dependent: dependent}) + case !verdict.namedChange(): + effects = append(effects, referenceEffect{dependent: dependent, touches: true}) + default: + effects = append(effects, run.namedEffect(dependent, verdict.ChangedNames)) + } + } + + return effects +} + +func (run *schedulerRun) namedEffect(dependent int, changedNames []string) referenceEffect { + identifiers, err := run.names.lookup(dependent) + if err != nil { + return referenceEffect{dependent: dependent, touches: true} + } + for _, name := range changedNames { + if _, found := identifiers[name]; found { + return referenceEffect{dependent: dependent, touches: true, named: true, name: name} + } + } + + return referenceEffect{dependent: dependent, named: true} +} + +// changedReferencesOf names the references that touched the unit at index, in the plan's order. +func (run *schedulerRun) changedReferencesOf(index int) []ChangedReference { + touched := slices.Clone(run.state.changedBy[index]) + if len(touched) == 0 { + return nil + } + slices.SortFunc(touched, func(a changedReference, b changedReference) int { return a.reference - b.reference }) + references := make([]ChangedReference, 0, len(touched)) + for _, reference := range touched { + references = append(references, ChangedReference{ + Assembly: run.plan.Units[reference.reference].Assembly.AssemblyName, Name: reference.name, + }) + } + + return references +} + +// recordNameSkip notes a unit left out although a reference changed, because it writes none of the +// names that reference's change touches. +func (run *schedulerRun) recordNameSkip(index int) { + unaffected := run.state.unaffectedBy[index] + if len(unaffected) == 0 { + return + } + run.nameSkips = append(run.nameSkips, NameSkip{ + Assembly: run.plan.Units[index].Assembly.AssemblyName, ChangedReferences: run.namesOf(slices.Sorted(slices.Values(unaffected))), + }) +} diff --git a/cli/dispatcher/internal/compilecheck/name_index_fifo_test.go b/cli/dispatcher/internal/compilecheck/name_index_fifo_test.go new file mode 100644 index 000000000..293704ccb --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/name_index_fifo_test.go @@ -0,0 +1,70 @@ +//go:build !windows + +package compilecheck + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// Verifies the generator output is read before start returns: with the background read held on a +// source until the generated directory is gone, as a compile that started meanwhile removes it, the +// names only the generator wrote are still in the index. +func TestNameIndexReadsGeneratedCodeBeforeStartReturns(t *testing.T) { + root := t.TempDir() + writeUnitSources(t, root, map[string]string{"Generated/Gen.g.cs": "partial class First { Foo f; }"}) + source := filepath.Join(root, "Assets", "A.cs") + if err := os.MkdirAll(filepath.Dir(source), 0o755); err != nil { + t.Fatalf("failed to create the source directory: %v", err) + } + // Why a FIFO: reading it blocks until the test writes it, which holds the background read at a + // point the test chooses. + if err := syscall.Mkfifo(source, 0o644); err != nil { + t.Fatalf("failed to create the source FIFO: %v", err) + } + unit := CompileUnit{Assembly: ResponseFile{AssemblyName: "Unit"}, Sources: []string{"Assets/A.cs"}} + index := nameIndex{} + + index.start(root, []CompileUnit{unit}, []string{filepath.Join(root, "Generated")}, []bool{true}) + if err := os.RemoveAll(filepath.Join(root, "Generated")); err != nil { + t.Fatalf("failed to remove the generated code: %v", err) + } + releaseFIFO(t, source, "class First { }") + + set, err := index.lookup(0) + if err != nil { + t.Fatalf("expected the identifiers of the unit, got %v", err) + } + assertHasIdentifiers(t, set, []string{"First", "Foo"}, nil) +} + +// releaseFIFO writes content to the FIFO at path once its reader opens it, and fails the test when +// no reader comes. +func releaseFIFO(t *testing.T, path string, content string) { + t.Helper() + written := make(chan error, 1) + go func() { + writer, err := os.OpenFile(path, os.O_WRONLY, 0) + if err != nil { + written <- err + + return + } + _, err = writer.WriteString(content) + if closeErr := writer.Close(); err == nil { + err = closeErr + } + written <- err + }() + select { + case err := <-written: + if err != nil { + t.Fatalf("failed to write the source FIFO: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("nothing read the source FIFO") + } +} diff --git a/cli/dispatcher/internal/compilecheck/name_index_test.go b/cli/dispatcher/internal/compilecheck/name_index_test.go new file mode 100644 index 000000000..bad746696 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/name_index_test.go @@ -0,0 +1,182 @@ +package compilecheck + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func identifiersOf(content string) identifierSet { + set := identifierSet{} + scanIdentifiers([]byte(content), set) + + return set +} + +func assertHasIdentifiers(t *testing.T, set identifierSet, present []string, absent []string) { + t.Helper() + for _, name := range present { + if _, found := set[name]; !found { + t.Errorf("expected %q among the identifiers, got %v", name, set) + } + } + for _, name := range absent { + if _, found := set[name]; found { + t.Errorf("expected %q not to be among the identifiers, got %v", name, set) + } + } +} + +// Verifies identifiers are cut by the characters C# allows in them: a spacing mark, a combining mark +// and a connector stay inside one identifier, a letter number starts one, and a formatting character +// is left out of the name, as the compiler leaves it out. +func TestScanIdentifiersCutsIdentifiersByTheCharactersCSharpAllows(t *testing.T) { + set := identifiersOf("नाम cafe\u0301 a\u203fb \u216bth Fo\u200do") + + assertHasIdentifiers(t, set, + []string{"नाम", "cafe\u0301", "a\u203fb", "\u216bth", "Foo"}, + []string{"न", "म", "cafe", "a", "b", "th", "Fo", "o"}) +} + +// Verifies a target-typed new is found with any white space before its parenthesis, and only then. +func TestScanIdentifiersFindsATargetTypedNewOnlyBeforeAParenthesis(t *testing.T) { + cases := map[string]bool{ + "new(1)": true, "new (1)": true, "new\n(1)": true, "new x(1)": false, "x = new": false, "new ": false, + } + for content, want := range cases { + if _, found := identifiersOf(content)[targetTypedNewToken]; found != want { + t.Errorf("%q: target-typed new found = %v, want %v", content, found, want) + } + } +} + +// BenchmarkScanIdentifiers reads a file holding many "new" expressions. +func BenchmarkScanIdentifiers(b *testing.B) { + content := []byte(strings.Repeat("var x = new Thing(); ", 40_000)) + for b.Loop() { + scanIdentifiers(content, identifierSet{}) + } +} + +// Verifies identifiers are cut the way C# spells them: a verbatim '@' left off, Unicode letters +// kept, words in strings and comments read too, a target-typed new found with or without a space, +// and an identifier that only starts with "new" not taken for one. +func TestScanIdentifiersCutsIdentifiersTheWayCSharpSpellsThem(t *testing.T) { + set := identifiersOf("var @class = new (1); Point p = new(2); newFoo(3); // Commented\n" + + "string s = \"Quoted\"; int 名前 = _under9; char c = '\\u0041';") + + assertHasIdentifiers(t, set, + []string{"class", "new(", "newFoo", "Commented", "Quoted", "名前", "_under9", "Point", "u0041"}, + []string{"@class", "A", "9"}) +} + +// Verifies "newFoo(" alone is not read as a target-typed new. +func TestScanIdentifiersDoesNotTakeAnIdentifierStartingWithNewForANew(t *testing.T) { + assertHasIdentifiers(t, identifiersOf("newFoo(1); renew (2);"), []string{"newFoo", "renew"}, []string{"new("}) +} + +// writeUnitSources writes the given project-relative files and returns a unit compiling them. +func writeUnitSources(t *testing.T, root string, files map[string]string) CompileUnit { + t.Helper() + unit := CompileUnit{Assembly: ResponseFile{AssemblyName: "Unit"}} + for path, content := range files { + absolute := filepath.Join(root, filepath.FromSlash(path)) + if err := os.MkdirAll(filepath.Dir(absolute), 0o755); err != nil { + t.Fatalf("failed to create %s: %v", path, err) + } + if err := os.WriteFile(absolute, []byte(content), 0o600); err != nil { + t.Fatalf("failed to write %s: %v", path, err) + } + unit.Sources = append(unit.Sources, path) + } + + return unit +} + +// Verifies a unit's identifiers come from every source, and its generator output's from every C# file +// under the directory, nested directories included, with reading the latter reported. +func TestReadIdentifiersReadsSourcesAndGeneratedCode(t *testing.T) { + root := t.TempDir() + unit := writeUnitSources(t, root, map[string]string{"Assets/A.cs": "class First { }", "Assets/B.cs": "class Second { }"}) + writeUnitSources(t, root, map[string]string{"Generated/sub/a.CS": "class Third { }", "Generated/notes.txt": "Fourth"}) + + set, withGenerated, err := readGeneratedIdentifiers(filepath.Join(root, "Generated")) + if err != nil { + t.Fatalf("expected the generated identifiers to be read, got %v", err) + } + if err := readSourceIdentifiers(root, unit, set); err != nil { + t.Fatalf("expected the source identifiers to be read, got %v", err) + } + + assertHasIdentifiers(t, set, []string{"First", "Second", "Third"}, []string{"Fourth"}) + if !withGenerated { + t.Error("expected the generated code to be reported as read") + } +} + +// Verifies a missing generator output directory reads as no generated code rather than an error. +func TestReadGeneratedIdentifiersWithoutGeneratedCode(t *testing.T) { + set, withGenerated, err := readGeneratedIdentifiers(filepath.Join(t.TempDir(), "Missing")) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if withGenerated || len(set) != 0 { + t.Errorf("expected no generated code, got %v (reported read: %v)", set, withGenerated) + } +} + +// Verifies a source given by absolute path, as a local package's is, is read from that path. +func TestReadSourceIdentifiersReadsAnAbsoluteSource(t *testing.T) { + elsewhere := t.TempDir() + writeUnitSources(t, elsewhere, map[string]string{"Package/P.cs": "class Packaged { }"}) + unit := CompileUnit{ + Assembly: ResponseFile{AssemblyName: "Unit"}, + Sources: []string{filepath.ToSlash(filepath.Join(elsewhere, "Package", "P.cs"))}, + } + set := identifierSet{} + + if err := readSourceIdentifiers(t.TempDir(), unit, set); err != nil { + t.Fatalf("expected the identifiers to be read, got %v", err) + } + + assertHasIdentifiers(t, set, []string{"Packaged"}, nil) +} + +// Verifies a unit without sources, or with one that cannot be read, is refused rather than read as +// an empty set, so it is never read as writing no changed name. +func TestReadSourceIdentifiersRefusesAUnitItCannotRead(t *testing.T) { + root := t.TempDir() + missing := CompileUnit{Assembly: ResponseFile{AssemblyName: "Unit"}, Sources: []string{"Assets/Missing.cs"}} + for label, unit := range map[string]CompileUnit{"no sources": {Assembly: ResponseFile{AssemblyName: "Unit"}}, "missing": missing} { + if err := readSourceIdentifiers(root, unit, identifierSet{}); err == nil { + t.Errorf("%s: expected an error", label) + } + } +} + +// Verifies the background read is over once wait returns, and that lookup answers only for the +// units it was asked to read. +func TestNameIndexIsOverOnceWaitReturns(t *testing.T) { + root := t.TempDir() + unit := writeUnitSources(t, root, map[string]string{"Assets/A.cs": "class First { }"}) + index := nameIndex{} + index.start(root, []CompileUnit{unit, unit}, []string{filepath.Join(root, "Missing"), ""}, []bool{true, false}) + + index.wait() + + select { + case <-index.done: + default: + t.Fatal("expected the read to be over once wait returned") + } + if _, err := index.lookup(0); err != nil { + t.Errorf("expected the identifiers of the read unit, got %v", err) + } + if _, err := index.lookup(1); err == nil { + t.Error("expected no identifiers for a unit that was not read") + } + if _, err := (&nameIndex{}).lookup(0); err == nil { + t.Error("expected no identifiers from an index that never started") + } +} diff --git a/cli/dispatcher/internal/compilecheck/scheduler.go b/cli/dispatcher/internal/compilecheck/scheduler.go index 77c69dce5..e6fe23301 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler.go +++ b/cli/dispatcher/internal/compilecheck/scheduler.go @@ -47,6 +47,10 @@ type schedule struct { blocked []bool // keptSurface marks, per unit, that it finished with the public surface of the last Unity build. keptSurface []bool + // unaffectedBy lists, per unit, the references that changed their surface only in names the unit + // never writes, and changedBy the ones whose change counted for it. + unaffectedBy [][]int + changedBy [][]changedReference unitProgress } @@ -60,6 +64,10 @@ type compileOutcome struct { // Blocked names the units left out because an assembly they reference has errors, in the plan's // order. They are kept apart from Skipped: nothing they compile from was read at all. Blocked []string + // NameSkips names the skipped units a reference did change under, in names they never write. + NameSkips []NameSkip + // NameIndex says what reading the units' names took. + NameIndex NameIndexReport } // DefaultJobs is how many assemblies compile at once when the caller names no number. @@ -86,6 +94,8 @@ type schedulerRun struct { completions chan unitCompletion running int referenceSkips int + names nameIndex + nameSkips []NameSkip } // compileUnits compiles a plan's assemblies, keeping at most jobs of them running at a time. It @@ -121,6 +131,10 @@ func compileUnits( compiled: make([]bool, len(plan.Units)), completions: make(chan unitCompletion, len(plan.Units)), } + run.startNameIndex() + // Why waited for on every way out: the read runs on files a caller may remove once the run + // returns. + defer run.names.wait() for index := range plan.Units { run.offer(index) } @@ -151,9 +165,11 @@ func compileUnits( } return compileOutcome{ - Units: run.compiledResults(), - Skipped: run.referenceSkips, - Blocked: run.blockedNames(), + Units: run.compiledResults(), + Skipped: run.referenceSkips, + Blocked: run.blockedNames(), + NameSkips: run.nameSkips, + NameIndex: run.names.report(), }, nil } @@ -217,14 +233,15 @@ func (run *schedulerRun) skipBlockedUnit(index int) (bool, error) { return false, err } run.state.blockDependents(index) - run.releaseAndOffer(index, false) + run.releaseAndOffer(index, SurfaceVerdict{Kept: true}) return true, nil } // skipUnchangedDependent leaves out a unit this run picked up only because something it references // was recompiled, when every one of those came out with the public surface the last Unity build -// recorded: compiling it would read exactly the API it was last compiled against. It counts as +// recorded, or changed only names the unit never writes: compiling it would read exactly the API it +// was last compiled against, as far as anything it writes can tell. It counts as // having kept Unity's surface, so the units that started before it finished read Unity's reference // assembly for it, as they assumed. // Why its earlier outputs are removed rather than left alone: a reference assembly an earlier run @@ -242,8 +259,9 @@ func (run *schedulerRun) skipUnchangedDependent(index int) (bool, error) { return false, err } run.referenceSkips++ + run.recordNameSkip(index) run.state.keptSurface[index] = true - run.releaseAndOffer(index, false) + run.releaseAndOffer(index, SurfaceVerdict{Kept: true}) return true, nil } @@ -362,6 +380,8 @@ func newSchedule(plan BuildPlan) *schedule { changedInputs: make([]int, len(plan.Units)), blocked: make([]bool, len(plan.Units)), keptSurface: make([]bool, len(plan.Units)), + unaffectedBy: make([][]int, len(plan.Units)), + changedBy: make([][]changedReference, len(plan.Units)), unitProgress: newUnitProgress(len(plan.Units)), } for index, unit := range plan.Units { @@ -395,22 +415,28 @@ func (state *schedule) blockDependents(index int) { } } -// release tells the units waiting on one that just got its final state that it has, recording -// whether what it produced still describes the same public surface as the last Unity build, and -// reports those units so the caller can decide which of them to queue. -// Why a unit that assumed this one is marked stale only when it changed: it read Unity's reference -// assembly for it, which is exactly right when the surface was kept. A failed or blocked one blocks -// it through blockDependents instead. -func (state *schedule) release(index int, changed bool) []int { - for _, dependent := range state.dependents[index] { +// release tells the units waiting on one that just got its final state that it has, recording for +// each whether what it produced touches it, and reports those units so the caller can decide which +// of them to queue. effects holds one entry per unit waiting on it, in the order of dependents. +// Why a unit that assumed this one is marked stale only when touched: it read Unity's reference +// assembly for it, which is exactly right when the surface was kept, and compiles the same when the +// change touches no name it writes. A failed or blocked one blocks it through blockDependents instead. +func (state *schedule) release(index int, effects []referenceEffect) []int { + for _, effect := range effects { + dependent := effect.dependent state.pending[dependent]-- if state.ownSources[index] { state.pendingOwn[dependent]-- } - if !changed { + if !effect.touches { + if effect.named { + state.unaffectedBy[dependent] = append(state.unaffectedBy[dependent], index) + } + continue } state.changedInputs[dependent]++ + state.changedBy[dependent] = append(state.changedBy[dependent], changedReference{reference: index, name: effect.name}) if !state.stale[dependent] && containsIndex(state.assumed[dependent], index) { state.stale[dependent] = true state.staleCause[dependent] = index diff --git a/cli/dispatcher/internal/compilecheck/scheduler_names_test.go b/cli/dispatcher/internal/compilecheck/scheduler_names_test.go new file mode 100644 index 000000000..c8689c85c --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/scheduler_names_test.go @@ -0,0 +1,281 @@ +package compilecheck + +import ( + "context" + "path/filepath" + "reflect" + "testing" + "time" +) + +// namedVerdicts stands in for the surface comparison: the units it names get the verdict it holds, +// and every other unit is compared by bytes, as a run without the worker does. +type namedVerdicts map[string]SurfaceVerdict + +func (verdicts namedVerdicts) compare(projectRoot string, plan BuildPlan, unit CompileUnit) SurfaceVerdict { + if verdict, found := verdicts[unit.Assembly.AssemblyName]; found { + return verdict + } + + return Compiler{ProjectRoot: projectRoot}.referenceSurfaceVerdict(context.Background(), plan, unit) +} + +// namedChangeOf is the verdict of a reference whose change the worker told by the given names. +func namedChangeOf(names ...string) SurfaceVerdict { + return SurfaceVerdict{Reason: SurfaceChanged, ChangedNames: names} +} + +// compileWithVerdicts runs the whole plan with the given surface verdicts and fails the test when the +// run could not finish. +func (project *cachedSchedulerProject) compileWithVerdicts( + t *testing.T, recorder *schedulerRecorder, verdicts namedVerdicts, +) compileOutcome { + t.Helper() + compiler := Compiler{ + Paths: project.paths, + ProjectRoot: project.root, + Timeout: time.Minute, + Run: recorder.runner(), + compareSurface: verdicts.compare, + } + outcome, err := compileUnits(context.Background(), compiler, project.plan, 4) + if err != nil { + t.Fatalf("expected the run to succeed, got error: %v", err) + } + + return outcome +} + +// newNamesProject is A, selected for its own sources, and B, a dependent of A, whose source is given. +func newNamesProject(t *testing.T, sourceOfB string) (*cachedSchedulerProject, *schedulerRecorder) { + t.Helper() + project, recorder := newOptimisticCachedProject(t, map[string][]string{"B": {"A"}}, []string{"B"}, "A", "B") + project.editSource(t, "B", sourceOfB) + + return project, recorder +} + +func assertChangedReferences(t *testing.T, result UnitResult, want []ChangedReference) { + t.Helper() + if !reflect.DeepEqual(result.ChangedReferences, want) { + t.Errorf("%s changed references = %+v, want %+v", result.Assembly, result.ChangedReferences, want) + } +} + +// Verifies a dependent that writes none of the names its reference's change touches is left out the +// way one whose reference kept its surface is, and is reported as left out by names. +func TestCompileUnitsSkipsADependentThatWritesNoChangedName(t *testing.T) { + project, recorder := newNamesProject(t, "class B { int Bar; }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("A", "Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A"}) + if recorder.started["B"] || outcome.Skipped != 1 { + t.Errorf("B started = %v, skipped = %d; want B left out", recorder.started["B"], outcome.Skipped) + } + want := []NameSkip{{Assembly: "B", ChangedReferences: []string{"A"}}} + if !reflect.DeepEqual(outcome.NameSkips, want) { + t.Errorf("name skips = %+v, want %+v", outcome.NameSkips, want) + } + if outcome.NameIndex.Units != 1 || outcome.NameIndex.Unreadable != 0 { + t.Errorf("name index = %+v, want one readable unit", outcome.NameIndex) + } +} + +// Verifies a dependent that writes one of the changed names compiles and says which name made it, +// whether it writes the member's name or only the type's. +func TestCompileUnitsCompilesADependentThatWritesAChangedName(t *testing.T) { + cases := map[string]struct { + source string + names []string + want string + }{ + "member": {source: "class B { int x = Make().Foo; }", names: []string{"A", "Foo"}, want: "Foo"}, + "type": {source: "class B { C c; }", names: []string{"C", "Foo"}, want: "C"}, + } + for label, testCase := range cases { + t.Run(label, func(t *testing.T) { + project, recorder := newNamesProject(t, testCase.source) + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf(testCase.names...)}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + assertChangedReferences(t, resultOf(t, outcome.Units, "B"), []ChangedReference{{Assembly: "A", Name: testCase.want}}) + if len(outcome.NameSkips) != 0 { + t.Errorf("name skips = %+v, want none", outcome.NameSkips) + } + }) + } +} + +// Verifies a dependent whose names cannot be read compiles on a named change, and is counted as +// unreadable. +func TestCompileUnitsCompilesADependentWhoseNamesCannotBeRead(t *testing.T) { + project, recorder := newNamesProject(t, "class B { }") + index := planIndexOf(t, project.plan, "B") + project.plan.Units[index].Sources = append(project.plan.Units[index].Sources, "Assets/B/Missing.cs") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + if outcome.NameIndex.Unreadable != 1 { + t.Errorf("name index = %+v, want one unreadable unit", outcome.NameIndex) + } +} + +// Verifies a change no name tells makes every dependent compile, even one that carries names as +// well, and the change is reported without a name. +func TestCompileUnitsCompilesEveryDependentOnAnUnnamedChange(t *testing.T) { + verdicts := []SurfaceVerdict{ + {Reason: SurfaceChanged, Unnamed: "operator op_Addition"}, + {Reason: SurfaceChanged, Unnamed: "operator op_Addition", ChangedNames: []string{"Foo"}}, + } + for _, verdict := range verdicts { + project, recorder := newNamesProject(t, "class B { }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": verdict}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + assertChangedReferences(t, resultOf(t, outcome.Units, "B"), []ChangedReference{{Assembly: "A"}}) + } +} + +// Verifies a dependent that started on the assumption that a reference keeps its surface keeps that +// compile when the reference changed only names the dependent never writes, whether it was selected +// as a dependent or for its own sources. +func TestCompileUnitsKeepsAnEarlyCompileWhenTheAssumedReferenceChangedNoNameItWrites(t *testing.T) { + for label, dependents := range map[string][]string{"dependent": {"D1", "D2"}, "own sources": {"D1"}} { + t.Run(label, func(t *testing.T) { + project, recorder := newOptimisticCachedProject(t, chainReferences, dependents, "X", "K", "D1", "D2") + recorder.holdUntilBegun(t, "K", "D2") + recorder.surfaces["K"] = keptSurfaceOf("K") + project.editSource(t, "D2", "class D2 { int Bar; }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"D1": namedChangeOf("D1", "Foo")}) + + if recorder.runs["D2"] != 1 { + t.Fatalf("D2 compiled %d times, want 1", recorder.runs["D2"]) + } + d2 := resultOf(t, outcome.Units, "D2") + if d2.CompiledAgainAfter != "" || !reflect.DeepEqual(d2.AssumedReferences, []string{"D1"}) { + t.Errorf("D2 = %+v, want one compile that assumed D1", d2) + } + assertChangedReferences(t, d2, []ChangedReference{{Assembly: "X"}}) + }) + } +} + +// Verifies a dependent that started on the assumption that a reference keeps its surface compiles +// again when the reference changed a name it writes. +func TestCompileUnitsCompilesAgainWhenTheAssumedReferenceChangedANameItWrites(t *testing.T) { + project, recorder := newOptimisticCachedProject(t, chainReferences, []string{"D1", "D2"}, "X", "K", "D1", "D2") + recorder.holdUntilBegun(t, "K", "D2") + recorder.surfaces["K"] = keptSurfaceOf("K") + project.editSource(t, "D2", "class D2 { int x = new D1().Foo; }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"D1": namedChangeOf("D1", "Foo")}) + + if recorder.runs["D2"] != 2 { + t.Fatalf("D2 compiled %d times, want 2", recorder.runs["D2"]) + } + d2 := resultOf(t, outcome.Units, "D2") + if d2.CompiledAgainAfter != "D1" { + t.Errorf("D2 compiled again after %q, want D1", d2.CompiledAgainAfter) + } + assertChangedReferences(t, d2, []ChangedReference{{Assembly: "X"}, {Assembly: "D1", Name: "D1"}}) +} + +// Verifies a unit selected for its own sources still compiles against the reference assembly this +// run produced for a reference that changed only names it never writes: whether a reference touches +// a unit never decides which file the unit reads. +func TestCompileUnitsReadsTheProducedReferenceOfANameChangeItDoesNotWrite(t *testing.T) { + project, recorder := newOptimisticCachedProject(t, map[string][]string{"B": {"A"}}, nil, "A", "B") + project.editSource(t, "B", "class B { }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + assertReferenceFlags(t, project.writtenResponseFile(t, "B"+responseFileExtension), + []string{producedReferenceFlag(project.plan, "A")}, []string{unityReferenceFlag("A")}) + assertChangedReferences(t, resultOf(t, outcome.Units, "B"), nil) +} + +// Verifies a dependent another reference touches compiles, and names only that reference. +func TestCompileUnitsNamesOnlyTheReferencesThatTouchedADependent(t *testing.T) { + project, recorder := newOptimisticCachedProject(t, map[string][]string{"D": {"X", "P"}}, []string{"D"}, "X", "P", "D") + project.editSource(t, "D", "class D { }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{ + "X": namedChangeOf("Foo"), + "P": {Reason: SurfaceChanged, Unnamed: "operator op_Addition"}, + }) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"X", "P", "D"}) + assertChangedReferences(t, resultOf(t, outcome.Units, "D"), []ChangedReference{{Assembly: "P"}}) +} + +// Verifies a dependent left out by names counts as keeping its surface, so the units below it are +// left out too. +func TestCompileUnitsSkipsTheChainBelowADependentLeftOutByNames(t *testing.T) { + project, recorder := newOptimisticCachedProject(t, + map[string][]string{"D": {"X"}, "E": {"D"}}, []string{"D", "E"}, "X", "D", "E") + project.editSource(t, "D", "class D { }") + project.editSource(t, "E", "class E { Foo f; }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"X": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"X"}) + if outcome.Skipped != 2 || recorder.started["E"] { + t.Errorf("skipped = %d, E started = %v; want D and E left out", outcome.Skipped, recorder.started["E"]) + } +} + +// Verifies a changed name only the dependent's saved generator output writes makes it compile. +func TestCompileUnitsCompilesADependentWhoseGeneratedCodeWritesAChangedName(t *testing.T) { + project, recorder := newNamesProject(t, "class B { }") + project.writeFile(t, filepath.Join(project.records().GeneratedDirectory("B"), "sub", "Gen.g.cs"), "partial class B { Foo f; }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + if outcome.NameIndex.WithGenerated != 1 { + t.Errorf("name index = %+v, want one unit read with its generated code", outcome.NameIndex) + } +} + +// planIndexOf finds a unit's position in the plan. +func planIndexOf(t *testing.T, plan BuildPlan, name string) int { + t.Helper() + for index, unit := range plan.Units { + if unit.Assembly.AssemblyName == name { + return index + } + } + t.Fatalf("the plan has no unit %s", name) + + return -1 +} + +// Verifies a unit that started on the assumption that a reference keeps its surface compiles again +// when the reference changed a name only the unit's saved generator output writes, although the unit's +// own compile removes that output as soon as it starts. +func TestCompileUnitsCompilesAgainOnANameOnlyGeneratedCodeWritesWhenTheEarlyCompileRemovesIt(t *testing.T) { + project, recorder := newOptimisticCachedProject(t, + map[string][]string{"D1": {"X"}, "D2": {"D1"}}, []string{"D1"}, "X", "D1", "D2") + project.editSource(t, "D2", "class D2 { int Bar; }") + project.writeFile(t, filepath.Join(project.records().GeneratedDirectory("D2"), "sub", "Gen.g.cs"), "partial class D2 { Foo f; }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{ + "X": {Reason: SurfaceChanged, Unnamed: "operator op_Addition"}, + "D1": namedChangeOf("Foo"), + }) + + if recorder.runs["D2"] != 2 { + t.Fatalf("D2 compiled %d times, want 2", recorder.runs["D2"]) + } + d2 := resultOf(t, outcome.Units, "D2") + if d2.CompiledAgainAfter != "D1" { + t.Errorf("D2 compiled again after %q, want D1", d2.CompiledAgainAfter) + } + assertChangedReferences(t, d2, []ChangedReference{{Assembly: "D1", Name: "Foo"}}) +} diff --git a/cli/dispatcher/internal/compilecheck/scheduler_settle.go b/cli/dispatcher/internal/compilecheck/scheduler_settle.go index 3ede0081d..574a44ee1 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_settle.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_settle.go @@ -130,10 +130,10 @@ func (run *schedulerRun) offer(index int) { state.enqueue(index) } -// releaseAndOffer tells the units waiting on one that just got its final state, and queues those that -// may start now. -func (run *schedulerRun) releaseAndOffer(index int, changed bool) { - for _, dependent := range run.state.release(index, changed) { +// releaseAndOffer tells the units waiting on one that just got its final state what its verdict means +// for each of them, and queues those that may start now. +func (run *schedulerRun) releaseAndOffer(index int, verdict SurfaceVerdict) { + for _, dependent := range run.state.release(index, run.effectsOf(index, verdict)) { run.offer(dependent) } } @@ -198,7 +198,7 @@ func (run *schedulerRun) settle(index int) error { return err } state.blockDependents(index) - run.releaseAndOffer(index, false) + run.releaseAndOffer(index, SurfaceVerdict{Kept: true}) case state.stale[index]: state.discarded[index] = run.results[index].Duration run.results[index] = UnitResult{} @@ -226,6 +226,7 @@ func (run *schedulerRun) adopt(index int) error { result.CompiledAgainAfter = run.plan.Units[cause].Assembly.AssemblyName result.DiscardedDuration = state.discarded[index] } + result.ChangedReferences = run.changedReferencesOf(index) run.results[index] = result if err := run.markFinal(index); err != nil { return err @@ -235,7 +236,10 @@ func (run *schedulerRun) adopt(index int) error { if !result.Succeeded { state.blockDependents(index) } - run.releaseAndOffer(index, !state.keptSurface[index]) + // Why the result's verdict rather than keptSurface alone: a unit compared and found changed + // carries the names its change touches, and a unit not compared - failed, or read by nothing - + // carries the zero verdict, which touches every unit waiting on it. + run.releaseAndOffer(index, result.Surface) return nil } diff --git a/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go b/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go index da98a9766..35ada1207 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go @@ -264,12 +264,12 @@ type surfaceComparisonCounter struct { } // compare records one comparison of the unit's surface. -func (counter *surfaceComparisonCounter) compare(_ string, _ BuildPlan, unit CompileUnit) bool { +func (counter *surfaceComparisonCounter) compare(_ string, _ BuildPlan, unit CompileUnit) SurfaceVerdict { counter.mutex.Lock() defer counter.mutex.Unlock() counter.compared[unit.Assembly.AssemblyName]++ - return false + return SurfaceVerdict{Reason: SurfaceBytesDiffer, Unnamed: SurfaceBytesDiffer} } // Verifies a run compares the surface only of the units whose answer something in the plan reads: diff --git a/cli/dispatcher/internal/compilecheck/session.go b/cli/dispatcher/internal/compilecheck/session.go index 829d882d0..b01acbda2 100644 --- a/cli/dispatcher/internal/compilecheck/session.go +++ b/cli/dispatcher/internal/compilecheck/session.go @@ -46,6 +46,10 @@ type Result struct { // They are kept apart from Skipped: a skipped assembly was read and found to have nothing left // to say, and nothing about these was read at all. Blocked []string + // NameSkips names the skipped assemblies a reference changed under, in names they never write. + NameSkips []NameSkip + // NameIndex says what reading the names of the assemblies with references in the plan took. + NameIndex NameIndexReport } // Run compiles the assemblies that need checking and collects their diagnostics. @@ -102,8 +106,10 @@ func Run(ctx context.Context, options Options) (Result, error) { // Why the two counts are added: both name an assembly the run did not compile - one because // nothing it compiles from changed, the other because everything it references kept the // public surface the last Unity build recorded. - Skipped: plan.Skipped + outcome.Skipped, - Blocked: outcome.Blocked, + Skipped: plan.Skipped + outcome.Skipped, + Blocked: outcome.Blocked, + NameSkips: outcome.NameSkips, + NameIndex: outcome.NameIndex, }, nil } diff --git a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go index cf7923ec5..74331aa1c 100644 --- a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go +++ b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go @@ -89,6 +89,7 @@ func (logRun compileCheckVibeLog) completed(response compileCheckResponse, resul context["baseline_notes"] = compileCheckVibeReasons(response.FastPathBaselineNotes) context["units"] = compileCheckVibeUnits(result.Units) context["surface_checks"] = compileCheckVibeSurfaceChecks(result.Units) + addCompileCheckVibeNameNarrowing(context, result) logRun.write(level, compileCheckVibeCompleteOperation, "compile-check finished.", context) } @@ -178,6 +179,33 @@ func addCompileCheckVibeScheduling(entry map[string]any, unit compilecheck.UnitR if unit.WaitedForRecord { entry["waited_for_record"] = true } + if len(unit.ChangedReferences) > 0 { + changedBy := make([]map[string]any, 0, len(unit.ChangedReferences)) + for _, reference := range unit.ChangedReferences { + changedBy = append(changedBy, map[string]any{"reference": reference.Assembly, "name": reference.Name}) + } + entry["changed_by"] = changedBy + } +} + +// addCompileCheckVibeNameNarrowing adds the units left out because they write none of the names a +// reference's change touches, and what reading the names took, each only when there is any. +func addCompileCheckVibeNameNarrowing(context map[string]any, result compilecheck.Result) { + if len(result.NameSkips) > 0 { + skips := make([]map[string]any, 0, len(result.NameSkips)) + for _, skip := range result.NameSkips { + skips = append(skips, map[string]any{"assembly": skip.Assembly, "unaffected_by": skip.ChangedReferences}) + } + context["name_skips"] = skips + } + if result.NameIndex.Units > 0 { + context["name_index"] = map[string]any{ + "duration_ms": result.NameIndex.Duration.Milliseconds(), + "units": result.NameIndex.Units, + "with_generated": result.NameIndex.WithGenerated, + "unreadable": result.NameIndex.Unreadable, + } + } } // compileCheckVibeSurfaceChecks lists every unit whose surface the worker, or its record of the same diff --git a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go index 71a41a9e1..1b12cffef 100644 --- a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go +++ b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go @@ -247,3 +247,51 @@ func TestCompileCheckVibeLogRecordsUnitsCompiledAgain(t *testing.T) { `{"assembly":"Epsilon","compiled_again_after":"Gamma","discarded_ms":50,"duration_ms":300,`+ `"succeeded":true,"waited_for_record":true}]`) } + +// Verifies a run that narrowed dependents by names logs which references touched each unit and by +// which name, which units it left out by names, and what reading the names took. +func TestCompileCheckVibeLogRecordsNameNarrowing(t *testing.T) { + t.Setenv(vibelog.CLIVibeLogEnvName, "1") + projectRoot := t.TempDir() + result := compilecheck.Result{ + Units: []compilecheck.UnitResult{ + {Assembly: "Alpha", Succeeded: true, Duration: 100 * time.Millisecond}, + { + Assembly: "Beta", Succeeded: true, Duration: 200 * time.Millisecond, + ChangedReferences: []compilecheck.ChangedReference{{Assembly: "Alpha", Name: "Foo"}, {Assembly: "Gamma"}}, + }, + }, + Skipped: 1, + NameSkips: []compilecheck.NameSkip{{Assembly: "Delta", ChangedReferences: []string{"Alpha"}}}, + NameIndex: compilecheck.NameIndexReport{Duration: 30 * time.Millisecond, Units: 3, WithGenerated: 1, Unreadable: 1}, + } + + logRun := startCompileCheckVibeLog(projectRoot, compileCheckOptions{}) + logRun.completed(buildCompileCheckResponse(result, projectRoot), result) + + complete := assertCompileCheckVibeRun(t, readCompileCheckVibeLog(t, projectRoot)) + assertVibeContextJSON(t, complete, "units", + `[{"assembly":"Alpha","duration_ms":100,"succeeded":true},`+ + `{"assembly":"Beta","changed_by":[{"name":"Foo","reference":"Alpha"},{"name":"","reference":"Gamma"}],`+ + `"duration_ms":200,"succeeded":true}]`) + assertVibeContextJSON(t, complete, "name_skips", `[{"assembly":"Delta","unaffected_by":["Alpha"]}]`) + assertVibeContextJSON(t, complete, "name_index", + `{"duration_ms":30,"units":3,"unreadable":1,"with_generated":1}`) +} + +// Verifies a run that read no names and left nothing out by names logs neither. +func TestCompileCheckVibeLogLeavesOutNameNarrowingWhenNothingWasRead(t *testing.T) { + t.Setenv(vibelog.CLIVibeLogEnvName, "1") + projectRoot := t.TempDir() + result := compilecheck.Result{Units: []compilecheck.UnitResult{{Assembly: "Alpha", Succeeded: true}}} + + logRun := startCompileCheckVibeLog(projectRoot, compileCheckOptions{}) + logRun.completed(buildCompileCheckResponse(result, projectRoot), result) + + complete := assertCompileCheckVibeRun(t, readCompileCheckVibeLog(t, projectRoot)) + for _, key := range []string{"name_skips", "name_index"} { + if _, found := complete.Context[key]; found { + t.Errorf("expected no %s in the log, got %v", key, complete.Context[key]) + } + } +} diff --git a/docs/compile-check.md b/docs/compile-check.md index 569de3b02..2b6afddb2 100644 --- a/docs/compile-check.md +++ b/docs/compile-check.md @@ -55,6 +55,23 @@ references, scripting defines and analyzers. `compile-check` replays those respo private nested type may report the error under another ID (CS0246, CS0426 or CS0117 in place of CS0122, or the other way round) until it compiles again; whether it compiles does not change. When the worker cannot be built or cannot answer, the dependents are compiled. + When the API did change, the worker also says which names the change touches: the simple name of + the type at each level of nesting (with and without an `Attribute` suffix), a namespace segment + only when one side lacks that namespace, the member's name (a property or event rather than its + accessor, the member alone for an explicit implementation), and `new(` for a constructor that a + target-typed `new` can reach. A dependent that writes none of those names - in its own sources + or in the generator output its last full compile saved, comments and strings included - is + dropped the same way, and counts as keeping its surface for the assemblies below it. Whether + a reference touches a dependent never decides which reference assembly the dependent reads when + it compiles for another reason. A change a dependent can feel without writing any name is + unnamed and compiles every dependent: an assembly-level row; for a type both sides have, its + flags or attributes, base type, interfaces, type parameters, an instance field of a value type, + a parameterless constructor or an indexer; an abstract member added; an operator; a + compiler-generated member; a member C# binds by pattern (`GetEnumerator`, `GetAwaiter`, `Add`, + `Deconstruct`, `Dispose`, `SetResult`, the LINQ query methods and the like); every member of an + interpolated string handler or collection builder type; every member of a type in `System`, + `Microsoft.CodeAnalysis` or a namespace under them, which the compiler looks up by name for + `init`, `^1` and the like; members that only moved; and a change naming more than 2000 names. A dependent whose sources cannot be read compiles too. Its answer is kept in `.surface.json`, keyed by the content of both reference assemblies and the worker build, so the runs until Unity builds again do not compare the same pair twice. The comparison is always against Unity's own artifact, never against the @@ -336,6 +353,20 @@ happened, and how long each assembly took. `docs/vibe-logs.md` lists their field invalidate a reused result**, because it is not part of the input digest — the same exposure the dependent skip in step 3 already has. One handed to the compiler as `/analyzerconfig:` is read by content and does invalidate it. `--all` compiles everything again. +- **A dependent left out by names can miss what a generator would write.** The names are read from + the dependent's sources and from the generator output its last full compile saved. Leaving a + dependent out - because its references kept their surface or changed only names it never writes - + removes that saved output, so in steady use the names come from the hand-written sources alone and + the VibeLog's `name_index.with_generated` stays near 0. A generator that emits code using a member + of a referenced type whose name the dependent's own sources never write is then not seen. Closing + that gap needs a left-out dependent to keep its outputs and record, which this check does not do + yet. Neither is a change to the type of an async method builder seen, which a dependent never + names; a change to its members is unnamed and compiles every dependent. +- **Every change to a type in `System`, `Microsoft.CodeAnalysis` or a namespace under them compiles + every dependent.** The compiler looks these types up by name, so only polyfills normally live + there; a project that puts its own extension classes in `System.*` gets no narrowing for them. +- **Identifiers written with `\u` escapes are not decoded** when the dependent's names are read, so + a dependent that writes a changed name only that way is left out. `--all` compiles everything. - **Linux (x86_64) is supported.** The Editor is looked up under `~/Unity/Hub/Editor//Editor/Unity`. ## Troubleshooting diff --git a/docs/vibe-logs.md b/docs/vibe-logs.md index 82260ad37..70510648b 100644 --- a/docs/vibe-logs.md +++ b/docs/vibe-logs.md @@ -280,7 +280,7 @@ because there is no project to write into. | `operation` | Written | `context` | |---|---|---| | `cli_compile_check_start` | once the project is found | `command`, `all`, `full`, `jobs`, `project_identity` | -| `cli_compile_check_complete` (`INFO`; `WARNING` when the run found compile errors; `ERROR` when it could not finish) | on every way out of a run that wrote the start entry | `command`, `outcome` (`succeeded`, `errors`, or `failed`), `duration_ms`; for `failed`, `error` and `error_code`; otherwise `error_count`, `warning_count`, `compiled`, `reused`, `fast_checked`, `blocked` (assembly names, as in the response), `skipped_count`, `fast_checks` (`assembly`, `reused`, `changed_file_count`), `fallbacks` and `baseline_notes` (`assembly`, `reason`), `units` (`assembly`, `duration_ms`, `succeeded` of each assembly the run compiled or checked; replayed ones are left out; `assumed_kept` names the references it started ahead of, assuming they kept Unity's surface, `compiled_again_after` the one whose changed surface made it compile again and `discarded_ms` how long the compile it threw away took, and `waited_for_record` says it waited for its references for a record that then did not apply - each only when it applies), and `surface_checks` (`assembly`, `verdict`, `detail`, `duration_ms`, `from_record` of each assembly whose rebuilt reference assembly differed in bytes from Unity's and was compared by the worker or taken from its record, replayed ones included; `changed_name_count` and `changed_names`, the first 10 of the identifiers a dependent has to write to be affected by the change, and `unnamed`, why no name tells the change, only when it applies) | +| `cli_compile_check_complete` (`INFO`; `WARNING` when the run found compile errors; `ERROR` when it could not finish) | on every way out of a run that wrote the start entry | `command`, `outcome` (`succeeded`, `errors`, or `failed`), `duration_ms`; for `failed`, `error` and `error_code`; otherwise `error_count`, `warning_count`, `compiled`, `reused`, `fast_checked`, `blocked` (assembly names, as in the response), `skipped_count`, `fast_checks` (`assembly`, `reused`, `changed_file_count`), `fallbacks` and `baseline_notes` (`assembly`, `reason`), `units` (`assembly`, `duration_ms`, `succeeded` of each assembly the run compiled or checked; replayed ones are left out; `assumed_kept` names the references it started ahead of, assuming they kept Unity's surface, `compiled_again_after` the one whose changed surface made it compile again and `discarded_ms` how long the compile it threw away took, `waited_for_record` says it waited for its references for a record that then did not apply, and `changed_by` lists the references whose changed surface counted for it (`reference`, and `name`, the first changed name it writes, empty when no name tells the change) - each only when it applies), and `surface_checks` (`assembly`, `verdict`, `detail`, `duration_ms`, `from_record` of each assembly whose rebuilt reference assembly differed in bytes from Unity's and was compared by the worker or taken from its record, replayed ones included; `changed_name_count` and `changed_names`, the first 10 of the identifiers a dependent has to write to be affected by the change, and `unnamed`, why no name tells the change, only when it applies), `name_skips` (`assembly`, `unaffected_by`: the assemblies left out although a reference changed, because they write none of the names the change touches; only when there is any), and `name_index` (`duration_ms`, `units`, `with_generated`, `unreadable`: what reading the names of the assemblies with references in the plan took; only when any was read) | - Unlike the tool-command entries, the complete entry holds the run's error message and fallback reasons: they can name assemblies and files of the project, which the command's own output prints