Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/TaskAnalyzer.Tests/PreferTypedParameterAnalyzerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public override bool Execute()
diags.Length.ShouldBe(1);
diags[0].GetMessage().ShouldContain("InputPath");
diags[0].GetMessage().ShouldContain("AbsolutePath");
diags[0].Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
diags[0].Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Info);
}

[Fact]
Expand All @@ -66,7 +66,7 @@ public override bool Execute()
diags.ShouldNotContain(d => d.Id == DiagnosticIds.PreferTypedPathParameter);
diags[0].GetMessage().ShouldContain("InputPath");
diags[0].GetMessage().ShouldContain("AbsolutePath");
diags[0].Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
diags[0].Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Info);
}

[Fact]
Expand Down Expand Up @@ -373,7 +373,7 @@ public override bool Execute()
diags.Length.ShouldBe(1);
diags[0].GetMessage().ShouldContain("int");
diags[0].GetMessage().ShouldContain("Item");
diags[0].Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
diags[0].Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Info);
}

[Fact]
Expand Down
49 changes: 43 additions & 6 deletions src/TaskAnalyzer.Tests/UnsupportedTaskItemTypeAnalyzerTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Linq;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Shouldly;
Expand Down Expand Up @@ -45,7 +46,7 @@ public class MyTask : Microsoft.Build.Utilities.Task
[InlineData("double")]
[InlineData("decimal")]
[InlineData("System.DateTime")]
public async Task ConvertChangeTypeType_ProducesError(string typeName)
public async Task ConvertChangeTypeType_ProducesWarning(string typeName)
{
var diags = await GetUnsupportedTaskItemTypeDiagnosticsAsync($$"""
using Microsoft.Build.Framework;
Expand All @@ -59,7 +60,7 @@ public class MyTask : Microsoft.Build.Utilities.Task
diags.ShouldNotContain(d => d.Id == DiagnosticIds.UnsupportedTaskItemType);
Diagnostic diagnostic = diags.ShouldHaveSingleItem();
diagnostic.Id.ShouldBe(DiagnosticIds.CultureSensitiveTaskItemType);
diagnostic.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Error);
diagnostic.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
diagnostic.GetMessage().ShouldContain("Convert.ChangeType");
diagnostic.GetMessage().ShouldContain("CultureInfo.InvariantCulture");
}
Expand Down Expand Up @@ -116,7 +117,7 @@ public class MyTask : Microsoft.Build.Utilities.Task
// ═══════════════════════════════════════════════════════════════════════

[Fact]
public async Task ConvertChangeTypeArray_ProducesError()
public async Task ConvertChangeTypeArray_ProducesWarning()
{
var diags = await GetUnsupportedTaskItemTypeDiagnosticsAsync("""
using Microsoft.Build.Framework;
Expand All @@ -129,7 +130,7 @@ public class MyTask : Microsoft.Build.Utilities.Task

Diagnostic diagnostic = diags.ShouldHaveSingleItem();
diagnostic.Id.ShouldBe(DiagnosticIds.CultureSensitiveTaskItemType);
diagnostic.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Error);
diagnostic.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
}

[Fact]
Expand All @@ -149,7 +150,7 @@ public class MyTask : Microsoft.Build.Utilities.Task
}

[Fact]
public async Task ConvertChangeTypeOutputProperty_ProducesError()
public async Task ConvertChangeTypeOutputProperty_ProducesWarning()
{
var diags = await GetUnsupportedTaskItemTypeDiagnosticsAsync("""
using Microsoft.Build.Framework;
Expand All @@ -163,7 +164,7 @@ public class MyTask : Microsoft.Build.Utilities.Task

Diagnostic diagnostic = diags.ShouldHaveSingleItem();
diagnostic.Id.ShouldBe(DiagnosticIds.CultureSensitiveTaskItemType);
diagnostic.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Error);
diagnostic.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
}

[Fact]
Expand Down Expand Up @@ -201,6 +202,7 @@ public class MyTask : Microsoft.Build.Utilities.Task
""");

diags.ShouldContain(d => d.Id == DiagnosticIds.UnsupportedTaskItemType);
diags.ShouldHaveSingleItem().Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
diags[0].GetMessage().ShouldContain("Item");
diags[0].GetMessage().ShouldContain("Guid");
diags[0].GetMessage().ShouldContain("string, bool, AbsolutePath, FileInfo, DirectoryInfo");
Expand All @@ -225,6 +227,41 @@ public class MyTask : Microsoft.Build.Utilities.Task
diags[0].GetMessage().ShouldContain("TimeSpan");
}

[Fact]
public async Task TypedTaskItemDiagnostics_AreIndependentOfMtOptIn()
{
var diags = await GetUnsupportedTaskItemTypeDiagnosticsAsync("""
using System;
using Microsoft.Build.Framework;
public class MyTask : Microsoft.Build.Utilities.Task
{
public ITaskItem<Guid> Invalid { get; set; } = null!;
public ITaskItem<int> CultureSensitive { get; set; } = null!;
public override bool Execute() => true;
}
""");

diags.Where(d => d.Id == DiagnosticIds.UnsupportedTaskItemType).ShouldHaveSingleItem()
.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
diags.Where(d => d.Id == DiagnosticIds.CultureSensitiveTaskItemType).ShouldHaveSingleItem()
.Severity.ShouldBe(Microsoft.CodeAnalysis.DiagnosticSeverity.Warning);
}

[Fact]
public async Task GenericTaskItemTypeParameter_NoDiagnostic()
{
var diags = await GetUnsupportedTaskItemTypeDiagnosticsAsync("""
using Microsoft.Build.Framework;
public class GenericTask<T> : Microsoft.Build.Utilities.Task
{
public ITaskItem<T> Item { get; set; } = null!;
public override bool Execute() => true;
}
""");

diags.ShouldBeEmpty();
}

[Fact]
public async Task Enum_ProducesDiagnostic()
{
Expand Down
8 changes: 4 additions & 4 deletions src/TaskAnalyzer/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ MSBuildTask0002 | MSBuild.TaskAuthoring | Warning | APIs that should use TaskEnv
MSBuildTask0003 | MSBuild.TaskAuthoring | Warning | File APIs that need absolute paths
MSBuildTask0004 | MSBuild.TaskAuthoring | Warning | APIs that may cause issues in multithreaded task execution
MSBuildTask0005 | MSBuild.TaskAuthoring | Warning | Transitive unsafe API usage detected in task call chain
MSBuildTask0006 | MSBuild.TaskAuthoring | Warning | Prefer typed path parameter (AbsolutePath/FileInfo/DirectoryInfo) over string (code fix available)
MSBuildTask0007 | MSBuild.TaskAuthoring | Warning | Prefer ITaskItem<T> over manual ItemSpec parsing (code fix available)
MSBuildTask0008 | MSBuild.TaskAuthoring | Warning | Initialize a relative default path in Execute() so TaskEnvironment can root it when the property is retyped (code fix available)
MSBuildTask0006 | MSBuild.TaskAuthoring | Info | Prefer typed path parameter (AbsolutePath/FileInfo/DirectoryInfo) over string (code fix available)
MSBuildTask0007 | MSBuild.TaskAuthoring | Info | Prefer ITaskItem<T> over manual ItemSpec parsing (code fix available)
MSBuildTask0008 | MSBuild.TaskAuthoring | Info | Initialize a relative default path in Execute() so TaskEnvironment can root it when the property is retyped (code fix available)
Comment thread
VolPlita marked this conversation as resolved.
MSBuildTask0009 | MSBuild.TaskAuthoring | Warning | ITaskItem<T> used with a type argument T that MSBuild cannot bind as a task parameter
MSBuildTask0010 | MSBuild.TaskAuthoring | Error | ITaskItem<T> used with a type argument T that MSBuild parses through Convert.ChangeType
MSBuildTask0010 | MSBuild.TaskAuthoring | Warning | ITaskItem<T> used with a type argument T that MSBuild parses through Convert.ChangeType
MSBuildTask0011 | MSBuild.TaskAuthoring | Info | Prefer constructor injection for TaskEnvironment
MSBuildTask0012 | MSBuild.TaskAuthoring | Warning | TaskEnvironment property is never assigned by MSBuild because the task does not implement IMultiThreadableTask
MSBuildTask0013 | MSBuild.TaskAuthoring | Info | Task declares IMultiThreadableTask but is not marked with [MSBuildMultiThreadableTask] (disabled by default)
Expand Down
8 changes: 4 additions & 4 deletions src/TaskAnalyzer/DiagnosticDescriptors.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ internal static class DiagnosticDescriptors
title: "Prefer typed path parameter over manual path construction",
messageFormat: "Consider changing task property '{0}' from '{1}' to '{2}' instead of converting inside the task body",
category: "MSBuild.TaskAuthoring",
defaultSeverity: DiagnosticSeverity.Warning,
defaultSeverity: DiagnosticSeverity.Info,
isEnabledByDefault: true,
description: "MSBuild can bind AbsolutePath, FileInfo, and DirectoryInfo task parameters automatically for tasks that opt into multithreaded support. Using these types avoids manual path construction in the task body.");

Expand All @@ -72,7 +72,7 @@ internal static class DiagnosticDescriptors
title: "Prefer ITaskItem<T> over manual ItemSpec parsing",
messageFormat: "Consider changing task property '{0}' from '{1}' to 'ITaskItem<{2}>{3}' instead of parsing ItemSpec manually",
category: "MSBuild.TaskAuthoring",
defaultSeverity: DiagnosticSeverity.Warning,
defaultSeverity: DiagnosticSeverity.Info,
isEnabledByDefault: true,
description: "MSBuild can bind ITaskItem<T> task parameters that provide a strongly-typed Value property parsed from ItemSpec for tasks that opt into multithreaded support. Using ITaskItem<T> avoids manual parsing in the task body.");

Expand All @@ -81,7 +81,7 @@ internal static class DiagnosticDescriptors
title: "Initialize relative default path in Execute()",
messageFormat: "Task property '{0}' has a relative default path; initialize it in Execute() so it can be rooted through TaskEnvironment when the property is changed to '{1}'",
category: "MSBuild.TaskAuthoring",
defaultSeverity: DiagnosticSeverity.Warning,
defaultSeverity: DiagnosticSeverity.Info,
isEnabledByDefault: true,
description: "A relative default path cannot be rooted in a property initializer because the MSBuild engine only assigns TaskEnvironment after the task is constructed. Move the default into Execute(), where TaskEnvironment.GetAbsolutePath can resolve it, guarding the assignment so a value bound from the project is not overwritten.");

Expand All @@ -99,7 +99,7 @@ internal static class DiagnosticDescriptors
title: "ITaskItem<T> type argument relies on culture-sensitive conversion",
messageFormat: "Task property '{0}' uses ITaskItem<{1}>, which MSBuild parses through Convert.ChangeType using CultureInfo.InvariantCulture. Use ITaskItem<string> and parse explicitly with a chosen culture.",
category: "MSBuild.TaskAuthoring",
defaultSeverity: DiagnosticSeverity.Error,
defaultSeverity: DiagnosticSeverity.Warning,
isEnabledByDefault: true,
description: "ITaskItem<T> type arguments parsed through Convert.ChangeType use CultureInfo.InvariantCulture. Bind the item as a string and parse it explicitly with the intended culture.");

Expand Down
24 changes: 10 additions & 14 deletions src/TaskAnalyzer/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ This analyzer catches unsafe API usage at compile time and offers code fixes to
| **MSBuildTask0003** | Warning | All `ITask` implementations | File system API requires absolute path |
| **MSBuildTask0004** | Warning | All `ITask` implementations | API may cause issues in multithreaded tasks |
| **MSBuildTask0005** | Warning | All `ITask` implementations | Transitive unsafe API usage in task call chain |
| **MSBuildTask0006** | Warning | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer typed path parameter over string |
| **MSBuildTask0007** | Warning | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer `ITaskItem<T>` over manual ItemSpec parsing |
| **MSBuildTask0008** | Warning | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Initialize a relative-default path property in `Execute()` |
| **MSBuildTask0006** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer typed path parameter over string |
| **MSBuildTask0007** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer `ITaskItem<T>` over manual ItemSpec parsing |
| **MSBuildTask0008** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Initialize a relative-default path property in `Execute()` |
| **MSBuildTask0009** | Warning | All `ITask` implementations | `ITaskItem<T>` used with unsupported type argument |
| **MSBuildTask0010** | Error | All `ITask` implementations | `ITaskItem<T>` relies on culture-sensitive conversion |
| **MSBuildTask0010** | Warning | All `ITask` implementations | `ITaskItem<T>` relies on culture-sensitive conversion |
| **MSBuildTask0011** | Info | Concrete `IMultiThreadableTask` implementations | Prefer constructor injection for `TaskEnvironment` |
| **MSBuildTask0012** | Warning | Concrete tasks with `[MSBuildMultiThreadableTask]` applied directly | MSBuild never assigns the `TaskEnvironment` property |
| **MSBuildTask0013** | Info (off by default) | Concrete tasks declaring `IMultiThreadableTask` in their own base list | Missing `[MSBuildMultiThreadableTask]`, so the task still runs out-of-proc |
Expand Down Expand Up @@ -245,7 +245,7 @@ public class MyTask : Task
{
public ITaskItem<System.Guid> Id { get; set; } // warning
public ITaskItem<System.TimeSpan>[] Durations { get; set; } // warning
public ITaskItem<int> Count { get; set; } // MSBuildTask0010 error
public ITaskItem<int> Count { get; set; } // MSBuildTask0010 warning
}
```

Expand All @@ -255,13 +255,13 @@ No code fix is offered for MSBuildTask0009 — the resolution depends on the int

### MSBuildTask0010 — Culture-Sensitive `ITaskItem<T>` Conversion

MSBuild binds `ITaskItem<T>` for `char`, numeric primitives, `decimal`, and `DateTime` through `Convert.ChangeType` using `CultureInfo.InvariantCulture`. Because this implicit conversion may not match the task's intended culture, the analyzer reports an **Error** whenever one of these types is used.
MSBuild binds `ITaskItem<T>` for `char`, numeric primitives, `decimal`, and `DateTime` through `Convert.ChangeType` using `CultureInfo.InvariantCulture`. Because this implicit conversion may not match the task's intended culture, the analyzer reports a **Warning** whenever one of these types is used.

```csharp
public class MyTask : Task
{
public ITaskItem<int> Count { get; set; } // error
public ITaskItem<DateTime>[] Dates { get; set; } // error
public ITaskItem<int> Count { get; set; } // warning
public ITaskItem<DateTime>[] Dates { get; set; } // warning
}
```

Expand Down Expand Up @@ -414,12 +414,8 @@ The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classe
### Severity Levels

- **MSBuildTask0001** is always **Error** — these APIs are never safe in any MSBuild task.
- **MSBuildTask0010** is always **Error** — task item conversions must not rely on `Convert.ChangeType`.
- **MSBuildTask0002–MSBuildTask0009** report as **Warning**, with MSBuildTask0006–MSBuildTask0008 limited to tasks directly marked with `[MSBuildMultiThreadableTask]`.
- **MSBuildTask0011** reports as **Info** — it is a modernization suggestion rather than a correctness issue.
- **MSBuildTask0012** reports as **Warning** — the `TaskEnvironment` property is silently inert, which is a correctness issue.
- **MSBuildTask0013** is **disabled by default** — running out-of-proc is a performance characteristic, and the shape it reports is a valid intermediate migration state.
- **MSBuildTask0014** reports as **Warning** — the attribute is inert, and the task the author meant to mark is usually still running out-of-proc.
- **MSBuildTask0002–MSBuildTask0005, MSBuildTask0009, and MSBuildTask0010** report as **Warning**.
- **MSBuildTask0006–MSBuildTask0008 and MSBuildTask0011** report as **Info** — these are modernization suggestions, not correctness issues.

## Code Fixes

Expand Down
7 changes: 7 additions & 0 deletions src/TaskAnalyzer/UnsupportedTaskItemTypeAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,13 @@ property.ContainingType is not null &&

ITypeSymbol typeArg = namedPropertyType.TypeArguments[0];

// A generic task can be constructed with a supported type. Its open type
// parameter does not provide enough information for a binding diagnostic.
if (typeArg.TypeKind == TypeKind.TypeParameter)
{
continue;
}
Comment thread
VolPlita marked this conversation as resolved.

if (SupportedTaskItemTypes.IsConvertChangeTypeTaskItemType(typeArg.SpecialType))
{
symbolContext.ReportDiagnostic(Diagnostic.Create(
Expand Down
Loading