diff --git a/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs b/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs index 42d9789a056..add7f0375a3 100644 --- a/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs +++ b/src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs @@ -476,6 +476,140 @@ public override bool Execute() // Safe wrapper recognition: Path.GetDirectoryName, Path.Combine, Path.GetFullPath // ═══════════════════════════════════════════════════════════════════════ + [Theory] + [InlineData("Path.GetDirectoryName(TargetFile)")] + [InlineData("Path.GetPathRoot(TargetFile)")] + [InlineData("System.IO.Path.GetDirectoryName(path: TargetFile)")] + [InlineData("IOPath.GetPathRoot(TargetFile)")] + [InlineData("(GetDirectoryName(TargetFile))!")] + public async Task InvertedPathExtraction_ProducesDiagnostic(string expression) + { + var source = $$""" + using System.IO; + using IOPath = System.IO.Path; + using static System.IO.Path; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public string TargetFile { get; set; } = "list.xml"; + public override bool Execute() + { + Directory.CreateDirectory(TaskEnvironment.GetAbsolutePath({{expression}})); + return true; + } + } + """; + var diags = await GetDiagnosticsAsync(source); + + var diagnostic = diags.ShouldHaveSingleItem(); + diagnostic.Id.ShouldBe(DiagnosticIds.ResolvePathBeforeExtraction); + source.Substring(diagnostic.Location.SourceSpan.Start, diagnostic.Location.SourceSpan.Length) + .ShouldBe($"TaskEnvironment.GetAbsolutePath({expression})"); + } + + [Fact] + public async Task InvertedPathExtraction_SystemIOPathLookalike_NoDiagnostic() + { + var diags = await GetDiagnosticsAsync(""" + using Microsoft.Build.Framework; + namespace System.IO + { + internal static class Path + { + public static string GetDirectoryName(string path) => path; + } + } + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + var dir = TaskEnvironment.GetAbsolutePath(System.IO.Path.GetDirectoryName("list.xml")); + return true; + } + } + """); + + diags.ShouldNotContain(d => d.Id == DiagnosticIds.ResolvePathBeforeExtraction); + } + + [Theory] + [InlineData("GetDirectoryName")] + [InlineData("GetPathRoot")] + public async Task PathExtraction_AfterResolvingPath_NoDiagnostic(string method) + { + var diags = await GetDiagnosticsAsync($$""" + using System.IO; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + string dir = Path.{{method}}(TaskEnvironment.GetAbsolutePath("list.xml")); + Directory.CreateDirectory(dir); + return true; + } + } + """); + + diags.ShouldBeEmpty(); + } + + [Theory] + [InlineData("all", "", 1)] + [InlineData("multithreadable_only", "", 0)] + [InlineData("multithreadable_only", ", IMultiThreadableTask", 1)] + public async Task InvertedPathExtraction_RespectsScope(string scope, string taskInterface, int expectedCount) + { + var diags = await GetDiagnosticsWithScopeAsync($$""" + using System.IO; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task{{taskInterface}} + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + var dir = TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName("list.xml")); + return true; + } + } + """, scope); + + diags.Length.ShouldBe(expectedCount); + diags.ShouldAllBe(d => d.Id == DiagnosticIds.ResolvePathBeforeExtraction); + } + + [Fact] + public async Task PathExtraction_UnrelatedMethods_NoDiagnostic() + { + var diags = await GetDiagnosticsAsync(""" + using System.IO; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + var a = TaskEnvironment.GetAbsolutePath(OtherPath.GetDirectoryName("list.xml")); + var b = TaskEnvironment.GetAbsolutePath(OtherPath.GetPathRoot("list.xml")); + var c = GetAbsolutePath(Path.GetDirectoryName("list.xml")); + var d = TaskEnvironment.GetAbsolutePath(Path.GetFileName("dir/list.xml")); + return true; + } + private static string GetAbsolutePath(string path) => path; + } + public static class OtherPath + { + public static string GetDirectoryName(string path) => path; + public static string GetPathRoot(string path) => path; + } + """); + + diags.ShouldBeEmpty(); + } + [Fact] public async Task DirectoryCreate_WithGetDirectoryNameOfAbsolutePath_NoDiagnostic() { diff --git a/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs b/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs index a84052d36ab..c58ad3589bc 100644 --- a/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs +++ b/src/TaskAnalyzer.Tests/MultiThreadableTaskCodeFixProviderTests.cs @@ -3,6 +3,7 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Testing; using Xunit; @@ -43,6 +44,7 @@ private static CSharpCodeFixTest new DiagnosticResult(DiagnosticDescriptors.FilePathRequiresAbsolute), DiagnosticIds.PotentialIssue => new DiagnosticResult(DiagnosticDescriptors.PotentialIssue), DiagnosticIds.TransitiveUnsafeCall => new DiagnosticResult(DiagnosticDescriptors.TransitiveUnsafeCall), + DiagnosticIds.ResolvePathBeforeExtraction => new DiagnosticResult(DiagnosticDescriptors.ResolvePathBeforeExtraction), _ => new DiagnosticResult(id, DiagnosticSeverity.Warning), }; @@ -216,6 +218,205 @@ public override bool Execute() .WithArguments("File.Exists(string?)", "wrap path argument with TaskEnvironment.GetAbsolutePath()")).RunAsync(); } + [Theory] + [InlineData("Path.GetDirectoryName(TargetFile)", "Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile))")] + [InlineData("Path.GetPathRoot(TargetFile)", "Path.GetPathRoot(TaskEnvironment.GetAbsolutePath(TargetFile))")] + [InlineData("System.IO.Path.GetDirectoryName(TargetFile)", "System.IO.Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile))")] + [InlineData("IOPath.GetPathRoot(TargetFile)", "IOPath.GetPathRoot(TaskEnvironment.GetAbsolutePath(TargetFile))")] + [InlineData("GetDirectoryName(TargetFile)", "GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile))")] + [InlineData("(Path.GetDirectoryName(path: TargetFile))!", "(Path.GetDirectoryName(path: TaskEnvironment.GetAbsolutePath(TargetFile)))!")] + [InlineData("Path.GetDirectoryName(/* file */ TargetFile)", "Path.GetDirectoryName(/* file */ TaskEnvironment.GetAbsolutePath(TargetFile))")] + [InlineData("Path.GetDirectoryName(Path.GetDirectoryName(TargetFile))", "Path.GetDirectoryName(Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile)))")] + public async Task Fix_PathExtraction_WrapsOriginalPath(string expression, string fixedExpression) + { + var source = """ + using System.IO; + using IOPath = System.IO.Path; + using static System.IO.Path; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public string TargetFile { get; set; } = "list.xml"; + public override bool Execute() + { + REPLACE; + return true; + } + } + """; + + await CreateFixTest( + source.Replace("REPLACE", "{|#0:Directory.CreateDirectory(path: " + expression + ")|}"), + source.Replace("REPLACE", "Directory.CreateDirectory(path: " + fixedExpression + ")"), + Diag(DiagnosticIds.FilePathRequiresAbsolute).WithLocation(0) + .WithArguments("Directory.CreateDirectory(string)", "wrap path argument with TaskEnvironment.GetAbsolutePath()")).RunAsync(); + } + + [Theory] + [InlineData("Path.GetDirectoryName(TargetFile)", "Path.GetDirectoryName(this.TaskEnvironment.GetAbsolutePath(path: TargetFile))", "GetDirectoryName")] + [InlineData("IOPath.GetPathRoot(path: TargetFile)", "IOPath.GetPathRoot(path: this.TaskEnvironment.GetAbsolutePath(path: TargetFile))", "GetPathRoot")] + [InlineData("(GetDirectoryName(TargetFile))!", "(GetDirectoryName(this.TaskEnvironment.GetAbsolutePath(path: TargetFile)))!", "GetDirectoryName")] + [InlineData("Path.GetDirectoryName(/* file */ TargetFile)", "Path.GetDirectoryName(/* file */ this.TaskEnvironment.GetAbsolutePath(path: TargetFile))", "GetDirectoryName")] + [InlineData("Path.GetDirectoryName(Path.GetDirectoryName(TargetFile))", "Path.GetDirectoryName(Path.GetDirectoryName(this.TaskEnvironment.GetAbsolutePath(path: TargetFile)))", "GetDirectoryName")] + public async Task Fix_InvertedPathExtraction_SwapsCalls(string expression, string fixedExpression, string method) + { + var source = """ + using System.IO; + using IOPath = System.IO.Path; + using static System.IO.Path; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public string TargetFile { get; set; } = "list.xml"; + public override bool Execute() + { + Directory.CreateDirectory(REPLACE); + return true; + } + } + """; + + await CreateFixTest( + source.Replace("REPLACE", "{|#0:this.TaskEnvironment.GetAbsolutePath(path: " + expression + ")|}"), + source.Replace("REPLACE", fixedExpression), + Diag(DiagnosticIds.ResolvePathBeforeExtraction).WithLocation(0).WithArguments(method)).RunAsync(); + } + + [Theory] + [InlineData("GetDirectoryName")] + [InlineData("GetPathRoot")] + public async Task Fix_PathExtraction_LookalikeMethod_WrapsWholeExpression(string method) + { + var source = $$""" + using System.IO; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + REPLACE; + return true; + } + private static class Path + { + public static string {{method}}(string path) => path; + } + } + """; + var expression = $"Path.{method}(\"list.xml\")"; + + await CreateFixTest( + source.Replace("REPLACE", "{|#0:Directory.CreateDirectory(" + expression + ")|}"), + source.Replace("REPLACE", "Directory.CreateDirectory(TaskEnvironment.GetAbsolutePath(" + expression + "))"), + Diag(DiagnosticIds.FilePathRequiresAbsolute).WithLocation(0) + .WithArguments("Directory.CreateDirectory(string)", "wrap path argument with TaskEnvironment.GetAbsolutePath()")).RunAsync(); + } + + [Fact] + public async Task Fix_InvertedPathExtraction_AbsolutePathConsumers_NoFixOffered() + { + await CreateNoFixTest( + """ + using System.IO; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() + { + AbsolutePath dir = {|#0:TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName("list.xml"))|}; + var root = {|#1:TaskEnvironment.GetAbsolutePath(Path.GetPathRoot("list.xml"))|}; + string value = root.Value; + return true; + } + } + """, + Diag(DiagnosticIds.ResolvePathBeforeExtraction).WithLocation(0).WithArguments("GetDirectoryName"), + Diag(DiagnosticIds.ResolvePathBeforeExtraction).WithLocation(1).WithArguments("GetPathRoot")); + } + + [Fact] + public async Task Fix_InvertedPathExtraction_FixAllPreservesReceiver() + { + var source = """ + using System.IO; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task + { + public override bool Execute() => true; + private static string Resolve(TaskEnvironment environment, string file) + { + string dir = DIRECTORY; + return ROOT; + } + } + """; + await CreateFixTest( + source.Replace("DIRECTORY", "{|#0:environment.GetAbsolutePath(Path.GetDirectoryName(file))|}") + .Replace("ROOT", "{|#1:environment.GetAbsolutePath(Path.GetPathRoot(file))|}"), + source.Replace("DIRECTORY", "Path.GetDirectoryName(environment.GetAbsolutePath(file))") + .Replace("ROOT", "Path.GetPathRoot(environment.GetAbsolutePath(file))"), + Diag(DiagnosticIds.ResolvePathBeforeExtraction).WithLocation(0).WithArguments("GetDirectoryName"), + Diag(DiagnosticIds.ResolvePathBeforeExtraction).WithLocation(1).WithArguments("GetPathRoot")).RunAsync(); + } + + [Theory] + [InlineData("GetDirectoryName", false, false)] + [InlineData("GetDirectoryName", true, false)] + [InlineData("GetPathRoot", false, false)] + [InlineData("GetPathRoot", true, false)] + [InlineData("GetDirectoryName", false, true)] + [InlineData("GetDirectoryName", true, true)] + [InlineData("GetPathRoot", false, true)] + [InlineData("GetPathRoot", true, true)] + public async Task Fix_PathExtraction_NullableInput(string method, bool inverted, bool suppressInput) + { + var source = """ + using System.IO; + using Microsoft.Build.Framework; + public class MyTask : Microsoft.Build.Utilities.Task, IMultiThreadableTask + { + public TaskEnvironment TaskEnvironment { get; set; } = null!; + public string? TargetFile { get; set; } + public override bool Execute() + { + REPLACE; + return true; + } + } + """; + var input = suppressInput ? "TargetFile!" : "TargetFile"; + var extraction = $"Path.{method}({input})!"; + var original = inverted + ? "Directory.CreateDirectory({|#0:TaskEnvironment.GetAbsolutePath(" + extraction + ")|})" + : "{|#0:Directory.CreateDirectory(" + extraction + ")|}"; + var diagnostic = inverted + ? Diag(DiagnosticIds.ResolvePathBeforeExtraction).WithLocation(0).WithArguments(method) + : Diag(DiagnosticIds.FilePathRequiresAbsolute).WithLocation(0) + .WithArguments("Directory.CreateDirectory(string)", "wrap path argument with TaskEnvironment.GetAbsolutePath()"); + var fixedStatement = suppressInput + ? $"Directory.CreateDirectory(Path.{method}(TaskEnvironment.GetAbsolutePath({input}))!)" + : original; + var test = CreateFixTest(source.Replace("REPLACE", original), source.Replace("REPLACE", fixedStatement), diagnostic); + if (!suppressInput) + { + test.FixedState.ExpectedDiagnostics.Add(diagnostic); + } + + test.CompilerDiagnostics = CompilerDiagnostics.Warnings; + test.SolutionTransforms.Add((solution, projectId) => + { + var project = solution.GetProject(projectId)!; + return solution.WithProjectCompilationOptions(projectId, + ((CSharpCompilationOptions)project.CompilationOptions!).WithNullableContextOptions(NullableContextOptions.Enable)) + .WithProjectParseOptions(projectId, project.ParseOptions!.WithDocumentationMode(DocumentationMode.Parse)); + }); + await test.RunAsync(); + } + [Fact] public async Task Fix_NewFileInfo_WrapsWithGetAbsolutePath() { diff --git a/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md b/src/TaskAnalyzer/AnalyzerReleases.Unshipped.md index 1a3d3ba0137..0a099c7fc40 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 | Resolve the original path before extracting its directory or root (code fix available) diff --git a/src/TaskAnalyzer/DiagnosticDescriptors.cs b/src/TaskAnalyzer/DiagnosticDescriptors.cs index 2698e65c45a..dbc2c65ae89 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 ResolvePathBeforeExtraction = new( + id: DiagnosticIds.ResolvePathBeforeExtraction, + title: "Resolve the path before extracting its directory or root", + messageFormat: "'Path.{0}' can return an empty or null path that GetAbsolutePath rejects; use Path.{0}(TaskEnvironment.GetAbsolutePath(...)) instead", + category: "MSBuild.TaskAuthoring", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "Resolve the original path through TaskEnvironment.GetAbsolutePath before calling Path.GetDirectoryName or Path.GetPathRoot, rather than resolving the extracted directory or root."); + public static ImmutableArray All { get; } = ImmutableArray.Create( CriticalError, TaskEnvironmentRequired, @@ -153,6 +162,7 @@ internal static class DiagnosticDescriptors PreferTaskEnvironmentConstructorInjection, TaskEnvironmentNeverAssigned, MissingMultiThreadableTaskAttribute, - MultiThreadableTaskAttributeHasNoEffect); + MultiThreadableTaskAttributeHasNoEffect, + ResolvePathBeforeExtraction); } } diff --git a/src/TaskAnalyzer/DiagnosticIds.cs b/src/TaskAnalyzer/DiagnosticIds.cs index ca688e318d9..30a8acbc9be 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"; + + /// Resolve the original path before extracting its directory or root. + public const string ResolvePathBeforeExtraction = "MSBuildTask0015"; } } diff --git a/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs b/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs index bcdddfdc825..fbfebcc037e 100644 --- a/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs +++ b/src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs @@ -18,7 +18,7 @@ namespace Microsoft.Build.TaskAuthoring.Analyzer /// /// Scope (controlled by .editorconfig option "msbuild_task_analyzer.scope"): /// - "all" (default): All rules fire on ALL ITask implementations - /// - "multithreadable_only": MSBuildTask0002, 0003 fire only on IMultiThreadableTask or [MSBuildMultiThreadableTask] + /// - "multithreadable_only": MSBuildTask0002, 0003, 0015 fire only on IMultiThreadableTask or [MSBuildMultiThreadableTask] /// (MSBuildTask0001 and MSBuildTask0004 always fire on all tasks regardless) /// /// Per review feedback from @rainersigwald: @@ -97,7 +97,7 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte // Helper classes with the attribute or tasks with [MSBuildMultiThreadableTask] are treated as IMultiThreadableTask bool analyzeAsMultiThreadable = isMultiThreadableTask || hasAnalyzedAttribute || hasMultiThreadableAttribute; - // When scope is "multithreadable_only", only analyze MSBuildTask0002/0003 for multithreadable tasks + // When scope is "multithreadable_only", only analyze environment rules for multithreadable tasks bool reportEnvironmentRules = analyzeAllTasks || analyzeAsMultiThreadable; // Register operation-level analysis within this type @@ -202,6 +202,20 @@ private static void AnalyzeOperation( } } + // MSBuildTask0015 supersedes the MSBuildTask0003 check below. The flagged call is itself a + // GetAbsolutePath resolution, so 0003 has nothing to add, and reporting both would produce + // two diagnostics — offering conflicting fixes — for a single defect. + if (reportEnvironmentRules && + context.Operation is IInvocationOperation pathInvocation && + GetInvertedPathExtraction(pathInvocation, taskEnvironmentType) is { } extraction) + { + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.ResolvePathBeforeExtraction, + context.Operation.Syntax.GetLocation(), + extraction.TargetMethod.Name)); + return; + } + // Check file path APIs (MSBuildTask0003) - gated by scope setting if (reportEnvironmentRules && !arguments.IsDefaultOrEmpty) { diff --git a/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs b/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs index 84b27963a67..cdc66ca0128 100644 --- a/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs +++ b/src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs @@ -22,13 +22,14 @@ namespace Microsoft.Build.TaskAuthoring.Analyzer /// Fixes: /// - MSBuildTask0002: Replaces banned APIs with TaskEnvironment equivalents /// - MSBuildTask0003: Wraps path arguments with TaskEnvironment.GetAbsolutePath() + /// - MSBuildTask0015: Resolves paths before extracting their directory or root /// [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(MultiThreadableTaskCodeFixProvider))] [Shared] public sealed class MultiThreadableTaskCodeFixProvider : CodeFixProvider { public override ImmutableArray FixableDiagnosticIds => - ImmutableArray.Create(DiagnosticIds.TaskEnvironmentRequired, DiagnosticIds.FilePathRequiresAbsolute); + ImmutableArray.Create(DiagnosticIds.TaskEnvironmentRequired, DiagnosticIds.FilePathRequiresAbsolute, DiagnosticIds.ResolvePathBeforeExtraction); public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; @@ -62,6 +63,25 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) { RegisterTaskEnvironmentFix(context, semanticModel, node, diagnostic); } + else if (diagnostic.Id == DiagnosticIds.ResolvePathBeforeExtraction && + node is InvocationExpressionSyntax invocationSyntax && + semanticModel.GetOperation(node) is IInvocationOperation invocation && + GetInvertedPathExtraction(invocation, semanticModel.Compilation.GetTypeByMetadataName(WellKnownTypeNames.TaskEnvironmentFullName)) is not null && + GetPathToResolve(invocation.Arguments[0]).Syntax.FirstAncestorOrSelf() is { } innerArgument) + { + // The swap returns string, not AbsolutePath, and GetAbsolutePath requires a non-null + // input even though extraction accepts null. Don't introduce compiler diagnostics. + if (invocation.Parent is IConversionOperation { Type.SpecialType: SpecialType.System_String } && + semanticModel.GetTypeInfo(innerArgument.Expression).Nullability.FlowState != NullableFlowState.MaybeNull) + { + context.RegisterCodeFix( + CodeAction.Create( + title: "Resolve path before extracting its directory or root", + createChangedDocument: ct => SwapPathExtractionAsync(context.Document, invocationSyntax, innerArgument, ct), + equivalenceKey: "ResolvePathBeforeExtraction"), + diagnostic); + } + } } } @@ -93,6 +113,13 @@ private static void RegisterFilePathFix(CodeFixContext context, SemanticModel se return; } + // Moving inside an extraction must not introduce a nullable argument warning. + if (!argumentList.Arguments.Contains(targetArg) && + semanticModel.GetTypeInfo(targetArg.Expression).Nullability.FlowState == NullableFlowState.MaybeNull) + { + return; + } + context.RegisterCodeFix( CodeAction.Create( title: "Wrap with TaskEnvironment.GetAbsolutePath()", @@ -134,12 +161,13 @@ private static void RegisterFilePathFix(CodeFixContext context, SemanticModel se } // Skip arguments that aren't written in this call's argument list (e.g. defaulted - // optional parameters, whose syntax is the call itself). - if (argument.Syntax is ArgumentSyntax argumentSyntax && + // optional parameters, whose syntax is the call itself). Null-forgiving syntax can + // make the operation point at the expression rather than its ArgumentSyntax. + if (argument.Syntax.FirstAncestorOrSelf() is { } argumentSyntax && argumentList.Arguments.Contains(argumentSyntax) && !IsWrappedSafely(argument.Value, taskEnvironmentType, absolutePathType, iTaskItemType)) { - return argumentSyntax; + return GetPathToResolve(argument).Syntax.FirstAncestorOrSelf(); } } @@ -157,6 +185,17 @@ private static void RegisterFilePathFix(CodeFixContext context, SemanticModel se return null; } + private static IArgumentOperation GetPathToResolve(IArgumentOperation argument) + { + // Resolve the original path before extracting a directory/root, including nested extractions. + while (argument.Value is IInvocationOperation extraction && IsPathExtraction(extraction)) + { + argument = extraction.Arguments[0]; + } + + return argument; + } + /// /// Determines whether a generated reference to the instance TaskEnvironment member would compile /// at : the enclosing type must actually expose such a member, and this @@ -375,6 +414,22 @@ private static async Task WrapArgumentWithGetAbsolutePathAsync( return editor.GetChangedDocument(); } + private static async Task SwapPathExtractionAsync( + Document document, InvocationExpressionSyntax invocation, ArgumentSyntax innerArgument, CancellationToken ct) + { + var editor = await DocumentEditor.CreateAsync(document, ct).ConfigureAwait(false); + var outerArgument = invocation.ArgumentList.Arguments[0]; + var resolvedArgument = outerArgument.WithExpression(innerArgument.Expression.WithoutTrivia()); + var resolvedPath = invocation.WithArgumentList( + invocation.ArgumentList.WithArguments(SyntaxFactory.SingletonSeparatedList(resolvedArgument))) + .WithoutTrivia(); + var replacement = outerArgument.Expression.ReplaceNode( + innerArgument.Expression, resolvedPath.WithTriviaFrom(innerArgument.Expression)); + editor.ReplaceNode(invocation, replacement.WithTriviaFrom(invocation)); + + return editor.GetChangedDocument(); + } + private static async Task ReplaceInvocationTargetAsync( Document document, InvocationExpressionSyntax invocation, string newTypeName, string newMethodName, CancellationToken ct) diff --git a/src/TaskAnalyzer/README.md b/src/TaskAnalyzer/README.md index 471ebbed926..2d32b21863b 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 | All `ITask` implementations | Resolve the path before extracting its directory or root | ### MSBuildTask0001 — Critical: No Safe Alternative @@ -101,6 +102,10 @@ File.Exists(item.GetMetadataValue("FullPath")) // 5. Argument already typed as AbsolutePath void Helper(AbsolutePath p) => File.Exists(p); + +// 6. Extract the directory or root of an absolute path +Directory.CreateDirectory(Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(relativePath))) +Directory.Exists(Path.GetPathRoot(TaskEnvironment.GetAbsolutePath(relativePath))) ``` ### MSBuildTask0004 — Potential Issue (Review Required) @@ -432,30 +437,46 @@ 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 — Resolve Before Extracting the Directory or Root + +Resolve the original path before calling `Path.GetDirectoryName` or `Path.GetPathRoot`: + +```csharp +// Incorrect: GetDirectoryName("list.xml") returns "", which GetAbsolutePath rejects. +TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName(TargetFile)); + +// Correct: resolve "list.xml" against the project directory, then take its parent. +Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(TargetFile)); +``` + +The same rule applies to `Path.GetPathRoot`, which returns an empty string for a relative path without a root. Extraction can also return `null`, which `GetAbsolutePath` rejects. The diagnostic checks the actual `TaskEnvironment` and `System.IO.Path` methods, including aliases and `using static`, rather than matching method names alone. + +Like MSBuildTask0002/0003, this rule honors `msbuild_task_analyzer.scope`: `all` by default, or `multithreadable_only` to restrict it to multithreadable tasks and opted-in helpers. The code fix swaps the calls where the result is consumed as a string. Uses that retain the `AbsolutePath` type (including inferred `var` locals) require manual migration of their consumers, so no automatic swap is offered there. + ## Analysis Scope The analyzer determines what to check based on the type declaration: | Type | Rules Applied | |---|---| -| Any class implementing `ITask` | MSBuildTask0001–MSBuildTask0005, MSBuildTask0009–MSBuildTask0010 | +| Any class implementing `ITask` | MSBuildTask0001–MSBuildTask0005, MSBuildTask0009–MSBuildTask0010, MSBuildTask0015 | | 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 `IMultiThreadableTask` without the attribute | MSBuildTask0001–MSBuildTask0005, MSBuildTask0009–MSBuildTask0011, MSBuildTask0015 | +| Helper class with `[MSBuildMultiThreadableTaskAnalyzed]` attribute | MSBuildTask0001–MSBuildTask0005, MSBuildTask0015 | | Regular class (no task interface or attribute) | Not analyzed | | Class with `[MSBuildMultiThreadableTask]` that does not implement `ITask` | MSBuildTask0014 | | Abstract class with `[MSBuildMultiThreadableTask]` | MSBuildTask0014 | MSBuildTask0006–MSBuildTask0008 apply only when the `[MSBuildMultiThreadableTask]` attribute is applied **directly** to the task class. The attribute is `Inherited = false`, so a task that merely derives from a base class implementing `IMultiThreadableTask` (or carrying the attribute) has not itself opted into multithreaded support and is not subject to these three rules. Input properties are collected from the task class **and its base classes**, so an `ITaskItem`/`string` input declared on a shared base task is still analyzed. -The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classes into **direct** analysis by the `MultiThreadableTaskAnalyzer` (MSBuildTask0001–0004). Without it, only classes implementing `ITask` receive per-line diagnostics and code fixes for those rules. The **transitive** analyzer (MSBuildTask0005) already discovers helpers via call graph analysis and reports at the unsafe call site, but it offers no code fixes and only fires for helpers actually reachable from a task. Adding this attribute to a helper class gives you diagnostics and code fixes in the helper's source regardless of whether a task reaches it. +The `[MSBuildMultiThreadableTaskAnalyzed]` attribute allows opting helper classes into **direct** analysis by the `MultiThreadableTaskAnalyzer` (MSBuildTask0001–0004 and MSBuildTask0015). Without it, only classes implementing `ITask` receive per-line diagnostics and code fixes for those rules. The **transitive** analyzer (MSBuildTask0005) already discovers helpers via call graph analysis and reports at the unsafe call site, but it offers no code fixes and only fires for helpers actually reachable from a task. Adding this attribute to a helper class gives you diagnostics and code fixes in the helper's source regardless of whether a task reaches it. **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). ### Severity Levels - **MSBuildTask0001** is always **Error** — these APIs are never safe in any MSBuild task. -- **MSBuildTask0002–MSBuildTask0005, MSBuildTask0009, and MSBuildTask0010** report as **Warning**. +- **MSBuildTask0002–MSBuildTask0005, MSBuildTask0009, MSBuildTask0010, and MSBuildTask0015** report as **Warning**. - **MSBuildTask0006–MSBuildTask0008 and MSBuildTask0011** report as **Info** — these are modernization suggestions, not correctness issues. ## Code Fixes @@ -471,6 +492,7 @@ The analyzer ships with a code fix provider that offers automatic replacements: | MSBuildTask0002: `Environment.CurrentDirectory` | → `TaskEnvironment.ProjectDirectory` | | MSBuildTask0002: `Directory.GetCurrentDirectory()` | → `TaskEnvironment.ProjectDirectory` | | MSBuildTask0003: `File.Exists(relativePath)` | → `File.Exists(TaskEnvironment.GetAbsolutePath(relativePath))` | +| MSBuildTask0003: `Directory.CreateDirectory(Path.GetDirectoryName(x))` | → `Directory.CreateDirectory(Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(x)))` (also applies to `GetPathRoot`) | | MSBuildTask0006: `new AbsolutePath(InputPath)` | → Retype `InputPath` to `AbsolutePath` and replace conversion with direct property usage | | MSBuildTask0006: `new FileInfo(FilePath)` / `new DirectoryInfo(DirPath)` | → Retype property to `FileInfo`/`DirectoryInfo` and replace conversion with direct property usage | | MSBuildTask0006: `Path.GetFullPath(InputPath)` | → Retype `InputPath` to `AbsolutePath` and collapse the call to the property (one-shot for the 0002 shape) | @@ -479,9 +501,14 @@ 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 | +| MSBuildTask0015: `TaskEnvironment.GetAbsolutePath(Path.GetDirectoryName(x))` | → `Path.GetDirectoryName(TaskEnvironment.GetAbsolutePath(x))` (also applies to `GetPathRoot`; string consumers only) | 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. +If the path argument extracts a directory or root, the fixer wraps the original input inside `Path.GetDirectoryName`/`Path.GetPathRoot`, including nested extractions, rather than wrapping their possibly empty or null result. + +Both extraction fixes are withheld if the original input is maybe-null under nullable analysis: extraction accepts null, but `GetAbsolutePath` does not. Validate the input first (or explicitly null-forgive it if it is known to be non-null); suppressing nullability only on the extraction's result does not establish that the input is non-null. + Both the MSBuildTask0002 and MSBuildTask0003 fixers reference the instance `TaskEnvironment` member, so no fix is offered where that reference would not compile: where `this` is unavailable — a static method, static local function, or static lambda (CS0120), or an instance field or property initializer (CS0236) — or where the task type simply has no `TaskEnvironment` member (CS0103), which the default `all` scope allows since it analyzes every `ITask`. Making the enclosing member non-static, moving the initializer into `Execute()`, or implementing `IMultiThreadableTask` re-enables the fix. When bulk-applying with `dotnet format analyzers`, note that the tool derives the batch from the *first* reported diagnostic: if that occurrence is one of the ones above where no fix is offered, it logs `Unable to fix MSBuildTask0003…` and applies nothing. Resolve or suppress that first occurrence by hand, then re-run. @@ -590,7 +617,7 @@ Unit tests for all rules, safe patterns, edge cases, code fixes, and compiler di | File | Purpose | |---|---| | `MultiThreadableTaskAnalyzer.cs` | Core analyzer — `RegisterSymbolStartAction` scopes per type, `RegisterOperationAction` checks each API call | -| `MultiThreadableTaskCodeFixProvider.cs` | Code fixes for MSBuildTask0002 and MSBuildTask0003 | +| `MultiThreadableTaskCodeFixProvider.cs` | Code fixes for MSBuildTask0002, MSBuildTask0003, and MSBuildTask0015 | | `BannedApiDefinitions.cs` | ~50 banned API entries resolved via `DocumentationCommentId` for O(1) symbol lookup | | `SharedAnalyzerHelpers.cs` | Shared path safety analysis, banned API resolution, and interface checking helpers | | `DiagnosticDescriptors.cs` | Eight diagnostic descriptors in category `MSBuild.TaskAuthoring` | diff --git a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs index a09348e13b9..863692fb88a 100644 --- a/src/TaskAnalyzer/SharedAnalyzerHelpers.cs +++ b/src/TaskAnalyzer/SharedAnalyzerHelpers.cs @@ -147,9 +147,7 @@ operation is not IInvocationOperation invocation || return false; } - // Resolve Path from the intrinsic string's assembly, not a source or referenced lookalike. - var pathType = invocation.TargetMethod.ReturnType.ContainingAssembly?.GetTypeByMetadataName("System.IO.Path"); - return SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, pathType); + return IsSystemIOPath(invocation); } /// @@ -213,10 +211,8 @@ literal.ConstantValue.Value is string metadataName && } } - // Check: Path.GetDirectoryName(safe) — directory of an absolute path is absolute - if (invocation.TargetMethod.Name == "GetDirectoryName" && - invocation.TargetMethod.ContainingType?.ToDisplayString() == "System.IO.Path" && - invocation.Arguments.Length >= 1 && + // Check: Path.GetDirectoryName(safe) / GetPathRoot(safe) — preserve absolute paths + if (IsPathExtraction(invocation) && IsWrappedSafely(invocation.Arguments[0].Value, taskEnvironmentType, absolutePathType, iTaskItemType)) { return true; @@ -224,7 +220,7 @@ literal.ConstantValue.Value is string metadataName && // Check: Path.Combine(safe, ...) — result is absolute when first arg is absolute if (invocation.TargetMethod.Name == "Combine" && - invocation.TargetMethod.ContainingType?.ToDisplayString() == "System.IO.Path" && + IsSystemIOPath(invocation) && invocation.Arguments.Length >= 2 && IsWrappedSafely(invocation.Arguments[0].Value, taskEnvironmentType, absolutePathType, iTaskItemType)) { @@ -240,7 +236,7 @@ literal.ConstantValue.Value is string metadataName && } if (invocation.TargetMethod.Name == "GetFullPath" && - invocation.TargetMethod.ContainingType?.ToDisplayString() == "System.IO.Path" && + IsSystemIOPath(invocation) && invocation.Arguments.Length >= 1 && IsWrappedSafely(invocation.Arguments[0].Value, taskEnvironmentType, absolutePathType, iTaskItemType)) { @@ -290,6 +286,38 @@ operation.Type is not null && return false; } + /// + /// True when targets the intrinsic System.IO.Path. The type is + /// resolved from the assembly that defines rather than matched by display name, + /// so a source-declared or referenced System.IO.Path lookalike does not satisfy the check. + /// + internal static bool IsSystemIOPath(IInvocationOperation invocation) + { + INamedTypeSymbol? pathType = invocation.SemanticModel?.Compilation + .GetSpecialType(SpecialType.System_String).ContainingAssembly? + .GetTypeByMetadataName(WellKnownTypeNames.PathFullName); + + return pathType is not null && + SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, pathType); + } + + internal static bool IsPathExtraction(IInvocationOperation invocation) => + invocation.TargetMethod.Name is "GetDirectoryName" or "GetPathRoot" && + IsSystemIOPath(invocation) && + invocation.Arguments.Length == 1 && + invocation.Arguments[0].Parameter?.Type.SpecialType == SpecialType.System_String; + + internal static IInvocationOperation? GetInvertedPathExtraction( + IInvocationOperation invocation, INamedTypeSymbol? taskEnvironmentType) => + invocation.TargetMethod.Name == "GetAbsolutePath" && + taskEnvironmentType is not null && + SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, taskEnvironmentType) && + invocation.Arguments.Length == 1 && + invocation.Arguments[0].Value is IInvocationOperation extraction && + IsPathExtraction(extraction) + ? extraction + : null; + /// /// Checks if a type is AbsolutePath or Nullable<AbsolutePath>. /// diff --git a/src/TaskAnalyzer/WellKnownTypeNames.cs b/src/TaskAnalyzer/WellKnownTypeNames.cs index da43da6dad5..29db0a167b2 100644 --- a/src/TaskAnalyzer/WellKnownTypeNames.cs +++ b/src/TaskAnalyzer/WellKnownTypeNames.cs @@ -20,6 +20,7 @@ internal static class WellKnownTypeNames internal const string AnalyzedAttributeFullName = "Microsoft.Build.Framework.MSBuildMultiThreadableTaskAnalyzedAttribute"; internal const string MultiThreadableTaskAttributeFullName = "Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute"; internal const string ConsoleFullName = "System.Console"; + internal const string PathFullName = "System.IO.Path"; internal const string FileSystemInfoFullName = "System.IO.FileSystemInfo"; internal const string FileInfoFullName = "System.IO.FileInfo"; internal const string DirectoryInfoFullName = "System.IO.DirectoryInfo";