diff --git a/documentation/specs/multithreading/thread-safe-tasks.md b/documentation/specs/multithreading/thread-safe-tasks.md index 57a261413a7..828883650ca 100644 --- a/documentation/specs/multithreading/thread-safe-tasks.md +++ b/documentation/specs/multithreading/thread-safe-tasks.md @@ -108,6 +108,10 @@ For tasks to be eligible for multithreaded execution using this approach, they m public class MyTask : Task {...} ``` +#### Keeping a Migrated Repository Migrated + +The attribute is not inherited, and a task that lacks it is routed to an out-of-proc task host rather than failing, so a task added after a migration regresses the repository silently. The task-authoring analyzer reports `MSBuildTask0015` for concrete task types that do not declare multithreading support, with a code fix that applies the attribute, implements `IMultiThreadableTask`, and adds the `TaskEnvironment` property. The rule reports nothing until a repository opts into it, either with `msbuild_task_analyzer.scope = require_multithreadable` or by configuring `dotnet_diagnostic.MSBuildTask0015.severity` explicitly. + ## TaskEnvironment API The `TaskEnvironment` provides thread-safe alternatives to APIs that use global process state, enabling tasks to execute safely in a multithreaded environment. diff --git a/src/TaskAnalyzer.Tests/MultiThreadableTaskDeclarationAnalyzerTests.cs b/src/TaskAnalyzer.Tests/MultiThreadableTaskDeclarationAnalyzerTests.cs index 8425c00070c..25ac9e4231a 100644 --- a/src/TaskAnalyzer.Tests/MultiThreadableTaskDeclarationAnalyzerTests.cs +++ b/src/TaskAnalyzer.Tests/MultiThreadableTaskDeclarationAnalyzerTests.cs @@ -417,6 +417,34 @@ public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask diagnostic.GetMessage().ShouldContain("MyTask"); } + /// + /// The attribute is matched by full name, mirroring the engine, so these rules keep working when a + /// repository's own copy of the attribute makes the name ambiguous and unresolvable as a symbol. + /// See SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute. + /// + [Fact] + public async Task AttributeFromReferencedAssembly_OnNonTaskType_ProducesWarning() + { + CSharpCompilation compilation = CreateCompilationWithAttributeFromReferences(""" + [Microsoft.Build.Framework.MSBuildMultiThreadableTask] + public class PathHelper + { + public string Combine(string a, string b) => a + b; + } + """); + + // The premise of the name-based matching: the symbol is unresolvable here. + compilation.GetTypeByMetadataName("Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute").ShouldBeNull(); + + var diagnostics = await compilation + .WithAnalyzers(ImmutableArray.Create(new MultiThreadableTaskDeclarationAnalyzer())) + .GetAnalyzerDiagnosticsAsync(); + + Diagnostic diagnostic = diagnostics + .Single(d => d.Id == DiagnosticIds.MultiThreadableTaskAttributeHasNoEffect); + diagnostic.GetMessage().ShouldContain("PathHelper"); + } + private static async Task GetDiagnosticsWithMissingAttributeRuleEnabledAsync(string source) { CSharpCompilation compilation = CreateCompilation(source); diff --git a/src/TaskAnalyzer.Tests/RequireMultiThreadableTaskAnalyzerTests.cs b/src/TaskAnalyzer.Tests/RequireMultiThreadableTaskAnalyzerTests.cs new file mode 100644 index 00000000000..7cd9a5ab90a --- /dev/null +++ b/src/TaskAnalyzer.Tests/RequireMultiThreadableTaskAnalyzerTests.cs @@ -0,0 +1,346 @@ +// 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.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Testing; +using Shouldly; +using Xunit; +using static Microsoft.Build.TaskAuthoring.Analyzer.Tests.TestHelpers; + +namespace Microsoft.Build.TaskAuthoring.Analyzer.Tests; + +public class RequireMultiThreadableTaskAnalyzerTests +{ + private const string SeverityOptionKey = "dotnet_diagnostic." + DiagnosticIds.RequireMultiThreadableTask + ".severity"; + + private const string MarkedUpTaskWithoutOptIn = """ + public class {|#0:MyTask|} : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """; + + private const string ConcreteTaskWithoutOptIn = """ + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """; + + [Fact] + public async Task RequireScope_ConcreteTaskWithoutAttribute_ProducesDiagnostic() + { + var diagnostics = await GetDiagnosticsForScopeAsync(ConcreteTaskWithoutOptIn, SharedAnalyzerHelpers.ScopeRequireMultiThreadable); + + Diagnostic diagnostic = diagnostics.Single(); + diagnostic.Id.ShouldBe(DiagnosticIds.RequireMultiThreadableTask); + diagnostic.Severity.ShouldBe(DiagnosticSeverity.Warning); + diagnostic.GetMessage().ShouldContain("MyTask"); + } + + [Fact] + public async Task RequireScope_ThroughBuildProperty_ProducesDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync( + ConcreteTaskWithoutOptIn, + new Dictionary + { + { $"build_property.{SharedAnalyzerHelpers.ScopeOptionKey}", SharedAnalyzerHelpers.ScopeRequireMultiThreadable }, + }); + + diagnostics.Single().Id.ShouldBe(DiagnosticIds.RequireMultiThreadableTask); + } + + [Fact] + public async Task NoScopeConfigured_ConcreteTaskWithoutAttribute_ProducesNoDiagnostic() + { + var diagnostics = await GetDiagnosticsAsync(ConcreteTaskWithoutOptIn, []); + + diagnostics.ShouldBeEmpty(); + } + + [Theory] + [InlineData(SharedAnalyzerHelpers.ScopeAll)] + [InlineData(SharedAnalyzerHelpers.ScopeMultiThreadableOnly)] + public async Task OtherScopes_ConcreteTaskWithoutAttribute_ProduceNoDiagnostic(string scope) + { + var diagnostics = await GetDiagnosticsForScopeAsync(ConcreteTaskWithoutOptIn, scope); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task NoConfiguration_ProducesNoDiagnostic() + { + await CreateAnalyzerTest(ConcreteTaskWithoutOptIn, analyzerConfig: null).RunAsync(); + } + + [Fact] + public async Task EditorConfigSeverity_WithoutScope_ProducesDiagnostic() + { + await CreateAnalyzerTest( + MarkedUpTaskWithoutOptIn, + analyzerConfig: ("/.editorconfig", $""" + root = true + [*.cs] + {SeverityOptionKey} = warning + """), + new DiagnosticResult(DiagnosticDescriptors.RequireMultiThreadableTask).WithLocation(0).WithArguments("MyTask")).RunAsync(); + } + + [Fact] + public async Task EditorConfigSeverityNone_WithoutScope_ProducesNoDiagnostic() + { + await CreateAnalyzerTest( + ConcreteTaskWithoutOptIn, + analyzerConfig: ("/.editorconfig", $""" + root = true + [*.cs] + {SeverityOptionKey} = none + """)).RunAsync(); + } + + [Fact] + public async Task EditorConfigSeverityOnOneFileOfPartialTask_ProducesDiagnosticThere() + { + var test = CreateAnalyzerTest( + """ + public partial class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """, + analyzerConfig: ("/.editorconfig", $""" + root = true + [Other.cs] + {SeverityOptionKey} = warning + """), + new DiagnosticResult(DiagnosticDescriptors.RequireMultiThreadableTask).WithLocation(0).WithArguments("MyTask")); + test.TestState.Sources.Add(("/0/Other.cs", """ + public partial class {|#0:MyTask|} + { + } + """)); + + await test.RunAsync(); + } + + [Fact] + public async Task RulesetSeverity_WithoutScope_ProducesDiagnostic() + { + // A ruleset or entry surfaces as a compilation-wide specific diagnostic option. + Compilation compilation = CreateCompilation(ConcreteTaskWithoutOptIn); + compilation = compilation.WithOptions(compilation.Options.WithSpecificDiagnosticOptions( + ImmutableDictionary.Empty.Add(DiagnosticIds.RequireMultiThreadableTask, ReportDiagnostic.Error))); + + var diagnostics = await compilation + .WithAnalyzers(ImmutableArray.Create(new RequireMultiThreadableTaskAnalyzer())) + .GetAnalyzerDiagnosticsAsync(); + + Diagnostic diagnostic = diagnostics.Single(); + diagnostic.Id.ShouldBe(DiagnosticIds.RequireMultiThreadableTask); + diagnostic.Severity.ShouldBe(DiagnosticSeverity.Error); + } + + [Fact] + public async Task RequireScope_TaskWithAttribute_ProducesNoDiagnostic() + { + var diagnostics = await GetRequiredDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task RequireScope_AbstractTask_ProducesNoDiagnostic() + { + var diagnostics = await GetRequiredDiagnosticsAsync(""" + public abstract class MyTask : Microsoft.Build.Utilities.Task + { + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task RequireScope_NonTaskClass_ProducesNoDiagnostic() + { + var diagnostics = await GetRequiredDiagnosticsAsync(""" + public class Helper + { + public bool Execute() => true; + } + """); + + diagnostics.ShouldBeEmpty(); + } + + [Fact] + public async Task RequireScope_ConcreteTaskDerivedFromAnnotatedBase_ProducesDiagnostic() + { + // The attribute is Inherited = false, so the leaf type has not opted in and the engine still + // routes it to a TaskHost — the mistake this rule exists to catch. + var diagnostics = await GetRequiredDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public abstract class MultiThreadableTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + } + + public class MyTask : MultiThreadableTask + { + public override bool Execute() => true; + } + """); + + diagnostics.Single().GetMessage().ShouldContain("MyTask"); + } + + [Fact] + public async Task RequireScope_MultiThreadableTaskWithoutAttribute_ProducesDiagnostic() + { + var diagnostics = await GetRequiredDiagnosticsAsync(""" + using Microsoft.Build.Framework; + + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + public override bool Execute() => true; + } + """); + + diagnostics.Single().GetMessage().ShouldContain("MyTask"); + } + + [Fact] + public async Task RequireScope_SameNamedAttributeFromAnotherNamespace_ProducesDiagnostic() + { + // The engine matches the attribute by namespace and name, so an unrelated attribute that merely + // shares the name is not an opt-in. + var diagnostics = await GetRequiredDiagnosticsAsync(""" + namespace Contoso + { + [System.AttributeUsage(System.AttributeTargets.Class)] + public sealed class MSBuildMultiThreadableTaskAttribute : System.Attribute + { + } + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + } + """); + + diagnostics.Single().GetMessage().ShouldContain("MyTask"); + } + + [Fact] + public async Task RequireScope_NestedConcreteTask_ProducesDiagnostic() + { + var diagnostics = await GetRequiredDiagnosticsAsync(""" + public class Outer + { + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + } + """); + + diagnostics.Single().GetMessage().ShouldContain("MyTask"); + } + + /// + /// The engine matches the attribute by full name and ignores the defining assembly, so a task marked with a + /// repository's own copy really is routed in-process and must not be told to opt in. That copy also makes the + /// name ambiguous, so the attribute cannot be resolved as a symbol at all -- the reason this rule and its + /// siblings match by name. See SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute. + /// + [Fact] + public async Task RequireScope_AttributeFromReferencedAssembly_ProducesNoDiagnostic() + { + var compilation = TestHelpers.CreateCompilationWithAttributeFromReferences(""" + [Microsoft.Build.Framework.MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """); + + // The premise of the rule's name-based matching: the symbol is unresolvable here. + compilation.GetTypeByMetadataName("Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute").ShouldBeNull(); + + var diagnostics = await TestHelpers.GetDiagnosticsWithGlobalOptionsAsync( + compilation, + new RequireMultiThreadableTaskAnalyzer(), + new Dictionary + { + { SharedAnalyzerHelpers.ScopeOptionKey, SharedAnalyzerHelpers.ScopeRequireMultiThreadable }, + }); + + diagnostics + .Where(diagnostic => diagnostic.Id == DiagnosticIds.RequireMultiThreadableTask) + .ShouldBeEmpty(); + } + + private static CSharpAnalyzerTest CreateAnalyzerTest( + string source, (string Path, string Content)? analyzerConfig, params DiagnosticResult[] expected) + { + var test = new CSharpAnalyzerTest + { + TestCode = source, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }; + test.TestState.Sources.Add(("Stubs.cs", FrameworkStubs)); + if (analyzerConfig is (string path, string content)) + { + test.TestState.AnalyzerConfigFiles.Add((path, content)); + } + + test.ExpectedDiagnostics.AddRange(expected); + return test; + } + + private static Task GetRequiredDiagnosticsAsync(string source) => + GetDiagnosticsAsync( + source, + new Dictionary + { + { SharedAnalyzerHelpers.ScopeOptionKey, SharedAnalyzerHelpers.ScopeRequireMultiThreadable }, + }); + + private static async Task GetDiagnosticsForScopeAsync(string source, string scope) => + await GetDiagnosticsAsync( + source, + new Dictionary { { SharedAnalyzerHelpers.ScopeOptionKey, scope } }); + + private static async Task GetDiagnosticsAsync(string source, Dictionary globalOptions) + { + var diagnostics = await GetDiagnosticsWithGlobalOptionsAsync( + source, + new RequireMultiThreadableTaskAnalyzer(), + globalOptions); + + return diagnostics + .Where(diagnostic => diagnostic.Id == DiagnosticIds.RequireMultiThreadableTask) + .ToArray(); + } +} diff --git a/src/TaskAnalyzer.Tests/RequireMultiThreadableTaskCodeFixProviderTests.cs b/src/TaskAnalyzer.Tests/RequireMultiThreadableTaskCodeFixProviderTests.cs new file mode 100644 index 00000000000..d6d13c443bd --- /dev/null +++ b/src/TaskAnalyzer.Tests/RequireMultiThreadableTaskCodeFixProviderTests.cs @@ -0,0 +1,226 @@ +// 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.CSharp.Testing; +using Microsoft.CodeAnalysis.Testing; +using Xunit; +using static Microsoft.Build.TaskAuthoring.Analyzer.Tests.TestHelpers; + +namespace Microsoft.Build.TaskAuthoring.Analyzer.Tests; + +/// +/// Tests for , the code fix for MSBuildTask0015. +/// The rule only reports when it is opted into, so every test supplies a .globalconfig setting the scope. +/// +public class RequireMultiThreadableTaskCodeFixProviderTests +{ + private const string GlobalConfig = """ + is_global = true + msbuild_task_analyzer.scope = require_multithreadable + """; + + private static CSharpCodeFixTest CreateFixTest( + string testCode, string fixedCode, params DiagnosticResult[] expected) + { + var test = new CSharpCodeFixTest + { + TestCode = testCode, + FixedCode = fixedCode, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }; + test.TestState.Sources.Add(("Stubs.cs", FrameworkStubs)); + test.FixedState.Sources.Add(("Stubs.cs", FrameworkStubs)); + test.TestState.AnalyzerConfigFiles.Add(("/.globalconfig", GlobalConfig)); + test.FixedState.AnalyzerConfigFiles.Add(("/.globalconfig", GlobalConfig)); + test.ExpectedDiagnostics.AddRange(expected); + return test; + } + + private static DiagnosticResult Diag(string taskName) => + new DiagnosticResult(DiagnosticDescriptors.RequireMultiThreadableTask).WithLocation(0).WithArguments(taskName); + + [Fact] + public async Task Fix_AddsAttributeInterfaceAndProperty() + { + await CreateFixTest( + testCode: """ + using Microsoft.Build.Framework; + public class {|#0:MyTask|} : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """, + fixedCode: """ + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; + + public override bool Execute() => true; + } + """, + Diag("MyTask")).RunAsync(); + } + + [Fact] + public async Task Fix_TaskAlreadyImplementingInterface_AddsAttributeOnly() + { + await CreateFixTest( + testCode: """ + using Microsoft.Build.Framework; + public class {|#0:MyTask|} : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; + public override bool Execute() => true; + } + """, + fixedCode: """ + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; + public override bool Execute() => true; + } + """, + Diag("MyTask")).RunAsync(); + } + + [Fact] + public async Task Fix_TaskDerivingFromMultiThreadableBase_AddsAttributeOnly() + { + // The base already provides the interface and the property; only the leaf's opt-in is missing, + // because the attribute is not inherited. + await CreateFixTest( + testCode: """ + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public abstract class MultiThreadableTaskBase : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; + } + + public class {|#0:MyTask|} : MultiThreadableTaskBase + { + public override bool Execute() => true; + } + """, + fixedCode: """ + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public abstract class MultiThreadableTaskBase : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; + } + + [MSBuildMultiThreadableTask] + public class MyTask : MultiThreadableTaskBase + { + public override bool Execute() => true; + } + """, + Diag("MyTask")).RunAsync(); + } + + [Fact] + public async Task Fix_TaskWithoutFrameworkUsing_QualifiesAddedTypes() + { + await CreateFixTest( + testCode: """ + public class {|#0:MyTask|} : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + } + """, + fixedCode: """ + [Microsoft.Build.Framework.MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task, Microsoft.Build.Framework.IMultiThreadableTask + { + public Microsoft.Build.Framework.TaskEnvironment TaskEnvironment { get; set; } = Microsoft.Build.Framework.TaskEnvironment.Fallback; + + public override bool Execute() => true; + } + """, + Diag("MyTask")).RunAsync(); + } + + [Fact] + public async Task Fix_TaskWithConflictingTaskEnvironmentMember_AddsAttributeOnly() + { + // Declaring the interface would not compile against an unrelated member of the same name, so only the + // attribute is applied — which is on its own a valid opt-in as far as the engine's routing is concerned. + await CreateFixTest( + testCode: """ + public class {|#0:MyTask|} : Microsoft.Build.Utilities.Task + { + public string TaskEnvironment { get; set; } = ""; + public override bool Execute() => true; + } + """, + fixedCode: """ + [Microsoft.Build.Framework.MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + public string TaskEnvironment { get; set; } = ""; + public override bool Execute() => true; + } + """, + Diag("MyTask")).RunAsync(); + } + + [Fact] + public async Task Fix_TaskWithNonPublicTaskEnvironmentSetter_AddsAttributeOnly() + { + // The property is of the right type but its setter is private, so it does not implement the interface + // member; declaring the interface would not compile and only the attribute is applied. + await CreateFixTest( + testCode: """ + public class {|#0:MyTask|} : Microsoft.Build.Utilities.Task + { + public Microsoft.Build.Framework.TaskEnvironment TaskEnvironment { get; private set; } = Microsoft.Build.Framework.TaskEnvironment.Fallback; + public override bool Execute() => true; + } + """, + fixedCode: """ + [Microsoft.Build.Framework.MSBuildMultiThreadableTask] + public class MyTask : Microsoft.Build.Utilities.Task + { + public Microsoft.Build.Framework.TaskEnvironment TaskEnvironment { get; private set; } = Microsoft.Build.Framework.TaskEnvironment.Fallback; + public override bool Execute() => true; + } + """, + Diag("MyTask")).RunAsync(); + } + + [Fact] + public async Task Fix_TaskImplementingITaskDirectly_AddsAttributeInterfaceAndProperty() + { + await CreateFixTest( + testCode: """ + using Microsoft.Build.Framework; + public class {|#0:MyTask|} : ITask + { + public IBuildEngine BuildEngine { get; set; } + public bool Execute() => true; + } + """, + fixedCode: """ + using Microsoft.Build.Framework; + + [MSBuildMultiThreadableTask] + public class MyTask : ITask, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; + public IBuildEngine BuildEngine { get; set; } + public bool Execute() => true; + } + """, + Diag("MyTask")).RunAsync(); + } +} diff --git a/src/TaskAnalyzer.Tests/TestHelpers.cs b/src/TaskAnalyzer.Tests/TestHelpers.cs index 73bbbb2dbf2..d5b659c98e1 100644 --- a/src/TaskAnalyzer.Tests/TestHelpers.cs +++ b/src/TaskAnalyzer.Tests/TestHelpers.cs @@ -40,6 +40,7 @@ public interface IMultiThreadableTask : ITask public class TaskEnvironment { + public static TaskEnvironment Fallback { get; } = new TaskEnvironment(); public AbsolutePath ProjectDirectory => default; public string? GetEnvironmentVariable(string name) => null; public void SetEnvironmentVariable(string name, string? value) { } @@ -241,22 +242,105 @@ public static CSharpCompilation CreateCompilation(string source) } /// - /// Runs the MultiThreadableTaskAnalyzer with a specific scope option and returns analyzer diagnostics. + /// Creates a compilation in which Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute is + /// contributed by two referenced assemblies rather than by source. + /// + /// This is the shape a repository ends up in when it keeps its own copy of the attribute -- the shim that + /// lets its tasks stay buildable against an MSBuild that predates it -- while also referencing a + /// Microsoft.Build.Framework that declares the real one. The engine matches the attribute by full name and + /// ignores the defining assembly, so those tasks really are routed in-process and the analyzers must agree. + /// It is also the case where gives up and returns null, + /// which is why the analyzers cannot resolve the attribute as a symbol. + /// /// - public static async System.Threading.Tasks.Task> GetDiagnosticsWithScopeAsync(string source, string scope) + public static CSharpCompilation CreateCompilationWithAttributeFromReferences(string source) { - var compilation = CreateCompilation(source); - var analyzer = new MultiThreadableTaskAnalyzer(); + const string attributeDeclaration = "public class MSBuildMultiThreadableTaskAttribute : System.Attribute { }"; - var globalOptions = new Dictionary + string stubsWithoutAttribute = FrameworkStubs.Replace(attributeDeclaration, string.Empty); + if (stubsWithoutAttribute.Length == FrameworkStubs.Length) { - { $"build_property.{SharedAnalyzerHelpers.ScopeOptionKey}", scope } - }; + throw new System.InvalidOperationException( + "The attribute declaration was not found in FrameworkStubs; this helper needs updating."); + } + + var references = s_coreReferences + .Concat(new[] { CreateAttributeAssembly("CustomerShim"), CreateAttributeAssembly("FrameworkLike") }); + + return CSharpCompilation.Create( + "TestAssembly", + new[] + { + CSharpSyntaxTree.ParseText(source, path: "Test.cs"), + CSharpSyntaxTree.ParseText(stubsWithoutAttribute, path: "Stubs.cs"), + }, + references, + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary) + .WithNullableContextOptions(NullableContextOptions.Enable)); + } + + private static MetadataReference CreateAttributeAssembly(string assemblyName) + { + var assembly = CSharpCompilation.Create( + assemblyName, + new[] + { + CSharpSyntaxTree.ParseText(""" + namespace Microsoft.Build.Framework + { + [System.AttributeUsage(System.AttributeTargets.Class, Inherited = false)] + public class MSBuildMultiThreadableTaskAttribute : System.Attribute { } + } + """), + }, + s_coreReferences, + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + + var stream = new System.IO.MemoryStream(); + Microsoft.CodeAnalysis.Emit.EmitResult result = assembly.Emit(stream); + if (!result.Success) + { + throw new System.InvalidOperationException( + $"Failed to compile '{assemblyName}': " + string.Join("; ", result.Diagnostics)); + } + + stream.Position = 0; + return MetadataReference.CreateFromStream(stream); + } + + /// + /// Runs the MultiThreadableTaskAnalyzer with a specific scope option and returns analyzer diagnostics. + /// + public static System.Threading.Tasks.Task> GetDiagnosticsWithScopeAsync(string source, string scope) => + GetDiagnosticsWithGlobalOptionsAsync( + source, + new MultiThreadableTaskAnalyzer(), + new Dictionary { { $"build_property.{SharedAnalyzerHelpers.ScopeOptionKey}", scope } }); + + /// + /// Runs the given analyzer with the supplied global analyzer config options (the options a + /// .globalconfig or an MSBuild property surfaces to analyzers) and returns analyzer diagnostics. + /// + public static async System.Threading.Tasks.Task> GetDiagnosticsWithGlobalOptionsAsync( + string source, + DiagnosticAnalyzer analyzer, + Dictionary globalOptions) => + await GetDiagnosticsWithGlobalOptionsAsync(CreateCompilation(source), analyzer, globalOptions); + + /// + /// Runs the given analyzer against an already-built compilation, for tests that need to control how the + /// compilation is assembled. + /// + public static async System.Threading.Tasks.Task> GetDiagnosticsWithGlobalOptionsAsync( + Compilation compilation, + DiagnosticAnalyzer analyzer, + Dictionary globalOptions) + { var optionsProvider = new TestAnalyzerConfigOptionsProvider(globalOptions); var options = new AnalyzerOptions(ImmutableArray.Empty, optionsProvider); var compilationWithAnalyzers = compilation.WithAnalyzers( - ImmutableArray.Create(analyzer), options); + ImmutableArray.Create(analyzer), options); return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); } diff --git a/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md b/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md index 1a3d3ba0137..3093329e77e 100644 --- a/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md +++ b/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md @@ -16,3 +16,4 @@ MSBuildTask0011 | MSBuild.TaskAuthoring | Info | Prefer constructor injection fo MSBuildTask0012 | MSBuild.TaskAuthoring | Warning | TaskEnvironment property is never assigned by MSBuild because the task does not implement IMultiThreadableTask MSBuildTask0013 | MSBuild.TaskAuthoring | Info | Task declares IMultiThreadableTask but is not marked with [MSBuildMultiThreadableTask] (disabled by default) MSBuildTask0014 | MSBuild.TaskAuthoring | Warning | [MSBuildMultiThreadableTask] applied to a type MSBuild never routes as a task -- not an ITask, or an abstract task whose attribute no subclass inherits -- where it has no effect +MSBuildTask0015 | MSBuild.TaskAuthoring | Warning | Concrete task type does not declare multithreading support; reports only when opted into with `msbuild_task_analyzer.scope = require_multithreadable` or an explicit severity (code fix available) diff --git a/src/TaskAnalyzer/DiagnosticDescriptors.cs b/src/TaskAnalyzer/DiagnosticDescriptors.cs index 2698e65c45a..eea2f7999d7 100644 --- a/src/TaskAnalyzer/DiagnosticDescriptors.cs +++ b/src/TaskAnalyzer/DiagnosticDescriptors.cs @@ -139,6 +139,15 @@ internal static class DiagnosticDescriptors isEnabledByDefault: true, description: "TaskRouter reads [MSBuildMultiThreadableTask] with inherit: false, off the concrete type the engine has just instantiated as a task. The attribute therefore only has an effect on a non-abstract class that implements ITask. On a type that is not a task, nothing ever reads it. On an abstract task, the engine never instantiates that type, and because the attribute is not inherited the concrete subclasses do not pick it up -- so every one of them is still routed to an out-of-proc TaskHost. Both shapes usually mean the attribute was applied to the wrong class: a helper type beside the real task, or a shared base instead of each task that derives from it."); + public static readonly DiagnosticDescriptor RequireMultiThreadableTask = new( + id: DiagnosticIds.RequireMultiThreadableTask, + title: "Concrete MSBuild task type does not opt into multithreaded execution", + messageFormat: "Task '{0}' does not declare multithreading support; apply [MSBuildMultiThreadableTask] so it is not routed to an out-of-proc TaskHost", + category: "MSBuild.TaskAuthoring", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "In multithreaded builds, a task without the [MSBuildMultiThreadableTask] attribute runs in an out-of-proc TaskHost, which succeeds but is slow. The attribute is not inherited, so deriving from a migrated base class is not enough. This rule reports nothing unless it is opted into, either with 'msbuild_task_analyzer.scope = require_multithreadable' or by configuring its severity explicitly; a repository that has finished migrating its tasks opts in to keep new tasks from silently regressing. It covers every concrete task type, so it subsumes MSBuildTask0013, which reports the same missing attribute only on the narrower set of tasks that declare IMultiThreadableTask; a repository that opts into this rule does not also need to enable that one."); + public static ImmutableArray All { get; } = ImmutableArray.Create( CriticalError, TaskEnvironmentRequired, @@ -153,6 +162,7 @@ internal static class DiagnosticDescriptors PreferTaskEnvironmentConstructorInjection, TaskEnvironmentNeverAssigned, MissingMultiThreadableTaskAttribute, - MultiThreadableTaskAttributeHasNoEffect); + MultiThreadableTaskAttributeHasNoEffect, + RequireMultiThreadableTask); } } diff --git a/src/TaskAnalyzer/DiagnosticIds.cs b/src/TaskAnalyzer/DiagnosticIds.cs index ca688e318d9..a7569e98995 100644 --- a/src/TaskAnalyzer/DiagnosticIds.cs +++ b/src/TaskAnalyzer/DiagnosticIds.cs @@ -50,5 +50,8 @@ public static class DiagnosticIds /// [MSBuildMultiThreadableTask] is applied to a type MSBuild never routes as a task, so it has no effect. public const string MultiThreadableTaskAttributeHasNoEffect = "MSBuildTask0014"; + + /// Concrete task type does not declare multithreading support and is routed to an out-of-proc TaskHost. + public const string RequireMultiThreadableTask = "MSBuildTask0015"; } } diff --git a/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs b/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs index 26a0d711125..4292d68530b 100644 --- a/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs +++ b/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs @@ -64,7 +64,6 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte var iTaskItemType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.ITaskItemFullName); var consoleType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.ConsoleFullName); var analyzedAttributeType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.AnalyzedAttributeFullName); - var multiThreadableTaskAttributeType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MultiThreadableTaskAttributeFullName); // Build symbol lookup for banned APIs var bannedApiLookup = BuildBannedApiLookup(compilationContext.Compilation); @@ -86,8 +85,7 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte namedType.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, analyzedAttributeType)); // Tasks marked with [MSBuildMultiThreadableTask] should be analyzed as multithreadable - bool hasMultiThreadableAttribute = multiThreadableTaskAttributeType is not null && - namedType.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, multiThreadableTaskAttributeType)); + bool hasMultiThreadableAttribute = SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute(namedType); if (!isTask && !hasAnalyzedAttribute) { diff --git a/src/TaskAnalyzer/MultiThreadableTaskDeclarationAnalyzer.cs b/src/TaskAnalyzer/MultiThreadableTaskDeclarationAnalyzer.cs index fc0714504c9..b3cd45d06df 100644 --- a/src/TaskAnalyzer/MultiThreadableTaskDeclarationAnalyzer.cs +++ b/src/TaskAnalyzer/MultiThreadableTaskDeclarationAnalyzer.cs @@ -46,10 +46,8 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) context.Compilation.GetTypeByMetadataName(WellKnownTypeNames.IMultiThreadableTaskFullName); INamedTypeSymbol? taskEnvironmentType = context.Compilation.GetTypeByMetadataName(WellKnownTypeNames.TaskEnvironmentFullName); - INamedTypeSymbol? attributeType = - context.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MultiThreadableTaskAttributeFullName); - if (taskType is null || multiThreadableTaskType is null || taskEnvironmentType is null || attributeType is null) + if (taskType is null || multiThreadableTaskType is null || taskEnvironmentType is null) { return; } @@ -59,8 +57,7 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) symbolContext, taskType, multiThreadableTaskType, - taskEnvironmentType, - attributeType), + taskEnvironmentType), SymbolKind.NamedType); } @@ -68,8 +65,7 @@ private static void AnalyzeNamedType( SymbolAnalysisContext context, INamedTypeSymbol taskType, INamedTypeSymbol multiThreadableTaskType, - INamedTypeSymbol taskEnvironmentType, - INamedTypeSymbol attributeType) + INamedTypeSymbol taskEnvironmentType) { var type = (INamedTypeSymbol)context.Symbol; @@ -78,7 +74,7 @@ private static void AnalyzeNamedType( return; } - bool hasAttribute = HasMultiThreadableTaskAttribute(type, attributeType); + bool hasAttribute = SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute(type); if (!SharedAnalyzerHelpers.ImplementsInterface(type, taskType)) { @@ -155,20 +151,6 @@ private static bool DeclaresMultiThreadableTaskInterface( return false; } - private static bool HasMultiThreadableTaskAttribute(INamedTypeSymbol type, INamedTypeSymbol attributeType) - { - // Matches TaskRouter, which reads the attribute with inherit: false. - foreach (AttributeData attribute in type.GetAttributes()) - { - if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, attributeType)) - { - return true; - } - } - - return false; - } - private static bool HasTaskEnvironmentProperty(INamedTypeSymbol type, INamedTypeSymbol taskEnvironmentType) { foreach (IPropertySymbol property in SharedAnalyzerHelpers.GetPropertiesIncludingBaseTypes(type)) diff --git a/src/TaskAnalyzer/PreferTypedParameterAnalyzer.cs b/src/TaskAnalyzer/PreferTypedParameterAnalyzer.cs index 8282129088b..9d3f7d8ed6a 100644 --- a/src/TaskAnalyzer/PreferTypedParameterAnalyzer.cs +++ b/src/TaskAnalyzer/PreferTypedParameterAnalyzer.cs @@ -46,7 +46,6 @@ private readonly struct WellKnownTaskTypes { public WellKnownTaskTypes( INamedTypeSymbol iTask, - INamedTypeSymbol multiThreadableTaskAttribute, INamedTypeSymbol? absolutePath, INamedTypeSymbol? taskEnvironment, INamedTypeSymbol? iTaskItem, @@ -55,7 +54,6 @@ public WellKnownTaskTypes( INamedTypeSymbol? directoryInfo) { ITask = iTask; - MultiThreadableTaskAttribute = multiThreadableTaskAttribute; AbsolutePath = absolutePath; TaskEnvironment = taskEnvironment; ITaskItem = iTaskItem; @@ -65,7 +63,6 @@ public WellKnownTaskTypes( } public INamedTypeSymbol ITask { get; } - public INamedTypeSymbol MultiThreadableTaskAttribute { get; } public INamedTypeSymbol? AbsolutePath { get; } public INamedTypeSymbol? TaskEnvironment { get; } public INamedTypeSymbol? ITaskItem { get; } @@ -89,7 +86,6 @@ private static void OnCompilationStart(CompilationStartAnalysisContext compilati } var iTaskType = types.ITask; - var multiThreadableTaskAttributeType = types.MultiThreadableTaskAttribute; var absolutePathType = types.AbsolutePath; var taskEnvironmentType = types.TaskEnvironment; var iTaskItemType = types.ITaskItem; @@ -102,7 +98,7 @@ private static void OnCompilationStart(CompilationStartAnalysisContext compilati var namedType = (INamedTypeSymbol)symbolStartContext.Symbol; // Only multithreadable tasks (ITask + directly-applied [MSBuildMultiThreadableTask]) are analyzed. - if (!IsMultiThreadableTaskType(namedType, iTaskType, multiThreadableTaskAttributeType)) + if (!IsMultiThreadableTaskType(namedType, iTaskType)) { return; } @@ -270,16 +266,10 @@ private static bool TryResolveWellKnownTaskTypes(Compilation compilation, out We // The task must additionally opt into multithreaded support by applying the // [MSBuildMultiThreadableTask] attribute. Implementing IMultiThreadableTask is not sufficient: the // attribute is Inherited = false, so a task that merely derives from a base class implementing the - // interface has not itself opted into multithreaded support. - var multiThreadableTaskAttributeType = compilation.GetTypeByMetadataName(WellKnownTypeNames.MultiThreadableTaskAttributeFullName); - if (multiThreadableTaskAttributeType is null) - { - return false; - } - + // interface has not itself opted into multithreaded support. The attribute is matched by name rather + // than resolved as a symbol, mirroring the engine -- see SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute. types = new WellKnownTaskTypes( iTaskType, - multiThreadableTaskAttributeType, compilation.GetTypeByMetadataName(WellKnownTypeNames.AbsolutePathFullName), compilation.GetTypeByMetadataName(WellKnownTypeNames.TaskEnvironmentFullName), compilation.GetTypeByMetadataName(WellKnownTypeNames.ITaskItemFullName), @@ -297,12 +287,10 @@ private static bool TryResolveWellKnownTaskTypes(Compilation compilation, out We /// private static bool IsMultiThreadableTaskType( INamedTypeSymbol namedType, - INamedTypeSymbol iTaskType, - INamedTypeSymbol multiThreadableTaskAttributeType) + INamedTypeSymbol iTaskType) { return ImplementsInterface(namedType, iTaskType) && - namedType.GetAttributes().Any( - attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, multiThreadableTaskAttributeType)); + SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute(namedType); } /// diff --git a/src/TaskAnalyzer/README.md b/src/TaskAnalyzer/README.md index de4ede6fd60..ae7958df43d 100644 --- a/src/TaskAnalyzer/README.md +++ b/src/TaskAnalyzer/README.md @@ -28,6 +28,7 @@ This analyzer catches unsafe API usage at compile time and offers code fixes to | **MSBuildTask0012** | Warning | Concrete tasks with `[MSBuildMultiThreadableTask]` applied directly | MSBuild never assigns the `TaskEnvironment` property | | **MSBuildTask0013** | Info (off by default) | Concrete tasks declaring `IMultiThreadableTask` in their own base list | Missing `[MSBuildMultiThreadableTask]`, so the task still runs out-of-proc | | **MSBuildTask0014** | Warning | Classes carrying `[MSBuildMultiThreadableTask]` that are not an `ITask`, or are abstract | The attribute has no effect because MSBuild never routes that type as a task | +| **MSBuildTask0015** | Warning (opt-in) | Concrete `ITask` implementations, only when opted in | Concrete task type does not opt into multithreaded execution | ### MSBuildTask0001 — Critical: No Safe Alternative @@ -391,6 +392,41 @@ Fix by moving the attribute onto each concrete task class. Both shapes usually m A concrete task that MSBuild cannot construct — no public parameterless constructor and no public single-`TaskEnvironment` constructor — is a third inert shape, but it is **not** reported. `Microsoft.Build.Utilities.Task.RegisterTask(string, Func)` lets a host supply an arbitrary factory, so such a task may be perfectly reachable. +### MSBuildTask0015 — Require Multithreading Opt-In + +In multithreaded builds the engine routes every task without a directly applied `[MSBuildMultiThreadableTask]` attribute to an out-of-proc TaskHost. That build still succeeds, just more slowly, so a task added after a repository finished migrating gives back part of the benefit of the migration without any diagnostic. This rule turns that silent regression into a warning: + +```csharp +public class MyTask : Task // ⚠️ MSBuildTask0015 — no opt-in, runs in a TaskHost +{ + public override bool Execute() => true; +} +``` + +The rule reports nothing unless it is opted into, because it would otherwise fire on every task in a repository that has not migrated yet. Repositories that have completed a migration opt in through the analysis scope: + +```ini +# .globalconfig +is_global = true +msbuild_task_analyzer.scope = require_multithreadable +``` + +Configuring the severity explicitly is also an opt-in, for finer control (for example, downgrading the rule for a directory of test tasks): + +```ini +[*.cs] +dotnet_diagnostic.MSBuildTask0015.severity = error + +[test/**/*.cs] +dotnet_diagnostic.MSBuildTask0015.severity = none +``` + +The attribute is `Inherited = false`, so **a concrete task deriving from an already-migrated base class is still reported** — the leaf type is what the engine's routing looks at. Abstract base classes and interfaces are not reported, since they cannot opt in on behalf of the types deriving from them. + +**Scope:** Concrete (non-abstract) classes implementing `ITask` that do not carry `[MSBuildMultiThreadableTask]` directly, when the rule is opted into. + +This rule covers every concrete task type, so it subsumes [MSBuildTask0013](#msbuildtask0013--missing-msbuildmultithreadabletask), which reports the same missing attribute on the narrower set of tasks that declare `IMultiThreadableTask`. A repository that opts into MSBuildTask0015 does not also need to enable MSBuildTask0013; 0013 remains useful on its own for a codebase that wants the narrower signal without the repo-wide gate. + ## Analysis Scope The analyzer determines what to check based on the type declaration: @@ -401,6 +437,7 @@ The analyzer determines what to check based on the type declaration: | 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 | +| Concrete class implementing `ITask` without the attribute, when opted in | MSBuildTask0015 (in addition to the rules above) | | Regular class (no task interface or attribute) | Not analyzed | | Class with `[MSBuildMultiThreadableTask]` that does not implement `ITask` | MSBuildTask0014 | | Abstract class with `[MSBuildMultiThreadableTask]` | MSBuildTask0014 | @@ -411,11 +448,25 @@ The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classe **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) on unsafe APIs within those helpers. Note that the MSBuildTask0002/0003 code fixes reference a `TaskEnvironment` member, so they are only offered in a helper that declares one — see [Code Fixes](#code-fixes). +### Configuring the Scope + +The `msbuild_task_analyzer.scope` option, set in a `.globalconfig` (or as an MSBuild property surfaced to analyzers), selects which task types are analyzed: + +| Value | Behavior | +|---|---| +| `all` (default) | Analyze every `ITask` implementation | +| `multithreadable_only` | Analyze only tasks that carry `[MSBuildMultiThreadableTask]` — useful while a migration is in progress | +| `require_multithreadable` | Analyze every `ITask` implementation **and** require each concrete task type to declare multithreading support (MSBuildTask0015) | + ### Severity Levels - **MSBuildTask0001** is always **Error** — these APIs are never safe in any MSBuild task. - **MSBuildTask0002–MSBuildTask0005, MSBuildTask0009, and MSBuildTask0010** report as **Warning**. - **MSBuildTask0006–MSBuildTask0008 and MSBuildTask0011** report as **Info** — these are modernization suggestions, not correctness issues. +- **MSBuildTask0012** reports as **Warning** — the `TaskEnvironment` property is silently inert, which is a correctness issue. +- **MSBuildTask0013** is **disabled by default** — running out-of-proc is a performance characteristic, and the shape it reports is a valid intermediate migration state. +- **MSBuildTask0014** reports as **Warning** — the attribute is inert, and the task the author meant to mark is usually still running out-of-proc. +- **MSBuildTask0015** reports as **Warning**, but only once it is opted into with `msbuild_task_analyzer.scope = require_multithreadable` or an explicit severity — otherwise it would fire on every task in a repository that has not migrated yet. ## Code Fixes @@ -438,6 +489,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: concrete task without the opt-in | → Apply `[MSBuildMultiThreadableTask]`, implement `IMultiThreadableTask`, and add the `TaskEnvironment` property | The MSBuildTask0003 fixer anchors on the **call the analyzer flagged** (the one whose parameter takes the path) and wraps that call's own path argument. This matters when the flagged call is nested inside another call — `new StreamWriter(File.Create(OutputPath))` becomes `new StreamWriter(File.Create(TaskEnvironment.GetAbsolutePath(OutputPath)))`, not a wrap around the `Stream` the outer constructor receives. Within that call it wraps the first **unwrapped** path parameter rather than blindly wrapping the first argument — so for `File.Copy(safePath, unsafePath)` it correctly wraps the second argument, and for `Directory.GetFiles(dir, searchPattern)` it leaves the search pattern alone. @@ -557,6 +609,8 @@ dotnet test | `DiagnosticIds.cs` | Public constants: `MSBuildTask0001`–`MSBuildTask0008` | | `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) | +| `RequireMultiThreadableTaskAnalyzer.cs` | Analyzer for MSBuildTask0012 — reports concrete task types that do not declare multithreading support, once the rule is opted into | +| `RequireMultiThreadableTaskCodeFixProvider.cs` | Code fix for MSBuildTask0012 — applies the attribute, implements `IMultiThreadableTask`, and adds the `TaskEnvironment` property | ### Performance diff --git a/src/TaskAnalyzer/RequireMultiThreadableTaskAnalyzer.cs b/src/TaskAnalyzer/RequireMultiThreadableTaskAnalyzer.cs new file mode 100644 index 00000000000..def92b3d71f --- /dev/null +++ b/src/TaskAnalyzer/RequireMultiThreadableTaskAnalyzer.cs @@ -0,0 +1,152 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Immutable; +using System.Threading; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Microsoft.Build.TaskAuthoring.Analyzer +{ + /// + /// Reports concrete MSBuild task types that do not declare multithreading support (MSBuildTask0015). + /// + /// In multithreaded builds the engine routes every task without a directly applied + /// [MSBuildMultiThreadableTask] attribute to an out-of-proc TaskHost. That is not an error and + /// produces no diagnostic of its own, so a task added after a repository finished migrating silently + /// gives back the benefit of the migration. This rule turns that silent regression into a diagnostic. + /// + /// The rule reports nothing unless it is opted into, either by setting + /// msbuild_task_analyzer.scope = require_multithreadable or by configuring + /// dotnet_diagnostic.MSBuildTask0015.severity explicitly. + /// + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class RequireMultiThreadableTaskAnalyzer : DiagnosticAnalyzer + { + public override ImmutableArray SupportedDiagnostics { get; } = + ImmutableArray.Create(DiagnosticDescriptors.RequireMultiThreadableTask); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterCompilationStartAction(OnCompilationStart); + } + + private static void OnCompilationStart(CompilationStartAnalysisContext context) + { + // The scope option, a ruleset / entry, and a .globalconfig severity are + // compilation-wide, so they are read once here. A severity set through .editorconfig can vary per file, + // so when nothing opted in compilation-wide the trees are scanned once for a per-file opt-in, and the + // tree of a type that would otherwise be reported is then checked individually. + bool optedIn = SharedAnalyzerHelpers.ReadRequireMultiThreadableOption(context.Options.AnalyzerConfigOptionsProvider) || + IsEnabledForCompilation(context.Compilation, context.CancellationToken); + + // No type is examined when the rule is not opted into anywhere in the compilation, so a repository that + // has not migrated pays nothing for the rule. + if (!optedIn && !IsEnabledForAnyTree(context.Compilation, context.CancellationToken)) + { + return; + } + + INamedTypeSymbol? iTaskType = context.Compilation.GetTypeByMetadataName(WellKnownTypeNames.ITaskFullName); + if (iTaskType is null) + { + return; + } + + context.RegisterSymbolAction(symbolContext => AnalyzeNamedType(symbolContext, iTaskType, optedIn), SymbolKind.NamedType); + } + + private static void AnalyzeNamedType(SymbolAnalysisContext context, INamedTypeSymbol iTaskType, bool optedIn) + { + var taskType = (INamedTypeSymbol)context.Symbol; + + // The attribute is not inherited, so only concrete types are asked to declare support: an abstract + // base cannot opt in on behalf of the types deriving from it. + if (taskType.TypeKind != TypeKind.Class || + taskType.IsAbstract || + !SharedAnalyzerHelpers.ImplementsInterface(taskType, iTaskType) || + SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute(taskType)) + { + return; + } + + // A partial type can be declared in several files, and .editorconfig can enable the rule for some of + // them only, so the first declaration the rule is enabled for is the one reported. + foreach (Location location in taskType.Locations) + { + if (location.IsInSource && + (optedIn || IsEnabledForTree(context.Compilation, location.SourceTree, context.CancellationToken))) + { + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.RequireMultiThreadableTask, + location, + taskType.Name)); + return; + } + } + } + + /// + /// Returns true when the rule's severity is configured for the whole compilation, either by a ruleset, + /// <WarningsAsErrors> and friends, or by dotnet_diagnostic.MSBuildTask0015.severity in a + /// .globalconfig. Configuring the severity is an opt-in on its own, so a repository that prefers per-rule + /// configuration over the scope option is not forced to set both. + /// + private static bool IsEnabledForCompilation(Compilation compilation, CancellationToken cancellationToken) + { + if (compilation.Options.SpecificDiagnosticOptions.TryGetValue(DiagnosticIds.RequireMultiThreadableTask, out ReportDiagnostic severity)) + { + return IsEnabled(severity); + } + + SyntaxTreeOptionsProvider? optionsProvider = compilation.Options.SyntaxTreeOptionsProvider; + return optionsProvider is not null && + optionsProvider.TryGetGlobalDiagnosticValue(DiagnosticIds.RequireMultiThreadableTask, cancellationToken, out severity) && + IsEnabled(severity); + } + + /// + /// Returns true when dotnet_diagnostic.MSBuildTask0015.severity is configured for any file in the + /// compilation, which is how a repository enables the rule for part of its sources only. + /// + private static bool IsEnabledForAnyTree(Compilation compilation, CancellationToken cancellationToken) + { + if (compilation.Options.SyntaxTreeOptionsProvider is null) + { + return false; + } + + foreach (SyntaxTree tree in compilation.SyntaxTrees) + { + if (IsEnabledForTree(compilation, tree, cancellationToken)) + { + return true; + } + } + + return false; + } + + /// + /// Returns true when dotnet_diagnostic.MSBuildTask0015.severity is configured for the given tree. + /// + private static bool IsEnabledForTree(Compilation compilation, SyntaxTree? tree, CancellationToken cancellationToken) + { + SyntaxTreeOptionsProvider? optionsProvider = compilation.Options.SyntaxTreeOptionsProvider; + return tree is not null && + optionsProvider is not null && + optionsProvider.TryGetDiagnosticValue(tree, DiagnosticIds.RequireMultiThreadableTask, cancellationToken, out ReportDiagnostic severity) && + IsEnabled(severity); + } + + /// + /// Roslyn applies the configured severity to what is reported; this only decides whether the rule was opted + /// into at all, so any severity other than "none" and "default" counts. + /// + private static bool IsEnabled(ReportDiagnostic severity) => + severity is not ReportDiagnostic.Suppress and not ReportDiagnostic.Default; + } +} diff --git a/src/TaskAnalyzer/RequireMultiThreadableTaskCodeFixProvider.cs b/src/TaskAnalyzer/RequireMultiThreadableTaskCodeFixProvider.cs new file mode 100644 index 00000000000..24f6585a471 --- /dev/null +++ b/src/TaskAnalyzer/RequireMultiThreadableTaskCodeFixProvider.cs @@ -0,0 +1,159 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +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.Simplification; + +namespace Microsoft.Build.TaskAuthoring.Analyzer +{ + /// + /// Code fix for MSBuildTask0015: declares multithreading support on a concrete task type by applying + /// [MSBuildMultiThreadableTask], implementing IMultiThreadableTask, and adding the + /// TaskEnvironment property the engine injects. The remaining rules then report whatever is actually + /// unsafe in the task body. + /// + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(RequireMultiThreadableTaskCodeFixProvider))] + [Shared] + public sealed class RequireMultiThreadableTaskCodeFixProvider : CodeFixProvider + { + private const string EquivalenceKey = "DeclareMultiThreadingSupport"; + private const string TaskEnvironmentPropertyName = "TaskEnvironment"; + private const string FallbackPropertyName = "Fallback"; + + public override ImmutableArray FixableDiagnosticIds => + ImmutableArray.Create(DiagnosticIds.RequireMultiThreadableTask); + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) + { + return; + } + + foreach (var diagnostic in context.Diagnostics) + { + var classDeclaration = root.FindNode(diagnostic.Location.SourceSpan) + .FirstAncestorOrSelf(); + if (classDeclaration is null) + { + continue; + } + + context.RegisterCodeFix( + CodeAction.Create( + title: "Declare multithreading support", + createChangedDocument: ct => DeclareMultiThreadingSupportAsync(context.Document, classDeclaration, ct), + equivalenceKey: EquivalenceKey), + diagnostic); + } + } + + private static async Task DeclareMultiThreadingSupportAsync( + Document document, ClassDeclarationSyntax classDeclaration, CancellationToken cancellationToken) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + var compilation = editor.SemanticModel.Compilation; + var taskType = editor.SemanticModel.GetDeclaredSymbol(classDeclaration, cancellationToken); + + var multiThreadableTaskType = compilation.GetTypeByMetadataName(WellKnownTypeNames.IMultiThreadableTaskFullName); + var taskEnvironmentType = compilation.GetTypeByMetadataName(WellKnownTypeNames.TaskEnvironmentFullName); + + // A task can also opt in with the attribute alone, which is what the engine's routing looks at, so the + // interface and the property are added only when doing so is guaranteed to keep the task compiling: + // when the framework types are available, the task does not already implement the interface, and it does + // not already declare a conflicting TaskEnvironment member. + if (taskType is not null && + multiThreadableTaskType is not null && + taskEnvironmentType is not null && + !SharedAnalyzerHelpers.ImplementsInterface(taskType, multiThreadableTaskType)) + { + IPropertySymbol? existingProperty = SharedAnalyzerHelpers.GetPropertiesIncludingBaseTypes(taskType) + .FirstOrDefault(property => string.Equals(property.Name, TaskEnvironmentPropertyName, StringComparison.Ordinal)); + + if (existingProperty is null) + { + editor.InsertMembers(classDeclaration, 0, [CreateTaskEnvironmentProperty(editor.Generator, taskEnvironmentType)]); + AddMultiThreadableTaskInterface(editor, classDeclaration, multiThreadableTaskType); + } + else if (CanImplementTaskEnvironmentProperty(existingProperty, taskEnvironmentType)) + { + AddMultiThreadableTaskInterface(editor, classDeclaration, multiThreadableTaskType); + } + } + + editor.AddAttribute(classDeclaration, CreateMultiThreadableTaskAttribute(editor.Generator)); + + return editor.GetChangedDocument(); + } + + /// + /// Returns true when an existing TaskEnvironment property already satisfies + /// IMultiThreadableTask, so declaring the interface does not break the build. Both accessors have to + /// be public in their own right: a public TaskEnvironment TaskEnvironment { get; private set; } does + /// not implement the interface member. + /// + private static bool CanImplementTaskEnvironmentProperty(IPropertySymbol property, INamedTypeSymbol taskEnvironmentType) => + !property.IsStatic && + property.DeclaredAccessibility == Accessibility.Public && + property.GetMethod is { DeclaredAccessibility: Accessibility.Public } && + property.SetMethod is { DeclaredAccessibility: Accessibility.Public } && + SymbolEqualityComparer.Default.Equals(property.Type, taskEnvironmentType); + + private static void AddMultiThreadableTaskInterface( + DocumentEditor editor, ClassDeclarationSyntax classDeclaration, INamedTypeSymbol multiThreadableTaskType) => + editor.AddInterfaceType( + classDeclaration, + editor.Generator.TypeExpression(multiThreadableTaskType).WithAdditionalAnnotations(Simplifier.Annotation)); + + /// + /// Builds public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;. The + /// initializer keeps the property usable when the task is instantiated outside the engine, and avoids + /// introducing a nullable warning; it is omitted when the referenced framework has no Fallback. + /// + private static SyntaxNode CreateTaskEnvironmentProperty(SyntaxGenerator generator, INamedTypeSymbol taskEnvironmentType) + { + var typeExpression = generator.TypeExpression(taskEnvironmentType).WithAdditionalAnnotations(Simplifier.Annotation); + var property = generator.PropertyDeclaration( + TaskEnvironmentPropertyName, + typeExpression, + Accessibility.Public); + + if (property is not PropertyDeclarationSyntax propertyDeclaration) + { + return property; + } + + ExpressionSyntax initializerValue = taskEnvironmentType + .GetMembers(FallbackPropertyName) + .Any(member => member is IPropertySymbol { IsStatic: true, DeclaredAccessibility: Accessibility.Public }) + ? SyntaxFactory.ParseExpression($"global::{WellKnownTypeNames.TaskEnvironmentFullName}.{FallbackPropertyName}") + .WithAdditionalAnnotations(Simplifier.Annotation) + : SyntaxFactory.PostfixUnaryExpression( + SyntaxKind.SuppressNullableWarningExpression, + SyntaxFactory.LiteralExpression(SyntaxKind.NullLiteralExpression)); + + return propertyDeclaration + .WithInitializer(SyntaxFactory.EqualsValueClause(initializerValue)) + .WithSemicolonToken(SyntaxFactory.Token(SyntaxKind.SemicolonToken)); + } + + private static SyntaxNode CreateMultiThreadableTaskAttribute(SyntaxGenerator generator) => + generator.Attribute( + SyntaxFactory.ParseName("global::" + WellKnownTypeNames.MultiThreadableTaskAttributeFullName) + .WithAdditionalAnnotations(Simplifier.Annotation)); + } +} diff --git a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs index 3c8f9643cb1..9c9a83a30a1 100644 --- a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs +++ b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs @@ -18,26 +18,46 @@ internal static class SharedAnalyzerHelpers { /// /// The .editorconfig key controlling analysis scope. - /// Values: "all" (default) | "multithreadable_only" + /// Values: "all" (default) | "multithreadable_only" | "require_multithreadable" /// internal const string ScopeOptionKey = "msbuild_task_analyzer.scope"; internal const string ScopeAll = "all"; internal const string ScopeMultiThreadableOnly = "multithreadable_only"; + /// + /// Analyze every task type, and additionally require each concrete task type to declare + /// multithreading support (MSBuildTask0015). Repositories that have finished migrating their + /// tasks set this so a newly added task cannot silently regress the migration. + /// + internal const string ScopeRequireMultiThreadable = "require_multithreadable"; + /// /// Reads the scope option from the analyzer config options provider. /// Returns true if all tasks should be analyzed; false if only multithreadable tasks. /// - internal static bool ReadAnalyzeAllTasksOption(AnalyzerConfigOptionsProvider optionsProvider) + internal static bool ReadAnalyzeAllTasksOption(AnalyzerConfigOptionsProvider optionsProvider) => + !string.Equals(ReadScopeOption(optionsProvider), ScopeMultiThreadableOnly, StringComparison.OrdinalIgnoreCase); + + /// + /// Returns true when the scope option requires every concrete task type to declare multithreading support. + /// + internal static bool ReadRequireMultiThreadableOption(AnalyzerConfigOptionsProvider optionsProvider) => + string.Equals(ReadScopeOption(optionsProvider), ScopeRequireMultiThreadable, StringComparison.OrdinalIgnoreCase); + + /// + /// Reads the raw scope option value, or null when it is not configured. + /// + private static string? ReadScopeOption(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 scopeValue; } - return true; // default: analyze all tasks + return null; } + /// /// Represents a resolved banned API entry for O(1) lookup during analysis. /// @@ -389,6 +409,39 @@ internal static bool ImplementsInterface(INamedTypeSymbol type, INamedTypeSymbol return false; } + /// + /// Returns true when the type directly carries Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute. + /// + /// The attribute is matched by namespace and name rather than by symbol identity, because that is what + /// the engine does: TaskRouter.HasMultiThreadableTaskAttribute compares + /// attr.GetType().FullName and ignores the defining assembly so that a task can be marked with a + /// copy of the attribute declared in its own assembly -- the shim a repository uses to stay buildable + /// against an MSBuild that predates the attribute. Symbol identity would disagree with routing for + /// exactly those tasks, and returns null outright once + /// the shim and Microsoft.Build.Framework both contribute the name, which is the normal state during + /// such a migration. + /// + /// + /// returns only directly applied attributes, matching the + /// attribute's Inherited = false semantics and TaskRouter's inherit: false lookup. + /// + /// + internal static bool HasMultiThreadableTaskAttribute(INamedTypeSymbol type) + { + foreach (AttributeData attribute in type.GetAttributes()) + { + INamedTypeSymbol? attributeClass = attribute.AttributeClass; + if (attributeClass is not null && + string.Equals(attributeClass.Name, WellKnownTypeNames.MultiThreadableTaskAttributeName, StringComparison.Ordinal) && + string.Equals(attributeClass.ContainingNamespace?.ToDisplayString(), WellKnownTypeNames.FrameworkNamespace, StringComparison.Ordinal)) + { + return true; + } + } + + return false; + } + /// /// Enumerates the properties declared on and all of its base types, /// most-derived first. A property hidden or overridden in a more derived type is yielded only diff --git a/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs b/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs index 9dc9d26aa99..f4bb593477d 100644 --- a/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs +++ b/src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs @@ -54,7 +54,6 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte bool analyzeAllTasks = SharedAnalyzerHelpers.ReadAnalyzeAllTasksOption(compilationContext.Options.AnalyzerConfigOptionsProvider); var iMultiThreadableTaskType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.IMultiThreadableTaskFullName); - var multiThreadableTaskAttributeType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MultiThreadableTaskAttributeFullName); var analyzedAttributeType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.AnalyzedAttributeFullName); var taskEnvironmentType = compilationContext.Compilation.GetTypeByMetadataName(WellKnownTypeNames.TaskEnvironmentFullName); @@ -85,7 +84,7 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte { AnalyzeTransitiveViolations(endCtx, callGraph, directViolations, iTaskType, bannedApiLookup, filePathTypes, taskEnvironmentType, absolutePathType, iTaskItemType, consoleType, - analyzeAllTasks, iMultiThreadableTaskType, multiThreadableTaskAttributeType, analyzedAttributeType); + analyzeAllTasks, iMultiThreadableTaskType, analyzedAttributeType); }); } @@ -231,7 +230,6 @@ private static void AnalyzeTransitiveViolations( INamedTypeSymbol? consoleType, bool analyzeAllTasks, INamedTypeSymbol? iMultiThreadableTaskType, - INamedTypeSymbol? multiThreadableTaskAttributeType, INamedTypeSymbol? analyzedAttributeType) { // Find all task types in the compilation @@ -248,7 +246,7 @@ private static void AnalyzeTransitiveViolations( { 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))) || + SharedAnalyzerHelpers.HasMultiThreadableTaskAttribute(t) || (analyzedAttributeType is not null && t.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, analyzedAttributeType)))).ToList(); if (taskTypes.Count == 0) diff --git a/src/TaskAnalyzer/WellKnownTypeNames.cs b/src/TaskAnalyzer/WellKnownTypeNames.cs index da43da6dad5..a7ba5f6c22a 100644 --- a/src/TaskAnalyzer/WellKnownTypeNames.cs +++ b/src/TaskAnalyzer/WellKnownTypeNames.cs @@ -19,6 +19,8 @@ internal static class WellKnownTypeNames internal const string RequiredAttributeFullName = "Microsoft.Build.Framework.RequiredAttribute"; internal const string AnalyzedAttributeFullName = "Microsoft.Build.Framework.MSBuildMultiThreadableTaskAnalyzedAttribute"; internal const string MultiThreadableTaskAttributeFullName = "Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute"; + internal const string FrameworkNamespace = "Microsoft.Build.Framework"; + internal const string MultiThreadableTaskAttributeName = "MSBuildMultiThreadableTaskAttribute"; internal const string ConsoleFullName = "System.Console"; internal const string FileSystemInfoFullName = "System.IO.FileSystemInfo"; internal const string FileInfoFullName = "System.IO.FileInfo";