reviewing-compiler-prs

# Reviewing Compiler PRs Full dimension definitions and CHECK rules live in the `expert-reviewer` agent. ## When to Invoke - PR touches `src/Compiler/` — invoke the `expert-reviewer` agent - PR touches `src/FSharp.Core/` — focus on FSharp.Core Stability, API Surface, Backward Compat, XML Docs - PR touches `vsintegration/`, `src/Compiler/Service/`, or `src/Compiler/Symbols/` — invoke `@ide-integration`. Do not review those trees here. - PR touches `LanguageServer/` — IDE responsiveness moved to `@ide-integration`. Compiler concurrency stays here. - PR touches `tests/` only — quick check: baselines explained? Cross-TFM coverage? Tests actually assert? - PR touches `eng/` or build scripts — focus on Build Infrastructure, Cross-Platform ## Performance claims For claimed compiler/FCS or generated-program performance improvements, use the [performance-improvement-review agent](../../agents/performance-improvement-review.md) and its [evidence and provenance resources](../../../docs/performance-improvement-review/evidence.md). It supplements normal correctness review; it is not an automatic dispatch for every compiler PR or a reason to launch all dimensions for artifact-only changes. ## Dimension Selection | Files Changed | Focus Dimensions | |---|---| | `Checking/`, `TypedTree/` | Type System, Overload Resolution, Struct Awareness, Feature Gating | | `CodeGen/`, `AbstractIL/` | IL Emission, Debug Experience, Test Coverage | | `Optimize/` | Optimization Correctness, IL Emission, Test Coverage | | `SyntaxTree/`, `pars.fsy` | Parser Integrity, Feature Gating, Typed Tree Discipline | | `TypedTreePickle.*`, `CompilerImports.*` | Binary Compatibility (highest priority) | | `Service/`, `Symbols/` | Read `compiler-service.md` via `@ide-integration`. Do not apply compiler dimensions here. | | `LanguageServer/` | IDE responsiveness: `@ide-integration`. Concurrency stays here. | | `Driver/` | Build Infrastructure, Incremental Checking, Cancellation | | `Facilities/` | Feature Gating, Concurrency | | `FSComp.txt` | Diagnostic Quality | | `FSharp.Core/` | FSharp.Core Stability, Backward Compat, XML Docs, RFC Process | | `vsintegration/` | Read `vsintegration.md` via `@ide-integration`. Memory and JTF: `@vs-integration-perf`. | | `eng/`, `setup/`, build scripts | Build Infrastructure, Cross-Platform | ## Subagent Dispatch For each selected dimension from the table above, the `expert-reviewer` agent MUST launch an independent subagent (background task) to assess that dimension. This is not optional — a single agent doing all dimensions sequentially produces shallow analysis and wall-of-text summaries. Each subagent receives: 1. The dimension's CHECK rules (from expert-reviewer.md) 2. The relevant file diffs (filtered by the dimension's hotspot paths) 3. Instructions to produce a structured finding: `{file, line, severity, dimension, issue, suggestion}` or `LGTM` if no findings The expert-reviewer consolidates subagent results, deduplicates, applies assessment gates, and posts as inline comments per Wave 5. **Assessment gates** — apply before flagging: - Understand execution context before judging (test harness ≠ compiler runtime) - Classify as regression, improvement, or unclear — only regressions are findings - Require a concrete failing scenario — no hypotheticals - "Correct convention" for the context in use → discard, not a finding - "Unexplained" ≠ "wrong" — missing rationale in a commit message is a doc gap, not a defect **Consolidation:** 1. Deduplicate findings at same location 2. Filter: wrong context → discard; improvement → downgrade; speculation → LOW 3. Classify: Behavioral (correctness) → Quality (structure) → Nitpick (style) 4. Rank by cross-model agreement (≥2 models agree = higher confidence) 5. Present Behavioral first; Nitpicks only if nothing higher — agents love producing nitpicks to have *something* to say, deprioritize them ## Self-Review Checklist 1. [ ] Every behavioral change has a test 2. [ ] FSharp.Core changes maintain binary compatibility 3. [ ] No unintended public API surface changes 4. [ ] New language features have a `LanguageFeature` guard and RFC 5. [ ] No raw `TType_*` matching without `stripTyEqns` 6. [ ] Cancellation tokens threaded through async operations 7. [ ] Cleanup changes separate from feature enablement Full dimension CHECK rules are in the `expert-reviewer` agent.