TaskAnalyzer: add an opt-in rule requiring concrete MSBuild tasks to declare multithreading support

[ViktorHofer]

Problem

Once a repo finishes migrating its tasks to [MSBuildMultiThreadableTask], nothing keeps it migrated. A task added next week silently regresses the repo, and no diagnostic fires.

All 11 shipping rules are conditioned on the attribute already being present, or only look at API usage:

• with msbuild_task_analyzer.scope = multithreadable_only, every rule skips types that lack the attribute — so a new unannotated task is invisible;
• with scope = all (the default), a new task's unsafe API usage is flagged, but a task that happens to use no banned API is never told to opt in.

Neither configuration ever says "this concrete task type should declare multithreading support."

I verified the rule inventory by reflecting over SupportedDiagnostics in Microsoft.Build.TaskAuthoring.Analyzer 18.11.0-1.26420.118:

MSBuildTask0001  on  Error    API is never safe in MSBuild task implementations
MSBuildTask0002  on  Warning  API requires TaskEnvironment alternative in MSBuild tasks
MSBuildTask0003  on  Warning  File system API requires absolute path in MSBuild tasks
MSBuildTask0004  on  Warning  API may cause issues in multithreaded MSBuild tasks
MSBuildTask0005  on  Warning  Transitive unsafe API usage in task call chain
MSBuildTask0006  on  Info     Prefer typed path parameter over manual path construction
MSBuildTask0007  on  Info     Prefer ITaskItem<T> over manual ItemSpec parsing
MSBuildTask0008  on  Info     Initialize relative default path in Execute()
MSBuildTask0009  on  Warning  ITaskItem<T> used with unsupported type argument
MSBuildTask0010  on  Error    ITaskItem<T> type argument relies on culture-sensitive conversion
MSBuildTask0011  on  Info     Prefer constructor injection for TaskEnvironment

There is no completeness rule, and MSBuildTask0012 is free.

Why the failure is silent, and therefore expensive

TaskRouter.NeedsTaskHostInMultiThreadedMode routes any task without the attribute to an out-of-proc sidecar TaskHost. That is not an error and produces no warning — the build still succeeds, just more slowly. Nobody notices in review, and nobody notices in CI.

Concretely, from the measurement that motivated migrating Arcade (dotnet/arcade#17378): comparing two dotnet/dotnet runtime inner-repo builds, one baseline and one with -mt + MSBuild Server + node reuse, TaskHost invocations went from 849 to 14,125 and added ~98s of net task time. The whole build improved only 51.50 → 49.64 min (3.6%), because nearly every task was being shipped out-of-proc for want of an attribute. A handful of unannotated hot tasks is enough to erase the benefit of the feature.

So the regression mode is: someone adds a task, it is correct, review passes, CI is green, and the repo quietly gives back a chunk of what the migration bought.

Why this shouldn't be solved per-repo

The obvious workaround is a reflection-based unit test over the built task assemblies asserting every concrete ITask carries the attribute. That works, but it is the wrong shape:

• Arcade ships the SDK consumed by essentially every .NET repo. Each one that migrates needs exactly this guarantee.
• Duplicating a bespoke test or a private analyzer across dozens of repos means dozens of slightly different implementations, each with its own opinion about test tasks, abstract bases, and generated code.
• A unit test also reports at test time rather than in the editor, so the author only learns after the fact — whereas the analyzer already has the type model, the scope configuration, and a code fix infrastructure for exactly this attribute.

This is the one piece that turns the analyzer from a migration aid into a regression guard, and it belongs next to the other 11 rules.

Proposal

MSBuildTask0012 — "Concrete MSBuild task type does not opt into multithreaded execution", disabled by default.

Off by default is essential: enabled by default it would fire on every task in every repo that has not migrated, which is currently all of them.

Enabling. Prefer extending the existing scope option rather than requiring per-rule severity configuration. Today SharedAnalyzerHelpers accepts:

internal const string ScopeAll = "all";
internal const string ScopeMultiThreadableOnly = "multithreadable_only";

Adding a third value, e.g.

msbuild_task_analyzer.scope = require_multithreadable

would mean "analyze all task types and require the attribute". A repo that has completed its migration flips one line in one .globalconfig and is protected from then on, including for repos that consume the setting transitively through a shared SDK. dotnet_diagnostic.MSBuildTask0012.severity = error should of course still work for anyone who wants finer control.

Code fix. Add [MSBuildMultiThreadableTask], add IMultiThreadableTask, and add the TaskEnvironment property. The property-emitting machinery already exists for MSBuildTask0011. After the fix, the other rules take over and report whatever is actually unsafe in the new task — which is the desired workflow.

Scoping details worth deciding explicitly

• Only concrete types. The attribute is Inherited = false, so abstract bases and interfaces should not be flagged.
• Do flag a concrete type whose base is annotated. Because the attribute does not inherit, deriving from a migrated base and forgetting the attribute on the leaf is an easy and completely silent mistake. This came up repeatedly in the Arcade migration.
• Test tasks. Repos have task types that exist only for testing. Rather than hardcoding a heuristic, letting the normal .editorconfig path-based severity configuration handle it is probably enough.
• Pairs naturally with the attribute-without-interface check already suggested in Multithreading: docs and analyzer should reflect that [MSBuildMultiThreadableTask] is the routing signal, not IMultiThreadableTask #14779: 0012 catches "no attribute at all", that one catches "attribute present but TaskEnvironment will never be injected". Together they cover both halves of the opt-in contract.

Unrelated doc nit spotted while verifying

src/TaskAnalyzer/AnalyzerReleases.Unshipped.md lists MSBuildTask0006, 0007 and 0008 as Warning, but their actual DefaultSeverity is Info. Happy to split this out if preferred.

Related

• Roslyn analyzers for MSBuild tasks #14772 — the analyzer epic; this is the "Migration completeness - new diagnostic" checkbox, filed separately because it is self-contained and is the prerequisite for every repo that finishes a migration
• Multithreading: docs and analyzer should reflect that [MSBuildMultiThreadableTask] is the routing signal, not IMultiThreadableTask #14779, TaskAnalyzer: MSBuildTask0003 code fix produces non-compiling code for nested calls and static methods #14780, TaskAnalyzer: MSBuildTask0002 is not reported for code inherited from an unannotated base class #14783, TaskAnalyzer: no diagnostic when a path property is resolved at one use site but used raw at another #14784, TaskAnalyzer: no diagnostic for unsynchronized shared mutable state in [MSBuildMultiThreadableTask] types #14785
• dotnet/arcade#17378, dotnet/arcade#17381 — 127 of 136 Arcade tasks migrated; this rule is what would keep them that way