Enhance the task-authoring analyzers after shipping
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
The task-authoring analyzers ship with a first set of rules. This epic covers what comes after: new rules for unsafe patterns the first release does not cover, fixes for rules that miss cases, and changes that let the analyzer see code it currently cannot reach.
This work follows shipping. It does not block it.
## Steps
- [ ] Enhance the analyzers and fix the bugs [size XL]
## Stakeholders
Task authors in dotnet/sdk, dotnet/nuget, dotnet/roslyn, dotnet/arcade and the VMR, plus task authors referencing latest `Microsoft.Build.Framework`.
## Definition of done
The analyzer covers the unsafe patterns found after shipping, and task authors get correct fixes for them. Further rules are added on demand when task migration shows a need.
Gaps found in the Csc/Vbc multithreaded-task migration audit
Develop Roslyn analyzers that help MSBuild task authors find correctness, reliability and performance problems at development time instead of during testing or in production builds.
The scope of this epic covers designing and implementing new diagnostics and their code fixes, both for multithreaded task migration and for problems unrelated to it.
## Gaps found in the Csc/Vbc multithreaded-task migration audit
### MSBuildTask0002
- [ ] **Canonicalization:** The `Path.GetFullPath` code fix changes canonicalization semantics. **Recommended mitigation:** Implement canonicalization or accept the divergence and note it to users.
- [ ] **Rooted versus fully qualified paths:** `Path.IsPathRooted` does not guarantee a fully qualified path. **Recommended mitigation:** Diagnose `IsPathRooted` in path-resolution logic and recommend `IsPathFullyQualified` or explicit absolutization.
- [ ] **Command names versus filesystem paths:** The analyzer cannot distinguish executable command names such as `dotnet.exe` from filesystem paths. **Recommended mitigation:** Track path intent or avoid offering `GetAbsolutePath` when the value may use command lookup.
- [ ] **Missing environment APIs:** The banned API list omits APIs including `Environment.GetCommandLineArgs` and `Environment.GetLogicalDrives`. **Recommended mitigation:** Add them to `BannedApiDefinitions`.
- [ ] **PATH fallback behavior:** The analyzer does not detect a dropped `$PATH` search or changed fallback behavior. **Recommended mitigation:** Add targeted checks for process-launch helpers, backed by behavioral tests; general equivalence is not statically provable.
- [ ] **Process environment semantics:** The analyzer does not detect environment inheritance being replaced with a snapshot or environment-variable casing changes on Unix. **Recommended mitigation:** Add a process-environment review diagnostic, or require migration tests covering inheritance, casing, and null semantics.
### MSBuildTask0003
- [ ] **Exception and control-flow preservation:** The code fix wraps null, empty, or invalid paths and can replace expected diagnostics with exceptions. **Recommended mitigation:** Suppress automatic fixes unless non-empty validity is proven; otherwise offer a guarded fix that preserves control flow.
- [ ] **Insufficient path-state tracking:** Any `GetAbsolutePath` result is considered safe without checking canonicalization, provenance, or intended use. **Recommended mitigation:** Track path state, including whether a value is qualified, canonical, or a command name, and require the state expected by the consuming API.
### MSBuildTask0005
- [ ] **Incomplete call graph:** Transitive analysis is compilation-local and weak around delegates, interfaces, virtual dispatch, and referenced assemblies. **Recommended mitigation:** Expand call-graph modeling where practical and document or report unresolved dispatch boundaries.
- [ ] **Unsafe safety propagation:** Transitive analysis propagates the same false "safe after `GetAbsolutePath`" assumption as MSBuildTask0002 and MSBuildTask0003. **Recommended mitigation:** Reuse richer path-state analysis instead of treating one wrapper method as universally safe.
### MSBuildTask0006
- [ ] **Base-class analysis:** The analyzer only examines concrete tasks directly carrying `[MSBuildMultiThreadableTask]`, missing operations inherited from unmarked bases such as `ManagedCompiler`. **Recommended mitigation:** Analyze the complete task hierarchy and the closure of derived tasks enabled by the attribute.
- [ ] **Externally visible path values:** The analyzer excludes `[Output]` properties, so normalized paths may leak into outputs or diagnostics. **Recommended mitigation:** Add a separate rule requiring preservation of `OriginalValue` for externally visible values.
### MSBuildTask0007
- [ ] **Inherited task-item parameters:** The analyzer has the same direct-attribute and base-class scope limitation for `ITaskItem` parameters. **Recommended mitigation:** Analyze inherited task parameters and their consumers across the task hierarchy.
### MSBuildTask0011
- [ ] **Constructor-injection noise:** The constructor-injection suggestion is noisy for `ToolTask`-derived tasks that need `TaskEnvironment` only during execution. **Recommended mitigation:** Report only when constructor injection is demonstrably required or matches the task's construction model.
### New diagnostics or non-analyzer mitigations
- [ ] **Migration completeness - new diagnostic:** No rule verifies that every concrete migrated task carries the non-inherited `[MSBuildMultiThreadableTask]` attribute. **Recommended mitigation:** Add a migration-completeness diagnostic for concrete task types.
- [ ] **Polyfill fidelity - new diagnostic or contract tests:** No rule verifies that an `IsPathFullyQualified` polyfill matches the BCL contract. **Recommended mitigation:** Prefer a shared framework implementation; otherwise require contract tests against the BCL.
- [ ] **Regression-test observability - test infrastructure:** Static analysis cannot detect regression tests that do not exercise the intended production branch. **Recommended mitigation:** Add mutation-style assertions or tests that fail when the production guard is removed.
MSBuildTask0001, MSBuildTask0004, MSBuildTask0008, MSBuildTask0009, and MSBuildTask0010 were not materially implicated by the reviewed migration findings.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading the existing task-authoring analyzer rules and the BannedApiDefinitions entry point, then inspect the behavioral, migration, and regression-test infrastructure mentioned in the epic. Choose one diagnostic gap with a bounded scope; done means the analyzer and code fix handle the targeted unsafe pattern and its tests cover the relevant behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system, tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100