Add unification plan document

documentation/project-docs/msbuild-api-usage-unification.md

@@ -0,0 +1,384 @@
+# MSBuild API Usage Unification Plan
+
+## Overview
+
+This document outlines the plan to unify and improve MSBuild API usage across the dotnet-sdk codebase by replacing direct MSBuild API access with purpose-built wrapper types that enforce best practices, telemetry integration, and evaluation caching.
+
+## Current State Analysis
+
+### MSBuild API Usage Patterns
+
+Based on comprehensive codebase analysis, MSBuild APIs are used in the following patterns:
+
+#### 1. **Project Property Evaluation** (~60% of usage)
+Reading project properties and metadata without building:
+- Target framework detection (`TargetFramework`, `TargetFrameworks`)
+- Configuration analysis (`Configuration`, `Platform`, `OutputType`)
+- Path resolution (`OutputPath`, `ProjectAssetsFile`, `MSBuildProjectFullPath`)
+- Feature detection (container support, workload requirements, package management)
+
+#### 2. **Project Item Collection Analysis** (~20% of usage)
+Inspecting project items and references:
+- Dependency analysis (`PackageReference`, `ProjectReference`)
+- Asset discovery (`Content`, `EmbeddedResource`, `Compile`)
+- Container configuration (`ContainerLabel`, `ContainerEnvironmentVariable`)
+- Workload requirements analysis
+
+#### 3. **Build Target Execution** (~10% of usage)
+Running specific MSBuild targets with telemetry:
+- Workload analysis (`_GetRequiredWorkloads`)
+- Dependency graph generation (`GenerateRestoreGraphFile`)
+- Run preparation (`_GetRunCommand`)
+- Container operations
+
+#### 4. **Solution and Multi-Project Operations** (~5% of usage)
+Managing collections of projects:
+- Solution analysis and cross-project operations
+- Reference management (add/remove project references)
+- Bulk operations across multiple projects
+
+#### 5. **Command-Specific Scenarios** (~5% of usage)
+- `dotnet run`: Project executability checks and launch configuration
+- Package commands: PackageReference analysis and central package management
+- Reference commands: ProjectReference management and validation
+- Workload commands: Project requirement analysis
+
+### Current Problems
+
+#### 1. **Evaluation Caching Anti-Patterns**
+- **Direct ProjectInstance Creation**: Many locations use `new ProjectInstance(projectFile, globalProperties, null)` instead of leveraging ProjectCollection caching
+- **Short-lived ProjectCollection Pattern**: Most commands create new ProjectCollection per operation with immediate disposal
+- **Inconsistent Global Properties**: Multiple construction points create variations that reduce cache hit rates
+
+#### 2. **Telemetry Integration Issues**
+- Manual telemetry logger setup required at each usage site
+- Risk of forgetting telemetry integration in new code
+- Complex distributed logging setup for build scenarios
+
+#### 3. **Resource Management Complexity**
+- ProjectCollection disposal requirements not always properly handled
+- Memory leaks possible with improper lifecycle management
+- No centralized resource management strategy
+
+#### 4. **API Misuse Potential**
+- Direct access to MSBuild APIs allows bypassing best practices
+- No enforcement of telemetry integration
+- Inconsistent error handling patterns
+
+## Global Properties Analysis
+
+### Common Global Properties Used
+
+**Core Properties (from MSBuildPropertyNames):**
+- `Configuration` - Release/Debug configuration
+- `TargetFramework` - Specific target framework for evaluation
+- `TargetFrameworks` - Multi-targeting scenarios
+- `PublishRelease`/`PackRelease` - Release optimization properties
+
+**Restore-Specific Properties:**
+```csharp
+{ "EnableDefaultCompileItems", "false" },
+{ "EnableDefaultEmbeddedResourceItems", "false" },
+{ "EnableDefaultNoneItems", "false" },
+{ "MSBuildRestoreSessionId", Guid.NewGuid().ToString("D") },
+{ "MSBuildIsRestoring", "true" }
+```
+
+**Runtime Properties:**
+- `DOTNET_HOST_PATH` - Always set to current host path
+- User-specified properties from command line (`-p:Property=Value`)
+
+**Virtual Project Properties:**
+```csharp
+{ "_BuildNonexistentProjectsByDefault", "true" },
+{ "RestoreUseSkipNonexistentTargets", "false" },
+{ "ProvideCommandLineArgs", "true" }
+```
+
+### Global Properties Construction Patterns
+
+- **`CommonRunHelpers.GetGlobalPropertiesFromArgs`** - Most common pattern for run/test/build commands
+- **`ReleasePropertyProjectLocator.InjectTargetFrameworkIntoGlobalProperties`** - Framework option handling
+- **Command-specific constructors** - Various specialized property sets
+
+## Existing Caching Mechanisms
+
+### VirtualProjectBuildingCommand (Advanced Example)
+- JSON-based cache with `CacheContext.GetCacheKey()`
+- Cache keys include: global properties, SDK version, runtime version, directives, implicit build files
+- File-based cache with timestamp validation
+- Sophisticated invalidation based on file changes and version mismatches
+
+### MSBuildEvaluator (Simple Example)
+- In-memory caching based on `ProjectCollection` instance
+- Single command execution lifetime scope
+- Used for template engine evaluations
+
+### Current Limitations
+- Most evaluation scenarios don't benefit from caching
+- ProjectCollection instances are short-lived
+- No sharing of evaluation results across similar operations
+
+## Proposed Architecture
+
+### Wrapper Type Design
+
+#### 1. **DotNetProjectEvaluator** (Evaluation-Only Scenarios)
+**Purpose**: Manages project evaluation with caching and consistent global properties
+
+```csharp
+public sealed class DotNetProjectEvaluator : IDisposable
+{
+    // Configuration
+    public DotNetProjectEvaluator(IDictionary<string, string>? globalProperties = null,
+                                  IEnumerable<ILogger>? loggers = null);
+
+    // Core evaluation methods
+    public DotNetProject LoadProject(string projectPath);
+    public DotNetProject LoadProject(string projectPath, IDictionary<string, string>? additionalGlobalProperties);
+
+    // Batch operations for solutions
+    public IEnumerable<DotNetProject> LoadProjects(IEnumerable<string> projectPaths);
+
+    // Resource management
+    public void Dispose();
+}
+```
+
+**Features**:
+- Manages ProjectCollection lifecycle with proper disposal
+- Automatic telemetry logger integration
+- Evaluation result caching within evaluator instance
+- Consistent global property management
+- Thread-safe for concurrent evaluations
+
+#### 2. **DotNetProject** (Project Wrapper)
+**Purpose**: Provides typed access to project properties and items
+
+```csharp
+public sealed class DotNetProject
+{
+    // Basic properties
+    public string FullPath { get; }
+    public string Directory { get; }
+
+    // Strongly-typed common properties
+    public string? TargetFramework => GetPropertyValue("TargetFramework");
+    public string[] TargetFrameworks => GetPropertyValue("TargetFrameworks")?.Split(';') ?? Array.Empty<string>();
+    public string Configuration => GetPropertyValue("Configuration") ?? "Debug";
+    public string Platform => GetPropertyValue("Platform") ?? "AnyCPU";
+    public string OutputType => GetPropertyValue("OutputType") ?? "";
+    public string? OutputPath => GetPropertyValue("OutputPath");
+
+    // Generic property access
+    public string? GetPropertyValue(string propertyName);
+    public IEnumerable<string> GetPropertyValues(string propertyName); // For multi-value properties
+
+    // Item access
+    public IEnumerable<DotNetProjectItem> GetItems(string itemType);
+    public IEnumerable<DotNetProjectItem> GetItems(string itemType, Func<DotNetProjectItem, bool> predicate);
+
+    // Convenience methods
+    public IEnumerable<string> GetConfigurations();
+    public IEnumerable<string> GetPlatforms();
+    public string GetProjectId();
+    public string? GetDefaultProjectTypeGuid();
+
+    // No public constructor - created by DotNetProjectEvaluator
+}
+```
+
+#### 3. **DotNetProjectBuilder** (Build Execution)
+**Purpose**: Handles target execution with telemetry integration
+
+```csharp
+public sealed class DotNetProjectBuilder
+{
+    public DotNetProjectBuilder(DotNetProject project, ILogger? telemetryCentralLogger = null);
+
+    // Build operations
+    public BuildResult Build(params string[] targets);
+    public BuildResult Build(string[] targets, IEnumerable<ILogger>? additionalLoggers);
+    public BuildResult Build(string[] targets, out IDictionary<string, TargetResult> targetOutputs);
+
+    // Advanced build with custom remote loggers
+    public BuildResult Build(string[] targets,
+                           IEnumerable<ILogger>? loggers,
+                           IEnumerable<ForwardingLoggerRecord>? remoteLoggers,
+                           out IDictionary<string, TargetResult> targetOutputs);
+}
+
+public record BuildResult(bool Success, IDictionary<string, TargetResult>? TargetOutputs = null);
+```
+
+#### 4. **DotNetProjectItem** (Item Wrapper)
+**Purpose**: Provides typed access to project items
+
+```csharp
+public sealed class DotNetProjectItem
+{
+    public string ItemType { get; }
+    public string EvaluatedInclude { get; }
+    public string UnevaluatedInclude { get; }
+
+    public string? GetMetadataValue(string metadataName);
+    public IEnumerable<string> GetMetadataNames();
+    public IDictionary<string, string> GetMetadata();
+}
+```
+
+### Factory and Configuration
+
+#### DotNetProjectEvaluatorFactory
+```csharp
+public static class DotNetProjectEvaluatorFactory
+{
+    // Standard configurations
+    public static DotNetProjectEvaluator CreateForCommand(MSBuildArgs? args = null);
+    public static DotNetProjectEvaluator CreateForRestore();
+    public static DotNetProjectEvaluator CreateForWorkloadAnalysis();
+
+    // Custom configuration
+    public static DotNetProjectEvaluator Create(IDictionary<string, string>? globalProperties = null,
+                                               IEnumerable<ILogger>? loggers = null);
+}
+```
+
+### Integration with Existing Telemetry
+
+The wrapper types will integrate with the existing telemetry infrastructure from `ProjectInstanceExtensions.cs`:
+
+- **Central Logger Creation**: `CreateTelemetryCentralLogger()` → Used internally by wrappers
+- **Distributed Logging**: `CreateTelemetryForwardingLoggerRecords()` → Used by `DotNetProjectBuilder`
+- **Logger Management**: `CreateLoggersWithTelemetry()` → Used by `DotNetProjectEvaluator`
+
+### BannedApiAnalyzer Integration
+
+Update `BannedSymbols.txt` to restrict direct MSBuild API usage:
+
+```
+# Direct ProjectInstance creation - use DotNetProjectEvaluator instead
+T:Microsoft.Build.Execution.ProjectInstance.#ctor(System.String,System.Collections.Generic.IDictionary{System.String,System.String},System.String)~Use DotNetProjectEvaluator.LoadProject instead
+
+# Direct ProjectCollection creation without telemetry - use DotNetProjectEvaluatorFactory
+T:Microsoft.Build.Evaluation.ProjectCollection.#ctor()~Use DotNetProjectEvaluatorFactory.Create instead
+T:Microsoft.Build.Evaluation.ProjectCollection.#ctor(System.Collections.Generic.IDictionary{System.String,System.String})~Use DotNetProjectEvaluatorFactory.Create instead
+
+# Direct Project.Build calls - use DotNetProjectBuilder
+M:Microsoft.Build.Execution.ProjectInstance.Build()~Use DotNetProjectBuilder.Build instead
+M:Microsoft.Build.Execution.ProjectInstance.Build(System.String[])~Use DotNetProjectBuilder.Build instead
+M:Microsoft.Build.Execution.ProjectInstance.Build(System.String[],System.Collections.Generic.IEnumerable{Microsoft.Build.Framework.ILogger})~Use DotNetProjectBuilder.Build instead
+
+# Direct Evaluation.Project usage - use DotNetProjectEvaluator
+T:Microsoft.Build.Evaluation.Project~Use DotNetProjectEvaluator and DotNetProject instead
+```
+
+## Migration Strategy
+
+### Phase 1: Create Wrapper Infrastructure
+1. Implement core wrapper types (`DotNetProjectEvaluator`, `DotNetProject`, `DotNetProjectBuilder`, `DotNetProjectItem`)
+2. Create factory class with standard configurations
+3. Add comprehensive unit tests
+4. Migrate telemetry integration from `ProjectInstanceExtensions.cs`
+
+### Phase 2: Update High-Impact Usage Sites
+1. **MSBuildEvaluator** - Replace with `DotNetProjectEvaluator`
+2. **ReleasePropertyProjectLocator** - Use cached evaluation
+3. **CommonRunHelpers** - Standardize global property construction
+4. **Solution processing** - Leverage batch evaluation capabilities
+
+### Phase 3: Command Integration
+1. **Run Command** - Update project analysis and launch preparation
+2. **Package Commands** - Replace PackageReference analysis
+3. **Reference Commands** - Update ProjectReference management
+4. **Workload Commands** - Replace requirement analysis
+5. **Test Command** - Update project discovery
+
+### Phase 4: Enforcement and Cleanup
+1. Update `BannedSymbols.txt` with new restrictions
+2. Remove deprecated `ProjectInstanceExtensions` methods
+3. Update remaining usage sites
+4. Add analyzer rules for proper usage patterns
+
+### Phase 5: Test Infrastructure
+1. Evaluate test-specific needs - may allow direct MSBuild API usage for testing implementations
+2. Create test helpers using wrapper types
+3. Update integration tests
+
+## Benefits
+
+### Performance Improvements
+- **Evaluation Caching**: Reuse ProjectCollection across multiple project evaluations
+- **Global Property Consistency**: Better cache hit rates through standardized properties
+- **Resource Management**: Proper lifecycle management reduces memory pressure
+
+### Code Quality
+- **Type Safety**: Strongly-typed access to common properties and items
+- **Error Reduction**: Consistent error handling and resource management
+- **Telemetry Integration**: Automatic telemetry without manual setup
+
+### Maintainability
+- **Centralized Logic**: All MSBuild interaction through controlled interfaces
+- **Best Practices**: Enforced through wrapper design and BannedApiAnalyzer
+- **Documentation**: Clear usage patterns and examples
+
+### Developer Experience
+- **Simplified API**: Higher-level abstractions for common scenarios
+- **IntelliSense**: Better discovery of available properties and items
+- **Consistency**: Uniform patterns across all commands
+
+## Implementation Notes
+
+### Backward Compatibility
+- `ProjectInstanceExtensions` methods can be marked `[Obsolete]` initially
+- Gradual migration allows testing and validation of new types
+- Test infrastructure may retain direct MSBuild API access temporarily
+
+### Performance Considerations
+- Wrapper overhead should be minimal - mostly delegation to underlying MSBuild types
+- Caching benefits should outweigh abstraction costs
+- Benchmark critical paths (evaluation-heavy scenarios)
+
+### Error Handling
+- Consistent exception handling across all wrapper types
+- Graceful degradation when telemetry fails
+- Clear error messages for common MSBuild issues
+
+### Thread Safety
+- `DotNetProjectEvaluator` should support concurrent project loading
+- Individual `DotNetProject` instances are read-only after creation
+- `DotNetProjectBuilder` instances are not thread-safe (by design)
+
+## Future Enhancements
+
+### Advanced Caching
+- Persistent evaluation cache (like `VirtualProjectBuildingCommand`)
+- Cross-session cache with invalidation based on file timestamps
+- Distributed cache for CI scenarios
+
+### Performance Monitoring
+- Telemetry for evaluation cache hit rates
+- Performance metrics for wrapper overhead
+- MSBuild API usage patterns analysis
+
+### Additional Wrappers
+- Solution-level operations wrapper
+- NuGet-specific project analysis wrapper
+- Template evaluation wrapper
+
+## Related Work
+
+This plan builds upon existing work in the codebase:
+
+- **PR #51068**: MSBuild telemetry integration and `ProjectInstanceExtensions.cs`
+- **VirtualProjectBuildingCommand**: Advanced caching patterns
+- **MSBuildEvaluator**: Existing evaluation abstraction
+- **BannedApiAnalyzer**: API usage enforcement infrastructure
+
+## Success Criteria
+
+1. **Telemetry Integration**: 100% of MSBuild API usage includes telemetry
+2. **Performance**: Evaluation-heavy scenarios show measurable performance improvement
+3. **Code Quality**: Reduced complexity in command implementations
+4. **Compliance**: BannedApiAnalyzer prevents direct MSBuild API usage in new code
+5. **Test Coverage**: Comprehensive tests for all wrapper functionality