dotnet / dotnet/msbuild

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

Open
#14,971 0 comments 1 reaction 2 assignees Claimed by @ViktorHofer View on GitHub
Area: Analyzer
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.