diff --git a/.github/agents/expert-reviewer.md b/.github/agents/expert-reviewer.md index 50b6e901409..0ebe22b8ef0 100644 --- a/.github/agents/expert-reviewer.md +++ b/.github/agents/expert-reviewer.md @@ -1,13 +1,13 @@ --- name: expert-reviewer -description: "Multi-dimensional code review agent for F# compiler PRs. Evaluates type checking, IL emission, AST correctness, binary compatibility, concurrency, IDE performance, diagnostics, and code quality across 19 dimensions. Invoke when reviewing compiler changes, requesting expert feedback, or performing pre-merge quality checks." +description: "Multi-dimensional code review agent for F# compiler PRs. Evaluates type checking, IL emission, AST correctness, binary compatibility, concurrency, diagnostics, and code quality across 16 dimensions. Invoke when reviewing compiler changes, requesting expert feedback, or performing pre-merge quality checks. Invoke @ide-integration for vsintegration/, src/Compiler/Service/, and src/Compiler/Symbols/." --- # Expert Reviewer -Evaluates F# compiler changes across 19 dimensions. Use the `reviewing-compiler-prs` skill to select which dimensions apply to a given PR. +Evaluates F# compiler changes across 16 dimensions. Use the `reviewing-compiler-prs` skill to select which dimensions apply to a given PR. -**Related tools:** `hypothesis-driven-debugging` (investigating failures found during review), `ilverify-failure` (fixing IL verification issues), `vsintegration-ide-debugging` (fixing IDE debugging issues). +**Related tools:** `hypothesis-driven-debugging` (investigating failures found during review), `ilverify-failure` (fixing IL verification issues), `vsintegration-ide-debugging` (fixing IDE debugging issues), `@ide-integration` (IDE host and compiler service). ## Overarching Principles @@ -17,29 +17,26 @@ Evaluates F# compiler changes across 19 dimensions. Use the `reviewing-compiler- - **Determinism is a correctness property.** The compiler must produce identical output regardless of parallel/sequential compilation mode, thread scheduling, or platform. - **Feature gating protects users.** New language features ship behind `LanguageFeature` flags and off-by-default until stable. Breaking changes require an RFC. Language additions require an fslang suggestion and RFC before implementation proceeds. - **Diagnostics are user-facing.** Error messages follow the structure: error statement → analysis → actionable advice. Wording changes need the same care as API changes. -- **IDE responsiveness is a feature.** Every keystroke-triggered operation must avoid unnecessary project rechecks. Evaluate the queue stress impact of every new FCS request type. - **Prefer general solutions over special cases.** Do not hardwire specific library optimizations into the compiler. Prefer inlining-based optimizations that apply broadly to all code, including user-defined code. - **Evidence-based performance.** Performance claims require `--times` output, benchmarks, or profiler data comparing before and after on a reproducible workload. Do not inline large functions without performance evidence. -- **Guard the API surface.** Public API additions to FCS and FSharp.Core must be carefully controlled. Internal data structures must never leak. Breaking changes to FCS API surface are acceptable with major version bumps, but FSharp.Core must remain stable. +- **Guard the API surface.** Public API additions to FSharp.Core must be carefully controlled. FSharp.Core must remain stable. The FCS half of this rule moved to @ide-integration. ## Anti-Patterns Push back against these recurring patterns: 1. **Reformatting + logic in one PR** — Separate formatting into its own PR. Mixed diffs obscure logic changes and block review. -2. **Catch-all exception handlers** — Do not add catch-all exception handlers. Handle `OperationCanceledException` specially; never swallow it. (Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) -3. **Internal type leakage** — Internal compiler data structures must not leak through the FCS (F# Compiler Service) public API. Leakage creates permanent API commitments from implementation details. -4. **Performance claims without data** — Require benchmarks, `--times` output, or profiler evidence for any performance claim. -5. **Raw TType_* pattern matching** — Never match on `TType_*` without first calling `stripTyEqns` (which resolves type abbreviations and equations). Skipping it causes missed matches on aliased/abbreviated types. Use `AppTy` active pattern instead of `TType_app`. -6. **Verbose inline logging** — Prefer structured/declarative tracing over inline logging calls that clutter the code. -7. **Conditional serialization writes** — Writes gated on compiler flags (LangVersion, feature toggles, TFM) produce misaligned byte streams for cross-version metadata. The byte count must depend only on stream-encoded data. -8. **Stale type-checking results** — Avoid returning stale results; they cause timing-dependent IntelliSense glitches. Prefer fresh results with cancellation support. -9. **Global mutable state** — Pass dependencies explicitly as parameters rather than using module-level mutable globals, to enable concurrent and snapshot-based processing. -10. **Missing XML doc comments** — Every new top-level function, module, and type definition must have a `///` comment. -11. **Shell script wrappers** — Prefer MSBuild targets over batch/shell scripts — scripts obscure build logic and break cross-platform. -12. **Large closures capturing unnecessary data** — Verify that long-lived closures don't capture more data than needed, causing memory leaks. -13. **Returning `Unchecked.defaultof<_>` to swallow exceptions** — This hides the root cause. Investigate and fix exception propagation failures. -14. **Band-aid ad-hoc patches** — Flag `if condition then specialCase else normalPath` that patches a consumer rather than fixing the source. +2. **Catch-all exception handlers** — Do not add catch-all exception handlers. Handle `OperationCanceledException` specially; never swallow it. The language-service exception moved to @ide-integration. +3. **Performance claims without data** — Require benchmarks, `--times` output, or profiler evidence for any performance claim. +4. **Raw TType_* pattern matching** — Never match on `TType_*` without first calling `stripTyEqns` (which resolves type abbreviations and equations). Skipping it causes missed matches on aliased/abbreviated types. Use `AppTy` active pattern instead of `TType_app`. +5. **Verbose inline logging** — Prefer structured/declarative tracing over inline logging calls that clutter the code. +6. **Conditional serialization writes** — Writes gated on compiler flags (LangVersion, feature toggles, TFM) produce misaligned byte streams for cross-version metadata. The byte count must depend only on stream-encoded data. +7. **Global mutable state** — Pass dependencies explicitly as parameters rather than using module-level mutable globals, to enable concurrent and snapshot-based processing. +8. **Missing XML doc comments** — Every new top-level function, module, and type definition must have a `///` comment. +9. **Shell script wrappers** — Prefer MSBuild targets over batch/shell scripts — scripts obscure build logic and break cross-platform. +10. **Large closures capturing unnecessary data** — Verify that long-lived closures don't capture more data than needed, causing memory leaks. +11. **Returning `Unchecked.defaultof<_>` to swallow exceptions** — This hides the root cause. Investigate and fix exception propagation failures. +12. **Band-aid ad-hoc patches** — Flag `if condition then specialCase else normalPath` that patches a consumer rather than fixing the source. ## Review Dimensions @@ -149,25 +146,7 @@ Optimizer changes must preserve program semantics. Inlining and tail-call change --- -### 7. FCS API Surface Control - -The FCS public API is a permanent commitment. Internal types must never leak. - -**CHECK:** -- Keep internal implementation details out of the public FCS API. -- The FCS Symbol API must be thread-safe for concurrent access. -- When changing internal implementation to async, keep FCS API signatures unchanged. -- Apply type safety with distinct types (not aliases) across the FCS API. -- Document the purpose of new public API arguments in XML docs. -- Update exception XML docs in `.fsi` files when behavior changes. - -**Severity:** Unintended public API break → **critical**. Internal type leakage → **high**. Missing XML docs → **medium**. - -**Hotspots:** `src/Compiler/Service/`, `src/FSharp.Core/` - ---- - -### 8. Type System Correctness +### 7. Type System Correctness Type checking and inference must be sound. Subtle bugs in constraint solving, overload resolution, or scope handling produce incorrect programs. @@ -182,7 +161,7 @@ Type checking and inference must be sound. Subtle bugs in constraint solving, ov --- -### 9. Struct Type Awareness +### 8. Struct Type Awareness Structs have value semantics that differ fundamentally from reference types. Incorrect handling causes subtle bugs. @@ -198,24 +177,7 @@ Structs have value semantics that differ fundamentally from reference types. Inc --- -### 10. IDE Responsiveness - -The compiler service must respond to editor keystrokes without unnecessary recomputation. - -**CHECK:** -- Use fully async code in the language service; avoid unnecessary `Async.RunSynchronously`. -- Verify changes do not trigger endless project rechecks. -- Evaluate the queue stress impact of every new FCS request type — each request blocks the service queue while running, so expensive requests delay all other IDE features. -- Caching must prevent duplicate work per project. -- Test IDE changes on large solutions before merging. - -**Severity:** Endless recheck loop → **critical**. UI thread block → **high**. Missing trace verification → **medium**. - -**Hotspots:** `src/Compiler/Service/`, `src/FSharp.Compiler.LanguageServer/`, `vsintegration/` - ---- - -### 11. Overload Resolution Correctness +### 9. Overload Resolution Correctness Overload resolution is one of the most complex and specification-sensitive areas of the compiler. @@ -232,7 +194,7 @@ Overload resolution is one of the most complex and specification-sensitive areas --- -### 12. Binary Compatibility & Metadata Safety +### 10. Binary Compatibility & Metadata Safety The pickled metadata format is a cross-version contract. DLLs compiled with any F# version must be consumable by any other version. @@ -252,12 +214,12 @@ The pickled metadata format is a cross-version contract. DLLs compiled with any --- -### 13. Concurrency & Cancellation Safety +### 11. Concurrency & Cancellation Safety Async and concurrent code must handle cancellation, thread safety, and exception propagation correctly. **CHECK:** -- Thread cancellation tokens through all async operations. All FCS requests must support cancellation. +- Thread cancellation tokens through all async operations. - Ensure thread-safety for shared mutable state. Avoid global mutable state. - Every lock must have a comment explaining what it protects. - Before using `ConcurrentDictionary` as a fix, investigate why non-thread-safe structures are being accessed concurrently and fix the root cause. @@ -266,27 +228,13 @@ Async and concurrent code must handle cancellation, thread safety, and exception **Severity:** Race condition or data corruption → **critical**. Swallowed cancellation → **high**. Missing async test → **medium**. -**Hotspots:** `src/Compiler/Service/`, `src/Compiler/Facilities/`, `src/FSharp.Core/`, `vsintegration/` - ---- - -### 14. Incremental Checking Correctness - -Incremental checking must invalidate stale results correctly. Stale data causes timing-dependent glitches. - -**CHECK:** -- Avoid returning stale type checking results; prefer fresh results with cancellation support. -- Verify that caching prevents redundant checks and that cache invalidation is correct. -- Verify that project setup handles clean solutions or unrestored packages without silently dropping references. -- Ensure error loggers are consistent across all checking phases to avoid missing errors. - -**Severity:** Stale results causing glitches → **critical**. Missed invalidation → **high**. Missing cache verification → **medium**. +**Hotspots:** `src/Compiler/Facilities/`, `src/FSharp.Core/` -**Hotspots:** `src/Compiler/Service/`, `src/Compiler/Driver/` +FCS request cancellation and the `vsintegration/` hotspot moved to @ide-integration. --- -### 15. Syntax Tree & Parser Integrity +### 12. Syntax Tree & Parser Integrity AST nodes must accurately represent source code. Parser changes are high-risk because they affect every downstream phase. @@ -302,7 +250,7 @@ AST nodes must accurately represent source code. Parser changes are high-risk be --- -### 16. Exception Handling Discipline +### 13. Exception Handling Discipline Exception handling must be precise. Catch-all handlers and swallowed exceptions hide bugs. @@ -311,16 +259,17 @@ Exception handling must be precise. Catch-all handlers and swallowed exceptions - Never swallow exceptions silently; handle `OperationCanceledException` specially. - Do not suppress task completion by silently ignoring `TrySetException` failures. - Returning `Unchecked.defaultof<_>` to swallow exceptions is dangerous — investigate and fix the root cause. -- At language service API boundaries (top-level entry points called by the IDE), catch all exceptions to prevent IDE crashes — but log them, don't silence them. - Inside the compiler, do not add catch-all exception handlers — they hide bugs. +The language-service boundary bullet moved to @ide-integration. + **Severity:** Swallowed cancellation → **critical**. Catch-all handler → **high**. Missing error → **medium**. **Hotspots:** `src/FSharp.Core/`, `src/Compiler/Service/`, `src/Compiler/Optimize/` --- -### 17. Diagnostic Quality +### 14. Diagnostic Quality Error and warning messages are the compiler's user interface. They must be precise, consistent, and actionable. @@ -335,7 +284,7 @@ Error and warning messages are the compiler's user interface. They must be preci --- -### 18. Debug Experience Quality +### 15. Debug Experience Quality Debug stepping, breakpoints, and locals display must work correctly. Debug experience regressions silently break developer workflows. @@ -350,7 +299,7 @@ Debug stepping, breakpoints, and locals display must work correctly. Debug exper --- -### 19. Feature Gating & Compatibility +### 16. Feature Gating & Compatibility New features must be gated behind language version checks. Breaking changes require RFC process. @@ -464,28 +413,26 @@ Execute review in six waves, each building on the previous. ### Wave 1: Structural Scan 1. Verify every behavioral change has a corresponding test (Dimension 1). -2. Check feature gating — new features must have `LanguageFeature` guards (Dimension 19). -3. Verify no unintended public API changes (Dimension 7). -4. Check for binary compatibility concerns in pickle/import code (Dimension 12). +2. Check feature gating — new features must have `LanguageFeature` guards (Dimension 16). +3. If the diff touches `vsintegration/`, `src/Compiler/Service/`, or `src/Compiler/Symbols/`, invoke @ide-integration. Do not review those trees here. +4. Check for binary compatibility concerns in pickle/import code (Dimension 10). 5. If FSharp.Core is touched, apply FSharp.Core Stability checks (Dimension 2). ### Wave 2: Correctness Deep-Dive -1. Trace type checking changes through constraint solving and inference (Dimension 8). +1. Trace type checking changes through constraint solving and inference (Dimension 7). 2. Verify IL emission correctness with both Debug and Release optimizations (Dimension 5). -3. Validate AST node accuracy against source syntax (Dimension 15). +3. Validate AST node accuracy against source syntax (Dimension 12). 4. Check parallel determinism if checking/name-generation code is touched. 5. Verify optimization correctness — no semantic-altering transforms (Dimension 6). -6. Verify struct semantics if value types are involved (Dimension 9). +6. Verify struct semantics if value types are involved (Dimension 8). ### Wave 3: Runtime & Integration -1. Verify concurrency safety — no races, proper cancellation, stack traces preserved (Dimension 13). -2. Check IDE impact — no unnecessary rechecks or keystroke-triggered rebuilds (Dimension 10). -3. Verify overload resolution correctness if constraint solving changes (Dimension 11). -4. Check incremental checking correctness — no stale results (Dimension 14). -5. Review diagnostic message quality (Dimension 17). -6. Verify debug experience — stepping, breakpoints, locals (Dimension 18). +1. Verify concurrency safety — no races, proper cancellation, stack traces preserved (Dimension 11). +2. Verify overload resolution correctness if constraint solving changes (Dimension 9). +3. Review diagnostic message quality (Dimension 14). +4. Verify debug experience — stepping, breakpoints, locals (Dimension 15). ### Wave 4: Quality & Polish diff --git a/.github/agents/extraction-pipeline.md b/.github/agents/extraction-pipeline.md index 4ab0e27e1e4..6470b47dbd9 100644 --- a/.github/agents/extraction-pipeline.md +++ b/.github/agents/extraction-pipeline.md @@ -508,3 +508,19 @@ Final check on the complete artifact set: - No rules referencing specific function names or line numbers unless those functions are long-lived stable APIs (verified by grep in 5.6) - Every CHECK item is phrased as a generalizable principle, not a transcription of one PR's feedback - Dimension frequency was counted by PRs, not by comments — a PR with 50 comments counts the same as one with 1 comment + +### 5.8 Structural leftover gate + +Run this before any claim-versus-code panel, and again before calling the artifacts clean. A panel that only checks whether a named symbol exists will miss a transplanted outline number. That panel did not finish. + +`### Wave` and `### Additional` are not numbered headings. Ignore lines inside fenced code. + +Fail the artifact when any of these is true: + +- A `### N.` heading is not exactly one more than the previous `### N.` in that file. The first one must be `### 1.`. +- A file has a `### N.` heading and also a topical `###` heading. A topical heading does not start with a digit, `Wave`, or `Additional`. +- A heading's body, up to the next heading, is only a "Moved to" line. +- An ordered item `N.` in prose has no item `N-1` earlier in that list. +- A heading, including its number, is copied from another agent's heading. + +Delete the foreign number and the empty stub. Keep a rule only under a heading that belongs in this file. Do not report the review clean while this gate fails. diff --git a/.github/agents/ide-integration.md b/.github/agents/ide-integration.md new file mode 100644 index 00000000000..a4204deae23 --- /dev/null +++ b/.github/agents/ide-integration.md @@ -0,0 +1,162 @@ +--- +name: ide-integration +description: "Reviews IDE integration: vsintegration/, src/Compiler/Service/, and src/Compiler/Symbols/. Read vsintegration.md or compiler-service.md for the tree the diff touches. Does not replace @expert-reviewer." +--- + +# IDE integration + +Sibling of `@expert-reviewer`. Does not replace it. Compiler types, IL, pickle, optimizer, feature gates, FSharp.Core, and compiler diagnostics stay there. + +Rules live in two files. Do not merge them. A pull request that touches both trees gets findings from both files. File an inline comment in the tree of its path. + +| Diff touches | Read | +|---|---| +| `vsintegration/**` | `.github/skills/reviewing-vsintegration/vsintegration.md` | +| `src/Compiler/Service/**`, `src/Compiler/Symbols/**` | `.github/skills/reviewing-vsintegration/compiler-service.md` | + +`vsintegration/tests/**` uses `vsintegration.md`. `tests/FSharp.Compiler.Service.Tests/**` uses `compiler-service.md`. + +Use the `reviewing-vsintegration` skill to pick the file. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. + +## Principle + +- A keystroke must not call `BackgroundCompiler.InvalidateConfiguration` when `FSharpProjectOptions.AreSameForChecking` is true. + +## Both trees + +Read both files when a diff touches both trees. These three rules apply when a change crosses `vsintegration/` and `src/Compiler/Service/` or `src/Compiler/Symbols/`. + +(Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) + +- At the Visual Studio entry point, catch non-cancellation exceptions, log them, and do not let them reach the UI. + +All FCS requests must support cancellation. + + +### Snapshot identity across the boundary + +Use the semantic inputs represented by `ProjectSnapshotBase.ParsingVersion`, `SignatureKey`, and `FullKey`, distinguishing their content digests from runtime `GetHashCode` identities. + +**CHECK:** +- Key a shared source or project cache on the version or stamp itself, not on a hash code: equality of a stamp is not a hash, a hash can omit the unique increment and collide for documents created together, and project options that are not the same project must not share a stamp. +- Use public `VersionStamp` identity and `ISourceTextNew.GetChecksum` without reflecting private fields or modifying host-owned hash implementations. +- For repeated `IncrementalBuilder.NotifyFileChanged` notifications with the same timestamp, verify that `Slot.Notify` leaves the slot unchanged. +- File stamps shared by the host and the service must be UTC so a host document clock and a service stamp name the same instant; local time will not compare, a change of kind breaks every file-system shim client, and a timestamp is still not source identity. +- With `useChangeNotifications` enabled, `computeStampedFileNames` must skip source-file stat calls while `computeStampedReferencedAssemblies` still checks reference freshness. +- `BackgroundCompiler.getOrCreateBuilder` must reuse a matching cached builder unless `IsReferencesInvalidated` is true, while `TransparentCompiler` uses `CompilerCaches` rather than an incremental builder. + +**Severity:** high. + +### Cancellation ownership + +A request token belongs to the caller that started the work. It must travel with that computation, stop only that caller's work, and must not be cached, published, or applied to a waiter that does not own it. + +**CHECK:** +- Install the request token through `Cancellable.UseToken` or `Cancellable.toAsync` and restore the previous `AsyncLocal` value when the scope exits. +- A worker must install the caller token with `Cancellable.UsingToken` and restore the previous token before the thread is reused. +- A cancellation check must run only inside the caller's computation and must share that computation's token; outside that context `Cancellable.CheckAndThrow` fails immediately unless `DISABLE_CHECKANDTHROW_ASSERT` is set. That failure is not a debug-only assert, and the host disables it rather than enabling it, and it must not leak an uncaught cancellation or observe another request's token. +- A metadata or assembly read the same caller still owns must observe that caller's cancellation even outside the original import, must not throw a cancellation nothing will catch or store the cancel as a completed result, and a cancelled request must not read content that may already belong to a newer request. +- Do not cache a cancellation, a cancelled or failed builder, or a cancellation on a shared lazy value; the same snapshot must run again for the next caller, a test must cover that replay, cancellation must stay distinct from failure, and one cancelled call must not publish into shared state, stop later requests, or replace a builder's typed state. +- `AsyncLazy.cancelIfUnawaited` must cancel a running shared CTS only when `requestCount` is zero and reset that computation for a later request. + +**Severity:** high. + +### Service must not depend on Visual Studio integration + +Host-neutral editor capabilities live on the public service API. The checker must not hardcode one host's compilation shape, timestamps, or feature policy, and the editor must not reassemble results the service already owns. + +**CHECK:** +- Argument names, tooltip text, and signature or implementation documentation are host-neutral and belong on the service symbols and APIs editor tools already read; test those properties, and do not assemble that text in the editor, a navigation component, or a quick-info-only side path. +- Host-neutral identifier splitting must be public as `QuickParse.GetPartialLongNameEx`, not friend-only or Visual Studio-only. +- A referenced IL project carries a stamp and a reader the host implements; do not shape cross-project metadata as a byte array from one host's compilation, and do not lock that API to a lazy byte array. +- Keep the `checkResults.HasErrors` policy gate in `UnusedDeclarationsAnalyzer.AnalyzeSemanticsAsync`, not in `UnusedDeclarations.getUnusedDeclarations`. + +**Severity:** high. + + + +## Review workflow + +Run these five waves when invoked. Do not skip a wave. Do not review `main` for code that exists only on the PR branch. + +### Wave 0: Build the review briefing + +Before any dimension analysis, assemble the full context. Sub-agents reviewing code without context hallucinate or miss important design intent. + +**Collect:** +- **PR metadata**: title, description, author, labels, linked issues/specs +- **Existing PR comments and reviews**: what has already been discussed — don't duplicate existing feedback +- **Referenced issues and design documents**: if the PR links to a spec or issue, read them for design intent +- **Changed files list**: paths, additions, deletions + +**Compute the correct diff.** The PR diff must reflect only the PR's own changes — not unrelated commits on `main` since the branch was created. + +```bash +gh pr diff {pr_number} --repo {owner}/{repo} +``` + +Do not use raw `git diff main..branch`. + +Read `.github/skills/reviewing-vsintegration/vsintegration.md` when the diff touches `vsintegration/**`. Read `.github/skills/reviewing-vsintegration/compiler-service.md` when it touches `src/Compiler/Service/**` or `src/Compiler/Symbols/**`. Read both when it touches both. Add the three rules under Both trees when the change crosses those trees, owns cancellation, or could make `src/Compiler/Service` depend on `vsintegration`. Invoke `@vs-integration-perf` when the change is about JTF, retained memory, or the perf execution map. Do not copy that agent. + +Save the briefing to a file. Every Wave 1 sub-agent receives it. + +### Wave 1: Find + +Launch one sub-agent per selected dimension, in parallel batches of 6. Each evaluates exactly one dimension against the PR diff. + +Report `$DimensionName — LGTM` only when no `CHECK` bullet in that dimension has a reproducible failing scenario. + +Report an ISSUE only when you can construct a concrete failing scenario: a specific input, a specific call sequence, a specific state that triggers the bug. No hypotheticals. + +Read the PR diff, not main. New files only exist in the PR branch. Never verify findings against `main`. Include exact file path and line range. Verify by tracing actual code flow. + +File the finding in the tree of the file. Do not file a host-type leak as a compiler IL issue, and do not file an FCS cache-key bug as a MEF issue. + +### Wave 2: Validate + +For each non-LGTM finding, actively prove or disprove it: + +- Trace callers, callees, state mutations, and error paths on `refs/pull/{pr}/head`. +- Write and run a minimal test when the claim is testable. If the test fails as predicted, the finding is confirmed. If it passes, it is disputed. +- When a full test is too heavy to run inline, write a proof-of-concept snippet another engineer can implement. +- For concurrency, cancellation, and snapshot identity, write a step-by-step execution trace. +- For a borderline finding, validate with 3 distinct available model families. Keep findings confirmed by at least 2 of 3. + +A finding is confirmed only with concrete evidence. Never validate against `main`. + +### Wave 3: Post + +Post confirmed findings as inline review comments at exact file:line. Every comment begins with `🤖🕵️`. + +```markdown +🤖🕵️ **[$SEVERITY] $DimensionName** + +$Concrete scenario that triggers the bug. + +**Recommendation:** $Fix. +``` + +Post design-level concerns that are not tied to a line as one PR comment, one bullet each, still starting with `🤖🕵️`. + +IDE impact and incremental-checking checks live in the two files above. Do not send those findings back to `@expert-reviewer`. + +### Wave 4: Summary + +Post a dimension checkbox table as the review body. The body starts with `🤖🕵️`. + +```markdown +🤖🕵️ + +| # | Dimension | Verdict | +|---|-----------|---------| +| 1 | Dimension Name | ✅ LGTM | +| 2 | Another | ⚠️ 1 MAJOR | + +- [x] Dimension Name +- [ ] Another — description of issue +``` + +`[x]` means LGTM or nits only. `[ ]` means major or blocking. + +All `[x]` → APPROVE. Any blocking → REQUEST_CHANGES. Otherwise → COMMENT. diff --git a/.github/instructions/CompilerService.instructions.md b/.github/instructions/CompilerService.instructions.md new file mode 100644 index 00000000000..c1a0f77b487 --- /dev/null +++ b/.github/instructions/CompilerService.instructions.md @@ -0,0 +1,12 @@ +--- +applyTo: + - "src/Compiler/Service/**/*.{fs,fsi}" + - "src/Compiler/Symbols/**/*.{fs,fsi}" +--- + +# Compiler service + +Read `.github/skills/reviewing-vsintegration/compiler-service.md`. Invoke `@ide-integration`. Do not restate that file here. + +- No VS, MEF, JTF, or Roslyn types in these folders. These folders must not reference `vsintegration`. +- `@expert-reviewer` still owns type checking, IL, pickle, and FSharp.Core. Do not add those rules here. diff --git a/.github/instructions/VSIntegration.instructions.md b/.github/instructions/VSIntegration.instructions.md new file mode 100644 index 00000000000..cb8ac964ac8 --- /dev/null +++ b/.github/instructions/VSIntegration.instructions.md @@ -0,0 +1,10 @@ +--- +applyTo: "vsintegration/**/*.{fs,fsi}" +--- + +# Visual Studio integration + +Read `.github/skills/reviewing-vsintegration/vsintegration.md`. Invoke `@ide-integration`. Do not restate that file here. + +- Host types stay in this tree. `src/Compiler/Service` and `src/Compiler/Symbols` stay host-neutral. +- For JTF, retained memory, and the perf execution map, invoke `@vs-integration-perf`. Do not copy it. diff --git a/.github/skills/reviewing-compiler-prs/SKILL.md b/.github/skills/reviewing-compiler-prs/SKILL.md index 915d257603a..1cbc438d846 100644 --- a/.github/skills/reviewing-compiler-prs/SKILL.md +++ b/.github/skills/reviewing-compiler-prs/SKILL.md @@ -1,6 +1,6 @@ --- name: reviewing-compiler-prs -description: "Performs multi-agent, multi-model code review of F# compiler PRs across 19 dimensions including type checking, IL emission, binary compatibility, and IDE performance. Dispatches parallel assessment agents per dimension, consolidates with cross-model agreement scoring, and filters false positives. Invoke when reviewing compiler changes, requesting expert feedback, or performing pre-merge quality checks." +description: "Performs multi-agent, multi-model code review of F# compiler PRs across 16 dimensions including type checking, IL emission, and binary compatibility. Dispatches parallel assessment agents per dimension, consolidates with cross-model agreement scoring, and filters false positives. Invoke when reviewing compiler changes, requesting expert feedback, or performing pre-merge quality checks. Invoke @ide-integration for vsintegration/, src/Compiler/Service/, and src/Compiler/Symbols/." --- # Reviewing Compiler PRs @@ -11,7 +11,8 @@ Full dimension definitions and CHECK rules live in the `expert-reviewer` agent. - 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/` or `LanguageServer/` — focus on IDE Responsiveness, Concurrency, Memory +- 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 @@ -24,13 +25,13 @@ Full dimension definitions and CHECK rules live in the `expert-reviewer` agent. | `Optimize/` | Optimization Correctness, IL Emission, Test Coverage | | `SyntaxTree/`, `pars.fsy` | Parser Integrity, Feature Gating, Typed Tree Discipline | | `TypedTreePickle.*`, `CompilerImports.*` | Binary Compatibility (highest priority) | -| `Service/` | FCS API Surface, IDE Responsiveness, Concurrency, Incremental Checking | -| `LanguageServer/` | IDE Responsiveness, Concurrency | +| `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/` | IDE Responsiveness, Memory Footprint, Cross-Platform | +| `vsintegration/` | Read `vsintegration.md` via `@ide-integration`. Memory and JTF: `@vs-integration-perf`. | | `eng/`, `setup/`, build scripts | Build Infrastructure, Cross-Platform | ## Subagent Dispatch diff --git a/.github/skills/reviewing-vsintegration/SKILL.md b/.github/skills/reviewing-vsintegration/SKILL.md new file mode 100644 index 00000000000..01297aeb56c --- /dev/null +++ b/.github/skills/reviewing-vsintegration/SKILL.md @@ -0,0 +1,42 @@ +--- +name: reviewing-vsintegration +description: "Routes a Visual Studio integration or compiler-service review to the file for that tree. Use when reviewing a PR, pre-merge change, or edit under vsintegration, FSharp.Editor, the legacy language service, FSI, the project system, MEF, src/Compiler/Service, src/Compiler/Symbols, FCS snapshots, the incremental builder, workspace, navigation, completion, or symbols. Invokes @ide-integration. Does not review compiler type checking, IL, pickle, or FSharp.Core." +--- + +# Reviewing VS integration + +Tree checks live in `vsintegration.md` and `compiler-service.md`; the three boundary checks live in `@ide-integration`. + +`@expert-reviewer` stays the compiler reviewer. Do not send `vsintegration` or `src/Compiler/Service` findings there. + +## Which file + +| Diff touches | Read | +|---|---| +| `vsintegration/**` | [vsintegration.md](vsintegration.md) | +| `src/Compiler/Service/**` or `src/Compiler/Symbols/**` | [compiler-service.md](compiler-service.md) | +| Both | Both files | +| `vsintegration/tests/**` | [vsintegration.md](vsintegration.md) | +| `tests/FSharp.Compiler.Service.Tests/**` | [compiler-service.md](compiler-service.md) | + +Use the Both trees rules only when the diff touches both trees or adds a reference from `src/Compiler/Service` to `vsintegration`. + +## Folder routing + +| Path | File | Select | +|---|---|---| +| `vsintegration/src/FSharp.Editor/**` | vsintegration.md | UI thread, MEF, caches, quick info, navigation, completion, diagnostics, options, classification | +| `vsintegration/src/FSharp.LanguageService/**` | vsintegration.md | Legacy language service, UI thread, solution lifetime | +| `vsintegration/src/FSharp.ProjectSystem*/**` | vsintegration.md | Project lifetime, UI thread, generator identity | +| `vsintegration/src/FSharp.VS.FSI/**` | vsintegration.md | Interactive session startup and cancellation | +| `vsintegration/tests/**` | vsintegration.md | Editor tests must call the host provider | +| `vsintegration/**` other | vsintegration.md | Do not push host types into the compiler service | +| `src/Compiler/Service/**` | compiler-service.md | Cache identity, incremental results, locks, public FCS API, diagnostics, readers | +| `src/Compiler/Symbols/**` | compiler-service.md | Symbol results, ranges, completion, walks | +| `tests/FSharp.Compiler.Service.Tests/**` | compiler-service.md | Assert the observable service result. Isolate process-wide state | + +## Do not route here + +- `src/Compiler/Checking/`, `src/Compiler/Optimize/`, `src/Compiler/CodeGen/`, `src/Compiler/TypedTree/`, `src/Compiler/SyntaxTree/`, `src/Compiler/Driver/` +- `src/FSharp.Core/**` +- `src/FSharp.Compiler.LanguageServer/**` is not this corpus. Compiler concurrency stays with `@expert-reviewer`. diff --git a/.github/skills/reviewing-vsintegration/compiler-service.md b/.github/skills/reviewing-vsintegration/compiler-service.md new file mode 100644 index 00000000000..6d42ad7791e --- /dev/null +++ b/.github/skills/reviewing-vsintegration/compiler-service.md @@ -0,0 +1,265 @@ +# Compiler service + +Paths: `src/Compiler/Service/**`, `src/Compiler/Symbols/**`. Tests: `tests/FSharp.Compiler.Service.Tests/**`. + +Host-neutral API for IDEs: checker, incremental builder, snapshots, workspace, navigation, completion, symbols, and the diagnostics surface. This is not the rest of `FSharp.Compiler.Service.fsproj`. No Visual Studio, MEF, JTF, or Roslyn types. + +- Do not fork `DelayedILModuleReader`. Cross-project IL reads use `FSharpReferencedProject.ILModuleReference`, or `PEReference` with `DelayedILModuleReader`. The Visual Studio host uses `PEReference`. +- Use `ProjectSnapshotBase.ParsingVersion`, `SignatureKey`, or `FullKey` according to the cached computation’s inputs rather than assigning every cache the whole project closure. +- Process-wide ambient state is a leak. Readers, file-system shims, diagnostic loggers, console encoding, cancellation tokens, and generated-name state travel with the request or are restored when that owner finishes. +- One syntactic form, one flag value, or the happy path is not the feature. Sibling constructs, the other checking flag, and the error path get the same rule and an assertion of the observable result. +- A green suite is not coverage. The test calls the public service API, asserts names, ranges, or text, fails when the fix is absent, and does not share process-wide state with other tests. +- Do not do work the caller did not ask for, and do not return before publishing a result the caller will observe. An unchanged stamp stays cold, an optional capture stays off, and a memoized get stores before it returns. +- Unfinished and ill-typed code stays resolvable. The checker does not throw out to the host, and it does not drop later names, uses, highlighting, or sequence-point ranges because one construct failed. +- Treat changes to existing exported `SynExpr`, `SynPat`, or service-member signatures as compatibility breaks, verified by `SurfaceAreaTest.VerifySurfaceAreaFSharpCompilerService`, even if source patterns compile. + +### FCS API surface + +A public FCS signature change must update `FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl`, including when the API is marked experimental. + +- Internal compiler types must not appear on the public FCS API. +- The FCS Symbol API must be safe for concurrent callers. +- An async implementation change must not change the public FCS signature. +- A new public argument and a changed exception need an XML doc in the `.fsi`. +- `CheckFileInProject` wins over `CheckFileInProjectAllowingStaleCachedResults` unless the caller uses the stale API. +- Project setup must not drop references for a clean solution or an unrestored package. +- Error loggers must be the same object across checking phases. + +All FCS requests must support cancellation. + +Service entry points: (Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) + +- Do not require `FSharpChecker` to catch all exceptions; host catch-and-log stays at the Visual Studio entry point. + +### Key caches on a semantic stamp + +A lookup returns another project's result, an older script closure, or a result whose types were dropped. + +**CHECK:** +- Build `ProjectSnapshotBase.FullKey` from frozen project options, reference versions, and source-file versions rather than retaining mutable options as the cache identity. +- Keep `ProjectId` distinct across target frameworks and retain the project/output-file pair in `ProjectConfig.Identifier` when selecting or clearing per-project cache entries. +- For `ProjectSnapshotBase.SignatureKey`, include signature-file versions but omit paired implementation versions while retaining script-closure and referenced-project dependencies. +- an imported assembly is shared only when the key covers the file, the whole reference closure, and the configuration that changes import, and a shared project signature is the reader's view, not the producer's live compilation. +- Verify that `BackgroundCompiler.ParseFile` uses the supplied `ISourceText` unchanged, and apply type-retention checks to successful `FSharpCheckFileResults`, not untyped `FSharpParseFileResults`. +- a caller that must not read or write the parse cache has an entry point that does neither, and a parse-file cache still distinguishes files, at least by file name. +- a full invalidation, including solution close, clears the module-reader cache, a host-supplied reader updates the write stamp when observable bytes change, and compiler-generated name state is per project. + +**Severity:** critical. + +**Hotspots:** `src/Compiler/Service/FSharpProjectSnapshot.fs`, `src/Compiler/Service/FSharpWorkspace*.fs`, `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*` + +### Refresh only what the stamp changed + +An edit reparses unchanged files, or a diagnostics request misses files the builder skipped and returns errors from a different parse. + +**CHECK:** +- an edit reparses only the file the stamp says changed, and a stamp update names those files so they can be reparsed without waiting for the whole graph. +- a request for project diagnostics type-checks implementation files the incremental builder skipped, and diagnostics are collected when the check is finalized, not only from files visited so far. +- a full type-check of an implementation that has a signature does not block the partial path; start that check so it can run alongside the graph. +- `FSharpChecker.ParseAndCheckProject` must return parse errors from the same parse the check used. +- `useTransparentCompiler` selects `BackgroundCompiler` or `TransparentCompiler`; do not add a third builder for the same setting. +- optional capture stays off unless the caller sets the flag, the compact type-check summary is computed when the full check finishes, a timestamp read does not take the builder lock, and a cache hit is a distinct event from building a new cache. + +**Severity:** high. + +**Hotspots:** `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `src/Compiler/Service/FSharpProjectSnapshot.fs`, `tests/FSharp.Compiler.Service.Tests/*` + +### Lock one key and publish before return + +A wide lock, a queued caller-thread sequence, or an unsynchronized cache deadlocks, races, or lets the caller observe a miss for work that already finished. + +**CHECK:** +- a builder lock covers one cache key, not the whole builder, and a cache count taken while another thread mutates the cache uses that same lock. +- a lock that only starts a parse stays synchronous and is not held for the parse; do not replace a caller-thread build-graph sequence with a thread-pool queue when that deadlocks the import lock. +- Evaluate `SyntaxTree.ParseNode` through `GraphNode.GetOrComputeValue` and share `BackgroundCompiler.parseFileInFlight` entries under `parseCacheLock` instead of requiring eager parsing at construction. +- a memoizing cache publishes its result before get returns so a following lookup observes that completion; a recent-results lookup may still miss a write that is queued, so a test waits for the completion event rather than assuming the write is visible. +- a cache of shared framework imports and a script-resolution cache tolerate concurrent updates, and temporary builder state uses the same guard as the published state. +- a finally that restores thread-static context does not assume it runs on the thread that saved that context. +- a mutually recursive check that never suspends stays a synchronous loop with an explicit cancel check; do not convert it to async only to share a cancellation type, do not start a thread per recursion chunk, and a deep concatenate is a tail call or covered by the stack guard. + +**Severity:** critical. + +**Hotspots:** `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*` + +### Keep later names after an error + +A type error or exception drops later resolution, or escapes the transparent compiler into the host. + +**CHECK:** +- a check of ordinary source does not throw out of the transparent compiler, and an exception while checking one file is contained in the incremental builder so the host process stays up. +- Type-error recovery in `CheckDeclarations` and `CheckPatterns` stays with `@expert-reviewer`. +- a type error in one pattern does not stop checking the rest of the match, and later clauses must not look unmatched. +- a type error in a base-constructor argument does not skip let bindings and do expressions in the implicit constructor, and a type error on an assignment target does not skip name resolution on the right-hand side. +- Use `RecoverableException` at recovery points that ignore the exception so `OperationCanceledException` is not swallowed. + +**Severity:** critical. + +**Hotspots:** `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `tests/FSharp.Compiler.Service.Tests/TypeChecker/**` + +### Keep diagnostics in the request that emitted them + +A logger, phase, or severity leaks into the next caller, or a missing range is reported as a real project file. + +**CHECK:** +- awaiting work inside the build graph keeps the compilation diagnostics scope, including restoring the logger and phase, on both the cached path and any operation that never enters that cache. +- `CompilationGlobalsScope` must restore the previous logger and phase, and callers must not rely on `AssertFalseDiagnosticsLogger` throwing because its sink currently discards diagnostics. +- ambient diagnostic state travels with the request; a thread-local copy is not equivalent until the same requests agree under both stores. +- any service operation that applies command-line options, including find-usages, runs inside a diagnostics scope, and diagnostics emitted while creating a project builder honor the compiler's severity options. +- a diagnostic with no range uses a sentinel that cannot name a project file, including whatever file is at index zero, and callers can tell that sentinel from a real range. +- Clamp parser EOF diagnostics to the current source bounds in `FSharpDocumentDiagnosticAnalyzer.GetDiagnostics` instead of treating every overrun as a snapshot mismatch. +- a diagnostic for a combined construct covers the keyword that introduces the error, a documentation comment that no declaration consumes is reported, and a diagnostic type name spells the concept. + +**Severity:** high. + +Use `src/Compiler/Service/{ServiceCompilerDiagnostics,service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants,BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Symbols/{FSharpDiagnostic,XmlDocInheritance,XmlDocSigParser}.*`, and `src/Compiler/Facilities/DiagnosticResolutionHints.*` as diagnostic hotspots. + +### Pass readers and restore process state + +A process-wide reader, shim, or console encoding leaks into the next solution or drops writers the host installed. + +**CHECK:** +- project reads and script reads are not distinguished by mutating the process-wide assembly reader; pass the reader with the referenced project, and accept a custom module reader rather than only a metadata stream. +- Host-supplied `ILModuleReader` values in `FSharpReferencedProject` must support concurrent reads and remain logically immutable until their `getStamp` value changes. +- a file-system shim installed for a solution is restored when that solution closes, and a default shim keeps its members overridable so a client replaces only the operations it needs. +- a file-system source read used by the lexer hands back characters directly, and a metadata reader that holds a binary view weakly does not capture that view in a closure. +- a module reader keeps nested namespaces and type definitions lazy until a lookup needs them. +- a compilation flag that changes console encoding restores the encoding object taken from the console, without rebuilding it from a code page and without replacing the host's output streams. + +**Severity:** high. + +**Hotspots:** `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `src/Compiler/Service/FSharpWorkspace*.fs`, `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Service/ServiceAssemblyContent.*`, `src/Compiler/Service/ExternalSymbol.*`, `src/Compiler/Service/ServiceLexing.*` + +### Treat a public signature change as a break + +A public symbol, diagnostic, or tooltip signature changes and is treated as cleanup, or a test-only member is published. + +**CHECK:** +- a public signature change is the break, even when the private representation is unchanged; restoring compatibility means restoring the signature callers construct. +- A public break is a diff in `FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl`; do not accept or reject it for being easier. +- A new public type must ship in `FSharp.Compiler.CodeAnalysis` or `FSharp.Compiler.Symbols`, and an experimental member must be marked experimental. +- a helper that exists only so tests can observe a cache is not a public member, and reader implementations are not published on the public IL signature; a host that already has assembly symbols supplies them through the same extension point as cross-project references. +- a diagnostic range uses the same range type as other service APIs, a symbol query that needs only a count or a yes-or-no answer does not force callers to build the full child list, and a symbol predicate tools already depend on is a member named like the existing predicates. +- a new symbol-description API returns the same text as declaration lists and tooltips, a display choice such as unqualified type names is an explicit option on the display context, and a new completion option defaults to the items callers already see. +- For public `SynExpr` or `SynPat` shape changes, verify source-pattern compatibility and `SurfaceAreaTest`’s exported constructor signatures separately. + +**Severity:** high. + +**Hotspots:** `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*`, `src/Compiler/Symbols/FSharpDiagnostic.*`, `src/Compiler/Service/{ServiceDeclarationLists,ServiceParamInfoLocations,ServiceInterfaceStubGenerator}.*`, `src/Compiler/Service/ServiceNavigation.*` + +### Report the symbol the user wrote + +Tooling sees a compiler-generated symbol as user-written, misses the property or method at that range, or looks up a symbol whose identity was dropped. + +**CHECK:** +- a compiler-generated symbol or attribute is marked synthetic and is not reported over the range of the symbol the user wrote; a type that analysis only saw, and that is not the type of a written expression, stays retrievable without being treated as completable. A generated name shown to tooling or the debugger is a legal identifier, does not collide with a name already in scope, and identifies a repeated construct by where it appears rather than by an ordinal alone. +- a symbol listing does not add another symbol on a range that already names a different member, unless the added symbol is an intended accessor of that member, and a range replace does not drop a constructor when both the type and the constructor belong there. +- Check `FSharpMemberOrFunctionOrValue.IsProperty` and `FSharpField.DeclaringUnionCase` for accessor and named-union-field results without requiring the unpublished `FSharpParameter.Owner` API. +- a symbol stored for later lookup keeps the case kind that lookup uses, including exception identity, and a symbol-use record used for rename carries the same value information already stored for other values, including signature parameters. +- symbol uses include a top-level namespace declaration, a unification failure still reports the method symbol, and a file signature exposed to tools includes members the signature file hides. +- a fake value inserted only so a symbol can be built does not enter the name environment used for completion, a namespace that comes from both an assembly and a type provider has one stable symbol tag, and a derived public path compares only the mangled path and the name. +- an accessor query does not throw when the member name is only the accessor prefix, and a base-constructor call does not prefer a nested environment that hides the primary constructor's parameters. + +**Severity:** high. + +**Hotspots:** `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*`, `src/Compiler/Service/ExternalSymbol.*`, `src/Compiler/Service/ServiceNavigation.*`, `src/Compiler/Service/{ServiceDeclarationLists,ServiceParamInfoLocations,ServiceInterfaceStubGenerator}.*`, `tests/FSharp.Compiler.Service.Tests/GotoDefinition/**` + +### Cover the construct the user wrote + +A navigation, collapse, or breakpoint range is empty or stops at the name, so the host lands off the declaration or disagrees with the compiler's sequence points. + +**CHECK:** +- Check the documented extents of `NavigationItem.Range`, `NavigationItem.BodyRange`, and `FSharpSymbol.DeclarationLocation` separately instead of applying one keyword-inclusive range rule to all three. +- a collapse range for a constructor call is non-empty, including a call split across lines, and a folding range may be one range for a large union rather than a range for each small case. +- a pattern range published to tools includes its delimiters and trivia, constructor parameter ranges include the parentheses, and an attribute-list range includes the closing delimiter. +- Preserve operator spelling and parentheses in `IdentTrivia.OriginalNotationWithParen` while keeping the underlying identifier range separate from the enclosing expression range. +- an optional or synthetic measure carries a range that distinguishes it from a measure that was parsed, and an optional token range is absent when recovery did not parse that token. +- Keep `SyntaxTraversal.pick`’s existing whitespace/end-of-input fallback instead of imposing strict range containment on every traversal. +- Sequence-point emission in `IlxGen` stays with `@expert-reviewer`. + +**Severity:** high. + +Use `src/Compiler/Service/{ServiceNavigation,ServiceStructure,FSharpParseFileResults,ServiceParsedInputOps,QuickParse,ServiceParseTreeWalk,ServiceXmlDocParser,SynExpr,SynPat}.*` and `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*` as range-check hotspots. + +### Complete from the tree, and keep legal items + +Completion hides a legal item, returns the wrong scope, or follows a source scan after the tree already preserves the node. + +**CHECK:** +- when the syntax tree preserves a completion context, the tree is the owner; a source scan may ship only while the tree lacks the node, and those tests stay as the regression net. +- a completion filter does not remove an item that is legal to write at that position; rank it lower, and a host that wants it hidden opts in through a completion option rather than changing the shared list. +- a pattern filter that drops values still offers a literal, a class-only filter does not drop static members of value types and simple types, and static members drawn from every referenced assembly are optional. +- completion in an empty record offers that record's fields and, at the first field, names from the enclosing scope; a position just after a type declaration returns the enclosing scope, and an empty prefix inside a record field type still returns items. +- an unfinished expression that ends in a dot is not checked as the prefix alone, and an unfinished type name still produces a symbol and a diagnostic that describes the real problem rather than reporting the type as undefined when it resolved. +- completion filters its items once, and classifying a same-named group uses the display name already on the symbol instead of building a compiled name for every item. +- a feature that already resolves the symbol at the range may use a completion-oriented environment, and tests of that position mark the caret in the source; coordinates that only matched a previous environment are not a regression. + +**Severity:** high. + +Use `src/Compiler/Service/{ServiceDeclarationLists,ServiceParamInfoLocations,ServiceInterfaceStubGenerator,FSharpParseFileResults,ServiceParsedInputOps,QuickParse,ServiceParseTreeWalk,ServiceXmlDocParser,SynExpr,SynPat,ServiceAssemblyContent}.*` and `tests/FSharp.Compiler.Service.Tests/{Completion,ParameterInfo}/**` as completion hotspots. + +### Apply the walk to every sibling form + +A walk or classification fixes one syntactic form and leaves the sibling wrong, or reads text and allocates before the irrelevant symbols are dropped. + +**CHECK:** +- a walk applies the same visitor rule to syntactic siblings, including anonymous records beside records, computed bindings beside ordinary bindings, and an active pattern used as an expression beside an operator used as an expression. +- a walk that visits types also visits the type in a type test, a record resolved as an expression is reported as a symbol use, and a record-update walk still exposes the keyword that separates the copied expression from the fields. +- a parsed node kept only for inspection stays on its own field, and an existing member walk does not treat that field as members unless the walk is explicitly extended. +- a lookup of captured items at one position skips every group whose range does not contain that position, and a query does not classify every captured type in the file or build a full list before a search that could stop. +- Apply the early local-value and generic-parameter exclusions in `UnusedOpens.filterSymbolUses` before reading source lines, not to rename or general symbol-use queries. +- Deduplicate primary classifications in `TcResolutions.GetSemanticClassification` while preserving extra classifications explicitly requested by `relatedSymbolKinds`. +- a closed set of metadata or attribute values is decoded once and shared, a one-element name-resolution item is stored directly, an empty symbol result reuses one empty array, and a fresh collection callers only read by length is an array rather than a list wrapped as read-only. + +**Severity:** high. + +Use `src/Compiler/Service/{FSharpParseFileResults,ServiceParsedInputOps,QuickParse,ServiceParseTreeWalk,ServiceXmlDocParser,SynExpr,SynPat,ServiceAnalysis,SemanticClassification*,ServiceAssemblyContent}.*` and `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*` as walk/classification hotspots. + +### Keep the parsed form for tools + +The tree tooling walks has been desugared, has dropped a legal form, or records a separator that was not written. + +**CHECK:** +- a syntax tree published to tools keeps the form that was parsed; desugaring for later compilation is not the only stored form, and parentheses and error nodes are not dropped for each tool to reconstruct differently. +- an identifier in the tree tooling walks keeps the original source text; replace it with the compiled representation only in a later phase. +- a legal syntactic form stays marked in the tree even when it is non-idiomatic, because tooling cannot analyze a form the tree drops. +- `SynExprSequentialTrivia.SeparatorRange` must match `sequentialExpr`’s recorded separator, including THEN/OTHEN ranges and `None` when the grammar records no separator. +- Use `FSharpToken.Range` for token extents and keep every `SyntaxTreeTests.ParseFile` baseline case addressable by its fixture filename. + +**Severity:** medium. + +Use `src/Compiler/Service/{FSharpParseFileResults,ServiceParsedInputOps,QuickParse,ServiceParseTreeWalk,ServiceXmlDocParser,SynExpr,SynPat,ServiceLexing}.*` as parsed-form hotspots. + +### Assert the observable service result + +A green suite never calls the public API, never sets the other flag, or asserts a count that stays green when the wrong names are reported. + +**CHECK:** +- a service behavior change has a test that calls the public checker, tooltip, or symbol API and asserts the observable result; a host or editor test alone does not cover other hosts. +- the assertion fails when the fix is absent, and it asserts names, ranges, or text, not a count that stays green when the wrong items are reported. +- both values of a checking flag other hosts set are tested, a new cache or mode is on for the scenarios the existing suite already runs, and a partial-checking change fails when implementations are fully checked or skipped wrongly. +- a new syntactic form, directive, attribute order, or diagnostic-range fix is asserted beside the existing sibling form, including a documentation comment preceded by attributes and a multi-line comment rather than only a single line, and the assertion that locks the original syntax is not replaced. +- completion and caret tests drive the caret through the existing fixture and mark it in the source; do not hand-supply a position the context already computes, and do not add a branch the range check already excludes. +- a cache test counts hits or waits until the cache reports no in-flight update; a green result, a fixed sleep, or an immediate read of a still-queued update does not show the cache behavior. +- an FSI evaluation test reads the streams it claims to capture, a parser test passes source in memory, a baseline update deletes generated files the new run no longer produces, and a public wrapper needs a new harness only when it changes the observable text. + +**Severity:** high. + +**Hotspots:** `tests/FSharp.Compiler.Service.Tests/**`, `tests/FSharp.Compiler.Service.Tests/Completion/**`, `tests/FSharp.Compiler.Service.Tests/ParameterInfo/**`, `tests/FSharp.Compiler.Service.Tests/Tooltip/**`, `tests/FSharp.Compiler.Service.Tests/GotoDefinition/**`, `tests/FSharp.Compiler.Service.Tests/ErrorList/**`, `tests/FSharp.Compiler.Service.Tests/TypeChecker/**` + +### Isolate process-wide test state + +One test cancels, redirects, or mutates process-wide state, and the failure is reported as a product bug. + +**CHECK:** +- a test must not cancel the process-wide default cancellation token, because that token is shared with other tests. +- a test that mutates process-wide service state, the console, the process environment, or a shared project is serialized or kept out of the parallel run, and the exclusion mark sits on the test class, not on a helper. +- a test must not fire-and-forget work, synchronize with a fixed delay, or initialize a shared checker from a static constructor that parallel workers can race. +- a test that captures console output does not redirect the process console, and a test that listens to a process-wide cache by name does not run beside other listeners of that name. +- a session that writes artifacts uses its own temporary directory; a test that does not write files can stay in its source directory, and a tokenizer fixture does not share a mutable buffer across cases. +- a test-collection definition lives in the assembly that uses it, and a test-runner change keeps the trait or filter that selects a batch; a green run does not mean a skipped batch passed. +- Distinguish test-owned state from the service state protected by `IncrementalBuilder.updateCurrentState` before classifying a together-only failure as isolation. + +**Severity:** high. + +**Hotspots:** `tests/FSharp.Compiler.Service.Tests/**` + diff --git a/.github/skills/reviewing-vsintegration/vsintegration.md b/.github/skills/reviewing-vsintegration/vsintegration.md new file mode 100644 index 00000000000..c124606927a --- /dev/null +++ b/.github/skills/reviewing-vsintegration/vsintegration.md @@ -0,0 +1,229 @@ +# Visual Studio integration + +Paths: `vsintegration/**`. Tests: `vsintegration/tests/**`. + +Review `FSharpLanguageDebugInfoService` and `FSharpBreakpointResolutionService` as Visual Studio debugger services, keeping their host types outside `src/Compiler/Service` and `src/Compiler/Symbols`. + +- Keep Visual Studio, MEF, shell, and Windows UI types in the host; do not push them into FCS, and do not move shared compiler sources into the VS tree. +- Do not block the UI thread, and do not call a UI-thread host service from a background callback. +- An editor feature must not let an exception reach the Visual Studio UI. +- Prefer the host's own option, search, tooltip, theme, and analysis-scope services over a private copy or a fixed delay. +- Interactive editor work must be cancellable and tied to the current document version or query. +- A cache entry must stay findable until it is evicted, and an idle editor cache must stay collectable. +- Editor behavior that depends on a language-rule relaxation must follow language version, not apply to every project. +- A green compiler suite does not prove a host feature; the editor test has to call the host provider and cover the shapes the feature claims. +- An editor callback must not call `Async.RunSynchronously`. +- A keystroke must not call `BackgroundCompiler.InvalidateConfiguration` when `FSharpProjectOptions.AreSameForChecking` is true. +- Separate cancellation from logged faults at IDE entry points, as `FSharpNavigation.TryGoToDefinition` and `RoslynHelpers.StartAsyncAsTask` do. + +### UI thread affinity + +A host call that requires the UI thread runs off it and deadlocks, or work that does not need the UI thread blocks it. + +**CHECK:** +- A host call that requires the UI thread, including a project-site read or update, language-service creation, interactive-session startup, and options-page activation, must run on that thread. +- Keep calls guarded by `UIThread.MustBeCalledFromUIThread` on the UI thread and preserve `FSharpProjectNode`’s Opening-state `GetProjectSite` thread check. +- In `FSharpPackage.RegisterInitializeAsyncWork`, retain `AddTask(false, ...)` for non-UI setup and `AddTask(true, ...)` for FSI and UI-service initialization. +- Resolve `XMLMemberIndexService` in `FSharpAsyncQuickInfoSourceProvider.TryCreateQuickInfoSource`, not in `FSharpAsyncQuickInfoSource.GetQuickInfoItemAsync`. +- Move the UI-thread switch outside the per-project setup loop, and do not start every project load at once when each load posts workspace updates to the UI thread. +- Work that deadlocks off the UI thread, including interactive-session startup, must stay on the foreground init path. Background package initialization must not take that work with it. + +**Severity:** critical. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Common/**`, `vsintegration/src/FSharp.Editor/LanguageService/**`, `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.LanguageService/**`, `vsintegration/src/FSharp.ProjectSystem.FSharp/**`, `vsintegration/src/FSharp.ProjectSystem.Base/**`, `vsintegration/src/FSharp.VS.FSI/**`, `vsintegration/src/FSharp.UIResources/**` + +### MEF composition and package load + +The editor does not compose, another language loads F# visuals, or the host runs resources that were never embedded in the package. + +**CHECK:** +- Declare MEF exports in an assembly the component model already composes. Do not import a part into a package that is not itself composed, and do not export a host settings service that is not a MEF part. +- Restrict `WpfFSharpStyleFactory`’s `TypeConversion` metadata to `FSharpStyle` rather than relying on internal CLR visibility to restrict MEF activation. +- One-time color or theme setup belongs on the language-service composition root, not on a chain of parts that may never be instantiated. Set workspace options once when the language service initializes, not on every new text view. +- Keep `EditorOptions` and `SettingsStore.ISettingsStore` available through composition, with `SettingsStore.Create` selecting the persistent or in-memory store instead of a mutable static singleton. +- Load styles through `UIResources.NavStyles`. `AbstractOptionPage` must get settings from `SettingsStore.ISettingsStore`, not from `IVsSolution`. +- Do not dynamically load XAML resources, and do not enable a preserialized resource form the component merger cannot read as a blanket project property. A resource change must be embedded in the extension package, and the steps must work from a clean tree; one IDE build is not enough. +- Removing an unused host interface must leave the editor able to start. Shared sources used by more than one product stay with the compiler; a project file may retarget a link. Reuse the existing editor helper for the same lookup instead of a second copy. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/*.{fs,fsproj,resx}`, `vsintegration/src/FSharp.Editor/Common/**`, `vsintegration/src/FSharp.Editor/LanguageService/**`, `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.Editor/xlf/**`, `vsintegration/src/FSharp.UIResources/**`, `vsintegration/src/FSharp.ProjectSystem.FSharp/**`, `vsintegration/Vsix/**` + +### Solution and project lifetime + +Project setup runs during solution load, a close leaves the previous solution cached, or a script or path is not the project the user loaded. + +**CHECK:** +- Defer project setup until `LegacyProjectWorkspaceMap.OnAfterOpenSolution`; do not replace that callback with a delay. +- Do not set up projects while the solution is loading, on the first open or on a later open of the same solution, and do not run a document analyzer on a temporary project the host creates before the solution has loaded. +- Register projects as the solution reports them, after solution open, instead of waiting for a document to open. +- Clear owned caches in `FSharpSolutionEvents.OnAfterCloseSolution` and pending setups in `LegacyProjectWorkspaceMap.OnAfterCloseSolution`, without using delays to hide project-disposal races. +- Project-file synchronization must be atomic across a host callback that can fire twice, and a lookup in a concurrent project table must be one atomic try-get. +- Treat script files as ordinary projects, analyze a closed script in the solution, and take symbol information from the project system so navigation does not need a special path. +- Normalize a path that contains a parent segment before matching it to a project document. When the solution contains the source of a referenced framework assembly, other projects must bind the assembly they reference, not that source project. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/LanguageService/**`, `vsintegration/src/FSharp.Editor/Build/**`, `vsintegration/src/FSharp.Editor/Diagnostics/**`, `vsintegration/src/FSharp.LanguageService/**`, `vsintegration/src/FSharp.ProjectSystem.FSharp/**`, `vsintegration/src/FSharp.ProjectSystem.Base/**` + +### Stable editor caches + +A cache key cannot be found again, so entries accumulate, or an idle cache stays rooted and cannot be collected. + +**CHECK:** +- A cache key's hash and equality must stay stable for as long as the entry is stored. Hash a long-lived key from source identity that does not change until the user edits, not from type resolutions that are still being solved. +- A cache must be able to find the entry it stored. An eviction that cannot find the stored key is not eviction and must not be the only bound on the cache; keep a separate candidate list. +- A background eviction agent must be shared and must not retain the caches that post work to it. A cache must not own that agent's lifetime, and an idle cache must stay collectable without a finalizer. +- A shared editor cache must not recycle mutable entities across concurrent callers. A missed eviction must be its own observable failure, not a host hang timeout. +- Do not force one storage strategy onto both one-off compilation and incremental editor use, and an editor session must be able to override a language-feature gate that would disable the incremental path. Do not keep a separate no-eviction path for one-off compilation when one bounded cache with headroom covers both modes. +- Background cache maintenance must not use a concurrency primitive that exhausts threads, wait handles, or memory. An editor cache that reuses a compilation fix must keep that fix's gain and must not regress typical incremental or standalone builds. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Common/**`, `vsintegration/src/FSharp.Editor/LanguageService/**`, `vsintegration/src/FSharp.LanguageService/**` + +### Quick info presentation + +Quick info is unreadable, wraps through a fake divider, shows signature-help text, or is built from a Windows-only element. + +**CHECK:** +- Keep Quick Info on `FSharpAsyncQuickInfoSource.TryGetToolTip` and hints on their existing symbol/type-formatting helpers without substituting signature-help parameter values. +- Use `FSharpStyle.Separator` through `QuickInfoViewProvider.stackWithSeparators` and `WpfFSharpStyleFactory`, retaining `XmlDocumentation.separatorText` only as the text fallback. +- Honor `QuickInfoOptions.ShowRemarks` and use `getLinkTooltip`’s shorter project-relative or available solution-relative path for navigation tooltips. +- Custom tooltip content must reuse the host quick-info controller so show and dismiss stay identical to the platform. Do not keep a session capture whose only job was to dismiss the tooltip on navigation once the host already does that. +- `QuickInfoViewProvider.provideContent` builds `Microsoft.VisualStudio.Text.Adornments.ClassifiedTextElement` content with Roslyn `ClassificationTypeNames`, not a custom Windows-only element. +- Pass `QuickInfoOptions.DescriptionWidth` to `TryGetToolTip` and preserve section `LineBreak` tokens and `ContainerElementStyle.Wrapped` in `QuickInfoViewProvider`. + +**Severity:** medium. + +Use `vsintegration/src/FSharp.Editor/{QuickInfo,DocComments,Hints,Options}/**` and `vsintegration/src/FSharp.UIResources/**` as the Quick Info presentation hotspots. + +### Navigate to the loaded definition + +Navigation opens a window itself, jumps to metadata or the wrong linked copy, or offers a link that goes nowhere. + +**CHECK:** +- A definition provider returns a navigable item. `GoToDefinition.NavigateToItem` and `GoToDefinition.TryNavigateToTextSpan` open the document. +- Navigation must be able to return both the signature and the implementation. From an implementation, try the implementation and fall back to the signature span. A failed lookup must not leave a link that does nothing. +- When quick info is already on a definition, do not offer a link that navigates nowhere. A link that is shown must toggle between signature and implementation the same way go-to-definition does. +- Whether a tooltip symbol is navigable must be decided against the loaded workspace, not assumed from the symbol range alone. Navigation must reach types that are only inferred, and a symbol defined in another loaded project must resolve. A startup sentinel, identified by the existing range value rather than a copied filename, must not be offered as a target. +- Navigation to a file linked into several projects must select the document for the project that owns the symbol, and must not reuse an already-open document from a different project. +- Metadata-as-source must use a separate workspace, created directly, and must not depend on shell document services or the user's current workspace. +- Go-to-all and solution search must accept a container or namespace qualifier, keep fuzzy matching, and rank an exact dotted-name match ahead of a longer match without hiding either. Do not sort one language's results inside the language service when the host already ranks every language together. Use the host pattern matcher, and do not add match-span highlighting until the host navigation API can return those spans. Clear a transient navigation status when the request finishes, including when it fails. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Navigation/**`, `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/LanguageService/**`, `vsintegration/src/FSharp.LanguageService/**` + +### Completion filtering and commit + +Completion offers items that cannot be declared there, removes items that are still legal, or commits an edit the host then overwrites. + +**CHECK:** +- Auto-triggered completion must filter items by syntactic position. After a binding keyword, do not offer symbols that cannot be declared there, and do not auto-trigger where the user is declaring a new identifier. +- In a pattern position, completion must offer only constructs that are valid destructuring, not the general symbol list. +- In a pattern, rank items of the expected type first, but do not remove other items that are still legal to type. Remove only an item that cannot resolve in a pattern. +- `FSharpCompletionProvider.GetChangeAsync` commits `NameInCode`, including a generated override body when `GenerateBodyForOverriddenMethod` is enabled. +- Return every edit of a completion commit as the completion change for the document version the host will apply. A side edit is applied to a version the host then overwrites. +- Reuse the current `DeclarationListItem` values in `FSharpCompletionProvider`, displaying `NameInList` and committing `NameInCode` through `GetChangeAsync`. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Completion/**`, `vsintegration/src/FSharp.Editor/AutomaticCompletion/**`, `vsintegration/src/FSharp.Editor/CodeFixes/**` + +### Diagnostics scope and language gates + +A code fix matches a culture-specific message, a host analysis limit is never applied, or a language relaxation hits every project. + +**CHECK:** +- A code fix must not identify a diagnostic by its displayed message. The diagnostic must carry structured data the fix can match in any culture. +- When a diagnostic payload already has more than one case, callers should read those fields directly rather than adding a separate type test. +- Keep `FSharpServiceTelemetry.otelExport` DEBUG-only and disabled unless `FSHARP_OTEL_EXPORT` has a nonempty value. +- Editor analyzers must follow the host's live-analysis scope setting. Do not invent a private active-document gate beside that setting. +- Apply `FSharpGlobalOptions.SetBackgroundAnalysisScope` from `FSharpPackage.RegisterInitializeAsyncWork` rather than placing the setting only in an unused initialization hook. +- A language-rule relaxation that editor tools depend on must be gated by language version and exposed in a form tools can read. An editor suggestion must not rewrite code into a form an older compiler warns about. A language feature that changes editing must be tryable behind a preview switch before it becomes the default. +- If a code-fix test fails because the diagnostic no longer reports that case, fix the diagnostic. A missed shape is not a reason to drop the test. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Diagnostics/**`, `vsintegration/src/FSharp.Editor/CodeFixes/**`, `vsintegration/src/FSharp.Editor/Refactor/**`, `vsintegration/src/FSharp.Editor/Options/**` + +### Store each option on its own + +Editor settings are persisted as one blob, so a new setting comes back as the zero value, or a setting cannot be found. + +**CHECK:** +- A new field on an options record must survive `SettingsStore.SaveSettings` and `JsonConvert.PopulateObject` at its default. +- An options record bound by the settings page must have `[]`, and the markup must bind to public properties. +- An options refactor must keep each existing setting persisted. A setting that no longer round-trips is a regression even if the page still opens. +- An editor setting must have a resource string the options window can search. An optional numeric setting must accept an empty field as unset, and a control that is not wired to the checker must not be shown. +- Prefer a public host option when it exists. Do not build UI for a built-in host setting whose persistence is not public, and do not reference another language's internal options implementation or copy its page markup. +- Load editor settings once when they are registered, then update the cache from the persistence change event so running IDE instances stay in sync. Do not re-read on every access. +- Editor settings must not depend on a host-only service, or unit tests cannot set them. Supply a test store instead of scattering fallback values. Treat a stored options object as immutable outside the options dialog. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.UIResources/**`, `vsintegration/src/FSharp.Editor/Common/**` + +### Classification follows theme and role + +A classification ignores the language role, or a baked-in foreground stays unreadable after the theme changes. + +**CHECK:** +- In `getClassificationTypeName`, keep Function and Method mappings distinct while retaining the `IntrinsicFunction` and `ComputationExpression` keyword mappings. +- Punctuation that is still part of a string must keep the string classification. Classifying that range as a delimiter hides the string extent and needs a test for that range. +- Use `ThemeColors.GetColor` for custom format defaults and preserve the `fontAndColorStorage.GetItem` guard when applying theme updates. +- Classification colors must follow the active editor theme. A theme switch must keep the user's per-mode classification color customizations and must not overwrite them. +- Custom editor format definitions must be reapplied from the active theme at startup as well as on a theme switch, because the host persists the first instantiated defaults. A theme-specific foreground must not be baked into that default. + +**Severity:** medium. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Classification/**`, `vsintegration/src/FSharp.Editor/Common/**`, `vsintegration/src/FSharp.LanguageService/**` + +### Feature failures stay inside the feature + +An exception from document check, Quick Info, analysis, or the options page reaches the Visual Studio UI. + +**CHECK:** +- A failure while the editor checks a document must not reach the host. It needs a repro and a test; a green compiler suite does not cover that host failure. +- An uninitialized diagnostics logger must not escape as an exception from document check into editor features. +- Enabling an editor analysis feature must not let an exception reach the Visual Studio UI. +- Quick Info must not throw out to the shell. A failure while resolving a symbol, including a primitive, has to stay inside the feature. +- Opening the editor options page must not let a binding exception reach the Visual Studio UI. The settings type the page binds must stay in the shape that binding expects. +- Do not set a process-global resource assembly. Probe `FSharp.UIResources` XAML dictionaries with `ProvideBindingPath`. `ProvideCodeBase` does not load them. + +**Severity:** critical. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Diagnostics/**`, `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.UIResources/**`, `vsintegration/src/FSharp.Editor/xlf/**`, `vsintegration/src/FSharp.Editor/Common/**` + +### Editor tests cover the host + +An editor change is tested only through the service API, or only for a flat module-level shape, so the host failure stays invisible. + +**CHECK:** +- An editor test must exercise the editor provider. A suite that only calls the service method does not cover the editor. +- Cover the declaration shapes the feature claims, including a binding nested in a type or in an expression, a dotted name in a nested pattern, and a union that is out of scope or requires qualified access, not only a flat module-level case. +- A known limitation of an editor feature may be recorded as a skipped test so the missing case stays visible. Do not leave that case with no test at all. +- Editor tests must reuse the cached `ExportProviderFactory` from `RoslynHelpers.fs`, not build a new composition per test. +- Put a test that needs host-internal types in the test project those internals are already visible to. Do not link a setup file into an editor test project that does not reference the utility project that makes that setup run. + +**Severity:** medium. + +**Hotspots:** `vsintegration/tests/FSharp.Editor.Tests/**`, `vsintegration/tests/UnitTests/**`, `vsintegration/tests/Salsa/**` + +### Interactive work stays cancellable + +A keystroke, search, or tooltip starts work that keeps running after the user has cancelled or moved on. + +**CHECK:** +- Live-buffer edits must not start a non-cancellable parse for every keystroke. That work must be a debounced request for the current document version. +- User-started editor search must pass cancellation into the parallel scan. A parallel loop that ignores the request token keeps running after the search is abandoned. +- Solution search must stay cancellable, include operators as well as identifiers, and keep its cache for the solution lifetime, cleared on close, rather than a short-lived cache rebuilt on every query. +- Render icon, description, then documentation in `QuickInfoViewProvider.provideContent`, using `ClassifiedTextRun` links whose `FSharpNavigation.NavigateTo` fallback runs on lookup failure, never cancellation. +- Do not add a second cap on editor background parallelism when that work is already bounded. A second cap drifts from the bound callers already use, so cancellation and degree disagree. + +**Severity:** high. + +**Hotspots:** `vsintegration/src/FSharp.Editor/Common/**`, `vsintegration/src/FSharp.Editor/LanguageService/**`, `vsintegration/src/FSharp.Editor/Navigation/**`, `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/Diagnostics/**` + +