From bcdc452a535501f4e8d9ca9a22d0b761eb0a5ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 30 Sep 2025 19:14:29 +0200 Subject: [PATCH 1/2] Add IMultiThreadableTask analyzer specification 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 #12143 --- .../specs/multithreading/analyzer-spec.md | 409 ++++++++++++++++++ 1 file changed, 409 insertions(+) create mode 100644 documentation/specs/multithreading/analyzer-spec.md diff --git a/documentation/specs/multithreading/analyzer-spec.md b/documentation/specs/multithreading/analyzer-spec.md new file mode 100644 index 00000000000..4108e20ce27 --- /dev/null +++ b/documentation/specs/multithreading/analyzer-spec.md @@ -0,0 +1,409 @@ +# IMultiThreadableTask Analyzer - Design Proposal + +**Status**: Proposal for Review +**Target MSBuild Version**: 18.1+ +**Authors**: @JanProvaznik +**Last Updated**: September 30, 2025 + +--- + +## Executive Summary + +### Problem Statement + +MSBuild's multithreaded execution allows tasks implementing `IMultiThreadableTask` to run concurrently within a single process. However, many .NET APIs commonly used in tasks depend on process-global state (e.g., current working directory, environment variables), creating race conditions when tasks execute simultaneously. + +**Example Race Condition**: +```csharp +// Task A: sets working directory to C:\ProjectA +// Task B: sets working directory to C:\ProjectB (races with A) +// Task A: calls File.Exists("bin\output.dll") (resolves to C:\ProjectB\bin - WRONG!) +``` + +### Proposed Solution + +A Roslyn analyzer that detects unsafe API usage in `IMultiThreadableTask` implementations at compile time, with an automated code fixer to wrap file paths with `TaskEnvironment.GetAbsolutePath()`. + +### Key Benefits + +- Catches threading issues at compile time instead of runtime +- Automated fixes reduce manual effort for task authors +- Educates developers about thread-safety requirements +- Prevents race conditions in multithreaded builds + +--- + +## 1. Design Overview + +### 1.1 Scope + +The analyzer detects four categories of problematic APIs, each with its own diagnostic code: + +1. **MSB9999**: APIs that should cause build errors (e.g., `Environment.Exit`, `Process.Kill`) +2. **MSB9998**: APIs requiring TaskEnvironment alternatives (e.g., `Environment.CurrentDirectory`, `Process.Start`) +3. **MSB9997**: Conditionally unsafe with relative paths (e.g., `File.Exists`, `Directory.CreateDirectory`) +4. **MSB9996**: APIs that should generate warnings (e.g., `Assembly.Load`, static fields) + +Detailed API list and categorization available in [thread-safe-tasks-api-analysis.md](thread-safe-tasks-api-analysis.md). + +### 1.2 Diagnostic Information + +| Code | Severity | Description | Code Fixer Available | +|------|----------|-------------|---------------------| +| MSB9999 | Error (proposal) | Critical errors - terminates process, kills threads | No | +| MSB9998 | Error (proposal) | Must use TaskEnvironment APIs instead | No | +| MSB9997 | Warning | Unsafe with relative paths - wrap with GetAbsolutePath | Yes | +| MSB9996 | Warning | Potential threading issues - review carefully | No | + +**Category**: Microsoft.Build.Tasks +**Activation**: Only within types implementing `IMultiThreadableTask` + +### 1.3 Code Fixer Capability + +Offers automated fix **only for MSB9997** (file path warnings): wraps path arguments with `TaskEnvironment.GetAbsolutePath()`. + +**Example Transformation**: +```csharp +// Before (MSB9997 warning): +if (File.Exists(relativePath)) { ... } + +// After (automated fix applied): +if (File.Exists(TaskEnvironment.GetAbsolutePath(relativePath))) { ... } +``` + +**Not available for**: MSB9999, MSB9998, MSB9996 (these require manual code changes or architectural decisions) + +--- + +## 2. Detection Strategy + +### 2.1 Category 1: Critical Errors (MSB9999) + +**Severity**: Error +**Description**: APIs that are never safe in multithreaded tasks - no acceptable alternative + +Detected via exact symbol matching: +- `Environment.Exit` - Terminates entire process +- `Environment.FailFast` (all overloads) - Immediately terminates process +- `Process.GetCurrentProcess().Kill()` - Terminates entire process +- `ThreadPool.SetMinThreads`, `ThreadPool.SetMaxThreads` - Modifies process-wide thread pool settings +- `CultureInfo.DefaultThreadCurrentCulture` (setter) - Affects all new threads in process +- `CultureInfo.DefaultThreadCurrentUICulture` (setter) - Affects all new threads in process + +**Code Fixer**: Not applicable (no safe alternative exists) + +**Rationale**: These APIs are never acceptable in multithreaded tasks as they affect the entire MSBuild process or all threads. + +### 2.2 Category 2: TaskEnvironment Required (MSB9998) + +**Severity**: Warning +**Description**: APIs that modify/access process-global state - must use TaskEnvironment instead + +Detected via exact symbol matching: +- `Environment.CurrentDirectory` (getter/setter) - Use `TaskEnvironment.ProjectCurrentDirectory` +- `Environment.GetEnvironmentVariable` - Use `TaskEnvironment.GetEnvironmentVariable` +- `Environment.SetEnvironmentVariable(string, string)` - Use `TaskEnvironment.SetEnvironmentVariable` +- `Environment.SetEnvironmentVariable(string, string, EnvironmentVariableTarget)` - No direct equivalent (drop target parameter) +- `Path.GetFullPath` (all overloads) - Use `TaskEnvironment.GetAbsolutePath` +- `Process.Start` (non-ProcessStartInfo overloads) - Use `TaskEnvironment.GetProcessStartInfo` +- `ProcessStartInfo` constructors - Use `TaskEnvironment.GetProcessStartInfo` + +**Code Fixer**: Automatic fixes provided for: +- ✅ `Environment.CurrentDirectory` → `TaskEnvironment.ProjectCurrentDirectory` +- ✅ `Environment.GetEnvironmentVariable(name)` → `TaskEnvironment.GetEnvironmentVariable(name)` +- ✅ `Environment.SetEnvironmentVariable(name, value)` → `TaskEnvironment.SetEnvironmentVariable(name, value)` +- ✅ `Path.GetFullPath(path)` → `TaskEnvironment.GetAbsolutePath(path)` +- ❌ `Environment.SetEnvironmentVariable(name, value, target)` - No code fixer (complex) +- ❌ `Process.Start(...)` - No code fixer (requires multi-statement refactoring) +- ❌ `ProcessStartInfo` constructors - No code fixer (requires multi-statement refactoring) + +**Rationale**: These APIs have TaskEnvironment alternatives. Simple property/method replacements are automated; complex transformations require manual refactoring. + +### 2.3 Category 3: Conditional - Require Absolute Paths (MSB9997) + +**Severity**: Warning +**Description**: File system APIs unsafe with relative paths + +File system types analyzed: +- `System.IO.File` +- `System.IO.Directory` +- `System.IO.FileInfo` +- `System.IO.DirectoryInfo` +- `System.IO.FileStream` +- `System.IO.StreamReader` +- `System.IO.StreamWriter` + +**Detection Logic**: +1. Check if invoked member belongs to one of these types +2. Find first `string` parameter (assumed to be file path) +3. Check if argument is wrapped with `TaskEnvironment.GetAbsolutePath()` or uses `.FullName` property +4. Report MSB9997 only if NOT wrapped + +**Recognized Safe Patterns**: +```csharp +// ✅ Safe - wrapped +File.Exists(TaskEnvironment.GetAbsolutePath(path)) + +// ✅ Safe - absolute path property +File.Exists(fileInfo.FullName) + +// ❌ MSB9997 - not wrapped +File.Exists(path) +``` + +**Code Fixer**: Yes - wraps first string argument with `TaskEnvironment.GetAbsolutePath(...)` + +**Rationale**: These APIs are safe when used with absolute paths, making them fixable via automation. + +### 2.4 Category 4: Warnings - Potential Issues (MSB9996) + +**Severity**: Warning +**Description**: APIs that may cause threading issues but require case-by-case review + +Detected via exact symbol matching: +- `Assembly.Load*`, `Assembly.LoadFrom`, `Assembly.LoadFile` - May cause version conflicts +- `Activator.CreateInstance*` - May cause version conflicts +- `AppDomain.Load`, `AppDomain.CreateInstance*` - May cause version conflicts +- `Console.Write`, `Console.WriteLine`, `Console.ReadLine` - Interferes with build output +- Static field access (future enhancement) - Shared state across threads + +**Code Fixer**: Not applicable (requires case-by-case analysis) + +**Rationale**: These APIs aren't always wrong but require careful review for thread-safety. + +**Recognized Safe Patterns**: +```csharp +// ✅ Safe - wrapped +File.Exists(TaskEnvironment.GetAbsolutePath(path)) + +// ✅ Safe - absolute path property +File.Exists(fileInfo.FullName) +File.Exists(directoryInfo.FullName) + +// ❌ Warning - not wrapped +File.Exists(path) +``` + +**Design Decision**: Pattern-based detection (vs. listing every method) keeps analyzer maintainable as APIs evolve. + +### 2.3 Current Limitations + +- **No data-flow analysis**: Doesn't track if `path` variable already contains absolute path +- **First parameter assumption**: Only checks first string parameter for paths +- **No constant analysis**: Warns even for string literals like `"C:\\Windows\\System32"` + +**Rationale**: These limitations prevent false negatives at cost of some false positives. Developers can suppress warnings where appropriate. + +--- + +## 3. Code Fixer Design + +### 3.1 Scope + +Code fixers provided for: +- **MSB9997**: Wraps file path arguments with `TaskEnvironment.GetAbsolutePath(...)` +- **MSB9998**: Simple API migrations to TaskEnvironment (property/method replacements only) + +### 3.2 MSB9997 Transformation Rules (File Paths) + +**Path wrapping fixes:** +- Wraps first string argument with `TaskEnvironment.GetAbsolutePath(...)` +- Preserves all other arguments unchanged +- Adds `using Microsoft.Build.Framework;` directive if needed + +**Example:** +```csharp +// Before: +File.Exists(somePath) + +// After: +File.Exists(TaskEnvironment.GetAbsolutePath(somePath)) +``` + +### 3.3 MSB9998 Transformation Rules (Simple API Migrations) + +**Property replacements:** +- `Environment.CurrentDirectory` → `TaskEnvironment.ProjectCurrentDirectory` +- Handles both getter and setter contexts +- Adds cast to `string` when needed for `AbsolutePath` return type + +**Method replacements:** +- `Environment.GetEnvironmentVariable(name)` → `TaskEnvironment.GetEnvironmentVariable(name)` +- `Environment.SetEnvironmentVariable(name, value)` → `TaskEnvironment.SetEnvironmentVariable(name, value)` +- `Path.GetFullPath(path)` → `TaskEnvironment.GetAbsolutePath(path)` + +**Not fixable (too complex):** +- `Process.Start(...)` - Requires multi-statement refactoring to use `GetProcessStartInfo()` +- `ProcessStartInfo` constructors - Requires multi-statement refactoring +- `Environment.SetEnvironmentVariable(name, value, target)` - Target parameter must be manually removed + +**Examples:** +```csharp +// Before: +string dir = Environment.CurrentDirectory; +string value = Environment.GetEnvironmentVariable("PATH"); +string full = Path.GetFullPath("file.txt"); + +// After: +string dir = TaskEnvironment.ProjectCurrentDirectory; +string value = TaskEnvironment.GetEnvironmentVariable("PATH"); +string full = TaskEnvironment.GetAbsolutePath("file.txt"); +``` + +### 3.4 User Experience + +- Fixes appear in Visual Studio Quick Actions (Ctrl+.) +- Action titles: + - "Wrap with TaskEnvironment.GetAbsolutePath()" (MSB9997) + - "Use TaskEnvironment.ProjectCurrentDirectory" (MSB9998) + - "Use TaskEnvironment.GetEnvironmentVariable()" (MSB9998) + - "Use TaskEnvironment.SetEnvironmentVariable()" (MSB9998) + - "Use TaskEnvironment.GetAbsolutePath()" (MSB9998 for Path.GetFullPath) +- Available immediately when warnings appear + +--- + +## 4. Open Questions for Review + +### 4.1 Distribution Model (Proposal) + +**Recommended**: Ship analyzer with `Microsoft.Build.Utilities.Core` NuGet package + +**Pros**: +- Task authors already reference this package +- Zero configuration required +- Analyzer version stays synchronized with APIs +- Automatic updates + +**Cons**: +- Increases package size +- Cannot be disabled without disabling entire analyzer infrastructure + +**Alternative Considered**: Standalone `Microsoft.Build.Analyzers` package +- **Rejected**: Discovery problem, extra manual step reduces adoption + +**Question**: Do we have consensus on shipping with Utilities.Core? + +### 4.2 Default Severity Levels + +**Current Proposal**: +- **MSB9999**: Error (no safe alternative - Environment.Exit, Process.Kill, ThreadPool settings, CultureInfo defaults) +- **MSB9998**: Warning (has TaskEnvironment alternative - Environment.CurrentDirectory, Get/SetEnvironmentVariable, Path.GetFullPath, Process.Start) +- **MSB9997**: Warning (conditional safety - file path methods) +- **MSB9996**: Warning (informational - Console, Assembly.Load) + +**Rationale for MSB9999 as Error**: These APIs have no acceptable workaround and would cause serious issues in multithreaded builds (process termination, process-wide state corruption). + +**Rationale for MSB9998 as Warning**: While these APIs need migration, they have clear TaskEnvironment alternatives and code fixers to ease transition. Warning severity allows gradual adoption. + +**Question**: Should MSB9998 be elevated to Error in a future release after adoption period? + +### 4.3 Opt-Out Mechanism + +**Proposal**: MSBuild property `EnableMSBuildThreadSafetyAnalyzer` + +```xml + + false + +``` + +**Question**: Is property name acceptable? Should we support per-diagnostic severity via `.editorconfig` only? + +### 4.4 Scope of Analysis + +**Proposal**: Analyzer only activates within types implementing `IMultiThreadableTask` + +**Alternative**: Analyze all code +- **Rejected**: Creates noise for non-multithreadable tasks + +**Question**: Should we offer opt-in to analyze all Task types (not just IMultiThreadableTask)? + +--- + +## 5. Testing Approach + +### 5.1 Validation + +A demo project demonstrates analyzer behavior: +- **ProblematicTask**: Task with 9 unsafe API usages → expects mixture of MSB9999/MSB9998/MSB9997/MSB9996 warnings +- **CorrectTask**: Same logic using wrapped paths → expects 0 warnings + +**Location**: `src/ThreadSafeTaskAnalyzer/VisualStudioDemo/` + +### 5.2 Manual Testing + +1. Open demo in Visual Studio +2. Observe diagnostics on unsafe API usages (squiggles based on severity) +3. Verify Quick Action offers "Wrap with..." fix for MSB9997 only +4. Apply fix and confirm warning disappears + +--- + +## 6. Design Rationale + +### 6.1 Why Two Detection Modes? + +**Always-banned APIs** require explicit symbol matching because: +- No safe usage pattern exists (e.g., `Environment.Exit` always wrong) +- Need precise, actionable error messages + +**Conditionally-banned APIs** use pattern detection because: +- Safe when used correctly (with absolute paths) +- Hundreds of methods across file system types - listing all is unmaintainable + +### 6.2 Why Check Only First String Parameter? + +.NET file system APIs follow consistent pattern: +```csharp +File.ReadAllText(string path) // path is first param +Directory.CreateDirectory(string path) // path is first param +new FileStream(string path, FileMode mode) // path is first param +``` + +**Tradeoff**: Simplicity and performance vs. handling rare edge cases. False negatives possible for unusual APIs, but unlikely in practice. + +### 6.3 Why Not Full Data-Flow Analysis? + +**Rejected**: Tracking whether variables contain absolute paths via data-flow analysis + +**Reasons**: +- Extremely complex for limited benefit +- Performance impact on live editing +- Many code paths are unknowable at compile time (user input, config files, etc.) + +**Chosen approach**: Conservative warnings + developer suppressions for known-safe cases + +--- + +## 7. Future Enhancements (Out of Scope for v1) + +1. **Constant analysis**: Suppress warnings for string literal absolute paths (`"C:\\Windows"`) +2. **Data-flow tracking**: Recognize when variable provably contains absolute path +3. **Strict mode**: Warn on all path operations, even when wrapped +4. **VB.NET support**: Currently C# only +5. **Additional banned APIs**: Console.SetOut, AppDomain.SetData, etc. +6. **Build-time enforcement**: MSBuild task that fails build on violations + +--- + +## 8. Known Limitations + +1. **String literals**: Warns on `File.Exists("C:\\absolute\\path")` even though safe +2. **Method chaining**: May not recognize `GetAbsolutePath()` in complex expressions +3. **Reflection**: Cannot detect dynamic API calls via reflection +4. **Code generation**: Does not analyze dynamically generated code +5. **Performance**: No specific performance optimizations yet implemented + +These are acceptable for initial version. Can be addressed based on user feedback. + +--- + +## 9. References +Full PR: [https://github.com/dotnet/msbuild/pull/12143](https://github.com/dotnet/msbuild/pull/12143) + +- [thread-safe-tasks-api-analysis.md](thread-safe-tasks-api-analysis.md) - Complete list of banned APIs +- [thread-safe-tasks.md](thread-safe-tasks.md) - Thread-safe tasks overview +- [multithreaded-msbuild.md](multithreaded-msbuild.md) - Multithreaded MSBuild specification +- Demo implementation: `src/ThreadSafeTaskAnalyzer/` +- Sample usage: `src/ThreadSafeTaskAnalyzer/VisualStudioDemo/` From cd711acccd2bd234173eaaa3f514d9e35921638d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Wed, 1 Oct 2025 12:10:05 +0200 Subject: [PATCH 2/2] make more concise --- .../specs/multithreading/analyzer-spec.md | 428 +++++------------- 1 file changed, 119 insertions(+), 309 deletions(-) diff --git a/documentation/specs/multithreading/analyzer-spec.md b/documentation/specs/multithreading/analyzer-spec.md index 4108e20ce27..a664ad72499 100644 --- a/documentation/specs/multithreading/analyzer-spec.md +++ b/documentation/specs/multithreading/analyzer-spec.md @@ -1,409 +1,219 @@ # IMultiThreadableTask Analyzer - Design Proposal -**Status**: Proposal for Review -**Target MSBuild Version**: 18.1+ -**Authors**: @JanProvaznik -**Last Updated**: September 30, 2025 +**Target MSBuild Version**: 18.1+ +**Authors**: @JanProvaznik +**Last Updated**: October 1, 2025 --- ## Executive Summary -### Problem Statement +### Problem -MSBuild's multithreaded execution allows tasks implementing `IMultiThreadableTask` to run concurrently within a single process. However, many .NET APIs commonly used in tasks depend on process-global state (e.g., current working directory, environment variables), creating race conditions when tasks execute simultaneously. +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: -**Example Race Condition**: ```csharp -// Task A: sets working directory to C:\ProjectA -// Task B: sets working directory to C:\ProjectB (races with A) -// Task A: calls File.Exists("bin\output.dll") (resolves to C:\ProjectB\bin - WRONG!) +// Task A: Environment.CurrentDirectory = "C:\\ProjectA" +// Task B: Environment.CurrentDirectory = "C:\\ProjectB" (races with A) +// Task A: File.Exists("bin\\output.dll") // Resolves incorrectly! ``` -### Proposed Solution +### Solution -A Roslyn analyzer that detects unsafe API usage in `IMultiThreadableTask` implementations at compile time, with an automated code fixer to wrap file paths with `TaskEnvironment.GetAbsolutePath()`. +Roslyn analyzer detecting unsafe API usage in `IMultiThreadableTask` with automated code fixers. -### Key Benefits +### Benefits -- Catches threading issues at compile time instead of runtime -- Automated fixes reduce manual effort for task authors -- Educates developers about thread-safety requirements -- Prevents race conditions in multithreaded builds +- Compile-time detection +- Automated fixes for common patterns +- Education through diagnostic messages --- ## 1. Design Overview -### 1.1 Scope +### 1.1 Diagnostic Codes -The analyzer detects four categories of problematic APIs, each with its own diagnostic code: +| Code | Severity | Description | Fixer | +|------|----------|-------------|-------| +| 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 | -1. **MSB9999**: APIs that should cause build errors (e.g., `Environment.Exit`, `Process.Kill`) -2. **MSB9998**: APIs requiring TaskEnvironment alternatives (e.g., `Environment.CurrentDirectory`, `Process.Start`) -3. **MSB9997**: Conditionally unsafe with relative paths (e.g., `File.Exists`, `Directory.CreateDirectory`) -4. **MSB9996**: APIs that should generate warnings (e.g., `Assembly.Load`, static fields) +**Activation**: Only in types implementing `IMultiThreadableTask` -Detailed API list and categorization available in [thread-safe-tasks-api-analysis.md](thread-safe-tasks-api-analysis.md). +### 1.2 Code Fixer Support -### 1.2 Diagnostic Information - -| Code | Severity | Description | Code Fixer Available | -|------|----------|-------------|---------------------| -| MSB9999 | Error (proposal) | Critical errors - terminates process, kills threads | No | -| MSB9998 | Error (proposal) | Must use TaskEnvironment APIs instead | No | -| MSB9997 | Warning | Unsafe with relative paths - wrap with GetAbsolutePath | Yes | -| MSB9996 | Warning | Potential threading issues - review carefully | No | - -**Category**: Microsoft.Build.Tasks -**Activation**: Only within types implementing `IMultiThreadableTask` - -### 1.3 Code Fixer Capability - -Offers automated fix **only for MSB9997** (file path warnings): wraps path arguments with `TaskEnvironment.GetAbsolutePath()`. - -**Example Transformation**: -```csharp -// Before (MSB9997 warning): -if (File.Exists(relativePath)) { ... } - -// After (automated fix applied): -if (File.Exists(TaskEnvironment.GetAbsolutePath(relativePath))) { ... } -``` - -**Not available for**: MSB9999, MSB9998, MSB9996 (these require manual code changes or architectural decisions) +- **MSB9997**: Wraps paths with `TaskEnvironment.GetAbsolutePath()` +- **MSB9998**: Replaces simple APIs (CurrentDirectory, GetEnvironmentVariable, GetFullPath) +- **Manual**: Process.Start, ProcessStartInfo, MSB9999, MSB9996 --- ## 2. Detection Strategy -### 2.1 Category 1: Critical Errors (MSB9999) - -**Severity**: Error -**Description**: APIs that are never safe in multithreaded tasks - no acceptable alternative - -Detected via exact symbol matching: -- `Environment.Exit` - Terminates entire process -- `Environment.FailFast` (all overloads) - Immediately terminates process -- `Process.GetCurrentProcess().Kill()` - Terminates entire process -- `ThreadPool.SetMinThreads`, `ThreadPool.SetMaxThreads` - Modifies process-wide thread pool settings -- `CultureInfo.DefaultThreadCurrentCulture` (setter) - Affects all new threads in process -- `CultureInfo.DefaultThreadCurrentUICulture` (setter) - Affects all new threads in process - -**Code Fixer**: Not applicable (no safe alternative exists) - -**Rationale**: These APIs are never acceptable in multithreaded tasks as they affect the entire MSBuild process or all threads. - -### 2.2 Category 2: TaskEnvironment Required (MSB9998) - -**Severity**: Warning -**Description**: APIs that modify/access process-global state - must use TaskEnvironment instead - -Detected via exact symbol matching: -- `Environment.CurrentDirectory` (getter/setter) - Use `TaskEnvironment.ProjectCurrentDirectory` -- `Environment.GetEnvironmentVariable` - Use `TaskEnvironment.GetEnvironmentVariable` -- `Environment.SetEnvironmentVariable(string, string)` - Use `TaskEnvironment.SetEnvironmentVariable` -- `Environment.SetEnvironmentVariable(string, string, EnvironmentVariableTarget)` - No direct equivalent (drop target parameter) -- `Path.GetFullPath` (all overloads) - Use `TaskEnvironment.GetAbsolutePath` -- `Process.Start` (non-ProcessStartInfo overloads) - Use `TaskEnvironment.GetProcessStartInfo` -- `ProcessStartInfo` constructors - Use `TaskEnvironment.GetProcessStartInfo` - -**Code Fixer**: Automatic fixes provided for: -- ✅ `Environment.CurrentDirectory` → `TaskEnvironment.ProjectCurrentDirectory` -- ✅ `Environment.GetEnvironmentVariable(name)` → `TaskEnvironment.GetEnvironmentVariable(name)` -- ✅ `Environment.SetEnvironmentVariable(name, value)` → `TaskEnvironment.SetEnvironmentVariable(name, value)` -- ✅ `Path.GetFullPath(path)` → `TaskEnvironment.GetAbsolutePath(path)` -- ❌ `Environment.SetEnvironmentVariable(name, value, target)` - No code fixer (complex) -- ❌ `Process.Start(...)` - No code fixer (requires multi-statement refactoring) -- ❌ `ProcessStartInfo` constructors - No code fixer (requires multi-statement refactoring) - -**Rationale**: These APIs have TaskEnvironment alternatives. Simple property/method replacements are automated; complex transformations require manual refactoring. - -### 2.3 Category 3: Conditional - Require Absolute Paths (MSB9997) - -**Severity**: Warning -**Description**: File system APIs unsafe with relative paths - -File system types analyzed: -- `System.IO.File` -- `System.IO.Directory` -- `System.IO.FileInfo` -- `System.IO.DirectoryInfo` -- `System.IO.FileStream` -- `System.IO.StreamReader` -- `System.IO.StreamWriter` - -**Detection Logic**: -1. Check if invoked member belongs to one of these types -2. Find first `string` parameter (assumed to be file path) -3. Check if argument is wrapped with `TaskEnvironment.GetAbsolutePath()` or uses `.FullName` property -4. Report MSB9997 only if NOT wrapped - -**Recognized Safe Patterns**: -```csharp -// ✅ Safe - wrapped -File.Exists(TaskEnvironment.GetAbsolutePath(path)) - -// ✅ Safe - absolute path property -File.Exists(fileInfo.FullName) +### 2.1 MSB9999: Critical Errors -// ❌ MSB9997 - not wrapped -File.Exists(path) -``` +**Severity**: Error | **Detection**: Symbol matching | **Fixer**: No -**Code Fixer**: Yes - wraps first string argument with `TaskEnvironment.GetAbsolutePath(...)` +**APIs**: +- `Environment.Exit`, `FailFast` - Terminate process +- `Process.Kill()` - Terminates process +- `ThreadPool.SetMinThreads/MaxThreads` - Process-wide settings +- `CultureInfo.DefaultThreadCurrent*Culture` (setters) - Affect all threads -**Rationale**: These APIs are safe when used with absolute paths, making them fixable via automation. +**Rationale**: No safe alternative. Tasks signal failure via return value/exception. -### 2.4 Category 4: Warnings - Potential Issues (MSB9996) +### 2.2 MSB9998: TaskEnvironment Required -**Severity**: Warning -**Description**: APIs that may cause threading issues but require case-by-case review +**Severity**: Warning | **Detection**: Symbol matching | **Fixer**: Partial -Detected via exact symbol matching: -- `Assembly.Load*`, `Assembly.LoadFrom`, `Assembly.LoadFile` - May cause version conflicts -- `Activator.CreateInstance*` - May cause version conflicts -- `AppDomain.Load`, `AppDomain.CreateInstance*` - May cause version conflicts -- `Console.Write`, `Console.WriteLine`, `Console.ReadLine` - Interferes with build output -- Static field access (future enhancement) - Shared state across threads +**APIs**: +- `Environment.CurrentDirectory` → `TaskEnvironment.ProjectCurrentDirectory` (✅ fixer) +- `Environment.GetEnvironmentVariable` → `TaskEnvironment.GetEnvironmentVariable` (✅ fixer) +- `Environment.SetEnvironmentVariable(name, value)` → `TaskEnvironment.SetEnvironmentVariable` (✅ fixer) +- `Path.GetFullPath` → `TaskEnvironment.GetAbsolutePath` (✅ fixer) +- `Process.Start(...)`, `ProcessStartInfo` ctors → `TaskEnvironment.GetProcessStartInfo()` (❌ manual) -**Code Fixer**: Not applicable (requires case-by-case analysis) +### 2.3 MSB9997: File System APIs -**Rationale**: These APIs aren't always wrong but require careful review for thread-safety. +**Severity**: Warning | **Detection**: Pattern matching | **Fixer**: Yes -**Recognized Safe Patterns**: -```csharp -// ✅ Safe - wrapped -File.Exists(TaskEnvironment.GetAbsolutePath(path)) +**Types analyzed**: `File`, `Directory`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter` -// ✅ Safe - absolute path property -File.Exists(fileInfo.FullName) -File.Exists(directoryInfo.FullName) +**Detection**: Warns if first `string` parameter is not: +- Wrapped with `TaskEnvironment.GetAbsolutePath()` +- A `.FullName` property +- An `AbsolutePath` type -// ❌ Warning - not wrapped -File.Exists(path) +**Example**: +```csharp +File.Exists(TaskEnvironment.GetAbsolutePath(path)); // ✅ +File.Exists(fileInfo.FullName); // ✅ +File.Exists(path); // ❌ MSB9997 ``` -**Design Decision**: Pattern-based detection (vs. listing every method) keeps analyzer maintainable as APIs evolve. +### 2.4 MSB9996: Potential Issues -### 2.3 Current Limitations +**Severity**: Warning | **Detection**: Symbol matching | **Fixer**: No -- **No data-flow analysis**: Doesn't track if `path` variable already contains absolute path -- **First parameter assumption**: Only checks first string parameter for paths -- **No constant analysis**: Warns even for string literals like `"C:\\Windows\\System32"` +**APIs**: +- `Assembly.Load*`, `LoadFrom`, `LoadFile` - May cause conflicts +- `Activator.CreateInstance*`, `AppDomain.Load/CreateInstance*` - May cause conflicts +- `Console.*` (Write, WriteLine, ReadLine, etc.) - Interferes with logging -**Rationale**: These limitations prevent false negatives at cost of some false positives. Developers can suppress warnings where appropriate. +**Rationale**: Not always wrong, requires case-by-case review. ---- +### 2.5 Limitations -## 3. Code Fixer Design +- No data-flow analysis (doesn't track if variable is absolute) +- First-parameter heuristic only +- No constant folding (warns on `"C:\\Windows"` literals) -### 3.1 Scope +**Suppression**: `#pragma`, `.editorconfig`, or `[SuppressMessage]` -Code fixers provided for: -- **MSB9997**: Wraps file path arguments with `TaskEnvironment.GetAbsolutePath(...)` -- **MSB9998**: Simple API migrations to TaskEnvironment (property/method replacements only) +--- -### 3.2 MSB9997 Transformation Rules (File Paths) +## 3. Code Fixers -**Path wrapping fixes:** -- Wraps first string argument with `TaskEnvironment.GetAbsolutePath(...)` -- Preserves all other arguments unchanged -- Adds `using Microsoft.Build.Framework;` directive if needed +### 3.1 MSB9998: API Replacements -**Example:** ```csharp -// Before: -File.Exists(somePath) - -// After: -File.Exists(TaskEnvironment.GetAbsolutePath(somePath)) +Environment.CurrentDirectory → TaskEnvironment.ProjectCurrentDirectory +Environment.GetEnvironmentVariable → TaskEnvironment.GetEnvironmentVariable +Environment.SetEnvironmentVariable → TaskEnvironment.SetEnvironmentVariable +Path.GetFullPath → TaskEnvironment.GetAbsolutePath ``` -### 3.3 MSB9998 Transformation Rules (Simple API Migrations) +### 3.2 MSB9997: Path Wrapping -**Property replacements:** -- `Environment.CurrentDirectory` → `TaskEnvironment.ProjectCurrentDirectory` -- Handles both getter and setter contexts -- Adds cast to `string` when needed for `AbsolutePath` return type +Wraps first `string` argument: -**Method replacements:** -- `Environment.GetEnvironmentVariable(name)` → `TaskEnvironment.GetEnvironmentVariable(name)` -- `Environment.SetEnvironmentVariable(name, value)` → `TaskEnvironment.SetEnvironmentVariable(name, value)` -- `Path.GetFullPath(path)` → `TaskEnvironment.GetAbsolutePath(path)` - -**Not fixable (too complex):** -- `Process.Start(...)` - Requires multi-statement refactoring to use `GetProcessStartInfo()` -- `ProcessStartInfo` constructors - Requires multi-statement refactoring -- `Environment.SetEnvironmentVariable(name, value, target)` - Target parameter must be manually removed - -**Examples:** ```csharp -// Before: -string dir = Environment.CurrentDirectory; -string value = Environment.GetEnvironmentVariable("PATH"); -string full = Path.GetFullPath("file.txt"); - -// After: -string dir = TaskEnvironment.ProjectCurrentDirectory; -string value = TaskEnvironment.GetEnvironmentVariable("PATH"); -string full = TaskEnvironment.GetAbsolutePath("file.txt"); +- File.Exists(somePath) ++ File.Exists(TaskEnvironment.GetAbsolutePath(somePath)) ``` -### 3.4 User Experience +### 3.3 User Experience -- Fixes appear in Visual Studio Quick Actions (Ctrl+.) -- Action titles: - - "Wrap with TaskEnvironment.GetAbsolutePath()" (MSB9997) - - "Use TaskEnvironment.ProjectCurrentDirectory" (MSB9998) - - "Use TaskEnvironment.GetEnvironmentVariable()" (MSB9998) - - "Use TaskEnvironment.SetEnvironmentVariable()" (MSB9998) - - "Use TaskEnvironment.GetAbsolutePath()" (MSB9998 for Path.GetFullPath) -- Available immediately when warnings appear +**IDE**: Quick Actions (Ctrl+.) with "Fix All" support +**CLI**: `dotnet format analyzers --diagnostics MSB9997 MSB9998` --- -## 4. Open Questions for Review - -### 4.1 Distribution Model (Proposal) +## 4. Open Questions -**Recommended**: Ship analyzer with `Microsoft.Build.Utilities.Core` NuGet package +### 4.1 Distribution -**Pros**: -- Task authors already reference this package -- Zero configuration required -- Analyzer version stays synchronized with APIs -- Automatic updates +**Proposal**: Ship with `Microsoft.Build.Utilities.Core` NuGet package -**Cons**: -- Increases package size -- Cannot be disabled without disabling entire analyzer infrastructure +**Pros**: Already referenced, zero config, automatic updates +**Cons**: Increases package size -**Alternative Considered**: Standalone `Microsoft.Build.Analyzers` package -- **Rejected**: Discovery problem, extra manual step reduces adoption +**Question**: Consensus on Utilities.Core vs standalone package? -**Question**: Do we have consensus on shipping with Utilities.Core? +### 4.2 Severity Levels -### 4.2 Default Severity Levels +**Proposal**: +- MSB9999: Error (no alternative exists) +- MSB9998: Warning (migration path available) +- MSB9997: Warning +- MSB9996: Warning -**Current Proposal**: -- **MSB9999**: Error (no safe alternative - Environment.Exit, Process.Kill, ThreadPool settings, CultureInfo defaults) -- **MSB9998**: Warning (has TaskEnvironment alternative - Environment.CurrentDirectory, Get/SetEnvironmentVariable, Path.GetFullPath, Process.Start) -- **MSB9997**: Warning (conditional safety - file path methods) -- **MSB9996**: Warning (informational - Console, Assembly.Load) +**Question**: Should MSB9998 become Error in future release? -**Rationale for MSB9999 as Error**: These APIs have no acceptable workaround and would cause serious issues in multithreaded builds (process termination, process-wide state corruption). +### 4.3 Opt-Out -**Rationale for MSB9998 as Warning**: While these APIs need migration, they have clear TaskEnvironment alternatives and code fixers to ease transition. Warning severity allows gradual adoption. +**Proposal**: `false` -**Question**: Should MSB9998 be elevated to Error in a future release after adoption period? +**Question**: Property name OK? Or only `.editorconfig` support? -### 4.3 Opt-Out Mechanism +### 4.4 Scope -**Proposal**: MSBuild property `EnableMSBuildThreadSafetyAnalyzer` - -```xml - - false - -``` +**Proposal**: Only analyze `IMultiThreadableTask` implementations -**Question**: Is property name acceptable? Should we support per-diagnostic severity via `.editorconfig` only? - -### 4.4 Scope of Analysis - -**Proposal**: Analyzer only activates within types implementing `IMultiThreadableTask` - -**Alternative**: Analyze all code -- **Rejected**: Creates noise for non-multithreadable tasks - -**Question**: Should we offer opt-in to analyze all Task types (not just IMultiThreadableTask)? +**Question**: Offer opt-in for all Task types? --- -## 5. Testing Approach - -### 5.1 Validation - -A demo project demonstrates analyzer behavior: -- **ProblematicTask**: Task with 9 unsafe API usages → expects mixture of MSB9999/MSB9998/MSB9997/MSB9996 warnings -- **CorrectTask**: Same logic using wrapped paths → expects 0 warnings +## 5. Testing -**Location**: `src/ThreadSafeTaskAnalyzer/VisualStudioDemo/` +**Demo**: `src/ThreadSafeTaskAnalyzer/VisualStudioDemo/` +- `ProblematicTask`: 13 diagnostics (2 errors, 11 warnings) +- `CorrectTask`: 0 diagnostics -### 5.2 Manual Testing - -1. Open demo in Visual Studio -2. Observe diagnostics on unsafe API usages (squiggles based on severity) -3. Verify Quick Action offers "Wrap with..." fix for MSB9997 only -4. Apply fix and confirm warning disappears +**Validation**: Open in VS, test fixers via Ctrl+., or `dotnet build` --- -## 6. Design Rationale - -### 6.1 Why Two Detection Modes? - -**Always-banned APIs** require explicit symbol matching because: -- No safe usage pattern exists (e.g., `Environment.Exit` always wrong) -- Need precise, actionable error messages +## 6. Design Decisions -**Conditionally-banned APIs** use pattern detection because: -- Safe when used correctly (with absolute paths) -- Hundreds of methods across file system types - listing all is unmaintainable +**Detection strategies**: +- Symbol matching (MSB9999/98/96): Exact identification for always-unsafe APIs +- Pattern matching (MSB9997): Type-based for conditionally-unsafe APIs -### 6.2 Why Check Only First String Parameter? +**First-parameter heuristic**: BCL file APIs consistently put path first. Simple and fast. -.NET file system APIs follow consistent pattern: -```csharp -File.ReadAllText(string path) // path is first param -Directory.CreateDirectory(string path) // path is first param -new FileStream(string path, FileMode mode) // path is first param -``` +**No data-flow analysis**: Minimizes false positives by detecting actual problematic API calls, not guessing about data. Path analysis would be complex, hurt performance, and still couldn't handle runtime values. -**Tradeoff**: Simplicity and performance vs. handling rare edge cases. False negatives possible for unusual APIs, but unlikely in practice. - -### 6.3 Why Not Full Data-Flow Analysis? - -**Rejected**: Tracking whether variables contain absolute paths via data-flow analysis - -**Reasons**: -- Extremely complex for limited benefit -- Performance impact on live editing -- Many code paths are unknowable at compile time (user input, config files, etc.) - -**Chosen approach**: Conservative warnings + developer suppressions for known-safe cases - ---- - -## 7. Future Enhancements (Out of Scope for v1) - -1. **Constant analysis**: Suppress warnings for string literal absolute paths (`"C:\\Windows"`) -2. **Data-flow tracking**: Recognize when variable provably contains absolute path -3. **Strict mode**: Warn on all path operations, even when wrapped -4. **VB.NET support**: Currently C# only -5. **Additional banned APIs**: Console.SetOut, AppDomain.SetData, etc. -6. **Build-time enforcement**: MSBuild task that fails build on violations --- ## 8. Known Limitations -1. **String literals**: Warns on `File.Exists("C:\\absolute\\path")` even though safe -2. **Method chaining**: May not recognize `GetAbsolutePath()` in complex expressions -3. **Reflection**: Cannot detect dynamic API calls via reflection -4. **Code generation**: Does not analyze dynamically generated code -5. **Performance**: No specific performance optimizations yet implemented - -These are acceptable for initial version. Can be addressed based on user feedback. +1. Warns on absolute path literals (`"C:\\Windows"`) +2. May miss `GetAbsolutePath()` in complex expressions +3. Cannot detect reflection-based API calls +4. No analysis of generated code --- ## 9. References -Full PR: [https://github.com/dotnet/msbuild/pull/12143](https://github.com/dotnet/msbuild/pull/12143) -- [thread-safe-tasks-api-analysis.md](thread-safe-tasks-api-analysis.md) - Complete list of banned APIs +**Full PR**: https://github.com/dotnet/msbuild/pull/12143 + +- [thread-safe-tasks-api-analysis.md](thread-safe-tasks-api-analysis.md) - Complete API list - [thread-safe-tasks.md](thread-safe-tasks.md) - Thread-safe tasks overview -- [multithreaded-msbuild.md](multithreaded-msbuild.md) - Multithreaded MSBuild specification -- Demo implementation: `src/ThreadSafeTaskAnalyzer/` -- Sample usage: `src/ThreadSafeTaskAnalyzer/VisualStudioDemo/` +- [multithreaded-msbuild.md](multithreaded-msbuild.md) - Multithreaded MSBuild spec +- Demo: `src/ThreadSafeTaskAnalyzer/VisualStudioDemo/`