Expose import tree as MSBuildImportedProject items#13681
Conversation
drewnoakes
force-pushed
the
imported-project-items
branch
from
80ba8d1 to
90ef9fe
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in mechanism (MSBuildProvideImportedProjects=true) to expose the evaluated import closure as regular MSBuild items (MSBuildImportedProject) on ProjectInstance, enabling targets/tasks to query the import graph without introducing new public API.
Changes:
- Introduces a new internal constant for the opt-in property name (
MSBuildProvideImportedProjects). - Synthesizes
MSBuildImportedProjectitems from the evaluated import closure duringProjectInstanceconstruction, includingImportingProjectPathandSdkmetadata. - Adds unit tests validating item creation behavior, metadata, and target visibility.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/Framework/Constants.cs | Adds an internal constant for the opt-in property name. |
| src/Build/Instance/ProjectInstance.cs | Creates MSBuildImportedProject items from the import closure when opted in. |
| src/Build.UnitTests/Instance/ProjectInstance_ImportedProjectItems_Tests.cs | New tests validating opt-in behavior and metadata for standard and SDK imports. |
Design-Level Observations
Note 🔒 Integrity filter blocked 1 itemThe following item were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
| # | Dimension | Verdict |
|---|---|---|
| 3 | Performance & Allocation Awareness | 🟡 1 MODERATE |
| 18 | Documentation Accuracy | 🟡 1 NIT |
| 22 | Correctness & Edge Cases | 🟡 2 (1 MAJOR, 1 MODERATE) |
✅ 21/24 dimensions clean.
- Correctness — unescaped path passed to
includeEscapedparameter; paths with%chars will be corrupted - Correctness — potential NRE if
SdkResult.SdkReferenceis null - Performance — per-iteration array allocation in import closure loop
- Documentation — constant XML doc says "during evaluation" but occurs during
ProjectInstancecreation
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #13681
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (on open) for issue #13681 · ● 26.1M
drewnoakes
force-pushed
the
imported-project-items
branch
from
90ef9fe to
6c88efa
Compare
drewnoakes
force-pushed
the
imported-project-items
branch
from
6c88efa to
e5985a5
Compare
There was a problem hiding this comment.
I'm not convinced this is the right place for the code. From the conversation with @rainersigwald, my understanding was that we'd place this in the evaluation stage: at the start of the items pass, after the property evaluation pass. This change runs after evaluation, during ProjectInstance construction, which means the synthesized items only appear on ProjectInstance, not on Project. I am not entirely sure how important this distinction is for your use case, but I can imagine there being cases (edge or otherwise) where it could make a difference.
There was a problem hiding this comment.
The Project OM already exposes the full import tree via Project.Imports (IList<ResolvedImport> with importing element, imported project, SDK result). Design-time tooling and language services that work with Project objects have no gap here.
The gap is for targets and tasks during build execution. Tasks don't have access to Project or ProjectInstance. They only see items, properties, and IBuildEngine. Regular MSBuild items are the mechanism by which data flows to them. Synthesizing MSBuildImportedProject items on ProjectInstance is the natural way to bridge this, since those items flow to targets and tasks automatically and serialize to out-of-proc worker nodes for free.
Moving this into the evaluator would add significant complexity (the generic Evaluator<P, I, M, D> requires item creation through IItemFactory backed by ProjectItemElement XML), and wouldn't benefit any consumer. Project users already have richer data, and targets/tasks would see the same items either way.
So, I think this should be left as-is.
There was a problem hiding this comment.
Makes sense! The pr looks good to me then. We always can move the code generating the items it to evaluation if anything would not work with current approach, but I think we should be good. @rainersigwald what do you think about it, can you please give a second review to the PR?
There was a problem hiding this comment.
I don't like the idea that the items project differ when looking at a Project vs a ProjectInstance. Is there any precedent for that?
There was a problem hiding this comment.
Fair enough. I changed the code to compute during evaluation and updated the tests. PTAL.
drewnoakes
force-pushed
the
imported-project-items
branch
from
ad9566a to
68a48af
Compare
When the MSBuild property `MSBuildProvideImportedProjects` is set to `true`, the engine synthesizes `MSBuildImportedProject` items from the import closure during ProjectInstance construction. Each item's identity is the full path of the imported project file, with metadata: - `ImportingProjectPath` — the file that contains the `<Import>` element - `Sdk` — the SDK name if this was an SDK-resolved import ```xml <PropertyGroup> <MSBuildProvideImportedProjects>true</MSBuildProvideImportedProjects> </PropertyGroup> <Target Name="ShowImports"> <Message Text="%(MSBuildImportedProject.Identity) imported by %(MSBuildImportedProject.ImportingProjectPath)" /> </Target> ``` Items are regular MSBuild items, so they naturally serialize to out-of-proc worker nodes and are available to any target or task. No new API surface is required. Projects that don't set the property pay zero cost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use EscapedFullPath for include and ImportingProjectPath metadata - Use EscapingUtilities.Escape(FullPath) for definingFileEscaped - Add null-conditional on SdkReference (SdkResult?.SdkReference?.Name) - Reuse metadata list with Clear() instead of allocating per iteration - Early-continue for root project (clarity) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix doc comment: 'during evaluation' -> 'during ProjectInstance creation' - Add test verifying MSBuildProvideImportedProjects works as a global property Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Document MSBuildProvideImportedProjects property and MSBuildImportedProject items in Built-in-Properties.md - Add MSBuildProvideImportedProjects property to Microsoft.Build.CommonTypes.xsd for tooling completion and documentation - Add MSBuildImportedProject item type with ImportingProjectPath and Sdk metadata to Microsoft.Build.CommonTypes.xsd Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address AR-May's review feedback: - Extract SynthesizeImportedProjectItems() from CreateImportsSnapshot - Add constants for item type and metadata names in Constants.cs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address rainersigwald's feedback: items should be consistent between Project and ProjectInstance. Synthesis now happens in the evaluator's items pass (Pass 3), so MSBuildImportedProject items appear on both Project and ProjectInstance. The evaluator collects resolved imports during the depth-first pass, then at the start of item evaluation checks MSBuildProvideImportedProjects and creates items via the item factory with disconnected XML elements for metadata backing. Tests verify items exist on both Project and ProjectInstance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
drewnoakes
force-pushed
the
imported-project-items
branch
from
134e344 to
3806108
Compare
Replaces #13680
When the MSBuild property
MSBuildProvideImportedProjectsis set totrue, the engine synthesizesMSBuildImportedProjectitems from the import closure during ProjectInstance construction.Each item's identity is the full path of the imported project file, with metadata:
ImportingProjectPath— the file that contains the<Import>elementSdk— the SDK name if this was an SDK-resolved importItems are regular MSBuild items, so they naturally serialize to out-of-proc worker nodes and are available to any target or task. No new API surface is required. Projects that don't set the property pay zero cost.