Enlighten AssignProjectConfiguration for multithreaded mode

[jankratochvilcz]

Summary

Attribute-only migration for AssignProjectConfiguration — adds [MSBuildMultiThreadableTask] to declare the task safe for multithreaded scheduling.

Why attribute-only?

• No Environment.CurrentDirectory, Path.GetFullPath, File.*, or ProcessStartInfo usage
• All path resolution uses MSBuild's GetMetadata(""FullPath"") which returns engine-resolved absolute paths
• All mutable state is instance-scoped (platform mapping dicts, solution config cache)
• Only static method (SetBuildInProjectAndReferenceOutputAssemblyMetadata) is pure/stateless

No IMultiThreadableTask implementation needed. No behavioral change. No ChangeWave required.

Fixes #13613
Parent epic: #11834

[Copilot]

Pull request overview

This PR “enlightens” the AssignProjectConfiguration built-in task for MSBuild’s multithreaded execution mode by marking it as safe to run in-process on thread nodes.

Changes:

• Add [MSBuildMultiThreadableTask] to AssignProjectConfiguration to opt into multithreaded scheduling.

[jankratochvilcz]

Pushing back on this — a concurrency test is not justified for an attribute-only migration:

1. Per-instance state: MSBuild creates a fresh task instance per Execute() call (verified in the expert review on this PR). All mutable state (_mappingsPopulated, _vcxToDefaultMap, _defaultToVcxMap, base class _cachedProjectReferencesByAbsolutePath, _solutionConfiguration) is instance-scoped, so concurrent task instances cannot interfere by construction.

2. No process-global state touched: The expert review on this PR systematically verified no Environment.CurrentDirectory, no Path.GetFullPath, no File.*/Directory.*, no ProcessStartInfo, no static mutable state. GetMetadata(\"FullPath\") resolves through FileUtilities.CurrentThreadWorkingDirectory (an AsyncLocal<string?>) under the multithreaded driver, not Environment.CurrentDirectory.

3. Pattern consistency: ~27 other attribute-only migrations in the codebase (e.g., CreateProperty, Message, AssignCulture, CombinePath) ship without concurrency tests for the same reason — the attribute is a metadata declaration, not behavior. A concurrency test for an attribute-only migration would be exercising MSBuild's task instantiation model, not this task.

The concurrency-safety claim is a property of the absence of shared mutable state, which is an audit outcome, not a runtime invariant a test can falsify.

[github-actions]

Review: Attribute-only multithreaded migration for AssignProjectConfiguration

Verdict: Looks correct ✅

I verified the PR author's claims systematically:

Thread Safety Analysis

Check	Result
Static mutable state	✅ None — only static member is SetBuildInProjectAndReferenceOutputAssemblyMetadata, which is pure/stateless
Environment.CurrentDirectory usage	✅ None
Path.GetFullPath (without base)	✅ None
File.* / Directory.* static methods	✅ None
ProcessStartInfo usage	✅ None
Instance state thread safety	✅ All mutable state (_mappingsPopulated, _vcxToDefaultMap, _defaultToVcxMap, _cachedProjectReferencesByAbsolutePath, _solutionConfiguration) is instance-scoped. MSBuild creates a new task instance per execution, so these are not shared across threads.

Path Resolution Safety

The task uses GetMetadata("FullPath") for path resolution (via ResolveProjectBase). This is safe in multithreaded mode because ItemSpecModifiers.ComputeFullPath resolves through FileUtilities.CurrentThreadWorkingDirectory — an AsyncLocal<string?> set by MultiThreadedTaskEnvironmentDriver per task thread, not Environment.CurrentDirectory.

Attribute-Only Pattern

No IMultiThreadableTask implementation is needed because the task doesn't use any TaskEnvironment APIs (no file I/O, no env var access, no process management). This is consistent with ~27 other attribute-only migrations in the codebase (e.g., CreateProperty, Message, AssignCulture, CombinePath).

Inheritance Chain

AssignProjectConfiguration → ResolveProjectBase → TaskExtension → Task — no thread safety issues in the chain. TaskLoggingHelper (from Task.Log) is documented as thread-safe.

Test Coverage

Existing functional tests in AssignProjectConfiguration_Tests.cs cover the task's core behavior. Dedicated multithreading tests are not required for attribute-only migrations (no behavioral change).

No issues found.

Generated by Expert Code Review (on open) for issue #13615 · ● 7.2M

[OvesN]

@AR-May
@JanProvaznik
Here we have this call chain: Execute() → new SolutionConfiguration(xmlString) →
FileUtilities.GetFullPathNoThrow(projectAbsolutePath).

The projectAbsolutePath is read from the AbsolutePath attribute of elements in
the SolutionConfigurationContents XML (e.g. <ProjectConfiguration Project="{GUID}"
AbsolutePath="c:\foo\A.csproj" ...>).

Is it possible that this AbsolutePath attribute could ever contain a relative path? If a relative path
ever slipped in, it would be a silent behavior change.

If we expect this to always be an absolute path, should we add an assert (Path.IsPathRooted) in
SolutionConfiguration to make this contract explicit?

[OvesN]

Yeah, I think just attribute is enough to make this task thread safe but only if we honor the absolute-path contract in XML ProjectConfiguration.AbsolutePath attribute.

[OvesN]

@AR-May @JanProvaznik Here we have this call chain: Execute() → new SolutionConfiguration(xmlString) → FileUtilities.GetFullPathNoThrow(projectAbsolutePath).

The projectAbsolutePath is read from the AbsolutePath attribute of elements in the SolutionConfigurationContents XML (e.g. <ProjectConfiguration Project="{GUID}" AbsolutePath="c:\foo\A.csproj" ...>).

Is it possible that this AbsolutePath attribute could ever contain a relative path? If a relative path ever slipped in, it would be a silent behavior change.

If we expect this to always be an absolute path, should we add an assert (Path.IsPathRooted) in SolutionConfiguration to make this contract explicit?

Rainer said that there is no need in assert. One source of this XML is us the second is VS, so we will trust them.