From c7ee92a2f384e83afd79b6957746c2f2709c7f51 Mon Sep 17 00:00:00 2001 From: VolPlita Date: Fri, 21 Aug 2026 12:16:14 +0200 Subject: [PATCH 1/2] default nt only --- .../MultiThreadableTaskAnalyzerTests.cs | 119 ++++++++++++++++++ src/TaskAnalyzer.Tests/TestHelpers.cs | 76 +++++++---- .../TransitiveCallChainAnalyzerTests.cs | 68 ++++++++++ .../MultiThreadableTaskAnalyzer.cs | 8 +- src/TaskAnalyzer/README.md | 21 +++- src/TaskAnalyzer/SharedAnalyzerHelpers.cs | 8 +- 6 files changed, 262 insertions(+), 38 deletions(-) diff --git a/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs b/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs index cc204fdcddb..c955e884c0b 100644 --- a/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs +++ b/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs @@ -1870,6 +1870,125 @@ public override bool Execute() // Scope option tests // ═══════════════════════════════════════════════════════════════════════ + [Fact] + public async Task Scope_Default_PlainTask_DoesNotGetEnvironmentOrPathDiagnostics() + { + var diags = await GetDiagnosticsWithDefaultScopeAsync(""" + using System; + using System.IO; + public class PlainTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var value = Environment.GetEnvironmentVariable("KEY"); + return File.Exists("relative.txt"); + } + } + """); + + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldBeEmpty(); + diags.Where(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute).ShouldBeEmpty(); + } + + [Fact] + public async Task Scope_Default_MultiThreadableTask_GetsEnvironmentAndPathDiagnostics() + { + var diags = await GetDiagnosticsWithDefaultScopeAsync(""" + using System; + using System.IO; + using Microsoft.Build.Framework; + public class MtTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + var value = Environment.GetEnvironmentVariable("KEY"); + return File.Exists("relative.txt"); + } + } + """); + + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldHaveSingleItem(); + diags.Where(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute).ShouldHaveSingleItem(); + } + + [Fact] + public async Task Scope_All_PlainTask_GetsEnvironmentAndPathDiagnostics() + { + var diags = await GetDiagnosticsWithScopeAsync(""" + using System; + using System.IO; + public class PlainTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var value = Environment.GetEnvironmentVariable("KEY"); + return File.Exists("relative.txt"); + } + } + """, SharedAnalyzerHelpers.ScopeAll); + + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldHaveSingleItem(); + diags.Where(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute).ShouldHaveSingleItem(); + } + + [Fact] + public async Task Scope_UnrecognizedValue_UsesDefault() + { + var diags = await GetDiagnosticsWithScopeAsync(""" + using System; + public class PlainTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var value = Environment.GetEnvironmentVariable("KEY"); + return true; + } + } + """, "unrecognized"); + + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldBeEmpty(); + } + + [Fact] + public async Task Scope_Default_MultiThreadableAttribute_OptsTaskIn() + { + var diags = await GetDiagnosticsWithDefaultScopeAsync(""" + using System; + using Microsoft.Build.Framework; + [MSBuildMultiThreadableTask] + public class MtTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var value = Environment.GetEnvironmentVariable("KEY"); + return true; + } + } + """); + + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldHaveSingleItem(); + } + + [Fact] + public async Task Scope_Default_AnalyzedAttribute_OptsHelperIn() + { + var diags = await GetDiagnosticsWithDefaultScopeAsync(""" + using System; + using Microsoft.Build.Framework; + [MSBuildMultiThreadableTaskAnalyzed] + public class MtHelper + { + public void Execute() + { + var value = Environment.GetEnvironmentVariable("KEY"); + } + } + """); + + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldHaveSingleItem(); + } + [Fact] public async Task Scope_MultithreadableOnly_PlainTask_NoDiagnostic() { diff --git a/src/TaskAnalyzer.Tests/TestHelpers.cs b/src/TaskAnalyzer.Tests/TestHelpers.cs index 73bbbb2dbf2..34735c70734 100644 --- a/src/TaskAnalyzer.Tests/TestHelpers.cs +++ b/src/TaskAnalyzer.Tests/TestHelpers.cs @@ -138,34 +138,16 @@ public static string FullyQualifiedPath(string tail) => public static MetadataReference[] GetCoreReferences() => s_coreReferences; /// - /// Runs the MultiThreadableTaskAnalyzer on the given source code and returns analyzer diagnostics. - /// Source is combined with framework stubs automatically. + /// Runs the MultiThreadableTaskAnalyzer in explicit all-task migration mode. /// - public static async System.Threading.Tasks.Task> GetDiagnosticsAsync(string source) - { - var compilation = CreateCompilation(source); - var analyzer = new MultiThreadableTaskAnalyzer(); - var compilationWithAnalyzers = compilation.WithAnalyzers( - ImmutableArray.Create(analyzer)); - - var allDiags = await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); - return allDiags; - } + public static System.Threading.Tasks.Task> GetDiagnosticsAsync(string source) => + GetDiagnosticsWithScopeAsync(source, SharedAnalyzerHelpers.ScopeAll); /// - /// Runs BOTH the direct and transitive analyzers on the given source code. + /// Runs both the direct and transitive analyzers in explicit all-task migration mode. /// - public static async System.Threading.Tasks.Task> GetAllDiagnosticsAsync(string source) - { - var compilation = CreateCompilation(source); - var analyzers = ImmutableArray.Create( - new MultiThreadableTaskAnalyzer(), - new TransitiveCallChainAnalyzer()); - var compilationWithAnalyzers = compilation.WithAnalyzers(analyzers); - - var allDiags = await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); - return allDiags; - } + public static System.Threading.Tasks.Task> GetAllDiagnosticsAsync(string source) => + GetAllDiagnosticsWithScopeAsync(source, SharedAnalyzerHelpers.ScopeAll); /// /// Runs compiler diagnostics together with analyzers and suppressors and returns @@ -260,6 +242,52 @@ public static async System.Threading.Tasks.Task> GetD return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); } + /// + /// Runs the MultiThreadableTaskAnalyzer without a scope option. + /// + public static async System.Threading.Tasks.Task> GetDiagnosticsWithDefaultScopeAsync(string source) + { + var compilation = CreateCompilation(source); + var analyzer = new MultiThreadableTaskAnalyzer(); + var compilationWithAnalyzers = compilation.WithAnalyzers( + ImmutableArray.Create(analyzer)); + return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); + } + + /// + /// Runs both the direct and transitive analyzers with a specific scope option. + /// + public static async System.Threading.Tasks.Task> GetAllDiagnosticsWithScopeAsync(string source, string scope) + { + var compilation = CreateCompilation(source); + var analyzers = ImmutableArray.Create( + new MultiThreadableTaskAnalyzer(), + new TransitiveCallChainAnalyzer()); + + var globalOptions = new Dictionary + { + { $"build_property.{SharedAnalyzerHelpers.ScopeOptionKey}", scope } + }; + var optionsProvider = new TestAnalyzerConfigOptionsProvider(globalOptions); + var options = new AnalyzerOptions(ImmutableArray.Empty, optionsProvider); + + var compilationWithAnalyzers = compilation.WithAnalyzers(analyzers, options); + return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); + } + + /// + /// Runs both the direct and transitive analyzers without a scope option. + /// + public static async System.Threading.Tasks.Task> GetAllDiagnosticsWithDefaultScopeAsync(string source) + { + var compilation = CreateCompilation(source); + var analyzers = ImmutableArray.Create( + new MultiThreadableTaskAnalyzer(), + new TransitiveCallChainAnalyzer()); + var compilationWithAnalyzers = compilation.WithAnalyzers(analyzers); + return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); + } + private static MetadataReference[] CreateCoreReferences() { // Reference the core runtime assemblies needed diff --git a/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs b/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs index 44e2e5a73bb..23ad3b18b14 100644 --- a/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs +++ b/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs @@ -244,4 +244,72 @@ public override bool Execute() msg.ShouldContain("A.Step1"); msg.ShouldContain("B.Step2"); } + + [Fact] + public async Task Scope_Default_PlainTask_DoesNotGetTransitiveDiagnostic() + { + var diags = await GetAllDiagnosticsWithDefaultScopeAsync(""" + using System; + public static class Helper + { + public static void Run() => Environment.GetEnvironmentVariable("KEY"); + } + public class PlainTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + Helper.Run(); + return true; + } + } + """); + + diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ShouldBeEmpty(); + } + + [Fact] + public async Task Scope_Default_MultiThreadableTask_GetsTransitiveDiagnostic() + { + var diags = await GetAllDiagnosticsWithDefaultScopeAsync(""" + using System; + using Microsoft.Build.Framework; + public static class Helper + { + public static void Run() => Environment.GetEnvironmentVariable("KEY"); + } + public class MtTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + Helper.Run(); + return true; + } + } + """); + + diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ShouldHaveSingleItem(); + } + + [Fact] + public async Task Scope_All_PlainTask_GetsTransitiveDiagnostic() + { + var diags = await GetAllDiagnosticsWithScopeAsync(""" + using System; + public static class Helper + { + public static void Run() => Environment.GetEnvironmentVariable("KEY"); + } + public class PlainTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + Helper.Run(); + return true; + } + } + """, SharedAnalyzerHelpers.ScopeAll); + + diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ShouldHaveSingleItem(); + } } \ No newline at end of file diff --git a/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs b/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs index 26a0d711125..77c7226845e 100644 --- a/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs +++ b/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs @@ -17,8 +17,8 @@ namespace Microsoft.Build.TaskAuthoring.Analyzer /// Roslyn analyzer that detects unsafe API usage in MSBuild task implementations. /// /// Scope (controlled by .editorconfig option "msbuild_task_analyzer.scope"): - /// - "all" (default): All rules fire on ALL ITask implementations - /// - "multithreadable_only": MSBuildTask0002, 0003 fire only on IMultiThreadableTask or [MSBuildMultiThreadableTask] + /// - "multithreadable_only" (default): MSBuildTask0002 and 0003 fire only on IMultiThreadableTask or [MSBuildMultiThreadableTask] + /// - "all": Enables MSBuildTask0002 and 0003 for all ITask implementations during migration /// (MSBuildTask0001 and MSBuildTask0004 always fire on all tasks regardless) /// /// Per review feedback from @rainersigwald: @@ -30,7 +30,7 @@ public sealed class MultiThreadableTaskAnalyzer : DiagnosticAnalyzer { /// /// The .editorconfig key controlling analysis scope. - /// Values: "all" (default) | "multithreadable_only" + /// Values: "multithreadable_only" (default) | "all" /// internal const string ScopeOptionKey = SharedAnalyzerHelpers.ScopeOptionKey; internal const string ScopeAll = SharedAnalyzerHelpers.ScopeAll; @@ -55,7 +55,7 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte return; } - // Read scope option from .editorconfig: "all" (default) or "multithreadable_only" + // Read scope option from .editorconfig: "multithreadable_only" (default) or "all" bool analyzeAllTasks = SharedAnalyzerHelpers.ReadAnalyzeAllTasksOption(compilationContext.Options.AnalyzerConfigOptionsProvider); var iMultiThreadableTaskType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.IMultiThreadableTaskFullName); diff --git a/src/TaskAnalyzer/README.md b/src/TaskAnalyzer/README.md index 98a82850c7b..fb99b05f247 100644 --- a/src/TaskAnalyzer/README.md +++ b/src/TaskAnalyzer/README.md @@ -15,10 +15,10 @@ This analyzer catches unsafe API usage at compile time and offers code fixes to | ID | Severity | Scope | Title | |---|---|---|---| | **MSBuildTask0001** | Error | All `ITask` implementations | API is never safe in MSBuild tasks | -| **MSBuildTask0002** | Warning | All `ITask` implementations | API requires `TaskEnvironment` alternative | -| **MSBuildTask0003** | Warning | All `ITask` implementations | File system API requires absolute path | +| **MSBuildTask0002** | Warning | MT tasks by default; all tasks in migration mode | API requires `TaskEnvironment` alternative | +| **MSBuildTask0003** | Warning | MT tasks by default; all tasks in migration mode | 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 | +| **MSBuildTask0005** | Warning | MT tasks by default; all tasks in migration mode | Transitive unsafe API usage in task call chain | | **MSBuildTask0006** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer typed path parameter over string | | **MSBuildTask0007** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer `ITaskItem` over manual ItemSpec parsing | | **MSBuildTask0008** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Initialize a relative-default path property in `Execute()` | @@ -289,16 +289,25 @@ The engine prefers this constructor when it is present. A public parameterless c ## Analysis Scope -The analyzer determines what to check based on the type declaration: +The default `multithreadable_only` scope prevents MT-specific warnings from affecting regular tasks. It recognizes `IMultiThreadableTask`, `[MSBuildMultiThreadableTask]`, and `[MSBuildMultiThreadableTaskAnalyzed]` as MT opt-ins. | Type | Rules Applied | |---|---| -| Any class implementing `ITask` | MSBuildTask0001–MSBuildTask0005, MSBuildTask0009–MSBuildTask0010 | +| Regular class implementing `ITask` | MSBuildTask0001, MSBuildTask0004, MSBuildTask0009–MSBuildTask0010 | | Class with `[MSBuildMultiThreadableTask]` attribute applied directly | MSBuildTask0006–MSBuildTask0008 (in addition to MSBuildTask0001–0005) | | Concrete class implementing `IMultiThreadableTask` without the attribute | MSBuildTask0001–MSBuildTask0005 and MSBuildTask0009–MSBuildTask0011 | | Helper class with `[MSBuildMultiThreadableTaskAnalyzed]` attribute | MSBuildTask0001–MSBuildTask0005 | | Regular class (no task interface or attribute) | Not analyzed | +Set the scope to `all` to analyze regular tasks for MSBuildTask0002, MSBuildTask0003, and MSBuildTask0005 before MT migration: + +```ini +[*.cs] +msbuild_task_analyzer.scope = all +``` + +Missing and unrecognized values use the safe `multithreadable_only` default. + MSBuildTask0006–MSBuildTask0008 apply only when the `[MSBuildMultiThreadableTask]` attribute is applied **directly** to the task class. The attribute is `Inherited = false`, so a task that merely derives from a base class implementing `IMultiThreadableTask` (or carrying the attribute) has not itself opted into multithreaded support and is not subject to these three rules. Input properties are collected from the task class **and its base classes**, so an `ITaskItem`/`string` input declared on a shared base task is still analyzed. The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classes into **direct** analysis by the `MultiThreadableTaskAnalyzer` (MSBuildTask0001–0004). Without it, only classes implementing `ITask` receive per-line diagnostics and code fixes for those rules. The **transitive** analyzer (MSBuildTask0005) already discovers helpers via call graph analysis, but it reports only at the task entry point. Adding this attribute to a helper class gives you inline diagnostics and code fixes directly in the helper's source. @@ -309,7 +318,7 @@ The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classe - **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–MSBuildTask0005 and MSBuildTask0009** report as **Warning** for all task types. +- **MSBuildTask0002–MSBuildTask0005 and MSBuildTask0009** report as **Warning** when their scope applies. - **MSBuildTask0006–MSBuildTask0008 and MSBuildTask0011** report as **Info** — these are modernization suggestions, not correctness issues. ## Code Fixes diff --git a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs index 3c8f9643cb1..61f6041d260 100644 --- a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs +++ b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs @@ -18,7 +18,7 @@ internal static class SharedAnalyzerHelpers { /// /// The .editorconfig key controlling analysis scope. - /// Values: "all" (default) | "multithreadable_only" + /// Values: "multithreadable_only" (default) | "all" /// internal const string ScopeOptionKey = "msbuild_task_analyzer.scope"; internal const string ScopeAll = "all"; @@ -26,17 +26,17 @@ internal static class SharedAnalyzerHelpers /// /// Reads the scope option from the analyzer config options provider. - /// Returns true if all tasks should be analyzed; false if only multithreadable tasks. + /// Returns true only when all-task migration analysis is explicitly enabled. /// internal static bool ReadAnalyzeAllTasksOption(AnalyzerConfigOptionsProvider optionsProvider) { if (optionsProvider.GlobalOptions.TryGetValue($"build_property.{ScopeOptionKey}", out var scopeValue) || optionsProvider.GlobalOptions.TryGetValue(ScopeOptionKey, out scopeValue)) { - return !string.Equals(scopeValue, ScopeMultiThreadableOnly, StringComparison.OrdinalIgnoreCase); + return string.Equals(scopeValue, ScopeAll, StringComparison.OrdinalIgnoreCase); } - return true; // default: analyze all tasks + return false; } /// /// Represents a resolved banned API entry for O(1) lookup during analysis. From 6880d18c4e6f413d74ff563edebc0c614c79b58d Mon Sep 17 00:00:00 2001 From: VolPlita Date: Fri, 21 Aug 2026 13:52:13 +0200 Subject: [PATCH 2/2] remove 0005 --- ...MultiThreadableTaskCodeFixProviderTests.cs | 1 - src/TaskAnalyzer.Tests/TestHelpers.cs | 20 ++ .../TransitiveCallChainAnalyzerTests.cs | 211 +++++++++++++++--- .../AnalyzerReleases.Unshipped.md | 1 - src/TaskAnalyzer/DiagnosticDescriptors.cs | 41 +++- src/TaskAnalyzer/DiagnosticIds.cs | 5 +- .../MultiThreadableTaskCodeFixProvider.cs | 5 + src/TaskAnalyzer/README.md | 19 +- .../TransitiveCallChainAnalyzer.cs | 119 +++++++--- src/Tasks/GenerateLauncher.cs | 4 +- src/Tasks/Microsoft.Build.Tasks.csproj | 2 +- 11 files changed, 341 insertions(+), 87 deletions(-) diff --git a/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs b/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs index ff43df8d145..8110d26cbcf 100644 --- a/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs +++ b/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs @@ -42,7 +42,6 @@ private static CSharpCodeFixTest new DiagnosticResult(DiagnosticDescriptors.TaskEnvironmentRequired), DiagnosticIds.FilePathRequiresAbsolute => new DiagnosticResult(DiagnosticDescriptors.FilePathRequiresAbsolute), DiagnosticIds.PotentialIssue => new DiagnosticResult(DiagnosticDescriptors.PotentialIssue), - DiagnosticIds.TransitiveUnsafeCall => new DiagnosticResult(DiagnosticDescriptors.TransitiveUnsafeCall), _ => new DiagnosticResult(id, DiagnosticSeverity.Warning), }; diff --git a/src/TaskAnalyzer.Tests/TestHelpers.cs b/src/TaskAnalyzer.Tests/TestHelpers.cs index 34735c70734..6a5f52cec25 100644 --- a/src/TaskAnalyzer.Tests/TestHelpers.cs +++ b/src/TaskAnalyzer.Tests/TestHelpers.cs @@ -288,6 +288,26 @@ public static async System.Threading.Tasks.Task> GetA return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); } + /// + /// Runs both direct and transitive analyzers with a configured action for one diagnostic ID. + /// + public static async System.Threading.Tasks.Task> GetAllDiagnosticsWithDiagnosticActionAsync( + string source, + string diagnosticId, + ReportDiagnostic action) + { + var compilation = CreateCompilation(source); + var options = ((CSharpCompilationOptions)compilation.Options).WithSpecificDiagnosticOptions( + compilation.Options.SpecificDiagnosticOptions.SetItem(diagnosticId, action)); + compilation = compilation.WithOptions(options); + + var analyzers = ImmutableArray.Create( + new MultiThreadableTaskAnalyzer(), + new TransitiveCallChainAnalyzer()); + var compilationWithAnalyzers = compilation.WithAnalyzers(analyzers); + return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); + } + private static MetadataReference[] CreateCoreReferences() { // Reference the core runtime assemblies needed diff --git a/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs b/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs index 23ad3b18b14..f97108d835d 100644 --- a/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs +++ b/src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs @@ -1,12 +1,17 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Text; using Shouldly; using Xunit; using static Microsoft.Build.TaskAuthoring.Analyzer.Tests.TestHelpers; @@ -20,11 +25,16 @@ namespace Microsoft.Build.TaskAuthoring.Analyzer.Tests; public class TransitiveCallChainAnalyzerTests { [Theory] - [InlineData("using System;", "Console.WriteLine(\"test\");", "Console.WriteLine")] - [InlineData("using System.IO;", "File.Exists(\"test.txt\");", "File.Exists")] - [InlineData("using System;", "Environment.GetEnvironmentVariable(\"KEY\");", "GetEnvironmentVariable")] - public async Task HelperCallingBannedApi_TransitivelyFromTask_ProducesDiagnostic( - string usingDirective, string helperBody, string expectedApiName) + [InlineData("using System;", "Console.WriteLine(\"test\");", "Console.WriteLine", DiagnosticIds.CriticalError, DiagnosticSeverity.Error)] + [InlineData("using System.IO;", "File.Exists(\"test.txt\");", "File.Exists", DiagnosticIds.FilePathRequiresAbsolute, DiagnosticSeverity.Warning)] + [InlineData("using System;", "Environment.GetEnvironmentVariable(\"KEY\");", "GetEnvironmentVariable", DiagnosticIds.TaskEnvironmentRequired, DiagnosticSeverity.Warning)] + [InlineData("using System.Reflection;", "Assembly.Load(\"Test\");", "Assembly.Load", DiagnosticIds.PotentialIssue, DiagnosticSeverity.Warning)] + public async Task HelperCallingBannedApi_ReportsUnderlyingDiagnostic( + string usingDirective, + string helperBody, + string expectedApiName, + string expectedDiagnosticId, + DiagnosticSeverity expectedSeverity) { var source = $$""" {{usingDirective}} @@ -45,9 +55,17 @@ public override bool Execute() var diags = await GetAllDiagnosticsAsync(source); - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ToArray(); - transitive.ShouldNotBeEmpty(); - transitive[0].GetMessage().ShouldContain(expectedApiName); + var transitive = diags.Where(d => + d.Id == expectedDiagnosticId && + d.GetMessage().Contains("reachable from task method")).ShouldHaveSingleItem(); + + transitive.Severity.ShouldBe(expectedSeverity); + transitive.GetMessage().ShouldContain(expectedApiName); + transitive.GetMessage().ShouldContain("MyTask.Execute"); + transitive.Properties[DiagnosticIds.IsTransitiveProperty].ShouldBe(bool.TrueString); + transitive.Location.SourceTree!.GetText().ToString(transitive.Location.SourceSpan).ShouldContain(expectedApiName); + transitive.AdditionalLocations.ShouldHaveSingleItem() + .SourceTree!.GetText().ToString(transitive.AdditionalLocations[0].SourceSpan).ShouldBe("Execute"); } [Fact] @@ -74,9 +92,10 @@ public override bool Execute() } """); - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ToArray(); - transitive.ShouldNotBeEmpty(); - var msg = transitive[0].GetMessage(); + var transitive = diags.Where(d => + d.Id == DiagnosticIds.CriticalError && + d.GetMessage().Contains("reachable from task method")).ShouldHaveSingleItem(); + var msg = transitive.GetMessage(); msg.ShouldContain("Environment.Exit"); // Chain should show: MyTask.Execute → OuterHelper.Process → InnerHelper.DoExit → Environment.Exit msg.ShouldContain("OuterHelper.Process"); @@ -99,11 +118,8 @@ public override bool Execute() } """); - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall); - transitive.ShouldBeEmpty(); - - var direct = diags.Where(d => d.Id == DiagnosticIds.CriticalError); - direct.ShouldNotBeEmpty(); + var direct = diags.Where(d => d.Id == DiagnosticIds.CriticalError).ShouldHaveSingleItem(); + direct.GetMessage().ShouldNotContain("reachable from task method"); } [Fact] @@ -125,8 +141,11 @@ public override bool Execute() } """); - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall); - transitive.ShouldBeEmpty(); + diags.Where(d => d.Id is + DiagnosticIds.CriticalError or + DiagnosticIds.TaskEnvironmentRequired or + DiagnosticIds.FilePathRequiresAbsolute or + DiagnosticIds.PotentialIssue).ShouldBeEmpty(); } [Fact] @@ -151,9 +170,10 @@ public override bool Execute() """); // Should still detect the violation without infinite loop - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ToArray(); - transitive.ShouldNotBeEmpty(); - transitive[0].GetMessage().ShouldContain("Console.WriteLine"); + var transitive = diags.Where(d => + d.Id == DiagnosticIds.CriticalError && + d.GetMessage().Contains("reachable from task method")).ShouldHaveSingleItem(); + transitive.GetMessage().ShouldContain("Console.WriteLine"); } [Fact] @@ -177,9 +197,10 @@ public override bool Execute() } """); - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ToArray(); - transitive.ShouldNotBeEmpty(); - transitive[0].GetMessage().ShouldContain("Console.Write"); + var transitive = diags.Where(d => + d.Id == DiagnosticIds.CriticalError && + d.GetMessage().Contains("reachable from task method")).ShouldHaveSingleItem(); + transitive.GetMessage().ShouldContain("Console.Write"); } [Fact] @@ -208,8 +229,10 @@ public override bool Execute() } """); - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ToArray(); - transitive.Length.ShouldBeGreaterThanOrEqualTo(3); + var transitive = diags.Where(d => d.GetMessage().Contains("reachable from task method")).ToArray(); + transitive.Length.ShouldBe(3); + transitive.Count(d => d.Id == DiagnosticIds.CriticalError).ShouldBe(2); + transitive.Count(d => d.Id == DiagnosticIds.FilePathRequiresAbsolute).ShouldBe(1); } [Fact] @@ -236,9 +259,10 @@ public override bool Execute() } """); - var transitive = diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ToArray(); - transitive.ShouldNotBeEmpty(); - var msg = transitive[0].GetMessage(); + var transitive = diags.Where(d => + d.Id == DiagnosticIds.CriticalError && + d.GetMessage().Contains("reachable from task method")).ShouldHaveSingleItem(); + var msg = transitive.GetMessage(); // Should contain arrow-separated chain msg.ShouldContain("→"); msg.ShouldContain("A.Step1"); @@ -264,7 +288,7 @@ public override bool Execute() } """); - diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ShouldBeEmpty(); + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldBeEmpty(); } [Fact] @@ -288,7 +312,8 @@ public override bool Execute() } """); - diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ShouldHaveSingleItem(); + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldHaveSingleItem() + .GetMessage().ShouldContain("reachable from task method"); } [Fact] @@ -310,6 +335,128 @@ public override bool Execute() } """, SharedAnalyzerHelpers.ScopeAll); - diags.Where(d => d.Id == DiagnosticIds.TransitiveUnsafeCall).ShouldHaveSingleItem(); + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldHaveSingleItem() + .GetMessage().ShouldContain("reachable from task method"); + } + + [Theory] + [InlineData("Console.WriteLine(\"test\");", DiagnosticIds.CriticalError, DiagnosticSeverity.Error)] + [InlineData("System.Reflection.Assembly.Load(\"Test\");", DiagnosticIds.PotentialIssue, DiagnosticSeverity.Warning)] + public async Task Scope_Default_PlainTask_GetsRulesThatApplyToAllTasks( + string helperBody, + string expectedDiagnosticId, + DiagnosticSeverity expectedSeverity) + { + var diags = await GetAllDiagnosticsWithDefaultScopeAsync($$""" + using System; + public static class Helper + { + public static void Run() { {{helperBody}} } + } + public class PlainTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + Helper.Run(); + return true; + } + } + """); + + var diagnostic = diags.Where(d => + d.Id == expectedDiagnosticId && + d.GetMessage().Contains("reachable from task method")).ShouldHaveSingleItem(); + diagnostic.Severity.ShouldBe(expectedSeverity); + } + + [Fact] + public async Task UnderlyingDiagnosticConfiguration_SuppressesTransitiveDiagnostic() + { + var diags = await GetAllDiagnosticsWithDiagnosticActionAsync(""" + using System; + using Microsoft.Build.Framework; + public static class Helper + { + public static void Run() => Environment.GetEnvironmentVariable("KEY"); + } + public class MtTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + Helper.Run(); + return true; + } + } + """, DiagnosticIds.TaskEnvironmentRequired, ReportDiagnostic.Suppress); + + diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldBeEmpty(); + } + + [Fact] + public async Task DirectlyAnalyzedHelper_DoesNotGetDuplicateTransitiveDiagnostic() + { + var diags = await GetAllDiagnosticsWithDefaultScopeAsync(""" + using System; + using Microsoft.Build.Framework; + [MSBuildMultiThreadableTaskAnalyzed] + public static class Helper + { + public static void Run() => Environment.GetEnvironmentVariable("KEY"); + } + public class MtTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + Helper.Run(); + return true; + } + } + """); + + var diagnostic = diags.Where(d => d.Id == DiagnosticIds.TaskEnvironmentRequired).ShouldHaveSingleItem(); + diagnostic.Properties.ContainsKey(DiagnosticIds.IsTransitiveProperty).ShouldBeFalse(); + } + + [Fact] + public async Task TransitiveDiagnostic_DoesNotOfferCodeFix() + { + const string source = """ + using System; + using Microsoft.Build.Framework; + public static class Helper + { + public static void Run() => Environment.GetEnvironmentVariable("KEY"); + } + public class MtTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + Helper.Run(); + return true; + } + } + """; + + var diagnostics = await GetAllDiagnosticsWithDefaultScopeAsync(source); + var diagnostic = diagnostics.Where(d => + d.Id == DiagnosticIds.TaskEnvironmentRequired && + d.Properties.ContainsKey(DiagnosticIds.IsTransitiveProperty)).ShouldHaveSingleItem(); + + using var workspace = new AdhocWorkspace(); + var project = workspace.AddProject("TestProject", LanguageNames.CSharp); + var document = workspace.AddDocument(project.Id, "Test.cs", SourceText.From(source)); + var actions = new List(); + var context = new CodeFixContext( + document, + diagnostic, + (action, _) => actions.Add(action), + CancellationToken.None); + + await new MultiThreadableTaskCodeFixProvider().RegisterCodeFixesAsync(context); + + actions.ShouldBeEmpty(); } } \ No newline at end of file diff --git a/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md b/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md index e501f7ddc3d..8a27011e6e2 100644 --- a/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md +++ b/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md @@ -6,7 +6,6 @@ MSBuildTask0001 | MSBuild.TaskAuthoring | Error | APIs that must not be used in MSBuildTask0002 | MSBuild.TaskAuthoring | Warning | APIs that should use TaskEnvironment alternatives 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 | Info | Prefer typed path parameter (AbsolutePath/FileInfo/DirectoryInfo) over string (code fix available) MSBuildTask0007 | MSBuild.TaskAuthoring | Info | Prefer ITaskItem 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) diff --git a/src/TaskAnalyzer/DiagnosticDescriptors.cs b/src/TaskAnalyzer/DiagnosticDescriptors.cs index 217db3e3c81..dd262081a2e 100644 --- a/src/TaskAnalyzer/DiagnosticDescriptors.cs +++ b/src/TaskAnalyzer/DiagnosticDescriptors.cs @@ -48,14 +48,44 @@ internal static class DiagnosticDescriptors isEnabledByDefault: true, description: "This API may cause threading issues or version conflicts. Review usage carefully."); - public static readonly DiagnosticDescriptor TransitiveUnsafeCall = new( - id: DiagnosticIds.TransitiveUnsafeCall, - title: "Transitive unsafe API usage in task call chain", - messageFormat: "'{0}' transitively calls unsafe API '{1}' via: {2}", + public static readonly DiagnosticDescriptor TransitiveCriticalError = new( + id: DiagnosticIds.CriticalError, + title: "API is never safe in MSBuild task implementations", + messageFormat: "'{0}' must not be used in MSBuild tasks: {1}. It is reachable from task method '{2}' via: {3}.", + category: "MSBuild.TaskAuthoring", + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: "This API has no safe alternative in MSBuild tasks. It is called by a helper reachable from an MSBuild task.", + customTags: WellKnownDiagnosticTags.CompilationEnd); + + public static readonly DiagnosticDescriptor TransitiveTaskEnvironmentRequired = new( + id: DiagnosticIds.TaskEnvironmentRequired, + title: "API requires TaskEnvironment alternative in MSBuild tasks", + messageFormat: "'{0}' should use TaskEnvironment alternative: {1}. It is reachable from task method '{2}' via: {3}.", + category: "MSBuild.TaskAuthoring", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "This API accesses process-global state. It is called by a helper reachable from an MSBuild task.", + customTags: WellKnownDiagnosticTags.CompilationEnd); + + public static readonly DiagnosticDescriptor TransitiveFilePathRequiresAbsolute = new( + id: DiagnosticIds.FilePathRequiresAbsolute, + title: "File system API requires absolute path in MSBuild tasks", + messageFormat: "'{0}' may resolve a relative path against the shared working directory: {1}. It is reachable from task method '{2}' via: {3}.", + category: "MSBuild.TaskAuthoring", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "This file system API receives a path that may be relative. It is called by a helper reachable from an MSBuild task.", + customTags: WellKnownDiagnosticTags.CompilationEnd); + + public static readonly DiagnosticDescriptor TransitivePotentialIssue = new( + id: DiagnosticIds.PotentialIssue, + title: "API may cause issues in multithreaded MSBuild tasks", + messageFormat: "'{0}' may cause issues in multithreaded tasks: {1}. It is reachable from task method '{2}' via: {3}.", category: "MSBuild.TaskAuthoring", defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "A method called from this task transitively uses an API that is unsafe in multithreaded task execution. Review the call chain and migrate the callee.", + description: "This API may cause threading issues or version conflicts. It is called by a helper reachable from an MSBuild task.", customTags: WellKnownDiagnosticTags.CompilationEnd); public static readonly DiagnosticDescriptor PreferTypedPathParameter = new( @@ -117,7 +147,6 @@ internal static class DiagnosticDescriptors TaskEnvironmentRequired, FilePathRequiresAbsolute, PotentialIssue, - TransitiveUnsafeCall, PreferTypedPathParameter, PreferTypedTaskItem, InitializeRelativeDefaultInExecute, diff --git a/src/TaskAnalyzer/DiagnosticIds.cs b/src/TaskAnalyzer/DiagnosticIds.cs index d846f1c652b..b8440a68b81 100644 --- a/src/TaskAnalyzer/DiagnosticIds.cs +++ b/src/TaskAnalyzer/DiagnosticIds.cs @@ -9,6 +9,8 @@ namespace Microsoft.Build.TaskAuthoring.Analyzer /// public static class DiagnosticIds { + internal const string IsTransitiveProperty = "MSBuildTask.IsTransitive"; + /// Critical APIs with no safe alternative (Environment.Exit, Console.*, ThreadPool). public const string CriticalError = "MSBuildTask0001"; @@ -21,9 +23,6 @@ public static class DiagnosticIds /// Potentially problematic APIs (Assembly.Load*, Activator.CreateInstance*). public const string PotentialIssue = "MSBuildTask0004"; - /// Transitive unsafe API usage detected in task call chain. - public const string TransitiveUnsafeCall = "MSBuildTask0005"; - /// Task input property should use AbsolutePath, FileInfo, or DirectoryInfo instead of string. public const string PreferTypedPathParameter = "MSBuildTask0006"; diff --git a/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs b/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs index 240c3b62d82..97443e45fee 100644 --- a/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs +++ b/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs @@ -40,6 +40,11 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) foreach (var diagnostic in context.Diagnostics) { + if (diagnostic.Properties.ContainsKey(DiagnosticIds.IsTransitiveProperty)) + { + continue; + } + var node = root.FindNode(diagnostic.Location.SourceSpan); if (diagnostic.Id == DiagnosticIds.FilePathRequiresAbsolute) diff --git a/src/TaskAnalyzer/README.md b/src/TaskAnalyzer/README.md index fb99b05f247..96bd538b485 100644 --- a/src/TaskAnalyzer/README.md +++ b/src/TaskAnalyzer/README.md @@ -18,7 +18,6 @@ This analyzer catches unsafe API usage at compile time and offers code fixes to | **MSBuildTask0002** | Warning | MT tasks by default; all tasks in migration mode | API requires `TaskEnvironment` alternative | | **MSBuildTask0003** | Warning | MT tasks by default; all tasks in migration mode | File system API requires absolute path | | **MSBuildTask0004** | Warning | All `ITask` implementations | API may cause issues in multithreaded tasks | -| **MSBuildTask0005** | Warning | MT tasks by default; all tasks in migration mode | Transitive unsafe API usage in task call chain | | **MSBuildTask0006** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer typed path parameter over string | | **MSBuildTask0007** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Prefer `ITaskItem` over manual ItemSpec parsing | | **MSBuildTask0008** | Info | Tasks with `[MSBuildMultiThreadableTask]` applied directly | Initialize a relative-default path property in `Execute()` | @@ -26,6 +25,8 @@ This analyzer catches unsafe API usage at compile time and offers code fixes to | **MSBuildTask0010** | Error | All `ITask` implementations | `ITaskItem` relies on culture-sensitive conversion | | **MSBuildTask0011** | Info | Concrete `IMultiThreadableTask` implementations | Prefer constructor injection for `TaskEnvironment` | +Unsafe API calls in helper methods use the corresponding MSBuildTask0001–MSBuildTask0004 diagnostic rather than a separate rule. The diagnostic is reported at the unsafe call and includes the task method and call chain that make the helper reachable. + ### MSBuildTask0001 — Critical: No Safe Alternative These APIs affect the entire process or interfere with build infrastructure. They are **errors** and should never appear in any MSBuild task. @@ -294,12 +295,12 @@ The default `multithreadable_only` scope prevents MT-specific warnings from affe | Type | Rules Applied | |---|---| | Regular class implementing `ITask` | MSBuildTask0001, MSBuildTask0004, MSBuildTask0009–MSBuildTask0010 | -| Class with `[MSBuildMultiThreadableTask]` attribute applied directly | MSBuildTask0006–MSBuildTask0008 (in addition to MSBuildTask0001–0005) | -| Concrete class implementing `IMultiThreadableTask` without the attribute | MSBuildTask0001–MSBuildTask0005 and MSBuildTask0009–MSBuildTask0011 | -| Helper class with `[MSBuildMultiThreadableTaskAnalyzed]` attribute | MSBuildTask0001–MSBuildTask0005 | +| Class with `[MSBuildMultiThreadableTask]` attribute applied directly | MSBuildTask0006–MSBuildTask0008 (in addition to MSBuildTask0001–MSBuildTask0004) | +| Concrete class implementing `IMultiThreadableTask` without the attribute | MSBuildTask0001–MSBuildTask0004 and MSBuildTask0009–MSBuildTask0011 | +| Helper class with `[MSBuildMultiThreadableTaskAnalyzed]` attribute | Direct MSBuildTask0001–MSBuildTask0004 analysis | | Regular class (no task interface or attribute) | Not analyzed | -Set the scope to `all` to analyze regular tasks for MSBuildTask0002, MSBuildTask0003, and MSBuildTask0005 before MT migration: +Set the scope to `all` to analyze regular tasks for direct and transitive MSBuildTask0002 and MSBuildTask0003 violations before MT migration: ```ini [*.cs] @@ -310,7 +311,7 @@ Missing and unrecognized values use the safe `multithreadable_only` default. MSBuildTask0006–MSBuildTask0008 apply only when the `[MSBuildMultiThreadableTask]` attribute is applied **directly** to the task class. The attribute is `Inherited = false`, so a task that merely derives from a base class implementing `IMultiThreadableTask` (or carrying the attribute) has not itself opted into multithreaded support and is not subject to these three rules. Input properties are collected from the task class **and its base classes**, so an `ITaskItem`/`string` input declared on a shared base task is still analyzed. -The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classes into **direct** analysis by the `MultiThreadableTaskAnalyzer` (MSBuildTask0001–0004). Without it, only classes implementing `ITask` receive per-line diagnostics and code fixes for those rules. The **transitive** analyzer (MSBuildTask0005) already discovers helpers via call graph analysis, but it reports only at the task entry point. Adding this attribute to a helper class gives you inline diagnostics and code fixes directly in the helper's source. +The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classes into **direct** analysis by the `MultiThreadableTaskAnalyzer` (MSBuildTask0001–0004). Without it, helpers receive a diagnostic only when call graph analysis finds that they are reachable from a task. A transitive diagnostic is reported at the unsafe call and includes the task method and call chain for context. Adding the attribute enables direct analysis and applicable code fixes without requiring task reachability; the transitive analyzer then skips that helper to avoid duplicate diagnostics. **When to use:** Apply `[MSBuildMultiThreadableTaskAnalyzed]` to utility or helper classes that are primarily used by multithreadable tasks and where you want immediate in-editor feedback (squiggles and code fixes) on unsafe APIs within those helpers. @@ -318,7 +319,7 @@ The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classe - **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–MSBuildTask0005 and MSBuildTask0009** report as **Warning** when their scope applies. +- **MSBuildTask0002–MSBuildTask0004 and MSBuildTask0009** report as **Warning** when their scope applies. - **MSBuildTask0006–MSBuildTask0008 and MSBuildTask0011** report as **Info** — these are modernization suggestions, not correctness issues. ## Code Fixes @@ -345,6 +346,8 @@ The analyzer ships with a code fix provider that offers automatic replacements: The MSBuildTask0003 fixer intelligently finds the first **unwrapped** path argument rather than blindly wrapping the first argument — so for `File.Copy(safePath, unsafePath)` it correctly wraps the second argument. +Transitive diagnostics do not offer code fixes because a helper method does not necessarily have access to the task's `TaskEnvironment`. Apply the migration in the helper's API and pass the required task-specific state from its caller. + The MSBuildTask0006/MSBuildTask0007 fixer is conservative by design: it only offers a fix when every reference to the property — across all partial declarations of the task type, in the current document — can be safely rewritten as part of the same change, so the resulting code keeps compiling after the property type is updated. If the property is referenced from another file (a partial class spread across documents) in a way this single-document fix can't rewrite, no fix is offered. When the property has a meaningful default (a non-empty string literal), the fixer only preserves it when the literal is **already fully qualified** (e.g. `= "C:/logs"`), re-expressing it through the `AbsolutePath` constructor: `= new AbsolutePath("C:/logs")` for an `AbsolutePath` property, and `= new FileInfo(new AbsolutePath("C:/logs"))` / `= new DirectoryInfo(new AbsolutePath("C:/logs"))` for the `FileInfo`/`DirectoryInfo` cases (mirroring the engine's string → `AbsolutePath` → `FileInfo`/`DirectoryInfo` chain). A **relative** default (such as `= "obj"`) is deliberately *not* rewritten: MSBuild roots a relative path via `TaskEnvironment.GetAbsolutePath`, but `TaskEnvironment` is only set by the engine *after* the task is constructed, so it isn't available inside a property initializer (which runs in the constructor). Emitting `new AbsolutePath("obj")` there would throw at construction time, so the fix is skipped instead. The fix is likewise skipped when the default isn't a compile-time constant or isn't a plausible path value, rather than dropping or corrupting the default. @@ -454,7 +457,7 @@ dotnet test | `BannedApiDefinitions.cs` | ~50 banned API entries resolved via `DocumentationCommentId` for O(1) symbol lookup | | `SharedAnalyzerHelpers.cs` | Shared path safety analysis, banned API resolution, and interface checking helpers | | `DiagnosticDescriptors.cs` | Eight diagnostic descriptors in category `MSBuild.TaskAuthoring` | -| `DiagnosticIds.cs` | Public constants: `MSBuildTask0001`–`MSBuildTask0008` | +| `DiagnosticIds.cs` | Public constants for active MSBuildTask diagnostics (`MSBuildTask0005` is reserved) | | `PreferTypedParameterAnalyzer.cs` | Analyzer for MSBuildTask0006, MSBuildTask0007, and MSBuildTask0008 — detects manual path construction, ItemSpec parsing, Path.Combine usage (first argument only), helper method wrapping, FileInfo/DirectoryInfo construction through AbsolutePath intermediaries, System.IO consumption sites (`File.*`/`Directory.*`/`FileStream`/`StreamReader`/`StreamWriter`) that bias suggestions toward `FileInfo`/`DirectoryInfo`, and relative default paths that must be initialized in `Execute()` | | `PathDefaultClassifier.cs` | Shared classification of string path defaults as fully-qualified vs relative (host-independent, netstandard2.0-safe) | diff --git a/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs b/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs index 9dc9d26aa99..ba4a877e034 100644 --- a/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs +++ b/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs @@ -21,11 +21,15 @@ namespace Microsoft.Build.TaskAuthoring.Analyzer /// a task class, this analyzer builds a compilation-wide call graph and traces method calls /// transitively to find unsafe APIs called by helper methods, utility classes, etc. /// - /// Reports MSBuildTask0005 with the full call chain for traceability. + /// Reports the underlying MSBuildTask0001-MSBuildTask0004 diagnostic at the unsafe call + /// with the full call chain for traceability. /// [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class TransitiveCallChainAnalyzer : DiagnosticAnalyzer { + private static readonly ImmutableDictionary s_transitiveProperties = + ImmutableDictionary.Empty.Add(DiagnosticIds.IsTransitiveProperty, bool.TrueString); + /// /// Maximum BFS depth. The visited set already prevents cycles, but this limits /// exploration of very deep non-cyclic call chains for performance. @@ -33,7 +37,11 @@ public sealed class TransitiveCallChainAnalyzer : DiagnosticAnalyzer private const int MaxCallChainDepth = 20; public override ImmutableArray SupportedDiagnostics => - ImmutableArray.Create(DiagnosticDescriptors.TransitiveUnsafeCall); + ImmutableArray.Create( + DiagnosticDescriptors.TransitiveCriticalError, + DiagnosticDescriptors.TransitiveTaskEnvironmentRequired, + DiagnosticDescriptors.TransitiveFilePathRequiresAbsolute, + DiagnosticDescriptors.TransitivePotentialIssue); public override void Initialize(AnalysisContext context) { @@ -73,7 +81,7 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte compilationContext.RegisterOperationAction(opCtx => { ScanOperation(opCtx, callGraph, directViolations, bannedApiLookup, filePathTypes, - taskEnvironmentType, absolutePathType, iTaskItemType, consoleType, iTaskType); + taskEnvironmentType, absolutePathType, iTaskItemType, consoleType, iTaskType, analyzedAttributeType); }, OperationKind.Invocation, OperationKind.ObjectCreation, @@ -102,7 +110,8 @@ private static void ScanOperation( INamedTypeSymbol? absolutePathType, INamedTypeSymbol? iTaskItemType, INamedTypeSymbol? consoleType, - INamedTypeSymbol iTaskType) + INamedTypeSymbol iTaskType, + INamedTypeSymbol? analyzedAttributeType) { var containingSymbol = context.ContainingSymbol; if (containingSymbol is not IMethodSymbol containingMethod) @@ -116,6 +125,9 @@ private static void ScanOperation( // Check if this method is inside a task type var containingType = containingMethod.ContainingType; bool isInsideTask = containingType is not null && ImplementsInterface(containingType, iTaskType); + bool isDirectlyAnalyzedHelper = containingType is not null && + analyzedAttributeType is not null && + containingType.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, analyzedAttributeType)); ISymbol? referencedSymbol = null; ImmutableArray arguments = default; @@ -168,7 +180,7 @@ private static void ScanOperation( // Only record violations for NON-task methods // Task methods get direct analysis from MultiThreadableTaskAnalyzer - if (isInsideTask) + if (isInsideTask || isDirectlyAnalyzedHelper) { return; } @@ -177,7 +189,12 @@ private static void ScanOperation( if (bannedApiLookup.TryGetValue(referencedSymbol, out var entry)) { var displayName = referencedSymbol.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat); - var violation = new ViolationInfo(entry.Category.ToString(), displayName, entry.Message); + var violation = new ViolationInfo( + GetTransitiveDescriptor(entry.Category), + entry.Category == BannedApiDefinitions.ApiCategory.TaskEnvironment, + context.Operation.Syntax.GetLocation(), + displayName, + entry.Message); directViolations.GetOrAdd(callerKey, _ => new ConcurrentBag()).Add(violation); return; } @@ -192,7 +209,12 @@ private static void ScanOperation( string message = referencedSymbol.Name.StartsWith("Read", StringComparison.Ordinal) ? "may cause deadlocks in automated builds" : "interferes with build logging; use Log.LogMessage instead"; - var violation = new ViolationInfo("CriticalError", displayName, message); + var violation = new ViolationInfo( + DiagnosticDescriptors.TransitiveCriticalError, + requiresMultiThreadableScope: false, + context.Operation.Syntax.GetLocation(), + displayName, + message); directViolations.GetOrAdd(callerKey, _ => new ConcurrentBag()).Add(violation); return; } @@ -207,7 +229,11 @@ private static void ScanOperation( if (HasUnwrappedPathArgument(arguments, taskEnvironmentType, absolutePathType, iTaskItemType)) { var displayName = referencedSymbol.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat); - var violation = new ViolationInfo("FilePathRequiresAbsolute", displayName, + var violation = new ViolationInfo( + DiagnosticDescriptors.TransitiveFilePathRequiresAbsolute, + requiresMultiThreadableScope: true, + context.Operation.Syntax.GetLocation(), + displayName, "may resolve relative paths against the process working directory"); directViolations.GetOrAdd(callerKey, _ => new ConcurrentBag()).Add(violation); } @@ -243,22 +269,13 @@ private static void AnalyzeTransitiveViolations( return; } - // When scope is "multithreadable_only", filter to only multithreadable tasks - if (!analyzeAllTasks) - { - taskTypes = taskTypes.Where(t => - (iMultiThreadableTaskType is not null && t.AllInterfaces.Any(i => SymbolEqualityComparer.Default.Equals(i, iMultiThreadableTaskType))) || - (multiThreadableTaskAttributeType is not null && t.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, multiThreadableTaskAttributeType))) || - (analyzedAttributeType is not null && t.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, analyzedAttributeType)))).ToList(); - - if (taskTypes.Count == 0) - { - return; - } - } - foreach (var taskType in taskTypes) { + bool reportEnvironmentRules = analyzeAllTasks || + (iMultiThreadableTaskType is not null && taskType.AllInterfaces.Any(i => SymbolEqualityComparer.Default.Equals(i, iMultiThreadableTaskType))) || + (multiThreadableTaskAttributeType is not null && taskType.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, multiThreadableTaskAttributeType))) || + (analyzedAttributeType is not null && taskType.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, analyzedAttributeType))); + // Track reported violations per task type to avoid flooding with duplicates. // Key: target banned API display name. We report only the shortest chain per API. var reportedPerTaskType = new HashSet(StringComparer.Ordinal); @@ -302,7 +319,7 @@ private static void AnalyzeTransitiveViolations( { foreach (var v in violations) { - ReportTransitiveViolation(context, method, v, chain, reportedPerTaskType); + ReportTransitiveViolation(context, method, v, chain, reportedPerTaskType, reportEnvironmentRules); } } @@ -340,8 +357,14 @@ private static void ReportTransitiveViolation( IMethodSymbol taskMethod, ViolationInfo violation, List chain, - HashSet reportedPerTaskType) + HashSet reportedPerTaskType, + bool reportEnvironmentRules) { + if (violation.RequiresMultiThreadableScope && !reportEnvironmentRules) + { + return; + } + // Deduplicate by target API — report each banned API only once per task type if (!reportedPerTaskType.Add(violation.ApiDisplayName)) { @@ -351,13 +374,34 @@ private static void ReportTransitiveViolation( var chainWithApi = new List(chain) { violation.ApiDisplayName }; var chainStr = string.Join(" → ", chainWithApi); - var location = taskMethod.Locations.Length > 0 ? taskMethod.Locations[0] : Location.None; + var taskLocation = taskMethod.Locations.Length > 0 ? taskMethod.Locations[0] : Location.None; + ImmutableArray additionalLocations = taskLocation == Location.None + ? ImmutableArray.Empty + : ImmutableArray.Create(taskLocation); + context.ReportDiagnostic(Diagnostic.Create( - DiagnosticDescriptors.TransitiveUnsafeCall, - location, - FormatMethodFull(taskMethod), - violation.ApiDisplayName, - chainStr)); + violation.Descriptor, + violation.Location, + additionalLocations, + s_transitiveProperties, + messageArgs: new object[] + { + violation.ApiDisplayName, + violation.Message, + FormatMethodFull(taskMethod), + chainStr, + })); + } + + private static DiagnosticDescriptor GetTransitiveDescriptor(BannedApiDefinitions.ApiCategory category) + { + return category switch + { + BannedApiDefinitions.ApiCategory.CriticalError => DiagnosticDescriptors.TransitiveCriticalError, + BannedApiDefinitions.ApiCategory.TaskEnvironment => DiagnosticDescriptors.TransitiveTaskEnvironmentRequired, + BannedApiDefinitions.ApiCategory.PotentialIssue => DiagnosticDescriptors.TransitivePotentialIssue, + _ => throw new ArgumentOutOfRangeException(nameof(category), category, null), + }; } /// @@ -421,13 +465,22 @@ private static string FormatSymbolShort(ISymbol symbol) internal readonly struct ViolationInfo { - public string Category { get; } + public DiagnosticDescriptor Descriptor { get; } + public bool RequiresMultiThreadableScope { get; } + public Location Location { get; } public string ApiDisplayName { get; } public string Message { get; } - public ViolationInfo(string category, string apiDisplayName, string message) + public ViolationInfo( + DiagnosticDescriptor descriptor, + bool requiresMultiThreadableScope, + Location location, + string apiDisplayName, + string message) { - Category = category; + Descriptor = descriptor; + RequiresMultiThreadableScope = requiresMultiThreadableScope; + Location = location; ApiDisplayName = apiDisplayName; Message = message; } diff --git a/src/Tasks/GenerateLauncher.cs b/src/Tasks/GenerateLauncher.cs index d6f1fac72e6..04c95cc8208 100644 --- a/src/Tasks/GenerateLauncher.cs +++ b/src/Tasks/GenerateLauncher.cs @@ -41,8 +41,8 @@ public sealed class GenerateLauncher : TaskExtension, IMultiThreadableTask public ITaskItem OutputEntryPoint { get; set; } #endregion - // MSBuildTask0005 (transitive unsafe API) warnings are currently emitted here due to - // an analyzer limitation around data-flow reachability. See https://github.com/dotnet/msbuild/issues/13867. + // Transitive task-analyzer diagnostics are currently emitted here due to an analyzer + // limitation around data-flow reachability. See https://github.com/dotnet/msbuild/issues/13867. public override bool Execute() { if (!NativeMethodsShared.IsWindows) diff --git a/src/Tasks/Microsoft.Build.Tasks.csproj b/src/Tasks/Microsoft.Build.Tasks.csproj index 08665d5b04f..3679fc1fd30 100644 --- a/src/Tasks/Microsoft.Build.Tasks.csproj +++ b/src/Tasks/Microsoft.Build.Tasks.csproj @@ -624,7 +624,7 @@ - $(WarningsNotAsErrors);MSBuildTask0002;MSBuildTask0003;MSBuildTask0004;MSBuildTask0005 + $(WarningsNotAsErrors);MSBuildTask0002;MSBuildTask0003;MSBuildTask0004