Document and configure TaskAnalyzer - #14820
Draft
VolPlita wants to merge 2 commits into
Draft
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add stable analyzer enablement and scope properties, package-consumption coverage, and complete task-author guidance. Remove unsafe temporary-path recommendations and preserve Path.GetFullPath canonicalization in code fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prepares TaskAnalyzer for consumption by task authors by documenting expected usage and wiring supported configuration controls through MSBuild-to-compiler visible properties, while aligning analyzer behavior with a safe default scope and adding coverage for configuration scenarios.
Changes:
- Added
MSBuildTaskAnalyzerEnabled/MSBuildTaskAnalyzerScopeconfiguration plumbing (MSBuild properties +.globalconfig) and ensured analyzers/suppressor honor the enable switch. - Updated rule scope/severity behavior and adjusted banned API coverage (including removing diagnostics where no behavior-equivalent replacement exists).
- Expanded documentation and added unit + end-to-end tests covering configuration, scope defaults, suppression, and warnings-as-errors.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TaskAnalyzer/UnsupportedTaskItemTypeAnalyzer.cs | Gated analyzer on enabled flag; skipped open generic type parameters for binding diagnostics. |
| src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs | Added enabled gating; updated scope-option comment. |
| src/TaskAnalyzer/TaskEnvironmentConstructorInjectionAnalyzer.cs | Added enabled gating at compilation start. |
| src/TaskAnalyzer/SharedAnalyzerHelpers.cs | Introduced enabled/scope option + MSBuild property keys and parsing behavior with safe defaults. |
| src/TaskAnalyzer/RequiredTaskPropertyInitializationSuppressor.cs | Suppressor now honors analyzer enabled switch. |
| src/TaskAnalyzer/README.md | Added/expanded task-author guide, configuration docs, and updated rule descriptions. |
| src/TaskAnalyzer/PreferTypedParameterCodeFixProvider.cs | Updated code fix behavior to attempt preserving Path.GetFullPath canonicalization. |
| src/TaskAnalyzer/PreferTypedParameterAnalyzer.cs | Added enabled gating at compilation start. |
| src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs | Updated Path.GetFullPath code fix to use canonicalization flow. |
| src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs | Added enabled gating; refined default-vs-migration scope logic for diagnostics. |
| src/TaskAnalyzer/DiagnosticDescriptors.cs | Updated severities for typed task item diagnostics (0009/0010). |
| src/TaskAnalyzer/BannedApiDefinitions.cs | Removed banned APIs without behavior-equivalent replacement; updated Path.GetFullPath guidance text. |
| src/TaskAnalyzer/AnalyzerReleases.Unshipped.md | Updated release metadata notes/severities. |
| src/TaskAnalyzer.Tests/UnsupportedTaskItemTypeAnalyzerTests.cs | Updated tests for new severities; added coverage for scope independence and generics. |
| src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs | Added tests for default scope vs all migration behavior. |
| src/TaskAnalyzer.Tests/TestHelpers.cs | Adjusted stubs/helpers to support new canonicalization expectations and option plumbing. |
| src/TaskAnalyzer.Tests/RequiredTaskPropertyInitializationSuppressorTests.cs | Added coverage for disable switch disabling CS8618 suppression. |
| src/TaskAnalyzer.Tests/PreferTypedParameterCodeFixProviderTests.cs | Updated code-fix expectations for canonicalization preservation. |
| src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs | Updated code-fix expected output/message for Path.GetFullPath replacement. |
| src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs | Updated banned-API coverage; added extensive scope/config/warnings-as-errors tests. |
| src/TaskAnalyzer.Tests/AnalyzerConfigurationTests.cs | New unit tests verifying enabled/scope parsing defaults and .globalconfig behavior. |
| src/MSBuild.EndToEnd.Tests/TaskAnalyzerConfiguration_Tests.cs | New end-to-end tests validating analyzer configuration through MSBuild project/solution builds. |
| src/MSBuild.EndToEnd.Tests/Microsoft.Build.EndToEnd.Tests.csproj | Wired TaskAnalyzer reference and test assets into end-to-end test project. |
| src/Framework/Microsoft.Build.Framework.csproj | Packed additional buildTransitive targets to expose configuration properties to the compiler. |
| src/Framework/buildTransitive/Microsoft.Build.Framework.TaskAnalyzer.targets | Defines CompilerVisibleProperty entries for analyzer config properties. |
| src/Framework/buildTransitive/Microsoft.Build.Framework.TaskAnalyzer.Root.targets | Root import to activate TaskAnalyzer compiler-visible properties. |
| src/Framework/buildTransitive/Microsoft.Build.Framework.TaskAnalyzer.Framework.targets | Full-framework import glue for TaskAnalyzer compiler-visible properties. |
| src/Framework/buildTransitive/Microsoft.Build.Framework.targets | Imports TaskAnalyzer compiler-visible properties from existing transitive targets. |
| documentation/specs/multithreading/thread-safe-tasks.md | Updated MT spec to reference TaskAnalyzer behavior/configuration and aligned terminology. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+206
to
+210
| InvocationExpressionSyntax getAbsolutePath = SyntaxFactory.InvocationExpression( | ||
| SyntaxFactory.MemberAccessExpression( | ||
| SyntaxKind.SimpleMemberAccessExpression, | ||
| SyntaxFactory.IdentifierName("TaskEnvironment"), | ||
| SyntaxFactory.IdentifierName("GetAbsolutePath")), |
Comment on lines
+636
to
+647
| private static MemberAccessExpressionSyntax AppendCanonicalValue(ExpressionSyntax expression) | ||
| { | ||
| InvocationExpressionSyntax canonicalForm = SyntaxFactory.InvocationExpression( | ||
| SyntaxFactory.MemberAccessExpression( | ||
| SyntaxKind.SimpleMemberAccessExpression, | ||
| expression.WithoutTrivia(), | ||
| SyntaxFactory.IdentifierName("GetCanonicalForm"))); | ||
|
|
||
| return SyntaxFactory.MemberAccessExpression( | ||
| SyntaxKind.SimpleMemberAccessExpression, | ||
| canonicalForm, | ||
| SyntaxFactory.IdentifierName("Value")); |
Comment on lines
104
to
107
| // Path.GetFullPath | ||
| new BannedApi("M:System.IO.Path.GetFullPath(System.String)", | ||
| ApiCategory.TaskEnvironment, "use TaskEnvironment.GetAbsolutePath instead"), | ||
| new BannedApi("M:System.IO.Path.GetFullPath(System.String,System.String)", | ||
| ApiCategory.TaskEnvironment, "use TaskEnvironment.GetAbsolutePath instead"), | ||
|
|
||
| // Path.GetTempPath / GetTempFileName - depend on environment variables | ||
| new BannedApi("M:System.IO.Path.GetTempPath", | ||
| ApiCategory.TaskEnvironment, "depends on TMP/TEMP environment variables; use TaskEnvironment.GetEnvironmentVariable(\"TMP\") instead"), | ||
| new BannedApi("M:System.IO.Path.GetTempFileName", | ||
| ApiCategory.TaskEnvironment, "depends on TMP/TEMP environment variables; use TaskEnvironment.GetEnvironmentVariable(\"TMP\") instead"), | ||
| ApiCategory.TaskEnvironment, "use TaskEnvironment.GetAbsolutePath(path).GetCanonicalForm().Value instead"), | ||
|
|
Comment on lines
84
to
89
| | `Environment.SetEnvironmentVariable()` | `TaskEnvironment.SetEnvironmentVariable()` | | ||
| | `Environment.GetEnvironmentVariables()` | `TaskEnvironment.GetEnvironmentVariables()` | | ||
| | `Environment.ExpandEnvironmentVariables()` | Use `TaskEnvironment.GetEnvironmentVariable()` per variable | | ||
| | `Environment.GetFolderPath()` | Use `TaskEnvironment.GetEnvironmentVariable()` | | ||
| | `Path.GetFullPath()` | `TaskEnvironment.GetAbsolutePath()` | | ||
| | `Path.GetTempPath()` | `TaskEnvironment.GetEnvironmentVariable("TMP")` | | ||
| | `Path.GetTempFileName()` | `TaskEnvironment.GetEnvironmentVariable("TMP")` | | ||
| | `Process.Start()` (all overloads) | `TaskEnvironment.GetProcessStartInfo()` | | ||
| | `Path.GetFullPath()` | `TaskEnvironment.GetAbsolutePath(path).GetCanonicalForm().Value` | | ||
| | `Process.Start()` overloads that accept strings | Create a `ProcessStartInfo` with `TaskEnvironment.GetProcessStartInfo()` | | ||
| | `new ProcessStartInfo()` (all overloads) | `TaskEnvironment.GetProcessStartInfo()` | |
Comment on lines
429
to
434
| | MSBuildTask0002: `Environment.GetEnvironmentVariable(x)` | → `TaskEnvironment.GetEnvironmentVariable(x)` | | ||
| | MSBuildTask0002: `Environment.SetEnvironmentVariable(x, y)` | → `TaskEnvironment.SetEnvironmentVariable(x, y)` | | ||
| | MSBuildTask0002: `Environment.GetEnvironmentVariables()` | → `TaskEnvironment.GetEnvironmentVariables()` | | ||
| | MSBuildTask0002: `Path.GetFullPath(x)` | → `TaskEnvironment.GetAbsolutePath(x)` | | ||
| | MSBuildTask0002: `Path.GetFullPath(x)` | → `TaskEnvironment.GetAbsolutePath(x).GetCanonicalForm().Value` | | ||
| | MSBuildTask0002: `Environment.CurrentDirectory` | → `TaskEnvironment.ProjectDirectory` | | ||
| | MSBuildTask0002: `Directory.GetCurrentDirectory()` | → `TaskEnvironment.ProjectDirectory` | |
Comment on lines
51
to
57
| public struct AbsolutePath : System.IEquatable<AbsolutePath> | ||
| { | ||
| public AbsolutePath(string path) { Value = path; OriginalValue = path; } | ||
| public string Value { get; } | ||
| public string OriginalValue { get; } | ||
| public AbsolutePath GetCanonicalForm() => this; | ||
| public static implicit operator string(AbsolutePath p) => p.Value; |
Comment on lines
+10
to
+12
| MSBuildTask0006 | MSBuild.TaskAuthoring | Info | Prefer typed path parameter (AbsolutePath/FileInfo/DirectoryInfo) over string (code fix available) | ||
| MSBuildTask0007 | MSBuild.TaskAuthoring | Info | Prefer ITaskItem<T> over manual ItemSpec parsing (code fix available) | ||
| MSBuildTask0008 | MSBuild.TaskAuthoring | Info | Initialize a relative default path in Execute() so TaskEnvironment can root it when the property is retyped (code fix available) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #14078
Context
TaskAnalyzer needs complete task-author documentation, safe temporary-path guidance, and supported configuration controls before publication. The default behavior must protect regular tasks while allowing explicit migration analysis.
This PR depends on #14811. It is based on that PR's single commit and does not depend on or include #14812.
Changes Made
MSBuildTaskAnalyzerEnabledandMSBuildTaskAnalyzerScopethroughCompilerVisiblePropertypackage targets.multithreadable_onlyas the safe scope default and supportsallas explicit migration mode..globalconfigoptions, diagnostic suppression, and standard warnings-as-errors behavior.Path.GetFullPathcanonicalization in TaskAnalyzer code fixes..globalconfig, suppression, and warnings-as-errors.Testing
TaskAnalyzer.Tests: 273 passed.TaskAnalyzerConfiguration_Tests: 18 passed across net11.0 and net472.build.cmd -v quiet /p:UseSharedCompilation=false: succeeded with 0 warnings and 0 errors.Notes
PR #14812 separately adds the analyzer DLL to
Microsoft.Build.Framework; this branch intentionally does not include that packaging commit.Path.GetTempPath()remains undiagnosed until #14729 provides the equivalent API.Path.GetTempFileName()remains undiagnosed until #14723 provides a file-creating equivalent.