Add Roslyn analyzers (MSBuildTask0006/0007) to suggest typed task parameters - #13972
Conversation
f4d5fe0 to
6b6a7d7
Compare
a4d536e to
b62b5ae
Compare
7629eac to
67efb90
Compare
… as task parameters (#13971) ## Summary Adds support for using AbsolutePath, System.IO.FileInfo, System.IO.DirectoryInfo, and ITaskItem<T> (for path-like T) as MSBuild task input/output parameters, in addition to the existing string and ITaskItem types. ## Changes - **AbsolutePath**: A new value type that wraps an absolute path string, validated via TaskEnvironment.GetAbsolutePath - **FileInfo / DirectoryInfo**: Paths are validated as absolute before creating the FileInfo/DirectoryInfo instance - **`ITaskItem<T>`**: A new generic interface allowing typed access to item specs, with T restricted to AbsolutePath, FileInfo, or DirectoryInfo in this PR - **`TaskItem<T>`**: Implementation of ITaskItem<T> with mutable metadata backing - **ValueTypeParser**: Shared utility for parsing/formatting typed values (infrastructure for future PRs) - Task host serialization support for typed outputs - Full test coverage for all new parameter types ## Performance A BenchmarkDotNet benchmark (`TaskParameterBindingBenchmark`) was added that drives the real `TaskExecutionHost.SetTaskParameters` path to measure the engine's per-parameter binding overhead for the new types against the `string` and `ITaskItem` baselines. Steady-state full-run results (AMD Ryzen 7 5700X3D, BDN v0.13.12), with the `ITaskItem<T>` constructor-delegate caching applied. `Mean` is per single parameter bind; `Allocated` is managed bytes per bind; ratios are vs the `string` baseline. **net10.0 (.NET 10.0.8)** | Parameter type | Mean | Ratio | Allocated | Alloc ratio | | --- | ---: | ---: | ---: | ---: | | `string` (baseline) | 1.68 µs | 1.00x | 1.05 KB | 1.00x | | `ITaskItem` | 1.83 µs | 1.09x | 1.27 KB | 1.20x | | `AbsolutePath` | 1.91 µs | 1.13x | 1.88 KB | 1.78x | | `FileInfo` | 2.31 µs | 1.38x | 2.28 KB | 2.16x | | `DirectoryInfo` | 3.08 µs | 1.88x | 2.27 KB | 2.16x | | `ITaskItem<AbsolutePath>` | 6.21 µs | 2.67x | 2.90 KB | 2.75x | | `ITaskItem<FileInfo>` | 7.16 µs | 4.26x | 2.91 KB | 2.76x | | `ITaskItem<DirectoryInfo>` | 7.36 µs | 4.06x | 2.91 KB | 2.76x | **net472 (.NET Framework 4.7.2)** | Parameter type | Mean | Ratio | Allocated | Alloc ratio | | --- | ---: | ---: | ---: | ---: | | `string` (baseline) | 5.94 µs | 1.00x | 397 B | 1.00x | | `ITaskItem` | 7.12 µs | 1.24x | 593 B | 1.49x | | `AbsolutePath` | 6.15 µs | 1.05x | 1346 B | 3.39x | | `FileInfo` | 7.61 µs | 1.26x | 1526 B | 3.84x | | `DirectoryInfo` | 7.38 µs | 1.23x | 1487 B | 3.75x | | `ITaskItem<AbsolutePath>` | 12.33 µs | 2.06x | 1222 B | 3.08x | | `ITaskItem<FileInfo>` | 15.42 µs | 2.67x | 1403 B | 3.53x | | `ITaskItem<DirectoryInfo>` | 14.33 µs | 2.39x | 1363 B | 3.43x | Takeaways: - Scalar value/path conversions (`AbsolutePath`, `FileInfo`, `DirectoryInfo`) add only ~10–90% over a plain `string` bind — roughly a microsecond per parameter and ~0.8–1.2 KB / ~0.9–1.1 KB extra allocation, negligible against real task execution. - The `ITaskItem<T>` path was the expensive case because the engine wrapped each item via `typeof(TaskItem<>).MakeGenericType(...).GetConstructor(...).Invoke(...)` on every bind. This PR now **caches a compiled `Func<ITaskItem, ITaskItem>` constructor delegate per generic argument T**, so the reflection work happens once. On .NET Framework this cut `ITaskItem<T>` bind time by ~37–42% (e.g. ~24 µs → ~14–15 µs) and trimmed ~180 B/bind; on .NET 10 the JIT already optimized the reflection well, so the win there is primarily reduced allocations and Gen0 churn (≈3.1 KB → ≈2.9 KB). ## Stacked on - Base: main - Next: #13972 (migration analyzers) /cc @baronfel --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Veronika Ovsyannikova <150850103+OvesN@users.noreply.github.com>
6b6a7d7 to
8bf4e6c
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the Microsoft.Build.TaskAuthoring.Analyzer package with two new analyzers (MSBuildTask0006/0007) aimed at modernizing multithreaded MSBuild tasks by suggesting strongly-typed task parameters (e.g., AbsolutePath, FileInfo, DirectoryInfo, ITaskItem<T>) and providing corresponding code fixes.
Changes:
- Add PreferTypedParameterAnalyzer (MSBuildTask0006/0007) to detect manual path construction and
ItemSpecparsing patterns in multithreaded tasks. - Add PreferTypedParameterCodeFixProvider to retype task properties and rewrite recognized conversion sites.
- Update docs/release tracking + expand test stubs and add extensive analyzer/code-fix test coverage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TaskAnalyzer/WellKnownTypeNames.cs | Adds new framework type-name constants used by the new analyzer. |
| src/TaskAnalyzer/README.md | Documents MSBuildTask0006/0007 behavior, scope, and code-fix behavior. |
| src/TaskAnalyzer/PreferTypedParameterCodeFixProvider.cs | New fixer for retyping properties and rewriting conversion sites. |
| src/TaskAnalyzer/PreferTypedParameterAnalyzer.cs | New analyzer implementing MSBuildTask0006/0007 detection + suggestion logic. |
| src/TaskAnalyzer/DiagnosticIds.cs | Adds public IDs for MSBuildTask0006/0007. |
| src/TaskAnalyzer/DiagnosticDescriptors.cs | Adds descriptors for MSBuildTask0006/0007 and includes them in the descriptor list. |
| src/TaskAnalyzer/AnalyzerReleases.Unshipped.md | Tracks new analyzer rule IDs and metadata. |
| src/TaskAnalyzer.Tests/TestHelpers.cs | Extends framework stubs (AbsolutePath ctor, OutputAttribute, ITaskItem) and adds a helper to run the new analyzer. |
| src/TaskAnalyzer.Tests/PreferTypedParameterCodeFixProviderTests.cs | Adds focused code-fix tests for MSBuildTask0006/0007. |
| src/TaskAnalyzer.Tests/PreferTypedParameterAnalyzerTests.cs | Adds broad analyzer tests for MSBuildTask0006/0007, including inference and negative cases. |
OvesN
left a comment
There was a problem hiding this comment.
Very good PR but I think we should improve the UX: today the analyzer only helps once a task is already mid-migration (it keys off an existing new AbsolutePath(...) , Path.Combine , etc.). I'd love to see it suggests the type before migration starts, when a string / ITaskItem input flows into a non-safe/path API like File.Exists and we can likely reuse the non-safe-API detection the analyzer already
Good feedback, but we already have analyzers that handle this part - @JanProvaznik made them a while back. Does this satisfy your request? |
But I mean it does not suggest to use the right type? So what the workflow for user will be? They will see the warnings from Jan's analyzer about non-safe API calling and wrap it in .getAbsolutePath() after that your analyzer will fire and will tell the user to remove it and use the correct type for task parameter itself? |
|
Thanks for the review and detailed feedback @OvesN - I've incorporated it into new features, tests, and one additional diagnostic. |
OvesN
left a comment
There was a problem hiding this comment.
This isn't blocking, but in my opinion, it would be good to refactor the code and separate it into smaller, single-purpose functions wherever possible. That would make it much easier to follow. For example, OnCompilationStart could serve only as an orchestrator.
Also, there are several complex if statements where it's not immediately obvious what is being checked, at least to me. It would be helpful either to add a brief comment above those conditions explaining the intent, or to move the logic into small helper functions with clear, descriptive names.
…ediaries Extend MSBuildTask0007 to detect patterns like: AbsolutePath abs = TaskEnvironment.GetAbsolutePath(item.ItemSpec); new FileInfo(abs); The analyzer now suggests ITaskItem<FileInfo> or ITaskItem<DirectoryInfo> instead of ITaskItem<AbsolutePath> when the AbsolutePath is only used as an intermediary to construct FileInfo/DirectoryInfo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When an ITaskItem[] property is detected, the analyzer now suggests ITaskItem<T>[] instead of ITaskItem<T>. For example, MakeDir.Directories now correctly suggests ITaskItem<AbsolutePath>[] instead of ITaskItem<AbsolutePath>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The diagnostic message now correctly formats array suggestions as 'ITaskItem<T>[]' instead of 'ITaskItem<T[]>'. For example, RemoveDir's Directories property now suggests 'ITaskItem<AbsolutePath>[]' with the brackets outside the angle brackets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion docs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… specific When the analyzer detects both GetAbsolutePath(item.ItemSpec) and a downstream new FileInfo(abs)/new DirectoryInfo(abs) for the same property, the AbsolutePath diagnostic is suppressed in favor of the more specific FileInfo/DirectoryInfo suggestion. This is done by collecting diagnostics during operation analysis and deduplicating in RegisterSymbolEndAction. For example, ZipDirectory.SourceDirectory now only suggests ITaskItem<DirectoryInfo> instead of both ITaskItem<AbsolutePath> and ITaskItem<DirectoryInfo>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…and Path.Combine to any argument
When a task input's rooted path flows into a System.IO.File.* call, a FileStream/StreamReader/StreamWriter
constructor, or a System.IO.Directory.* call, suggest ITaskItem<FileInfo>/ITaskItem<DirectoryInfo> directly
rather than the more generic ITaskItem<AbsolutePath>. Tracing follows both the direct ItemSpec and an
AbsolutePath intermediary, including locals that are declared then assigned once (the common
'AbsolutePath? p = null; try { p = GetAbsolutePath(...); }' pattern).
Path.Combine now flags every distinct task input flowing into any argument position, not just the first.
Deduplication is reworked to use structured diagnostic properties instead of parsing messages, and resolves
contradictory file-vs-directory inference by falling back to AbsolutePath.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g detection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetParsedTypeFromMethod previously accepted any value type with a Parse/TryParse method (e.g. Guid, TimeSpan, DateTimeOffset). These types are not supported by ValueTypeParser, so suggesting ITaskItem<Guid> would result in a runtime failure. Restrict suggestions to only the types ValueTypeParser actually handles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Exclude TryParse from MSBuildTask0007: it is defensive (bool + out parameter), suggesting ITaskItem<T> would change error handling to a bind-time throw, and its multi-argument shape is never rewritable by the code fix. - Broaden the code fix's item-rule IsExpectedConversion to accept Convert.ToXxx and TaskEnvironment.GetAbsolutePath, so those reported diagnostics are actually fixable (previously only Parse was, leaving Convert/GetAbsolutePath diagnostics unfixable and able to block the whole-property fix). - Add analyzer + code fix tests covering the above. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…008 relative-default fix
MSBuildTask0007 now treats item.GetMetadata("FullPath") (the documented way to
read an item's absolute path, metadata name compared case-insensitively) the
same as item.ItemSpec, both as a detection source and as a fixable conversion
site rewritten to item.Value.
Also includes the MSBuildTask0008 diagnostic + code fix that moves relative
default path initialization into Execute() using TaskEnvironment.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…s inputs Addresses review feedback: the PreferTypedParameter analyzer previously fired on any task deriving from a base that implements IMultiThreadableTask, even without the [MSBuildMultiThreadableTask] attribute. Such a task has not opted into multithreaded support (the attribute is Inherited = false), so applicability now requires the attribute applied directly to the type. Input properties are now collected from the task class and all of its base types (GetPropertiesIncludingBaseTypes), so an ITaskItem/string input declared on a shared base task is still analyzed. Updated existing tests that opted in via the interface to also carry the attribute, and added tests locking in both behaviors. Refreshed README applicability tables and test count (184). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Previously a user with `Path.GetFullPath(prop)` (0002) or a raw string prop flowing into a System.IO path parameter (0003) had to apply the 0002/0003 fix first (which only introduces a conversion) and then was told to apply 0006 to retype the property. This daisy-chains two fixes. MSBuildTask0006 now also detects these raw-string shapes so the property can be retyped in one shot: - `Path.GetFullPath(prop)` -> suggests AbsolutePath - `File.*`/`Directory.*` string path args -> suggests FileInfo/DirectoryInfo - `new FileStream/StreamReader/StreamWriter(prop, ...)` -> suggests FileInfo The code fix rewrites each raw site as part of the retype: `Path.GetFullPath(prop)` collapses to `prop` (or `prop.FullName`), and a raw string consumption is fed through `prop.FullName` for FileInfo/DirectoryInfo (left unchanged for AbsolutePath, which converts to string implicitly). Args already rooted through TaskEnvironment.GetAbsolutePath/new AbsolutePath(...) are skipped. The fixer stays conservative: any unrecognized reference shape bails out with no fix. Adds 9 tests (6 analyzer, 3 fixer); total 193. Updates README. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
When a property's value flows into both System.IO.File and System.IO.Directory
path APIs but never through an AbsolutePath site, the suggested-type set was
{FileInfo, DirectoryInfo} with no AbsolutePath. The specific-type suppression
only fired when the set already contained AbsolutePath, so both FileInfo and
DirectoryInfo survived and the user saw two contradictory retype suggestions
with no coherent fallback.
Now, on a file/dir conflict we collapse to the AbsolutePath fallback in both
cases: if an AbsolutePath suggestion already exists we drop the specifics and
let it carry, otherwise we rewrite the specific diagnostic to AbsolutePath so
the property is still flagged with a single coherent type.
Adds ToAbsolutePathFallback helper and a test; total 194.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The previous IsFullyQualifiedPath recognized both Windows and Unix absolute forms regardless of the host OS, so "C:/path" was classified as fully qualified even on Linux — where it is really a relative path. That does not match what AbsolutePath does at runtime (Path.IsPathFullyQualified, unavailable on netstandard2.0), which is OS-specific. Polyfill the runtime's PathInternal.IsPartiallyQualified logic and branch on the current OS: on Unix only a '/'-rooted path is fully qualified; on Windows a UNC/device prefix or a drive-absolute "X:\"/"X:/" (with a valid drive letter) is fully qualified, while "\foo" (drive-relative) and "X:foo" (drive-relative) are not. This keeps MSBuildTask0008's relative-default detection aligned with how AbsolutePath would actually root the value. Adds a dedicated PathDefaultClassifierTests suite; total 204. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rename the type-resolution dedup locals for readability: suggestedTypesByProp -> suggestedTypesByPropertyKey, set -> suggestedTypes, key -> propertyKey, and the TryGetDiagnosticKey out parameter key -> propertyKey. No behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Split the monolithic OnCompilationStart into focused, named methods so the setup, gating, property collection, and dedup/report phases each read as a single responsibility (Veronika review feedback): - TryResolveWellKnownTaskTypes + WellKnownTaskTypes struct: resolve all required symbols once per compilation; fail fast when ITask or the [MSBuildMultiThreadableTask] attribute type is unavailable. - IsMultiThreadableTaskType: the ITask + directly-applied-attribute gate. - CollectInputProperties: partition public settable non-[Output] inputs into string and ITaskItem candidates. - DeduplicateAndReportDiagnostics: the SymbolEndAction body, extracted verbatim. Behavior-neutral; all 204 analyzer tests pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the scattered "System.IO.FileInfo" / "System.IO.DirectoryInfo" / "System.IO.FileSystemInfo" string literals with FileInfoFullName, DirectoryInfoFullName, and FileSystemInfoFullName constants on WellKnownTypeNames (Veronika review feedback), matching how every other resolved type name is centralized. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
0d8307e to
6c468a2
Compare
The OS-aware PathDefaultClassifier change made "C:/…" default values classify as relative on Unix (matching Path.IsPathFullyQualified), which is correct but broke three tests that hard-coded "C:/…" defaults and expected them to stay fully-qualified. A single literal can't be absolute on both OSes, so add a TestHelpers.FullyQualifiedPath(tail) helper that yields "C:/tail" on Windows and "/tail" on Unix, and interpolate it into the affected test sources: - PreferTypedParameterAnalyzerTests.FullyQualifiedDefault_StaysOnPathDiagnostic - PreferTypedParameterCodeFixProviderTests.Fix_0006_AbsolutePathDefault_NormalizedThroughAbsolutePath - PreferTypedParameterCodeFixProviderTests.Fix_0006_AbsolutePathDefault_FileInfo_NormalizedThroughAbsolutePath Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
OvesN
left a comment
There was a problem hiding this comment.
I found only one issue, otherwise after refactoring it looks great and after the fix it can be shipped.
….FullName When the code fix retypes a string property to FileInfo/DirectoryInfo, the raw string-consumption branch rewrites every string-argument site to prop.FullName. For a null-tolerant guard such as string.IsNullOrEmpty(prop) / string.IsNullOrWhiteSpace(prop), that rewrite dereferences a possibly-null FileInfo/DirectoryInfo and throws at runtime, silently defeating the guard (Veronika review feedback). Detect this shape via IsNullGuardArgument and return false from TryRewritePathConversion so the whole fix is withheld (the diagnostic still surfaces), preserving the conservative "all references safely rewritable" guarantee. AbsolutePath is unaffected: it is a struct with an implicit string conversion, so its sites are left unchanged and cannot NRE. Adds three tests: FileInfo + IsNullOrEmpty and DirectoryInfo + IsNullOrWhiteSpace both withhold the fix; AbsolutePath + IsNullOrEmpty is still fixed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…em<T> type arguments (#13973) ## Summary Adds **MSBuildTask0009 (UnsupportedTaskItemType)**: a Roslyn analyzer that fires when a task property uses `ITaskItem<T>` where `T` is not currently supported by MSBuild's task parameter binder. This catches generic type arguments that would fail at runtime. The analyzer currently recognizes `AbsolutePath`, `FileInfo`, and `DirectoryInfo`. Primitive and other value types will be added when the engine-side binding support lands. ## Stacked on - Requires: #13972 (MSBuildTask0006/0007 migration analyzers) - Next: #13974 (value type support) /cc @baronfel --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…itive conversions (#13974) ## Summary Expands `ITaskItem<T>` / `TaskItem<T>` engine binding beyond path types while keeping the accepted type set aligned with `ValueTypeParser`. The binder now accepts: - Directly parsed types: `string`, `bool`, `AbsolutePath`, `FileInfo`, `DirectoryInfo` - `Convert.ChangeType` types: `char`, numeric primitives, `decimal`, and `DateTime` Enums, nullable value types, `Guid`, `TimeSpan`, and custom structs remain unsupported. ## Changes - Replaces broad `IsValueType` checks with an explicit `ValueTypeParser`-aligned allowlist. - Applies the same validation to scalar and array inputs and outputs for both `ITaskItem<T>` and concrete `TaskItem<T>`. - Adds binding coverage for `string`, MSBuild boolean syntax, and the full supported/unsupported type matrix. - Updates MSBuildTask0009 to match runtime support and recommend only directly parsed types. - Adds **MSBuildTask0010** as an error when `ITaskItem<T>` uses a type parsed through `Convert.ChangeType`. These conversions use `CultureInfo.InvariantCulture`, which may not match the task's intended culture. Authors should bind as `ITaskItem<string>` and parse explicitly with the intended culture. ## Dependency status The preceding PRs (#13971, #13972, and #13973) are merged, and this PR is rebased directly on `main`. /cc @baronfel --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Adds two Roslyn analyzers to the MSBuild task analyzer package that suggest migration from string to typed task parameters (AbsolutePath/FileInfo/DirectoryInfo/ITaskItem):
new AbsolutePath(Prop),
new FileInfo(Prop), or similar patterns, suggesting the property be retyped to the stronger type.
Both analyzers include code-fix providers that automatically apply the suggested retyping.
Analyzers are restricted to multithreaded tasks only (classes implementing
IMultiThreadableTaskor annotated with the[MSBuildMultiThreadableTask]attribute).Stacked on
/cc @baronfel