Skip to content

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

Description

@ViktorHofer

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

// 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:

// 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:

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

"Wrap with TaskEnvironment.GetAbsolutePath()" produces:

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 <ProjectDirectory> 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions