Correct MT routing docs and add analyzer rules for mismatched declarations

[ViktorHofer]

Fixes #14779
Fixes #14790

The problem

[MSBuildMultiThreadableTask] and IMultiThreadableTask do different jobs, but both the spec and TaskRouter's own doc comment described them as interchangeable alternatives:

Declaration	Effect	Read by
[MSBuildMultiThreadableTask]	Runs in-process instead of an out-of-proc TaskHost	TaskRouter.NeedsTaskHostInMultiThreadedMode
IMultiThreadableTask	Receives a TaskEnvironment	TaskExecutionHost

The spec went further and told authors on newer MSBuild versions to prefer the interface. Following that advice produces a task that resolves paths correctly and then runs in a TaskHost anyway, gaining nothing. This tripped up a real migration of ~150 tasks across dotnet/arcade, dotnet/source-build-assets and the VMR.

The interface cannot be promoted to a routing signal, which was the original suggestion in #14779. ToolTask implements IMultiThreadableTask (src/Utilities/ToolTask.cs), so routing on it would silently opt in every ToolTask-derived task in the ecosystem, none of which have been reviewed for thread safety. The attribute stays the routing signal; the docs and the analyzer are what need to change.

Documentation (#14790)

• Rewrote the spec's Thread-Safe Capability Indicators section around the routing/injection split. Capability is declared by the attribute alone; the interface is an additional mechanism for TaskEnvironment access, and a task that needs only the attribute is fully migrated.
• Removed "customers using newer MSBuild versions should prefer the Interface-Based Thread-Safe Capability Declaration".
• Documented both silent failure modes, including what actually happens to an interface-only task: the out-of-proc host does not assign the property. It supplies TaskEnvironment.Fallback to a TaskEnvironment constructor when one is declared, and otherwise leaves the property at the task's own default. That is correct there, because Fallback is backed by MultiProcessTaskEnvironmentDriver and the host process is dedicated to a single task. Existing coverage: TaskHost_MultiThreadableTask_Tests.InheritedTask_InTaskHost_HasUsableTaskEnvironment.
• Removed the MultiThreadableTask abstract base class section — that type was never added.
• Fixed the attribute code snippet, which showed internal class and omitted Inherited = false — contradicting the text two lines below it and the actual declaration in src/Framework/MSBuildMultiThreadableTaskAttribute.cs.
• Rewrote the TaskRouter class-level doc comment to state that the attribute is the only routing signal, and why the interface is deliberately not consulted.
• Moved the "why not route on the interface" rationale into the spec's existing Appendix: Alternatives, where the other design alternatives already live.

Analyzer (#14779)

Three new rules turn these mismatches into build diagnostics instead of silent runtime behaviour.

MSBuildTask0012 — TaskEnvironment property is never assigned (Warning, on by default)

Fires when a task has the attribute, declares a settable TaskEnvironment property, and does not implement the interface. The engine never assigns the property, so it retains the task's own default and resolves every path against the shared process working directory — while the attribute has put the task in-process, which is exactly where that is wrong.

Two shapes are deliberately not reported:

• Attribute without a TaskEnvironment property. This is the supported compatibility-bridge state and is correct for a task that does not resolve relative paths or read environment variables. 26 Arcade tasks intentionally sit here.
• Attribute plus a public single-TaskEnvironment constructor, without the interface. LoadedType selects that constructor by signature alone, independently of the interface, so the task does receive an environment. Flagging it would have been a false positive.

MSBuildTask0013 — Missing [MSBuildMultiThreadableTask] (Info, off by default)

Fires when a task declares the interface but lacks the attribute, so it still pays for a TaskHost. Off by default because that is a valid intermediate migration state — path handling done, thread-safety review not yet complete.

Only a type that declares the interface in its own base list is reported. Because ToolTask implements IMultiThreadableTask, keying on AllInterfaces would have fired on every ToolTask subclass in the ecosystem — the same trap as the routing bug this PR documents.

MSBuildTask0014 — Attribute has no effect on this type (Warning, on by default)

TaskRouter reads [MSBuildMultiThreadableTask] with inherit: false, off the concrete type the engine has just instantiated as a task. The attribute therefore only takes effect on a non-abstract class that implements ITask. Two shapes are reported.

• Not a task at all. Nothing ever reads the attribute. The motivating case is a file that declares a task alongside helper types, where the attribute lands on the wrong class — leaving the real task unmarked and still routed to a TaskHost. That occurred during the Arcade migration.
• An abstract task. The engine never instantiates that type, and no subclass inherits the attribute, so every concrete task deriving from it is still routed out-of-proc while the base looks migrated. Usually means the attribute belongs on each derived task instead.

A concrete task MSBuild cannot construct — no public parameterless constructor and no public single-TaskEnvironment constructor — is a third inert shape, deliberately not reported: Task.RegisterTask(string, Func<TaskEnvironment, ITask>) lets a host supply an arbitrary factory, so such a task may be perfectly reachable.

Validation

• src/TaskAnalyzer and src/Build build clean (0 warnings, 0 errors).
• 18 tests in MultiThreadableTaskDeclarationAnalyzerTests, including negative cases for both MSBuildTask0012 exclusions above, for ToolTask-like bases, for a non-task type without the attribute, and for an abstract base without the attribute. The ToolTask and 0013 tests run with the rule explicitly enabled, so they cannot pass trivially by virtue of the rule being off by default.
• Full TaskAnalyzer.Tests suite: 266/266.

[Copilot]

Pull request overview

This PR clarifies the multithreaded-task contract in MSBuild by documenting the distinct roles of [MSBuildMultiThreadableTask] (routing) vs IMultiThreadableTask (TaskEnvironment injection), and adds analyzer diagnostics to catch the two common “half-migrated” shapes that previously failed silently.

Changes:

• Updates multithreading docs (spec + TaskRouter comment) to state that the attribute is the only routing signal, and explains why the interface cannot be used for routing (ToolTask inheritance).
• Adds two new TaskAnalyzer rules: MSBuildTask0012 (enabled Warning) and MSBuildTask0013 (disabled-by-default Info) plus unshipped analyzer release notes.
• Adds a new analyzer implementation and a dedicated test suite covering positive/negative cases (including ToolTask-like inheritance).

Show a summary per file

File	Description
src/TaskAnalyzer/README.md	Documents the new MSBuildTask0012/0013 rules and their intended migration guidance.
src/TaskAnalyzer/MultiThreadableTaskDeclarationAnalyzer.cs	Implements the new analyzer that detects mismatches between routing and injection declarations.
src/TaskAnalyzer/DiagnosticIds.cs	Adds IDs MSBuildTask0012 and MSBuildTask0013.
src/TaskAnalyzer/DiagnosticDescriptors.cs	Adds descriptors (severity/default-enabled behavior) for the new diagnostics.
src/TaskAnalyzer/AnalyzerReleases.Unshipped.md	Records the new analyzer rules for release tracking.
src/TaskAnalyzer.Tests/MultiThreadableTaskDeclarationAnalyzerTests.cs	Adds coverage for both diagnostics, including the ToolTask-like inheritance exclusion.
src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs	Fixes the class-level comment to match the attribute-only routing behavior and rationale.
documentation/specs/multithreading/thread-safe-tasks.md	Rewrites the “capability indicators” section around the routing/injection split and documents silent failure modes.

Review details

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (1)

src/TaskAnalyzer/DiagnosticDescriptors.cs:122

• The message/description currently assert the property "stays TaskEnvironment.Fallback". That is not guaranteed: the task may initialize TaskEnvironment to a different value, but MSBuild still won’t inject the proper environment unless the interface (or a TaskEnvironment constructor) is used. Rewording to "MSBuild never assigns it" + "retains its default" would be more accurate.

            messageFormat: "Task '{0}' declares a TaskEnvironment property but does not implement IMultiThreadableTask, so the engine never assigns it and it stays TaskEnvironment.Fallback; implement IMultiThreadableTask or add a public constructor taking a TaskEnvironment",
            category: "MSBuild.TaskAuthoring",
            defaultSeverity: DiagnosticSeverity.Warning,
            isEnabledByDefault: true,
            description: "The MSBuild engine assigns TaskEnvironment only to tasks that implement IMultiThreadableTask, or through a public constructor that takes a single TaskEnvironment. A task that declares the property without either receives no environment: the property silently remains TaskEnvironment.Fallback and every path is resolved against the shared process working directory rather than the project directory. Because the task carries [MSBuildMultiThreadableTask] it runs in-process, which is exactly where that resolution is wrong.");

• Files reviewed: 8/8 changed files
• Comments generated: 4
• Review effort level: Lite

[AR-May]

Overall new analyzer looks good to me. Have some comments concerning docs update.

[AR-May]

That's not exactly correct, the declaration of mt capabilities is only the attribute. Task then may use the interface. But some very simple tasks may need only the attribute and still be fully and properly migrated. It is explained further and I would like to avoid misunderstanding here.

[ViktorHofer]

You're right, and I conflated the two. Fixed in 1b3a222 — the section now opens:

Thread-safe capability is declared by a single mechanism: the [MSBuildMultiThreadableTask] attribute. A task may additionally implement the IMultiThreadableTask interface to gain access to thread-safe APIs. The two do different jobs and are not alternatives:

I also strengthened the attribute-only bullet, which undersold the same point — it said "a supported state", now:

Attribute only — a complete, properly migrated state for a task that does not resolve relative paths or read environment variables.

[AR-May]

uff, out of my mind, I am not fully sure whether the msbuild would set the environment in this case. Probably yes. Can we verify that?

[ViktorHofer]

Yes it does. Let me see if I can add a test for that.

[ViktorHofer]

Verified — and my "yes it does" above was wrong. Thanks for pushing on it.

MSBuild does not assign the property in the TaskHost. TaskExecutionHost.cs#L545 is the in-proc path only. The out-of-proc path is OutOfProcTaskAppDomainWrapperBase.CreateTask, which passes TaskEnvironment.Fallback to TaskLoader.CreateTask solely so that a task whose only constructor takes a TaskEnvironment stays instantiable. It never touches the property, and its own comment says why:

The out-of-proc task host runs tasks in multi-process mode, where each process provides its own isolated environment.

It still works, but through two mechanisms rather than injection:

1. TaskEnvironment.Fallback is new(MultiProcessTaskEnvironmentDriver.Instance) (TaskEnvironment.cs:34) — it reads real process state, which is correct in a host process dedicated to one task.
2. The task keeps its own initializer. Every in-box declaration sets = TaskEnvironment.Fallback, including ToolTask.cs#L217 — 36 declarations, none uninitialized.

So the "resolves paths correctly" claim holds, but the reason is process isolation plus the task's own default. The bullet now says that explicitly in 1b3a222.

On the test: one already exists and covers exactly this — TaskHost_MultiThreadableTask_Tests.InheritedTask_InTaskHost_HasUsableTaskEnvironment builds a project with TaskFactory="TaskHostFactory", asserts the task really ran in the task host, and asserts it read TaskEnvironment.ProjectDirectory without an NRE. Its comment names the mechanism as "the inherited default", not injection. So I did not add a duplicate.

One corollary worth flagging: an IMultiThreadableTask whose property is an auto-property with no initializer would be null out-of-proc, since nothing assigns it there. Nothing in-box is in that shape, which is presumably why it has never bitten us. Say the word if you want a rule for it — otherwise I will leave it alone.

[AR-May]

Do we need this information in specs? This is an alternative design question and not information what is happening in the code.

[ViktorHofer]

Fair. Moved to the existing ## Appendix: Alternatives section in 1b3a222, which is already where the design alternatives live, as a new "Alternative: Routing on IMultiThreadableTask Instead of the Attribute" entry. The body of the spec now just states the behaviour.

[AR-May]

I think we decided not to provide the MultiThreadableTask. We should remove this whole section.

[ViktorHofer]

Removed the whole section in 1b3a222 — the prose, the code block, and the sentence I had added to it. Confirmed there is no such type: git grep 'class MultiThreadableTask' under src/ returns only analyzer/test types with similar names, nothing in Microsoft.Build.Utilities.

I kept the following paragraph, since it is about built-in tasks rather than the abstract class and is still accurate:

Built-in MSBuild tasks initialize TaskEnvironment with a MultiProcessTaskEnvironmentDriver-backed default. [...]

[AR-May]

"Because the attribute is the routing signal, every task that should run in-process must carry it — including tasks that also implement IMultiThreadableTask "I think at this point we repeat this too much in this document. It is getting too verbose.

[ViktorHofer]

Agreed — dropped that sentence in 1b3a222. The paragraph now keeps only the part that is not stated anywhere else, the non-inheritance:

MSBuild detects MSBuildMultiThreadableTaskAttribute by its namespace and name only, ignoring the defining assembly, which allows customers to define the attribute in their own assemblies alongside their tasks. The attribute is not inherited (Inherited = false, and TaskRouter reads it with inherit: false), so it must be applied to each concrete task class rather than to a shared base.

[AR-May]

Do we need to repeat this point? I think spec and summary should make it clear enough.

[ViktorHofer]

I would keep it here as well as not everyone reads the spec. The source code is the authority at the end of the day.

[ViktorHofer]

Taking the verbosity point even while keeping it here — condensed from five lines to four in 1b3a222, dropping the restatement of what the attribute does and keeping only why the interface is deliberately not consulted:

/// The attribute is the only routing signal. IMultiThreadableTask is
/// deliberately not consulted here: Microsoft.Build.Utilities.ToolTask implements it, so honoring it
/// would silently opt in every ToolTask-derived task in the ecosystem. The interface instead controls
/// TaskEnvironment injection, which TaskExecutionHost handles separately.

[AR-May]

hm, maybe we should call this out as warning instead? the attribute is supposed to be used only on tasks?

[ViktorHofer]

Good call — implemented as MSBuildTask0014 (Warning, on by default) in 1b3a222. TaskRouter only ever inspects types the engine is about to run as a task, so on anything that is not an ITask the attribute is genuinely inert.

The shape I actually care about is the second test:

[MSBuildMultiThreadableTask]     // ⚠️ MSBuildTask0014
public class MyTaskHelper { ... }

public class MyTask : Task       // the real task, still routed to a TaskHost
{
    public override bool Execute() => true;
}

The attribute lands on the wrong class of a multi-class file, and the task is silently left unmigrated — which is exactly the kind of failure this PR is trying to make loud. This turned up for real during the Arcade migration.

Three tests added: the plain non-task case, the helper-beside-task case, and a negative case for a non-task type without the attribute.

[ViktorHofer]

Widened the rule to cover the abstract case too, in 3f7af01 — you were asking about exactly this shape.

TaskRouter reads the attribute with inherit: false, off the concrete type the engine has just instantiated. So it only takes effect on a non-abstract class implementing ITask, and there are two inert shapes rather than one:

• not a task at all — nothing ever reads it;
• an abstract task — the engine never instantiates that type, and no subclass inherits the attribute, so every concrete task deriving from it is still routed to a TaskHost while the base looks migrated.

The second one is the quieter of the two: annotating a shared base is an easy thing to do, and nothing else in the system signals that the derived tasks did not actually opt in. The rule is now "attribute has no effect on this type" with the reason carried in the message. Both shapes stay zero-false-positive.