Fix TaskAnalyzer path-resolution order and detect inverted compositions

[Copilot]

Work item (Internal use):

Summary

MSBuildTask0003 can generate GetAbsolutePath(Path.GetDirectoryName(path)), which throws for bare filenames. Existing inverted calls also go undiagnosed.

• Fixer: Resolve the original input before GetDirectoryName/GetPathRoot, including nested extractions.
• Analyzer: Add scope-aware MSBuildTask0015 with a swap fix; recognize root extraction from absolute paths as safe.

Directory.CreateDirectory(
    Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile)));

Customer Impact

Prevents migration-generated ArgumentException for inputs such as "list.xml" and identifies previously accepted inverted calls.

Regression?

Existing analyzer/fixer gap, not a runtime regression.

Testing

Regression coverage for both extraction methods, aliases, named arguments, nested calls, nullable inputs, scope, lookalike methods, and Fix All.

Risk

Limited to the unshipped, opt-in analyzer; no runtime changes. The new warning honors existing scope settings. Fixes are withheld where swapping would break AbsolutePath consumers or introduce nullable-input warnings.

• Fixes TaskAnalyzer: code fix generates GetAbsolutePath(Path.GetDirectoryName(x)) and no diagnostic flags the inverted composition #14971

[dotnet-policy-service]

Hello @copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo.

[github-actions]

Design-level notes (MSBuild expert review, PR #14972)

• Dimension 1 / 3 — new warning is acceptable here, but note the ordering side effect. MSBuildTask0015 is a new Warning that is isEnabledByDefault: true, which would normally be a WarnAsError break — it is fine because the analyzer is unshipped and opt-in, as the PR description states. However, the report block in MultiThreadableTaskAnalyzer.AnalyzeOperation returns immediately after reporting 0015, before the MSBuildTask0003 file-path block. For an operation that would previously have produced MSBuildTask0003 (e.g. a TaskEnvironment.GetAbsolutePath(...) call nested in a flagged shape), the user now sees only 0015. That is a deliberate de-duplication choice, but it is undocumented; a short comment explaining why 0015 supersedes the later checks would help future maintainers.

• Dimension 20 — scope. The PR mixes three concerns: a fixer bug fix (GetPathToResolve), a new diagnostic + fix (0015), and a behavioral tweak to the existing 0003 fixer (the maybe-null withholding guard). The last one is the riskiest and least tested part; it changes when an already-shipped-in-branch fix is offered. Consider calling it out explicitly in the description so reviewers of the 0003 fixer know to look.

• Dimension 18 — documentation is strong. The README additions accurately describe the new rule, the scope interaction, and the withheld-fix conditions, including the subtle point that null-forgiving the extraction result does not establish non-nullness of the input. Nice.

• Dimension 8 — API surface. IsPathExtraction / GetInvertedPathExtraction are correctly internal. DiagnosticIds.ResolvePathBeforeExtraction is public only because the surrounding class already is; consistent with the existing pattern.

Dimensions not applicable to this change: ChangeWave (2), target authoring (9), evaluation model (21), SDK boundaries (15), logging/binlog (6), concurrency (13), security (24), dependencies (23).

Generated by Expert Code Review (on open) for #14972 · copilot · auto · 159 AIC · ⌖ 5.47 AIC · ⊞ 8.9K · ◷

[github-actions]

Expert MSBuild review — PR #14972

Solid, well-documented change with genuinely thorough test coverage (aliases, using static, named args, trivia, nested extractions, nullable flow, scope, Fix All). No blocking issues.

#	Dimension	Verdict
22	Correctness & Edge Cases	🟡 2 MODERATE
5	Error Message Quality	🟡 1 MODERATE
4	Test Coverage	🟡 1 MODERATE
3/12	Performance / Simplification	⚪ 1 NIT

✅ 20/24 dimensions clean.

• GetInvertedPathExtraction does not exempt an already-absolute inner input — false positive on GetAbsolutePath(Path.GetDirectoryName(GetAbsolutePath(x))), and the offered fix produces a double resolve.
• FindPathArgument can now return null where a non-null ArgumentSyntax was previously guaranteed — silently drops an existing MSBuildTask0003 fix.
• MSBuildTask0015 message hardcodes Path./TaskEnvironment, so it is non-compiling guidance under using static or a local TaskEnvironment receiver (both shapes the analyzer explicitly supports).
• No regression test that the non-extraction maybe-null MSBuildTask0003 fix is still offered after the new withholding guard, nor for nullable-disabled (FlowState.None).

Also left a PR-level comment on the return; ordering between 0015 and 0003, and on PR scope.

Note: attempted delegation to a sub-reviewer agent three times; the agent runtime returned no response each time, so this review was completed directly.

Generated by Expert Code Review (on open) for #14972 · copilot · auto · 159 AIC · ⌖ 5.47 AIC · ⊞ 8.9K

Comments that could not be inline-anchored

src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs:170

[MODERATE] Dimension 22: Correctness & Edge Cases

This changes the return from a value already proven non-null (argumentSyntax, matched by is { } and verified present in argumentList.Arguments) to a fresh FirstAncestorOrSelf<ArgumentSyntax>() on a different node, which can be null.

Scenario: the peeled inner expression is not enclosed in an ArgumentSyntax — e.g. the extraction's operation syntax is synthesized/reduced (extension-method or lowered form), or the peel lands on …

src/TaskAnalyzer/SharedAnalyzerHelpers.cs:297

[MODERATE] Dimension 22: Correctness & Edge Cases (also Dimension 1: Backwards Compatibility)

GetInvertedPathExtraction flags any GetAbsolutePath(Path.GetDirectoryName(x)), without asking whether x is already rooted — unlike the sibling logic in IsWrappedSafely, which does exactly that check a few lines above.

Concrete scenario:

TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile)))

The inner input is already absolute,…

src/TaskAnalyzer/DiagnosticDescriptors.cs:145

[MODERATE] Dimension 5: Error Message Quality

The message hardcodes both Path. and TaskEnvironment, but the analyzer deliberately matches aliases, using static, and non-property receivers (your own tests cover IOPath.GetPathRoot, using static System.IO.Path, and environment.GetAbsolutePath(...) in Fix_InvertedPathExtraction_FixAllPreservesReceiver).

For the using static + local-receiver shape the emitted guidance —
`'Path.GetDirectoryName' can return ... use Path.GetDirect…

src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs:117

[MODERATE] Dimension 4: Test Coverage & Completeness

This new guard changes the pre-existing MSBuildTask0003 fixer, and the added coverage only exercises the extraction shape (Fix_PathExtraction_NullableInput always wraps the argument in Path.{method}(...)!). There is no regression test asserting that the plain, non-extraction maybe-null case is still fixed — e.g. string? TargetFile with Directory.CreateDirectory(TargetFile) under NullableContextOptions.Enable. That path is…

src/TaskAnalyzer/SharedAnalyzerHelpers.cs:291

[NIT] Dimension 12: Code Simplification / Dimension 22: Correctness

ToDisplayString() allocates a formatted string per candidate invocation and is compared ordinally against a literal. The file already does this for GetFullPath, so it is consistent — but since this predicate now runs on every invocation operation in every analyzed task (the new MSBuildTask0015 path), it is worth resolving System.IO.Path once via compilation.GetTypeByMetadataName("System.IO.Path") and using `Symbo…

[Copilot]

🟢 Approval recommended

The behavior change is scoped to the opt-in analyzer, is backed by extensive new unit tests, and the only outstanding feedback is a small documentation-precision tweak.

Pull request overview

This PR improves the MSBuild TaskAnalyzer’s path-safety analysis and code fixes by preventing generation of the inverted composition GetAbsolutePath(GetDirectoryName(...)) (which can throw on bare filenames) and by adding a new diagnostic/fix to detect and correct existing inverted usages.

Changes:

• Extend “safe wrapper” recognition to treat Path.GetDirectoryName(...) and Path.GetPathRoot(...) of an already-safe (absolute) path as safe.
• Add MSBuildTask0015 to detect TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName(...)) / GetPathRoot(...) and offer a swap fix (when the result is consumed as string and the input is non-nullable).
• Update docs, unshipped analyzer release notes, and add targeted analyzer/code-fix regression tests (aliases, using static, named args, nested extractions, nullable flows, Fix All).

File summaries

File	Description
src/TaskAnalyzer/SharedAnalyzerHelpers.cs	Adds shared helpers for identifying path extractions and detecting inverted extraction composition.
src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs	Reports new MSBuildTask0015 diagnostic under scope gating.
src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs	Updates MSBuildTask0003 fix to wrap the original path under extractions; adds MSBuildTask0015 swap fix.
src/TaskAnalyzer/DiagnosticIds.cs	Introduces MSBuildTask0015 ID constant.
src/TaskAnalyzer/DiagnosticDescriptors.cs	Defines MSBuildTask0015 descriptor and registers it.
src/TaskAnalyzer/AnalyzerReleases.Unshipped.md	Documents new unshipped analyzer rule MSBuildTask0015.
src/TaskAnalyzer/README.md	Documents MSBuildTask0015 behavior and code-fix availability.
src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs	Adds regression tests for extraction wrapping, inversion swap, nullable gating, and Fix All behavior.
src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs	Adds analyzer coverage for detection, safe forms, scope behavior, and lookalike exclusions.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.