dotnet / dotnet/msbuild

TaskAnalyzer: null-conditional access defeats safe-path detection (fi.Directory?.FullName flagged, fi.Directory.FullName not)

Open Beginner friendly
#14,979 0 comments 0 reactions 0 assignees View on GitHub
Area: Analyzer
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

`SharedAnalyzerHelpers.IsWrappedSafely` never unwraps `IConditionalAccessOperation`, so any safe path expression reached through `?.` falls through to `return false` and gets flagged.

Found while migrating arcade tasks (dotnet/arcade#17507), where the only workaround was to restructure the code to avoid `?.`.

### Repro

Inside a `[MSBuildMultiThreadableTask]` task, with `fi` a `FileInfo` obtained safely:

```csharp
Directory.CreateDirectory(fi.Directory.FullName); // OK - no diagnostic
Directory.CreateDirectory(fi.Directory?.FullName); // MSBuildTask0003
```

Both expressions have identical path semantics, but only the second is flagged:

> MSBuildTask0003: `Directory.CreateDirectory(...)` may resolve a relative path against the shared working directory

The same gap applies to the other safe forms, e.g. `item?.GetMetadata("FullPath")` and `TaskEnvironment?.GetAbsolutePath(p)`.

### Cause

`IsWrappedSafely` unwraps `IConversionOperation` in a loop (SharedAnalyzerHelpers.cs:125) but has no case for `IConditionalAccessOperation` — there are no references to that operation kind anywhere in `src/TaskAnalyzer`. For `a?.B`, Roslyn produces an `IConditionalAccessOperation` whose `WhenNotNull` holds the actual member access, so none of the existing safe-expression rules are ever reached.

### Suggested fix

Recurse into `WhenNotNull`, next to the existing conversion unwrapping:

```csharp
// Unwrap null-conditional access: `info?.FullName` has the same path
// semantics as `info.FullName`; the null branch yields no path at all.
if (operation is IConditionalAccessOperation conditionalAccess)
{
return IsWrappedSafely(conditionalAccess.WhenNotNull, taskEnvironmentType, absolutePathType, iTaskItemType);
}
```

The receiver inside `WhenNotNull` is an `IConditionalAccessInstanceOperation` placeholder, but the rules that matter here (`FileSystemInfo.FullName`, `ITaskItem.GetMetadata`, `TaskEnvironment.GetAbsolutePath`) match on the member itself and do not inspect the receiver, so the recursion resolves correctly.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/TaskAnalyzer/SharedAnalyzerHelpers.cs at IsWrappedSafely and review the existing conversion unwrapping and safe-expression rules. Reproduce the two Directory.CreateDirectory expressions and the other null-conditional forms from the issue, then verify that safe paths reached through IConditionalAccessOperation are no longer diagnosed while unsafe paths remain covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
build-system
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
86/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.