Skip to content

Expose import graph to tasks#13680

Closed
drewnoakes wants to merge 1 commit intodotnet:mainfrom
drewnoakes:import-tree
Closed

Expose import graph to tasks#13680
drewnoakes wants to merge 1 commit intodotnet:mainfrom
drewnoakes:import-tree

Conversation

@drewnoakes
Copy link
Copy Markdown
Member

Tasks currently have no way to discover which files were imported during project evaluation. The data exists internally during evaluation but was never surfaced through any task-facing API.

This change adds an EngineServices.ImportEdges property that returns a structured list of import edges — each edge represents one <Import> element connecting an importing project file to the file it resolved to, optionally carrying the SDK name for SDK-resolved imports.

A new ProjectImportEdge readonly struct in Microsoft.Build.Framework carries ImportedProjectPath, ImportingProjectPath, and SdkName. Each imported file appears at most once (first occurrence in depth-first evaluation order), so the collection forms a tree rather than a DAG.

Usage

Tasks can use pattern matching — the null default naturally handles older engines and out-of-proc nodes where the data was not serialized:

if (BuildEngine is IBuildEngine10 { EngineServices.ImportEdges: IReadOnlyList<ProjectImportEdge> edges })
{
    // use edges
}

Opt-in serialization for out-of-proc nodes

On the in-process node the data is always available. On out-of-process worker nodes the data is only available if the project opts in, because serializing the import graph across nodes has a cost we don't want to impose on every build:

<PropertyGroup>
  <MSBuildProvideImportGraph>true</MSBuildProvideImportGraph>
</PropertyGroup>

When this property is not set, serialization is skipped and ImportEdges returns null on the worker node. This means zero additional IPC cost for builds that don't need the import graph.

Tasks currently have no way to discover which files were imported during project evaluation. The data exists internally during evaluation but was never surfaced through any task-facing API.

This change adds an `EngineServices.ImportEdges` property that returns a structured list of import edges — each edge represents one `<Import>` element connecting an importing project file to the file it resolved to, optionally carrying the SDK name for SDK-resolved imports.

A new `ProjectImportEdge` readonly struct in `Microsoft.Build.Framework` carries `ImportedProjectPath`, `ImportingProjectPath`, and `SdkName`. Each imported file appears at most once (first occurrence in depth-first evaluation order), so the collection forms a tree rather than a DAG.

## Usage

Tasks can use pattern matching — the `null` default naturally handles older engines and out-of-proc nodes where the data was not serialized:

```csharp
if (BuildEngine is IBuildEngine10 { EngineServices.ImportEdges: IReadOnlyList<ProjectImportEdge> edges })
{
    // use edges
}
```

## Opt-in serialization for out-of-proc nodes

On the in-process node the data is always available. On out-of-process worker nodes the data is only available if the project opts in, because serializing the import graph across nodes has a cost we don't want to impose on every build:

```xml
<PropertyGroup>
  <MSBuildProvideImportGraph>true</MSBuildProvideImportGraph>
</PropertyGroup>
```

When this property is not set, serialization is skipped and `ImportEdges` returns `null` on the worker node. This means **zero additional IPC cost** for builds that don't need the import graph.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR exposes a structured project import graph to tasks by introducing a new Microsoft.Build.Framework.ProjectImportEdge type and plumbing evaluation-time import relationships through ProjectInstance into EngineServices.ImportEdges, with conditional serialization for out-of-proc nodes via MSBuildProvideImportGraph.

Changes:

  • Added public ProjectImportEdge readonly record struct in Microsoft.Build.Framework to represent individual <Import> relationships (including optional SDK name).
  • Added EngineServices.ImportEdges task-facing API and surfaced ProjectInstance.ImportEdges through the task host.
  • Implemented conditional ProjectInstance serialization of import edges for out-of-proc nodes and added unit tests validating population, deep copy, and serialization behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/Framework/ProjectImportEdge.cs New public struct representing an import edge, with docs and ToString().
src/Framework/EngineServices.cs New ImportEdges virtual property on task-facing engine services.
src/Framework/Constants.cs Added constant for MSBuildProvideImportGraph property name.
src/Build/Instance/ProjectInstance.cs Captures import edges during evaluation; conditionally serializes them across nodes; deep copy support.
src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs Surfaces ProjectInstance.ImportEdges via EngineServices implementation for tasks.
src/Build.UnitTests/Instance/ProjectInstance_ImportEdges_Tests.cs Unit tests covering evaluation population, root exclusion, deep copy, and serialization opt-in/out.

Comment on lines +60 to +88
#nullable enable
/// <summary>
/// Gets the import edges discovered during project evaluation, representing the graph of
/// <c>&lt;Import&gt;</c> relationships between project files.
/// </summary>
/// <value>
/// A read-only list of <see cref="ProjectImportEdge"/> values describing each import relationship,
/// or <see langword="null"/> if the import graph is not available on this node.
/// </value>
/// <remarks>
/// <para>
/// The import graph is always available when the task runs on the in-process node.
/// For out-of-process nodes, set the MSBuild property <c>MSBuildProvideImportGraph</c> to <c>true</c>
/// in your project to opt in to serializing import graph data across nodes.
/// </para>
/// <para>
/// Tasks can use pattern matching to access this property:
/// <code>
/// if (BuildEngine is IBuildEngine10 { EngineServices.ImportEdges: IReadOnlyList&lt;ProjectImportEdge&gt; edges })
/// {
/// // use edges
/// }
/// </code>
/// The pattern naturally handles older engines (where the property is absent) and
/// out-of-proc nodes where the data was not serialized (where the property returns <see langword="null"/>).
/// </para>
/// </remarks>
public virtual IReadOnlyList<ProjectImportEdge>? ImportEdges => null;
#nullable restore
public override bool IsOutOfProcRarNodeEnabled => _taskHost._host.BuildParameters.EnableRarNode;

/// <inheritdoc/>
public override IReadOnlyList<ProjectImportEdge> ImportEdges =>
/// May be <see langword="null"/> on out-of-process nodes if the project did not opt in
/// to serializing import edges via the <c>MSBuildProvideImportGraph</c> property.
/// </remarks>
internal IReadOnlyList<ProjectImportEdge> ImportEdges { get; private set; }
Comment on lines +2574 to +2583
bool hasImportEdges = false;

if (translator.Mode == TranslationDirection.WriteToStream)
{
hasImportEdges = ImportEdges is { Count: > 0 }
&& string.Equals(
_properties?.GetProperty(Constants.MSBuildProvideImportGraphPropertyName)?.EvaluatedValue,
"true",
StringComparison.OrdinalIgnoreCase);
}
Comment on lines +23 to +37
/// <param name="ImportedProjectPath">Full path of the imported project file.</param>
/// <param name="ImportingProjectPath">Full path of the project file that contains the <c>&lt;Import&gt;</c> element, or <see langword="null"/> if this is a direct import from the root project.</param>
/// <param name="SdkName">The SDK name if this import was resolved via an SDK reference (e.g. <c>"Microsoft.NET.Sdk"</c>); otherwise <see langword="null"/>.</param>
public readonly record struct ProjectImportEdge(
string ImportedProjectPath,
string? ImportingProjectPath,
string? SdkName = null)
{
/// <inheritdoc/>
public override string ToString()
{
string arrow = ImportingProjectPath is null
? $"[root] -> {ImportedProjectPath}"
: $"{ImportingProjectPath} -> {ImportedProjectPath}";

{
/// <summary>
/// Tests for the import edge feature: structured import graph data on ProjectInstance
/// exposed to tasks via EngineServices.GetImportEdges().
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #13680 (ImportEdges on ProjectInstance / EngineServices)

# Dimension Verdict
8 API Surface — EngineServices.Version 🟠 1 MAJOR
18 Documentation — ImportingProjectPath null claim 🟠 1 MAJOR
22 Correctness — deser null guard 🟡 1 MODERATE
16 Idiomatic C# / Nullable Consistency 🟡 1 MODERATE

✅ 20/24 dimensions clean.

  • API SurfaceVersion not incremented for new ImportEdges member; class contract requires Version3
  • DocumentationImportingProjectPath doc says "null if direct root import" but construction code never produces null; doc is wrong or the type annotation should be tightened
  • CorrectnessimportedPath can remain null after Translate(ref importedPath) on a corrupted/mismatched stream, silently violating the non-nullable string ImportedProjectPath contract
  • Nullable — scoped #nullable enable/#nullable restore in EngineServices.cs; should be file-level

Investigated but not flagged:

  • PublicAPI.Unshipped.txt — repo does not use Microsoft.CodeAnalysis.PublicApiAnalyzers; no file to update. ✅
  • using Xunit.Abstractions — project uses xUnit v3 (Verify.XunitV3), where ITestOutputHelper lives in the Xunit namespace; no import needed. Confirmed by checking adjacent file ProjectInstance_Internal_Tests.cs which follows the same pattern. ✅
  • IPC Translate(ref string) null semantics — confirmed via BinaryTranslator: on read, TranslateNullable reads one boolean; if false, returns early leaving the local unchanged. The writer-side invariant (paths are always non-null) makes this safe in the happy path, but the deserializer has no defensive guard (see Correctness finding above). ✅ (writer side), ⚠️ (reader side — flagged)
  • ImportingProjectPath nullability on ResolvedImport — confirmed via ResolvedImport.cs: ImportingElement is documented as "Null if this is the top project"; the if (ImportingElement != null) guard means ContainingProject.FullPath is always set for any edge that is recorded. ✅
  • OutOfProcTaskHostNode.EngineServicesImpl — does not override ImportEdges; the default null return is the correct fallback for the OOP task-host node (it has no ProjectInstance reference). ✅
  • MockEngine — does not override ImportEdges; default null is safe for tests that don't exercise this feature. ✅
  • Backwards compatibilityImportEdges returns null by default (not throw NotImplementedException); this is consistent with the nullable-return design and does not break existing consumers. ✅
  • Test quality — 6 tests covering population, deep copy, serialisation opt-in/opt-out, and ToString. All well-named. ✅

Note

🔒 Integrity filter blocked 1 item

The following item were blocked because they don't meet the GitHub integrity level.

  • #13680 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 | none

Generated by Expert Code Review (on open) for issue #13680 · ● 18.8M

/// out-of-proc nodes where the data was not serialized (where the property returns <see langword="null"/>).
/// </para>
/// </remarks>
public virtual IReadOnlyList<ProjectImportEdge>? ImportEdges => null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] API Surface — EngineServices.Version not incremented for new member

The class XML doc comment on Version says:

"Must be incremented whenever new members are added. Derived classes should override the property to return the version actually being implemented."

IsOutOfProcRarNodeEnabled was Version2. Adding ImportEdges requires Version3. Without bumping the version, tasks that use the version number to gate capability detection will silently see Version2 even on an engine that fully implements ImportEdges.

Recommendation: Add a constant and bump Version:

/// <summary>Version 3 with ImportEdges.</summary>
public const int Version3 = 3;

// Update the base default and the TaskHost.EngineServicesImpl override:
public virtual int Version => Version3;

TaskHost.EngineServicesImpl and any other concrete implementations that override ImportEdges should return Version3.

/// </para>
/// </remarks>
public virtual IReadOnlyList<ProjectImportEdge>? ImportEdges => null;
#nullable restore
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MODERATE] Nullable Consistency — scoped #nullable enable/#nullable restore instead of file-level

MSBuild convention (see ProjectImportEdge.cs in this same PR) is to place #nullable enable at the top of the file. The scoped annotation here:

#nullable enable
public virtual IReadOnlyList<ProjectImportEdge>? ImportEdges => null;
#nullable restore

leaves the rest of the file (and any future additions) in the nullable-disabled context restored by #nullable restore. A new contributor adding a member below the block will not get nullable diagnostics.

Recommendation: Move #nullable enable to the top of the file (after the using directives) and remove the scoped #nullable restore. The only member affected by NRT in this file is ImportEdges — the other members use value types (bool) and don't need annotation changes.

}
}
else
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MODERATE] Correctness — Null importedPath silently violates non-nullable contract

During deserialization the code initialises:

string importedPath = null;
translator.Translate(ref importedPath);
// ...
edges.Add(new ProjectImportEdge(importedPath, importingPath, sdkName));

ITranslator.Translate(ref string) wraps the value in a nullable-marker protocol: on the read side, TranslateNullable reads one boolean; if false, it returns early and leaves the local unchanged (still null). So if the stream ever marks the path as null (corrupted packet, version skew, or crafted stream), importedPath stays null and gets silently passed to ProjectImportEdge whose first positional parameter is string ImportedProjectPath — non-nullable by declaration. This will produce an ImportEdges list whose entries have ImportedProjectPath == null, causing NREs in any caller that unconditionally dereferences it.

In practice the writer never emits a null ImportedProjectPath because resolvedImport.ImportedProject.FullPath is always populated. But the deserializer has no local guard.

Recommendation:

translator.Translate(ref importedPath);
translator.Translate(ref importingPath);
translator.Translate(ref sdkName);

// Guard against a corrupted or version-mismatched stream.
if (importedPath is null)
{
    continue; // or throw InvalidProjectFileException
}

edges.Add(new ProjectImportEdge(importedPath, importingPath, sdkName));

/// <param name="ImportedProjectPath">Full path of the imported project file.</param>
/// <param name="ImportingProjectPath">Full path of the project file that contains the <c>&lt;Import&gt;</c> element, or <see langword="null"/> if this is a direct import from the root project.</param>
/// <param name="SdkName">The SDK name if this import was resolved via an SDK reference (e.g. <c>"Microsoft.NET.Sdk"</c>); otherwise <see langword="null"/>.</param>
public readonly record struct ProjectImportEdge(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Documentation Accuracy — ImportingProjectPath null claim is incorrect

The parameter doc says:

"or null if this is a direct import from the root project"

But tracing the construction path in ProjectInstance.CreateImportsSnapshot:

// Only enters the `if` when ImportingElement != null (i.e., NOT the outer project)
if (resolvedImport.ImportingElement != null)
{
    importEdges.Add(new ProjectImportEdge(
        importedPath,
        resolvedImport.ImportingElement.ContainingProject.FullPath,  // ← always non-null here
        ...));
}

ResolvedImport.ImportingElement is documented as "Null if this is the top project" — meaning for the top project itself, not for its imports. When the top project imports a file, ImportingElement is the <Import> element in the top project, and ContainingProject.FullPath is the top project's path (non-null for any project loaded from disk).

Result: ImportingProjectPath is never null as produced by CreateImportsSnapshot. The null claim in the doc is misleading — it implies a behaviour that never occurs in practice.

Recommendation (two options):

  1. Change string? ImportingProjectPath to string ImportingProjectPath (non-nullable) and remove the doc claim about null, OR
  2. Keep string? but correct the doc to describe the actual case where null can occur (a programmatically-created project that has no file path, producing an empty FullPath), and add a guard in the construction code.

/// <para>
/// Tasks can use pattern matching to access this property:
/// <code>
/// if (BuildEngine is IBuildEngine10 { EngineServices.ImportEdges: IReadOnlyList&lt;ProjectImportEdge&gt; edges })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Doc sample — minor correctness note

The pattern IBuildEngine10 { EngineServices.ImportEdges: IReadOnlyList<ProjectImportEdge> edges } uses extended property patterns (C# 10+). While syntactically valid, task authors targeting older language versions or using .NET Framework won't be able to use this syntax.

Consider also showing a more compatible alternative in the doc:

if (BuildEngine is IBuildEngine10 be && be.EngineServices.ImportEdges is { } edges)
{
    // use edges
}

Comment on lines +2601 to +2606
string importedPath = edge.ImportedProjectPath;
string importingPath = edge.ImportingProjectPath;
string sdkName = edge.SdkName;
translator.Translate(ref importedPath);
translator.Translate(ref importingPath);
translator.Translate(ref sdkName);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nullable safety gap in deserialization

string importedPath = null;
string importingPath = null;
string sdkName = null;
translator.Translate(ref importedPath);
translator.Translate(ref importingPath);
translator.Translate(ref sdkName);
edges.Add(new ProjectImportEdge(importedPath, importingPath, sdkName));

importedPath is initialized to null and passed to Translate(ref string). Per BinaryTranslator.TranslateNullable: if the stream marker is false, the method returns early and importedPath remains null. This null is then passed to new ProjectImportEdge(importedPath, ...) where ImportedProjectPath is declared as non-nullable string.

While the writing path guarantees non-null values (they come from FullPath which is validated), corrupted or forward-incompatible streams could trigger this path.

Consider adding a defensive null check:

translator.Translate(ref importedPath);
translator.Translate(ref importingPath);
translator.Translate(ref sdkName);
if (importedPath is not null)
{
    edges.Add(new ProjectImportEdge(importedPath, importingPath, sdkName));
}

Or at minimum suppress the nullable warning with importedPath! to document the invariant explicitly.

Comment on lines +25 to +26
/// <param name="SdkName">The SDK name if this import was resolved via an SDK reference (e.g. <c>"Microsoft.NET.Sdk"</c>); otherwise <see langword="null"/>.</param>
public readonly record struct ProjectImportEdge(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 ImportingProjectPath nullability doc is inaccurate

The doc for ImportingProjectPath says:

"or null if this is a direct import from the root project"

But in CreateImportsSnapshot, edges are only created inside the if (resolvedImport.ImportingElement != null) guard — which explicitly excludes the root project entry. For every recorded edge, ImportingProjectPath is set to resolvedImport.ImportingElement.ContainingProject.FullPath, which is always non-null.

The ToString() method handles null, and the type is string?, but no code path actually produces a null ImportingProjectPath. This creates a misleading API contract.

Suggestion: Either:

  1. Tighten the type to string (non-nullable) and remove the null doc/handling in ToString(), or
  2. Correct the doc to describe a realistic scenario where null would occur (e.g., "reserved for future use" or during deserialization of corrupted data)


public virtual bool IsOutOfProcRarNodeEnabled => throw new NotImplementedException();

#nullable enable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Scoped #nullable enable/#nullable restore inconsistency

This creates an isolated nullable island. The same PR adds ProjectImportEdge.cs with file-level #nullable enable — following the preferred convention for new code.

Consider enabling nullable for the entire file instead of using a scoped region. The existing members (LogsMessagesOfImportance, IsTaskInputLoggingEnabled, etc.) don't use nullable types so there's no annotation burden — you can safely add #nullable enable at file level and remove the #nullable restore.

/// out-of-proc nodes where the data was not serialized (where the property returns <see langword="null"/>).
/// </para>
/// </remarks>
public virtual IReadOnlyList<ProjectImportEdge>? ImportEdges => null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Missing Version3 constant — API contract violation

The EngineServices class contract (lines 32–35) explicitly states:

Must be incremented whenever new members are added. Derived classes should override the property to return the version actually being implemented.

Adding ImportEdges as a new public virtual member requires:

  1. A new public const int Version3 = 3; constant
  2. Updating public virtual int Version => Version3;
  3. Updating TaskHost.EngineServicesImpl to override Version returning Version3

This is important for external EngineServices implementations that may check the version to determine which members are safe to call.

@drewnoakes
Copy link
Copy Markdown
Member Author

After discussion, we're going a different route.

Closing in favour of #13681.

@drewnoakes drewnoakes closed this May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants