Add IMultiThreadableTask analyzer specification #12583
Conversation
This spec proposes a Roslyn analyzer to detect unsafe API usage in IMultiThreadableTask implementations. The analyzer categorizes problematic APIs into 4 tiers (MSB9996-9999) and provides automated code fixes for common issues. Key features: - MSB9999 (Error): Critical APIs with no safe alternative (Environment.Exit, ThreadPool settings) - MSB9998 (Warning): APIs requiring TaskEnvironment alternatives (Environment.CurrentDirectory, GetEnvironmentVariable) - MSB9997 (Warning): File APIs requiring absolute paths (File.*, Directory.*) - MSB9996 (Warning): Potentially problematic APIs (Console.*, Assembly.Load*) Full implementation available in PR dotnet#12143
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| ### Problem | ||
|
|
||
| MSBuild's multithreaded execution allows tasks implementing `IMultiThreadableTask` to run concurrently. Many .NET APIs depend on process-global state (working directory, environment variables), creating race conditions: |
There was a problem hiding this comment.
I might broaden this a bit to something like "some things are bad practice to do in tasks, so let's have an analyzer that can help users write tasks. The first things we'll focus on will be the threading issues"
| | MSB9999 | Error | Critical APIs (no safe alternative) | No | | ||
| | MSB9998 | Warning | APIs needing TaskEnvironment | Partial | | ||
| | MSB9997 | Warning | File APIs needing absolute paths | Yes | | ||
| | MSB9996 | Warning | Potentially problematic APIs | No | |
There was a problem hiding this comment.
I think I'd like to have a distinct namespace/error code space here since these aren't exactly build-time errors.
Maybe just MSBuildTask1234? @baronfel?
| **Detection**: Warns if first `string` parameter is not: | ||
| - Wrapped with `TaskEnvironment.GetAbsolutePath()` | ||
| - A `.FullName` property | ||
| - An `AbsolutePath` type |
There was a problem hiding this comment.
I'd love it if we could detect + accept item.GetMetadataValue("FullPath")
| **APIs**: | ||
| - `Assembly.Load*`, `LoadFrom`, `LoadFile` - May cause conflicts | ||
| - `Activator.CreateInstance*`, `AppDomain.Load/CreateInstance*` - May cause conflicts | ||
| - `Console.*` (Write, WriteLine, ReadLine, etc.) - Interferes with logging |
There was a problem hiding this comment.
I'd promote the Console.* ones to "always wrong".
| **Pros**: Already referenced, zero config, automatic updates | ||
| **Cons**: Increases package size | ||
|
|
||
| **Question**: Consensus on Utilities.Core vs standalone package? |
There was a problem hiding this comment.
I think I want a new package like Microsoft.Build.TaskAuthoring that
- has logic to make sure you're not unnecessarily redistributing MSBuild assemblies
- Has this analyzer
- provides the API-surface references as needed
- maybe in the future some test thing?
There was a problem hiding this comment.
so is your suggestion to rework this to a broader "task authoring user experience" doc?
This feels like a overlapping epic, rather than only a part of multithreaded.
There was a problem hiding this comment.
Agreed. For the scope of this doc I think my answer is "let's make it a standalone package".
|
|
||
| **Proposal**: Only analyze `IMultiThreadableTask` implementations | ||
|
|
||
| **Question**: Offer opt-in for all Task types? |
There was a problem hiding this comment.
I'd do "all tasks get analyzed by default" even if no rules apply to them yet.
|
|
||
| **Proposal**: Only analyze `IMultiThreadableTask` implementations | ||
|
|
||
| **Question**: Offer opt-in for all Task types? |
There was a problem hiding this comment.
We should also have an opt-in for helper classes IMO. let's not make it too easy to break everything by moving "the hard parts" out of the task itself.
2 participants
This spec proposes a Roslyn analyzer to detect unsafe API usage in IMultiThreadableTask implementations. The analyzer categorizes problematic APIs into 4 tiers (MSB9996-9999) and provides automated code fixes for common issues.
Key features:
Full sample implementation available in PR #12143
Fixes #
Context
Changes Made
Testing
Notes