Improve method overload resolution performance by 27% with arity filtering, type pre-checks, lazy evaluation, and caching

.github/skills/pr-build-status/SKILL.md

@@ -1,62 +1,177 @@
 ---
 name: pr-build-status
-description: "Retrieve Azure DevOps build information for GitHub Pull Requests, including build IDs, stage status, and failed jobs."
+description: "Retrieve and analyze Azure DevOps build failures for GitHub PRs. Use when CI fails. CRITICAL: Collect ALL errors from ALL platforms FIRST, write hypotheses to file, then fix systematically."
 metadata:
   author: dotnet-maui
-  version: "1.0"
+  version: "2.0"
 compatibility: Requires GitHub CLI (gh) authenticated with access to dotnet/fsharp repository.
 ---
 
 # PR Build Status Skill
 
-Retrieve Azure DevOps build information for GitHub Pull Requests.
+Retrieve and systematically analyze Azure DevOps build failures for GitHub PRs.
 
-## Tools Required
+## CRITICAL: Collect-First Workflow
 
-This skill uses `bash` together with `pwsh` (PowerShell 7+) to run the PowerShell scripts. No file editing or other tools are required.
+**DO NOT push fixes until ALL errors are collected and reproduced locally.**
 
-## When to Use
+LLMs tend to focus on the first error found and ignore others. This causes:
+- Multiple push/wait/fail cycles
+- CI results being overwritten before full analysis
+- Missing platform-specific failures (Linux vs Windows vs MacOS)
 
-- User asks about CI/CD status for a PR
-- User asks about failed checks or builds
-- User asks "what's failing on PR #XXXXX"
-- User wants to see test results
+### Mandatory Workflow
+
+```
+1. COLLECT ALL     → Get errors from ALL jobs across ALL platforms
+2. DOCUMENT        → Write CI_ERRORS.md with hypotheses per platform
+3. REPRODUCE       → Run each failing test LOCALLY (in isolation!)
+4. FIX             → Fix each issue, verify locally
+5. PUSH            → Only after ALL issues verified fixed
+```
 
 ## Scripts
 
 All scripts are in `.github/skills/pr-build-status/scripts/`
 
 ### 1. Get Build IDs for a PR
-```bash
+```powershell
 pwsh .github/skills/pr-build-status/scripts/Get-PrBuildIds.ps1 -PrNumber <PR_NUMBER>
 ```
 
-### 2. Get Build Status
-```bash
+### 2. Get Build Status (List ALL Failed Jobs)
+```powershell
+# Get overview of all stages and jobs
 pwsh .github/skills/pr-build-status/scripts/Get-BuildInfo.ps1 -BuildId <BUILD_ID>
-# For failed jobs only:
+
+# Get ONLY failed jobs (use this to see all failing platforms)
 pwsh .github/skills/pr-build-status/scripts/Get-BuildInfo.ps1 -BuildId <BUILD_ID> -FailedOnly
 ```
 
 ### 3. Get Build Errors and Test Failures
-```bash
-# Get all errors (build errors + test failures)
+```powershell
+# Get ALL errors (build errors + test failures) - USE THIS FIRST
 pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId <BUILD_ID>
 
-# Get only build/compilation errors
-pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId <BUILD_ID> -ErrorsOnly
+# Filter to specific job (after getting overview)
+pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId <BUILD_ID> -JobFilter "*Linux*"
+pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId <BUILD_ID> -JobFilter "*Windows*"
+pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId <BUILD_ID> -JobFilter "*MacOS*"
+```
+
+### 4. Direct API Access (for detailed logs)
+```powershell
+# Get timeline with all jobs
+$uri = "https://dev.azure.com/dnceng-public/public/_apis/build/builds/<BUILD_ID>/timeline?api-version=7.1"
+Invoke-RestMethod -Uri $uri | Select-Object -ExpandProperty records | Where-Object { $_.result -eq "failed" }
 
-# Get only test failures
-pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId <BUILD_ID> -TestsOnly
+# Get specific log content
+$logUri = "https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_apis/build/builds/<BUILD_ID>/logs/<LOG_ID>"
+Invoke-RestMethod -Uri $logUri | Select-String "Failed|Error|FAIL"
 ```
 
-## Workflow
+## Step-by-Step Analysis Procedure
+
+### Step 1: Get Failed Build ID
+```powershell
+pwsh .github/skills/pr-build-status/scripts/Get-PrBuildIds.ps1 -PrNumber XXXXX
+# Note the BuildId with FAILED state
+```
 
-1. Get build IDs: `scripts/Get-PrBuildIds.ps1 -PrNumber XXXXX`
-2. For each build, get status: `scripts/Get-BuildInfo.ps1 -BuildId YYYYY`
-3. For failed builds, get error details: `scripts/Get-BuildErrors.ps1 -BuildId YYYYY`
+### Step 2: List ALL Failed Jobs (Cross-Platform!)
+```powershell
+pwsh .github/skills/pr-build-status/scripts/Get-BuildInfo.ps1 -BuildId YYYYY -FailedOnly
+```
+**IMPORTANT**: Note jobs from EACH platform:
+- Linux jobs
+- Windows jobs  
+- MacOS jobs
+- Different test configurations (net10.0 vs net472, etc.)
+
+### Step 3: Get Errors Per Platform
+```powershell
+# Collect errors from EACH platform separately
+pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId YYYYY -JobFilter "*Linux*"
+pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId YYYYY -JobFilter "*Windows*"
+pwsh .github/skills/pr-build-status/scripts/Get-BuildErrors.ps1 -BuildId YYYYY -JobFilter "*MacOS*"
+```
+
+### Step 4: Write CI_ERRORS.md
+Create a file in session workspace with ALL findings:
+```markdown
+# CI Errors for PR #XXXXX - Build YYYYY
+
+## Failed Jobs Summary
+| Platform | Job Name | Error Type |
+|----------|----------|------------|
+| Linux    | ...      | Test       |
+| Windows  | ...      | Test       |
+
+## Hypothesis Per Platform
+
+### Linux/MacOS Failures
+- Error: "The type 'int' is not defined"
+- Hypothesis: Missing FSharp.Core reference in test setup
+- Reproduction: `dotnet test ... -f net10.0`
+
+### Windows Failures  
+- Error: "Expected cache hits for generic patterns"
+- Hypothesis: Flaky test assertion, passes with other tests
+- Reproduction: `dotnet test ... --filter "FullyQualifiedName~rigid generic"`
+
+## Reproduction Commands
+...
+
+## Fix Verification Checklist
+- [ ] Linux error reproduced locally
+- [ ] Windows error reproduced locally
+- [ ] Fix verified for Linux
+- [ ] Fix verified for Windows
+- [ ] Tests run IN ISOLATION (not just with other tests)
+```
+
+### Step 5: Reproduce Locally BEFORE Fixing
+```powershell
+# Run failing tests IN ISOLATION (critical!)
+dotnet test ... --filter "FullyQualifiedName~FailingTestName" -f net10.0
+
+# Run multiple times to check for flakiness
+for ($i = 1; $i -le 3; $i++) { dotnet test ... }
+```
+
+### Step 6: Fix and Verify
+Only after ALL issues reproduced:
+1. Fix each issue
+2. Verify each fix locally (run test in isolation!)
+3. Run full test suite
+4. Check formatting
+5. THEN push
+
+## Common Pitfalls
+
+### ❌ Mistake: Focus on First Error Only
+```
+See Linux error → Fix → Push → Wait → See Windows error → Fix → Push → ...
+```
+
+### ✅ Correct: Collect All First
+```
+See Linux error → See Windows error → See MacOS error → Document all → 
+Fix all → Verify all locally → Push once
+```
+
+### ❌ Mistake: Run Tests Together
+```
+dotnet test ... --filter "OverloadCacheTests"  # All 8 pass together
+```
+
+### ✅ Correct: Run Tests in Isolation
+```
+dotnet test ... --filter "FullyQualifiedName~specific test name"  # May fail alone!
+```
 
 ## Prerequisites
 
 - `gh` (GitHub CLI) - authenticated
-- `pwsh` (PowerShell 7+)
+- `pwsh` (PowerShell 7+)
+- Local build environment matching CI

.ralph/CONTEXT.md

@@ -0,0 +1,83 @@
+# Product Increments
+
+This file is updated after each sprint completes. Use it to understand what was delivered.
+
+---
+
+## Sprint 1: Extract storeCacheResult helper
+
+**Summary:** Completed in 2 iterations
+
+**Files touched:** Check git log for details.
+
+---
+
+## Sprint 2: Cache lookup optimization
+
+**Summary:** Modified `storeCacheResult` to accept the cache directly instead of TcGlobals, eliminating 4 redundant `getOverloadResolutionCache` calls. Updated:
+- `storeCacheResult` signature: now takes `cache` instead of `g: TcGlobals`
+- `ResolveOverloadingCore` signature: added `cache` parameter
+- `GetMostApplicableOverload` signature: added `cache` parameter
+- All 4 call sites pass the cache captured at line 3820
+
+**DoD verification:**
+- Build: 0 errors, 0 warnings
+- Tests: 8 OverloadCache tests passed
+- Formatting: fantomas --check passes
+
+**Files touched:** src/Compiler/Checking/ConstraintSolver.fs
+
+---
+
+## Sprint 3: ResizeArray pattern fix
+
+**Summary:** Replaced mutable list + prepend + List.rev pattern with ResizeArray in `tryComputeOverloadCacheKey` (lines 476-551):
+- Changed `objArgStructures` from `mutable list` to `ResizeArray()`
+- Changed `argStructures` from `mutable list` to `ResizeArray()`
+- Use `.Add()` instead of prepending with `::`
+- Use `Seq.toList` instead of `List.rev` for conversion
+
+**DoD verification:**
+- Build: 0 errors, 0 warnings
+- Tests: 8 OverloadCache tests passed (all frameworks)
+- Formatting: fantomas --check passes
+- List.rev calls eliminated
+
+**Files touched:** src/Compiler/Checking/ConstraintSolver.fs
+
+---
+
+## Sprint 4: ProvidedMeth hash improvement
+
+**Summary:** Improved hash computation for ProvidedMeth in `computeMethInfoHash` (line 448):
+- Changed from hashing only the method name to hashing both method name AND declaring type FullName
+- Before: `hash (mb.PUntaint((fun m -> m.Name), range0))`
+- After: `hash (mb.PUntaint((fun m -> m.Name, (nonNull<ProvidedType> m.DeclaringType).FullName |> string), range0))`
+- This prevents cache collisions between same-named methods in different type providers
+
+**DoD verification:**
+- Build: 0 errors, 0 warnings
+- Tests: 8 OverloadCache tests passed (net10.0: 8 passed, net472: 8 passed)
+- ProvidedMeth case hashes both method name and declaring type
+- No functional change in behavior
+
+**Files touched:** src/Compiler/Checking/ConstraintSolver.fs
+
+---
+
+## Sprint 5: MaxTokenCount constant
+
+**Summary:** Defined `[<Literal>] let MaxTokenCount = 256` constant in TypeHashing.fs and replaced all 10 magic number occurrences with it:
+- Added constant definition after line 46 in HashingPrimitives module with doc comment
+- Replaced 256 at lines: 418 (ResizeArray initial capacity), 448, 456, 463, 469, 483, 490, 497, 554, 575
+
+**DoD verification:**
+- Build: 0 errors, 0 warnings
+- Tests: 8 OverloadCache tests passed (net10.0: 8 passed, net472: 8 passed)
+- MaxTokenCount constant defined
+- All 10 magic number 256 occurrences replaced
+- No hardcoded 256 remains in type structure generation (only in constant definition)
+
+**Files touched:** src/Compiler/Utilities/TypeHashing.fs
+
+---

.ralph/VISION.md

@@ -0,0 +1,62 @@
+# VISION: Overload Resolution Cache Quality Cleanup
+
+## High-Level Goal
+
+Refactor the Overload Resolution Cache implementation in the F# compiler to address quality issues identified in `QUALITY_AUDIT.md`. These are cleanup tasks that improve code quality (DRY, performance, maintainability) without changing functionality.
+
+## Key Design Decisions
+
+### 1. Helper Function for Cache Store Pattern ✅ DONE (Sprint 1)
+Extract the duplicated cache-store pattern into a single `storeCacheResult` helper function. This is called from 4 locations in ConstraintSolver.fs and represents a clear DRY violation.
+- **Completed:** Lines 296-315 define `storeCacheResult`, called at lines 3694, 3718, 3745, 4107.
+
+### 2. CachedFailed Handling - INTENTIONAL
+The audit shows `CachedFailed` is stored but effectively ignored on lookup (falls through to re-resolution). After reviewing the code at line 3833-3837, `CachedFailed` IS handled - it triggers re-resolution intentionally (per comment: "to generate proper error messages"). The audit is partially incorrect.
+- **Decision:** No code change needed. The comment already explains the intent.
+
+### 3. Cache Lookup Optimization ✅ DONE (Sprint 2)
+Capture `getOverloadResolutionCache g` once at the start of `ResolveOverloading` and pass cache reference through to `storeCacheResult` and related functions. Avoids 4 redundant WeakMap lookups.
+- **Completed:** `storeCacheResult`, `ResolveOverloadingCore`, and `GetMostApplicableOverload` now accept the cache directly instead of `TcGlobals`. All 4 call sites pass the cache captured at line 3820.
+
+### 4. List Allocation Pattern - REMAINING  
+Replace `mutable list + prepend + List.rev` pattern with `ResizeArray` in `tryComputeOverloadCacheKey` (lines 476-504). Build structures forward instead of prepend + reverse.
+
+### 5. ProvidedMeth Hash Improvement - REMAINING
+Include declaring type in hash for type provider methods (lines 447-448). Currently only hashes method name.
+
+### 6. Magic Number 256 - REMAINING
+Define `[<Literal>] let private MaxTokenCount = 256` in TypeHashing.fs to replace 10+ magic number occurrences (lines 413, 443, 451, 458, 464, 478, 485, 492, 549, 570).
+
+### 7. Code Style: System.Object.ReferenceEquals ✅ DONE (Sprint 1)
+Use `obj.ReferenceEquals` for consistency. Done in `storeCacheResult` helper.
+
+### 8. Test Boilerplate - REMAINING
+Extract `checkSourceHasNoErrors` helper in OverloadCacheTests.fs to reduce repetition across 8+ tests (lines 66, 119, 161, 194, 223, 258, 300, 340).
+
+## Constraints and Gotchas
+
+1. **Safety is paramount**: The cache must NEVER return a wrong overload. Any refactoring must preserve exact semantics.
+2. **No functional changes**: These are pure quality/refactoring changes.
+3. **Bootstrap awareness**: Changes to ConstraintSolver.fs affect compiler bootstrap.
+4. **Formatting**: Must pass `dotnet fantomas . --check` after changes.
+5. **Tests**: All existing tests must pass, especially OverloadCacheTests.
+
+## Lessons Learned
+
+- Sprint 1 successfully extracted `storeCacheResult` helper. Tests pass.
+- Sprint 2 successfully optimized cache lookups - `storeCacheResult` now accepts the cache directly, eliminating 4 redundant `getOverloadResolutionCache` calls.
+- The `CachedFailed` case is properly documented - no action needed.
+
+## Sprint Strategy
+
+Completed:
+- Sprint 1: storeCacheResult helper extraction ✅
+- Sprint 2: Cache lookup optimization (pass cache to storeCacheResult) ✅
+
+Remaining (ordered by priority):
+1. Sprint 3: ResizeArray pattern in tryComputeOverloadCacheKey  
+2. Sprint 4: ProvidedMeth hash to include declaring type
+3. Sprint 5: MaxTokenCount constant in TypeHashing.fs
+4. Sprint 6: Test boilerplate extraction
+
+Each sprint delivers a tested, working increment. Tests are included in each sprint.