From b026b5f311cc46dafc3a2d0b24864227a7c5adb8 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Sat, 17 Oct 2026 03:32:05 +0900 Subject: [PATCH 01/11] docs: record that compile-check stays its own command and is shown (#3355) --- ...e-check-is-its-own-command-and-is-shown.md | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md diff --git a/docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md b/docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md new file mode 100644 index 000000000..d01e0f5be --- /dev/null +++ b/docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md @@ -0,0 +1,101 @@ +# Compile Check Is Its Own Command and Is Shown + +Date: 2026-10-17 + +## Decision + +`uloop compile-check` stays a command of its own rather than an option of `uloop compile`, and it +is listed in `--help`, `uloop list`, shell completion and the installed skills. + +It is shown once the inputs that can change after the last Unity build and that it cannot detect +are only the ones under "What Stays Undetected" below. Every other input that moved since the last +Unity build either is compiled from the current files or makes the command refuse with +`COMPILE_CHECK_UNITY_BUILD_REQUIRED`, which a Unity build clears. + +## Context + +`uloop compile-check` replays the response files of the last Unity build through the +Editor-bundled Roslyn compiler. It was hidden because the inputs that could change after that +build without the replay seeing them - the limitations listed in `docs/compile-check.md` - were +too many to recommend it; each of them made it miss errors silently. + +It was not made an option of `uloop compile`, for three reasons: + +1. `uloop compile` produces the assemblies Unity loads and changes the Editor's state. The offline + check touches no Editor and nothing it produces is ever loaded. If a flag decided whether the + result reaches Unity, an agent would read a passing check as an applied change. +2. `uloop compile` is a Unity tool reached over IPC: it needs a running Editor and is rejected with + BUSY while another command runs. The offline check runs without an Editor, takes its own + project lock and fails with its own `COMPILE_CHECK_*` errors. +3. The options of `uloop compile` are defined by the Unity-side tool schema; the skill's parameter + table has to match it, and the default tool catalog is generated from the table. The generator + keeps a short allowlist of options the runner reads before Unity sees them + (`run-tests --skip-compile`, `clear-pause-point --file` and `--line`), so a `--check` flag could + be added the same way. Those options tune how the Unity tool runs; `--check` would make + `compile` never reach Unity at all, so the flag would change what the command is rather than how + it runs. + +## What It Detects + +The command refuses with `COMPILE_CHECK_UNITY_BUILD_REQUIRED` when: + +- there are no Bee build outputs, or which build to replay cannot be decided; +- an assembly definition was added or removed; +- an assembly definition gained a reference, or lost one - except the four Unity adds without an + `.asmdef` line: `UnityEngine.UI`, `UnityEditor.UI`, `UnityEngine.TestRunner`, + `UnityEditor.TestRunner`; +- `allowUnsafeCode` was turned on, or a precompiled reference was added under + `overrideReferences`; +- an assembly definition or assembly definition reference moved; +- a scripting define of the active build target group is missing from the last build; +- since the last Unity build, a scripting define, a `csc.rsp`, the `defineConstraints` or + `versionDefines` of an assembly definition, `Packages/packages-lock.json` or + `Packages/manifest.json` changed. This compares against a record tied to the files Bee rewrites + on every build (`Library/Bee/tundra.log.json` and its neighbours); +- a new script outside every assembly definition belongs to a predefined assembly + (`Assembly-CSharp` and its three siblings) that the last build did not produce. + +## What It Compiles Rather Than Narrows + +A dependent with no saved generator output - one the command has never compiled - is compiled when +a reference it uses changes its surface, instead of being left out by the names it writes. After +that compile its generator output is saved and read like its sources. + +A dependent that writes a changed name only with `\u` escapes is compiled too: the names it writes +are read with the escapes decoded, where ADR 0017 left such a name unseen. + +## What Stays Undetected + +These are documented and not detected; after any of them, run `uloop compile`: + +- (a) a source generator in a dependent that changes what it generates by reading a referenced + assembly's contents rather than the dependent's own code; +- (b) a change to the type of an async method builder; +- (c) on a Unity version whose `Library/Bee` has none of the build-trace files, a scripting define + removed since the last build and a changed `csc.rsp` and the other recorded settings (an added + define is still seen); +- (d) a change made before the record exists - that is, before the first run after that build - + other than an added scripting define: a removed define, or a changed `csc.rsp`, + `defineConstraints`, `versionDefines`, `packages-lock.json` or `manifest.json`; +- (e) a new script in a package folder that has no assembly definition; +- (f) an analyzer config Unity passes with `/analyzerconfig:` that was added since the last build; +- (g) scripting defines are not compared in a project whose active build target is Dedicated + Server (the response files carry `UNITY_SERVER`), because which settings group Unity reads for it + cannot be determined. + +## Consequences + +- Agents find the command through help, the command list and its skill, and can check C# edits + without a running Editor. +- A refusal means "build once in Unity"; a refusal that a Unity build does not clear would make the + command unusable, so every refusal above is cleared by the next `uloop compile`. +- The limitations above stay in `docs/compile-check.md` and the skill, so an agent knows when to + fall back to `uloop compile`. + +## Revisit When + +- A tester round shows the check reporting an error `uloop compile` does not report, or a refusal + that `uloop compile` does not clear. Then hide the command again until the cause is closed. +- Running the check offline before `uloop compile` - returning its errors without touching Unity - + is decided in a separate ADR, once the detection above has gone through one tester round without + a miss. From 2ccb3f15417c427f901991a64dc03516e2dad940 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Sat, 17 Oct 2026 03:42:12 +0900 Subject: [PATCH 02/11] fix: compile-check detects a reference the assembly definition lost (#3356) --- .../CliOnlyTools~/CompileCheck/Skill/SKILL.md | 6 +- .../internal/compilecheck/asmdef_contract.go | 26 +++-- .../compilecheck/asmdef_lost_reference.go | 41 ++++++++ .../asmdef_lost_reference_test.go | 99 +++++++++++++++++++ .../compilecheck/assembly_reference_test.go | 5 +- docs/compile-check.md | 25 ++--- 6 files changed, 178 insertions(+), 24 deletions(-) create mode 100644 cli/dispatcher/internal/compilecheck/asmdef_lost_reference.go create mode 100644 cli/dispatcher/internal/compilecheck/asmdef_lost_reference_test.go diff --git a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md index 6e73277ee..6af8674c3 100644 --- a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md +++ b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md @@ -110,9 +110,9 @@ compile-check once more rather than retrying in a loop. - Changes to `defineConstraints` and `versionDefines` are **not** detected, because they depend on scripting defines and package versions that cannot be evaluated outside the Editor. Run `uloop compile` after editing them. -- A reference **removed** from an `.asmdef` is not detected either: Unity injects references of its - own that no `.asmdef` declares, so only added references can be told apart from those. Diagnostics - may therefore miss an error that the removal would cause. Run `uloop compile` after removing one. +- A reference removed from an `.asmdef` stops the run with `COMPILE_CHECK_UNITY_BUILD_REQUIRED`; the + four references Unity adds on its own are allowed, and an `.asmdef` with a reference that matches + no `.asmdef` in the project is not judged. - Changed scripting defines are not detected; the defines from the last build are reused. - The predefined assemblies (`Assembly-CSharp` and friends) keep the source list of the last build, so a brand-new `.cs` file outside an `.asmdef` is not compiled until Unity imports it. diff --git a/cli/dispatcher/internal/compilecheck/asmdef_contract.go b/cli/dispatcher/internal/compilecheck/asmdef_contract.go index 2d9d1ead9..ea10263a1 100644 --- a/cli/dispatcher/internal/compilecheck/asmdef_contract.go +++ b/cli/dispatcher/internal/compilecheck/asmdef_contract.go @@ -15,8 +15,9 @@ const ( // AssemblyContext is what the contract check needs to know about the rest of the project. type AssemblyContext struct { - BuiltAssemblies map[string]bool // assembly names Bee wrote a response file for - AssemblyByGUID map[string]string // .asmdef GUID -> the assembly name it declares + BuiltAssemblies map[string]bool // assembly names Bee wrote a response file for + DefinedAssemblies map[string]bool // assembly names an .asmdef in the project declares, built or not + AssemblyByGUID map[string]string // .asmdef GUID -> the assembly name it declares } // NewAssemblyContext collects the project-wide lookups the contract check compares against. @@ -28,14 +29,16 @@ func NewAssemblyContext( built[name] = true } + defined := make(map[string]bool, len(assemblyDefinitions)) byGUID := make(map[string]string, len(assemblyDefinitions)) for name, definition := range assemblyDefinitions { + defined[name] = true if definition.GUID != "" { byGUID[definition.GUID] = name } } - return AssemblyContext{BuiltAssemblies: built, AssemblyByGUID: byGUID} + return AssemblyContext{BuiltAssemblies: built, DefinedAssemblies: defined, AssemblyByGUID: byGUID} } // assemblyDefinitionContract is the part of an .asmdef the response file has to agree with. @@ -97,14 +100,17 @@ func detectContractDisagreement( if name, found := missingPrecompiledReference(contract, rsp); found { return fmt.Sprintf("now requires the precompiled reference %s", name) } + if name, found := lostProjectReference(contract, rsp, dagDir, context); found { + return fmt.Sprintf("no longer references %s", name) + } return "" } // missingProjectReference names an assembly the .asmdef references that the response file lacks. -// Why the check is one-directional: Unity injects references of its own (the UI and test-runner -// assemblies) that no .asmdef declares, so demanding equality would reject healthy projects. Only a -// reference the .asmdef gained is detectable; a removed one is documented as undetectable instead. +// Why a removed reference is checked separately: the only references Unity adds without an .asmdef +// line are the four in unityInjectedProjectReferences, so a response-file reference outside the +// .asmdef and that list is one the .asmdef lost; lostProjectReference reports it. func missingProjectReference( contract assemblyDefinitionContract, rsp ResponseFile, dagDir string, context AssemblyContext, ) (string, bool) { @@ -127,10 +133,14 @@ func missingProjectReference( return "", false } -// resolveReferenceName turns one .asmdef reference, in GUID or name form, into an assembly name. +// resolveReferenceName turns one .asmdef reference, in GUID or name form, into the name of an +// assembly an .asmdef in the project declares. false means no .asmdef matches it. +// Why a name reference is looked up too: Unity rewrites some names when it reads an .asmdef - the old +// Unity.RenderPipelines.Lightweight.* names become Unity.RenderPipelines.Universal.* - so a name no +// .asmdef declares may still stand for a reference the response file carries under another name. func resolveReferenceName(reference string, context AssemblyContext) (string, bool) { if !strings.HasPrefix(reference, guidReferencePrefix) { - return reference, reference != "" + return reference, context.DefinedAssemblies[reference] } name, found := context.AssemblyByGUID[strings.TrimPrefix(reference, guidReferencePrefix)] diff --git a/cli/dispatcher/internal/compilecheck/asmdef_lost_reference.go b/cli/dispatcher/internal/compilecheck/asmdef_lost_reference.go new file mode 100644 index 000000000..a9e69c6b8 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/asmdef_lost_reference.go @@ -0,0 +1,41 @@ +package compilecheck + +// unityInjectedProjectReferences are the project references Unity adds to a response file without +// an .asmdef line: the two UI assemblies come from its auto-referenced package assemblies and the two +// test-runner assemblies from its test-runner rules. +var unityInjectedProjectReferences = map[string]bool{ + "UnityEngine.UI": true, + "UnityEditor.UI": true, + "UnityEngine.TestRunner": true, + "UnityEditor.TestRunner": true, +} + +// lostProjectReference names a project assembly the response file references that the .asmdef no +// longer declares and Unity does not add on its own. +// Why every declared reference is resolved before the response file is read: one reference that +// cannot be resolved leaves the declared set incomplete, and then a response-file reference outside +// it may be a lost one or merely the unresolved one. Such an .asmdef is not judged, which prefers a +// miss over refusing a healthy project, the same way missingProjectReference skips what it cannot +// resolve. +func lostProjectReference( + contract assemblyDefinitionContract, rsp ResponseFile, dagDir string, context AssemblyContext, +) (string, bool) { + declared := make(map[string]bool, len(contract.References)) + for _, reference := range contract.References { + name, resolved := resolveReferenceName(reference, context) + if !resolved { + return "", false + } + declared[name] = true + } + + for _, name := range rsp.ProjectAssemblyReferences(dagDir) { + if declared[name] || unityInjectedProjectReferences[name] { + continue + } + + return name, true + } + + return "", false +} diff --git a/cli/dispatcher/internal/compilecheck/asmdef_lost_reference_test.go b/cli/dispatcher/internal/compilecheck/asmdef_lost_reference_test.go new file mode 100644 index 000000000..e453bc34f --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/asmdef_lost_reference_test.go @@ -0,0 +1,99 @@ +package compilecheck + +import ( + "testing" +) + +// contractDisagreementOf reads A's assembly definition from a contract project and returns what the +// contract check says about it against the given response file. +func contractDisagreementOf( + t *testing.T, rsp ResponseFile, asmdef AssemblyDefinition, context AssemblyContext, +) string { + t.Helper() + contract, err := readAssemblyDefinitionContract(asmdef.Path) + if err != nil { + t.Fatalf("failed to read the assembly definition contract: %v", err) + } + + return detectContractDisagreement(contract, rsp, planDagDirectory, context) +} + +// Verifies the lost-reference check is skipped when a declared GUID resolves to no assembly +// definition, since the response-file reference may be that unresolved one rather than a lost one. +func TestDetectContractDisagreementSkipsTheLostReferenceCheckWhenAReferenceGUIDIsUnresolvable(t *testing.T) { + _, rsp, asmdef, context := newContractProject( + t, `{"name":"A","references":["GUID:00000000000000000000000000000000"]}`) + + if reason := contractDisagreementOf(t, rsp, asmdef, context); reason != "" { + t.Fatalf("expected an unresolvable GUID to skip the lost-reference check, got %q", reason) + } +} + +// Verifies the lost-reference check is skipped when a declared name matches no assembly definition, +// such as an old render-pipeline name Unity rewrites to the one the response file carries. +func TestDetectContractDisagreementSkipsTheLostReferenceCheckWhenAReferenceNamesNoAssemblyDefinition(t *testing.T) { + _, rsp, asmdef, context := newContractProject( + t, `{"name":"A","references":["B","Unity.RenderPipelines.Lightweight.Runtime"]}`) + rsp.References = append(rsp.References, + planDagDirectory+"/Unity.RenderPipelines.Universal.Runtime.ref.dll") + + if reason := contractDisagreementOf(t, rsp, asmdef, context); reason != "" { + t.Fatalf("expected a name no assembly definition declares to skip the lost-reference check, got %q", reason) + } +} + +// Verifies a response-file reference the assembly definition still declares, by name or by GUID, is +// not reported as lost. +func TestDetectContractDisagreementAcceptsAReferenceTheAssemblyDefinitionStillDeclares(t *testing.T) { + cases := map[string]string{ + "by name": `{"name":"A","references":["B"]}`, + "by GUID": `{"name":"A","references":["GUID:` + referencedGUID + `"]}`, + } + for name, body := range cases { + t.Run(name, func(t *testing.T) { + _, rsp, asmdef, context := newContractProject(t, body) + + if reason := contractDisagreementOf(t, rsp, asmdef, context); reason != "" { + t.Fatalf("expected a still-declared reference to be accepted, got %q", reason) + } + }) + } +} + +// Verifies each project reference Unity adds without an .asmdef line is not reported as lost. +func TestDetectContractDisagreementAcceptsTheReferencesUnityInjects(t *testing.T) { + for _, injected := range []string{ + "UnityEngine.UI", "UnityEditor.UI", "UnityEngine.TestRunner", "UnityEditor.TestRunner", + } { + t.Run(injected, func(t *testing.T) { + _, rsp, asmdef, context := newContractProject(t, `{"name":"A","references":["B"]}`) + rsp.References = append(rsp.References, planDagDirectory+"/"+injected+".ref.dll") + + if reason := contractDisagreementOf(t, rsp, asmdef, context); reason != "" { + t.Fatalf("expected the Unity-injected %s to be accepted, got %q", injected, reason) + } + }) + } +} + +// Verifies a project reference the response file still carries but the assembly definition no +// longer declares is refused, naming the lost assembly. +func TestDetectContractDisagreementRefusesAReferenceTheAssemblyDefinitionLost(t *testing.T) { + _, rsp, asmdef, context := newContractProject(t, `{"name":"A"}`) + + reason := contractDisagreementOf(t, rsp, asmdef, context) + if reason != "no longer references B" { + t.Fatalf("expected the lost reference B to be refused, got %q", reason) + } +} + +// Verifies a reference the assembly definition gained is reported before one it lost, so the +// existing order of the contract checks is kept. +func TestDetectContractDisagreementReportsAGainedReferenceBeforeALostOne(t *testing.T) { + _, rsp, asmdef, context := newContractProject(t, `{"name":"A","references":["C"]}`) + + reason := contractDisagreementOf(t, rsp, asmdef, context) + if reason != "now references C, which the last build did not compile against" { + t.Fatalf("expected the gained reference C to be reported first, got %q", reason) + } +} diff --git a/cli/dispatcher/internal/compilecheck/assembly_reference_test.go b/cli/dispatcher/internal/compilecheck/assembly_reference_test.go index 1daf3ad2e..1683945f8 100644 --- a/cli/dispatcher/internal/compilecheck/assembly_reference_test.go +++ b/cli/dispatcher/internal/compilecheck/assembly_reference_test.go @@ -38,8 +38,9 @@ func newAssemblyReferenceProject(t *testing.T) (string, assemblyGraph, AssemblyC }, } context := AssemblyContext{ - BuiltAssemblies: map[string]bool{"P": true, "Q": true}, - AssemblyByGUID: map[string]string{"qqqq": "Q"}, + BuiltAssemblies: map[string]bool{"P": true, "Q": true}, + DefinedAssemblies: map[string]bool{"P": true, "Q": true}, + AssemblyByGUID: map[string]string{"qqqq": "Q"}, } return projectRoot, graph, context, buildTime diff --git a/docs/compile-check.md b/docs/compile-check.md index 6183fd8be..7b4b7be1f 100644 --- a/docs/compile-check.md +++ b/docs/compile-check.md @@ -117,8 +117,11 @@ references, scripting defines and analyzers. `compile-check` replays those respo order gives an assembly nobody edited a reference assembly in other bytes than Unity's, and every assembly that references it compiles again. 4. **Check that the response files still describe the project.** An assembly definition that was - added, removed, or that stopped building for the Editor, and a reference or a precompiled - reference that was added to one, invalidate the recorded build. The run then stops with + added, removed, or that stopped building for the Editor, a reference or a precompiled + reference that was added to one, and a project reference it lost (a reference in the response + file that the `.asmdef` no longer declares and Unity does not add on its own: `UnityEngine.UI`, + `UnityEditor.UI`, `UnityEngine.TestRunner`, `UnityEditor.TestRunner`), invalidate the recorded + build. The run then stops with `COMPILE_CHECK_UNITY_BUILD_REQUIRED` rather than reporting diagnostics for a configuration the project no longer has. 5. **Compile in dependency order** and parse the compiler's diagnostics into JSON. An assembly @@ -339,10 +342,12 @@ happened, and how long each assembly took. `docs/vibe-logs.md` lists their field ## Limitations -- **It replays the last Unity build, and only some staleness is detectable.** Four `.asmdef` +- **It replays the last Unity build, and only some staleness is detectable.** Five `.asmdef` settings are compared against the response file, because they reach `csc` and can be read without evaluating defines or package versions: whether the assembly still builds for the Editor, a - project reference it gained, `allowUnsafeCode` turning on, and a precompiled reference it gained + project reference it gained, a project reference it lost (a reference in the response file that + the `.asmdef` no longer declares and Unity does not add on its own: `UnityEngine.UI`, + `UnityEditor.UI`, `UnityEngine.TestRunner`, `UnityEditor.TestRunner`), `allowUnsafeCode` turning on, and a precompiled reference it gained under `overrideReferences`. A new or deleted `.asmdef` is detected too, as is every way assembly membership moves without any `.cs` file changing: an `.asmdef` that no longer sits over any source its response file recorded (it was moved into another assembly's folder), an `.asmref` @@ -352,10 +357,6 @@ happened, and how long each assembly took. `docs/vibe-logs.md` lists their field `uloop compile`. Every other edit listed below is invisible to the check: the run proceeds and silently reuses what the last build recorded. -- **A removed `.asmdef` reference is not detected.** Unity injects references of its own that no - `.asmdef` declares, so a reference present in the response file but absent from the `.asmdef` - cannot be told apart from an injected one. Diagnostics may therefore miss an error that removing - a reference would cause. Run `uloop compile` after removing one. - **`defineConstraints` and `versionDefines` changes are not detected**, because they depend on scripting defines and package versions that can only be evaluated inside the Editor. - **Changed scripting defines are not detected**; the defines recorded in the last build are reused. @@ -410,9 +411,11 @@ run this headless build once per job before calling compile-check. **`COMPILE_CHECK_UNITY_BUILD_REQUIRED`: an assembly definition no longer matches the last build** Raised when an `.asmdef` was added, deleted or moved, when an `.asmref` was added, moved, removed or retargeted so that a folder's sources now belong to a different assembly than the one the last -build compiled them into, or when one of the four compared settings changed (no longer builds for -the Editor, gained a project reference, turned on `allowUnsafeCode`, gained a precompiled reference -under `overrideReferences`). This is the intended refusal, not a bug. Run +build compiled them into, or when one of the five compared settings changed (no longer builds for +the Editor, gained a project reference, lost a project reference, turned on `allowUnsafeCode`, +gained a precompiled reference under `overrideReferences`). A lost reference reads +`no longer references `; a reference, by GUID or by name, that matches no `.asmdef` in the project +makes the check skip that `.asmdef` rather than guess. This is the intended refusal, not a bug. Run `uloop compile` once so Unity rewrites the response files, then `compile-check` works again against the new configuration. From d32cb9369ee770f14894be3f1cd1842b4f043f3f Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Sat, 17 Oct 2026 04:10:11 +0900 Subject: [PATCH 03/11] feat: compile-check detects build settings that moved since the last Unity build (#3357) --- .../CliOnlyTools~/CompileCheck/Skill/SKILL.md | 15 +- .../internal/compilecheck/build_settings.go | 261 ++++++++++ .../compilecheck/build_settings_record.go | 305 +++++++++++ .../compilecheck/build_settings_test.go | 478 ++++++++++++++++++ .../build_settings_wiring_test.go | 59 +++ cli/dispatcher/internal/compilecheck/plan.go | 45 +- .../compilecheck/resultstore/baseline.go | 2 +- .../resultstore/compile_result.go | 2 +- .../compilecheck/resultstore/fast_check.go | 2 +- .../compilecheck/resultstore/store.go | 4 +- .../compilecheck/resultstore/surface.go | 2 +- .../internal/compilecheck/session.go | 11 +- .../dispatcher/compile_check_vibe_log.go | 15 + .../dispatcher/compile_check_vibe_log_test.go | 51 ++ ...e-check-is-its-own-command-and-is-shown.md | 5 +- docs/compile-check.md | 60 ++- docs/vibe-logs.md | 2 +- 17 files changed, 1275 insertions(+), 44 deletions(-) create mode 100644 cli/dispatcher/internal/compilecheck/build_settings.go create mode 100644 cli/dispatcher/internal/compilecheck/build_settings_record.go create mode 100644 cli/dispatcher/internal/compilecheck/build_settings_test.go create mode 100644 cli/dispatcher/internal/compilecheck/build_settings_wiring_test.go diff --git a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md index 6af8674c3..344139fe9 100644 --- a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md +++ b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md @@ -35,8 +35,8 @@ in `Library/uloop/compile-check/` and are never handed to the Editor. ``` `uloop launch` does the same with the Editor window open. After that one build, compile-check works - without the Editor; it asks for another build when it detects an `.asmdef` change. A scripting define - change it cannot detect, so run `uloop compile` yourself after one. + without the Editor; it asks for another build when it detects an `.asmdef` change or a build setting + that moved; see the limitations for what stays unseen. ## Usage @@ -107,13 +107,16 @@ compile-check once more rather than retrying in a loop. - It replays the last Unity build's response files. A new or deleted `.asmdef`, a changed `.asmdef` reference, an `.asmdef` that stopped building for the Editor, or a newly required precompiled reference stops the run and asks for `uloop compile`. -- Changes to `defineConstraints` and `versionDefines` are **not** detected, because they depend on - scripting defines and package versions that cannot be evaluated outside the Editor. Run - `uloop compile` after editing them. +- Build settings are compared against a record the first run after a Unity build writes: a + removed scripting define, a changed `csc.rsp`, `defineConstraints`, `versionDefines`, + `packages-lock.json` or `manifest.json` stops the run, and a scripting define the last build lacks + stops it when `ProjectSettings.asset` was saved after that build. A change made before that first + run (other than an added define) is not seen, so run `uloop compile` after one. Under a Unity 6 + build profile that overrides Player Settings or carries its own defines, only the global + ProjectSettings defines are compared. - A reference removed from an `.asmdef` stops the run with `COMPILE_CHECK_UNITY_BUILD_REQUIRED`; the four references Unity adds on its own are allowed, and an `.asmdef` with a reference that matches no `.asmdef` in the project is not judged. -- Changed scripting defines are not detected; the defines from the last build are reused. - The predefined assemblies (`Assembly-CSharp` and friends) keep the source list of the last build, so a brand-new `.cs` file outside an `.asmdef` is not compiled until Unity imports it. - The produced DLLs are never loaded by Unity, and the command never contacts a running Editor. diff --git a/cli/dispatcher/internal/compilecheck/build_settings.go b/cli/dispatcher/internal/compilecheck/build_settings.go new file mode 100644 index 000000000..db4a691a2 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/build_settings.go @@ -0,0 +1,261 @@ +package compilecheck + +import ( + "bufio" + "bytes" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" +) + +const ( + projectSettingsRelativePath = "ProjectSettings/ProjectSettings.asset" + packageManifestRelativePath = "Packages/manifest.json" + packageLockRelativePath = "Packages/packages-lock.json" + assetsResponseFileRelative = "Assets/csc.rsp" + compilerResponseFileName = "csc.rsp" + scriptingDefineSymbolsKey = "scriptingDefineSymbols:" + scriptingDefineSymbolsIndent = 2 + scriptingDefineEntryIndent = 4 + textYAMLHeader = "%YAML" + dedicatedServerDefine = "UNITY_SERVER" +) + +// defineGroupsByPlatformDefine maps the platform define Bee writes into every response file to the +// key ProjectSettings files that platform's scripting defines under, in the order they are tried. +var defineGroupsByPlatformDefine = []struct { + define string + group string +}{ + {"UNITY_STANDALONE", "Standalone"}, + {"UNITY_IOS", "iPhone"}, + {"UNITY_ANDROID", "Android"}, + {"UNITY_WEBGL", "WebGL"}, + {"UNITY_TVOS", "tvOS"}, + {"UNITY_VISIONOS", "VisionOS"}, + {"UNITY_WSA", "Windows Store Apps"}, +} + +// buildSettings are inputs of a Unity build that the response files cannot be read back into. They +// are recorded and compared with the next run's. +type buildSettings struct { + // DefineGroup is the ProjectSettings group of the response files' platform, "" when it cannot be + // decided. + DefineGroup string `json:"defineGroup"` + // ProjectDefines are that group's scripting defines, sorted; nil when DefineGroup is "". + ProjectDefines []string `json:"projectDefines"` + // ResponseFileDigests maps each csc.rsp Unity may read (project-relative, slash-separated) to the + // sha256 of its content. A file that does not exist is left out. + ResponseFileDigests map[string]string `json:"responseFileDigests"` + // AssemblyConstraints maps each .asmdef outside the package cache (project-relative) to the + // sha256 of its defineConstraints and versionDefines. + AssemblyConstraints map[string]string `json:"assemblyConstraints"` + ManifestDigest string `json:"manifestDigest"` // "" when the file does not exist + PackageLockDigest string `json:"packageLockDigest"` // "" when the file does not exist +} + +// readBuildSettings reads the build settings of the project as they are now. +func readBuildSettings( + projectRoot string, graph assemblyGraph, assemblyDefinitions map[string]AssemblyDefinition, +) (buildSettings, error) { + settings := buildSettings{ + ResponseFileDigests: map[string]string{}, + AssemblyConstraints: map[string]string{}, + } + + group, defines, err := readProjectDefines(projectRoot, defineGroupOf(graph)) + if err != nil { + return buildSettings{}, err + } + settings.DefineGroup, settings.ProjectDefines = group, defines + + if err := addResponseFileDigests(settings.ResponseFileDigests, projectRoot, assemblyDefinitions); err != nil { + return buildSettings{}, err + } + if err := addAssemblyConstraints(settings.AssemblyConstraints, projectRoot, assemblyDefinitions); err != nil { + return buildSettings{}, err + } + + manifest, err := optionalFileDigest(filepath.Join(projectRoot, filepath.FromSlash(packageManifestRelativePath))) + if err != nil { + return buildSettings{}, err + } + lock, err := optionalFileDigest(filepath.Join(projectRoot, filepath.FromSlash(packageLockRelativePath))) + if err != nil { + return buildSettings{}, err + } + settings.ManifestDigest, settings.PackageLockDigest = manifest, lock + + return settings, nil +} + +// defineGroupOf decides the ProjectSettings group the response files were built for, from the +// platform define of the first response file in name order. "" means it cannot be decided. +// Why UNITY_SERVER is checked first: a Dedicated Server build carries UNITY_STANDALONE too, and +// whether Unity reads its defines from the Server key or the Standalone key cannot be confirmed. +func defineGroupOf(graph assemblyGraph) string { + if len(graph.names) == 0 { + return "" + } + defines := map[string]bool{} + for _, define := range graph.byName[graph.names[0]].Defines { + defines[define] = true + } + if defines[dedicatedServerDefine] { + return "" + } + for _, candidate := range defineGroupsByPlatformDefine { + if defines[candidate.define] { + return candidate.group + } + } + + return "" +} + +// readProjectDefines reads the scripting defines ProjectSettings keeps for one group, and returns +// the group it could read them for: "" with nil defines when the group is unknown or the file is +// not text. +func readProjectDefines(projectRoot string, group string) (string, []string, error) { + if group == "" { + return "", nil, nil + } + content, err := os.ReadFile(filepath.Join(projectRoot, filepath.FromSlash(projectSettingsRelativePath))) + if errors.Is(err, fs.ErrNotExist) { + return "", nil, nil + } + if err != nil { + return "", nil, fmt.Errorf("failed to read %s: %w", projectSettingsRelativePath, err) + } + // Why a binary file gives up on the defines: Unity can serialize ProjectSettings in binary, + // which this line scan cannot read, and guessing would refuse runs Unity would build. + if !bytes.HasPrefix(content, []byte(textYAMLHeader)) { + return "", nil, nil + } + + value, found := scriptingDefinesValue(content, group) + if !found { + return group, []string{}, nil + } + + return group, splitDefines(value), nil +} + +// scriptingDefinesValue finds the value of one group's line under scriptingDefineSymbols. +func scriptingDefinesValue(content []byte, group string) (string, bool) { + scanner := bufio.NewScanner(bytes.NewReader(content)) + scanner.Buffer(make([]byte, 0, 64*1024), len(content)+1) + inside := false + for scanner.Scan() { + line := strings.TrimRight(scanner.Text(), "\r") + indent := len(line) - len(strings.TrimLeft(line, " ")) + trimmed := strings.TrimSpace(line) + if !inside { + inside = indent == scriptingDefineSymbolsIndent && trimmed == scriptingDefineSymbolsKey + continue + } + if trimmed == "" { + continue + } + if indent < scriptingDefineEntryIndent { + return "", false + } + key, value, hasValue := strings.Cut(trimmed, ":") + if hasValue && key == group { + return strings.TrimSpace(value), true + } + } + + return "", false +} + +// splitDefines splits a scripting define value on every separator Unity accepts, sorted. +func splitDefines(value string) []string { + defines := strings.FieldsFunc(value, func(character rune) bool { + return character == ';' || character == ',' || character == ' ' || character == '\t' + }) + sort.Strings(defines) + + return defines +} + +// addResponseFileDigests records Assets/csc.rsp and the csc.rsp beside each .asmdef outside the +// package cache, by the digest of their content. +func addResponseFileDigests( + digests map[string]string, projectRoot string, assemblyDefinitions map[string]AssemblyDefinition, +) error { + paths := []string{filepath.Join(projectRoot, filepath.FromSlash(assetsResponseFileRelative))} + for _, definition := range assemblyDefinitions { + if isUnderPackageCache(projectRoot, definition.Directory) { + continue + } + paths = append(paths, filepath.Join(definition.Directory, compilerResponseFileName)) + } + + for _, path := range paths { + digest, err := optionalFileDigest(path) + if err != nil { + return err + } + if digest != "" { + digests[recordSourcePath(projectRoot, path)] = digest + } + } + + return nil +} + +// addAssemblyConstraints records the defineConstraints and versionDefines of each .asmdef outside +// the package cache. +// Why the package cache is left out: the package manager restores it and nobody edits it by hand, +// and a package version change already shows in packages-lock.json. +func addAssemblyConstraints( + constraints map[string]string, projectRoot string, assemblyDefinitions map[string]AssemblyDefinition, +) error { + for _, definition := range assemblyDefinitions { + if isUnderPackageCache(projectRoot, definition.Directory) { + continue + } + var read struct { + DefineConstraints []string `json:"defineConstraints"` + VersionDefines []map[string]any `json:"versionDefines"` + } + if err := readUnityJSONFile(definition.Path, &read); err != nil { + return fmt.Errorf("failed to read %s: %w", definition.Path, err) + } + normalized, err := json.Marshal(read) + if err != nil { + return fmt.Errorf("failed to read %s: %w", definition.Path, err) + } + constraints[recordSourcePath(projectRoot, definition.Path)] = digestOf(normalized) + } + + return nil +} + +// optionalFileDigest is the sha256 of a file's content, or "" when the file does not exist. +func optionalFileDigest(path string) (string, error) { + content, err := os.ReadFile(path) + if errors.Is(err, fs.ErrNotExist) { + return "", nil + } + if err != nil { + return "", fmt.Errorf("failed to read %s: %w", path, err) + } + + return digestOf(content), nil +} + +// digestOf is the hex sha256 of some bytes. +func digestOf(content []byte) string { + sum := sha256.Sum256(content) + + return hex.EncodeToString(sum[:]) +} diff --git a/cli/dispatcher/internal/compilecheck/build_settings_record.go b/cli/dispatcher/internal/compilecheck/build_settings_record.go new file mode 100644 index 000000000..91746475d --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/build_settings_record.go @@ -0,0 +1,305 @@ +package compilecheck + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "sort" + "strings" + + "github.com/hatayama/unity-cli-loop/dispatcher/internal/compilecheck/resultstore" +) + +const ( + buildSettingsRecordFileName = "build-settings.json" + buildSettingsRecordFormatVersion = 1 + buildSettingsRecordNew = "new" + buildSettingsRecordReplaced = "replaced" + buildSettingsRecordKept = "kept" +) + +// buildStampRelativePaths are the files Bee rewrites on every build, even one that compiles nothing, +// tried in this order. +var buildStampRelativePaths = []string{ + "Library/Bee/tundra.log.json", + "Library/Bee/TundraBuildState.state", + "Library/Bee/fullprofile.json", +} + +// buildStamp identifies the last Unity build by the newest file Bee rewrote for it. +type buildStamp struct { + Path string `json:"path"` // project-relative + ModTime int64 `json:"modTime"` // UnixNano + Size int64 `json:"size"` +} + +// buildSettingsRecord is the build settings a run found right after a Unity build, keyed to that +// build. +type buildSettingsRecord struct { + FormatVersion int `json:"formatVersion"` + Stamp buildStamp `json:"stamp"` + Settings buildSettings `json:"settings"` +} + +// BuildSettingsReport says what the build-settings comparison of a run found. +type BuildSettingsReport struct { + StampFound bool // a build-trace file of the last Unity build exists + Record string // "new", "replaced", "kept", or "" when nothing was recorded or compared + // RefusedPart names the setting that moved since the recorded build when the run was refused + // for it. + RefusedPart string + // DefinesChecked says the run looked for a project define the build lacks, which it does only + // when there is no build-trace file or ProjectSettings was saved after the last build. + DefinesChecked bool +} + +// BuildSettingsError carries the build-settings report of a run that stopped with an error after +// the comparison ran. It unwraps to that error, so the error still classifies as itself. +type BuildSettingsError struct { + Report BuildSettingsReport + Err error +} + +func (err BuildSettingsError) Error() string { return err.Err.Error() } + +func (err BuildSettingsError) Unwrap() error { return err.Err } + +// BuildSettingsReportOf returns the build-settings report a failed run's error carries. +func BuildSettingsReportOf(err error) (BuildSettingsReport, bool) { + var carrier BuildSettingsError + if !errors.As(err, &carrier) { + return BuildSettingsReport{}, false + } + + return carrier.Report, true +} + +// withBuildSettingsReport attaches the build-settings report to an error, leaving nil as nil. +func withBuildSettingsReport(report BuildSettingsReport, err error) error { + if err == nil { + return nil + } + + return BuildSettingsError{Report: report, Err: err} +} + +// DetectBuildSettingsChange refuses a run whose build settings are not the ones the last Unity +// build used: a scripting define of the active group the response files lack, and - against a +// record keyed to the last build - any recorded setting that moved since. +// Why the one-way define check runs before the record and on every run: an added define should be +// seen by the first run as well, and a record written before the check would take the added define +// for part of the build. +// Why a different stamp never refuses: Unity built again, so the response files reflect the +// settings as they are now and there is nothing to compare them with; a setting removed afterwards +// shows against the record the next run reads. +func DetectBuildSettingsChange( + projectRoot string, dagDir string, graph assemblyGraph, + assemblyDefinitions map[string]AssemblyDefinition, +) (BuildSettingsReport, error) { + report := BuildSettingsReport{} + current, err := readBuildSettings(projectRoot, graph, assemblyDefinitions) + if err != nil { + return report, err + } + stamp, found := readBuildStamp(projectRoot) + report.StampFound = found + report.DefinesChecked = !found || projectSettingsSavedAfter(projectRoot, stamp) + if report.DefinesChecked { + if define := defineTheBuildLacks(current, graph); define != "" { + return report, unityBuildRequired("scripting define %s is not in the last Unity build", define) + } + } + if !found { + return report, nil + } + + recordPath := filepath.Join(projectRoot, filepath.FromSlash(compileCheckOutputRoot), + filepath.Base(dagDir), buildSettingsRecordFileName) + recorded, loaded := loadBuildSettingsRecord(recordPath) + if loaded && recorded.Stamp == stamp { + report.Record = buildSettingsRecordKept + if part := firstDifference(recorded.Settings, current); part != "" { + report.RefusedPart = part + return report, unityBuildRequired("%s changed since the last Unity build", part) + } + return report, nil + } + + report.Record = buildSettingsRecordNew + if loaded { + report.Record = buildSettingsRecordReplaced + } + record := buildSettingsRecord{ + FormatVersion: buildSettingsRecordFormatVersion, Stamp: stamp, Settings: current, + } + + return report, writeBuildSettingsRecord(recordPath, record) +} + +// projectSettingsSavedAfter reports whether ProjectSettings was saved after the build the stamp +// belongs to. +// Why an older ProjectSettings is not checked for defines the build lacks: its defines were already +// in force when Unity built, so one missing from the response files was left out for a reason this +// check cannot see - a Unity 6 build profile that overrides Player Settings is one - and refusing it +// would refuse every run, since building again writes the same response files. +func projectSettingsSavedAfter(projectRoot string, stamp buildStamp) bool { + info, err := os.Stat(filepath.Join(projectRoot, filepath.FromSlash(projectSettingsRelativePath))) + if err != nil { + return false + } + + return info.ModTime().UnixNano() > stamp.ModTime +} + +// defineTheBuildLacks names a scripting define of the active group that some response file lacks. +// Unity adds the project's defines to every assembly, so one missing anywhere was added after the +// build. +// Why csc.rsp defines are not checked here: Unity reads the csc.rsp beside an .asmdef instead of +// Assets/csc.rsp and copies its text without splitting it, so reading defines back out of the +// response files would refuse projects Unity builds; csc.rsp is compared against the record only. +func defineTheBuildLacks(current buildSettings, graph assemblyGraph) string { + if current.DefineGroup == "" { + return "" + } + for _, define := range current.ProjectDefines { + for _, name := range graph.names { + if !slices.Contains(graph.byName[name].Defines, define) { + return define + } + } + } + + return "" +} + +// readBuildStamp finds the newest build-trace file Bee left. false means none exists. +func readBuildStamp(projectRoot string) (buildStamp, bool) { + newest := buildStamp{} + found := false + for _, relativePath := range buildStampRelativePaths { + info, err := os.Stat(filepath.Join(projectRoot, filepath.FromSlash(relativePath))) + if err != nil { + continue + } + modTime := info.ModTime().UnixNano() + if found && modTime <= newest.ModTime { + continue + } + newest = buildStamp{Path: relativePath, ModTime: modTime, Size: info.Size()} + found = true + } + + return newest, found +} + +// loadBuildSettingsRecord reads the record. false means there is none this version can use. +func loadBuildSettingsRecord(path string) (buildSettingsRecord, bool) { + record := buildSettingsRecord{} + content, err := os.ReadFile(path) + if err != nil { + return record, false + } + if json.Unmarshal(content, &record) != nil || record.FormatVersion != buildSettingsRecordFormatVersion { + return buildSettingsRecord{}, false + } + + return record, true +} + +// writeBuildSettingsRecord writes the record through a temporary file, so a reader never sees half +// of it. +func writeBuildSettingsRecord(path string, record buildSettingsRecord) error { + content, err := json.Marshal(record) + if err != nil { + return fmt.Errorf("failed to encode %s: %w", path, err) + } + directory := filepath.Dir(path) + if err := os.MkdirAll(directory, 0o755); err != nil { + return fmt.Errorf("failed to create %s: %w", directory, err) + } + + return resultstore.WriteFileAtomically(directory, path, content) +} + +// firstDifference names the first recorded setting that differs now, or "" when none does. +func firstDifference(recorded buildSettings, current buildSettings) string { + if part := defineDifference(recorded, current); part != "" { + return part + } + if path := firstDifferentKey(recorded.ResponseFileDigests, current.ResponseFileDigests); path != "" { + return path + } + if path := firstDifferentKey(recorded.AssemblyConstraints, current.AssemblyConstraints); path != "" { + return "defineConstraints or versionDefines of " + path + } + if recorded.PackageLockDigest != current.PackageLockDigest { + return packageLockRelativePath + } + if recorded.ManifestDigest != current.ManifestDigest { + return packageManifestRelativePath + } + + return "" +} + +// defineDifference describes how the scripting defines moved, or "" when they did not. +func defineDifference(recorded buildSettings, current buildSettings) string { + if recorded.DefineGroup != current.DefineGroup { + return fmt.Sprintf("scripting define group (%q to %q)", recorded.DefineGroup, current.DefineGroup) + } + added := missingFrom(current.ProjectDefines, recorded.ProjectDefines) + removed := missingFrom(recorded.ProjectDefines, current.ProjectDefines) + if len(added) == 0 && len(removed) == 0 { + return "" + } + changes := []string{} + if len(added) > 0 { + changes = append(changes, "added: "+strings.Join(added, ", ")) + } + if len(removed) > 0 { + changes = append(changes, "removed: "+strings.Join(removed, ", ")) + } + + return fmt.Sprintf("scripting defines of %s (%s)", current.DefineGroup, strings.Join(changes, "; ")) +} + +// missingFrom lists the values of from that values lacks, in the order of from. +func missingFrom(from []string, values []string) []string { + missing := []string{} + for _, value := range from { + if !slices.Contains(values, value) { + missing = append(missing, value) + } + } + + return missing +} + +// firstDifferentKey names, in sorted order, the first key whose value differs or that only one map +// has. +func firstDifferentKey(recorded map[string]string, current map[string]string) string { + keys := map[string]bool{} + for key := range recorded { + keys[key] = true + } + for key := range current { + keys[key] = true + } + sorted := make([]string, 0, len(keys)) + for key := range keys { + sorted = append(sorted, key) + } + sort.Strings(sorted) + for _, key := range sorted { + recordedValue, inRecorded := recorded[key] + currentValue, inCurrent := current[key] + if inRecorded != inCurrent || recordedValue != currentValue { + return key + } + } + + return "" +} diff --git a/cli/dispatcher/internal/compilecheck/build_settings_test.go b/cli/dispatcher/internal/compilecheck/build_settings_test.go new file mode 100644 index 000000000..7322ecec2 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/build_settings_test.go @@ -0,0 +1,478 @@ +package compilecheck + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" +) + +const buildSettingsStampPath = "Library/Bee/tundra.log.json" + +// buildSettingsProject is a project of two built assemblies, A and B, whose response files carry the +// Standalone platform define and the given project defines. +type buildSettingsProject struct { + root string +} + +// newBuildSettingsProject writes the two assemblies, a text ProjectSettings whose Standalone group +// holds standaloneDefines, and a build-trace file. +func newBuildSettingsProject(t *testing.T, builtDefines []string, standaloneDefines string) buildSettingsProject { + t.Helper() + project := buildSettingsProject{root: t.TempDir()} + for _, name := range []string{"A", "B"} { + writePlanAssembly(t, project.root, name, nil) + project.writeResponseFileDefines(t, name, builtDefines) + } + project.writeProjectSettings(t, standaloneDefines) + writeFileAt(t, filepath.Join(project.root, filepath.FromSlash(buildSettingsStampPath)), "{}") + + return project +} + +// writeResponseFileDefines appends the platform define and the given defines to one response file. +func (project buildSettingsProject) writeResponseFileDefines(t *testing.T, name string, defines []string) { + t.Helper() + path := filepath.Join(project.root, planDagDirectory, name+".rsp") + content, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read %s: %v", path, err) + } + lines := []string{"-define:UNITY_STANDALONE"} + for _, define := range defines { + lines = append(lines, "-define:"+define) + } + writeFileAt(t, path, string(content)+"\n"+joinLines(lines)) +} + +// writeProjectSettings writes a text ProjectSettings with a Standalone and an iPhone group. +func (project buildSettingsProject) writeProjectSettings(t *testing.T, standaloneDefines string) { + t.Helper() + writeFileAt(t, filepath.Join(project.root, filepath.FromSlash(projectSettingsRelativePath)), + "%YAML 1.1\n%TAG !u! tag:unity3d.com,2011:\n--- !u!129 &1\nPlayerSettings:\n"+ + " scriptingDefineSymbols:\n"+ + " Standalone: "+standaloneDefines+"\n"+ + " iPhone: IOS_ONLY\n"+ + " additionalCompilerArguments: {}\n"+ + " scriptingBackend:\n"+ + " Android: 1\n") +} + +// dateProjectSettings sets the modification time of ProjectSettings to the build-trace file's plus +// offset. +func (project buildSettingsProject) dateProjectSettings(t *testing.T, offset time.Duration) { + t.Helper() + stampInfo, err := os.Stat(filepath.Join(project.root, filepath.FromSlash(buildSettingsStampPath))) + if err != nil { + t.Fatalf("failed to stat the build-trace file: %v", err) + } + setModificationTime(t, filepath.Join(project.root, filepath.FromSlash(projectSettingsRelativePath)), + stampInfo.ModTime().Add(offset)) +} + +// write writes a project-relative file. +func (project buildSettingsProject) write(t *testing.T, relativePath string, content string) { + t.Helper() + writeFileAt(t, filepath.Join(project.root, filepath.FromSlash(relativePath)), content) +} + +// advanceStamp moves the build-trace file forward, as a Unity build does. +func (project buildSettingsProject) advanceStamp(t *testing.T) { + t.Helper() + setModificationTime(t, filepath.Join(project.root, filepath.FromSlash(buildSettingsStampPath)), + time.Now().Add(time.Hour)) +} + +// detect runs the build-settings comparison on the project as it is now. +func (project buildSettingsProject) detect(t *testing.T) (BuildSettingsReport, error) { + t.Helper() + graph, err := loadAssemblyGraph(project.root, planDagDirectory) + if err != nil { + t.Fatalf("failed to load the assembly graph: %v", err) + } + assemblyDefinitions, err := IndexAssemblyDefinitions(walkProjectTree(project.root)) + if err != nil { + t.Fatalf("failed to index the assembly definitions: %v", err) + } + + return DetectBuildSettingsChange(project.root, planDagDirectory, graph, assemblyDefinitions) +} + +// mustDetect runs the comparison and fails the test when it refuses. +func (project buildSettingsProject) mustDetect(t *testing.T) BuildSettingsReport { + t.Helper() + report, err := project.detect(t) + if err != nil { + t.Fatalf("expected the run to pass, got error: %v", err) + } + + return report +} + +// recordPath is where the comparison keeps its record. +func (project buildSettingsProject) recordPath() string { + return filepath.Join(project.root, filepath.FromSlash(compileCheckOutputRoot), + filepath.Base(planDagDirectory), buildSettingsRecordFileName) +} + +// assertBuildRequired checks the error is a Unity-build-required refusal containing every fragment. +func assertBuildRequired(t *testing.T, err error, fragments ...string) { + t.Helper() + var required UnityBuildRequiredError + if !errors.As(err, &required) { + t.Fatalf("expected a Unity build to be required, got %v", err) + } + for _, fragment := range fragments { + if !strings.Contains(err.Error(), fragment) { + t.Errorf("expected the refusal to contain %q, got: %v", fragment, err) + } + } +} + +// Verifies a scripting define of the active group that the response files lack is refused, before +// any record exists. +func TestDetectBuildSettingsChangeRefusesADefineTheLastBuildLacks(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO;ADDED_LATER") + project.dateProjectSettings(t, time.Hour) + + report, err := project.detect(t) + + assertBuildRequired(t, err, "ADDED_LATER", "not in the last Unity build") + if !report.StampFound || !report.DefinesChecked { + t.Errorf("report = %+v, want the build-trace file found and the defines checked", report) + } + if _, statErr := os.Stat(project.recordPath()); statErr == nil { + t.Error("expected no record to be written for a refused run") + } +} + +// Verifies a define the response files lack is left to the record when ProjectSettings is older than +// the last build: the build already had it in force and left it out for a reason of its own, such as +// a build profile overriding Player Settings, so refusing it would never clear. +func TestDetectBuildSettingsChangeLeavesADefineOfAnOlderProjectSettingsToTheRecord(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO;OVERRIDDEN_AWAY") + project.dateProjectSettings(t, -time.Hour) + + report := project.mustDetect(t) + + if report.Record != buildSettingsRecordNew || report.DefinesChecked { + t.Errorf("report = %+v, want a new record without the defines checked", report) + } +} + +// Verifies a define in Assets/csc.rsp that the response files lack is not refused by the one-way +// check, and the settings are recorded so a later change shows against the record. +func TestDetectBuildSettingsChangeLeavesCscRspToTheRecord(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + project.write(t, assetsResponseFileRelative, "-define:NOT_IN_THE_BUILD\n") + + report := project.mustDetect(t) + + if report.Record != buildSettingsRecordNew { + t.Errorf("record = %q, want %q", report.Record, buildSettingsRecordNew) + } +} + +// Verifies the csc.rsp of an assembly the last build left out does not refuse the run, whatever +// defines it holds. +func TestDetectBuildSettingsChangeIgnoresTheCscRspOfAnAssemblyTheBuildLeftOut(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + project.write(t, "Assets/Unbuilt/Unbuilt.asmdef", `{"name":"Unbuilt","includePlatforms":["Android"]}`) + project.write(t, "Assets/Unbuilt/csc.rsp", "-define:NOT_IN_THE_BUILD\n") + + project.mustDetect(t) +} + +// Verifies a project without a build-trace file passes and writes no record. +func TestDetectBuildSettingsChangeSkipsTheRecordWithoutABuildStamp(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + if err := os.Remove(filepath.Join(project.root, filepath.FromSlash(buildSettingsStampPath))); err != nil { + t.Fatalf("failed to remove the build-trace file: %v", err) + } + + report := project.mustDetect(t) + + if report.StampFound || report.Record != "" { + t.Errorf("report = %+v, want no stamp and no record", report) + } + if _, statErr := os.Stat(project.recordPath()); statErr == nil { + t.Error("expected no record without a build-trace file") + } +} + +// Verifies the first run after a build records the settings it found, keyed to that build. +func TestDetectBuildSettingsChangeRecordsTheSettingsOfANewBuild(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + + report := project.mustDetect(t) + + if !report.StampFound || report.Record != buildSettingsRecordNew { + t.Fatalf("report = %+v, want a new record", report) + } + recorded, loaded := loadBuildSettingsRecord(project.recordPath()) + if !loaded { + t.Fatal("expected the record to be readable") + } + if recorded.Settings.DefineGroup != "Standalone" || + !reflect.DeepEqual(recorded.Settings.ProjectDefines, []string{"FOO"}) { + t.Errorf("recorded settings = %+v", recorded.Settings) + } + if recorded.Stamp.Path != buildSettingsStampPath { + t.Errorf("stamp path = %q, want %q", recorded.Stamp.Path, buildSettingsStampPath) + } +} + +// Verifies a record of an earlier build is replaced, not compared, once Unity built again, even +// when the settings differ from it. +func TestDetectBuildSettingsChangeReplacesTheRecordAfterAnotherUnityBuild(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + project.mustDetect(t) + project.write(t, packageLockRelativePath, `{"dependencies":{}}`) + project.advanceStamp(t) + + report := project.mustDetect(t) + + if report.Record != buildSettingsRecordReplaced { + t.Errorf("record = %q, want %q", report.Record, buildSettingsRecordReplaced) + } +} + +// Verifies a record this version cannot read - written by another format version, or not JSON at +// all - is replaced rather than compared, even when it carries the current stamp and other settings. +func TestDetectBuildSettingsChangeReplacesARecordItCannotRead(t *testing.T) { + cases := map[string]func(stamp buildStamp) string{ + "another format version": func(stamp buildStamp) string { + encoded, err := json.Marshal(buildSettingsRecord{ + FormatVersion: buildSettingsRecordFormatVersion + 1, + Stamp: stamp, + Settings: buildSettings{ResponseFileDigests: map[string]string{assetsResponseFileRelative: "0000"}}, + }) + if err != nil { + t.Fatalf("failed to encode the record: %v", err) + } + return string(encoded) + }, + "not JSON": func(buildStamp) string { return "{" }, + } + for name, recordOf := range cases { + t.Run(name, func(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + stamp, found := readBuildStamp(project.root) + if !found { + t.Fatal("expected the fixture to have a build-trace file") + } + writeFileAt(t, project.recordPath(), recordOf(stamp)) + + report := project.mustDetect(t) + + if report.Record != buildSettingsRecordNew { + t.Errorf("record = %q, want %q", report.Record, buildSettingsRecordNew) + } + rewritten, loaded := loadBuildSettingsRecord(project.recordPath()) + if !loaded || rewritten.FormatVersion != buildSettingsRecordFormatVersion { + t.Errorf("rewritten record = %+v (loaded %v), want format version %d", + rewritten, loaded, buildSettingsRecordFormatVersion) + } + }) + } +} + +// Verifies settings that match the record of the same build pass and keep the record. +func TestDetectBuildSettingsChangeAcceptsUnchangedSettings(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + project.mustDetect(t) + + report := project.mustDetect(t) + + if report.Record != buildSettingsRecordKept || report.RefusedPart != "" { + t.Errorf("report = %+v, want a kept record and no refusal", report) + } +} + +// Verifies every recorded setting that moved since the recorded build refuses the run and names +// itself. +func TestDetectBuildSettingsChangeRefusesSettingsThatMovedSinceTheRecordedBuild(t *testing.T) { + cases := []struct { + name string + before func(project buildSettingsProject, t *testing.T) + change func(project buildSettingsProject, t *testing.T) + part string + }{ + { + name: "a removed define", + change: func(project buildSettingsProject, t *testing.T) { project.writeProjectSettings(t, "") }, + part: "scripting defines of Standalone (removed: FOO)", + }, + { + name: "the content of Assets/csc.rsp", + before: func(project buildSettingsProject, t *testing.T) { + project.write(t, assetsResponseFileRelative, "-nowarn:CS0168\n") + }, + change: func(project buildSettingsProject, t *testing.T) { + project.write(t, assetsResponseFileRelative, "-nowarn:CS0169\n") + }, + part: assetsResponseFileRelative, + }, + { + name: "a csc.rsp added beside an assembly definition", + change: func(project buildSettingsProject, t *testing.T) { + project.write(t, "Assets/A/csc.rsp", "-nowarn:CS0168\n") + }, + part: "Assets/A/csc.rsp", + }, + { + name: "defineConstraints", + change: func(project buildSettingsProject, t *testing.T) { + project.write(t, "Assets/A/A.asmdef", `{"name":"A","defineConstraints":["FOO"]}`) + }, + part: "defineConstraints or versionDefines of Assets/A/A.asmdef", + }, + { + name: "versionDefines", + change: func(project buildSettingsProject, t *testing.T) { + project.write(t, "Assets/A/A.asmdef", + `{"name":"A","versionDefines":[{"name":"com.example","expression":"1.0","define":"HAS_EXAMPLE"}]}`) + }, + part: "defineConstraints or versionDefines of Assets/A/A.asmdef", + }, + { + name: "packages-lock.json", + change: func(project buildSettingsProject, t *testing.T) { + project.write(t, packageLockRelativePath, `{"dependencies":{"com.example":{}}}`) + }, + part: packageLockRelativePath, + }, + { + name: "manifest.json", + change: func(project buildSettingsProject, t *testing.T) { + project.write(t, packageManifestRelativePath, `{"dependencies":{"com.example":"1.0.0"}}`) + }, + part: packageManifestRelativePath, + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + if testCase.before != nil { + testCase.before(project, t) + } + project.mustDetect(t) + testCase.change(project, t) + + report, err := project.detect(t) + + assertBuildRequired(t, err, testCase.part, "changed since the last Unity build") + if report.RefusedPart != testCase.part || report.Record != buildSettingsRecordKept { + t.Errorf("report = %+v, want a kept record refused for %q", report, testCase.part) + } + }) + } +} + +// Verifies a refusal for a moved setting clears once Unity builds again: the stamp moves forward +// and the next run records the settings as they are. +func TestDetectBuildSettingsChangeClearsARefusalOnceUnityBuildsAgain(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO") + project.mustDetect(t) + project.write(t, assetsResponseFileRelative, "-nowarn:CS0168\n") + if _, err := project.detect(t); err == nil { + t.Fatal("expected the changed csc.rsp to be refused before Unity builds again") + } + project.advanceStamp(t) + + report := project.mustDetect(t) + + if report.Record != buildSettingsRecordReplaced { + t.Errorf("record = %q, want %q", report.Record, buildSettingsRecordReplaced) + } + project.mustDetect(t) +} + +// Verifies a binary ProjectSettings leaves the scripting defines out of the settings, so only the +// other settings are compared. +func TestReadBuildSettingsLeavesTheDefinesOutOfABinaryProjectSettings(t *testing.T) { + project := newBuildSettingsProject(t, []string{}, "") + project.write(t, projectSettingsRelativePath, "\x00\x00\x00\x00binary FOO;ADDED_LATER") + + report := project.mustDetect(t) + recorded, _ := loadBuildSettingsRecord(project.recordPath()) + + if report.Record != buildSettingsRecordNew || recorded.Settings.DefineGroup != "" || + recorded.Settings.ProjectDefines != nil { + t.Errorf("report = %+v, settings = %+v, want no define group and no defines", report, recorded.Settings) + } +} + +// Verifies the ProjectSettings group is picked from the platform define of the response files, and +// a Dedicated Server build or an unknown platform leaves it undecided. +func TestReadProjectDefinesPicksTheGroupOfTheResponseFilePlatform(t *testing.T) { + cases := []struct { + name string + defines []string + group string + }{ + {"Standalone", []string{"UNITY_STANDALONE"}, "Standalone"}, + {"iPhone", []string{"UNITY_IOS"}, "iPhone"}, + {"Dedicated Server", []string{"UNITY_STANDALONE", "UNITY_SERVER"}, ""}, + {"unknown", []string{"UNITY_SOMETHING_ELSE"}, ""}, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + graph := assemblyGraph{ + names: []string{"A"}, + byName: map[string]ResponseFile{"A": {AssemblyName: "A", Defines: testCase.defines}}, + } + + if group := defineGroupOf(graph); group != testCase.group { + t.Errorf("group = %q, want %q", group, testCase.group) + } + }) + } +} + +// Verifies a scripting define value is split on semicolons, commas and spaces alike. +func TestReadProjectDefinesSplitsOnEverySeparator(t *testing.T) { + project := newBuildSettingsProject(t, []string{}, "A;B, C D") + + _, defines, err := readProjectDefines(project.root, "Standalone") + if err != nil { + t.Fatalf("failed to read the defines: %v", err) + } + if !reflect.DeepEqual(defines, []string{"A", "B", "C", "D"}) { + t.Errorf("defines = %v, want [A B C D]", defines) + } +} + +// Verifies the scan of one group's defines ends where the next map of the same depth begins, so a +// group with no defines does not take another map's value for its own. +func TestReadProjectDefinesStopsAtTheNextMap(t *testing.T) { + project := newBuildSettingsProject(t, []string{}, "") + + content, err := os.ReadFile(filepath.Join(project.root, filepath.FromSlash(projectSettingsRelativePath))) + if err != nil { + t.Fatalf("failed to read the fixture: %v", err) + } + if value, found := scriptingDefinesValue(content, "Android"); found { + t.Errorf("Android defines = %q, want none found", value) + } +} + +// Verifies an empty scriptingDefineSymbols map gives the group no defines rather than an unknown +// group. +func TestReadProjectDefinesReadsAnEmptyMap(t *testing.T) { + project := newBuildSettingsProject(t, []string{}, "") + project.write(t, projectSettingsRelativePath, + "%YAML 1.1\nPlayerSettings:\n scriptingDefineSymbols: {}\n additionalCompilerArguments: {}\n") + + group, defines, err := readProjectDefines(project.root, "Standalone") + if err != nil { + t.Fatalf("failed to read the defines: %v", err) + } + if group != "Standalone" || defines == nil || len(defines) != 0 { + t.Errorf("group = %q, defines = %#v, want Standalone with an empty list", group, defines) + } +} diff --git a/cli/dispatcher/internal/compilecheck/build_settings_wiring_test.go b/cli/dispatcher/internal/compilecheck/build_settings_wiring_test.go new file mode 100644 index 000000000..b5f49f89c --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/build_settings_wiring_test.go @@ -0,0 +1,59 @@ +package compilecheck + +import ( + "context" + "path/filepath" + "testing" + "time" +) + +// Verifies a plan refused for a define the build lacks carries the build-settings report on its +// error, so the run's log can say what the comparison found. +func TestBuildCompilePlanCarriesTheBuildSettingsReportOfARefusal(t *testing.T) { + project := newBuildSettingsProject(t, []string{"FOO"}, "FOO;ADDED_LATER") + project.dateProjectSettings(t, time.Hour) + + _, err := BuildCompilePlan(project.root, planDagDirectory, false) + + assertBuildRequired(t, err, "ADDED_LATER") + report, found := BuildSettingsReportOf(err) + if !found || !report.StampFound || !report.DefinesChecked { + t.Errorf("report = %+v (found %v), want the report of a run that found the build-trace file", report, found) + } +} + +// writeBuildStamp writes the build-trace file the build-settings record is keyed to. +func writeBuildStamp(t *testing.T, projectRoot string) { + t.Helper() + writeFileAt(t, filepath.Join(projectRoot, filepath.FromSlash(buildSettingsStampPath)), "{}") +} + +// Verifies a plan that builds carries the build-settings report of the comparison it ran. +func TestBuildCompilePlanCarriesTheBuildSettingsReport(t *testing.T) { + projectRoot := newPlanProject(t) + writeBuildStamp(t, projectRoot) + + plan, err := BuildCompilePlan(projectRoot, planDagDirectory, false) + if err != nil { + t.Fatalf("expected the plan to build, got error: %v", err) + } + + if plan.BuildSettings.Record != buildSettingsRecordNew { + t.Errorf("record = %q, want %q", plan.BuildSettings.Record, buildSettingsRecordNew) + } +} + +// Verifies a run reports the build-settings comparison of its plan in its result. +func TestRunReportsTheBuildSettingsComparison(t *testing.T) { + projectRoot := newPlanProject(t) + writeBuildStamp(t, projectRoot) + + result, err := Run(context.Background(), Options{ProjectRoot: projectRoot, EditorExecutablePath: writeFakeEditor(t)}) + if err != nil { + t.Fatalf("Run failed: %v", err) + } + + if result.BuildSettings.Record != buildSettingsRecordNew { + t.Errorf("record = %q, want %q", result.BuildSettings.Record, buildSettingsRecordNew) + } +} diff --git a/cli/dispatcher/internal/compilecheck/plan.go b/cli/dispatcher/internal/compilecheck/plan.go index a20a86b37..a0337ad65 100644 --- a/cli/dispatcher/internal/compilecheck/plan.go +++ b/cli/dispatcher/internal/compilecheck/plan.go @@ -43,6 +43,8 @@ type BuildPlan struct { // All says the caller asked for every assembly to be compiled. Such a run reuses nothing, which // is what makes --all the way out of a reuse that got something wrong. All bool + // BuildSettings says what comparing the build settings with the last Unity build found. + BuildSettings BuildSettingsReport } // assemblyGraph holds every response file in the dag together with the edges between them. @@ -62,7 +64,7 @@ func BuildCompilePlan(projectRoot string, dagDir string, all bool) (BuildPlan, e } graph := inputs.graph - reasons, err := selectChangedAssemblies(projectRoot, dagDir, inputs, all) + reasons, buildSettings, err := selectChangedAssemblies(projectRoot, dagDir, inputs, all) if err != nil { return BuildPlan{}, err } @@ -70,7 +72,7 @@ func BuildCompilePlan(projectRoot string, dagDir string, all bool) (BuildPlan, e order, err := topologicalOrder(graph, reasons) if err != nil { - return BuildPlan{}, err + return BuildPlan{}, withBuildSettingsReport(buildSettings, err) } units := make([]CompileUnit, 0, len(order)) @@ -85,11 +87,12 @@ func BuildCompilePlan(projectRoot string, dagDir string, all bool) (BuildPlan, e } return BuildPlan{ - DagDir: dagDir, - OutputDir: filepath.Join(filepath.FromSlash(compileCheckOutputRoot), filepath.Base(dagDir)), - Units: units, - Skipped: len(graph.names) - len(units), - All: all, + DagDir: dagDir, + OutputDir: filepath.Join(filepath.FromSlash(compileCheckOutputRoot), filepath.Base(dagDir)), + Units: units, + Skipped: len(graph.names) - len(units), + All: all, + BuildSettings: buildSettings, }, nil } @@ -172,18 +175,36 @@ func listResponseFiles(dagDirectoryPath string) ([]string, error) { // below read the same walk. func selectChangedAssemblies( projectRoot string, dagDir string, inputs planInputs, all bool, -) (map[string]string, error) { +) (map[string]string, BuildSettingsReport, error) { graph := inputs.graph - tree := inputs.tree - assemblyDefinitions, err := IndexAssemblyDefinitions(tree) + assemblyDefinitions, err := IndexAssemblyDefinitions(inputs.tree) if err != nil { - return nil, err + return nil, BuildSettingsReport{}, err } if setErr := DetectAssemblySetChange(projectRoot, dagDir, graph, assemblyDefinitions); setErr != nil { - return nil, setErr + return nil, BuildSettingsReport{}, setErr } + buildSettings, settingsErr := DetectBuildSettingsChange(projectRoot, dagDir, graph, assemblyDefinitions) + if settingsErr != nil { + return nil, buildSettings, withBuildSettingsReport(buildSettings, settingsErr) + } + + reasons, err := selectAssembliesByOwnership(projectRoot, dagDir, inputs, assemblyDefinitions, all) + + return reasons, buildSettings, withBuildSettingsReport(buildSettings, err) +} + +// selectAssembliesByOwnership checks which assemblies own which sources now, refuses a run whose +// ownership moved, and returns why each changed assembly needs compiling. +func selectAssembliesByOwnership( + projectRoot string, dagDir string, inputs planInputs, + assemblyDefinitions map[string]AssemblyDefinition, all bool, +) (map[string]string, error) { + graph := inputs.graph + tree := inputs.tree + // Why the .asmref files are indexed here: both the reference check and every source rebuild need // them. references, referenceIndexErr := IndexAssemblyReferences(tree) diff --git a/cli/dispatcher/internal/compilecheck/resultstore/baseline.go b/cli/dispatcher/internal/compilecheck/resultstore/baseline.go index 964266dcf..6df405633 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/baseline.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/baseline.go @@ -233,7 +233,7 @@ func writeBaselineRecord(directoryPath string, assembly string, record baselineR return fmt.Errorf("failed to record the baseline of %s: %w", assembly, err) } - return writeFileAtomically(directoryPath, baselineRecordPath(directoryPath, assembly), content) + return WriteFileAtomically(directoryPath, baselineRecordPath(directoryPath, assembly), content) } // baselineRecordPath names where one assembly's baseline lives. diff --git a/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go b/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go index 8d39cfa3a..30206e6e5 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/compile_result.go @@ -203,7 +203,7 @@ func (s Store[D]) RecordCompile( return fmt.Errorf("failed to record the result of %s: %w", assembly, err) } - return writeFileAtomically(s.directoryPath(), manifestPath(s.directoryPath(), assembly), content) + return WriteFileAtomically(s.directoryPath(), manifestPath(s.directoryPath(), assembly), content) } // hashOutputs hashes the named files a compile wrote in directoryPath. diff --git a/cli/dispatcher/internal/compilecheck/resultstore/fast_check.go b/cli/dispatcher/internal/compilecheck/resultstore/fast_check.go index e31bed27b..7ac2cb045 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/fast_check.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/fast_check.go @@ -102,7 +102,7 @@ func (s Store[D]) RecordFastCheck( return fmt.Errorf("failed to record the changed-files check of %s: %w", assembly, err) } - return writeFileAtomically(s.directoryPath(), fastResultPath(s.directoryPath(), assembly), content) + return WriteFileAtomically(s.directoryPath(), fastResultPath(s.directoryPath(), assembly), content) } // missingFastCheckKey names the first of the keys a check is looked up by that is empty, or returns diff --git a/cli/dispatcher/internal/compilecheck/resultstore/store.go b/cli/dispatcher/internal/compilecheck/resultstore/store.go index ddef83305..99621edd8 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/store.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/store.go @@ -90,8 +90,8 @@ func (s Store[D]) Forget(assembly string) error { return nil } -// writeFileAtomically writes content to path through a temporary file in directoryPath. -func writeFileAtomically(directoryPath string, path string, content []byte) error { +// WriteFileAtomically writes content to path through a temporary file in directoryPath. +func WriteFileAtomically(directoryPath string, path string, content []byte) error { // Why the temporary name is unique per writer: two runs of the same project writing the same // file at once must not rename each other's half-written file into place. temporary, err := os.CreateTemp(directoryPath, filepath.Base(path)+".*"+resultManifestTempSuffix) diff --git a/cli/dispatcher/internal/compilecheck/resultstore/surface.go b/cli/dispatcher/internal/compilecheck/resultstore/surface.go index 7d40df568..bc1e5272d 100644 --- a/cli/dispatcher/internal/compilecheck/resultstore/surface.go +++ b/cli/dispatcher/internal/compilecheck/resultstore/surface.go @@ -93,5 +93,5 @@ func (s Store[D]) RecordSurfaceVerdict(assembly string, inputs SurfaceInputs, ve return fmt.Errorf("failed to encode the surface verdict of %s: %w", assembly, err) } - return writeFileAtomically(s.directoryPath(), surfaceRecordPath(s.directoryPath(), assembly), content) + return WriteFileAtomically(s.directoryPath(), surfaceRecordPath(s.directoryPath(), assembly), content) } diff --git a/cli/dispatcher/internal/compilecheck/session.go b/cli/dispatcher/internal/compilecheck/session.go index b01acbda2..55a00f118 100644 --- a/cli/dispatcher/internal/compilecheck/session.go +++ b/cli/dispatcher/internal/compilecheck/session.go @@ -50,6 +50,8 @@ type Result struct { NameSkips []NameSkip // NameIndex says what reading the names of the assemblies with references in the plan took. NameIndex NameIndexReport + // BuildSettings says what comparing the build settings with the last Unity build found. + BuildSettings BuildSettingsReport } // Run compiles the assemblies that need checking and collects their diagnostics. @@ -106,10 +108,11 @@ 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, - NameSkips: outcome.NameSkips, - NameIndex: outcome.NameIndex, + Skipped: plan.Skipped + outcome.Skipped, + Blocked: outcome.Blocked, + NameSkips: outcome.NameSkips, + NameIndex: outcome.NameIndex, + BuildSettings: plan.BuildSettings, }, nil } diff --git a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go index 2af733511..e48236a6d 100644 --- a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go +++ b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go @@ -62,6 +62,9 @@ func (logRun compileCheckVibeLog) failed(err error) { context["error_code"] = clierrors.ClassifyError(err, clierrors.ErrorContext{ Command: clicore.CompileCheckCommandName, ProjectRoot: logRun.projectRoot, }).ErrorCode + if report, found := compilecheck.BuildSettingsReportOf(err); found { + context["build_settings"] = compileCheckVibeBuildSettings(report) + } logRun.write("ERROR", compileCheckVibeCompleteOperation, "compile-check failed.", context) } @@ -90,6 +93,7 @@ func (logRun compileCheckVibeLog) completed(response compileCheckResponse, resul context["units"] = compileCheckVibeUnits(result.Units) context["surface_checks"] = compileCheckVibeSurfaceChecks(result.Units) addCompileCheckVibeNameNarrowing(context, result) + context["build_settings"] = compileCheckVibeBuildSettings(result.BuildSettings) logRun.write(level, compileCheckVibeCompleteOperation, "compile-check finished.", context) } @@ -114,6 +118,17 @@ func (logRun compileCheckVibeLog) write(level string, operation string, message }) } +// compileCheckVibeBuildSettings says whether the last Unity build left a build-trace file, what +// happened to the build-settings record, and which setting a refusal was for. +func compileCheckVibeBuildSettings(report compilecheck.BuildSettingsReport) map[string]any { + return map[string]any{ + "stamp_found": report.StampFound, + "record": report.Record, + "refused_part": report.RefusedPart, + "defines_checked": report.DefinesChecked, + } +} + // compileCheckVibeFastChecks lists each fast check by assembly, whether it was replayed, and how // many files it covered. func compileCheckVibeFastChecks(checks []compileCheckFastCheck) []map[string]any { 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 d1bf70488..2d46b9ae7 100644 --- a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go +++ b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go @@ -314,3 +314,54 @@ func TestCompileCheckVibeLogLeavesOutNameNarrowingWhenNothingWasRead(t *testing. } } } + +// Verifies a completed run logs whether the last Unity build left a build-trace file and what +// happened to the build-settings record. +func TestCompileCheckVibeLogRecordsTheBuildSettingsOfACompletedRun(t *testing.T) { + t.Setenv(vibelog.CLIVibeLogEnvName, "1") + projectRoot := t.TempDir() + result := compilecheck.Result{ + BuildSettings: compilecheck.BuildSettingsReport{StampFound: true, Record: "new", DefinesChecked: true}, + } + + logRun := startCompileCheckVibeLog(projectRoot, compileCheckOptions{}) + logRun.completed(buildCompileCheckResponse(result, projectRoot), result) + + complete := assertCompileCheckVibeRun(t, readCompileCheckVibeLog(t, projectRoot)) + assertVibeContextJSON(t, complete, "build_settings", + `{"defines_checked":true,"record":"new","refused_part":"","stamp_found":true}`) +} + +// Verifies a run refused for a build setting that moved logs the refused setting on the failed +// complete entry, and the error still classifies as a Unity build being required. +func TestCompileCheckVibeLogRecordsTheBuildSettingOfARefusedRun(t *testing.T) { + t.Setenv(vibelog.CLIVibeLogEnvName, "1") + projectRoot := t.TempDir() + refusal := compilecheck.BuildSettingsError{ + Report: compilecheck.BuildSettingsReport{StampFound: true, Record: "kept", RefusedPart: "Assets/csc.rsp"}, + Err: compilecheck.UnityBuildRequiredError{Reason: "Assets/csc.rsp changed since the last Unity build"}, + } + + logRun := startCompileCheckVibeLog(projectRoot, compileCheckOptions{}) + logRun.failed(refusal) + + complete := assertCompileCheckVibeRun(t, readCompileCheckVibeLog(t, projectRoot)) + assertVibeContextJSON(t, complete, "build_settings", + `{"defines_checked":false,"record":"kept","refused_part":"Assets/csc.rsp","stamp_found":true}`) + assertVibeContextJSON(t, complete, "error_code", `"`+clierrors.ErrorCodeCompileCheckUnityBuildRequired+`"`) +} + +// Verifies a run that failed before the build settings were compared logs no build_settings field. +func TestCompileCheckVibeLogLeavesOutBuildSettingsOfARunThatFailedEarlier(t *testing.T) { + t.Setenv(vibelog.CLIVibeLogEnvName, "1") + projectRoot := t.TempDir() + + logRun := startCompileCheckVibeLog(projectRoot, compileCheckOptions{}) + logRun.failed(errors.New("no Bee build found")) + + complete := assertCompileCheckVibeRun(t, readCompileCheckVibeLog(t, projectRoot)) + if _, found := complete.Context["build_settings"]; found { + t.Errorf("expected no build_settings on a run that failed before comparing, got %v", + complete.Context["build_settings"]) + } +} diff --git a/docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md b/docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md index d01e0f5be..d4e015c38 100644 --- a/docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md +++ b/docs/adr/0018-compile-check-is-its-own-command-and-is-shown.md @@ -81,7 +81,10 @@ These are documented and not detected; after any of them, run `uloop compile`: - (f) an analyzer config Unity passes with `/analyzerconfig:` that was added since the last build; - (g) scripting defines are not compared in a project whose active build target is Dedicated Server (the response files carry `UNITY_SERVER`), because which settings group Unity reads for it - cannot be determined. + cannot be determined; +- (h) a Unity 6 build profile that overrides Player Settings or carries its own scripting defines: + only the global ProjectSettings defines are compared, and a global define the build lacks is + refused only when `ProjectSettings.asset` was saved after the last build. ## Consequences diff --git a/docs/compile-check.md b/docs/compile-check.md index 7b4b7be1f..f0e3e7e6e 100644 --- a/docs/compile-check.md +++ b/docs/compile-check.md @@ -121,9 +121,17 @@ references, scripting defines and analyzers. `compile-check` replays those respo reference that was added to one, and a project reference it lost (a reference in the response file that the `.asmdef` no longer declares and Unity does not add on its own: `UnityEngine.UI`, `UnityEditor.UI`, `UnityEngine.TestRunner`, `UnityEditor.TestRunner`), invalidate the recorded - build. The run then stops with - `COMPILE_CHECK_UNITY_BUILD_REQUIRED` rather than reporting diagnostics for a configuration the - project no longer has. + build. The run then stops with `COMPILE_CHECK_UNITY_BUILD_REQUIRED` rather than reporting + diagnostics for a configuration the project no longer has. + + The build settings the response files cannot be read back into - scripting defines, + `csc.rsp`, `defineConstraints` and `versionDefines`, and the package manifest and lock file - + are recorded by the first run after a Unity build in + `Library/uloop/compile-check//build-settings.json`, keyed to the build-trace file Bee + rewrites on every build. A later run against the same build refuses any of them that moved; a + run after another Unity build replaces the record instead. A scripting define of the active + build target group that the response files lack is refused whenever `ProjectSettings.asset` was + saved after the last build, or there is no build-trace file. 5. **Compile in dependency order** and parse the compiler's diagnostics into JSON. An assembly starts once every assembly it references that this run also compiles has finished, so independent assemblies compile at the same time. An assembly selected for its own sources, or a @@ -353,19 +361,34 @@ happened, and how long each assembly took. `docs/vibe-logs.md` lists their field any source its response file recorded (it was moved into another assembly's folder), an `.asmref` whose folder holds sources the assembly it names does not record (it was added or moved), and a recorded source no `.asmref` attaches to its assembly any more (one was removed or retargeted). - Those cases — and only those — stop the run with `COMPILE_CHECK_UNITY_BUILD_REQUIRED` and ask for - `uloop compile`. + So do the build settings described below. Those cases — and only those — stop the run with + `COMPILE_CHECK_UNITY_BUILD_REQUIRED` and ask for `uloop compile`. Every other edit listed below is invisible to the check: the run proceeds and silently reuses what the last build recorded. -- **`defineConstraints` and `versionDefines` changes are not detected**, because they depend on - scripting defines and package versions that can only be evaluated inside the Editor. -- **Changed scripting defines are not detected**; the defines recorded in the last build are reused. +- **Build settings are compared against a record keyed to the last Unity build.** The scripting + defines of the active build target group (from `ProjectSettings/ProjectSettings.asset`), every + `csc.rsp` Unity may read (`Assets/csc.rsp` and the one beside each `.asmdef` outside the package + cache), the `defineConstraints` and `versionDefines` of each such `.asmdef`, + `Packages/packages-lock.json` and `Packages/manifest.json` are recorded by the first run after a + Unity build, keyed to the build-trace file Bee rewrites on every build + (`Library/Bee/tundra.log.json`, `TundraBuildState.state` or `fullprofile.json`, whichever is + newest). A later run against the same build refuses any of them that moved. A scripting define + that the response files lack is refused, record or not, whenever `ProjectSettings.asset` was + saved after the last build or there is no build-trace file. What stays invisible: + - on a Unity version whose `Library/Bee` has none of the build-trace files, only an added + scripting define is seen - not a removed one, and not the other recorded settings; + - a change made before the record exists (before the first run after that build), other than an + added scripting define; + - the scripting defines of a project whose active build target is Dedicated Server (the + response files carry `UNITY_SERVER`), because which settings group Unity reads for it cannot be + determined, and those of a `ProjectSettings.asset` saved in binary; + - an analyzer config added since the last build that Unity passes with `/analyzerconfig:`; one a + response file already names is read by content and invalidates a reused result; + - a Unity 6 build profile that overrides Player Settings or carries its own scripting defines: + only the global ProjectSettings defines are compared, and a global define the build lacks is + refused only when `ProjectSettings.asset` was saved after the last build. - **The predefined assemblies** (`Assembly-CSharp` and friends) keep the source list of the last build, so a brand-new `.cs` file outside any `.asmdef` is not compiled until Unity imports it. -- **An `.editorconfig` or global analyzer config that is not named by a response file does not - 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 - @@ -404,8 +427,8 @@ headless run imports the project, writes the response files and exits. `uloop launch` is the same first build with the Editor window open; use whichever fits. The run takes as long as the project's initial import (about a minute for a 100-assembly project, longer for a large one). Once `Library` exists, compile-check runs without the Editor; it refuses again only -when it detects an `.asmdef` change. A scripting define change is not detected (see Limitations), so -run `uloop compile` yourself after one. The same applies on CI: restore `Library` from a cache or +when it detects an `.asmdef` change or a build setting that moved (see Limitations for what stays +unseen). The same applies on CI: restore `Library` from a cache or run this headless build once per job before calling compile-check. **`COMPILE_CHECK_UNITY_BUILD_REQUIRED`: an assembly definition no longer matches the last build** @@ -423,6 +446,15 @@ An `.asmdef` edit that is not one of those does **not** raise this error — see goes undetected. In that case the run succeeds against the previous configuration, so run `uloop compile` yourself after such an edit rather than trusting a clean result. +**`COMPILE_CHECK_UNITY_BUILD_REQUIRED`: a build setting changed since the last Unity build** +The message reads ` changed since the last Unity build` - for example +`scripting defines of Standalone (removed: FOO)`, `Assets/csc.rsp`, +`defineConstraints or versionDefines of Assets/X/X.asmdef` or `Packages/packages-lock.json` - or +`scripting define is not in the last Unity build`. The response files were written with the +settings of the last Unity build, so compiling against them would check a configuration the project +no longer has. Run `uloop compile` once; the build moves the build-trace file forward, the next run +records the settings as they are now, and `compile-check` works again. + **`COMPILE_CHECK_UNITY_BUILD_REQUIRED`: cannot tell which Bee build to check** Both a debug and a release dag exist and neither `tundra.log.json` nor `Library/EditorOnlyScriptingSettings.json` could be read. Building once from the Editor restores the diff --git a/docs/vibe-logs.md b/docs/vibe-logs.md index e97b7ca49..f7c934bea 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, `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`, `sources_read`: what reading the names of the assemblies with references in the plan took, and whether the run read their sources - it does only once it compiles an assembly, has the worker check changed files, or a verdict needs the names; only when the plan has an assembly with references) | +| `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`, and `build_settings` when the run got as far as comparing the build settings; 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`, `sources_read`: what reading the names of the assemblies with references in the plan took, and whether the run read their sources - it does only once it compiles an assembly, has the worker check changed files, or a verdict needs the names; only when the plan has an assembly with references), and `build_settings` (`stamp_found`: a build-trace file of the last Unity build exists; `record`: `new` when the run recorded the settings of a build for the first time, `replaced` when it recorded them after another Unity build, `kept` when it compared them against the record of the same build, empty when there was no build-trace file; `refused_part`: the setting a refusal was for, empty otherwise; `defines_checked`: the run looked for a scripting define the build lacks, which it does only when `ProjectSettings.asset` was saved after the last build or there is no build-trace file) | - 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 From 6015733ae4e4fdd0c45a6ff0c92701d6f24947d6 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Sat, 17 Oct 2026 04:19:54 +0900 Subject: [PATCH 04/11] feat: compile-check keeps the records of skipped dependents and compiles one with no saved generator output (#3358) --- .../internal/compilecheck/compiler.go | 19 +++ .../compilecheck/compiler_generated_test.go | 70 +++++++++ .../internal/compilecheck/name_index.go | 67 ++++++-- .../compilecheck/name_index_fifo_test.go | 2 +- .../internal/compilecheck/name_index_test.go | 6 +- .../internal/compilecheck/scheduler.go | 16 +- .../compilecheck/scheduler_cache_test.go | 10 +- .../compilecheck/scheduler_generated_test.go | 144 ++++++++++++++++++ .../compilecheck/scheduler_names_test.go | 30 ++++ .../compilecheck/scheduler_surface_test.go | 52 +++++++ .../internal/compilecheck/scheduler_test.go | 8 +- .../dispatcher/compile_check_vibe_log.go | 17 ++- .../dispatcher/compile_check_vibe_log_test.go | 17 ++- ...s-that-name-nothing-a-reference-changed.md | 17 ++- docs/compile-check.md | 34 ++--- docs/vibe-logs.md | 2 +- 16 files changed, 445 insertions(+), 66 deletions(-) create mode 100644 cli/dispatcher/internal/compilecheck/compiler_generated_test.go create mode 100644 cli/dispatcher/internal/compilecheck/scheduler_generated_test.go diff --git a/cli/dispatcher/internal/compilecheck/compiler.go b/cli/dispatcher/internal/compilecheck/compiler.go index 762823240..4bc5e204a 100644 --- a/cli/dispatcher/internal/compilecheck/compiler.go +++ b/cli/dispatcher/internal/compilecheck/compiler.go @@ -161,6 +161,25 @@ func (c Compiler) CompileUnit( return UnitResult{}, fmt.Errorf("failed to create %s: %w", generatedPath, err) } + result, err := c.invokeCompiler(ctx, plan, unit) + // Why the generator output goes when csc did not finish: name narrowing takes the directory as + // the output of a finished compile, even an empty one, so a directory left by a compile that + // never ran would let a later run narrow the unit by its hand-written sources alone. A dispatcher + // killed mid-compile still leaves one, until the unit compiles again and Forget removes it. + if err != nil || result.RawOutput != "" { + if removeErr := os.RemoveAll(generatedPath); removeErr != nil { + return result, errors.Join(err, fmt.Errorf("failed to remove %s: %w", generatedPath, removeErr)) + } + } + + return result, err +} + +// invokeCompiler writes the unit's response file and runs csc on it, once its outputs are cleared +// and its generator output directory exists. +func (c Compiler) invokeCompiler( + ctx context.Context, plan BuildPlan, unit CompileUnit, +) (UnitResult, error) { // Why the generator output is saved: a later fast check reads it instead of running the generators. responseFilePath, err := writeRewrittenResponseFile(c.ProjectRoot, plan, unit, unit.Assembly.AssemblyName+responseFileExtension, diff --git a/cli/dispatcher/internal/compilecheck/compiler_generated_test.go b/cli/dispatcher/internal/compilecheck/compiler_generated_test.go new file mode 100644 index 000000000..40fcc1c16 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/compiler_generated_test.go @@ -0,0 +1,70 @@ +package compilecheck + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "testing" + "time" +) + +// compileSecondUnitRunning compiles the second unit of the compiler plan with the given runner and +// reports whether its generator output directory is there afterwards. +func compileSecondUnitRunning(t *testing.T, run CommandRunner) (UnitResult, error, bool) { + t.Helper() + projectRoot := t.TempDir() + plan := newCompilerPlan() + compiler := Compiler{ + Paths: EditorCompilerPaths{DotnetHostPath: "/dotnet", CompilerDllPath: "/csc.dll"}, + ProjectRoot: projectRoot, + Timeout: time.Minute, + Run: run, + } + writeFileAt(t, filepath.Join(projectRoot, plan.OutputDir, "A.ref.dll"), "") + + result, err := compiler.CompileUnit(context.Background(), plan, plan.Units[1]) + _, statErr := os.Stat(filepath.Join(projectRoot, compiler.records(plan).GeneratedDirectory("B"))) + + return result, err, statErr == nil +} + +// Verifies a compile whose compiler never ran leaves no generator output directory, so a later run +// does not take it for the saved output of a finished compile. +func TestCompileUnitLeavesNoGeneratorOutputWhenTheCompilerDoesNotRun(t *testing.T) { + failToStart := func(context.Context, *exec.Cmd) (string, string, int, error) { + return "", "", -1, errors.New("cannot start") + } + + _, err, kept := compileSecondUnitRunning(t, failToStart) + + if err == nil { + t.Fatal("expected the failure to start the compiler to be reported") + } + if kept { + t.Error("expected the generator output directory to be removed") + } +} + +// Verifies a compile that failed without an error diagnostic leaves no generator output directory +// either, while one that finished with compile errors keeps it. +func TestCompileUnitKeepsGeneratorOutputOnlyWhenTheCompilerFinished(t *testing.T) { + var captured exec.Cmd + result, err, kept := compileSecondUnitRunning(t, stubRunner("Unhandled exception.", 1, &captured)) + if err != nil || result.RawOutput == "" { + t.Fatalf("expected a raw-output failure, got result %+v and error %v", result, err) + } + if kept { + t.Error("expected a compile that did not finish to remove its generator output directory") + } + + _, err, kept = compileSecondUnitRunning(t, + stubRunner("Assets/B/B.cs(1,1): error CS0029: Cannot convert", 1, &captured)) + if err != nil { + t.Fatalf("expected compile errors as diagnostics, got error %v", err) + } + if !kept { + t.Error("expected a compile that finished with errors to keep its generator output directory") + } +} diff --git a/cli/dispatcher/internal/compilecheck/name_index.go b/cli/dispatcher/internal/compilecheck/name_index.go index c1111b0ce..0347b3fe5 100644 --- a/cli/dispatcher/internal/compilecheck/name_index.go +++ b/cli/dispatcher/internal/compilecheck/name_index.go @@ -20,11 +20,18 @@ import ( // names a changed constructor. const targetTypedNewToken = "new(" +// noSavedGeneratorOutputReason says a unit compiled on a reference's named change without being +// narrowed by names, because nothing tells what its generators would write. +const noSavedGeneratorOutputReason = "no saved generator output" + // 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 + // Reason says why the change was not narrowed by names when it was told by names; today only + // "no saved generator output". + Reason string } // NameSkip names a unit left out although a reference it reads changed its surface, because the unit @@ -43,6 +50,9 @@ type NameIndexReport struct { Units int WithGenerated int Unreadable int + // WithoutGenerated counts the units compiled on a named change without being narrowed by names, + // because they have analyzers and no saved generator output. + WithoutGenerated int // SourcesRead says whether the run needed the sources, by compiling a unit, by having the worker // check its changed files, or by asking a verdict for names. SourcesRead bool @@ -268,21 +278,21 @@ func (index *nameIndex) keep(results []unitIdentifiers) { } // lookup reads the sources when nothing has yet, waits for them and returns the identifiers of one -// unit, or an error when it has none. -func (index *nameIndex) lookup(position int) (identifierSet, error) { +// unit and whether its saved generator output was among them, or an error when it has none. +func (index *nameIndex) lookup(position int) (identifierSet, bool, error) { if !index.started { - return nil, errors.New("the names of the plan were never read") + return nil, false, errors.New("the names of the plan were never read") } index.readSources() <-index.done if !index.indexed[position] { - return nil, errors.New("the names of this unit were not read") + return nil, false, errors.New("the names of this unit were not read") } if index.errs[position] != nil { - return nil, index.errs[position] + return nil, false, index.errs[position] } - return index.sets[position], nil + return index.sets[position], index.withGenerated[position], nil } // wait returns once the background read of the sources is over, so nothing outlives the run that @@ -352,8 +362,9 @@ type referenceEffect struct { 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 + named bool + name string + reason string // why a named change touched the unit without a name it writes } // changedReference is a reference that touched a unit, by its position in the plan, with the first @@ -361,6 +372,7 @@ type referenceEffect struct { type changedReference struct { reference int name string + reason string } // effectsOf decides, for every unit reading the unit at index, whether its verdict touches that unit: @@ -383,8 +395,14 @@ func (run *schedulerRun) effectsOf(index int, verdict SurfaceVerdict) []referenc return effects } +// namedEffect decides whether a change told by names touches one unit: when the unit writes one of +// the names, or when it has analyzers and no saved generator output. +// Why the names are matched first: a name the unit writes tells a reader more than the reason does. +// Why a unit without saved generator output compiles: its generators may write a changed name its +// own sources never do, and nothing on disk says what they write. A compile saves that output, even +// empty, so this happens once per unit. Without analyzers there are no generators to miss. func (run *schedulerRun) namedEffect(dependent int, changedNames []string) referenceEffect { - identifiers, err := run.names.lookup(dependent) + identifiers, withGenerated, err := run.names.lookup(dependent) if err != nil { return referenceEffect{dependent: dependent, touches: true} } @@ -393,10 +411,40 @@ func (run *schedulerRun) namedEffect(dependent int, changedNames []string) refer return referenceEffect{dependent: dependent, touches: true, named: true, name: name} } } + if !withGenerated && len(run.plan.Units[dependent].Assembly.Analyzers) > 0 { + return referenceEffect{dependent: dependent, touches: true, reason: noSavedGeneratorOutputReason} + } return referenceEffect{dependent: dependent, named: true} } +// nameIndexReport is what reading the names took, with the units compiled for want of saved +// generator output. +func (run *schedulerRun) nameIndexReport() NameIndexReport { + report := run.names.report() + report.WithoutGenerated = run.withoutGeneratedCount() + + return report +} + +// withoutGeneratedCount counts the units compiled because a named change touched them for want of +// saved generator output. +func (run *schedulerRun) withoutGeneratedCount() int { + count := 0 + for index, touched := range run.state.changedBy { + // Why only compiled units count: a unit touched for this reason and then blocked by another + // reference's errors never compiled, and its changed references are never reported either. + if !run.compiled[index] { + continue + } + if slices.ContainsFunc(touched, func(reference changedReference) bool { return reference.reason != "" }) { + count++ + } + } + + return count +} + // 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]) @@ -408,6 +456,7 @@ func (run *schedulerRun) changedReferencesOf(index int) []ChangedReference { for _, reference := range touched { references = append(references, ChangedReference{ Assembly: run.plan.Units[reference.reference].Assembly.AssemblyName, Name: reference.name, + Reason: reference.reason, }) } diff --git a/cli/dispatcher/internal/compilecheck/name_index_fifo_test.go b/cli/dispatcher/internal/compilecheck/name_index_fifo_test.go index aac8f5c7e..c0532d941 100644 --- a/cli/dispatcher/internal/compilecheck/name_index_fifo_test.go +++ b/cli/dispatcher/internal/compilecheck/name_index_fifo_test.go @@ -35,7 +35,7 @@ func TestNameIndexReadsGeneratedCodeBeforeStartReturns(t *testing.T) { index.readSources() releaseFIFO(t, source, "class First { }") - set, err := index.lookup(0) + set, _, err := index.lookup(0) if err != nil { t.Fatalf("expected the identifiers of the unit, got %v", err) } diff --git a/cli/dispatcher/internal/compilecheck/name_index_test.go b/cli/dispatcher/internal/compilecheck/name_index_test.go index 1ddfb846f..2aabb6801 100644 --- a/cli/dispatcher/internal/compilecheck/name_index_test.go +++ b/cli/dispatcher/internal/compilecheck/name_index_test.go @@ -172,13 +172,13 @@ func TestNameIndexIsOverOnceWaitReturns(t *testing.T) { default: t.Fatal("expected the read to be over once wait returned") } - if _, err := index.lookup(0); err != nil { + 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 { + 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 { + 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 e6fe23301..f5aa3c2c6 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler.go +++ b/cli/dispatcher/internal/compilecheck/scheduler.go @@ -169,7 +169,7 @@ func compileUnits( Skipped: run.referenceSkips, Blocked: run.blockedNames(), NameSkips: run.nameSkips, - NameIndex: run.names.report(), + NameIndex: run.nameIndexReport(), }, nil } @@ -244,17 +244,16 @@ func (run *schedulerRun) skipBlockedUnit(index int) (bool, error) { // 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 -// wrote would otherwise be picked up as this run's own output, and the units below it would be -// compiled against a result this run never produced. +// Why its earlier outputs and records are left alone: the units of this run read Unity's reference +// assembly for it because it counts as having kept the surface, a later run trusts them only through +// the input key and the output hashes its record holds, and its baseline and saved generator output are what later +// changed-files checks and name narrowing read. Removing them made the next edit of the unit compile +// it in full, and left the names of its generated code unread. func (run *schedulerRun) skipUnchangedDependent(index int) (bool, error) { unit := run.plan.Units[index] if !unit.SelectedAsDependent || run.state.changedInputs[index] != 0 { return false, nil } - if err := run.compiler.removeUnitOutputs(run.plan, unit); err != nil { - return false, err - } if err := run.markFinal(index); err != nil { return false, err } @@ -436,7 +435,8 @@ func (state *schedule) release(index int, effects []referenceEffect) []int { continue } state.changedInputs[dependent]++ - state.changedBy[dependent] = append(state.changedBy[dependent], changedReference{reference: index, name: effect.name}) + state.changedBy[dependent] = append(state.changedBy[dependent], + changedReference{reference: index, name: effect.name, reason: effect.reason}) 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_cache_test.go b/cli/dispatcher/internal/compilecheck/scheduler_cache_test.go index fb66ff6e8..50543db36 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_cache_test.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_cache_test.go @@ -462,9 +462,9 @@ func TestCompileUnitsCompilesAgainWhenAnOutputWasChanged(t *testing.T) { } } -// Verifies the skip of a dependent still wins over the reuse and takes the recorded result with it: -// the outputs it describes are removed, so nothing may replay diagnostics for them. -func TestCompileUnitsLeavesNoManifestForASkippedDependent(t *testing.T) { +// Verifies the skip of a dependent still wins over the reuse and keeps the recorded result of its +// earlier compile, so a later run can still reuse or check against it. +func TestCompileUnitsKeepsTheRecordOfASkippedDependent(t *testing.T) { project := newCachedSchedulerProject(t, map[string][]string{"Beta": {"Alpha"}}, "Alpha", "Beta") project.compileOrFail(t, newCachedSchedulerRecorder("Alpha", "Beta")) if !project.hasRecordedCompile("Beta") { @@ -488,8 +488,8 @@ func TestCompileUnitsLeavesNoManifestForASkippedDependent(t *testing.T) { if recorder.runs["Beta"] != 0 { t.Errorf("the skipped dependent should not have compiled, got %d runs", recorder.runs["Beta"]) } - if project.hasRecordedCompile("Beta") { - t.Error("the skipped dependent's recorded result should have been removed with its outputs") + if !project.hasRecordedCompile("Beta") { + t.Error("the skipped dependent's recorded result should have been kept") } } diff --git a/cli/dispatcher/internal/compilecheck/scheduler_generated_test.go b/cli/dispatcher/internal/compilecheck/scheduler_generated_test.go new file mode 100644 index 000000000..da4651807 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/scheduler_generated_test.go @@ -0,0 +1,144 @@ +package compilecheck + +import ( + "context" + "testing" + "time" +) + +// withAnalyzer gives the named unit of the project an analyzer, which is how a source generator +// reaches the compiler. +func withAnalyzer(t *testing.T, project *cachedSchedulerProject, name string) { + t.Helper() + index := planIndexOf(t, project.plan, name) + project.plan.Units[index].Assembly.Analyzers = []string{"Library/Generators/Generator.dll"} +} + +// Verifies a dependent with analyzers and no saved generator output that writes a changed name +// compiles for that name, and is reported with the name rather than the reason. +func TestCompileUnitsNamesTheChangedNameEvenWithoutSavedGeneratorOutput(t *testing.T) { + project, recorder := newNamesProject(t, "class B { Foo f; }") + withAnalyzer(t, project, "B") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + assertChangedReferences(t, resultOf(t, outcome.Units, "B"), []ChangedReference{{Assembly: "A", Name: "Foo"}}) + if outcome.NameIndex.WithoutGenerated != 0 { + t.Errorf("name index = %+v, want no unit compiled for want of generator output", outcome.NameIndex) + } +} + +// Verifies a dependent with analyzers and no saved generator output compiles on a named change it +// writes no name of, because its generators might, and is counted and reported with the reason. +func TestCompileUnitsCompilesADependentWithoutSavedGeneratorOutput(t *testing.T) { + project, recorder := newNamesProject(t, "class B { int Bar; }") + withAnalyzer(t, project, "B") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + assertChangedReferences(t, resultOf(t, outcome.Units, "B"), + []ChangedReference{{Assembly: "A", Reason: noSavedGeneratorOutputReason}}) + if outcome.NameIndex.WithoutGenerated != 1 || len(outcome.NameSkips) != 0 { + t.Errorf("name index = %+v, name skips = %+v; want one unit compiled for want of generator output", + outcome.NameIndex, outcome.NameSkips) + } +} + +// Verifies a dependent without analyzers is still left out by its sources alone when it has no saved +// generator output, since it has no generators to miss. +func TestCompileUnitsLeavesOutADependentWithoutAnalyzersByItsSourcesAlone(t *testing.T) { + project, recorder := newNamesProject(t, "class B { int Bar; }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A"}) + if recorder.started["B"] || outcome.NameIndex.WithoutGenerated != 0 { + t.Errorf("B started = %v, name index = %+v; want B left out by names", recorder.started["B"], outcome.NameIndex) + } +} + +// Verifies a dependent compiled once for want of saved generator output is left out by names on the +// next named change it writes no name of, since that compile saved its generator output. +func TestCompileUnitsLeavesOutADependentOnceItHasSavedGeneratorOutput(t *testing.T) { + project, recorder := newNamesProject(t, "class B { int Bar; }") + withAnalyzer(t, project, "B") + project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + if recorder.runs["B"] != 1 { + t.Fatalf("expected the first run to compile B once, got %d", recorder.runs["B"]) + } + + project.editSource(t, "A", "class A { void Body() { } }") + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + if recorder.runs["B"] != 1 || outcome.NameIndex.WithoutGenerated != 0 { + t.Errorf("B compiled %d times in all, name index = %+v; want B left out by names the second time", + recorder.runs["B"], outcome.NameIndex) + } + if outcome.NameIndex.WithGenerated != 1 { + t.Errorf("name index = %+v, want B read with its saved generator output", outcome.NameIndex) + } +} + +// Verifies a dependent whose compile never ran is compiled again for want of saved generator output +// on the next named change, since the failed compile saved none. +func TestCompileUnitsCompilesAgainADependentWhoseCompileNeverRan(t *testing.T) { + project, recorder := newNamesProject(t, "class B { int Bar; }") + withAnalyzer(t, project, "B") + recorder.failingUnit = "B" + failing := Compiler{ + Paths: project.paths, ProjectRoot: project.root, Timeout: time.Minute, Run: recorder.runner(), + compareSurface: namedVerdicts{"A": namedChangeOf("Foo")}.compare, + } + if _, err := compileUnits(context.Background(), failing, project.plan, 4); err == nil { + t.Fatal("expected the first run to fail when B's compiler cannot start") + } + + recorder.failingUnit = "" + project.editSource(t, "A", "class A { void Body() { } }") + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertChangedReferences(t, resultOf(t, outcome.Units, "B"), + []ChangedReference{{Assembly: "A", Reason: noSavedGeneratorOutputReason}}) + if outcome.NameIndex.WithoutGenerated != 1 || len(outcome.NameSkips) != 0 { + t.Errorf("name index = %+v, name skips = %+v; want B compiled for want of generator output", + outcome.NameIndex, outcome.NameSkips) + } +} + +// Verifies a dependent touched for want of saved generator output but blocked by another reference's +// errors is not counted, since it never compiled. +func TestCompileUnitsDoesNotCountABlockedDependentAsCompiledWithoutGeneratorOutput(t *testing.T) { + project, recorder := newOptimisticCachedProject(t, map[string][]string{"D": {"A", "C"}}, []string{"D"}, "A", "C", "D") + project.editSource(t, "D", "class D { int Bar; }") + withAnalyzer(t, project, "D") + recorder.output["C"] = fakeCompilerOutput{stdout: "Assets/C/C.cs(1,1): error CS1002: ; expected", exitCode: 1} + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "blocked", outcome.Blocked, []string{"D"}) + if outcome.NameIndex.WithoutGenerated != 0 { + t.Errorf("name index = %+v, want the blocked dependent left out of the count", outcome.NameIndex) + } +} + +// Verifies a dependent left out by names keeps its saved generator output, so a later named change +// it writes no name of still leaves it out with that output read. +func TestCompileUnitsKeepsTheSavedGeneratorOutputOfASkippedDependent(t *testing.T) { + project, recorder := newNamesProject(t, "class B { int Bar; }") + withAnalyzer(t, project, "B") + project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + project.editSource(t, "A", "class A { void Body() { } }") + project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + project.editSource(t, "A", "class A { void Body() { int x; } }") + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + if recorder.runs["B"] != 1 { + t.Errorf("B compiled %d times in all, want once", recorder.runs["B"]) + } + if outcome.NameIndex.WithGenerated != 1 || outcome.NameIndex.WithoutGenerated != 0 { + t.Errorf("name index = %+v, want B read with its saved generator output", outcome.NameIndex) + } +} diff --git a/cli/dispatcher/internal/compilecheck/scheduler_names_test.go b/cli/dispatcher/internal/compilecheck/scheduler_names_test.go index 149cbca1e..539a13647 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_names_test.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_names_test.go @@ -165,6 +165,36 @@ func TestCompileUnitsKeepsAnEarlyCompileWhenTheAssumedReferenceChangedNoNameItWr } } +// Verifies a unit with analyzers and no saved generator output that started ahead of a reference +// compiles again when the reference changed only names it never writes, whether it was picked up as +// a dependent or started on its own sources: without that output, names cannot say it is untouched. +func TestCompileUnitsCompilesAgainAnEarlyUnitWithoutSavedGeneratorOutput(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; }") + withAnalyzer(t, project, "D2") + + 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", Reason: noSavedGeneratorOutputReason}}) + if outcome.NameIndex.WithoutGenerated != 1 { + t.Errorf("name index = %+v, want D2 counted once", outcome.NameIndex) + } + }) + } +} + // 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) { diff --git a/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go b/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go index 35ada1207..47635e6ab 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_surface_test.go @@ -304,3 +304,55 @@ func TestCompileUnitsDoesNotCompareASurfaceNobodyInThePlanReads(t *testing.T) { } } } + +// Verifies a dependent left out because its reference kept the surface keeps the baseline of its own +// earlier full compile, so its next edit is checked on the changed files instead of compiled in full. +func TestCompileUnitsKeepsTheBaselineOfASkippedDependent(t *testing.T) { + project, worker := newLargeDependentProject(t) + compileWithFastPath(t, project, worker, newCachedSchedulerRecorder("Beta"), "Beta") + if !project.hasBaselineOf("Beta") { + t.Fatal("expected the first full compile of Beta to record a baseline") + } + + project.editSource(t, "Alpha", "class Alpha { void Body() { } }") + markSelectedAsDependent(t, project.plan, "Beta") + skipping := newCachedSchedulerRecorder("Alpha", "Beta") + skipping.surfaces["Alpha"] = keptSurfaceOf("Alpha") + skipped := compileWithFastPath(t, project, worker, skipping, "Alpha", "Beta") + if skipping.runs["Beta"] != 0 || skipped.Skipped != 1 { + t.Fatalf("expected Beta to be left out, got %d compiles and %d skips", skipping.runs["Beta"], skipped.Skipped) + } + + project.plan.Units[indexOfUnit(t, project.plan, "Beta")].SelectedAsDependent = false + project.writeFile(t, "Assets/Beta/File007.cs", bodyContent(7, "edited body")) + recorder := newCachedSchedulerRecorder("Beta") + outcome := compileWithFastPath(t, project, worker, recorder, "Beta") + + beta := resultOf(t, outcome.Units, "Beta") + if !beta.FastChecked || strings.Contains(beta.FastPathFallbackReason, "no baseline") { + t.Fatalf("expected Beta's edit to be checked against the kept baseline, got %+v", beta) + } +} + +// Verifies a dependent compiled in the same run reads Unity's reference assembly for a skipped +// dependent it references, not the output an earlier run left for it, while it reads this run's +// output for a reference whose surface changed. +func TestCompileUnitsReadsUnitysReferenceForASkippedDependent(t *testing.T) { + project := newCachedSchedulerProject(t, + map[string][]string{"Beta": {"Alpha"}, "Delta": {"Beta", "Gamma"}}, "Alpha", "Gamma", "Beta", "Delta") + markSelectedAsDependent(t, project.plan, "Beta", "Delta") + writeStaleUnitOutput(t, project.root, project.plan, "Beta") + recorder := newCachedSchedulerRecorder("Alpha", "Gamma", "Beta", "Delta") + recorder.surfaces["Alpha"] = keptSurfaceOf("Alpha") + recorder.surfaces["Gamma"] = "changed Gamma" + + project.compileOrFail(t, recorder) + + if recorder.runs["Beta"] != 0 || recorder.runs["Delta"] != 1 { + t.Fatalf("expected Beta left out and Delta compiled once, got %d and %d", + recorder.runs["Beta"], recorder.runs["Delta"]) + } + assertReferenceFlags(t, project.writtenResponseFile(t, "Delta"+responseFileExtension), + []string{unityReferenceFlag("Beta"), producedReferenceFlag(project.plan, "Gamma")}, + []string{producedReferenceFlag(project.plan, "Beta")}) +} diff --git a/cli/dispatcher/internal/compilecheck/scheduler_test.go b/cli/dispatcher/internal/compilecheck/scheduler_test.go index bf31ecaff..a9320cd21 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_test.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_test.go @@ -434,8 +434,8 @@ func compiledNames(results []UnitResult) []string { } // Verifies a dependent is left out when the assembly it waited for came out byte-identical to the -// one the last Unity build produced, and that the stale output of an earlier run is removed with it. -func TestCompileUnitsSkipsADependentWhoseReferenceKeptItsSurface(t *testing.T) { +// one the last Unity build produced, and that the output an earlier run left for it is kept. +func TestCompileUnitsKeepsTheOutputOfASkippedDependent(t *testing.T) { plan := newSchedulerPlan(map[string][]string{"B": {"A"}}, "A", "B") markSelectedAsDependent(t, plan, "B") recorder := newSchedulerRecorder(0) @@ -457,8 +457,8 @@ func TestCompileUnitsSkipsADependentWhoseReferenceKeptItsSurface(t *testing.T) { if recorder.started["B"] { t.Error("a dependent whose reference kept its surface should not have been compiled") } - if _, statErr := os.Stat(staleOutput); !os.IsNotExist(statErr) { - t.Error("the skipped unit's earlier output should have been removed") + if _, statErr := os.Stat(staleOutput); statErr != nil { + t.Errorf("the skipped unit's earlier output should have been kept, got: %v", statErr) } } diff --git a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go index e48236a6d..102c7d76c 100644 --- a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go +++ b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log.go @@ -197,7 +197,11 @@ func addCompileCheckVibeScheduling(entry map[string]any, unit compilecheck.UnitR 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 := map[string]any{"reference": reference.Assembly, "name": reference.Name} + if reference.Reason != "" { + entry["reason"] = reference.Reason + } + changedBy = append(changedBy, entry) } entry["changed_by"] = changedBy } @@ -215,11 +219,12 @@ func addCompileCheckVibeNameNarrowing(context map[string]any, result compilechec } 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, - "sources_read": result.NameIndex.SourcesRead, + "duration_ms": result.NameIndex.Duration.Milliseconds(), + "units": result.NameIndex.Units, + "with_generated": result.NameIndex.WithGenerated, + "without_generated": result.NameIndex.WithoutGenerated, + "unreadable": result.NameIndex.Unreadable, + "sources_read": result.NameIndex.SourcesRead, } } } 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 2d46b9ae7..d99839806 100644 --- a/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go +++ b/cli/dispatcher/internal/dispatcher/compile_check_vibe_log_test.go @@ -249,7 +249,7 @@ func TestCompileCheckVibeLogRecordsUnitsCompiledAgain(t *testing.T) { } // 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. +// which name or for which reason, 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() @@ -258,12 +258,16 @@ func TestCompileCheckVibeLogRecordsNameNarrowing(t *testing.T) { {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"}}, + ChangedReferences: []compilecheck.ChangedReference{ + {Assembly: "Alpha", Name: "Foo"}, {Assembly: "Gamma"}, {Assembly: "Epsilon", Reason: "no saved generator output"}, + }, }, }, Skipped: 1, NameSkips: []compilecheck.NameSkip{{Assembly: "Delta", ChangedReferences: []string{"Alpha"}}}, - NameIndex: compilecheck.NameIndexReport{Duration: 30 * time.Millisecond, Units: 3, WithGenerated: 1, Unreadable: 1, SourcesRead: true}, + NameIndex: compilecheck.NameIndexReport{ + Duration: 30 * time.Millisecond, Units: 3, WithGenerated: 1, WithoutGenerated: 1, Unreadable: 1, SourcesRead: true, + }, } logRun := startCompileCheckVibeLog(projectRoot, compileCheckOptions{}) @@ -272,11 +276,12 @@ func TestCompileCheckVibeLogRecordsNameNarrowing(t *testing.T) { 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"}],`+ + `{"assembly":"Beta","changed_by":[{"name":"Foo","reference":"Alpha"},{"name":"","reference":"Gamma"},`+ + `{"name":"","reason":"no saved generator output","reference":"Epsilon"}],`+ `"duration_ms":200,"succeeded":true}]`) assertVibeContextJSON(t, complete, "name_skips", `[{"assembly":"Delta","unaffected_by":["Alpha"]}]`) assertVibeContextJSON(t, complete, "name_index", - `{"duration_ms":30,"sources_read":true,"units":3,"unreadable":1,"with_generated":1}`) + `{"duration_ms":30,"sources_read":true,"units":3,"unreadable":1,"with_generated":1,"without_generated":1}`) } // Verifies a run that read only the saved generator output of the units with references logs the @@ -295,7 +300,7 @@ func TestCompileCheckVibeLogRecordsANameIndexWhoseSourcesWereNotRead(t *testing. complete := assertCompileCheckVibeRun(t, readCompileCheckVibeLog(t, projectRoot)) assertVibeContextJSON(t, complete, "name_index", - `{"duration_ms":2,"sources_read":false,"units":2,"unreadable":0,"with_generated":0}`) + `{"duration_ms":2,"sources_read":false,"units":2,"unreadable":0,"with_generated":0,"without_generated":0}`) } // Verifies a run that read no names and left nothing out by names logs neither. diff --git a/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md b/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md index 65f1ad200..be8b36764 100644 --- a/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md +++ b/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md @@ -62,12 +62,13 @@ same result, and only spend the time. shapes compile about as many dependents as before. - A dependent left out by names is not compiled, so an error only compiling it would show is missed when the names miss it. These are the known gaps: - - a source generator that writes code using a member of a referenced type whose name the - dependent's sources never write, in a dependent with no saved generator output. Leaving a - dependent out - because its references kept their surface or changed only names it never - writes - removes its saved generator 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. Closing - this gap needs a left-out dependent to keep its outputs and record, which is a separate change; + - a source generator that generates from a referenced assembly's contents rather than from the + dependent's own code. A unit with analyzers and no saved generator output - one the check has + never compiled to the end - is compiled on a named change rather than narrowed, whether it was + picked up as a dependent or started on its own sources ahead of the reference, and leaving a + dependent out keeps its saved generator output, so after its first compile the names its + generators write are read along with its sources (VibeLog `name_index.with_generated`, and + `name_index.without_generated` for the units compiled for want of that output); - a change to the type of an async method builder, which a dependent never names (a change to its members is unnamed); - an identifier a dependent writes only with `\u` escapes. @@ -85,3 +86,7 @@ same result, and only spend the time. change to the unnamed list, or stop leaving dependents out by names. - The `name_index` duration in the VibeLog is more than a tenth of a run in which no reference changed by names. Then start reading the names only when the first named change arrives. + +Amended 2026-10-17 by PR-3 of feature/compile-check-visible: a left-out dependent keeps its outputs +and records, and a unit with analyzers and no saved generator output, whether it was picked up as a +dependent or started on its own sources ahead of the reference, is compiled rather than narrowed. diff --git a/docs/compile-check.md b/docs/compile-check.md index f0e3e7e6e..92758420a 100644 --- a/docs/compile-check.md +++ b/docs/compile-check.md @@ -228,10 +228,9 @@ build - does not call the worker again: it reports what the previous check repor Otherwise the assembly is compiled in full as before, and `FastPathFallbacks` says why: -- `no baseline: no full compile of this assembly is on record` (none ran yet, a later run - picked the assembly up only because a reference changed and then skipped it, which removes its - result and baseline, or an input changed while the last full compile ran, so it left no record; - `FastPathBaselineNotes` of that run says which) +- `no baseline: no full compile of this assembly is on record` (none ran yet, or an input changed + while the last full compile ran, so it left no record; `FastPathBaselineNotes` of that run says + which) - `no baseline: the last full compile had errors; the first full compile without errors records one` (a full compile that fails drops the baseline an earlier one left, and the assembly is compiled in full until a compile succeeds) @@ -273,10 +272,10 @@ So the first body edit is compiled in full more often than it looks: with `inputs other than source contents changed: references`. - An assembly a run leaves out because nothing it compiles from changed keeps its result and baseline, so its next body edit is checked against them. An assembly a run picks up only because - a reference changed loses its result and baseline either way: compiled, it records no baseline, - and left out, because every such reference kept its public surface or changed only names the - assembly never writes, its result and baseline are removed. The next body edit of that assembly - is compiled in full. A run that blocks an assembly because one it references has errors keeps + a reference changed records no baseline when it is compiled, so its next body edit is compiled in + full. When it is left out instead, because every such reference kept its public surface or + changed only names the assembly never writes, it keeps its result, baseline and saved generator + output, so its next body edit is checked against them as before. A run that blocks an assembly because one it references has errors keeps its result and baseline: the next run checks them against the inputs and output hashes they were recorded with before using them, so a reference that comes back the same leaves the assembly reused or checked on its changed files, and one that comes back changed compiles it. @@ -389,15 +388,16 @@ happened, and how long each assembly took. `docs/vibe-logs.md` lists their field refused only when `ProjectSettings.asset` was saved after the last build. - **The predefined assemblies** (`Assembly-CSharp` and friends) keep the source list of the last build, so a brand-new `.cs` file outside any `.asmdef` is not compiled until Unity imports it. -- **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. +- **A dependent compile-check has never compiled is compiled, not narrowed by names, the first time + a reference changes.** The names are read from the dependent's sources and from the generator + output its last finished compile saved; a unit with analyzers and no saved output, whether it was + picked up as a dependent or started on its own sources ahead of the reference, compiles on any + named change, and the VibeLog reports it with `changed_by.reason` `no saved generator output` and + counts it in `name_index.without_generated`. After that its saved generator output is read, and + leaving it out keeps that output. What stays invisible is a generator that generates from a + referenced assembly's contents rather than from the dependent's own code. 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. diff --git a/docs/vibe-logs.md b/docs/vibe-logs.md index f7c934bea..fae15d2ec 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`, and `build_settings` when the run got as far as comparing the build settings; 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`, `sources_read`: what reading the names of the assemblies with references in the plan took, and whether the run read their sources - it does only once it compiles an assembly, has the worker check changed files, or a verdict needs the names; only when the plan has an assembly with references), and `build_settings` (`stamp_found`: a build-trace file of the last Unity build exists; `record`: `new` when the run recorded the settings of a build for the first time, `replaced` when it recorded them after another Unity build, `kept` when it compared them against the record of the same build, empty when there was no build-trace file; `refused_part`: the setting a refusal was for, empty otherwise; `defines_checked`: the run looked for a scripting define the build lacks, which it does only when `ProjectSettings.asset` was saved after the last build or there is no build-trace file) | +| `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`, and `build_settings` when the run got as far as comparing the build settings; 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, and `reason` when a change told by names counted without a name it writes: `no saved generator output`) - 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`, `without_generated` (the assemblies compiled on a named change because they have analyzers and no saved generator output), `unreadable`, `sources_read`: what reading the names of the assemblies with references in the plan took, and whether the run read their sources - it does only once it compiles an assembly, has the worker check changed files, or a verdict needs the names; only when the plan has an assembly with references), and `build_settings` (`stamp_found`: a build-trace file of the last Unity build exists; `record`: `new` when the run recorded the settings of a build for the first time, `replaced` when it recorded them after another Unity build, `kept` when it compared them against the record of the same build, empty when there was no build-trace file; `refused_part`: the setting a refusal was for, empty otherwise; `defines_checked`: the run looked for a scripting define the build lacks, which it does only when `ProjectSettings.asset` was saved after the last build or there is no build-trace file) | - 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 From e3ba5a9f5df8538460011878383a75cbd7fc7133 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Sat, 17 Oct 2026 04:35:42 +0900 Subject: [PATCH 05/11] fix: compile-check reads identifiers written with unicode escapes (#3359) --- .../compilecheck/identifier_escape.go | 94 +++++++++++++++++++ .../internal/compilecheck/name_index.go | 22 +++-- .../internal/compilecheck/name_index_test.go | 68 +++++++++++++- .../compilecheck/scheduler_names_test.go | 10 ++ ...s-that-name-nothing-a-reference-changed.md | 5 +- docs/compile-check.md | 2 - 6 files changed, 185 insertions(+), 16 deletions(-) create mode 100644 cli/dispatcher/internal/compilecheck/identifier_escape.go diff --git a/cli/dispatcher/internal/compilecheck/identifier_escape.go b/cli/dispatcher/internal/compilecheck/identifier_escape.go new file mode 100644 index 000000000..bda74f601 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/identifier_escape.go @@ -0,0 +1,94 @@ +package compilecheck + +import ( + "unicode/utf8" +) + +const ( + shortUnicodeEscapeDigits = 4 // \uXXXX + longUnicodeEscapeDigits = 8 // \UXXXXXXXX + surrogateFirst = 0xD800 + surrogateLast = 0xDFFF +) + +// decodeUnicodeEscape decodes the \uXXXX or \UXXXXXXXX escape at position, the way C# reads one in +// an identifier, and reports the character and the length of the whole escape. ok is false when +// there is no escape there, when a digit is missing or not hexadecimal, or when the value is a +// surrogate or beyond the last code point, which the compiler does not take as an identifier +// character either. +func decodeUnicodeEscape(content []byte, position int) (rune, int, bool) { + if position+1 >= len(content) || content[position] != '\\' { + return 0, 0, false + } + digits := 0 + switch content[position+1] { + case 'u': + digits = shortUnicodeEscapeDigits + case 'U': + digits = longUnicodeEscapeDigits + default: + return 0, 0, false + } + size := 2 + digits + if position+size > len(content) { + return 0, 0, false + } + + // Why the value is gathered unsigned: eight digits from 8 up overflow a rune into a negative + // value, which would slip past the upper bound. + value := uint32(0) + for _, digit := range content[position+2 : position+size] { + nibble, isHex := hexValue(digit) + if !isHex { + return 0, 0, false + } + value = value<<4 | uint32(nibble) + } + if (value >= surrogateFirst && value <= surrogateLast) || value > utf8.MaxRune { + return 0, 0, false + } + + return rune(value), size, true +} + +// hexValue is the value of one hexadecimal digit. +func hexValue(digit byte) (rune, bool) { + switch { + case digit >= '0' && digit <= '9': + return rune(digit - '0'), true + case digit >= 'a' && digit <= 'f': + return rune(digit-'a') + 10, true + case digit >= 'A' && digit <= 'F': + return rune(digit-'A') + 10, true + } + + return 0, false +} + +// runeAt is the character at position and how many bytes it takes: a \u or \U escape decoded, and +// otherwise the UTF-8 character there. +func runeAt(content []byte, position int) (rune, int, bool) { + // Why ASCII is answered first: nearly every byte of a source is ASCII and no escape, and the scan + // asks for every one of them. + if first := content[position]; first < utf8.RuneSelf && first != '\\' { + return rune(first), 1, false + } + if character, size, ok := decodeUnicodeEscape(content, position); ok { + return character, size, true + } + character, size := utf8.DecodeRune(content[position:]) + + return character, size, false +} + +// decodedText spells an identifier's text with its escapes decoded. +func decodedText(text []byte) []byte { + decoded := make([]byte, 0, len(text)) + for position := 0; position < len(text); { + character, size, _ := runeAt(text, position) + decoded = utf8.AppendRune(decoded, character) + position += size + } + + return decoded +} diff --git a/cli/dispatcher/internal/compilecheck/name_index.go b/cli/dispatcher/internal/compilecheck/name_index.go index 0347b3fe5..aa63b558e 100644 --- a/cli/dispatcher/internal/compilecheck/name_index.go +++ b/cli/dispatcher/internal/compilecheck/name_index.go @@ -13,7 +13,6 @@ import ( "sync/atomic" "time" "unicode" - "unicode/utf8" ) // targetTypedNewToken stands for a target-typed new in a dependent's identifiers, the way the worker @@ -63,14 +62,14 @@ 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. +// characters, the last left out of the name as the compiler leaves them out. A \u or \U escape counts +// as the character it spells, as it does for the compiler. 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. func scanIdentifiers(content []byte, into identifierSet) { position := 0 for position < len(content) { - character, size := utf8.DecodeRune(content[position:]) + character, size, escaped := runeAt(content, position) if !isIdentifierStart(character) { position += size @@ -79,13 +78,20 @@ func scanIdentifiers(content []byte, into identifierSet) { start := position position += size for position < len(content) { - next, nextSize := utf8.DecodeRune(content[position:]) + next, nextSize, nextEscaped := runeAt(content, position) if !isIdentifierPart(next) { break } + escaped = escaped || nextEscaped position += nextSize } - identifier := identifierText(content[start:position]) + text := content[start:position] + // Why the text is decoded only when it holds an escape: nearly no identifier does, and + // decoding every one would copy each name the scan meets. + if escaped { + text = decodedText(text) + } + identifier := identifierText(text) into[identifier] = struct{}{} if identifier == "new" && opensParenthesis(content[position:]) { into[targetTypedNewToken] = struct{}{} diff --git a/cli/dispatcher/internal/compilecheck/name_index_test.go b/cli/dispatcher/internal/compilecheck/name_index_test.go index 2aabb6801..0bf594cff 100644 --- a/cli/dispatcher/internal/compilecheck/name_index_test.go +++ b/cli/dispatcher/internal/compilecheck/name_index_test.go @@ -61,15 +61,16 @@ func BenchmarkScanIdentifiers(b *testing.B) { } // 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. +// kept, words in strings and comments read too - a \u escape in a literal decoded like one in an +// identifier - 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"}) + []string{"class", "new(", "newFoo", "Commented", "Quoted", "名前", "_under9", "Point", "A"}, + []string{"@class", "u0041", "9"}) } // Verifies "newFoo(" alone is not read as a target-typed new. @@ -202,3 +203,62 @@ func TestNameIndexReportCountsOnlyTheTimeSpentReading(t *testing.T) { t.Errorf("report = %+v, want the sources read in well under 150 ms", report) } } + +// escapeOf spells a \u or \U escape: the backslash followed by the given text. +func escapeOf(text string) string { + return string(rune(0x5C)) + text +} + +// Verifies \u and \U escapes are decoded into the identifier they spell, at its start or inside it. +func TestScanIdentifiersDecodesUnicodeEscapes(t *testing.T) { + cases := map[string]string{ + escapeOf("u0041") + "bc": "Abc", + "A" + escapeOf("u0042"): "AB", + escapeOf("U00000041"): "A", + "A" + escapeOf("U00000042"): "AB", + escapeOf("uFF21") + "oo": "\uFF21oo", + } + for content, want := range cases { + set := identifiersOf(content + " ") + if _, found := set[want]; !found || len(set) != 1 { + t.Errorf("%q: identifiers = %v, want only %q", content, set, want) + } + } +} + +// Verifies an escape with a digit that is not hexadecimal, or with too few digits, is left as text. +func TestScanIdentifiersLeavesAMalformedEscapeAlone(t *testing.T) { + assertHasIdentifiers(t, identifiersOf(`\u00G1 `), []string{"u00G1"}, []string{"A"}) + assertHasIdentifiers(t, identifiersOf(`\u004 `), []string{"u004"}, []string{"\x04"}) +} + +// Verifies an escape spelling a surrogate, at either end of the range, is not decoded, while the +// first value past the range is. +func TestDecodeUnicodeEscapeRejectsASurrogate(t *testing.T) { + for _, content := range []string{escapeOf("uD800"), escapeOf("uDFFF")} { + if character, _, ok := decodeUnicodeEscape([]byte(content), 0); ok { + t.Errorf("%q: expected a surrogate escape to be rejected, got %q", content, character) + } + } + if character, _, ok := decodeUnicodeEscape([]byte(escapeOf("uE000")), 0); !ok || character != 0xE000 { + t.Errorf("expected the first value past the surrogates to decode, got %q (ok %v)", character, ok) + } +} + +// Verifies a file ending in a lone backslash is read to its end without failing. +func TestScanIdentifiersReadsAFileEndingInABackslash(t *testing.T) { + assertHasIdentifiers(t, identifiersOf("x "+escapeOf("")), []string{"x"}, []string{}) +} + +// Verifies an escape cut short by the end of the file - down to a lone backslash - or spelling a +// value past the last code point, even one that overflows a signed 32-bit value, is not decoded. +func TestDecodeUnicodeEscapeRejectsATruncatedOrOutOfRangeEscape(t *testing.T) { + for _, content := range []string{ + escapeOf(""), escapeOf("u004"), escapeOf("U0000004"), escapeOf("U00110000"), + escapeOf("U80000000"), escapeOf("UFFFFFFFF"), + } { + if character, size, ok := decodeUnicodeEscape([]byte(content), 0); ok { + t.Errorf("%q: decoded %q (size %d), want it rejected", content, character, size) + } + } +} diff --git a/cli/dispatcher/internal/compilecheck/scheduler_names_test.go b/cli/dispatcher/internal/compilecheck/scheduler_names_test.go index 539a13647..e8e8c9935 100644 --- a/cli/dispatcher/internal/compilecheck/scheduler_names_test.go +++ b/cli/dispatcher/internal/compilecheck/scheduler_names_test.go @@ -371,3 +371,13 @@ func TestCompileUnitsReadsTheSourcesOnceAUnitCompilesInFull(t *testing.T) { t.Errorf("name index = %+v, want the sources read", outcome.NameIndex) } } + +// Verifies a dependent that writes a changed name only with a \u escape compiles for that name. +func TestCompileUnitsCompilesADependentThatWritesTheChangedNameWithAnEscape(t *testing.T) { + project, recorder := newNamesProject(t, "class B { int x = "+escapeOf("u0046")+"oo(); }") + + outcome := project.compileWithVerdicts(t, recorder, namedVerdicts{"A": namedChangeOf("Foo")}) + + assertStrings(t, "results", compiledNames(outcome.Units), []string{"A", "B"}) + assertChangedReferences(t, resultOf(t, outcome.Units, "B"), []ChangedReference{{Assembly: "A", Name: "Foo"}}) +} diff --git a/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md b/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md index be8b36764..abdd77336 100644 --- a/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md +++ b/docs/adr/0017-compile-check-skips-dependents-that-name-nothing-a-reference-changed.md @@ -70,8 +70,7 @@ same result, and only spend the time. generators write are read along with its sources (VibeLog `name_index.with_generated`, and `name_index.without_generated` for the units compiled for want of that output); - a change to the type of an async method builder, which a dependent never names (a change to - its members is unnamed); - - an identifier a dependent writes only with `\u` escapes. + its members is unnamed). - A project that puts its own extension classes in `System.*` gets every dependent compiled when one of their members changes, because the whole namespace is unnamed. - The members C# binds by pattern include common method names such as `Create`, `Add`, `Count` and @@ -90,3 +89,5 @@ same result, and only spend the time. Amended 2026-10-17 by PR-3 of feature/compile-check-visible: a left-out dependent keeps its outputs and records, and a unit with analyzers and no saved generator output, whether it was picked up as a dependent or started on its own sources ahead of the reference, is compiled rather than narrowed. +Amended 2026-10-17 by PR-4 of feature/compile-check-visible: identifiers written with `\u` escapes +are read decoded, so that gap is closed. diff --git a/docs/compile-check.md b/docs/compile-check.md index 92758420a..f914a4af8 100644 --- a/docs/compile-check.md +++ b/docs/compile-check.md @@ -401,8 +401,6 @@ happened, and how long each assembly took. `docs/vibe-logs.md` lists their field - **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 From 7695a5a098fcda999cdf0765bcb1f3fa1375b3c7 Mon Sep 17 00:00:00 2001 From: Masamichi Hatayama Date: Sat, 17 Oct 2026 04:48:23 +0900 Subject: [PATCH 06/11] feat: compile-check picks up new scripts outside every assembly definition (#3360) --- .../CliOnlyTools~/CompileCheck/Skill/SKILL.md | 5 +- .../internal/compilecheck/assembly_set.go | 8 +- cli/dispatcher/internal/compilecheck/plan.go | 10 + .../compilecheck/predefined_sources.go | 131 +++++++ .../compilecheck/predefined_sources_test.go | 323 ++++++++++++++++++ .../internal/compilecheck/source_rescue.go | 20 +- .../internal/compilecheck/source_set.go | 17 +- .../internal/compilecheck/source_set_test.go | 2 +- docs/compile-check.md | 33 +- 9 files changed, 528 insertions(+), 21 deletions(-) create mode 100644 cli/dispatcher/internal/compilecheck/predefined_sources.go create mode 100644 cli/dispatcher/internal/compilecheck/predefined_sources_test.go diff --git a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md index 344139fe9..856b06740 100644 --- a/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md +++ b/Packages/src/Editor/CliOnlyTools~/CompileCheck/Skill/SKILL.md @@ -117,6 +117,7 @@ compile-check once more rather than retrying in a loop. - A reference removed from an `.asmdef` stops the run with `COMPILE_CHECK_UNITY_BUILD_REQUIRED`; the four references Unity adds on its own are allowed, and an `.asmdef` with a reference that matches no `.asmdef` in the project is not judged. -- The predefined assemblies (`Assembly-CSharp` and friends) keep the source list of the last build, - so a brand-new `.cs` file outside an `.asmdef` is not compiled until Unity imports it. +- A new `.cs` file outside every `.asmdef` goes to `Assembly-CSharp` or a sibling by Unity's path + rules (`Editor` folders, `Assets/Plugins`); if the last build did not produce that assembly, the run + stops and asks for `uloop compile`. A new script in a package folder without an `.asmdef` is not seen. - The produced DLLs are never loaded by Unity, and the command never contacts a running Editor. diff --git a/cli/dispatcher/internal/compilecheck/assembly_set.go b/cli/dispatcher/internal/compilecheck/assembly_set.go index 956cae049..a461ba280 100644 --- a/cli/dispatcher/internal/compilecheck/assembly_set.go +++ b/cli/dispatcher/internal/compilecheck/assembly_set.go @@ -15,10 +15,10 @@ const ( // predefinedAssemblyNames are the assemblies Unity builds without an .asmdef of their own. var predefinedAssemblyNames = map[string]bool{ - "Assembly-CSharp": true, - "Assembly-CSharp-firstpass": true, - "Assembly-CSharp-Editor": true, - "Assembly-CSharp-Editor-firstpass": true, + assemblyCSharp: true, + assemblyCSharpFirstpass: true, + assemblyCSharpEditor: true, + assemblyCSharpEditorFirstpass: true, } // DetectAssemblySetChange refuses a run whose set of assemblies no longer matches the last build. diff --git a/cli/dispatcher/internal/compilecheck/plan.go b/cli/dispatcher/internal/compilecheck/plan.go index a0337ad65..e1a201c3a 100644 --- a/cli/dispatcher/internal/compilecheck/plan.go +++ b/cli/dispatcher/internal/compilecheck/plan.go @@ -214,6 +214,16 @@ func selectAssembliesByOwnership( context := NewAssemblyContext(graph, assemblyDefinitions) owners := newAssemblyOwnerIndex(assemblyDefinitions, references, context, tree) + // Why Assets is walked once here rather than per assembly: one walk decides the assembly of every + // script outside the assembly definitions, and each predefined assembly takes its share of it. + predefined, predefinedErr := globPredefinedSources(projectRoot, owners) + if predefinedErr != nil { + return nil, predefinedErr + } + if lackErr := detectPredefinedAssemblyTheBuildLacks(predefined, graph); lackErr != nil { + return nil, lackErr + } + owners.predefinedSources = predefined if referenceErr := DetectAssemblyReferenceChange( projectRoot, graph, context, references, owners); referenceErr != nil { return nil, referenceErr diff --git a/cli/dispatcher/internal/compilecheck/predefined_sources.go b/cli/dispatcher/internal/compilecheck/predefined_sources.go new file mode 100644 index 000000000..5cce333bd --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/predefined_sources.go @@ -0,0 +1,131 @@ +package compilecheck + +import ( + "path/filepath" + "sort" + "strings" +) + +const ( + assemblyCSharp = "Assembly-CSharp" + assemblyCSharpFirstpass = "Assembly-CSharp-firstpass" + assemblyCSharpEditor = "Assembly-CSharp-Editor" + assemblyCSharpEditorFirstpass = "Assembly-CSharp-Editor-firstpass" + editorPathSegment = "/editor/" +) + +// firstpassPathPrefixes are the folders whose scripts Unity compiles before the rest, written the +// way Unity matches them: lower case, from a '/' put before the project-relative path. +var firstpassPathPrefixes = []string{ + "/assets/plugins/", "/assets/standard assets/", "/assets/pro standard assets/", "/assets/iphone standard assets/", +} + +// uncompiledPathPrefixes are the folders under Assets whose scripts Unity imports but never compiles, +// written the same way. Unity's script importer holds this list in native code, out of sight of +// its C# build rules. +var uncompiledPathPrefixes = []string{ + "/assets/streamingassets/", "/assets/webplayertemplates/", "/assets/webgltemplates/", "/assets/plugins/android/", +} + +// hasPathPrefix reports whether a project-relative path, compared the way Unity compares it, starts +// with one of the prefixes. +func hasPathPrefix(relativePath string, prefixes []string) bool { + lower := "/" + strings.ToLower(filepath.ToSlash(relativePath)) + for _, prefix := range prefixes { + if strings.HasPrefix(lower, prefix) { + return true + } + } + + return false +} + +// predefinedAssemblyFor names the assembly Unity compiles a script outside every assembly definition +// into, from its project-relative path. Unity picks the assembly whose path rule matches deepest; an +// "editor" folder anywhere always matches deeper than a first-pass folder, and of the two editor +// assemblies that then tie, the first-pass one comes first. +func predefinedAssemblyFor(relativePath string) string { + firstpass := hasPathPrefix(relativePath, firstpassPathPrefixes) + editor := strings.Contains("/"+strings.ToLower(filepath.ToSlash(relativePath)), editorPathSegment) + + switch { + case firstpass && editor: + return assemblyCSharpEditorFirstpass + case firstpass: + return assemblyCSharpFirstpass + case editor: + return assemblyCSharpEditor + default: + return assemblyCSharp + } +} + +// globPredefinedSources lists the scripts under Assets that no assembly definition or reference +// owns, by the predefined assembly Unity compiles each into, sorted. Scripts in the folders Unity +// never compiles are left out. +// Why Packages is left out: Unity's first-pass rule only looks at Assets, and where a script in a +// package folder with no assembly definition goes changes between Unity versions. +func globPredefinedSources(projectRoot string, owners assemblyOwnerIndex) (map[string][]string, error) { + assets := filepath.Join(projectRoot, assetsDirectoryName) + byAssembly := map[string][]string{} + // Why an assembly definition in Assets itself ends the search: every script below then belongs to + // it, and Unity builds no predefined assembly at all. + if owners.startsAnotherAssembly(assets) { + return byAssembly, nil + } + + sources, err := globAssemblySources(projectRoot, assets, owners) + if err != nil { + return nil, err + } + for _, source := range sources { + if hasPathPrefix(source, uncompiledPathPrefixes) { + continue + } + name := predefinedAssemblyFor(source) + byAssembly[name] = append(byAssembly[name], source) + } + for _, assemblySources := range byAssembly { + sort.Strings(assemblySources) + } + + return byAssembly, nil +} + +// detectPredefinedAssemblyTheBuildLacks refuses a script whose predefined assembly the last Unity +// build did not produce, such as the first editor script of a project that had none: the response +// file that would compile it does not exist yet. +func detectPredefinedAssemblyTheBuildLacks(predefined map[string][]string, graph assemblyGraph) error { + names := make([]string, 0, len(predefined)) + for name := range predefined { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + if _, built := graph.byName[name]; built { + continue + } + + return unityBuildRequired("%s belongs to %s, which the last Unity build did not produce", + predefined[name][0], name) + } + + return nil +} + +// appendPredefinedSources adds the scripts a predefined assembly gained since the build after the +// ones it recorded, keeping the recorded order and putting the added ones in ordinal order. +func appendPredefinedSources(existing []string, globbed []string) []string { + recorded := make(map[string]bool, len(existing)) + for _, source := range existing { + recorded[filepath.ToSlash(source)] = true + } + sources := existing + for _, source := range globbed { + if !recorded[source] { + sources = append(sources, source) + } + } + + return sources +} diff --git a/cli/dispatcher/internal/compilecheck/predefined_sources_test.go b/cli/dispatcher/internal/compilecheck/predefined_sources_test.go new file mode 100644 index 000000000..0b88abcf8 --- /dev/null +++ b/cli/dispatcher/internal/compilecheck/predefined_sources_test.go @@ -0,0 +1,323 @@ +package compilecheck + +import ( + "path/filepath" + "testing" + "time" +) + +// predefinedOwnersFor indexes a project the way one run does, with only the named assemblies built, +// and globs the scripts no assembly definition owns. +func predefinedOwnersFor(t *testing.T, projectRoot string, built ...string) assemblyOwnerIndex { + t.Helper() + tree := walkProjectTree(projectRoot) + definitions, err := IndexAssemblyDefinitions(tree) + if err != nil { + t.Fatalf("failed to index the assembly definitions: %v", err) + } + references, referenceErr := IndexAssemblyReferences(tree) + if referenceErr != nil { + t.Fatalf("failed to index the assembly references: %v", referenceErr) + } + graph := assemblyGraph{byName: map[string]ResponseFile{}} + for _, name := range built { + graph.byName[name] = ResponseFile{AssemblyName: name} + } + owners := newAssemblyOwnerIndex(definitions, references, NewAssemblyContext(graph, definitions), tree) + predefined, globErr := globPredefinedSources(projectRoot, owners) + if globErr != nil { + t.Fatalf("globPredefinedSources returned an error: %v", globErr) + } + owners.predefinedSources = predefined + + return owners +} + +// rebuildPredefinedSources rebuilds the sources of a predefined assembly that recorded the given ones. +func rebuildPredefinedSources(t *testing.T, projectRoot string, assemblyName string, recorded ...string) []string { + t.Helper() + rsp := ResponseFile{AssemblyName: assemblyName, Sources: recorded} + sources, err := RebuildSources(projectRoot, rsp, nil, predefinedOwnersFor(t, projectRoot)) + if err != nil { + t.Fatalf("RebuildSources returned an error: %v", err) + } + + return sources +} + +// writeProjectFiles writes empty files at the given project-relative paths. +func writeProjectFiles(t *testing.T, projectRoot string, relativePaths ...string) { + t.Helper() + for _, relativePath := range relativePaths { + writeFileAt(t, filepath.Join(projectRoot, filepath.FromSlash(relativePath)), "") + } +} + +// Verifies the editor rule matches an "editor" folder in any case and anywhere in the path, and not +// a folder whose name only contains the word. +func TestPredefinedAssemblyForMatchesTheEditorSegmentOnly(t *testing.T) { + cases := map[string]string{ + "Assets/Editor/X.cs": assemblyCSharpEditor, + "Assets/Tools/editor/X.cs": assemblyCSharpEditor, + "Assets/EDITOR/X.cs": assemblyCSharpEditor, + "Assets/Scripts/EditorTools/X.cs": assemblyCSharp, + "Assets/MyEditor/X.cs": assemblyCSharp, + "Assets/Editor.cs": assemblyCSharp, + } + for path, want := range cases { + if got := predefinedAssemblyFor(path); got != want { + t.Errorf("%s: assembly = %q, want %q", path, got, want) + } + } +} + +// Verifies the first-pass rule matches each first-pass folder only at the top of Assets, in any case. +func TestPredefinedAssemblyForMatchesTheFirstpassFolders(t *testing.T) { + cases := map[string]string{ + "Assets/Plugins/X.cs": assemblyCSharpFirstpass, + "Assets/plugins/Sub/X.cs": assemblyCSharpFirstpass, + "Assets/Standard Assets/X.cs": assemblyCSharpFirstpass, + "Assets/Pro Standard Assets/X.cs": assemblyCSharpFirstpass, + "Assets/iPhone Standard Assets/X.cs": assemblyCSharpFirstpass, + "Assets/Game/Plugins/X.cs": assemblyCSharp, + "Assets/Plugins/Editor/X.cs": assemblyCSharpEditorFirstpass, + "Assets/Standard Assets/Sub/editor/X.cs": assemblyCSharpEditorFirstpass, + } + for path, want := range cases { + if got := predefinedAssemblyFor(path); got != want { + t.Errorf("%s: assembly = %q, want %q", path, got, want) + } + } +} + +// Verifies a script an assembly definition owns is left to that assembly. +func TestGlobPredefinedSourcesLeavesOutAScriptAnAssemblyDefinitionOwns(t *testing.T) { + projectRoot := t.TempDir() + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Foo", "Foo.asmdef"), `{"name":"Foo"}`) + writeProjectFiles(t, projectRoot, "Assets/Foo/Owned.cs", "Assets/Loose.cs") + + predefined := predefinedOwnersFor(t, projectRoot, "Foo").predefinedSources + + assertStrings(t, assemblyCSharp, predefined[assemblyCSharp], []string{"Assets/Loose.cs"}) + if len(predefined) != 1 { + t.Errorf("predefined = %v, want only %s", predefined, assemblyCSharp) + } +} + +// Verifies a folder Unity ignores by its name, ending in '~' or starting with '.', is not read. +func TestGlobPredefinedSourcesLeavesOutAHiddenFolder(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Assets/Foo~/X.cs", "Assets/.hidden/X.cs", "Assets/Kept.cs") + + predefined := predefinedOwnersFor(t, projectRoot).predefinedSources + + assertStrings(t, assemblyCSharp, predefined[assemblyCSharp], []string{"Assets/Kept.cs"}) +} + +// Verifies an assembly definition in Assets itself leaves no script to the predefined assemblies. +func TestGlobPredefinedSourcesReturnsNothingWhenAnAssemblyDefinitionSitsInAssets(t *testing.T) { + projectRoot := t.TempDir() + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Root.asmdef"), `{"name":"Root"}`) + writeProjectFiles(t, projectRoot, "Assets/X.cs", "Assets/Editor/Y.cs") + + if predefined := predefinedOwnersFor(t, projectRoot, "Root").predefinedSources; len(predefined) != 0 { + t.Errorf("predefined = %v, want none", predefined) + } +} + +// Verifies a script in a package folder is not given to a predefined assembly. +func TestGlobPredefinedSourcesLeavesOutPackageFolders(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Packages/com.example.loose/Loose.cs", "Assets/Kept.cs") + + predefined := predefinedOwnersFor(t, projectRoot).predefinedSources + + assertStrings(t, assemblyCSharp, predefined[assemblyCSharp], []string{"Assets/Kept.cs"}) +} + +// Verifies the folder of an .asmref naming an assembly definition this build did not produce gives +// its scripts to no assembly, as Unity attaches the folder before dropping the unbuilt assembly. +func TestGlobPredefinedSourcesStopsAtAReferenceToAnUnbuiltAssembly(t *testing.T) { + projectRoot := t.TempDir() + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Unbuilt", "Unbuilt.asmdef"), `{"name":"Unbuilt"}`) + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Extra", "Extra.asmref"), `{"reference":"Unbuilt"}`) + writeProjectFiles(t, projectRoot, "Assets/Extra/X.cs", "Assets/Kept.cs") + + predefined := predefinedOwnersFor(t, projectRoot).predefinedSources + + assertStrings(t, assemblyCSharp, predefined[assemblyCSharp], []string{"Assets/Kept.cs"}) +} + +// Verifies the glob of an assembly definition stops at the same folder, so its scripts are not +// compiled into the assembly around it either. +func TestGlobAssemblySourcesStopsAtAReferenceToAnUnbuiltAssembly(t *testing.T) { + projectRoot := t.TempDir() + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Outer", "Outer.asmdef"), `{"name":"Outer"}`) + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Unbuilt", "Unbuilt.asmdef"), `{"name":"Unbuilt"}`) + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Outer", "Extra", "Extra.asmref"), `{"reference":"Unbuilt"}`) + writeProjectFiles(t, projectRoot, "Assets/Outer/Outer.cs", "Assets/Outer/Extra/X.cs") + owners := predefinedOwnersFor(t, projectRoot, "Outer") + + sources, err := globAssemblySources(projectRoot, filepath.Join(projectRoot, "Assets", "Outer"), owners) + if err != nil { + t.Fatalf("globAssemblySources returned an error: %v", err) + } + + assertStrings(t, "sources", sources, []string{"Assets/Outer/Outer.cs"}) +} + +// Verifies the folder of an .asmref whose GUID resolves to nothing is read into the assembly around +// it, as Unity does. +func TestGlobPredefinedSourcesDescendsIntoAReferenceToNoAssembly(t *testing.T) { + projectRoot := t.TempDir() + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Extra", "Extra.asmref"), `{"reference":"GUID:0000000000000000"}`) + writeProjectFiles(t, projectRoot, "Assets/Extra/X.cs") + + predefined := predefinedOwnersFor(t, projectRoot).predefinedSources + + assertStrings(t, assemblyCSharp, predefined[assemblyCSharp], []string{"Assets/Extra/X.cs"}) +} + +// Verifies the folder of an .asmref naming an assembly no .asmdef declares is read into the assembly +// around it, as Unity does. +func TestGlobPredefinedSourcesDescendsIntoAReferenceByAnUnknownName(t *testing.T) { + projectRoot := t.TempDir() + writeFileAt(t, filepath.Join(projectRoot, "Assets", "Extra", "Extra.asmref"), `{"reference":"Nowhere"}`) + writeProjectFiles(t, projectRoot, "Assets/Extra/X.cs") + + predefined := predefinedOwnersFor(t, projectRoot).predefinedSources + + assertStrings(t, assemblyCSharp, predefined[assemblyCSharp], []string{"Assets/Extra/X.cs"}) +} + +// Verifies a script created outside every assembly definition after the build is compiled with +// Assembly-CSharp, after the recorded ones. +func TestRebuildSourcesPicksUpANewScriptOutsideEveryAssemblyDefinition(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Assets/Old.cs", "Assets/A/New.cs") + + sources := rebuildPredefinedSources(t, projectRoot, assemblyCSharp, "Assets/Old.cs") + + assertStrings(t, "sources", sources, []string{"Assets/Old.cs", "Assets/A/New.cs"}) +} + +// Verifies a new script in an editor folder goes to Assembly-CSharp-Editor and not to +// Assembly-CSharp. +func TestRebuildSourcesRoutesANewEditorScriptToTheEditorAssembly(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Assets/Old.cs", "Assets/Editor/New.cs") + + assertStrings(t, assemblyCSharpEditor, + rebuildPredefinedSources(t, projectRoot, assemblyCSharpEditor), []string{"Assets/Editor/New.cs"}) + assertStrings(t, assemblyCSharp, + rebuildPredefinedSources(t, projectRoot, assemblyCSharp, "Assets/Old.cs"), []string{"Assets/Old.cs"}) +} + +// Verifies a new script in a plugins folder goes to Assembly-CSharp-firstpass. +func TestRebuildSourcesRoutesANewPluginScriptToFirstpass(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Assets/Plugins/New.cs") + + assertStrings(t, assemblyCSharpFirstpass, + rebuildPredefinedSources(t, projectRoot, assemblyCSharpFirstpass), []string{"Assets/Plugins/New.cs"}) + assertStrings(t, assemblyCSharp, rebuildPredefinedSources(t, projectRoot, assemblyCSharp), []string{}) +} + +// Verifies a new script in an editor folder under plugins goes to Assembly-CSharp-Editor-firstpass. +func TestRebuildSourcesRoutesANewPluginEditorScriptToEditorFirstpass(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Assets/Plugins/Editor/New.cs") + + assertStrings(t, assemblyCSharpEditorFirstpass, + rebuildPredefinedSources(t, projectRoot, assemblyCSharpEditorFirstpass), + []string{"Assets/Plugins/Editor/New.cs"}) + assertStrings(t, assemblyCSharpEditor, rebuildPredefinedSources(t, projectRoot, assemblyCSharpEditor), []string{}) +} + +// Verifies a script the response file already recorded is listed once, in its recorded place. +func TestRebuildSourcesKeepsARecordedPredefinedSourceOnce(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Assets/B.cs", "Assets/A.cs", "Assets/C.cs") + + sources := rebuildPredefinedSources(t, projectRoot, assemblyCSharp, "Assets/B.cs", "Assets/A.cs") + + assertStrings(t, "sources", sources, []string{"Assets/B.cs", "Assets/A.cs", "Assets/C.cs"}) +} + +// Verifies a run refuses a script whose predefined assembly the last Unity build did not produce. +func TestSelectChangedAssembliesRefusesAScriptForAPredefinedAssemblyTheLastBuildLacks(t *testing.T) { + projectRoot := newPlanProject(t) + writeProjectFiles(t, projectRoot, "Assets/Editor/FirstEditorScript.cs") + + _, err := BuildCompilePlan(projectRoot, planDagDirectory, false) + + assertBuildRequired(t, err, "Assets/Editor/FirstEditorScript.cs", assemblyCSharpEditor, "did not produce") +} + +// Verifies the scripts of the folders Unity imports but never compiles are left out, in any case, +// while a deeper folder of the same name and the other platform folders under Plugins are not. +func TestGlobPredefinedSourcesLeavesOutTheFoldersUnityNeverCompiles(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, + "Assets/StreamingAssets/A.cs", "Assets/WebPlayerTemplates/B.cs", "Assets/webgltemplates/C.cs", + "Assets/Plugins/Android/D.cs", "Assets/Plugins/iOS/X.cs", "Assets/Game/StreamingAssets/X.cs") + + predefined := predefinedOwnersFor(t, projectRoot).predefinedSources + + assertStrings(t, assemblyCSharpFirstpass, predefined[assemblyCSharpFirstpass], []string{"Assets/Plugins/iOS/X.cs"}) + assertStrings(t, assemblyCSharp, predefined[assemblyCSharp], []string{"Assets/Game/StreamingAssets/X.cs"}) + if len(predefined) != 2 { + t.Errorf("predefined = %v, want only %s and %s", predefined, assemblyCSharp, assemblyCSharpFirstpass) + } +} + +// writeBuiltAssemblyCSharp adds an Assembly-CSharp the last build produced from Assets/Old.cs, with +// the source dated before the build. +func writeBuiltAssemblyCSharp(t *testing.T, projectRoot string) { + t.Helper() + writeFileAt(t, filepath.Join(projectRoot, planDagDirectory, assemblyCSharp+".rsp"), joinLines([]string{ + `-out:"` + planDagDirectory + `/` + assemblyCSharp + `.dll"`, + `-refout:"` + planDagDirectory + `/` + assemblyCSharp + `.ref.dll"`, + `/additionalfile:"` + planDagDirectory + `/` + assemblyCSharp + `.UnityAdditionalFile.txt"`, + `"Assets/Old.cs"`, + })) + writeFileAt(t, filepath.Join(projectRoot, planDagDirectory, assemblyCSharp+".UnityAdditionalFile.txt"), "") + writeFileAt(t, filepath.Join(projectRoot, planDagDirectory, assemblyCSharp+".dll"), "") + writeProjectFiles(t, projectRoot, "Assets/Old.cs") + buildTime := time.Now() + setModificationTime(t, filepath.Join(projectRoot, "Assets", "Old.cs"), buildTime.Add(-time.Hour)) + setModificationTime(t, filepath.Join(projectRoot, planDagDirectory, assemblyCSharp+".dll"), buildTime) +} + +// Verifies a plan compiles a script created outside every assembly definition with the predefined +// assembly the last build produced, after its recorded sources, rather than refusing the run. +func TestBuildCompilePlanPicksUpANewScriptOutsideEveryAssemblyDefinition(t *testing.T) { + projectRoot := newPlanProject(t) + writeBuiltAssemblyCSharp(t, projectRoot) + writeProjectFiles(t, projectRoot, "Assets/New.cs") + + plan, err := BuildCompilePlan(projectRoot, planDagDirectory, false) + if err != nil { + t.Fatalf("expected the plan to build, got error: %v", err) + } + + for _, unit := range plan.Units { + if unit.Assembly.AssemblyName == assemblyCSharp { + assertStrings(t, "sources", unit.Sources, []string{"Assets/Old.cs", "Assets/New.cs"}) + return + } + } + t.Fatalf("units = %v, want %s compiled for its new script", unitNames(plan), assemblyCSharp) +} + +// Verifies the sources of a predefined assembly are refused when Assets was never walked, rather +// than rebuilt without the scripts created since the build. +func TestRebuildSourcesRefusesAPredefinedAssemblyWhenAssetsWasNeverWalked(t *testing.T) { + projectRoot := t.TempDir() + writeProjectFiles(t, projectRoot, "Assets/Old.cs") + rsp := ResponseFile{AssemblyName: assemblyCSharp, Sources: []string{"Assets/Old.cs"}} + + if _, err := RebuildSources(projectRoot, rsp, nil, newOwnerIndexForProject(t, projectRoot)); err == nil { + t.Fatal("expected an unwalked Assets to be refused for a predefined assembly") + } +} diff --git a/cli/dispatcher/internal/compilecheck/source_rescue.go b/cli/dispatcher/internal/compilecheck/source_rescue.go index 3fe86ee58..303dca29d 100644 --- a/cli/dispatcher/internal/compilecheck/source_rescue.go +++ b/cli/dispatcher/internal/compilecheck/source_rescue.go @@ -20,14 +20,22 @@ type assemblyOwnerIndex struct { definitionDirectories map[string]bool referencesByDirectory map[string]string tree projectTree // the listing the run took, which the source globs read + // predefinedSources are the scripts under Assets no assembly definition owns, by the predefined + // assembly each belongs to; set once the index is built, because the walk needs the index. Nil + // means Assets was never walked, which RebuildSources refuses for a predefined assembly as a + // broken contract; an empty map means the walk found none. + predefinedSources map[string][]string } // newAssemblyOwnerIndex maps every directory that starts an assembly to what it starts. -// Why an .asmref naming an assembly this build never produced is left out: Unity has nothing to -// attach the folder to, so it compiles the folder's sources into the enclosing assembly instead. -// A package shipping a sample .asmref for an optional render pipeline is the ordinary case - the -// pipeline is not installed, the GUID resolves to nothing, and the sample compiles into the package's -// own editor assembly. +// Why an .asmref whose GUID resolves to nothing, or whose name matches no .asmdef, is left out: Unity +// has nothing to attach the folder to, so it compiles the folder's sources into the enclosing +// assembly instead. A package shipping a sample .asmref for an optional render pipeline is the +// ordinary case - the pipeline is not installed, the GUID resolves to nothing, and the sample +// compiles into the package's own editor assembly. +// Why an .asmref naming an .asmdef this build did not produce still owns its folder: Unity attaches +// the folder to that .asmdef first and only then drops the scripts of an assembly it does not build, +// so the folder's scripts belong to no assembly at all rather than to the one around it. func newAssemblyOwnerIndex( assemblyDefinitions map[string]AssemblyDefinition, references []AssemblyReference, context AssemblyContext, tree projectTree, @@ -40,7 +48,7 @@ func newAssemblyOwnerIndex( referencesByDirectory := make(map[string]string, len(references)) for _, reference := range references { name, resolved := resolveReferenceName(reference.Reference, context) - if !resolved || !context.BuiltAssemblies[name] { + if !resolved { continue } referencesByDirectory[filepath.Clean(reference.Directory)] = name diff --git a/cli/dispatcher/internal/compilecheck/source_set.go b/cli/dispatcher/internal/compilecheck/source_set.go index 2e737a507..2e8ea97c7 100644 --- a/cli/dispatcher/internal/compilecheck/source_set.go +++ b/cli/dispatcher/internal/compilecheck/source_set.go @@ -185,9 +185,24 @@ func RebuildSources( existing = append(existing, source) } + // Why only the predefined assemblies gain sources without an .asmdef: an assembly with no .asmdef + // that is not one of them is an .asmdef the index does not hold, so nothing says which folder it + // owns. + if asmdef == nil { + if predefinedAssemblyNames[rsp.AssemblyName] { + if owners.predefinedSources == nil { + return nil, fmt.Errorf( + "the scripts outside every assembly definition were never listed for %s", rsp.AssemblyName) + } + + return appendPredefinedSources(existing, owners.predefinedSources[rsp.AssemblyName]), nil + } + + return existing, nil + } // Why package-cache assemblies are not re-globbed: their contents are restored from the package // manager and never edited in place, so the walk would only cost time. - if asmdef == nil || isUnderPackageCache(projectRoot, asmdef.Directory) { + if isUnderPackageCache(projectRoot, asmdef.Directory) { return existing, nil } diff --git a/cli/dispatcher/internal/compilecheck/source_set_test.go b/cli/dispatcher/internal/compilecheck/source_set_test.go index 49ed10bd0..0471ca795 100644 --- a/cli/dispatcher/internal/compilecheck/source_set_test.go +++ b/cli/dispatcher/internal/compilecheck/source_set_test.go @@ -23,7 +23,7 @@ func newAssemblyProject(t *testing.T) string { // newOwnerIndexForProject indexes the .asmdef and .asmref files of a project the way one run does, // treating every assembly the project declares as one the last build produced - an .asmref naming -// an assembly that was never built owns nothing, which the unbuilt-target test covers on its own. +// an assembly that was not built is covered by the predefined-source tests on their own. func newOwnerIndexForProject(t *testing.T, projectRoot string) assemblyOwnerIndex { t.Helper() tree := walkProjectTree(projectRoot) diff --git a/docs/compile-check.md b/docs/compile-check.md index f914a4af8..c383cb750 100644 --- a/docs/compile-check.md +++ b/docs/compile-check.md @@ -105,11 +105,19 @@ references, scripting defines and analyzers. `compile-check` replays those respo their `.asmref` folders are re-globbed. A source the last build recorded for this assembly that no glob produces any more stops the run unless an `.asmref` still attaches its folder to this assembly: the walk up from the file to the first - folder holding an `.asmref` or an `.asmdef` says which assembly owns it today. An `.asmref` naming an assembly the build never produced — an - unresolvable reference, or one excluded by platform or package settings — attaches nothing, so - its folder is treated as an ordinary part of the assembly around it, which is what Unity compiles - it as. A package shipping a sample for a render pipeline the project does not install is the - ordinary case. Membership is compared without looking at any timestamp, because + folder holding an `.asmref` or an `.asmdef` says which assembly owns it today. An `.asmref` whose + reference resolves to no `.asmdef` in the project attaches nothing, so its folder is treated as an + ordinary part of the assembly around it, which is what Unity compiles it as. A package shipping a + sample for a render pipeline the project does not install is the ordinary case. An `.asmref` + naming an `.asmdef` the build left out by platform or package settings still owns its folder: + Unity attaches the folder to that assembly and then drops it, so the folder's sources go into no + assembly. Scripts under `Assets` that no `.asmdef` or `.asmref` owns are walked once per run and + given to the four predefined assemblies by Unity's own path rules: a folder named `Editor` + anywhere sends a script to an editor assembly, and `Assets/Plugins`, `Assets/Standard Assets`, + `Assets/Pro Standard Assets` and `Assets/iPhone Standard Assets` send it to a first-pass one, so + a new script there is compiled with the assembly Unity would put it in. Scripts in + `Assets/StreamingAssets`, `Assets/WebPlayerTemplates`, `Assets/WebGLTemplates` and + `Assets/Plugins/Android` are left out, as Unity imports them without compiling them. Membership is compared without looking at any timestamp, because moving a file carries its modification time along, and either an `.asmdef` or an `.asmref` can hand a folder's sources to a different assembly without editing a single `.cs` file. The recorded sources keep the order of the response file, and the ones added since the last Unity @@ -386,8 +394,13 @@ happened, and how long each assembly took. `docs/vibe-logs.md` lists their field - a Unity 6 build profile that overrides Player Settings or carries its own scripting defines: only the global ProjectSettings defines are compared, and a global define the build lacks is refused only when `ProjectSettings.asset` was saved after the last build. -- **The predefined assemblies** (`Assembly-CSharp` and friends) keep the source list of the last - build, so a brand-new `.cs` file outside any `.asmdef` is not compiled until Unity imports it. +- **A new script outside every `.asmdef` is given to one of the four predefined assemblies by + Unity's path rules.** When that assembly is one the last build did not produce - the first + editor script of a project that had none - the run stops with `COMPILE_CHECK_UNITY_BUILD_REQUIRED`. + Scripts in the folders Unity never compiles (`Assets/StreamingAssets`, `Assets/WebPlayerTemplates`, + `Assets/WebGLTemplates`, `Assets/Plugins/Android`) are left out. + A new script in a package folder that has no `.asmdef` is not seen, because where Unity puts + such a script differs between Unity versions. - **A dependent compile-check has never compiled is compiled, not narrowed by names, the first time a reference changes.** The names are read from the dependent's sources and from the generator output its last finished compile saved; a unit with analyzers and no saved output, whether it was @@ -453,6 +466,12 @@ settings of the last Unity build, so compiling against them would check a config no longer has. Run `uloop compile` once; the build moves the build-trace file forward, the next run records the settings as they are now, and `compile-check` works again. +**`COMPILE_CHECK_UNITY_BUILD_REQUIRED`: a script belongs to an assembly the last build did not produce** +The message reads `