Use partial evaluation for -preprocess, schema validation, and .wdproj dependency scan - #14326
Use partial evaluation for -preprocess, schema validation, and .wdproj dependency scan#14326ViktorHofer wants to merge 2 commits into
Conversation
…j dependency scan Builds on the opt-in partial (stop-after-pass) evaluation added in #14290 and adopted by -getProperty/-getItem in #14296. Extends it to three more consumers that only read early-pass data and never build: - `-preprocess`/`-pp` (SaveLogicalProject): only walks the import closure, resolved in pass 1 -> stops after the Properties pass. - `-validate` schema validation: the loaded project is used only for ToolsVersion -> stops after the Properties pass (net472 only). - SolutionProjectGenerator.ScanProjectDependencies (legacy .wdproj web deployment projects): reads only ProjectDependency items and the SourceWebProject property -> stops after the Items pass. All three are gated behind change wave 18.10, reverting to full evaluation when MSBUILDDISABLEFEATURESFROMVERSION=18.10 is set. Adds a PreprocessUsesPartialEvaluation change-wave test mirroring the existing -getProperty test, and documents the new consumers in ChangeWaves.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 396c7e9b-3bcc-42b3-910b-35e039562e5c
There was a problem hiding this comment.
Pull request overview
Extends the opt-in partial (stop-after-pass) evaluation feature (ChangeWave 18.10) to additional MSBuild consumers that only need early-pass data, reducing unnecessary evaluation work while preserving an opt-out path via MSBUILDDISABLEFEATURESFROMVERSION.
Changes:
- Switch
-validate(schema validation) and-preprocessto stop evaluation after the Properties pass when ChangeWave 18.10 is enabled. - Update
.wdprojdependency scanning inSolutionProjectGenerator.ScanProjectDependenciesto stop after the Items pass when ChangeWave 18.10 is enabled. - Add a new change-wave test covering
-preprocessand update ChangeWaves documentation.
Show a summary per file
| File | Description |
|---|---|
| src/MSBuild/XMake.cs | Uses ProjectOptions.EvaluationStage to avoid full evaluation for schema validation and preprocess under wave 18.10. |
| src/MSBuild.UnitTests/XMake_Tests.cs | Adds a change-wave regression test verifying -preprocess benefits from partial evaluation. |
| src/Build/Construction/Solution/SolutionProjectGenerator.cs | Uses partial evaluation (stop-after Items) for .wdproj dependency scanning under wave 18.10. |
| documentation/wiki/ChangeWaves.md | Documents additional 18.10 consumers and fixes an existing formatting/typo issue. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Link ChangeWaves bullets to the PRs that enabled the behavior (#14296 for the -getProperty/-getItem CLI adoption, #14326 for the new consumers). - Capture diagnostics in PreprocessUsesPartialEvaluation: pass _output to TestEnvironment.Create and RunnerUtilities.ExecMSBuild. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 396c7e9b-3bcc-42b3-910b-35e039562e5c
|
/review |
|
✅ Expert Code Review (command) completed successfully! Caution agentic threat detected Expert reviewer subagent posted review on PR #14326. Main finding: missing change-wave tests for -validate and ScanProjectDependencies consumers. Implementation itself is correct. Subagent handled all safe-output writes. |
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.
Review Summary — PR #14326 (Partial evaluation for -preprocess, schema validation, .wdproj)
Positive observations
- ChangeWave 18.10 gates all three changes correctly; opt-out via
MSBUILDDISABLEFEATURESFROMVERSION=18.10is preserved. - ChangeWaves.md is updated and the pre-existing typo (
14290)arget)→14296) is fixed. EvaluationStagechoices are correct:Propertiesis sufficient for preprocessing (import closure is resolved in pass 1) and for schema validation (onlyToolsVersionis needed);Itemsis correct for the.wdprojscan (ProjectDependencyitems +SourceWebProjectproperty).Project.FromFile+ProjectOptionsis the right API; it avoids adding the project to the collection under the oldLoadProjectpath, andUnloadProjectis called where it was before.- The
PreprocessUsesPartialEvaluationtest is well-structured and tests both wave-enabled and wave-disabled paths, matching the pattern set byGetPropertyWithoutTargetUsesPartialEvaluation.
Blocking / Major issue
Missing test coverage for schema validation and .wdproj dependency scan (see inline comment on XMake_Tests.cs line 933).
The PR introduces partial-evaluation changes to three consumers but adds a test for only one of them. The other two — schema validation (-validate, .NET Framework only) and ScanProjectDependencies (.wdproj) — have no test verifying:
- that partial evaluation is used when wave 18.10 is enabled, and
- that full evaluation is restored when the wave is disabled.
The .wdproj path is especially important to cover because it is the only consumer that stops at Items (not Properties) and it lives in a different assembly (SolutionProjectGenerator), so it needs its own integration test.
Minor observations
ScanProjectDependenciescreatesProject.FromFileinside atry/catchbut never callsUnloadProjectbefore or after the exception path. The oldnew Project(...)call had the same behaviour, so this is unchanged, but it could be noted as a follow-up.- The PR description says schema validation partial evaluation is "net472 only" — this is correct because the entire block is inside
#if FEATURE_XML_SCHEMA_VALIDATION, but the code comment inside the block does not mention this conditional compilation guard. A brief note would help future readers.
Generated by Expert Code Review (command) for #14326 · 245.1 AIC · ⊞ 30.1K
Comment /review to run again
|
These code paths aren't worth touching and they all use Project and not ProjectInstance. |
Context
Builds on the opt-in partial (stop-after-pass) evaluation added in #14290 and adopted by
-getProperty/-getItemin #14296. This extends partial evaluation to three more consumers that only read early-pass data and never build a project.Changes
-preprocess/-pp(SaveLogicalProject)-validateschema validationToolsVersiononlySolutionProjectGenerator.ScanProjectDependencies(legacy.wdproj)ProjectDependencyitems +SourceWebProjectpropertyAll three skip the later using-tasks and target-registration passes.
Change wave
Gated behind change wave 18.10 (same wave as #14296). Setting
MSBUILDDISABLEFEATURESFROMVERSION=18.10restores the historical full-evaluation behavior.Tests
PreprocessUsesPartialEvaluationchange-wave test inXMake_Tests.cs, mirroring the existing-getPropertytest: a project whose only error is in the targets pass preprocesses successfully with the wave on, and fails (full evaluation) when the wave is disabled. Passes on net10.0 + net472.Microsoft.Buildand theMSBuildCLI (net10.0 + net472), 0 warnings / 0 errors.Docs
ChangeWaves.md(and fixes a stray typo on the existing 18.10 entry).