dotnet / dotnet/msbuild

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

Open
#14,787 0 comments 1 reaction 2 assignees Claimed by @ViktorHofer View on GitHub
Area: Analyzer
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

### 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 over manual ItemSpec parsing
MSBuildTask0008 on Info Initialize relative default path in Execute()
MSBuildTask0009 on Warning ITaskItem used with unsupported type argument
MSBuildTask0010 on Error ITaskItem 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](https://github.com/dotnet/arcade/issues/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:

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

Adding a third value, e.g.

```ini
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 #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

- #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
- #14779, #14780, #14783, #14784, #14785
- [dotnet/arcade#17378](https://github.com/dotnet/arcade/issues/17378), [dotnet/arcade#17381](https://github.com/dotnet/arcade/pull/17381) — 127 of 136 Arcade tasks migrated; this rule is what would keep them that way

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.