dotnet / dotnet/msbuild

TaskAnalyzer: MSBuildTask0002 is not reported for code inherited from an unannotated base class

Open
#14,783 1 comment 1 reaction 2 assignees Claimed by @ViktorHofer View on GitHub
Area: Analyzer Priority:1 triaged
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

### Issue Description

`MSBuildTask0002` (and, by the same mechanism, `0003`/`0005`) is not reported when the offending code lives in a **base class** of a task that carries `[MSBuildMultiThreadableTask]`.

#14772 already tracks base-class scope limitations, but only under **MSBuildTask0006** ("Base-class analysis") and **MSBuildTask0007**. This issue reports that the same limitation applies to the *path and environment* rules, which is where it actually causes silent correctness bugs.

This is not a theoretical concern: it is the dominant shape in the Arcade task migration ([dotnet/arcade#17381](https://github.com/dotnet/arcade/pull/17381), tracked by [dotnet/arcade#17378](https://github.com/dotnet/arcade/issues/17378)). 14 of 136 tasks had to be de-annotated *after* CI failures because their process-global state lived in a base class that the analyzer never looked at. The analyzer reported a clean build for every one of them.

### Steps to Reproduce

Analyzer `Microsoft.Build.TaskAuthoring.Analyzer` `18.11.0-1.26420.118`, `Microsoft.Build.Utilities.Core` `18.8.2`, with:

```ini
is_global = true
msbuild_task_analyzer.scope = multithreadable_only
```

```csharp
using System;
using System.IO;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;

// ---------- CONTROL A: usage directly in the annotated class ----------
[MSBuildMultiThreadableTask]
public sealed class ControlDirect : Task, IMultiThreadableTask
{
public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;

public override bool Execute()
{
string a = Environment.GetEnvironmentVariable("SYSTEM_ACCESSTOKEN");
string b = Directory.GetCurrentDirectory();
string c = Environment.CurrentDirectory;
Log.LogMessage(a + b + c);
return true;
}
}

// ---------- CONTROL B: same usage via a same-class static helper ----------
[MSBuildMultiThreadableTask]
public sealed class ControlSameClassHelper : Task, IMultiThreadableTask
{
public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;

public override bool Execute()
{
Log.LogMessage(Read());
return true;
}

private static string Read() => Environment.GetEnvironmentVariable("SYSTEM_ACCESSTOKEN");
}

// ---------- THE GAP: identical usage, but in an unannotated base ----------
public abstract class BaseWithEnv : Task
{
protected string Token => Environment.GetEnvironmentVariable("SYSTEM_ACCESSTOKEN");
protected string Here() => Directory.GetCurrentDirectory();

public override bool Execute()
{
Log.LogMessage(Token + Here());
return ExecuteCore();
}

protected abstract bool ExecuteCore();
}

[MSBuildMultiThreadableTask]
public sealed class DerivedAnnotated : BaseWithEnv, IMultiThreadableTask
{
public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;

protected override bool ExecuteCore() => true;
}
```

Build with `dotnet build -c Release --no-incremental`.

### Expected Behavior

`DerivedAnnotated` is the type the attribute is applied to, and it is the type MSBuild will schedule onto a shared node. The environment and CWD reads it inherits execute on that shared node, so they should be reported — as they are for the two controls.

### Actual Behavior

The two controls are reported. The inherited case is silent.

```
Control.cs(16,24): warning MSBuildTask0002: 'Environment.GetEnvironmentVariable(string)' should use TaskEnvironment alternative: use TaskEnvironment.GetEnvironmentVariable instead
Control.cs(17,24): warning MSBuildTask0002: 'Directory.GetCurrentDirectory()' should use TaskEnvironment alternative: use TaskEnvironment.ProjectDirectory instead
Control.cs(18,24): warning MSBuildTask0002: 'Environment.CurrentDirectory' should use TaskEnvironment alternative: use TaskEnvironment.ProjectDirectory instead
Control.cs(50,41): warning MSBuildTask0002: 'Environment.GetEnvironmentVariable(string)' should use TaskEnvironment alternative: use TaskEnvironment.GetEnvironmentVariable instead

(nothing for BaseWithEnv / DerivedAnnotated)
```

Control B is the significant one: it proves the analyzer **does** perform interprocedural analysis, and reaches a `private static` helper in the same type. The analysis therefore stops at the **type** boundary specifically, not at the method boundary.

`MSBuildTask0011` *does* fire on `DerivedAnnotated`, so the type is recognised as an annotated task — the rule simply never inspects its inherited members.

### Impact

This is the worst possible failure mode for a migration aid: it is silent, and it is silent precisely on the tasks that are hardest to reason about. A task author annotates a class, gets a clean analyzer run, ships it, and the shared-node violation only surfaces as a nondeterministic CI failure.

### Suggested Fix

When a type carries `[MSBuildMultiThreadableTask]`, analyze the transitive closure of members it inherits, not just members it declares. Diagnostics can be reported at the declaration site in the base with a message naming the annotated derived type (or on the derived type's attribute), whichever is less noisy when many tasks share a base.

A cheaper interim mitigation that would have caught all 14 Arcade cases: report an informational diagnostic on the annotated type whenever it derives from a base outside the current compilation, or from an unannotated base that itself contains banned APIs, telling the author the base was not verified.

### Versions & Configurations

- `Microsoft.Build.TaskAuthoring.Analyzer` `18.11.0-1.26420.118`
- `Microsoft.Build.Utilities.Core` `18.8.2`
- SDK `11.0.0-preview.6.26359.118`

### Related

- #14772 — tracks the same limitation for `MSBuildTask0006`/`0007`
- #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

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.