Roslyn analyzers for MSBuild tasks

[AR-May]

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.

[ViktorHofer]

Adding field evidence for the MSBuildTask0003 → "Exception and control-flow preservation" checkbox, since we hit it at scale and it turned out to be the highest-impact item on this list by a wide margin.

Context: migrating all 136 Arcade tasks to [MSBuildMultiThreadableTask] (dotnet/arcade#17381). Following MSBuildTask0003's guidance mechanically broke all 10 source-only build legs in the dotnet/dotnet VMR.

The specific shape

File.Exists and Directory.Exists are, as far as I can tell, the only two path-taking BCL APIs that accept null/"" and return false instead of throwing. That makes if (File.Exists(x)) the idiomatic guard for an optional (non-[Required]) task property. MSBuildTask0003 flags it and recommends the wrap, which converts the guard into a throw:

public string OptionalPath { get; set; }   // no [Required]

// before: false when unset
if (File.Exists(OptionalPath))

// after MSBuildTask0003: ArgumentNullException when unset
if (File.Exists(TaskEnvironment.GetAbsolutePath(OptionalPath)))

Measured behavior

Microsoft.Build.Utilities.Core 18.8.2, net11.0:

input	File.Exists	Directory.Exists	TaskEnvironment.GetAbsolutePath
null	false	false	throws ArgumentNullException
""	false	false	throws ArgumentException
" "	false	false	<projdir>\
"sub/file.txt"	false	false	<projdir>\sub/file.txt

For comparison, File.ReadAllText, Directory.GetFiles, File.Copy, new FileInfo, ZipFile.* and XDocument.Load all already throw on null/"", so wrapping those preserves observable behavior. Only these two invert a false into an exception.

The failure is also maximally awkward to diagnose: the task worked in every unit test (which pass a real path), and only failed in the legs that exercised the optional-property path.

Suggestions

1. Special-case these two APIs in the fixer. Either don't offer an automatic fix, or offer a null-preserving one:

   if (!string.IsNullOrEmpty(OptionalPath) && File.Exists(TaskEnvironment.GetAbsolutePath(OptionalPath)))

2. Use [Required] as the discriminator. When the flagged argument traces back to a task property without [Required], the unguarded wrap is a behavior change by construction; when it has [Required], MSBuild guarantees non-null and the plain wrap is safe. That is a cheap, precise signal the analyzer already has access to.
3. Add TaskEnvironment.TryGetAbsolutePath(string, out AbsolutePath), or make GetAbsolutePath return a null/empty AbsolutePath for null/empty input rather than throwing. This would make the whole class of guard code migrate cleanly, and arguably matches the "resolve a path relative to the project directory" contract better than throwing does — there is no relative path to resolve.

Option 3 is the one I'd advocate for: it fixes the ~292 call sites we audited without any of them needing an analyzer suppression or a hand-written guard.

Filed separately, as they appear not to be covered above: #14783 (base-class scope also affects MSBuildTask0002), #14784 (path resolved at one use site, raw at a sibling), #14785 (unsynchronized shared mutable static state).