diff --git a/documentation/specs/multithreading/thread-safe-tasks.md b/documentation/specs/multithreading/thread-safe-tasks.md index 9369877f3ed..d23d4471733 100644 --- a/documentation/specs/multithreading/thread-safe-tasks.md +++ b/documentation/specs/multithreading/thread-safe-tasks.md @@ -174,6 +174,21 @@ public bool Execute(...) } ``` +### Tasks That Construct Other Tasks + +The engine assigns `TaskEnvironment` only to the task instances it creates itself. A task that instantiates another task directly must pass its own environment along, otherwise the inner task keeps `TaskEnvironment.Fallback` and resolves paths and environment variables against the shared process state: + +```csharp +_runningExec = new Exec +{ + BuildEngine = BuildEngine, + TaskEnvironment = TaskEnvironment, // required: the engine does not inject here + Command = Command, +}; +``` + +The task-authoring analyzer reports `MSBuildTask0012` at Warning severity when a multithreadable task constructs an `ITask` without handing it a `TaskEnvironment` — through the object initializer, a constructor argument, or a later assignment on the instance — and offers a code fix that adds the initializer entry. + ## Appendix: Alternatives This appendix collects alternative approaches considered during design. diff --git a/src/TaskAnalyzer.Tests/TaskEnvironmentPropagationAnalyzerTests.cs b/src/TaskAnalyzer.Tests/TaskEnvironmentPropagationAnalyzerTests.cs new file mode 100644 index 00000000000..72a6f3c5f8f --- /dev/null +++ b/src/TaskAnalyzer.Tests/TaskEnvironmentPropagationAnalyzerTests.cs @@ -0,0 +1,412 @@ +// 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; +using Xunit; +using static Microsoft.Build.TaskAuthoring.Analyzer.Tests.TestHelpers; + +namespace Microsoft.Build.TaskAuthoring.Analyzer.Tests; + +public class TaskEnvironmentPropagationAnalyzerTests +{ + private const string InnerTasks = """ + public class InnerTask : Microsoft.Build.Utilities.Task, Microsoft.Build.Framework.IMultiThreadableTask + { + public Microsoft.Build.Framework.TaskEnvironment TaskEnvironment { get; set; } = null!; + public override bool Execute() => true; + } + + public class LegacyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """; + + [Fact] + public async Task ConstructedTaskWithoutTaskEnvironment_ProducesWarning() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new InnerTask { BuildEngine = BuildEngine }; + return inner.Execute(); + } + } + """); + + Diagnostic diagnostic = diagnostics.Single(); + diagnostic.Id.ShouldBe(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + diagnostic.Severity.ShouldBe(DiagnosticSeverity.Warning); + diagnostic.GetMessage().ShouldContain("InnerTask"); + } + + [Fact] + public async Task ConstructedToolTaskWithoutTaskEnvironment_ProducesWarning() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + private InnerToolTask? _running; + + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + _running = new InnerToolTask + { + BuildEngine = BuildEngine, + }; + + return _running.Execute(); + } + } + + public class InnerToolTask : Microsoft.Build.Utilities.ToolTask + { + protected override string ToolName => "tool"; + protected override string GenerateFullPathToTool() => "tool"; + public override bool Execute() => true; + } + """); + + diagnostics.Single().Id.ShouldBe(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + } + + [Fact] + public async Task ImplicitObjectCreation_ProducesWarning() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + InnerTask inner = new(); + return inner.Execute(); + } + } + """); + + diagnostics.Single().Id.ShouldBe(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + } + + [Fact] + public async Task TaskEnvironmentInObjectInitializer_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new InnerTask { BuildEngine = BuildEngine, TaskEnvironment = TaskEnvironment }; + return inner.Execute(); + } + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task TaskEnvironmentAssignedAfterCreation_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new InnerTask(); + inner.BuildEngine = BuildEngine; + inner.TaskEnvironment = TaskEnvironment; + return inner.Execute(); + } + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task TaskEnvironmentAssignedToFieldInAnotherMethod_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + private InnerTask? _inner; + + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + _inner = new InnerTask { BuildEngine = BuildEngine }; + Configure(); + return _inner.Execute(); + } + + private void Configure() => _inner!.TaskEnvironment = TaskEnvironment; + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task TaskEnvironmentPassedToConstructor_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new InjectedTask(TaskEnvironment); + return inner.Execute(); + } + } + + public class InjectedTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public InjectedTask(TaskEnvironment taskEnvironment) => TaskEnvironment = taskEnvironment; + + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() => true; + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task ConstructorTakingTaskEnvironmentNotUsed_ProducesWarning() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new InjectedTask(); + return inner.Execute(); + } + } + + public class InjectedTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public InjectedTask() => TaskEnvironment = null!; + + public InjectedTask(TaskEnvironment taskEnvironment) => TaskEnvironment = taskEnvironment; + + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() => true; + } + """); + + diagnostics.Single().Id.ShouldBe(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + } + + [Fact] + public async Task ConstructingTaskIsNotMultiThreadable_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var inner = new InnerTask { BuildEngine = BuildEngine }; + return inner.Execute(); + } + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task MultiThreadableAttributeWithoutTaskEnvironment_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() + { + var inner = new InnerTask { BuildEngine = BuildEngine }; + return inner.Execute(); + } + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task MultiThreadableAttributeWithTaskEnvironment_ProducesWarning() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + private readonly TaskEnvironment _taskEnvironment = new(); + + public override bool Execute() + { + var inner = new InnerTask { BuildEngine = BuildEngine }; + return inner.Execute(); + } + } + """); + + diagnostics.Single().Id.ShouldBe(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + } + + [Fact] + public async Task ConstructedTaskCannotReceiveTaskEnvironment_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new LegacyTask { BuildEngine = BuildEngine }; + return inner.Execute(); + } + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task ConstructedTypeIsNotATask_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var item = new TaskItem(); + return item.ItemSpec.Length == 0; + } + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task ConstructedTaskInFieldInitializer_ProducesWarning() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + private readonly InnerTask _inner = new InnerTask(); + + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() => _inner.Execute(); + } + """); + + diagnostics.Single().Id.ShouldBe(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + } + + [Fact] + public async Task ConstructedTaskInFieldInitializerConfiguredLater_DoesNotProduceDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + private readonly InnerTask _inner = new InnerTask(); + + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + _inner.TaskEnvironment = TaskEnvironment; + return _inner.Execute(); + } + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task ConstructedTaskInStaticMethod_ProducesWarning() + { + var diagnostics = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() => Run(); + + private static bool Run() => new InnerTask().Execute(); + } + """); + + diagnostics.Single().Id.ShouldBe(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + } + + private static async Task GetDiagnosticsAsync(string source) + { + var diagnostics = await GetCompilerAndAnalyzerDiagnosticsAsync( + $"{source}{System.Environment.NewLine}{InnerTasks}", + new TaskEnvironmentPropagationAnalyzer()); + + diagnostics.Where(diagnostic => diagnostic.Severity == DiagnosticSeverity.Error).ShouldBeEmpty(); + diagnostics.Where(diagnostic => diagnostic.Id == "AD0001").ShouldBeEmpty(); + + return diagnostics + .Where(diagnostic => diagnostic.Id == DiagnosticIds.PropagateTaskEnvironmentToConstructedTask) + .ToArray(); + } +} diff --git a/src/TaskAnalyzer.Tests/TaskEnvironmentPropagationCodeFixProviderTests.cs b/src/TaskAnalyzer.Tests/TaskEnvironmentPropagationCodeFixProviderTests.cs new file mode 100644 index 00000000000..d4ab9dd9506 --- /dev/null +++ b/src/TaskAnalyzer.Tests/TaskEnvironmentPropagationCodeFixProviderTests.cs @@ -0,0 +1,174 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Testing; +using Xunit; +using static Microsoft.Build.TaskAuthoring.Analyzer.Tests.TestHelpers; + +namespace Microsoft.Build.TaskAuthoring.Analyzer.Tests; + +/// +/// Tests for , which adds the missing +/// TaskEnvironment entry to the object initializer of a task constructed inside another task. +/// +public class TaskEnvironmentPropagationCodeFixProviderTests +{ + private const string InnerTask = """ + + public class InnerTask : Microsoft.Build.Utilities.Task, Microsoft.Build.Framework.IMultiThreadableTask + { + public Microsoft.Build.Framework.TaskEnvironment TaskEnvironment { get; set; } = null!; + public override bool Execute() => true; + } + """; + + private static CSharpCodeFixTest CreateFixTest( + string testCode, string fixedCode, params DiagnosticResult[] expected) + { + var test = new CSharpCodeFixTest + { + TestCode = testCode + InnerTask, + FixedCode = fixedCode + InnerTask, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }; + test.TestState.Sources.Add(("Stubs.cs", FrameworkStubs)); + test.FixedState.Sources.Add(("Stubs.cs", FrameworkStubs)); + test.ExpectedDiagnostics.AddRange(expected); + return test; + } + + private static DiagnosticResult Diag() => + new DiagnosticResult(DiagnosticDescriptors.PropagateTaskEnvironmentToConstructedTask); + + [Fact] + public async Task Fix_AddsEntryToExistingInitializer() + { + await CreateFixTest( + testCode: """ + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = {|#0:new InnerTask { BuildEngine = BuildEngine }|}; + return inner.Execute(); + } + } + """, + fixedCode: """ + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new InnerTask { BuildEngine = BuildEngine, TaskEnvironment = TaskEnvironment }; + return inner.Execute(); + } + } + """, + Diag().WithLocation(0).WithArguments("InnerTask")).RunAsync(); + } + + [Fact] + public async Task Fix_AddsInitializerWhenMissing() + { + await CreateFixTest( + testCode: """ + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = {|#0:new InnerTask()|}; + return inner.Execute(); + } + } + """, + fixedCode: """ + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() + { + var inner = new InnerTask() { TaskEnvironment = TaskEnvironment }; + return inner.Execute(); + } + } + """, + Diag().WithLocation(0).WithArguments("InnerTask")).RunAsync(); + } + + [Fact] + public async Task Fix_UsesTaskEnvironmentFieldOfConstructingTask() + { + await CreateFixTest( + testCode: """ + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + private readonly TaskEnvironment _taskEnvironment = new(); + + public override bool Execute() + { + var inner = {|#0:new InnerTask { BuildEngine = BuildEngine }|}; + return inner.Execute(); + } + } + """, + fixedCode: """ + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + private readonly TaskEnvironment _taskEnvironment = new(); + + public override bool Execute() + { + var inner = new InnerTask { BuildEngine = BuildEngine, TaskEnvironment = _taskEnvironment }; + return inner.Execute(); + } + } + """, + Diag().WithLocation(0).WithArguments("InnerTask")).RunAsync(); + } + + [Fact] + public async Task Fix_IsNotOfferedInStaticContext() + { + const string Source = """ + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + + public override bool Execute() => Run(); + + private static bool Run() => {|#0:new InnerTask()|}.Execute(); + } + """; + + await CreateFixTest( + testCode: Source, + fixedCode: Source, + Diag().WithLocation(0).WithArguments("InnerTask")).RunAsync(); + } +} diff --git a/src/TaskAnalyzer.Tests/TestHelpers.cs b/src/TaskAnalyzer.Tests/TestHelpers.cs index 73bbbb2dbf2..0de6cea57cc 100644 --- a/src/TaskAnalyzer.Tests/TestHelpers.cs +++ b/src/TaskAnalyzer.Tests/TestHelpers.cs @@ -110,8 +110,9 @@ public abstract class Task : Microsoft.Build.Framework.ITask public abstract bool Execute(); } - public abstract class ToolTask : Task + public abstract class ToolTask : Task, Microsoft.Build.Framework.IMultiThreadableTask { + public virtual Microsoft.Build.Framework.TaskEnvironment TaskEnvironment { get; set; } = new(); protected abstract string ToolName { get; } protected abstract string GenerateFullPathToTool(); } diff --git a/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md b/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md index 4dcafa5e110..137a0be8048 100644 --- a/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md +++ b/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md @@ -13,3 +13,4 @@ MSBuildTask0008 | MSBuild.TaskAuthoring | Warning | Initialize a relative defaul MSBuildTask0009 | MSBuild.TaskAuthoring | Warning | ITaskItem used with a type argument T that MSBuild cannot bind as a task parameter MSBuildTask0010 | MSBuild.TaskAuthoring | Error | ITaskItem 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 | A task constructed inside a multithreadable task does not receive its TaskEnvironment (code fix available) diff --git a/src/TaskAnalyzer/DiagnosticDescriptors.cs b/src/TaskAnalyzer/DiagnosticDescriptors.cs index 396f6ae2c86..4d8cdd51a51 100644 --- a/src/TaskAnalyzer/DiagnosticDescriptors.cs +++ b/src/TaskAnalyzer/DiagnosticDescriptors.cs @@ -112,6 +112,15 @@ internal static class DiagnosticDescriptors isEnabledByDefault: true, description: "Constructor injection makes TaskEnvironment available to constructor logic and environment-dependent default initialization. The MSBuild engine prefers a public constructor with a single TaskEnvironment parameter when one is available."); + public static readonly DiagnosticDescriptor PropagateTaskEnvironmentToConstructedTask = new( + id: DiagnosticIds.PropagateTaskEnvironmentToConstructedTask, + title: "Propagate TaskEnvironment to a task constructed inside a task", + messageFormat: "'{0}' is constructed without receiving a TaskEnvironment; pass this task's TaskEnvironment to it so it does not fall back to the shared process environment", + category: "MSBuild.TaskAuthoring", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "MSBuild only supplies TaskEnvironment to tasks it instantiates itself. A task instantiated by another task falls back to TaskEnvironment.Fallback and resolves paths and environment variables against the shared process state, so the constructing task must pass its own TaskEnvironment along."); + public static ImmutableArray All { get; } = ImmutableArray.Create( CriticalError, TaskEnvironmentRequired, @@ -123,6 +132,7 @@ internal static class DiagnosticDescriptors InitializeRelativeDefaultInExecute, UnsupportedTaskItemType, CultureSensitiveTaskItemType, - PreferTaskEnvironmentConstructorInjection); + PreferTaskEnvironmentConstructorInjection, + PropagateTaskEnvironmentToConstructedTask); } } diff --git a/src/TaskAnalyzer/DiagnosticIds.cs b/src/TaskAnalyzer/DiagnosticIds.cs index d846f1c652b..70e0db7bf76 100644 --- a/src/TaskAnalyzer/DiagnosticIds.cs +++ b/src/TaskAnalyzer/DiagnosticIds.cs @@ -41,5 +41,8 @@ public static class DiagnosticIds /// Task should receive TaskEnvironment through constructor injection. public const string PreferTaskEnvironmentConstructorInjection = "MSBuildTask0011"; + + /// A task constructed inside a task does not receive the constructing task's TaskEnvironment. + public const string PropagateTaskEnvironmentToConstructedTask = "MSBuildTask0012"; } } diff --git a/src/TaskAnalyzer/README.md b/src/TaskAnalyzer/README.md index d4e3cc2f511..96f237e9a3f 100644 --- a/src/TaskAnalyzer/README.md +++ b/src/TaskAnalyzer/README.md @@ -25,6 +25,7 @@ This analyzer catches unsafe API usage at compile time and offers code fixes to | **MSBuildTask0009** | Warning | All `ITask` implementations | `ITaskItem` used with unsupported type argument | | **MSBuildTask0010** | Error | All `ITask` implementations | `ITaskItem` relies on culture-sensitive conversion | | **MSBuildTask0011** | Info | Concrete `IMultiThreadableTask` implementations | Prefer constructor injection for `TaskEnvironment` | +| **MSBuildTask0012** | Warning | Multithreadable tasks that hold a `TaskEnvironment` | Task constructed inside a task does not receive `TaskEnvironment` | ### MSBuildTask0001 — Critical: No Safe Alternative @@ -287,6 +288,45 @@ The engine prefers this constructor when it is present. A public parameterless c **Scope:** Concrete classes implementing `IMultiThreadableTask`. Abstract base classes and tasks that already declare a public single-`TaskEnvironment` constructor do not produce the diagnostic. +### MSBuildTask0012 — Propagate `TaskEnvironment` to a Constructed Task + +MSBuild injects `TaskEnvironment` only into the tasks it instantiates itself. A task instance created by *another* task therefore keeps `TaskEnvironment.Fallback` and resolves paths and environment variables against the shared process state — even when the constructing task is fully migrated and uses `TaskEnvironment` correctly everywhere in its own body: + +```csharp +[MSBuildMultiThreadableTask] +public class ExecWithRetries : Task, IMultiThreadableTask +{ + public TaskEnvironment TaskEnvironment { get; set; } + + public override bool Execute() + { + // ⚠️ MSBuildTask0012: 'Exec' is constructed without receiving a TaskEnvironment + _runningExec = new Exec + { + BuildEngine = BuildEngine, + Command = Command, + }; + + return _runningExec.Execute(); + } +} +``` + +Hand the constructed task the environment of the task that creates it: + +```csharp +_runningExec = new Exec +{ + BuildEngine = BuildEngine, + TaskEnvironment = TaskEnvironment, // inner task now resolves paths like its host + Command = Command, +}; +``` + +The environment counts as propagated when it is assigned in the object initializer, passed as a constructor argument, or assigned on the instance afterwards — including from another member of the same type, so a field configured in a helper method is recognized. + +**Scope:** Types implementing `IMultiThreadableTask` or carrying `[MSBuildMultiThreadableTask]` that hold a `TaskEnvironment` of their own; a task with no environment to propagate is not reported. The created type must implement `ITask` and be able to receive an environment — through a publicly settable `TaskEnvironment` property (such as `ToolTask.TaskEnvironment`, which is `public virtual`) or a constructor parameter — so the diagnostic is always actionable. + ## Analysis Scope The analyzer determines what to check based on the type declaration: @@ -294,8 +334,8 @@ The analyzer determines what to check based on the type declaration: | Type | Rules Applied | |---|---| | Any class implementing `ITask` | MSBuildTask0001–MSBuildTask0005, 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 | +| Class with `[MSBuildMultiThreadableTask]` attribute applied directly | MSBuildTask0006–MSBuildTask0008 and MSBuildTask0012 (in addition to MSBuildTask0001–0005 and MSBuildTask0009–MSBuildTask0010) | +| Concrete class implementing `IMultiThreadableTask` without the attribute | MSBuildTask0001–MSBuildTask0005 and MSBuildTask0009–MSBuildTask0012 | | Helper class with `[MSBuildMultiThreadableTaskAnalyzed]` attribute | MSBuildTask0001–MSBuildTask0005 | | Regular class (no task interface or attribute) | Not analyzed | @@ -311,6 +351,7 @@ The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classe - **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** — a constructed task silently losing the environment is a correctness issue, but only in multithreaded execution. ## Code Fixes @@ -333,6 +374,7 @@ The analyzer ships with a code fix provider that offers automatic replacements: | MSBuildTask0007: `new FileInfo(item.ItemSpec)` in `foreach` over `ITaskItem[]` | → Retype source property to ``ITaskItem[]`` and replace with `item.Value` | | MSBuildTask0007: `new AbsolutePath(Item.GetMetadata("FullPath"))` | → Retype `Item` to ``ITaskItem`` and replace with `Item.Value` | | MSBuildTask0008: relative default `= "obj"` on a path property | → Retype the property (unset default) and move the default into `Execute()` as a guarded, `TaskEnvironment`-rooted assignment | +| MSBuildTask0012: `new Exec { BuildEngine = BuildEngine }` | → `new Exec { BuildEngine = BuildEngine, TaskEnvironment = TaskEnvironment }` | 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. diff --git a/src/TaskAnalyzer/TaskEnvironmentPropagationAnalyzer.cs b/src/TaskAnalyzer/TaskEnvironmentPropagationAnalyzer.cs new file mode 100644 index 00000000000..22cf09066bd --- /dev/null +++ b/src/TaskAnalyzer/TaskEnvironmentPropagationAnalyzer.cs @@ -0,0 +1,325 @@ +// 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.Concurrent; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +using static Microsoft.Build.TaskAuthoring.Analyzer.SharedAnalyzerHelpers; + +namespace Microsoft.Build.TaskAuthoring.Analyzer +{ + /// + /// Roslyn analyzer that reports MSBuildTask0012: a multithreadable task constructs another + /// ITask without handing it a TaskEnvironment. + /// + /// MSBuild only injects TaskEnvironment into the tasks it instantiates itself, so a task + /// instance created by another task silently falls back to TaskEnvironment.Fallback and + /// resolves paths and environment variables against the shared process state. + /// + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class TaskEnvironmentPropagationAnalyzer : DiagnosticAnalyzer + { + public override ImmutableArray SupportedDiagnostics { get; } = + ImmutableArray.Create(DiagnosticDescriptors.PropagateTaskEnvironmentToConstructedTask); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterCompilationStartAction(OnCompilationStart); + } + + private static void OnCompilationStart(CompilationStartAnalysisContext compilationContext) + { + var iTaskType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.ITaskFullName); + var taskEnvironmentType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.TaskEnvironmentFullName); + if (iTaskType is null || taskEnvironmentType is null) + { + return; + } + + var iMultiThreadableTaskType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.IMultiThreadableTaskFullName); + var multiThreadableTaskAttributeType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MultiThreadableTaskAttributeFullName); + + compilationContext.RegisterSymbolStartAction(symbolStartContext => + { + var namedType = (INamedTypeSymbol)symbolStartContext.Symbol; + + // Only tasks that have a TaskEnvironment of their own can propagate one. Tasks that merely + // carry [MSBuildMultiThreadableTask] without holding a TaskEnvironment have nothing to pass on. + if (!IsMultiThreadable(namedType, iMultiThreadableTaskType, multiThreadableTaskAttributeType) || + !HasTaskEnvironmentMember(namedType, taskEnvironmentType)) + { + return; + } + + // Operation actions within a symbol may run concurrently, so both collections must be thread-safe. + var candidates = new ConcurrentBag<(Location Location, string TypeName, ISymbol? Target)>(); + var receiversWithEnvironment = new ConcurrentDictionary(SymbolEqualityComparer.Default); + + symbolStartContext.RegisterOperationAction( + operationContext => AnalyzeObjectCreation(operationContext, candidates, iTaskType, taskEnvironmentType), + OperationKind.ObjectCreation); + + symbolStartContext.RegisterOperationAction( + operationContext => TrackTaskEnvironmentAssignment(operationContext, receiversWithEnvironment, taskEnvironmentType), + OperationKind.SimpleAssignment); + + symbolStartContext.RegisterSymbolEndAction(symbolEndContext => + { + foreach ((Location location, string typeName, ISymbol? target) in candidates) + { + // A task stored in a local, field, or property may receive its environment through a + // later assignment anywhere in the declaring type. + if (target is not null && receiversWithEnvironment.ContainsKey(target)) + { + continue; + } + + symbolEndContext.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.PropagateTaskEnvironmentToConstructedTask, + location, + typeName)); + } + }); + }, SymbolKind.NamedType); + } + + private static void AnalyzeObjectCreation( + OperationAnalysisContext context, + ConcurrentBag<(Location Location, string TypeName, ISymbol? Target)> candidates, + INamedTypeSymbol iTaskType, + INamedTypeSymbol taskEnvironmentType) + { + var creation = (IObjectCreationOperation)context.Operation; + + if (creation.Type is not INamedTypeSymbol createdType || + !ImplementsInterface(createdType, iTaskType) || + !CanReceiveTaskEnvironment(createdType, taskEnvironmentType) || + ReceivesTaskEnvironment(creation, taskEnvironmentType)) + { + return; + } + + candidates.Add(( + creation.Syntax.GetLocation(), + createdType.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat), + GetCreationTarget(creation))); + } + + /// + /// Records the local, field, or property whose TaskEnvironment is assigned, so a creation + /// stored in that symbol is not reported. + /// + private static void TrackTaskEnvironmentAssignment( + OperationAnalysisContext context, + ConcurrentDictionary receiversWithEnvironment, + INamedTypeSymbol taskEnvironmentType) + { + var assignment = (ISimpleAssignmentOperation)context.Operation; + + IOperation? instance = assignment.Target switch + { + IPropertyReferenceOperation propertyReference when IsTaskEnvironmentType(propertyReference.Property.Type, taskEnvironmentType) => propertyReference.Instance, + IFieldReferenceOperation fieldReference when IsTaskEnvironmentType(fieldReference.Field.Type, taskEnvironmentType) => fieldReference.Instance, + _ => null, + }; + + if (GetReferencedSymbol(instance) is ISymbol receiver) + { + receiversWithEnvironment[receiver] = true; + } + } + + /// + /// Checks whether the created task is handed a TaskEnvironment through a constructor + /// argument or through an object initializer. + /// + private static bool ReceivesTaskEnvironment(IObjectCreationOperation creation, INamedTypeSymbol taskEnvironmentType) + { + foreach (IArgumentOperation argument in creation.Arguments) + { + if (argument.ArgumentKind != ArgumentKind.DefaultValue && + argument.Parameter is not null && + IsTaskEnvironmentType(argument.Parameter.Type, taskEnvironmentType)) + { + return true; + } + } + + if (creation.Initializer is IObjectOrCollectionInitializerOperation initializer) + { + foreach (IOperation initializerOperation in initializer.Initializers) + { + if (initializerOperation is ISimpleAssignmentOperation assignment && + IsTaskEnvironmentType(assignment.Target.Type, taskEnvironmentType)) + { + return true; + } + } + } + + return false; + } + + /// + /// Returns the local, field, or property the newly created task is stored in, or + /// when the instance is not stored anywhere this analyzer can track. + /// + private static ISymbol? GetCreationTarget(IObjectCreationOperation creation) + { + IOperation? parent = creation.Parent; + while (parent is IConversionOperation conversion && conversion.IsImplicit) + { + parent = conversion.Parent; + } + + return parent switch + { + IVariableInitializerOperation { Parent: IVariableDeclaratorOperation declarator } => declarator.Symbol, + IFieldInitializerOperation fieldInitializer => fieldInitializer.InitializedFields.FirstOrDefault(), + IPropertyInitializerOperation propertyInitializer => propertyInitializer.InitializedProperties.FirstOrDefault(), + ISimpleAssignmentOperation assignment => GetReferencedSymbol(assignment.Target), + _ => null, + }; + } + + private static ISymbol? GetReferencedSymbol(IOperation? operation) => operation switch + { + ILocalReferenceOperation localReference => localReference.Local, + IFieldReferenceOperation fieldReference => fieldReference.Field, + IPropertyReferenceOperation propertyReference => propertyReference.Property, + IParameterReferenceOperation parameterReference => parameterReference.Parameter, + _ => null, + }; + + /// + /// Checks whether the constructing type declares or inherits a readable TaskEnvironment member. + /// + private static bool HasTaskEnvironmentMember(INamedTypeSymbol type, INamedTypeSymbol taskEnvironmentType) + { + foreach (IPropertySymbol property in GetPropertiesIncludingBaseTypes(type)) + { + if (!property.IsStatic && property.GetMethod is not null && IsTaskEnvironmentType(property.Type, taskEnvironmentType)) + { + return true; + } + } + + for (INamedTypeSymbol? current = type; + current is not null && current.SpecialType != SpecialType.System_Object; + current = current.BaseType) + { + foreach (ISymbol member in current.GetMembers()) + { + if (member is IFieldSymbol { IsStatic: false, IsImplicitlyDeclared: false } field && + IsTaskEnvironmentType(field.Type, taskEnvironmentType)) + { + return true; + } + } + } + + return false; + } + + /// + /// Checks whether a TaskEnvironment can be handed to the created task at all — through a + /// settable property or through a constructor parameter. Without either, there is nothing to suggest. + /// + private static bool CanReceiveTaskEnvironment(INamedTypeSymbol createdType, INamedTypeSymbol taskEnvironmentType) + { + if (TryGetTaskEnvironmentProperty(createdType, taskEnvironmentType, out _)) + { + return true; + } + + foreach (IMethodSymbol constructor in createdType.InstanceConstructors) + { + foreach (IParameterSymbol parameter in constructor.Parameters) + { + if (IsTaskEnvironmentType(parameter.Type, taskEnvironmentType)) + { + return true; + } + } + } + + return false; + } + + /// + /// Finds a publicly settable instance property of type TaskEnvironment on the type or one of its bases. + /// + internal static bool TryGetTaskEnvironmentProperty( + INamedTypeSymbol type, + INamedTypeSymbol taskEnvironmentType, + out IPropertySymbol? taskEnvironmentProperty) + { + foreach (IPropertySymbol property in GetPropertiesIncludingBaseTypes(type)) + { + if (!property.IsStatic && + property.DeclaredAccessibility == Accessibility.Public && + property.SetMethod?.DeclaredAccessibility == Accessibility.Public && + IsTaskEnvironmentType(property.Type, taskEnvironmentType)) + { + taskEnvironmentProperty = property; + return true; + } + } + + taskEnvironmentProperty = null; + return false; + } + + /// + /// Checks whether a type is TaskEnvironment or derives from it. + /// + internal static bool IsTaskEnvironmentType(ITypeSymbol? type, INamedTypeSymbol taskEnvironmentType) + { + for (ITypeSymbol? current = type; current is not null; current = current.BaseType) + { + if (SymbolEqualityComparer.Default.Equals(current, taskEnvironmentType)) + { + return true; + } + } + + return false; + } + + /// + /// Checks whether a type opts into multithreaded task execution, either through + /// IMultiThreadableTask or through the [MSBuildMultiThreadableTask] attribute. + /// + private static bool IsMultiThreadable( + INamedTypeSymbol type, + INamedTypeSymbol? iMultiThreadableTaskType, + INamedTypeSymbol? multiThreadableTaskAttributeType) + { + if (iMultiThreadableTaskType is not null && ImplementsInterface(type, iMultiThreadableTaskType)) + { + return true; + } + + if (multiThreadableTaskAttributeType is null) + { + return false; + } + + foreach (AttributeData attribute in type.GetAttributes()) + { + if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, multiThreadableTaskAttributeType)) + { + return true; + } + } + + return false; + } + } +} diff --git a/src/TaskAnalyzer/TaskEnvironmentPropagationCodeFixProvider.cs b/src/TaskAnalyzer/TaskEnvironmentPropagationCodeFixProvider.cs new file mode 100644 index 00000000000..dc4a17ce2b4 --- /dev/null +++ b/src/TaskAnalyzer/TaskEnvironmentPropagationCodeFixProvider.cs @@ -0,0 +1,190 @@ +// 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.Immutable; +using System.Composition; +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.CSharp.Syntax; +using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Formatting; + +namespace Microsoft.Build.TaskAuthoring.Analyzer +{ + /// + /// Code fixer for MSBuildTask0012: adds a TaskEnvironment entry to the object initializer of a + /// task constructed inside another task, so the constructed task receives the constructing task's environment. + /// + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(TaskEnvironmentPropagationCodeFixProvider))] + [Shared] + public sealed class TaskEnvironmentPropagationCodeFixProvider : CodeFixProvider + { + public override ImmutableArray FixableDiagnosticIds => + ImmutableArray.Create(DiagnosticIds.PropagateTaskEnvironmentToConstructedTask); + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null || semanticModel is null) + { + return; + } + + var taskEnvironmentType = semanticModel.Compilation.GetTypeByMetadataName(WellKnownTypeNames.TaskEnvironmentFullName); + if (taskEnvironmentType is null) + { + return; + } + + foreach (var diagnostic in context.Diagnostics) + { + var node = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + if (node.AncestorsAndSelf().OfType().FirstOrDefault() is not BaseObjectCreationExpressionSyntax creation) + { + continue; + } + + // A collection initializer cannot carry a member assignment. + if (creation.Initializer is not null && !creation.Initializer.IsKind(SyntaxKind.ObjectInitializerExpression)) + { + continue; + } + + if (semanticModel.GetTypeInfo(creation, context.CancellationToken).Type is not INamedTypeSymbol createdType || + !TaskEnvironmentPropagationAnalyzer.TryGetTaskEnvironmentProperty(createdType, taskEnvironmentType, out IPropertySymbol? taskEnvironmentProperty) || + taskEnvironmentProperty is null) + { + continue; + } + + if (FindTaskEnvironmentSource(semanticModel, creation, taskEnvironmentType, context.CancellationToken) is not string sourceName) + { + continue; + } + + string targetName = taskEnvironmentProperty.Name; + context.RegisterCodeFix( + CodeAction.Create( + title: $"Assign {targetName} from the constructing task", + createChangedDocument: ct => AddTaskEnvironmentInitializerAsync(context.Document, creation, targetName, sourceName, ct), + equivalenceKey: "PropagateTaskEnvironment"), + diagnostic); + } + } + + /// + /// Finds a readable TaskEnvironment member of the constructing type that is accessible and usable + /// at the creation site, and returns the name to reference it by. + /// + private static string? FindTaskEnvironmentSource( + SemanticModel semanticModel, + SyntaxNode creation, + INamedTypeSymbol taskEnvironmentType, + CancellationToken cancellationToken) + { + int position = creation.SpanStart; + ISymbol? enclosingSymbol = semanticModel.GetEnclosingSymbol(position, cancellationToken); + if (enclosingSymbol is null || IsInStaticContext(enclosingSymbol)) + { + return null; + } + + INamedTypeSymbol? containingType = enclosingSymbol as INamedTypeSymbol ?? enclosingSymbol.ContainingType; + if (containingType is null) + { + return null; + } + + foreach (IPropertySymbol property in SharedAnalyzerHelpers.GetPropertiesIncludingBaseTypes(containingType)) + { + if (!property.IsStatic && + property.GetMethod is not null && + TaskEnvironmentPropagationAnalyzer.IsTaskEnvironmentType(property.Type, taskEnvironmentType) && + semanticModel.IsAccessible(position, property)) + { + return property.Name; + } + } + + for (INamedTypeSymbol? current = containingType; + current is not null && current.SpecialType != SpecialType.System_Object; + current = current.BaseType) + { + foreach (ISymbol member in current.GetMembers()) + { + if (member is IFieldSymbol { IsStatic: false, IsImplicitlyDeclared: false } field && + TaskEnvironmentPropagationAnalyzer.IsTaskEnvironmentType(field.Type, taskEnvironmentType) && + semanticModel.IsAccessible(position, field)) + { + return field.Name; + } + } + } + + return null; + } + + /// + /// Determines whether instance members are unavailable at the creation site, walking out of lambdas and + /// local functions to the member that encloses them. + /// + private static bool IsInStaticContext(ISymbol enclosingSymbol) + { + for (ISymbol? symbol = enclosingSymbol; symbol is not null and not INamedTypeSymbol; symbol = symbol.ContainingSymbol) + { + bool isStatic = symbol switch + { + IMethodSymbol method => method.IsStatic, + IFieldSymbol field => field.IsStatic, + IPropertySymbol property => property.IsStatic, + IEventSymbol @event => @event.IsStatic, + _ => false, + }; + + if (isStatic) + { + return true; + } + } + + return false; + } + + private static async Task AddTaskEnvironmentInitializerAsync( + Document document, + BaseObjectCreationExpressionSyntax creation, + string targetName, + string sourceName, + CancellationToken cancellationToken) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + + var assignment = SyntaxFactory.AssignmentExpression( + SyntaxKind.SimpleAssignmentExpression, + SyntaxFactory.IdentifierName(targetName), + SyntaxFactory.IdentifierName(sourceName)); + + InitializerExpressionSyntax initializer = creation.Initializer is InitializerExpressionSyntax existingInitializer + ? existingInitializer.WithExpressions(existingInitializer.Expressions.Add(assignment)) + : SyntaxFactory.InitializerExpression( + SyntaxKind.ObjectInitializerExpression, + SyntaxFactory.SingletonSeparatedList(assignment)); + + SyntaxNode newCreation = creation + .WithInitializer(initializer) + .WithAdditionalAnnotations(Formatter.Annotation); + + editor.ReplaceNode(creation, newCreation); + + return editor.GetChangedDocument(); + } + } +}