Add MSBuildTask0015: require concrete MSBuild tasks to declare multithreading support - #14789
Add MSBuildTask0015: require concrete MSBuild tasks to declare multithreading support#14789ViktorHofer with Copilot wants to merge 9 commits into
Conversation
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new opt-in TaskAuthoring analyzer rule to prevent “silent” multithreading regression by requiring concrete MSBuild tasks to explicitly opt into multithreaded execution via [MSBuildMultiThreadableTask], plus a code fix and accompanying documentation/tests.
Changes:
- Introduces MSBuildTask0012 analyzer (
RequireMultiThreadableTaskAnalyzer) with opt-in viamsbuild_task_analyzer.scope = require_multithreadableor explicit severity configuration. - Adds a code fix to apply
[MSBuildMultiThreadableTask]and, when safe, implementIMultiThreadableTaskand add theTaskEnvironmentproperty. - Updates docs, release notes, and unit test infrastructure + adds new analyzer/codefix test suites.
Show a summary per file
| File | Description |
|---|---|
| src/TaskAnalyzer/WellKnownTypeNames.cs | Adds constants to support attribute detection by namespace + name. |
| src/TaskAnalyzer/SharedAnalyzerHelpers.cs | Adds new require_multithreadable scope value and helper option readers. |
| src/TaskAnalyzer/RequireMultiThreadableTaskAnalyzer.cs | New MSBuildTask0012 analyzer with opt-in gating and per-tree severity support. |
| src/TaskAnalyzer/RequireMultiThreadableTaskCodeFixProvider.cs | New code fix to declare multithreading support (attribute + optional interface/property). |
| src/TaskAnalyzer/DiagnosticIds.cs | Adds MSBuildTask0012 ID constant. |
| src/TaskAnalyzer/DiagnosticDescriptors.cs | Adds MSBuildTask0012 descriptor and includes it in the global descriptor list. |
| src/TaskAnalyzer/README.md | Documents MSBuildTask0012 and the expanded scope option behavior. |
| src/TaskAnalyzer/AnalyzerReleases.Unshipped.md | Adds unshipped release entry for MSBuildTask0012. |
| src/TaskAnalyzer.Tests/TestHelpers.cs | Extends test helpers to run arbitrary analyzers with supplied global options. |
| src/TaskAnalyzer.Tests/RequireMultiThreadableTaskAnalyzerTests.cs | New analyzer tests covering scope/severity opt-in and inheritance/abstract behavior. |
| src/TaskAnalyzer.Tests/RequireMultiThreadableTaskCodeFixProviderTests.cs | New code fix tests for the various “safe to implement interface/property” cases. |
| documentation/specs/multithreading/thread-safe-tasks.md | Documents MSBuildTask0012 as a way to keep migrated repos from regressing. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
… opt-in comment Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
#14809 merged first and claimed MSBuildTask0012 through MSBuildTask0014, so the rule added here moves from MSBuildTask0012 to MSBuildTask0015. Conflicts were confined to the four shared registration points -- DiagnosticIds, DiagnosticDescriptors, AnalyzerReleases.Unshipped.md and README.md -- where both sides appended to the same lists; both sets of rules are kept, ordered by ID. MSBuildTask0015 covers every concrete task type, so it subsumes the merged MSBuildTask0013, which reports the same missing attribute on the narrower set of tasks that declare IMultiThreadableTask. That relationship is now called out in the descriptor and the README so the two opt-ins are not enabled redundantly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
All five call sites that look for [MSBuildMultiThreadableTask] now go through SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute, which matches the attribute by full name exactly as TaskRouter does in the engine. The four pre-existing sites resolved the attribute through Compilation.GetTypeByMetadataName and compared symbol identity. That diverges from the engine in two ways. It rejects a repository's own copy of the attribute, which the engine deliberately accepts, and GetTypeByMetadataName returns null once more than one referenced assembly contributes the name, which silently disabled MSBuildTask0006 through MSBuildTask0008 and MSBuildTask0012 through MSBuildTask0014 for exactly the compat-shim setup the engine's name matching exists to support.
The severity bullet list in the TaskAnalyzer README conflicted: main reclassified MSBuildTask0006-0008 as Info and MSBuildTask0010 as Warning in #14811, while this branch had appended bullets for MSBuildTask0012-0015. Kept main's rewritten taxonomy for MSBuildTask0001-0011 and re-added the four newer bullets after it. Also removed a stray conflict marker that the previous merge left in the MSBuildTask0014 section.
There was a problem hiding this comment.
Review details
Suppressed comments (4)
src/TaskAnalyzer/RequireMultiThreadableTaskCodeFixProvider.cs:85
- The conflict check only enumerates properties. If the task declares a field, event, or method named
TaskEnvironment,existingPropertyis null and the fixer inserts a property with the same name, producing CS0102 instead of a compiling fix. Inspect all members across the hierarchy and add the interface/property only when there is no conflicting member, while accepting an existing compatible property.
IPropertySymbol? existingProperty = SharedAnalyzerHelpers.GetPropertiesIncludingBaseTypes(taskType)
.FirstOrDefault(property => string.Equals(property.Name, TaskEnvironmentPropertyName, StringComparison.Ordinal));
src/TaskAnalyzer/RequireMultiThreadableTaskAnalyzer.cs:108
- The new
TryGetGlobalDiagnosticValueopt-in path is not exercised by the added tests: they cover per-tree.editorconfigandSpecificDiagnosticOptions, but notdotnet_diagnostic.MSBuildTask0015.severityin a.globalconfig. This is the subtle severity path called out in the PR description, so add a.globalconfigseverity test to prevent it from silently becoming a no-op again.
SyntaxTreeOptionsProvider? optionsProvider = compilation.Options.SyntaxTreeOptionsProvider;
return optionsProvider is not null &&
optionsProvider.TryGetGlobalDiagnosticValue(DiagnosticIds.RequireMultiThreadableTask, cancellationToken, out severity) &&
IsEnabled(severity);
src/TaskAnalyzer/README.md:613
- Both new architecture entries identify these components as implementing MSBuildTask0012, but the analyzer and fixer use MSBuildTask0015. Keeping the old number here sends contributors to the wrong rule.
| `RequireMultiThreadableTaskAnalyzer.cs` | Analyzer for MSBuildTask0012 — reports concrete task types that do not declare multithreading support, once the rule is opted into |
| `RequireMultiThreadableTaskCodeFixProvider.cs` | Code fix for MSBuildTask0012 — applies the attribute, implements `IMultiThreadableTask`, and adds the `TaskEnvironment` property |
src/TaskAnalyzer/RequireMultiThreadableTaskCodeFixProvider.cs:113
- A public
initaccessor has a publicSetMethod, but it cannot implement the interface's ordinarysetaccessor. This predicate therefore addsIMultiThreadableTaskto a task with{ get; init; }and leaves the fixed document with an interface implementation error. Exclude init-only setters.
property.SetMethod is { DeclaredAccessibility: Accessibility.Public } &&
- Files reviewed: 17/17 changed files
- Comments generated: 4
- Review effort level: Balanced
| if (attributeClass is not null && | ||
| string.Equals(attributeClass.Name, WellKnownTypeNames.MultiThreadableTaskAttributeName, StringComparison.Ordinal) && | ||
| string.Equals(attributeClass.ContainingNamespace?.ToDisplayString(), WellKnownTypeNames.FrameworkNamespace, StringComparison.Ordinal)) |
| var classDeclaration = root.FindNode(diagnostic.Location.SourceSpan) | ||
| .FirstAncestorOrSelf<ClassDeclarationSyntax>(); |
| if (location.IsInSource && | ||
| (optedIn || IsEnabledForTree(context.Compilation, location.SourceTree, context.CancellationToken))) |
| | MSBuildTask0007: `new FileInfo(item.ItemSpec)` in `foreach` over `ITaskItem[]` | → Retype source property to ``ITaskItem<FileInfo>[]`` and replace with `item.Value` | | ||
| | MSBuildTask0007: `new AbsolutePath(Item.GetMetadata("FullPath"))` | → Retype `Item` to ``ITaskItem<AbsolutePath>`` and replace with `Item.Value` | | ||
| | MSBuildTask0008: relative default `= "obj"` on a path property | → Retype the property (unset default) and move the default into `Execute()` as a guarded, `TaskEnvironment`-rooted assignment | | ||
| | MSBuildTask0012: concrete task without the opt-in | → Apply `[MSBuildMultiThreadableTask]`, implement `IMultiThreadableTask`, and add the `TaskEnvironment` property | |
| MSBuildTask0012 | MSBuild.TaskAuthoring | Warning | TaskEnvironment property is never assigned by MSBuild because the task does not implement IMultiThreadableTask | ||
| MSBuildTask0013 | MSBuild.TaskAuthoring | Info | Task declares IMultiThreadableTask but is not marked with [MSBuildMultiThreadableTask] (disabled by default) | ||
| MSBuildTask0014 | MSBuild.TaskAuthoring | Warning | [MSBuildMultiThreadableTask] applied to a type MSBuild never routes as a task -- not an ITask, or an abstract task whose attribute no subclass inherits -- where it has no effect | ||
| MSBuildTask0015 | MSBuild.TaskAuthoring | Warning | Concrete task type does not declare multithreading support; reports only when opted into with `msbuild_task_analyzer.scope = require_multithreadable` or an explicit severity (code fix available) |
There was a problem hiding this comment.
I would push back on having a fix for this error. It does not seem right to me. Adding the attribute is the last step when making a task multithread-safe. If applied without a proper understanding of the task, it can be quite harmful.
| public static readonly DiagnosticDescriptor RequireMultiThreadableTask = new( | ||
| id: DiagnosticIds.RequireMultiThreadableTask, | ||
| title: "Concrete MSBuild task type does not opt into multithreaded execution", | ||
| messageFormat: "Task '{0}' does not declare multithreading support; apply [MSBuildMultiThreadableTask] so it is not routed to an out-of-proc TaskHost", |
There was a problem hiding this comment.
This message misleads - applying the attribute is not the only thing that needs to be done if task does not opt into multithreaded execution.
| ```ini | ||
| # .globalconfig | ||
| is_global = true | ||
| msbuild_task_analyzer.scope = require_multithreadable |
There was a problem hiding this comment.
I think of scope as defining which tasks are analyzed, so introducing a separate scope value solely to enable one extra rule feels like overkill. My suggestion would be to remove the scope = require_multithreadable opt-in and instead control the rule through its severity configuration.
I think this rule actually can be enabled with scope all as well, since we produce mt related diagnostic for this scope for all tasks.
| **Scope:** Classes carrying the attribute that either do not implement `ITask` or are abstract. | ||
|
|
||
| A concrete task that MSBuild cannot construct — no public parameterless constructor and no public single-`TaskEnvironment` constructor — is a third inert shape, but it is **not** reported. `Microsoft.Build.Utilities.Task.RegisterTask(string, Func<TaskEnvironment, ITask>)` lets a host supply an arbitrary factory, so such a task may be perfectly reachable. | ||
| >>>>>>> origin/main |
There was a problem hiding this comment.
nit: something went wrong during a merge.
Context
Once a repo finishes migrating its tasks to
[MSBuildMultiThreadableTask], nothing keeps it migrated.TaskRouter.NeedsTaskHostInMultiThreadedModeroutes any task lacking the attribute to an out-of-proc TaskHost — no error, no warning, just a slower build. The existing rules either skip unannotated types (scope = multithreadable_only) or only flag unsafe API usage (scope = all), so a new task that happens to touch no banned API is never told to opt in.MSBuildTask0015turns that silent regression into a diagnostic.Changes Made
New rule (
RequireMultiThreadableTaskAnalyzer.cs) — reports non-abstract classes implementingITaskwithout a directly appliedMicrosoft.Build.Framework.MSBuildMultiThreadableTaskAttribute. Attribute detection mirrors the engine: matched by namespace + name viaGetAttributes()(direct only), honoringInherited = false. Consequently:.editorconfigseverity.Opting in — silent otherwise:
msbuild_task_analyzer.scope = require_multithreadable, a third value alongsideallandmultithreadable_only, meaning "analyze all task types and require the attribute";dotnet_diagnostic.MSBuildTask0015.severity(.editorconfigper-file,.globalconfig, ruleset,<WarningsAsErrors>).Code fix — "Declare multithreading support" adds the attribute,
IMultiThreadableTask, and theTaskEnvironmentproperty, falling back to attribute-only when the type already has a conflictingTaskEnvironmentmember. The other rules then take over on the newly annotated task.Docs — the rule, and the scope option's three values, in
src/TaskAnalyzer/README.md(scope was previously undocumented); a "Keeping a Migrated Repository Migrated" section in the multithreading spec; theAnalyzerReleases.Unshipped.mdentry.Relationship to MSBuildTask0013 (added by #14809) — that rule reports the same missing attribute, but only on the narrower set of tasks that declare
IMultiThreadableTask, and it is off by default.MSBuildTask0015covers every concrete task type, so it subsumes it: a repository that opts intoMSBuildTask0015does not also need to enableMSBuildTask0013. Called out in both the descriptor and the README so the two opt-ins are not enabled redundantly.MSBuildTask0013remains useful on its own for a codebase that wants the narrower signal without the repo-wide gate.Testing
24 tests across
RequireMultiThreadableTaskAnalyzerTestsandRequireMultiThreadableTaskCodeFixProviderTestscover the scope values, each severity-configuration path, the inheritance and abstract-type scoping rules, and the code fix including the conflicting-member fallback. Also verified end to end against a realdotnet build, which is how the severity bug in the notes below surfaced.Post-merge, the full
TaskAnalyzer.Testssuite passes at 290/290, so the rules from #14809 are unaffected.Notes
Two implementation constraints worth flagging for review:
isEnabledByDefault: true, with the opt-in enforced in analyzer code. Roslyn filters disabled-by-default descriptors before an analyzer can consult its options, soisEnabledByDefault: falsecannot be re-enabled by the scope setting. To keep the cost of this at zero for unmigrated repos, no symbol action is registered at compilation start when nothing opts in.dotnet_diagnostic.*.severitynever reachesAnalyzerConfigOptions— Roslyn strips it intoTreeOptions. The severity opt-in initially looked correct but did nothing in a real build; it now readsCompilation.Options.SyntaxTreeOptionsProvider(plusSpecificDiagnosticOptionsfor rulesets).The doc nit in the issue does not apply here:
MSBuildTask0006/0007/0008are alreadyWarningin both this repo's source andAnalyzerReleases.Unshipped.md; theInfoseverities came from the shipped18.11.0-1.26420.118package. Nothing changed.