Add opt-in partial (stop-after-pass) project evaluation - #14290
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in “stop-after-pass” evaluation mode so callers can request partial project evaluation (e.g., properties-only) and avoid later-pass costs like item globbing and target registration.
Changes:
- Introduces
ProjectEvaluationStageand threads it throughProjectOptions,Project,ProjectInstance, and the evaluator to stop evaluation after a requested pass. - Adds fail-fast exceptions when accessing later-pass state (items/targets/etc.) on partially-evaluated objects, and blocks building from partial
ProjectInstances. - Adds unit tests plus a proposed spec document describing the API and behavior.
Show a summary per file
| File | Description |
|---|---|
| src/Build/Resources/xlf/Strings.zh-Hant.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.zh-Hans.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.tr.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.ru.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.pt-BR.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.pl.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.ko.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.ja.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.it.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.fr.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.es.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.de.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/xlf/Strings.cs.xlf | Adds localized placeholders for new partial-evaluation OM error strings. |
| src/Build/Resources/Strings.resx | Adds new OM resource strings for partial-evaluation member/build guards. |
| src/Build/Microsoft.Build.csproj | Includes new ProjectEvaluationStage.cs in the build. |
| src/Build/Instance/ProjectInstance.cs | Threads stage through creation and adds stage-based member guards (plus exposes EvaluationStage). |
| src/Build/Evaluation/Evaluator.cs | Implements early returns after requested evaluation stage. |
| src/Build/Definition/ProjectOptions.cs | Adds ProjectOptions.EvaluationStage knob (default Full). |
| src/Build/Definition/ProjectEvaluationStage.cs | Defines the public ProjectEvaluationStage enum. |
| src/Build/Definition/ProjectCollection.cs | Updates cache lookup to consider evaluation stage and upgrade partial cached projects. |
| src/Build/Definition/Project.cs | Threads stage through initialization; adds stage-based member guards and exposes EvaluationStage. |
| src/Build/BackEnd/BuildManager/BuildRequestData.cs | Blocks building from a partially-evaluated ProjectInstance. |
| src/Build.UnitTests/Evaluation/PartialEvaluation_Tests.cs | Adds unit tests covering partial evaluation behavior. |
| documentation/specs/proposed/partial-evaluation.md | Adds proposed spec describing the API, behavior, and caching expectations. |
Copilot's findings
- Files reviewed: 24/24 changed files
- Comments generated: 5
There was a problem hiding this comment.
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Expert Review — PR #14290: Add opt-in partial (stop-after-pass) project evaluation
The overall design is sound — the ProjectEvaluationStage knob, the early-return placement inside profiler scopes, the FinishEvaluation() call on all exit paths, the cache-upgrade-in-place semantic, and the fail-fast guards are all well-conceived. Five issues need resolution before merge; two are blocking.
Summary Table
| # | Dimension | Verdict | Severity | Location |
|---|---|---|---|---|
| 1 | Concurrency & Thread Safety | ❌ ISSUE | BLOCKING | ProjectCollection.cs:2027-2030 |
| 2 | Evaluation Model Integrity | ❌ ISSUE | MODERATE | Evaluator.cs:779-816 |
| 3 | API Surface — Enum Gap | ❌ ISSUE | MODERATE | ProjectEvaluationStage.cs:56 |
| 4 | Error Message Quality | ❌ ISSUE | MODERATE | Strings.resx:1575 |
| 5 | Test Coverage / Quality | ❌ ISSUE | MAJOR | PartialEvaluation_Tests.cs:15,53 |
| 6 | Backwards Compatibility | ✅ LGTM | — | — |
| 7–24 | All other dimensions | ✅ LGTM | — | — |
Finding 1 — BLOCKING: Race condition in GetMatchingProjectIfAny concurrent upgrade
src/Build/Definition/ProjectCollection.cs, lines 2027–2030.
Two concurrent ProjectCollection.LoadProject() callers find the same partial project under _loadedProjects lock, both release the lock, both read EvaluationStage < Full == true, and both call match.ReevaluateIfNecessary(). Because ProjectImpl has no per-project lock, both threads write _evaluationStage = Full / _explicitlyMarkedDirty = true unsynchronized, then both enter Reevaluate() → Evaluator.Evaluate(_data, ...) on the same _data instance concurrently. Two threads calling _data.InitializeForEvaluation() simultaneously and then appending items/properties to the same mutable collections produces corrupt evaluation state (duplicate entries, torn collections). See inline comment for the exact interleaving and the double-checked locking fix.
Finding 2 — MODERATE: EvaluatePass4Stop is orphaned when stopping at UsingTasks
src/Build/Evaluation/Evaluator.cs, line 816.
EvaluatePass4Start fires before pass-4 work (line 779); the early-return for UsingTasks stage exits at line 795; EvaluatePass4Stop is placed at line 816 — after that return. Every other pass emits its Stop before its early-return check; Pass 4 alone does not. ETW-based profilers (PerfView, dotnet-trace) see an orphaned Pass4Start and report corrupted durations.
Fix: Move MSBuildEventSource.Log.EvaluatePass4Stop(projectFile) to immediately after line 790 (closing brace of the using TrackPass block), before the guard at 792.
Finding 3 — MODERATE: Enum gap silently breaks Targets access for values 5–2147483646
src/Build/Definition/ProjectEvaluationStage.cs, line 56.
(ProjectEvaluationStage)5 passes through ProjectOptions without validation. In the evaluator none of the four <= early-return guards fire (5 > 4), so all passes including target registration complete. But _evaluationStage is stored as 5, and the guard VerifyThrowEvaluationStageReached(Full, "Targets") checks 5 < int.MaxValue == true — it throws even though targets are fully populated. Add an Enum.IsDefined validation in the EvaluationStage setter, or map any value > UsingTasks and < Full to Full.
Finding 4 — MODERATE: Error message omits the minimum required stage
src/Build/Resources/Strings.resx, line 1575.
OM_PartialEvaluationMemberUnavailable tells the user their current stage but not which stage they need. "Re-evaluate with a later evaluation stage" is under-specified — Items or UsingTasks still won't help if the member requires Full. The requiredStage parameter is already available inside VerifyThrowEvaluationStageReached; pass it as {2} and add "at least the '{2}' stage" to the message.
Finding 5 — MAJOR: Test-quality issues in PartialEvaluation_Tests.cs
src/Build.UnitTests/Evaluation/PartialEvaluation_Tests.cs.
ProjectCollectionleak:OptionsFor()allocates a newProjectCollectionper call (7+ call sites) and none are disposed. Useusing ProjectCollection collection = new ProjectCollection()locally in each test.- Manual
try/finallyfor temp files: three tests usePath.GetTempFileName()+ manual cleanup instead ofTestEnvironment.CreateFile(). Useusing TestEnvironment env = TestEnvironment.Create(_output)andenv.CreateFile(...). - Missing
ITestOutputHelper: the class has no constructor injection; diagnostic output is lost in CI. Addprivate readonly ITestOutputHelper _output; public PartialEvaluation_Tests(ITestOutputHelper output) => _output = output;. #nullable disable: new files must not suppress nullable analysis per repo conventions. Remove and annotate as needed.
Positive notes
- The
Full = int.MaxValuetrick elegantly ensures comparisons work without needing a dedicated "max" sentinel — clever. FinishEvaluation()is called on every early-return path — correct.- All
BuildRequestDataconstructor overloads chain to the single guarded overload — no bypasses. - The cache invariant (one slot per key, upgraded in place rather than duplicated) is the right design.
CreateProjectInstance()upgrading a partial project to Full before snapshotting is the right behaviour.- Spec document is clear and thorough.
Generated by Expert Code Review (on open) for #14290 · 1.9K AIC · ⊞ 30.4K
Comments that could not be inline-anchored
src/Build/Definition/ProjectCollection.cs:2030
[BLOCKING] Race condition — concurrent upgrade of the same partial project
Two threads can both call ProjectCollection.LoadProject(path) while the cache holds a partial project. Both capture match inside the lock, then both release the lock, both observe match.EvaluationStage < requestedStage == true, and both call match.ReevaluateIfNecessary() concurrently.
ReevaluateIfNecessary (public override) sets _evaluationStage = Full and _explicitlyMarkedDirty = true without any per…
src/Build.UnitTests/Evaluation/PartialEvaluation_Tests.cs:15
[MODERATE] Three test-quality issues in this new file
1. #nullable disable in a new file — per repo conventions, new files must not suppress nullable reference-type analysis. Remove #nullable disable and add ? annotations where a reference type is intentionally nullable.
2. Manual try/finally for temp files — the three tests that write a real file (CacheUpgrade_PartialThenFull_ProjectCollection, CacheHit_LowerStageRequest_ReturnsCachedProject, `ReevaluateIfNecessary_On…
src/Build.UnitTests/Evaluation/PartialEvaluation_Tests.cs:53
[MAJOR] ProjectCollection is leaked in every test that calls OptionsFor()
OptionsFor() constructs a new ProjectCollection on every call and nothing ever disposes it. ProjectCollection holds file-system watchers and event-handler registrations. This helper is called from at least 7 test sites in this file.
Fix: Remove the static OptionsFor() helper. Each test that needs a ProjectCollection should declare and dispose it explicitly:
using ProjectCollection collec…
</details>
<details><summary>src/Build/Resources/Strings.resx:1575</summary>
**[MODERATE] Error message doesn't tell the user which stage they actually need**
The message tells users *what stage evaluation stopped at* (`{1}`), but not *what stage they need* to access the requested member. For a user who reads `Targets` after stopping at `Properties`, the message says "Re-evaluate the project with a later evaluation stage" — but `Items` and `UsingTasks` still won't work; only `Full` does.
`requiredStage` is already available as the first parameter of `VerifyThrowEvalua…
</details>
<details><summary>src/Build/Evaluation/Evaluator.cs:816</summary>
**[MODERATE] `EvaluatePass4Stop` is never emitted when stopping at `UsingTasks`**
`EvaluatePass4Start` fires at line 779, the pass-4 work completes by line 790, but `EvaluatePass4Stop` is placed here — **after** the early-return guard at lines 792–796. Any caller with `EvaluationStage = ProjectEvaluationStage.UsingTasks` exits at line 795 and `EvaluatePass4Stop` is never called.
This creates an unpaired ETW event. PerfView, `dotnet-trace` (with MSBuild provider), and any custom profiler that …
</details>
<details><summary>src/Build/Definition/ProjectEvaluationStage.cs:56</summary>
**[MODERATE] Enum gap between `UsingTasks = 4` and `Full = int.MaxValue` causes false-positive guards**
The gap of 2,147,483,642 unnamed values (5 through `int.MaxValue − 1`) is silently accepted by `ProjectOptions.EvaluationStage`. When any such value is stored:
1. **Evaluator**: none of the `<= Properties/ItemDefinitions/Items/UsingTasks` checks fire (e.g. `5 > 4`), so **all passes including target registration run** — the project is fully evaluated.
2. **Guards**: `VerifyThrowEvaluationSta…
</details>Implements #14288. Adds ProjectEvaluationStage and ProjectOptions.EvaluationStage so callers that only need early-pass data (e.g. a single property) can stop evaluation after any pass instead of running a full evaluation. - Evaluator returns early after the requested pass (Properties, ItemDefinitions, Items, UsingTasks) while still calling FinishEvaluation. - Stage is threaded through Project/ProjectImpl and ProjectInstance construction and the From* factory methods; both expose EvaluationStage. - Reading not-yet-computed state (Items/GetItems/Targets/DefaultTargets/ AllEvaluatedItems/ItemsIgnoringCondition) throws InvalidOperationException. - Cache is stage-aware: ProjectCollection reuses a cached project only if cachedStage >= requestedStage and upgrades a partial one in place; ReevaluateIfNecessary()/CreateProjectInstance() upgrade to Full; building a partial ProjectInstance throws. - Default behavior (Full) is unchanged; feature is additive/opt-in. - Adds unit tests and a spec doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Emit EvaluatePass4Stop before the UsingTasks-stage early return so ETW/ profiling start/stop pairs stay balanced; remove the old misplaced duplicate. - Add a fail-fast VerifyThrowEvaluationStageReached guard to the ItemDefinitions getter on both Project and ProjectInstance so it throws when evaluation stopped at Properties, matching the ProjectEvaluationStage contract. - Extend tests: assert ItemDefinitions is unavailable at Properties stage on both types, and add a positive ItemDefinitions-stage test. - Update the spec to list ItemDefinitions among the guarded members. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t quality
Finding 1 (BLOCKING) - concurrent upgrade race: move the partial->Full
upgrade of a cached project out of the lock-free GetMatchingProjectIfAny
lookup and into LoadProject, where it runs under the existing per-path
load lock so concurrent loads of the same path cannot re-evaluate the
same project instance simultaneously. GetMatchingProjectIfAny is now a
pure lookup again.
Finding 3 - enum gap: validate ProjectOptions.EvaluationStage with
Enum.IsDefined so undefined values (the 5..int.MaxValue-1 gap, 0,
negatives) throw ArgumentOutOfRangeException instead of silently running
every pass while the guards still report state as unavailable.
Finding 4 - error message: OM_PartialEvaluationMemberUnavailable now also
names the minimum required stage ({2}); both guard helpers pass it. xlf
regenerated.
Finding 5 - test quality: inject ITestOutputHelper, use TestEnvironment
(shared/disposed ProjectCollection + CreateFile instead of manual temp
file try/finally), remove #nullable disable. Added an EvaluationStage
validation theory.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a1a7e62 to
c830c60
Compare
There was a problem hiding this comment.
Straightforward implementation and the use case/benefit is clear to me.
Main question I have is one of design - today the 'state' of the Project/Instance is held and used to validate property access, which adds a small-but-nonzero amount of overhead to each access. A Type-based modeling of the evaluation state would allow disallowed members to be hardcoded to throw, which could sidestep this problem. I'm thinking of like private inner classes that all implement the same Project/Instance signature but can be swapped out so that the lifetimes of the various properties are correct-by-construction:
private class PropertiesLifetimeProject(...) {
public override IDictionary<string, ProjectItemDefinition> ItemDefinitions => ErrorUtilities.ThrowInvalidOperation("OM_PartialEvaluationMemberUnavailable", memberName, _evaluationStage, ProjectEvaluationStage.ItemDefinitions);
}
private class ItemDefinitionsLifetimeProject(...) {
public override IDictionary<string, ProjectItemDefinition> ItemDefinitions => _data.ItemDefinitions;
public override ICollection<ProjectItem> Items => ErrorUtilities.ThrowInvalidOperation("OM_PartialEvaluationMemberUnavailable", memberName, _evaluationStage, ProjectEvaluationStage.Items);
}and so on.
|
That totally makes sense and I considered that as well. Let me file a follow-up issue for that (as a perf optimization) and merge this in meanwhile. |
…4296) ### Context Builds on the opt-in partial (stop-after-pass) evaluation added in #14290. When `msbuild -getProperty:Foo` / `-getItem:Bar` is invoked **without a target**, the CLI currently runs a full project evaluation even though the requested data is produced by an early evaluation pass. This PR stops evaluation as soon as that data is available: - `-getProperty` (only) → stop after the **Properties** pass - `-getItem` (or both) → stop after the **Items** pass This skips the later using-tasks and target-registration passes. ### Change wave Gated behind change wave **18.10** (already present in `main`). Setting `MSBUILDDISABLEFEATURESFROMVERSION=18.10` restores the historical full-evaluation behavior. ### Measured impact Measured on a simple solution — a `dotnet new console` referencing a `dotnet new classlib` (with real code + a `Newtonsoft.Json` package reference), which registers **547 targets** on full evaluation. Numbers are the median of interleaved evaluations via the object model (Debug engine, so treat the *ratios* as the signal, not absolute values). Wall-clock reproduced across multiple runs and both projects; allocations (`GC.GetTotalAllocatedBytes(precise)`) were essentially deterministic across runs. | Path (stage) | Wall-clock vs full | Allocations vs full | |---|---|---| | `-getProperty` (stop after Properties) | **~15% faster** | **~22% less** | | `-getItem` (stop after Items) | **~7% faster** | **~10% less** | The `-getProperty` win skips the item-definition, item, using-task and target-registration passes. The `-getItem` win still skips the using-task and target-registration passes — a real, reproducible saving. Allocation savings exceed the wall-clock savings because the skipped passes (target registration in particular) are allocation-heavy. The remaining fixed cost that neither path avoids is shared import/property processing that any evaluation must pay. ### Tests - Change-wave opt-out test in `XMake_Tests.cs`: a project whose only error is in the targets pass succeeds for `-getProperty` (wave on) and fails when the wave is disabled (full evaluation). - A `PartialEvaluationBenchmark` (BenchmarkDotNet) comparing full vs partial evaluation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…14349) Reverts the change from #14274, which injected `ExcludeRestorePackageImports=true` into the implicit restore global properties (behind change wave 18.10) so that NuGet's restore re-invocation would reuse MSBuild's initial evaluation instead of forcing a second one. ### Why revert The change breaks restore for projects that have a **nonexistent `<ProjectReference>`**. Once the top-level restore evaluation shares the same global property set that NuGet's inner `_GenerateRestoreProjectPathWalk` invocation uses, that walk runs in the *same* project instance and its unfiltered `_RestoreProjectPathItems` (which contains the raw `ProjectReference` paths, including missing ones) leaks into `_GenerateRestoreGraph`'s `_GenerateRestoreGraphProjectEntry` MSBuild call. That call does not set `SkipNonexistentProjects`, so it fails with `MSB3202` instead of skipping the missing project. ``` NuGet.targets(571,5): error MSB3202: The project file "...\TestLibrary\TestLibrary.csproj" was not found. ``` This regressed the dotnet/sdk test `ItCanTestAMultiTFMProjectWithImplicitRestore` — see dotnet/sdk#55245 for the full root-cause analysis and repro. ### Details - Only the implicit-restore (`-restore` switch / `ExecuteRestore`) path was affected; explicit `dotnet restore` / `-t:Restore` never added the property. - Reproduced with `dotnet build -t:Restore /p:ExcludeRestorePackageImports=true` on a multi-TFM project with a dangling `ProjectReference`: without the property restore succeeds (missing project skipped gracefully); with it, `MSB3202`. ### What this PR does - Removes the `ExcludeRestorePackageImports=true` injection in `XMake.ExecuteRestore`, replacing it with a comment documenting **why** the property must not be set there. - Removes the `ExcludeRestorePackageImports` / `ExcludeRestorePackageImportsValue` constants and the two related unit tests. - Removes the 18.10 ChangeWaves.md bullet for this feature. - **Retains** change wave `18.10` itself — it is still used by the `-getProperty`/`-getItem` evaluation change (#14290). --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Implements #14288.
Summary
Adds an opt-in partial (stop-after-pass) evaluation model. Callers that only need data produced by an early evaluation pass (most commonly a single property, e.g. the SDK's
ReleasePropertyProjectLocatorreadingPublishRelease/PackRelease) can now stop evaluation after any pass instead of forcing a full evaluation.A new
ProjectEvaluationStageenum andProjectOptions.EvaluationStageknob select how far evaluation proceeds:PropertiesItemDefinitionsItemsUsingTasksFull(default)Property values are final after pass 1, so a stop-at-
Propertiesevaluation yields property values identical to a full evaluation.Changes
Evaluator.cs): early-return after the requested pass, inside the profiler scopes;FinishEvaluation()is still called.Project/ProjectImplandProjectInstanceconstruction,Initialize, andFromFile/FromProjectRootElement/FromXmlReader. Both types exposeEvaluationStage.Items,GetItems,ItemsIgnoringCondition,AllEvaluatedItems,Targets,DefaultTargets) throwsInvalidOperationExceptionnaming the member and stage. Properties andInitialTargetsremain valid fromPropertiesonward.ProjectCollectionreuses a cached project only ifcachedStage >= requestedStageand upgrades a partial cached project in place; publicReevaluateIfNecessary()andCreateProjectInstance()upgrade a partial project toFull; constructingBuildRequestDatafrom a partialProjectInstancethrows.Full) is unchanged — the feature is purely additive/opt-in.PartialEvaluation_Tests.cs(10 tests) and a spec doc.Perf
File-heavy project (500 source files, ~200 properties, 50 targets), Debug engine:
Stop-at-Properties saves ~46% of evaluation wall-clock. Complements a shared
EvaluationContext(see dotnet/sdk#55193) — the two levers stack.Testing
PartialEvaluation_Tests— 10 tests, all pass.Evaluator_Tests(153),ProjectInstance*(55),EvaluationContext(53).Microsoft.Build(net10.0 + net472), 0 warnings / 0 errors.