TaskAnalyzer: no diagnostic when a path property is resolved at one use site but used raw at another
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
### Issue Description
There is no diagnostic for **inconsistent path resolution**: a task property that is correctly wrapped in `TaskEnvironment.GetAbsolutePath()` at one use site, but consumed raw at a sibling site in the same method.
This is distinct from the "Insufficient path-state tracking" item under `MSBuildTask0003` in #14772. That item is about the *state of a resolved value* (canonical / qualified / command name). This is about the *unresolved* value continuing to be used alongside the resolved one — typically in a length offset or a prefix comparison, where no path API is involved at all, so no existing rule looks at it.
It is the single most common bug class we hit in the Arcade migration: **10 real defects** across [dotnet/arcade#17381](https://github.com/dotnet/arcade/pull/17381), every one of them introduced *by following the analyzer's own `MSBuildTask0003` guidance* on only the call site it flagged.
### Steps to Reproduce
```csharp
[MSBuildMultiThreadableTask]
public sealed class PartialResolutionTask : Task, IMultiThreadableTask
{
public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;
[Required]
public string Root { get; set; }
public override bool Execute()
{
foreach (string f in Directory.GetFiles(TaskEnvironment.GetAbsolutePath(Root), "*", SearchOption.AllDirectories))
{
// Root is still relative here, but f is absolute.
string rel = f.Substring(Root.Length + 1);
if (!f.StartsWith(Root, StringComparison.Ordinal))
{
Log.LogMessage(rel);
}
}
return true;
}
}
```
### Expected Behavior
A diagnostic on the `Root.Length` and `StartsWith(Root)` uses, noting that `Root` is resolved elsewhere in the same scope and that mixing resolved and unresolved forms of the same value produces incorrect results.
### Actual Behavior
No diagnostic. The only thing reported is `MSBuildTask0006` on the property itself:
```
Repro.cs(80,53): warning MSBuildTask0006: Consider changing task property 'Root' from 'string' to 'AbsolutePath' instead of converting inside the task body
```
`MSBuildTask0006` is a good hint here, but it is advisory, is not a correctness statement, and does not point at the two defective expressions.
### Why This Matters More Than It Looks
Every one of these produces a **silently wrong result rather than an exception**:
- `f.Substring(Root.Length + 1)` — `f` is absolute, `Root` is relative, so the offset is short by the length of the working directory prefix. The result is a mangled relative path, not a crash.
- `f.StartsWith(Root)` — always `false`, so a filter silently matches nothing (or everything, when negated).
Because there is no exception, these survive unit tests that use absolute inputs, and only manifest when the task is invoked with a relative property value.
A concrete example from the migration, [`WritePackageUsageData.cs`](https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs), had **five** use sites of `RootDir`. `MSBuildTask0003` flagged the `Directory.GetFiles` call. The other four — two `Path.Combine`, one `Substring` offset, one `StartsWith` — were invisible to the analyzer, and the last of them was missed by human code review as well.
### Suggested Fix
Within a method body, when a symbol (task property, parameter, or local) flows into `TaskEnvironment.GetAbsolutePath()` at least once, report the other reads of that same symbol that flow into:
- `string.Length` used as an index or offset against a value derived from the resolved form,
- `StartsWith` / `EndsWith` / `IndexOf` / `Equals` / `Replace` against such a value,
- `Path.Combine` / `Path.GetDirectoryName` / any path-shaped API,
with a message along the lines of *"'Root' is resolved with `TaskEnvironment.GetAbsolutePath` elsewhere in this method but is used unresolved here; the two forms are not interchangeable."*
This would also make the `MSBuildTask0006` suggestion actionable, since converting the property to `AbsolutePath` is precisely the fix that removes the whole class.
We wrote a crude standalone checker for exactly this pattern while auditing the migration and it had a very high signal-to-noise ratio — it found one true positive that 24 inline review comments had missed, with no false positives across 136 tasks. Happy to share it if useful.
### Versions & Configurations
- `Microsoft.Build.TaskAuthoring.Analyzer` `18.11.0-1.26420.118`
- `Microsoft.Build.Utilities.Core` `18.8.2`
- `msbuild_task_analyzer.scope = multithreadable_only`
### Related
- #14772, #14779, #14780
- [dotnet/arcade#17378](https://github.com/dotnet/arcade/issues/17378), [dotnet/arcade#17381](https://github.com/dotnet/arcade/pull/17381)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the TaskAnalyzer rules related to MSBuildTask0003 and MSBuildTask0006, then reproduce the PartialResolutionTask example from the issue. Trace how symbols flow through TaskEnvironment.GetAbsolutePath and define diagnostics for unresolved sibling reads; done means the Root.Length and StartsWith(Root) uses are reported without flagging unrelated reads.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system, developer-experience
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 58/100