[TaskAnalyzer] Align diagnostic severities and ignore open generic ITaskItem<T> - #14811
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR finalizes TaskAnalyzer’s initial shipping behavior by gating MT-focused diagnostics behind an explicit MT opt-in (with a migration scope=all override) while refining the severities and documentation for typed task-property binding diagnostics. It also expands the test suite to cover default scope behavior, migration mode, .globalconfig configuration, invalid scope handling, and warnings-as-errors behavior.
Changes:
- Updated analyzer scope handling so MSBuildTask0001–MSBuildTask0005 are MT-only by default, with
msbuild_task_analyzer.scope=allenabling analysis for regular tasks. - Adjusted typed
ITaskItem<T>diagnostics: MSBuildTask0009 to Error (unsupported types cannot bind) and MSBuildTask0010 to Warning (binding succeeds but may be culture-sensitive). - Added/updated tests and docs for scope behavior,
.globalconfigconfiguration, and genericITaskItem<T>cases.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/TaskAnalyzer/UnsupportedTaskItemTypeAnalyzer.cs | Suppresses MSBuildTask0009/0010 for open generic ITaskItem<T> type parameters to avoid unactionable diagnostics. |
| src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs | Updates scope comment and ensures scope option is read consistently for transitive analysis gating. |
| src/TaskAnalyzer/SharedAnalyzerHelpers.cs | Changes scope parsing to default to MT-only unless scope=all is explicitly set. |
| src/TaskAnalyzer/README.md | Updates scope/severity documentation and adds .globalconfig guidance for migration mode. |
| src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs | Applies scope gating consistently to MSBuildTask0001–0004 and updates documentation/comments to match new defaults. |
| src/TaskAnalyzer/DiagnosticDescriptors.cs | Changes default severities for MSBuildTask0009 (Error) and MSBuildTask0010 (Warning). |
| src/TaskAnalyzer/AnalyzerReleases.Unshipped.md | Updates the unshipped release table to reflect intended severities after the policy change. |
| src/TaskAnalyzer.Tests/UnsupportedTaskItemTypeAnalyzerTests.cs | Updates severity expectations and adds coverage for MT-scope independence + generic task cases. |
| src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs | Adds default-scope vs scope=all test coverage for transitive diagnostics. |
| src/TaskAnalyzer.Tests/TestHelpers.cs | Refactors helpers to support running analyzers with default scope and explicit scope values. |
| src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs | Adds tests for default scope, scope=all, .globalconfig, invalid scope values, and warnings-as-errors promotion. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Design-level notes (not tied to a specific diff line)
|
VolPlita
left a comment
There was a problem hiding this comment.
Second review pass on head 51074bb, diffed against base 8610df47f3. Every finding below was reproduced by building the analyzer from this branch and running it against MSBuild's own task library; candidates that did not reproduce were dropped.
| # | Dimension | Verdict |
|---|---|---|
| 19 | Build Infrastructure Care | 🔴 1 MAJOR |
| 1 | Backwards Compatibility | 🔴 1 MAJOR |
| 4 | Test Coverage & Completeness | 🟡 1 MODERATE |
| 12 | Code Simplification | 🟡 1 MODERATE |
| 20 | Scope & PR Discipline | 🟡 1 MODERATE |
| 18 | Documentation Accuracy | ⚪ 1 NIT |
Combined with the first pass (dimensions 6, 10, 14, 18, 22), 14 of 24 dimensions are clean. No BLOCKING findings, so this is a COMMENT review.
- Build Infrastructure -
src/Tasksconsumes this analyzer underBuildAnalyzer=trueand theBootstrapMSBuildWithMTModeCI job runs it. Nothing in the repo setsmsbuild_task_analyzer.scope, so MSBuildTask0005 findings on MSBuild's own tasks drop 288 -> 132 and unique warnings drop 280 -> 193. - Backwards Compatibility - MSBuildTask0009 Warning -> Error is un-gated and undocumented, so a non-MT consumer both loses MSBuildTask0001-0005 and gains a hard build break in the same update.
- Test Coverage -
TypedTaskItemDiagnostics_AreIndependentOfMtScopeuses an MT task and sets no scope, so it cannot fail for the reason it names. - Code Simplification - the per-operation callback cannot report anything when
reportScopedRulesis false, yet is still registered; that is the common path under the new default. - Documentation - the
@rainersigwaldrationale block still saysConsole.*is "always wrong in tasks" while the same commit stops checking it for ordinary tasks. - Scope & PR Discipline - see the separate design-level comment: four independent concerns are bundled, one of them a self-declared unfixed defect.
Verified clean in this pass
Concurrency (13) - the scope flag is an immutable bool captured per CompilationStart; no new shared state under EnableConcurrentExecution. Performance (3) - the option is read once per compilation, and $"build_property.{ScopeOptionKey}" is a constant interpolated string over two const strings, so it is folded rather than allocated. String comparison (7) - OrdinalIgnoreCase throughout, null-safe. API surface (8) - no public API added, changed, or removed; the touched Scope* members are internal const. Dependency management (23) - no eng/Versions.props, Version.Details.xml, Directory.Packages.props or PackageReference edits. Security (24) - the Assembly.LoadFrom / Process.Start / Directory.SetCurrentDirectory banned-API entries are unchanged and still enforced for MT tasks and under scope = all; the narrowed default removes no security boundary. ChangeWave (2) - N/A: this runs in csc, not the MSBuild engine; opt-in is expressed by msbuild_task_analyzer.scope, and I confirmed a project-directory .globalconfig with is_global = true / msbuild_task_analyzer.scope = all really does re-enable MSBuildTask0001/0002/0004 on a plain ITask. Also clean: cross-platform (11), file I/O (17), idiomatic C# (16), target authoring (9), SDK boundary (15), evaluation model (21).
Correctness re-checks that came back clean: the new TypeKind.TypeParameter guard is correctly placed before IsConvertChangeTypeTaskItemType and is harmless for MSBuildTask0010, since a type parameter's SpecialType is always None. ITaskItem<T>[] unwraps to ITaskItem<T> and is skipped consistently. The README.md:299 row change from MSBuildTask0001-0005 to MSBuildTask0001-0004 for [MSBuildMultiThreadableTaskAnalyzed] helpers is correct - FindTaskTypes (TransitiveCallChainAnalyzer.cs:239) seeds only from ITask implementations, so a non-ITask helper is never an MSBuildTask0005 root. The stale rows at README.md:440/455/456 are pre-existing and outside every hunk. dotnet test src\TaskAnalyzer.Tests\TaskAnalyzer.Tests.csproj -> 262/262.
PR metadata
The title and description were rewritten against the diff. Corrections: the build.cmd -v quiet validation line was replaced, because build.cmd does not set BuildAnalyzer=true and therefore exercises nothing this PR changes; the MSBuildTask0006-0008 item no longer claims to be "being resolved in review", since it is still present at 51074bb; the documentation section now lists the "Severity Levels" rewrite and the [MSBuildMultiThreadableTaskAnalyzed] row correction; the MT opt-in list is now precise about MSBuildTask0005; and the src/Tasks / CI impact plus the missing MSBuildTask0009 downgrade path are stated explicitly.
Design-level note from the second review pass (not tied to a diff line)Four independent concerns are bundled, and one of them is a known unfixed defect. The diff carries:
Item 3 is uncontroversial and fully separable; today it cannot merge until items 1 and 2 are settled. Item 4 is unrelated to all of the others and is still present at Suggestion: land item 3 as its own PR so it ships immediately, and drop the MSBuildTask0006-0008 rows from this diff entirely rather than tracking them as a known issue. That leaves this PR to argue exactly one thing - the shipping scope and severity policy - which is the part that actually needs a decision. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Part of #14078.
Summary
Aligns TaskAnalyzer descriptor severities with the documented intent and prevents an unsupported-type false positive when a task uses an open generic
ITaskItem<T>property.This PR intentionally does not change
msbuild_task_analyzer.scope. The scope-policy implementation and tests belong to #14775 and are excluded from this PR.Changes
Convert.ChangeTypebinding succeeds, but its invariant-culture behavior may not match the task's intended parsing semantics.ITaskItem<T>properties because an unresolved type parameter does not provide enough information to determine whether the closed task parameter will be supported.DiagnosticDescriptors.cs,AnalyzerReleases.Unshipped.md, the README, and analyzer tests so all severity declarations agree.Compatibility
The severity changes only reduce diagnostics from Warning/Error to Info/Warning. No new warning or error is introduced, and analyzer scope behavior is unchanged.
Testing
.dotnet\dotnet.exe test src\TaskAnalyzer.Tests\TaskAnalyzer.Tests.csproj -c Release: 250 passed, 0 failed, 0 skipped..\build.cmd -v quiet: succeeded with 0 warnings and 0 errors.Related