TaskAnalyzer: code fix generates GetAbsolutePath(Path.GetDirectoryName(x)) and no diagnostic flags the inverted composition
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 133
Description
The analyzer understands the *correct* composition but has no diagnostic for the *inverted* one, and the MSBuildTask0003 code fix actively produces the inverted one.
### Correct vs. inverted
```csharp
// Correct: resolve the path, then take its parent.
Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile))
// Inverted: takes the parent of a *relative* string, then resolves that.
TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName(TargetFile))
```
`SharedAnalyzerHelpers.cs:177-182` explicitly models the correct form:
```csharp
// Check: Path.GetDirectoryName(safe) - directory of an absolute path is absolute
if (invocation.TargetMethod.Name == "GetDirectoryName" && ...
```
There is no corresponding check for the inverted form.
### The code fix generates the inverted form
`WrapArgumentWithGetAbsolutePathAsync` (`MultiThreadableTaskCodeFixProvider.cs:210-225`) wraps the flagged argument expression verbatim, without inspecting it. So for the very common shape:
```csharp
Directory.CreateDirectory(Path.GetDirectoryName(TargetFile));
```
"Wrap with `TaskEnvironment.GetAbsolutePath()`" produces:
```csharp
Directory.CreateDirectory(TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName(TargetFile)));
```
`IsAlreadyWrapped` (`MultiThreadableTaskCodeFixProvider.cs:105-115`) then matches on the outer `GetAbsolutePath` identifier only, so the result is treated as fixed and never re-flagged. The migration looks complete and stays silently wrong.
### Why the inverted form is wrong
`Path.GetDirectoryName` returns `""` for a bare filename and `null` for a root, and `AbsolutePath` rejects both:
- `AbsolutePath.cs:107` - `ArgumentException.ThrowIfNullOrEmpty(path)` in `AbsolutePath(string, AbsolutePath)`
- `AbsolutePath.cs:87` - same in `ValidatePath`
So with `TargetFile="list.xml"`, the inverted form throws `ArgumentException` from deep inside `GetAbsolutePath`, whereas the correct form yields `` and works. It also resolves the wrong thing conceptually: `GetAbsolutePath` is applied to a string that is already one level up, so any base-path anchoring logic (`MakeFullyQualifiedRelativeToBasePath`) operates on the parent rather than the path itself.
### Real-world hit rate
While migrating tasks in dotnet/arcade (dotnet/arcade#17497) I wrote this inverted form at **4 of 4** sites that used the `Directory.CreateDirectory(Path.GetDirectoryName(x))` idiom, and the analyzer accepted all 4. They were only caught by manual review:
- `CreateFrameworkListFile.cs` (`TargetFile`)
- `WriteBuildOutputProps.cs` (`OutputPath`)
- `WritePackageUsageData.cs` x2 (`DataFile`, `ProjectAssetsJsonArchiveFile`)
### Suggested fix
Either (or both):
1. **Diagnostic**: flag `TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName(x))` and suggest swapping to `Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(x))`. `Path.GetPathRoot` has the same `""`/`null` behavior and deserves the same treatment. This is a purely syntactic pattern, so false positives should be negligible.
2. **Smarter fixer**: when the argument to wrap is itself a `Path.GetDirectoryName(inner)` / `Path.GetPathRoot(inner)` call, wrap `inner` instead of the whole expression.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.