From a655dbfd61924de1f1a41aea1a5304a2f6a5eb1a Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 13 Oct 2026 15:31:37 +0200 Subject: [PATCH 01/12] Add VS integration reviewer artifacts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/expert-reviewer.md | 75 +- .github/agents/vsintegration-reviewer.md | 724 ++++++++++++++++++ .../CompilerService.instructions.md | 14 + .../VSIntegration.instructions.md | 12 + .../skills/reviewing-compiler-prs/SKILL.md | 9 +- .../skills/reviewing-vsintegration/SKILL.md | 44 ++ 6 files changed, 814 insertions(+), 64 deletions(-) create mode 100644 .github/agents/vsintegration-reviewer.md create mode 100644 .github/instructions/CompilerService.instructions.md create mode 100644 .github/instructions/VSIntegration.instructions.md create mode 100644 .github/skills/reviewing-vsintegration/SKILL.md diff --git a/.github/agents/expert-reviewer.md b/.github/agents/expert-reviewer.md index 50b6e901409..938dfd0bacc 100644 --- a/.github/agents/expert-reviewer.md +++ b/.github/agents/expert-reviewer.md @@ -7,7 +7,7 @@ description: "Multi-dimensional code review agent for F# compiler PRs. Evaluates Evaluates F# compiler changes across 19 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), `@vsintegration-reviewer` (VS host and IDE service). ## Overarching Principles @@ -17,23 +17,23 @@ 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. +- Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - **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. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ## 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. +2. **Catch-all exception handlers** — Do not add catch-all exception handlers. Handle `OperationCanceledException` specially; never swallow it. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +3. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. 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. +8. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. 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. @@ -149,23 +149,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/` - ---- +Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 8. Type System Correctness @@ -198,22 +182,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/` - ---- +Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 11. Overload Resolution Correctness @@ -257,7 +226,7 @@ The pickled metadata format is a cross-version contract. DLLs compiled with any 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. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - 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,25 +235,11 @@ 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/` +**Hotspots:** `src/Compiler/Service/`, `src/Compiler/Facilities/`, `src/FSharp.Core/` Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. --- -### 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/Service/`, `src/Compiler/Driver/` - ---- +Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 15. Syntax Tree & Parser Integrity @@ -311,7 +266,7 @@ 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. +- Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - Inside the compiler, do not add catch-all exception handlers — they hide bugs. **Severity:** Swallowed cancellation → **critical**. Catch-all handler → **high**. Missing error → **medium**. @@ -465,7 +420,7 @@ Execute review in six waves, each building on the previous. 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). +3. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. 4. Check for binary compatibility concerns in pickle/import code (Dimension 12). 5. If FSharp.Core is touched, apply FSharp.Core Stability checks (Dimension 2). @@ -481,9 +436,9 @@ Execute review in six waves, each building on the previous. ### 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). +2. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. 3. Verify overload resolution correctness if constraint solving changes (Dimension 11). -4. Check incremental checking correctness — no stale results (Dimension 14). +4. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. 5. Review diagnostic message quality (Dimension 17). 6. Verify debug experience — stepping, breakpoints, locals (Dimension 18). diff --git a/.github/agents/vsintegration-reviewer.md b/.github/agents/vsintegration-reviewer.md new file mode 100644 index 00000000000..2a88957fc0b --- /dev/null +++ b/.github/agents/vsintegration-reviewer.md @@ -0,0 +1,724 @@ +--- +name: vsintegration-reviewer +description: "Reviews Visual Studio integration and the host-neutral F# compiler service. Use when a change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. Pack A is the VS host. Pack B is the IDE service. Does not replace @expert-reviewer." +--- + +# VS Integration Reviewer + +Sibling of `@expert-reviewer`. Does not replace it. Compiler types, IL, pickle, optimizer, feature gates, FSharp.Core, and compiler diagnostics stay there. + +Two packs. Do not merge them. A PR that touches both produces findings in both. File an inline comment under the pack of its file path. + +Use the `reviewing-vsintegration` skill to route folders to a pack and to select dimensions. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. + +## Moved principle + +- **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. + +## Shared boundary + +Only these three rules may appear in both packs. Everything else stays in one pack. + +(Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) + +- 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. + +All FCS requests must support cancellation. + + +### Shared dimensions + +### Snapshot identity across the boundary + +Host and service must share one identity for source text, project options, and dependency freshness. A hash code, checksum, private host field, or bare timestamp is not that identity, and a notification that does not name the text must not invalidate the parse. + +**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. +- Do not read private fields of a host version type or patch a hash the host owns; ask the host for a stable identity, or hash the public string form, and pass the source version explicitly instead of hiding a checksum in an overridden hash code. +- A file-change notification must carry a source-text version, and the builder must not reparse for a timestamp, a workspace-changed event, or a repeat with no text change; a live buffer has no identity unless that notification carries one, which cancellation alone cannot supply. +- 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. +- Dependency freshness is a host-supplied stamp on the project options the checker already takes and must cover referenced assemblies as well as sources; when the host already reports every change, the service must not also stat or poll those inputs. +- One project has one incremental builder, every host parse of that project goes through that cache, and a snapshot key must include every dependency that can change the result and must not retain an exceptional result, so the next caller can run again. + +**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:** +- Keep the request token with the async computation and install it once at the async boundary or in the computation builder, not in thread-static storage, not inside an inlined cancel check, and not through a second helper or a manual install at each call site; a missing context is an explicit empty value, not a default token. +- When a token is installed on the thread that runs the computation, save and restore the previous token before that thread is reused, including across a thread switch, and copy the feature-entry token onto a worker first so a leftover token cannot cancel work that does not own it; a test must mix that computation with async, tasks, and thread switches. +- A cancellation check must run only inside the caller's computation and must share that computation's token; outside that context it is a misuse that fails immediately as an internal error in product builds a host can enable, not as a debug assert, 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. +- One caller must not cancel or dispose a cached job or builder that another caller is still awaiting; cancel shared work only when every caller has cancelled, never from inside an awaited job, and do not start a non-cancellable parse per edit notification—the edit owner debounces the burst, or the builder drops a notification that is not a new version. + +**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. +- A capability every editor should use, including host-neutral parsing such as splitting a long identifier and host-neutral diagnostic caching, must be public on the service; do not grant it only through a friend assembly, leave it only in the Visual Studio integration, or add a Visual Studio-only cache around results the service already stores. +- 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. +- Host policy stays outside the checker: a cache-validity rule that encodes one host's timestamps is a caller-supplied predicate, and an analysis API returns what the given check results support instead of embedding a policy such as skipping unused-symbol analysis when the file has errors. + +**Severity:** high. + + +## Pack A — VS host + +Paths: `vsintegration/**`. Editor, project system, legacy language service, FSI, debug adapter, MEF, UI thread. Must not push host types into FCS. + +- 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. + +### IDE Responsiveness + +Applies to this pack. The section below is the host copy of the moved dimension. + +### 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/` + +--- + +Language-service entry points in this pack: (Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) + +- 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. + +Hotspot moved with cancellation: `vsintegration/`. + +### 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. +- A newer host version is not evidence that project-system initialization left the UI thread. Keep that work on the UI thread until the host contract says otherwise. +- Package initialization and workspace updates that do not touch UI services must not take the UI thread, and a workspace project context whose host factory is built for the background must not stay on the UI thread. +- Resolve a UI-thread service when the editor part is created, on the main thread, through one main-thread or async lookup. Do not fetch it from a background tooltip or command callback, which deadlocks even when the result is unused. +- 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. +- A MEF view factory must not be visible to other languages. Keep custom visuals on an internal type so another language's tooltip does not load the F# editor. +- 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. +- Editor options must be a composed part that features can import. Expose as little of the editor to MEF as possible: a small factory should choose the host or test store instead of exporting the whole options implementation, and do not keep a static singleton the language service initializes by hand. +- Assign a process-global resource assembly once, from a composition singleton. Assigning it again for each view throws. The options page must open with no solution loaded. +- 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 the host signals that the solution UI is ready. Do not replace that signal with a custom wait or a fixed 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. +- On solution close, run the same cleanup as document close and release documents that are still open, because the workspace does not raise a close for them. Fix a project-close race by ordering disconnect against in-flight work, not by delaying the call. +- 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:** +- Quick info and inline hints must show the compiler's existing tooltip text, must not show a parameter value that belongs to signature help, and must not ship a debug tooltip that shows an internal layout tag. +- A section separator must have a fixed length. Do not size it by repeating glyphs or draw it as ASCII art, and do not treat a hand-drawn divider as wrapping. Break long documentation explicitly. +- Quick-info maximum width must scale with the editor font. Documentation may match the signature width only up to a fixed cap, alignment must follow the document reading direction, and documentation must not be fully justified. +- A quick-info section that other languages expose as a setting must be configurable, not always shown. A secondary tooltip must follow the active theme, and a path it displays must be relative to the solution. +- 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. +- Quick-info content must stay a view model another host can translate. Do not build that model out of a Windows-only element. Record and value text shown in tools should keep a single space between braces and fields. +- Do not add a width control that fights the host's own tooltip wrapping. If a setting does not always apply, mark it experimental and explain the effect. Quick info must keep line breaks between sections, and long documentation must still wrap after a host API change. + +**Severity:** medium. + +**Hotspots:** `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/Hints/**`, `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.UIResources/**` + +### 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 request must return a navigation item and must not open a window itself. Hover must not jump to decompiled metadata because the pointer is over an external symbol. +- 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. +- Completion must not invent a method body. Override completion must accept the member name first and the body in a later step, and must not insert the whole override, including a generated body, on one accept. +- 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. +- A name suggestion should reuse the completion items for that context when that lookup is cheap. Names the editor shows must stay separate from the representation that guides compiled form. + +**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. +- A diagnostics exporter must not stay enabled on the analysis path. Left running, it adds overhead that shows up as a compile-time regression. +- Editor analyzers must follow the host's live-analysis scope setting. Do not invent a private active-document gate beside that setting. +- A host limit or setting must be applied on the override or callback the host actually invokes. A limit that lives only in an initialization method the host no longer calls is never applied. +- 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:** +- Declare, store, and retrieve each editor setting on its own so a newly added setting keeps its default. Do not persist the whole settings type as one reflected blob. +- An options type the settings page binds must keep the marker that makes its properties settable, and options-page 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:** +- Classify a name by its language role. A function must not be colored as a keyword, and a method and a function value must not share a classification when their calling rules differ. +- 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. +- Do not assign a fixed foreground on a classification format. Leave it unset so the host theme supplies a readable color and the user can still customize it. +- 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 use a process-global resource assembly to locate one extension's dictionaries. Another extension assigning that same property causes a runtime failure. Resolve resources from this extension's install location. + +**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 one MEF composition instead of building a new export provider for every test. Tests must not share a process-wide options part across cases. If desktop tests share one host, a throw from a background thread must still name the case. +- 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. +- Tooltip navigation must use the host's built-in link and must be cancellable. If the user cancels the slower implementation lookup, fall back to the instant range. Render each tooltip element as icon, then description, then documentation. +- Before adding a custom parallelism cap for editor background work, compare it with the existing parallel combinator that already takes a maximum degree. + +**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/**` + + +## Pack B — IDE service + +Paths: `src/Compiler/Service/**`, `src/Compiler/Symbols/**`. Host-neutral API for IDEs. No VS, MEF, JTF, or Roslyn types. Checker, incremental builder, snapshots, workspace, navigation, completion, symbols, diagnostics surface. Tests in `tests/FSharp.Compiler.Service.Tests/**` belong to this pack. They are not a third pack. + +- The service stays host-neutral. A capability an editor needs is exposed on the existing checker or symbol shape, a host test does not prove it, and a host on another runtime must not keep a private fork of a shim the service should own. +- A cache identity is a semantic stamp of one project closure: options, source text, earlier files, included scripts, and reference write times. A similar path, a mutable options object, or a live typed graph is not that stamp. +- 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. +- A public signature change is a break even when the private representation is unchanged. A break can be right when it helps the clients that use the type; it is not cleanup. A service syntax-tree reshape is not that break if existing client patterns still compile. + +### FCS API sentences + +Public API additions to FCS must be carefully controlled. Internal data structures must never leak. Breaking changes to FCS API surface are acceptable with major version bumps. + +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. + +8. **Stale type-checking results** — Avoid returning stale results; they cause timing-dependent IntelliSense glitches. Prefer fresh results with cancellation support. + +### 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/` + +--- + +### IDE Responsiveness + +Applies to this pack. The section below is the service copy of the moved dimension. + +### 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/` + +--- + +### 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/Service/`, `src/Compiler/Driver/` + +--- + +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.) + +- 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. + +### 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:** +- a cache key is a slim structural value with the same equality as the type, not the live typed object, and a snapshot key is a frozen semantic stamp, not a mutable options object. +- a clear or lookup targets the same project options only; another target framework that shares the path is a different project, and project identity strings compare ordinally unless the host already normalized them. +- a cached file result stays valid only while earlier files in that project are unchanged, a script snapshot changes when an included script changes, and a referenced in-memory module carries a freshness stamp checked like a write time. +- 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. +- a parse or check cache does not return a result that dropped the types of values, a parse uses only the source the caller passed, and a host that supplies source text hits the same parse cache as an on-disk path. +- 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. +- a parse that feeds navigation or breakpoint ranges does not wait behind the shared analysis reactor, and a project parse-and-check returns parse errors from the same parse the check used. +- a checking setting does not split the builder into two implementations; one path that can turn the setting on is the shape to keep. +- a builder-owned parse cache is one switch, does not retain an implementation that has a signature, and is reclaimable with that builder; do not add a size threshold until measurement shows the weak cache is not enough. A parse cache that is not tied to a project stays small. +- 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. +- build-graph request continuations run asynchronously, an eager parse is queued when the node is created, and concurrent incremental updates share one builder and one in-flight parse under the same lock. +- 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. +- a type error does not drop the failed declaration from the environment or skip later uses; the name stays bound so tooling can still resolve it. +- 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. +- a recovery point that does not use the exception catches every failure, so an unlisted exception does not abort the rest of the check. + +**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. +- a computation that installs an ambient diagnostics logger restores the previous logger, and a diagnostic push with no logger initialized fails in the configurations that ship, not only in a debug assert. +- 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. +- a diagnostic range past the end of the document means the source and the check are out of sync; do not treat it as an ordinary range, and do not drop its correction unless the checker can no longer emit it. +- 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. + +**Hotspots:** `src/Compiler/Service/{ServiceCompilerDiagnostics,ServiceErrorResolutionHints}.*`, `src/Compiler/Symbols/FSharpDiagnostic.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Symbols/{XmlDocInheritance,XmlDocSigParser}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*` + +### 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. +- an assembly reader is told whether the read is for a project or an interactive session, a host-supplied reader accepts a read lock on any thread the service uses, and it stays logically immutable until the write stamp 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. +- a host on a different runtime than the compiler still resolves type providers through a shim the service owns, in a shape that allows an out-of-process host, and the service works against the core library as a referenced assembly without an in-solution workaround. + +**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 service type break is acceptable when it makes that type easier for the clients that use it; do not treat the break as free cleanup, and do not reject it only because those clients must update. +- moving or renaming a public layout or tooltip member is a source and binary break; an annotation forced by an internal resource overload is not. A new public type ships in the namespace clients should keep, and an experimental member is 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. +- a checking mode that does not differ per project belongs on the checker, not on project options. A service syntax-tree reshape may ship if existing client patterns still compile; adding trivia must not force those clients onto named patterns. New member emission is gated by language version, and a per-feature kill switch is added only for a concrete break and warns when it disables specified behavior. + +**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. +- a property name still has a property symbol when get and set are separate members, a method parameter exposes its owning method, and a named argument that is a union-case field surfaces as a field so the caller can reach the case. +- 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:** +- a declaration range includes the starting keyword, attributes, visibility, and modifiers for every construct, including inherit, interface, constructors, and fields, not only the form currently being fixed. +- 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. +- an identifier keeps the range that was parsed; a parenthesized operator is a parenthesis node around the identifier, not an identifier whose range swallows the parentheses, and that identifier range is not copied onto parent expressions as a diagnostic 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. +- a position outside a declaration is not visited as that declaration, and a walk still finds a position in unfinished code when the declaration range stops at the last token. +- breakpoint ranges the service reports match sequence points the paired compiler emits, a loop sequence point covers the pattern and not the whole comprehension, and a range change is gated on language version when a newer service can run with an older compiler. + +**Severity:** high. + +**Hotspots:** `src/Compiler/Service/ServiceNavigation.*`, `src/Compiler/Service/ServiceStructure.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*` + +### 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. + +**Hotspots:** `src/Compiler/Service/{ServiceDeclarationLists,ServiceParamInfoLocations,ServiceInterfaceStubGenerator}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAssemblyContent.*`, `tests/FSharp.Compiler.Service.Tests/Completion/**`, `tests/FSharp.Compiler.Service.Tests/ParameterInfo/**` + +### 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. +- an IDE symbol walk drops locals, parameters, and type parameters before it parses text or reads document lines, and does not also drop bindings until a use misclassified as a binding is handled. +- semantic classification keeps one winner per range, an unresolved name is colored differently from an entity whose kind is unknown, and a parse-helper change meant to fix coloring is shown to be on the coloring path with a test of the colors that were wrong. +- 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. + +**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAnalysis.*`, `src/Compiler/Service/SemanticClassification*.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*`, `src/Compiler/Service/ServiceAssemblyContent.*` + +### 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. +- a separator the service tree exposes is absent when none was written, and only written separators such as a semicolon or a comma are captured, not a keyword that merely introduces a clause. +- a single-line token stores its start position and derives the range, and a large syntax baseline is a named test per case that can dump the tokens for that case. + +**Severity:** medium. + +**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceLexing.*` + +### 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. +- a test must not treat a forced collection timeout as the product behavior, a failure that appears only when cases run together is an isolation bug, and a test must not assume an inlined internal computation expression is reachable through friend visibility from another assembly. + +**Severity:** high. + +**Hotspots:** `tests/FSharp.Compiler.Service.Tests/**` + +## 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 Review Briefing Pack + +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`. + +Select dimensions with the `reviewing-vsintegration` skill. Pack A if the diff touches `vsintegration/**`. Pack B if it touches `src/Compiler/Service/**` or `src/Compiler/Symbols/**`. Both, if both. Add the three shared dimensions when the change crosses the boundary, owns cancellation, or could make 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 pack 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` when the dimension is genuinely clean. Do not explain away real issues to produce a clean result. + +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 under the pack 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 `🤖🕵️`. + +Wave items that used to live on `@expert-reviewer` as IDE impact and incremental-checking checks are these packs. 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..c4da79375e6 --- /dev/null +++ b/.github/instructions/CompilerService.instructions.md @@ -0,0 +1,14 @@ +--- +applyTo: + - "src/Compiler/Service/**/*.{fs,fsi}" + - "src/Compiler/Symbols/**/*.{fs,fsi}" +--- + +# IDE service + +These folders are Pack B of `@vsintegration-reviewer`. Invoke that agent for a review. Do not restate its CHECK list here. + +- This API is host-neutral. No VS, MEF, JTF, or Roslyn types. Service must not depend on `vsintegration`. +- A public signature change is a break even when the private representation is unchanged. +- Cache identity is a semantic stamp of one project, not object identity. +- `@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..64b82142c3f --- /dev/null +++ b/.github/instructions/VSIntegration.instructions.md @@ -0,0 +1,12 @@ +--- +applyTo: "vsintegration/**/*.{fs,fsi}" +--- + +# VS integration + +This folder is Pack A of `@vsintegration-reviewer`. Invoke that agent for a review. Do not restate its CHECK list here. + +- Keep VS, MEF, shell, and UI-thread types in this tree. Do not push them into `src/Compiler/Service` or `src/Compiler/Symbols`. +- Do not block the UI thread, and do not call a UI-thread service from a background callback. +- An editor feature must not let an exception reach the Visual Studio UI. +- 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..c0101c70f57 100644 --- a/.github/skills/reviewing-compiler-prs/SKILL.md +++ b/.github/skills/reviewing-compiler-prs/SKILL.md @@ -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 `@vsintegration-reviewer`. Do not review those trees here. +- PR touches `LanguageServer/` — IDE responsiveness moved to `@vsintegration-reviewer`. 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/` | Invoke `@vsintegration-reviewer` Pack B. Do not apply compiler dimensions here. | +| `LanguageServer/` | IDE responsiveness: `@vsintegration-reviewer`. 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/` | Invoke `@vsintegration-reviewer` Pack A. 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..2e87c9cbe20 --- /dev/null +++ b/.github/skills/reviewing-vsintegration/SKILL.md @@ -0,0 +1,44 @@ +--- +name: reviewing-vsintegration +description: "Routes Visual Studio integration and IDE-service review to @vsintegration-reviewer. Use when a PR or edit touches vsintegration, FSharp.Editor, the legacy language service, FSI, the F# project system, MEF, src/Compiler/Service, src/Compiler/Symbols, FCS snapshots, the incremental builder, workspace, navigation, completion, or symbols. Does not review compiler type checking, IL, pickle, or FSharp.Core." +--- + +# Reviewing VS integration + +Full CHECK rules and the five-wave workflow live in `@vsintegration-reviewer`. This skill only routes folders to a pack. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. + +`@expert-reviewer` stays the compiler reviewer. Do not send Service or `vsintegration` findings there. + +## When to invoke + +- Diff touches `vsintegration/**` → Pack A +- Diff touches `src/Compiler/Service/**` or `src/Compiler/Symbols/**` → Pack B +- Diff touches both → both packs +- Diff touches `tests/FSharp.Compiler.Service.Tests/**` or `vsintegration/tests/**` → the pack those tests belong to, not a third pack +- Boundary, cancellation ownership, or a Service reference to `vsintegration` → the three shared dimensions in the agent + +## Folder routing + +| Path | Pack | Select | +|---|---|---| +| `vsintegration/src/FSharp.Editor/**` | A | UI thread, MEF, caches, quick info, navigation, completion, diagnostics scope, options, classification, feature failures | +| `vsintegration/src/FSharp.LanguageService/**` | A | Legacy language service, UI thread, solution lifetime | +| `vsintegration/src/FSharp.ProjectSystem*/**` | A | Project lifetime, UI thread, generator identity | +| `vsintegration/src/FSharp.VS.FSI/**` | A | Interactive session startup and cancellation | +| `vsintegration/tests/**` | A | Editor tests must call the host provider | +| `vsintegration/**` other | A | Same pack. Do not push host types into FCS | +| `src/Compiler/Service/**` | B | Cache identity, incremental results, locks, public FCS API, diagnostics, readers | +| `src/Compiler/Symbols/**` | B | Symbol results, ranges, completion, walks | +| `tests/FSharp.Compiler.Service.Tests/**` | B | Assert the observable service result. Isolate process-wide state | + +## Shared, and only these + +- Snapshot identity across the host/service boundary +- Cancellation ownership +- Service must not depend on `vsintegration` + +## Do not route here + +- `src/Compiler/Checking/`, `Optimize/`, `CodeGen/`, `TypedTree/`, `SyntaxTree/`, `Driver/` except a Service caller that is already in Pack B +- `src/FSharp.Core/**` +- `src/FSharp.Compiler.LanguageServer/**` — not this corpus. IDE responsiveness checks live in `@vsintegration-reviewer` because that dimension moved; do not add LanguageServer rules here From 2dadf675a63b00794d7e631df4a23aa9881976cd Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 13 Oct 2026 15:32:38 +0200 Subject: [PATCH 02/12] Anonymize VS integration reviewer wording. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/vsintegration-reviewer.md | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/.github/agents/vsintegration-reviewer.md b/.github/agents/vsintegration-reviewer.md index 2a88957fc0b..44e614347bf 100644 --- a/.github/agents/vsintegration-reviewer.md +++ b/.github/agents/vsintegration-reviewer.md @@ -11,7 +11,7 @@ Two packs. Do not merge them. A PR that touches both produces findings in both. Use the `reviewing-vsintegration` skill to route folders to a pack and to select dimensions. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. -## Moved principle +## Principle - **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. @@ -82,10 +82,6 @@ Paths: `vsintegration/**`. Editor, project system, legacy language service, FSI, - 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. -### IDE Responsiveness - -Applies to this pack. The section below is the host copy of the moved dimension. - ### 10. IDE Responsiveness The compiler service must respond to editor keystrokes without unnecessary recomputation. @@ -107,7 +103,7 @@ Language-service entry points in this pack: (Exception: top-level language servi - 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. -Hotspot moved with cancellation: `vsintegration/`. +Cancellation hotspot: `vsintegration/`. ### UI thread affinity @@ -334,7 +330,7 @@ Paths: `src/Compiler/Service/**`, `src/Compiler/Symbols/**`. Host-neutral API fo - 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. - A public signature change is a break even when the private representation is unchanged. A break can be right when it helps the clients that use the type; it is not cleanup. A service syntax-tree reshape is not that break if existing client patterns still compile. -### FCS API sentences +### FCS API surface Public API additions to FCS must be carefully controlled. Internal data structures must never leak. Breaking changes to FCS API surface are acceptable with major version bumps. @@ -360,10 +356,6 @@ The FCS public API is a permanent commitment. Internal types must never leak. --- -### IDE Responsiveness - -Applies to this pack. The section below is the service copy of the moved dimension. - ### 10. IDE Responsiveness The compiler service must respond to editor keystrokes without unnecessary recomputation. From d9b117c50365cad31ba39e1ded61e1067bc4f454 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 13 Oct 2026 15:33:02 +0200 Subject: [PATCH 03/12] Tighten VS integration review skill triggers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/skills/reviewing-vsintegration/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/skills/reviewing-vsintegration/SKILL.md b/.github/skills/reviewing-vsintegration/SKILL.md index 2e87c9cbe20..d81c7692e10 100644 --- a/.github/skills/reviewing-vsintegration/SKILL.md +++ b/.github/skills/reviewing-vsintegration/SKILL.md @@ -1,6 +1,6 @@ --- name: reviewing-vsintegration -description: "Routes Visual Studio integration and IDE-service review to @vsintegration-reviewer. Use when a PR or edit touches vsintegration, FSharp.Editor, the legacy language service, FSI, the F# project system, MEF, src/Compiler/Service, src/Compiler/Symbols, FCS snapshots, the incremental builder, workspace, navigation, completion, or symbols. Does not review compiler type checking, IL, pickle, or FSharp.Core." +description: "Selects Pack A and Pack B review dimensions for Visual Studio integration and the host-neutral F# compiler service. 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 @vsintegration-reviewer. Does not review compiler type checking, IL, pickle, or FSharp.Core." --- # Reviewing VS integration From e07ee5f414d8ea82c0e3c1a02dac07ecf428da69 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 13 Oct 2026 15:33:29 +0200 Subject: [PATCH 04/12] Deduplicate VS integration instruction reminders. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/instructions/CompilerService.instructions.md | 5 ++--- .github/instructions/VSIntegration.instructions.md | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/.github/instructions/CompilerService.instructions.md b/.github/instructions/CompilerService.instructions.md index c4da79375e6..70d09926fde 100644 --- a/.github/instructions/CompilerService.instructions.md +++ b/.github/instructions/CompilerService.instructions.md @@ -8,7 +8,6 @@ applyTo: These folders are Pack B of `@vsintegration-reviewer`. Invoke that agent for a review. Do not restate its CHECK list here. -- This API is host-neutral. No VS, MEF, JTF, or Roslyn types. Service must not depend on `vsintegration`. -- A public signature change is a break even when the private representation is unchanged. -- Cache identity is a semantic stamp of one project, not object identity. +- No VS, MEF, JTF, or Roslyn types in these folders. Service must not reference `vsintegration`. +- Public signature, cache identity, and stale results are Pack B checks. Do not restate them here. - `@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 index 64b82142c3f..fbb80788ef7 100644 --- a/.github/instructions/VSIntegration.instructions.md +++ b/.github/instructions/VSIntegration.instructions.md @@ -6,7 +6,6 @@ applyTo: "vsintegration/**/*.{fs,fsi}" This folder is Pack A of `@vsintegration-reviewer`. Invoke that agent for a review. Do not restate its CHECK list here. -- Keep VS, MEF, shell, and UI-thread types in this tree. Do not push them into `src/Compiler/Service` or `src/Compiler/Symbols`. -- Do not block the UI thread, and do not call a UI-thread service from a background callback. -- An editor feature must not let an exception reach the Visual Studio UI. +- Host types stay in this tree. Service and Symbols stay host-neutral. +- UI-thread calls and editor exceptions are Pack A checks. Do not restate them here. - For JTF, retained memory, and the perf execution map, invoke `@vs-integration-perf`. Do not copy it. From 5bd5303383071bbe5ad77a30c71d3860b61ba242 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 13 Oct 2026 15:44:39 +0200 Subject: [PATCH 05/12] Generalize an overfitted editor parallelism check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/vsintegration-reviewer.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/agents/vsintegration-reviewer.md b/.github/agents/vsintegration-reviewer.md index 44e614347bf..81afa1b9369 100644 --- a/.github/agents/vsintegration-reviewer.md +++ b/.github/agents/vsintegration-reviewer.md @@ -309,8 +309,8 @@ A keystroke, search, or tooltip starts work that keeps running after the user ha - 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. -- Tooltip navigation must use the host's built-in link and must be cancellable. If the user cancels the slower implementation lookup, fall back to the instant range. Render each tooltip element as icon, then description, then documentation. -- Before adding a custom parallelism cap for editor background work, compare it with the existing parallel combinator that already takes a maximum degree. +- Tooltip navigation must use the host's built-in link and must be cancellable. If the user cancels the slower implementation lookup, fall back to the instant range. Render each tooltip element as icon, then description, then documentation, because the host link parser reads that order. +- 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. From 3b88cd0c551ddc6a69c71258394147430d4c16b1 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 13 Oct 2026 15:47:08 +0200 Subject: [PATCH 06/12] Keep compiler rules intact when pointing at the VS reviewer. The moved IDE and FCS blocks stay in vsintegration-reviewer. Pointers no longer replace sentences that still belong here. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/expert-reviewer.md | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/.github/agents/expert-reviewer.md b/.github/agents/expert-reviewer.md index 938dfd0bacc..be958008d16 100644 --- a/.github/agents/expert-reviewer.md +++ b/.github/agents/expert-reviewer.md @@ -20,20 +20,20 @@ Evaluates F# compiler changes across 19 dimensions. Use the `reviewing-compiler- - Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - **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 FSharp.Core must be carefully controlled. FSharp.Core must remain stable. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +- **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 @vsintegration-reviewer. ## 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. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. -3. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +2. **Catch-all exception handlers** — Do not add catch-all exception handlers. Handle `OperationCanceledException` specially; never swallow it. The language-service exception moved to @vsintegration-reviewer. +3. **Internal type leakage** — Moved to @vsintegration-reviewer. 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. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +8. **Stale type-checking results** — Moved to @vsintegration-reviewer. 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. @@ -149,6 +149,8 @@ Optimizer changes must preserve program semantics. Inlining and tail-call change --- +### 7. FCS API Surface Control + Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 8. Type System Correctness @@ -182,6 +184,8 @@ Structs have value semantics that differ fundamentally from reference types. Inc --- +### 10. IDE Responsiveness + Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 11. Overload Resolution Correctness @@ -226,7 +230,7 @@ The pickled metadata format is a cross-version contract. DLLs compiled with any Async and concurrent code must handle cancellation, thread safety, and exception propagation correctly. **CHECK:** -- Thread cancellation tokens through all async operations. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +- 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. @@ -235,10 +239,14 @@ 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/` Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +**Hotspots:** `src/Compiler/Facilities/`, `src/FSharp.Core/` + +FCS request cancellation and the `vsintegration/` hotspot moved to @vsintegration-reviewer. --- +### 14. Incremental Checking Correctness + Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 15. Syntax Tree & Parser Integrity @@ -266,9 +274,10 @@ 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. -- Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - Inside the compiler, do not add catch-all exception handlers — they hide bugs. +The language-service boundary bullet moved to @vsintegration-reviewer. + **Severity:** Swallowed cancellation → **critical**. Catch-all handler → **high**. Missing error → **medium**. **Hotspots:** `src/FSharp.Core/`, `src/Compiler/Service/`, `src/Compiler/Optimize/` @@ -420,7 +429,7 @@ Execute review in six waves, each building on the previous. 1. Verify every behavioral change has a corresponding test (Dimension 1). 2. Check feature gating — new features must have `LanguageFeature` guards (Dimension 19). -3. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +3. IDE impact and incremental checking moved to @vsintegration-reviewer (Dimensions 10 and 14). 4. Check for binary compatibility concerns in pickle/import code (Dimension 12). 5. If FSharp.Core is touched, apply FSharp.Core Stability checks (Dimension 2). @@ -436,9 +445,9 @@ Execute review in six waves, each building on the previous. ### Wave 3: Runtime & Integration 1. Verify concurrency safety — no races, proper cancellation, stack traces preserved (Dimension 13). -2. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +2. IDE impact moved to @vsintegration-reviewer (Dimension 10). 3. Verify overload resolution correctness if constraint solving changes (Dimension 11). -4. Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +4. Incremental checking moved to @vsintegration-reviewer (Dimension 14). 5. Review diagnostic message quality (Dimension 17). 6. Verify debug experience — stepping, breakpoints, locals (Dimension 18). From 9ec0f1ba10c035fd922543e787372aac920b9fab Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Wed, 14 Oct 2026 11:23:43 +0200 Subject: [PATCH 07/12] Name the VS and compiler-service review files. Drop Pack A and Pack B. Each tree's rules live in the file named for that tree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/vsintegration-reviewer.md | 590 +----------------- .../CompilerService.instructions.md | 7 +- .../VSIntegration.instructions.md | 7 +- .../skills/reviewing-compiler-prs/SKILL.md | 4 +- .../skills/reviewing-vsintegration/SKILL.md | 52 +- .../compiler-service.md | 314 ++++++++++ .../reviewing-vsintegration/vsintegration.md | 250 ++++++++ 7 files changed, 615 insertions(+), 609 deletions(-) create mode 100644 .github/skills/reviewing-vsintegration/compiler-service.md create mode 100644 .github/skills/reviewing-vsintegration/vsintegration.md diff --git a/.github/agents/vsintegration-reviewer.md b/.github/agents/vsintegration-reviewer.md index 81afa1b9369..0e3d566dfc8 100644 --- a/.github/agents/vsintegration-reviewer.md +++ b/.github/agents/vsintegration-reviewer.md @@ -1,23 +1,30 @@ --- name: vsintegration-reviewer -description: "Reviews Visual Studio integration and the host-neutral F# compiler service. Use when a change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. Pack A is the VS host. Pack B is the IDE service. Does not replace @expert-reviewer." +description: "Reviews Visual Studio integration and the host-neutral compiler service. Use when a change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. Read vsintegration.md or compiler-service.md for the tree the diff touches. Does not replace @expert-reviewer." --- -# VS Integration Reviewer +# VS integration reviewer Sibling of `@expert-reviewer`. Does not replace it. Compiler types, IL, pickle, optimizer, feature gates, FSharp.Core, and compiler diagnostics stay there. -Two packs. Do not merge them. A PR that touches both produces findings in both. File an inline comment under the pack of its file path. +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. -Use the `reviewing-vsintegration` skill to route folders to a pack and to select dimensions. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. +| 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 - **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. -## Shared boundary +## Both trees -Only these three rules may appear in both packs. Everything else stays in one pack. +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.) @@ -26,8 +33,6 @@ Only these three rules may appear in both packs. Everything else stays in one pa All FCS requests must support cancellation. -### Shared dimensions - ### Snapshot identity across the boundary Host and service must share one identity for source text, project options, and dependency freshness. A hash code, checksum, private host field, or bare timestamp is not that identity, and a notification that does not name the text must not invalidate the parse. @@ -69,571 +74,12 @@ Host-neutral editor capabilities live on the public service API. The checker mus **Severity:** high. -## Pack A — VS host - -Paths: `vsintegration/**`. Editor, project system, legacy language service, FSI, debug adapter, MEF, UI thread. Must not push host types into FCS. - -- 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. - -### 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/` - ---- - -Language-service entry points in this pack: (Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) - -- 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. - -Cancellation hotspot: `vsintegration/`. - -### 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. -- A newer host version is not evidence that project-system initialization left the UI thread. Keep that work on the UI thread until the host contract says otherwise. -- Package initialization and workspace updates that do not touch UI services must not take the UI thread, and a workspace project context whose host factory is built for the background must not stay on the UI thread. -- Resolve a UI-thread service when the editor part is created, on the main thread, through one main-thread or async lookup. Do not fetch it from a background tooltip or command callback, which deadlocks even when the result is unused. -- 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. -- A MEF view factory must not be visible to other languages. Keep custom visuals on an internal type so another language's tooltip does not load the F# editor. -- 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. -- Editor options must be a composed part that features can import. Expose as little of the editor to MEF as possible: a small factory should choose the host or test store instead of exporting the whole options implementation, and do not keep a static singleton the language service initializes by hand. -- Assign a process-global resource assembly once, from a composition singleton. Assigning it again for each view throws. The options page must open with no solution loaded. -- 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 the host signals that the solution UI is ready. Do not replace that signal with a custom wait or a fixed 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. -- On solution close, run the same cleanup as document close and release documents that are still open, because the workspace does not raise a close for them. Fix a project-close race by ordering disconnect against in-flight work, not by delaying the call. -- 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:** -- Quick info and inline hints must show the compiler's existing tooltip text, must not show a parameter value that belongs to signature help, and must not ship a debug tooltip that shows an internal layout tag. -- A section separator must have a fixed length. Do not size it by repeating glyphs or draw it as ASCII art, and do not treat a hand-drawn divider as wrapping. Break long documentation explicitly. -- Quick-info maximum width must scale with the editor font. Documentation may match the signature width only up to a fixed cap, alignment must follow the document reading direction, and documentation must not be fully justified. -- A quick-info section that other languages expose as a setting must be configurable, not always shown. A secondary tooltip must follow the active theme, and a path it displays must be relative to the solution. -- 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. -- Quick-info content must stay a view model another host can translate. Do not build that model out of a Windows-only element. Record and value text shown in tools should keep a single space between braces and fields. -- Do not add a width control that fights the host's own tooltip wrapping. If a setting does not always apply, mark it experimental and explain the effect. Quick info must keep line breaks between sections, and long documentation must still wrap after a host API change. - -**Severity:** medium. - -**Hotspots:** `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/Hints/**`, `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.UIResources/**` - -### 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 request must return a navigation item and must not open a window itself. Hover must not jump to decompiled metadata because the pointer is over an external symbol. -- 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. -- Completion must not invent a method body. Override completion must accept the member name first and the body in a later step, and must not insert the whole override, including a generated body, on one accept. -- 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. -- A name suggestion should reuse the completion items for that context when that lookup is cheap. Names the editor shows must stay separate from the representation that guides compiled form. - -**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. -- A diagnostics exporter must not stay enabled on the analysis path. Left running, it adds overhead that shows up as a compile-time regression. -- Editor analyzers must follow the host's live-analysis scope setting. Do not invent a private active-document gate beside that setting. -- A host limit or setting must be applied on the override or callback the host actually invokes. A limit that lives only in an initialization method the host no longer calls is never applied. -- 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:** -- Declare, store, and retrieve each editor setting on its own so a newly added setting keeps its default. Do not persist the whole settings type as one reflected blob. -- An options type the settings page binds must keep the marker that makes its properties settable, and options-page 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:** -- Classify a name by its language role. A function must not be colored as a keyword, and a method and a function value must not share a classification when their calling rules differ. -- 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. -- Do not assign a fixed foreground on a classification format. Leave it unset so the host theme supplies a readable color and the user can still customize it. -- 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 use a process-global resource assembly to locate one extension's dictionaries. Another extension assigning that same property causes a runtime failure. Resolve resources from this extension's install location. - -**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 one MEF composition instead of building a new export provider for every test. Tests must not share a process-wide options part across cases. If desktop tests share one host, a throw from a background thread must still name the case. -- 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. -- Tooltip navigation must use the host's built-in link and must be cancellable. If the user cancels the slower implementation lookup, fall back to the instant range. Render each tooltip element as icon, then description, then documentation, because the host link parser reads that order. -- 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/**` - - -## Pack B — IDE service - -Paths: `src/Compiler/Service/**`, `src/Compiler/Symbols/**`. Host-neutral API for IDEs. No VS, MEF, JTF, or Roslyn types. Checker, incremental builder, snapshots, workspace, navigation, completion, symbols, diagnostics surface. Tests in `tests/FSharp.Compiler.Service.Tests/**` belong to this pack. They are not a third pack. - -- The service stays host-neutral. A capability an editor needs is exposed on the existing checker or symbol shape, a host test does not prove it, and a host on another runtime must not keep a private fork of a shim the service should own. -- A cache identity is a semantic stamp of one project closure: options, source text, earlier files, included scripts, and reference write times. A similar path, a mutable options object, or a live typed graph is not that stamp. -- 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. -- A public signature change is a break even when the private representation is unchanged. A break can be right when it helps the clients that use the type; it is not cleanup. A service syntax-tree reshape is not that break if existing client patterns still compile. - -### FCS API surface - -Public API additions to FCS must be carefully controlled. Internal data structures must never leak. Breaking changes to FCS API surface are acceptable with major version bumps. - -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. - -8. **Stale type-checking results** — Avoid returning stale results; they cause timing-dependent IntelliSense glitches. Prefer fresh results with cancellation support. - -### 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/` - ---- - -### 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/` - ---- - -### 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/Service/`, `src/Compiler/Driver/` - ---- - -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.) - -- 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. - -### 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:** -- a cache key is a slim structural value with the same equality as the type, not the live typed object, and a snapshot key is a frozen semantic stamp, not a mutable options object. -- a clear or lookup targets the same project options only; another target framework that shares the path is a different project, and project identity strings compare ordinally unless the host already normalized them. -- a cached file result stays valid only while earlier files in that project are unchanged, a script snapshot changes when an included script changes, and a referenced in-memory module carries a freshness stamp checked like a write time. -- 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. -- a parse or check cache does not return a result that dropped the types of values, a parse uses only the source the caller passed, and a host that supplies source text hits the same parse cache as an on-disk path. -- 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. -- a parse that feeds navigation or breakpoint ranges does not wait behind the shared analysis reactor, and a project parse-and-check returns parse errors from the same parse the check used. -- a checking setting does not split the builder into two implementations; one path that can turn the setting on is the shape to keep. -- a builder-owned parse cache is one switch, does not retain an implementation that has a signature, and is reclaimable with that builder; do not add a size threshold until measurement shows the weak cache is not enough. A parse cache that is not tied to a project stays small. -- 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. -- build-graph request continuations run asynchronously, an eager parse is queued when the node is created, and concurrent incremental updates share one builder and one in-flight parse under the same lock. -- 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. -- a type error does not drop the failed declaration from the environment or skip later uses; the name stays bound so tooling can still resolve it. -- 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. -- a recovery point that does not use the exception catches every failure, so an unlisted exception does not abort the rest of the check. - -**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. -- a computation that installs an ambient diagnostics logger restores the previous logger, and a diagnostic push with no logger initialized fails in the configurations that ship, not only in a debug assert. -- 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. -- a diagnostic range past the end of the document means the source and the check are out of sync; do not treat it as an ordinary range, and do not drop its correction unless the checker can no longer emit it. -- 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. - -**Hotspots:** `src/Compiler/Service/{ServiceCompilerDiagnostics,ServiceErrorResolutionHints}.*`, `src/Compiler/Symbols/FSharpDiagnostic.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Symbols/{XmlDocInheritance,XmlDocSigParser}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*` - -### 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. -- an assembly reader is told whether the read is for a project or an interactive session, a host-supplied reader accepts a read lock on any thread the service uses, and it stays logically immutable until the write stamp 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. -- a host on a different runtime than the compiler still resolves type providers through a shim the service owns, in a shape that allows an out-of-process host, and the service works against the core library as a referenced assembly without an in-solution workaround. - -**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 service type break is acceptable when it makes that type easier for the clients that use it; do not treat the break as free cleanup, and do not reject it only because those clients must update. -- moving or renaming a public layout or tooltip member is a source and binary break; an annotation forced by an internal resource overload is not. A new public type ships in the namespace clients should keep, and an experimental member is 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. -- a checking mode that does not differ per project belongs on the checker, not on project options. A service syntax-tree reshape may ship if existing client patterns still compile; adding trivia must not force those clients onto named patterns. New member emission is gated by language version, and a per-feature kill switch is added only for a concrete break and warns when it disables specified behavior. - -**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. -- a property name still has a property symbol when get and set are separate members, a method parameter exposes its owning method, and a named argument that is a union-case field surfaces as a field so the caller can reach the case. -- 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:** -- a declaration range includes the starting keyword, attributes, visibility, and modifiers for every construct, including inherit, interface, constructors, and fields, not only the form currently being fixed. -- 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. -- an identifier keeps the range that was parsed; a parenthesized operator is a parenthesis node around the identifier, not an identifier whose range swallows the parentheses, and that identifier range is not copied onto parent expressions as a diagnostic 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. -- a position outside a declaration is not visited as that declaration, and a walk still finds a position in unfinished code when the declaration range stops at the last token. -- breakpoint ranges the service reports match sequence points the paired compiler emits, a loop sequence point covers the pattern and not the whole comprehension, and a range change is gated on language version when a newer service can run with an older compiler. - -**Severity:** high. - -**Hotspots:** `src/Compiler/Service/ServiceNavigation.*`, `src/Compiler/Service/ServiceStructure.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*` - -### 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. - -**Hotspots:** `src/Compiler/Service/{ServiceDeclarationLists,ServiceParamInfoLocations,ServiceInterfaceStubGenerator}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAssemblyContent.*`, `tests/FSharp.Compiler.Service.Tests/Completion/**`, `tests/FSharp.Compiler.Service.Tests/ParameterInfo/**` - -### 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. -- an IDE symbol walk drops locals, parameters, and type parameters before it parses text or reads document lines, and does not also drop bindings until a use misclassified as a binding is handled. -- semantic classification keeps one winner per range, an unresolved name is colored differently from an entity whose kind is unknown, and a parse-helper change meant to fix coloring is shown to be on the coloring path with a test of the colors that were wrong. -- 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. - -**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAnalysis.*`, `src/Compiler/Service/SemanticClassification*.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*`, `src/Compiler/Service/ServiceAssemblyContent.*` - -### 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. -- a separator the service tree exposes is absent when none was written, and only written separators such as a semicolon or a comma are captured, not a keyword that merely introduces a clause. -- a single-line token stores its start position and derives the range, and a large syntax baseline is a named test per case that can dump the tokens for that case. - -**Severity:** medium. - -**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceLexing.*` - -### 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. -- a test must not treat a forced collection timeout as the product behavior, a failure that appears only when cases run together is an isolation bug, and a test must not assume an inlined internal computation expression is reachable through friend visibility from another assembly. - -**Severity:** high. - -**Hotspots:** `tests/FSharp.Compiler.Service.Tests/**` ## 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 Review Briefing Pack +### 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. @@ -651,9 +97,9 @@ gh pr diff {pr_number} --repo {owner}/{repo} Do not use raw `git diff main..branch`. -Select dimensions with the `reviewing-vsintegration` skill. Pack A if the diff touches `vsintegration/**`. Pack B if it touches `src/Compiler/Service/**` or `src/Compiler/Symbols/**`. Both, if both. Add the three shared dimensions when the change crosses the boundary, owns cancellation, or could make 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. +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 pack to a file. Every Wave 1 sub-agent receives it. +Save the briefing to a file. Every Wave 1 sub-agent receives it. ### Wave 1: Find @@ -665,7 +111,7 @@ Report an ISSUE only when you can construct a concrete failing scenario: a speci 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 under the pack 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. +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 @@ -693,7 +139,7 @@ $Concrete scenario that triggers the bug. Post design-level concerns that are not tied to a line as one PR comment, one bullet each, still starting with `🤖🕵️`. -Wave items that used to live on `@expert-reviewer` as IDE impact and incremental-checking checks are these packs. Do not send those findings back to `@expert-reviewer`. +IDE impact and incremental-checking checks live in the two files above. Do not send those findings back to `@expert-reviewer`. ### Wave 4: Summary diff --git a/.github/instructions/CompilerService.instructions.md b/.github/instructions/CompilerService.instructions.md index 70d09926fde..09d59417b79 100644 --- a/.github/instructions/CompilerService.instructions.md +++ b/.github/instructions/CompilerService.instructions.md @@ -4,10 +4,9 @@ applyTo: - "src/Compiler/Symbols/**/*.{fs,fsi}" --- -# IDE service +# Compiler service -These folders are Pack B of `@vsintegration-reviewer`. Invoke that agent for a review. Do not restate its CHECK list here. +Read `.github/skills/reviewing-vsintegration/compiler-service.md`. Invoke `@vsintegration-reviewer`. Do not restate that file here. -- No VS, MEF, JTF, or Roslyn types in these folders. Service must not reference `vsintegration`. -- Public signature, cache identity, and stale results are Pack B checks. Do not restate them 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 index fbb80788ef7..6d71e96e917 100644 --- a/.github/instructions/VSIntegration.instructions.md +++ b/.github/instructions/VSIntegration.instructions.md @@ -2,10 +2,9 @@ applyTo: "vsintegration/**/*.{fs,fsi}" --- -# VS integration +# Visual Studio integration -This folder is Pack A of `@vsintegration-reviewer`. Invoke that agent for a review. Do not restate its CHECK list here. +Read `.github/skills/reviewing-vsintegration/vsintegration.md`. Invoke `@vsintegration-reviewer`. Do not restate that file here. -- Host types stay in this tree. Service and Symbols stay host-neutral. -- UI-thread calls and editor exceptions are Pack A checks. Do not restate them 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 c0101c70f57..70707047eca 100644 --- a/.github/skills/reviewing-compiler-prs/SKILL.md +++ b/.github/skills/reviewing-compiler-prs/SKILL.md @@ -25,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/`, `Symbols/` | Invoke `@vsintegration-reviewer` Pack B. Do not apply compiler dimensions here. | +| `Service/`, `Symbols/` | Read `compiler-service.md` via `@vsintegration-reviewer`. Do not apply compiler dimensions here. | | `LanguageServer/` | IDE responsiveness: `@vsintegration-reviewer`. 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/` | Invoke `@vsintegration-reviewer` Pack A. Memory and JTF: `@vs-integration-perf`. | +| `vsintegration/` | Read `vsintegration.md` via `@vsintegration-reviewer`. 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 index d81c7692e10..b87c114542d 100644 --- a/.github/skills/reviewing-vsintegration/SKILL.md +++ b/.github/skills/reviewing-vsintegration/SKILL.md @@ -1,44 +1,42 @@ --- name: reviewing-vsintegration -description: "Selects Pack A and Pack B review dimensions for Visual Studio integration and the host-neutral F# compiler service. 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 @vsintegration-reviewer. Does not review compiler type checking, IL, pickle, or FSharp.Core." +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 @vsintegration-reviewer. Does not review compiler type checking, IL, pickle, or FSharp.Core." --- # Reviewing VS integration -Full CHECK rules and the five-wave workflow live in `@vsintegration-reviewer`. This skill only routes folders to a pack. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. +CHECK rules live in two files. The five-wave workflow lives in `@vsintegration-reviewer`. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. -`@expert-reviewer` stays the compiler reviewer. Do not send Service or `vsintegration` findings there. +`@expert-reviewer` stays the compiler reviewer. Do not send `vsintegration` or `src/Compiler/Service` findings there. -## When to invoke +## Which file -- Diff touches `vsintegration/**` → Pack A -- Diff touches `src/Compiler/Service/**` or `src/Compiler/Symbols/**` → Pack B -- Diff touches both → both packs -- Diff touches `tests/FSharp.Compiler.Service.Tests/**` or `vsintegration/tests/**` → the pack those tests belong to, not a third pack -- Boundary, cancellation ownership, or a Service reference to `vsintegration` → the three shared dimensions in the agent +| 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) | + +A change that crosses the two trees, owns cancellation, or could make `src/Compiler/Service` depend on `vsintegration` also uses the three rules under Both trees in `@vsintegration-reviewer`. ## Folder routing -| Path | Pack | Select | +| Path | File | Select | |---|---|---| -| `vsintegration/src/FSharp.Editor/**` | A | UI thread, MEF, caches, quick info, navigation, completion, diagnostics scope, options, classification, feature failures | -| `vsintegration/src/FSharp.LanguageService/**` | A | Legacy language service, UI thread, solution lifetime | -| `vsintegration/src/FSharp.ProjectSystem*/**` | A | Project lifetime, UI thread, generator identity | -| `vsintegration/src/FSharp.VS.FSI/**` | A | Interactive session startup and cancellation | -| `vsintegration/tests/**` | A | Editor tests must call the host provider | -| `vsintegration/**` other | A | Same pack. Do not push host types into FCS | -| `src/Compiler/Service/**` | B | Cache identity, incremental results, locks, public FCS API, diagnostics, readers | -| `src/Compiler/Symbols/**` | B | Symbol results, ranges, completion, walks | -| `tests/FSharp.Compiler.Service.Tests/**` | B | Assert the observable service result. Isolate process-wide state | - -## Shared, and only these - -- Snapshot identity across the host/service boundary -- Cancellation ownership -- Service must not depend on `vsintegration` +| `vsintegration/src/FSharp.Editor/**` | vsintegration.md | UI thread, MEF, caches, quick info, navigation, completion, diagnostics scope, options, classification, feature failures | +| `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/`, `Optimize/`, `CodeGen/`, `TypedTree/`, `SyntaxTree/`, `Driver/` except a Service caller that is already in Pack B +- `src/Compiler/Checking/`, `Optimize/`, `CodeGen/`, `TypedTree/`, `SyntaxTree/`, `Driver/` - `src/FSharp.Core/**` -- `src/FSharp.Compiler.LanguageServer/**` — not this corpus. IDE responsiveness checks live in `@vsintegration-reviewer` because that dimension moved; do not add LanguageServer rules here +- `src/FSharp.Compiler.LanguageServer/**` — not this corpus. The IDE responsiveness section in both files still names that hotspot because that section was moved intact. diff --git a/.github/skills/reviewing-vsintegration/compiler-service.md b/.github/skills/reviewing-vsintegration/compiler-service.md new file mode 100644 index 00000000000..d766c91a09a --- /dev/null +++ b/.github/skills/reviewing-vsintegration/compiler-service.md @@ -0,0 +1,314 @@ +# 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. + +- The service stays host-neutral. A capability an editor needs is exposed on the existing checker or symbol shape, a host test does not prove it, and a host on another runtime must not keep a private fork of a shim the service should own. +- A cache identity is a semantic stamp of one project closure: options, source text, earlier files, included scripts, and reference write times. A similar path, a mutable options object, or a live typed graph is not that stamp. +- 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. +- A public signature change is a break even when the private representation is unchanged. A break can be right when it helps the clients that use the type; it is not cleanup. A service syntax-tree reshape is not that break if existing client patterns still compile. + +### FCS API surface + +Public API additions to FCS must be carefully controlled. Internal data structures must never leak. Breaking changes to FCS API surface are acceptable with major version bumps. + +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. + +8. **Stale type-checking results** — Avoid returning stale results; they cause timing-dependent IntelliSense glitches. Prefer fresh results with cancellation support. + +### 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/` + +--- + +### 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/` + +--- + +### 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/Service/`, `src/Compiler/Driver/` + +--- + +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.) + +- 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. + +### 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:** +- a cache key is a slim structural value with the same equality as the type, not the live typed object, and a snapshot key is a frozen semantic stamp, not a mutable options object. +- a clear or lookup targets the same project options only; another target framework that shares the path is a different project, and project identity strings compare ordinally unless the host already normalized them. +- a cached file result stays valid only while earlier files in that project are unchanged, a script snapshot changes when an included script changes, and a referenced in-memory module carries a freshness stamp checked like a write time. +- 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. +- a parse or check cache does not return a result that dropped the types of values, a parse uses only the source the caller passed, and a host that supplies source text hits the same parse cache as an on-disk path. +- 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. +- a parse that feeds navigation or breakpoint ranges does not wait behind the shared analysis reactor, and a project parse-and-check returns parse errors from the same parse the check used. +- a checking setting does not split the builder into two implementations; one path that can turn the setting on is the shape to keep. +- a builder-owned parse cache is one switch, does not retain an implementation that has a signature, and is reclaimable with that builder; do not add a size threshold until measurement shows the weak cache is not enough. A parse cache that is not tied to a project stays small. +- 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. +- build-graph request continuations run asynchronously, an eager parse is queued when the node is created, and concurrent incremental updates share one builder and one in-flight parse under the same lock. +- 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. +- a type error does not drop the failed declaration from the environment or skip later uses; the name stays bound so tooling can still resolve it. +- 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. +- a recovery point that does not use the exception catches every failure, so an unlisted exception does not abort the rest of the check. + +**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. +- a computation that installs an ambient diagnostics logger restores the previous logger, and a diagnostic push with no logger initialized fails in the configurations that ship, not only in a debug assert. +- 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. +- a diagnostic range past the end of the document means the source and the check are out of sync; do not treat it as an ordinary range, and do not drop its correction unless the checker can no longer emit it. +- 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. + +**Hotspots:** `src/Compiler/Service/{ServiceCompilerDiagnostics,ServiceErrorResolutionHints}.*`, `src/Compiler/Symbols/FSharpDiagnostic.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Symbols/{XmlDocInheritance,XmlDocSigParser}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*` + +### 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. +- an assembly reader is told whether the read is for a project or an interactive session, a host-supplied reader accepts a read lock on any thread the service uses, and it stays logically immutable until the write stamp 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. +- a host on a different runtime than the compiler still resolves type providers through a shim the service owns, in a shape that allows an out-of-process host, and the service works against the core library as a referenced assembly without an in-solution workaround. + +**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 service type break is acceptable when it makes that type easier for the clients that use it; do not treat the break as free cleanup, and do not reject it only because those clients must update. +- moving or renaming a public layout or tooltip member is a source and binary break; an annotation forced by an internal resource overload is not. A new public type ships in the namespace clients should keep, and an experimental member is 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. +- a checking mode that does not differ per project belongs on the checker, not on project options. A service syntax-tree reshape may ship if existing client patterns still compile; adding trivia must not force those clients onto named patterns. New member emission is gated by language version, and a per-feature kill switch is added only for a concrete break and warns when it disables specified behavior. + +**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. +- a property name still has a property symbol when get and set are separate members, a method parameter exposes its owning method, and a named argument that is a union-case field surfaces as a field so the caller can reach the case. +- 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:** +- a declaration range includes the starting keyword, attributes, visibility, and modifiers for every construct, including inherit, interface, constructors, and fields, not only the form currently being fixed. +- 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. +- an identifier keeps the range that was parsed; a parenthesized operator is a parenthesis node around the identifier, not an identifier whose range swallows the parentheses, and that identifier range is not copied onto parent expressions as a diagnostic 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. +- a position outside a declaration is not visited as that declaration, and a walk still finds a position in unfinished code when the declaration range stops at the last token. +- breakpoint ranges the service reports match sequence points the paired compiler emits, a loop sequence point covers the pattern and not the whole comprehension, and a range change is gated on language version when a newer service can run with an older compiler. + +**Severity:** high. + +**Hotspots:** `src/Compiler/Service/ServiceNavigation.*`, `src/Compiler/Service/ServiceStructure.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*` + +### 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. + +**Hotspots:** `src/Compiler/Service/{ServiceDeclarationLists,ServiceParamInfoLocations,ServiceInterfaceStubGenerator}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAssemblyContent.*`, `tests/FSharp.Compiler.Service.Tests/Completion/**`, `tests/FSharp.Compiler.Service.Tests/ParameterInfo/**` + +### 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. +- an IDE symbol walk drops locals, parameters, and type parameters before it parses text or reads document lines, and does not also drop bindings until a use misclassified as a binding is handled. +- semantic classification keeps one winner per range, an unresolved name is colored differently from an entity whose kind is unknown, and a parse-helper change meant to fix coloring is shown to be on the coloring path with a test of the colors that were wrong. +- 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. + +**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAnalysis.*`, `src/Compiler/Service/SemanticClassification*.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*`, `src/Compiler/Service/ServiceAssemblyContent.*` + +### 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. +- a separator the service tree exposes is absent when none was written, and only written separators such as a semicolon or a comma are captured, not a keyword that merely introduces a clause. +- a single-line token stores its start position and derives the range, and a large syntax baseline is a named test per case that can dump the tokens for that case. + +**Severity:** medium. + +**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceLexing.*` + +### 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. +- a test must not treat a forced collection timeout as the product behavior, a failure that appears only when cases run together is an isolation bug, and a test must not assume an inlined internal computation expression is reachable through friend visibility from another assembly. + +**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..3676887f645 --- /dev/null +++ b/.github/skills/reviewing-vsintegration/vsintegration.md @@ -0,0 +1,250 @@ +# Visual Studio integration + +Paths: `vsintegration/**`. Tests: `vsintegration/tests/**`. + +Editor, project system, legacy language service, FSI, debug adapter, MEF, and the UI thread. Do not push Visual Studio, MEF, JTF, or Roslyn types into `src/Compiler/Service` or `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. + +### 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/` + +--- + +(Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) + +- 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. + +Cancellation hotspot: `vsintegration/`. + +### 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. +- A newer host version is not evidence that project-system initialization left the UI thread. Keep that work on the UI thread until the host contract says otherwise. +- Package initialization and workspace updates that do not touch UI services must not take the UI thread, and a workspace project context whose host factory is built for the background must not stay on the UI thread. +- Resolve a UI-thread service when the editor part is created, on the main thread, through one main-thread or async lookup. Do not fetch it from a background tooltip or command callback, which deadlocks even when the result is unused. +- 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. +- A MEF view factory must not be visible to other languages. Keep custom visuals on an internal type so another language's tooltip does not load the F# editor. +- 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. +- Editor options must be a composed part that features can import. Expose as little of the editor to MEF as possible: a small factory should choose the host or test store instead of exporting the whole options implementation, and do not keep a static singleton the language service initializes by hand. +- Assign a process-global resource assembly once, from a composition singleton. Assigning it again for each view throws. The options page must open with no solution loaded. +- 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 the host signals that the solution UI is ready. Do not replace that signal with a custom wait or a fixed 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. +- On solution close, run the same cleanup as document close and release documents that are still open, because the workspace does not raise a close for them. Fix a project-close race by ordering disconnect against in-flight work, not by delaying the call. +- 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:** +- Quick info and inline hints must show the compiler's existing tooltip text, must not show a parameter value that belongs to signature help, and must not ship a debug tooltip that shows an internal layout tag. +- A section separator must have a fixed length. Do not size it by repeating glyphs or draw it as ASCII art, and do not treat a hand-drawn divider as wrapping. Break long documentation explicitly. +- Quick-info maximum width must scale with the editor font. Documentation may match the signature width only up to a fixed cap, alignment must follow the document reading direction, and documentation must not be fully justified. +- A quick-info section that other languages expose as a setting must be configurable, not always shown. A secondary tooltip must follow the active theme, and a path it displays must be relative to the solution. +- 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. +- Quick-info content must stay a view model another host can translate. Do not build that model out of a Windows-only element. Record and value text shown in tools should keep a single space between braces and fields. +- Do not add a width control that fights the host's own tooltip wrapping. If a setting does not always apply, mark it experimental and explain the effect. Quick info must keep line breaks between sections, and long documentation must still wrap after a host API change. + +**Severity:** medium. + +**Hotspots:** `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/Hints/**`, `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.UIResources/**` + +### 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 request must return a navigation item and must not open a window itself. Hover must not jump to decompiled metadata because the pointer is over an external symbol. +- 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. +- Completion must not invent a method body. Override completion must accept the member name first and the body in a later step, and must not insert the whole override, including a generated body, on one accept. +- 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. +- A name suggestion should reuse the completion items for that context when that lookup is cheap. Names the editor shows must stay separate from the representation that guides compiled form. + +**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. +- A diagnostics exporter must not stay enabled on the analysis path. Left running, it adds overhead that shows up as a compile-time regression. +- Editor analyzers must follow the host's live-analysis scope setting. Do not invent a private active-document gate beside that setting. +- A host limit or setting must be applied on the override or callback the host actually invokes. A limit that lives only in an initialization method the host no longer calls is never applied. +- 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:** +- Declare, store, and retrieve each editor setting on its own so a newly added setting keeps its default. Do not persist the whole settings type as one reflected blob. +- An options type the settings page binds must keep the marker that makes its properties settable, and options-page 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:** +- Classify a name by its language role. A function must not be colored as a keyword, and a method and a function value must not share a classification when their calling rules differ. +- 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. +- Do not assign a fixed foreground on a classification format. Leave it unset so the host theme supplies a readable color and the user can still customize it. +- 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 use a process-global resource assembly to locate one extension's dictionaries. Another extension assigning that same property causes a runtime failure. Resolve resources from this extension's install location. + +**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 one MEF composition instead of building a new export provider for every test. Tests must not share a process-wide options part across cases. If desktop tests share one host, a throw from a background thread must still name the case. +- 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. +- Tooltip navigation must use the host's built-in link and must be cancellable. If the user cancels the slower implementation lookup, fall back to the instant range. Render each tooltip element as icon, then description, then documentation, because the host link parser reads that order. +- 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/**` + + From 73c2abf3012d45f0dfc4c2daf91cf1db4546fd86 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Wed, 14 Oct 2026 11:25:30 +0200 Subject: [PATCH 08/12] Rename the IDE reviewer agent to ide-integration. The invoke name no longer says VS. Tree files stay named for their folders. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/expert-reviewer.md | 28 +++++++++---------- ...gration-reviewer.md => ide-integration.md} | 6 ++-- .../CompilerService.instructions.md | 2 +- .../VSIntegration.instructions.md | 2 +- .../skills/reviewing-compiler-prs/SKILL.md | 10 +++---- .../skills/reviewing-vsintegration/SKILL.md | 6 ++-- 6 files changed, 27 insertions(+), 27 deletions(-) rename .github/agents/{vsintegration-reviewer.md => ide-integration.md} (97%) diff --git a/.github/agents/expert-reviewer.md b/.github/agents/expert-reviewer.md index be958008d16..ab030527f81 100644 --- a/.github/agents/expert-reviewer.md +++ b/.github/agents/expert-reviewer.md @@ -7,7 +7,7 @@ description: "Multi-dimensional code review agent for F# compiler PRs. Evaluates Evaluates F# compiler changes across 19 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), `@vsintegration-reviewer` (VS host and IDE service). +**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,23 +17,23 @@ 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. -- Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +- Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - **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 FSharp.Core must be carefully controlled. FSharp.Core must remain stable. The FCS half of this rule moved to @vsintegration-reviewer. +- **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. The language-service exception moved to @vsintegration-reviewer. -3. **Internal type leakage** — Moved to @vsintegration-reviewer. +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. **Internal type leakage** — Moved to @ide-integration. 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** — Moved to @vsintegration-reviewer. +8. **Stale type-checking results** — Moved to @ide-integration. 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. @@ -151,7 +151,7 @@ Optimizer changes must preserve program semantics. Inlining and tail-call change ### 7. FCS API Surface Control -Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 8. Type System Correctness @@ -186,7 +186,7 @@ Structs have value semantics that differ fundamentally from reference types. Inc ### 10. IDE Responsiveness -Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 11. Overload Resolution Correctness @@ -241,13 +241,13 @@ Async and concurrent code must handle cancellation, thread safety, and exception **Hotspots:** `src/Compiler/Facilities/`, `src/FSharp.Core/` -FCS request cancellation and the `vsintegration/` hotspot moved to @vsintegration-reviewer. +FCS request cancellation and the `vsintegration/` hotspot moved to @ide-integration. --- ### 14. Incremental Checking Correctness -Moved to @vsintegration-reviewer. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. +Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. ### 15. Syntax Tree & Parser Integrity @@ -276,7 +276,7 @@ Exception handling must be precise. Catch-all handlers and swallowed exceptions - Returning `Unchecked.defaultof<_>` to swallow exceptions is dangerous — investigate and fix the root cause. - Inside the compiler, do not add catch-all exception handlers — they hide bugs. -The language-service boundary bullet moved to @vsintegration-reviewer. +The language-service boundary bullet moved to @ide-integration. **Severity:** Swallowed cancellation → **critical**. Catch-all handler → **high**. Missing error → **medium**. @@ -429,7 +429,7 @@ Execute review in six waves, each building on the previous. 1. Verify every behavioral change has a corresponding test (Dimension 1). 2. Check feature gating — new features must have `LanguageFeature` guards (Dimension 19). -3. IDE impact and incremental checking moved to @vsintegration-reviewer (Dimensions 10 and 14). +3. IDE impact and incremental checking moved to @ide-integration (Dimensions 10 and 14). 4. Check for binary compatibility concerns in pickle/import code (Dimension 12). 5. If FSharp.Core is touched, apply FSharp.Core Stability checks (Dimension 2). @@ -445,9 +445,9 @@ Execute review in six waves, each building on the previous. ### Wave 3: Runtime & Integration 1. Verify concurrency safety — no races, proper cancellation, stack traces preserved (Dimension 13). -2. IDE impact moved to @vsintegration-reviewer (Dimension 10). +2. IDE impact moved to @ide-integration (Dimension 10). 3. Verify overload resolution correctness if constraint solving changes (Dimension 11). -4. Incremental checking moved to @vsintegration-reviewer (Dimension 14). +4. Incremental checking moved to @ide-integration (Dimension 14). 5. Review diagnostic message quality (Dimension 17). 6. Verify debug experience — stepping, breakpoints, locals (Dimension 18). diff --git a/.github/agents/vsintegration-reviewer.md b/.github/agents/ide-integration.md similarity index 97% rename from .github/agents/vsintegration-reviewer.md rename to .github/agents/ide-integration.md index 0e3d566dfc8..cdf7dc27878 100644 --- a/.github/agents/vsintegration-reviewer.md +++ b/.github/agents/ide-integration.md @@ -1,9 +1,9 @@ --- -name: vsintegration-reviewer -description: "Reviews Visual Studio integration and the host-neutral compiler service. Use when a change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. Read vsintegration.md or compiler-service.md for the tree the diff touches. Does not replace @expert-reviewer." +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." --- -# VS integration 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. diff --git a/.github/instructions/CompilerService.instructions.md b/.github/instructions/CompilerService.instructions.md index 09d59417b79..c1a0f77b487 100644 --- a/.github/instructions/CompilerService.instructions.md +++ b/.github/instructions/CompilerService.instructions.md @@ -6,7 +6,7 @@ applyTo: # Compiler service -Read `.github/skills/reviewing-vsintegration/compiler-service.md`. Invoke `@vsintegration-reviewer`. Do not restate that file here. +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 index 6d71e96e917..cb8ac964ac8 100644 --- a/.github/instructions/VSIntegration.instructions.md +++ b/.github/instructions/VSIntegration.instructions.md @@ -4,7 +4,7 @@ applyTo: "vsintegration/**/*.{fs,fsi}" # Visual Studio integration -Read `.github/skills/reviewing-vsintegration/vsintegration.md`. Invoke `@vsintegration-reviewer`. Do not restate that file here. +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 70707047eca..aa0142d1ea5 100644 --- a/.github/skills/reviewing-compiler-prs/SKILL.md +++ b/.github/skills/reviewing-compiler-prs/SKILL.md @@ -11,8 +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/`, `src/Compiler/Service/`, or `src/Compiler/Symbols/` — invoke `@vsintegration-reviewer`. Do not review those trees here. -- PR touches `LanguageServer/` — IDE responsiveness moved to `@vsintegration-reviewer`. Compiler concurrency stays here. +- 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 @@ -25,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/`, `Symbols/` | Read `compiler-service.md` via `@vsintegration-reviewer`. Do not apply compiler dimensions here. | -| `LanguageServer/` | IDE responsiveness: `@vsintegration-reviewer`. Concurrency stays here. | +| `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 `@vsintegration-reviewer`. Memory and JTF: `@vs-integration-perf`. | +| `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 index b87c114542d..7c46a7f44e9 100644 --- a/.github/skills/reviewing-vsintegration/SKILL.md +++ b/.github/skills/reviewing-vsintegration/SKILL.md @@ -1,11 +1,11 @@ --- 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 @vsintegration-reviewer. Does not review compiler type checking, IL, pickle, or FSharp.Core." +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 -CHECK rules live in two files. The five-wave workflow lives in `@vsintegration-reviewer`. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. +CHECK rules live in two files. The five-wave workflow lives in `@ide-integration`. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. `@expert-reviewer` stays the compiler reviewer. Do not send `vsintegration` or `src/Compiler/Service` findings there. @@ -19,7 +19,7 @@ CHECK rules live in two files. The five-wave workflow lives in `@vsintegration-r | `vsintegration/tests/**` | [vsintegration.md](vsintegration.md) | | `tests/FSharp.Compiler.Service.Tests/**` | [compiler-service.md](compiler-service.md) | -A change that crosses the two trees, owns cancellation, or could make `src/Compiler/Service` depend on `vsintegration` also uses the three rules under Both trees in `@vsintegration-reviewer`. +A change that crosses the two trees, owns cancellation, or could make `src/Compiler/Service` depend on `vsintegration` also uses the three rules under Both trees in `@ide-integration`. ## Folder routing From 947362d87a0be02e1cef781c870e0c9506efa075 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Wed, 14 Oct 2026 13:01:53 +0200 Subject: [PATCH 09/12] Tighten IDE review rules the panel agreed were fuzzy or obsolete. Two review rounds. Applied the shorter fixes that at least two reviewers agreed on. Dropped rules with no current code oracle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/ide-integration.md | 26 +++--- .../skills/reviewing-vsintegration/SKILL.md | 8 +- .../compiler-service.md | 86 +++++++++---------- .../reviewing-vsintegration/vsintegration.md | 61 +++++++------ 4 files changed, 87 insertions(+), 94 deletions(-) diff --git a/.github/agents/ide-integration.md b/.github/agents/ide-integration.md index cdf7dc27878..8aa1e99c00b 100644 --- a/.github/agents/ide-integration.md +++ b/.github/agents/ide-integration.md @@ -20,7 +20,7 @@ Use the `reviewing-vsintegration` skill to pick the file. Do not copy `@vs-integ ## Principle -- **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. +- A keystroke must not call `BackgroundCompiler.InvalidateConfiguration` when `FSharpProjectOptions.AreSameForChecking` is true. ## Both trees @@ -28,22 +28,22 @@ Read both files when a diff touches both trees. These three rules apply when a c (Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) -- 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. +- 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 -Host and service must share one identity for source text, project options, and dependency freshness. A hash code, checksum, private host field, or bare timestamp is not that identity, and a notification that does not name the text must not invalidate the parse. +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. -- Do not read private fields of a host version type or patch a hash the host owns; ask the host for a stable identity, or hash the public string form, and pass the source version explicitly instead of hiding a checksum in an overridden hash code. -- A file-change notification must carry a source-text version, and the builder must not reparse for a timestamp, a workspace-changed event, or a repeat with no text change; a live buffer has no identity unless that notification carries one, which cancellation alone cannot supply. +- 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. -- Dependency freshness is a host-supplied stamp on the project options the checker already takes and must cover referenced assemblies as well as sources; when the host already reports every change, the service must not also stat or poll those inputs. -- One project has one incremental builder, every host parse of that project goes through that cache, and a snapshot key must include every dependency that can change the result and must not retain an exceptional result, so the next caller can run again. +- 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. @@ -52,12 +52,12 @@ Host and service must share one identity for source text, project options, and d 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:** -- Keep the request token with the async computation and install it once at the async boundary or in the computation builder, not in thread-static storage, not inside an inlined cancel check, and not through a second helper or a manual install at each call site; a missing context is an explicit empty value, not a default token. -- When a token is installed on the thread that runs the computation, save and restore the previous token before that thread is reused, including across a thread switch, and copy the feature-entry token onto a worker first so a leftover token cannot cancel work that does not own it; a test must mix that computation with async, tasks, and thread switches. +- 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 it is a misuse that fails immediately as an internal error in product builds a host can enable, not as a debug assert, 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. -- One caller must not cancel or dispose a cached job or builder that another caller is still awaiting; cancel shared work only when every caller has cancelled, never from inside an awaited job, and do not start a non-cancellable parse per edit notification—the edit owner debounces the burst, or the builder drops a notification that is not a new version. +- `AsyncLazy.cancelIfUnawaited` must cancel a running shared CTS only when `requestCount` is zero and reset that computation for a later request. **Severity:** high. @@ -67,9 +67,9 @@ Host-neutral editor capabilities live on the public service API. The checker mus **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. -- A capability every editor should use, including host-neutral parsing such as splitting a long identifier and host-neutral diagnostic caching, must be public on the service; do not grant it only through a friend assembly, leave it only in the Visual Studio integration, or add a Visual Studio-only cache around results the service already stores. +- 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. -- Host policy stays outside the checker: a cache-validity rule that encodes one host's timestamps is a caller-supplied predicate, and an analysis API returns what the given check results support instead of embedding a policy such as skipping unused-symbol analysis when the file has errors. +- Keep the `checkResults.HasErrors` policy gate in `UnusedDeclarationsAnalyzer.AnalyzeSemanticsAsync`, not in `UnusedDeclarations.getUnusedDeclarations`. **Severity:** high. @@ -105,7 +105,7 @@ Save the briefing to a file. Every Wave 1 sub-agent receives it. Launch one sub-agent per selected dimension, in parallel batches of 6. Each evaluates exactly one dimension against the PR diff. -Report `$DimensionName — LGTM` when the dimension is genuinely clean. Do not explain away real issues to produce a clean result. +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. diff --git a/.github/skills/reviewing-vsintegration/SKILL.md b/.github/skills/reviewing-vsintegration/SKILL.md index 7c46a7f44e9..3f648343b4e 100644 --- a/.github/skills/reviewing-vsintegration/SKILL.md +++ b/.github/skills/reviewing-vsintegration/SKILL.md @@ -5,7 +5,7 @@ description: "Routes a Visual Studio integration or compiler-service review to t # Reviewing VS integration -CHECK rules live in two files. The five-wave workflow lives in `@ide-integration`. Do not copy `@vs-integration-perf`. Invoke it for JTF, retained memory, and the perf execution map. +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. @@ -19,13 +19,13 @@ CHECK rules live in two files. The five-wave workflow lives in `@ide-integration | `vsintegration/tests/**` | [vsintegration.md](vsintegration.md) | | `tests/FSharp.Compiler.Service.Tests/**` | [compiler-service.md](compiler-service.md) | -A change that crosses the two trees, owns cancellation, or could make `src/Compiler/Service` depend on `vsintegration` also uses the three rules under Both trees in `@ide-integration`. +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 scope, options, classification, feature failures | +For `vsintegration/src/FSharp.Editor/**`, select every section of `vsintegration.md` whose hotspots include the changed path. | `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 | @@ -39,4 +39,4 @@ A change that crosses the two trees, owns cancellation, or could make `src/Compi - `src/Compiler/Checking/`, `Optimize/`, `CodeGen/`, `TypedTree/`, `SyntaxTree/`, `Driver/` - `src/FSharp.Core/**` -- `src/FSharp.Compiler.LanguageServer/**` — not this corpus. The IDE responsiveness section in both files still names that hotspot because that section was moved intact. +- Route `src/FSharp.Compiler.LanguageServer/**` IDE responsiveness here; compiler concurrency stays with `@expert-reviewer`. diff --git a/.github/skills/reviewing-vsintegration/compiler-service.md b/.github/skills/reviewing-vsintegration/compiler-service.md index d766c91a09a..61adf229fd6 100644 --- a/.github/skills/reviewing-vsintegration/compiler-service.md +++ b/.github/skills/reviewing-vsintegration/compiler-service.md @@ -4,22 +4,22 @@ Paths: `src/Compiler/Service/**`, `src/Compiler/Symbols/**`. Tests: `tests/FShar 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. -- The service stays host-neutral. A capability an editor needs is exposed on the existing checker or symbol shape, a host test does not prove it, and a host on another runtime must not keep a private fork of a shim the service should own. -- A cache identity is a semantic stamp of one project closure: options, source text, earlier files, included scripts, and reference write times. A similar path, a mutable options object, or a live typed graph is not that stamp. +- A host must not fork `DelayedILModuleReader`; cross-project IL reads go through `FSharpReferencedProject.ILModuleReference`. +- 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. -- A public signature change is a break even when the private representation is unchanged. A break can be right when it helps the clients that use the type; it is not cleanup. A service syntax-tree reshape is not that break if existing client patterns still compile. +- 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 -Public API additions to FCS must be carefully controlled. Internal data structures must never leak. Breaking changes to FCS API surface are acceptable with major version bumps. +A public FCS signature change must update `FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl` or be marked experimental. 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. -8. **Stale type-checking results** — Avoid returning stale results; they cause timing-dependent IntelliSense glitches. Prefer fresh results with cancellation support. +- `CheckFileInProject` wins over `CheckFileInProjectAllowingStaleCachedResults` unless the caller uses the stale API. ### 7. FCS API Surface Control @@ -29,13 +29,12 @@ The FCS public API is a permanent commitment. Internal types must never leak. - 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/` +Hotspots: `src/Compiler/Service/`. --- @@ -46,11 +45,9 @@ The compiler service must respond to editor keystrokes without unnecessary recom **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**. +An endless recheck through `InvalidateConfiguration` is critical. **Hotspots:** `src/Compiler/Service/`, `src/FSharp.Compiler.LanguageServer/`, `vsintegration/` @@ -66,9 +63,9 @@ Incremental checking must invalidate stale results correctly. Stale data causes - 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**. +**Severity:** Stale results causing glitches → **critical**. Missed invalidation → **high**. -**Hotspots:** `src/Compiler/Service/`, `src/Compiler/Driver/` +**Hotspots:** `src/Compiler/Service/` --- @@ -76,18 +73,18 @@ 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.) -- 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. +- 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:** -- a cache key is a slim structural value with the same equality as the type, not the live typed object, and a snapshot key is a frozen semantic stamp, not a mutable options object. -- a clear or lookup targets the same project options only; another target framework that shares the path is a different project, and project identity strings compare ordinally unless the host already normalized them. -- a cached file result stays valid only while earlier files in that project are unchanged, a script snapshot changes when an included script changes, and a referenced in-memory module carries a freshness stamp checked like a write time. +- 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. -- a parse or check cache does not return a result that dropped the types of values, a parse uses only the source the caller passed, and a host that supplies source text hits the same parse cache as an on-disk path. +- 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. @@ -103,9 +100,9 @@ An edit reparses unchanged files, or a diagnostics request misses files the buil - 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. -- a parse that feeds navigation or breakpoint ranges does not wait behind the shared analysis reactor, and a project parse-and-check returns parse errors from the same parse the check used. -- a checking setting does not split the builder into two implementations; one path that can turn the setting on is the shape to keep. -- a builder-owned parse cache is one switch, does not retain an implementation that has a signature, and is reclaimable with that builder; do not add a size threshold until measurement shows the weak cache is not enough. A parse cache that is not tied to a project stays small. +- `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. +- Do not add a size threshold on the weak parse cache in `IncrementalBuild`. - 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. @@ -119,7 +116,7 @@ A wide lock, a queued caller-thread sequence, or an unsynchronized cache deadloc **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. -- build-graph request continuations run asynchronously, an eager parse is queued when the node is created, and concurrent incremental updates share one builder and one in-flight parse under the same 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. @@ -135,10 +132,10 @@ A type error or exception drops later resolution, or escapes the transparent com **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. -- a type error does not drop the failed declaration from the environment or skip later uses; the name stays bound so tooling can still resolve it. +- 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. -- a recovery point that does not use the exception catches every failure, so an unlisted exception does not abort the rest of the check. +- Use `RecoverableException` at recovery points that ignore the exception so `OperationCanceledException` is not swallowed. **Severity:** critical. @@ -150,16 +147,16 @@ A logger, phase, or severity leaks into the next caller, or a missing range is r **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. -- a computation that installs an ambient diagnostics logger restores the previous logger, and a diagnostic push with no logger initialized fails in the configurations that ship, not only in a debug assert. +- `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. -- a diagnostic range past the end of the document means the source and the check are out of sync; do not treat it as an ordinary range, and do not drop its correction unless the checker can no longer emit it. +- 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. -**Hotspots:** `src/Compiler/Service/{ServiceCompilerDiagnostics,ServiceErrorResolutionHints}.*`, `src/Compiler/Symbols/FSharpDiagnostic.*`, `src/Compiler/Service/{service,FSharpCheckerResults,FSharpParseFileResults,FSharpSource,ServiceConstants}.*`, `src/Compiler/Service/{BackgroundCompiler,IncrementalBuild,TransparentCompiler,ItemKey}.*`, `src/Compiler/Symbols/{XmlDocInheritance,XmlDocSigParser}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*` +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 @@ -167,12 +164,11 @@ A process-wide reader, shim, or console encoding leaks into the next solution or **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. -- an assembly reader is told whether the read is for a project or an interactive session, a host-supplied reader accepts a read lock on any thread the service uses, and it stays logically immutable until the write stamp changes. +- 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. -- a host on a different runtime than the compiler still resolves type providers through a shim the service owns, in a shape that allows an out-of-process host, and the service works against the core library as a referenced assembly without an in-solution workaround. **Severity:** high. @@ -184,12 +180,12 @@ A public symbol, diagnostic, or tooltip signature changes and is treated as clea **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 service type break is acceptable when it makes that type easier for the clients that use it; do not treat the break as free cleanup, and do not reject it only because those clients must update. -- moving or renaming a public layout or tooltip member is a source and binary break; an annotation forced by an internal resource overload is not. A new public type ships in the namespace clients should keep, and an experimental member is marked experimental. +- 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. -- a checking mode that does not differ per project belongs on the checker, not on project options. A service syntax-tree reshape may ship if existing client patterns still compile; adding trivia must not force those clients onto named patterns. New member emission is gated by language version, and a per-feature kill switch is added only for a concrete break and warns when it disables specified behavior. +- For public `SynExpr` or `SynPat` shape changes, verify source-pattern compatibility and `SurfaceAreaTest`’s exported constructor signatures separately. **Severity:** high. @@ -202,7 +198,7 @@ Tooling sees a compiler-generated symbol as user-written, misses the property or **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. -- a property name still has a property symbol when get and set are separate members, a method parameter exposes its owning method, and a named argument that is a union-case field surfaces as a field so the caller can reach the case. +- 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. @@ -217,17 +213,17 @@ Tooling sees a compiler-generated symbol as user-written, misses the property or 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:** -- a declaration range includes the starting keyword, attributes, visibility, and modifiers for every construct, including inherit, interface, constructors, and fields, not only the form currently being fixed. +- 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. -- an identifier keeps the range that was parsed; a parenthesized operator is a parenthesis node around the identifier, not an identifier whose range swallows the parentheses, and that identifier range is not copied onto parent expressions as a diagnostic range. +- 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. -- a position outside a declaration is not visited as that declaration, and a walk still finds a position in unfinished code when the declaration range stops at the last token. -- breakpoint ranges the service reports match sequence points the paired compiler emits, a loop sequence point covers the pattern and not the whole comprehension, and a range change is gated on language version when a newer service can run with an older compiler. +- 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. -**Hotspots:** `src/Compiler/Service/ServiceNavigation.*`, `src/Compiler/Service/ServiceStructure.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*` +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 @@ -244,7 +240,7 @@ Completion hides a legal item, returns the wrong scope, or follows a source scan **Severity:** high. -**Hotspots:** `src/Compiler/Service/{ServiceDeclarationLists,ServiceParamInfoLocations,ServiceInterfaceStubGenerator}.*`, `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAssemblyContent.*`, `tests/FSharp.Compiler.Service.Tests/Completion/**`, `tests/FSharp.Compiler.Service.Tests/ParameterInfo/**` +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 @@ -255,13 +251,13 @@ A walk or classification fixes one syntactic form and leaves the sibling wrong, - 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. -- an IDE symbol walk drops locals, parameters, and type parameters before it parses text or reads document lines, and does not also drop bindings until a use misclassified as a binding is handled. -- semantic classification keeps one winner per range, an unresolved name is colored differently from an entity whose kind is unknown, and a parse-helper change meant to fix coloring is shown to be on the coloring path with a test of the colors that were wrong. +- 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. -**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceAnalysis.*`, `src/Compiler/Service/SemanticClassification*.*`, `src/Compiler/Symbols/{Symbols,SymbolHelpers,SymbolPatterns,Exprs}.*`, `src/Compiler/Service/ServiceAssemblyContent.*` +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 @@ -271,12 +267,12 @@ The tree tooling walks has been desugared, has dropped a legal form, or records - 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. -- a separator the service tree exposes is absent when none was written, and only written separators such as a semicolon or a comma are captured, not a keyword that merely introduces a clause. -- a single-line token stores its start position and derives the range, and a large syntax baseline is a named test per case that can dump the tokens for that case. +- `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. -**Hotspots:** `src/Compiler/Service/{ServiceUntypedParse,QuickParse,ServiceParseTreeWalk,ServiceParsedInputOps,ServiceXmlDocParser,SynExpr,SynPat}.*`, `src/Compiler/Service/ServiceLexing.*` +Use `src/Compiler/Service/{FSharpParseFileResults,ServiceParsedInputOps,QuickParse,ServiceParseTreeWalk,ServiceXmlDocParser,SynExpr,SynPat,ServiceLexing}.*` as parsed-form hotspots. ### Assert the observable service result @@ -306,7 +302,7 @@ One test cancels, redirects, or mutates process-wide state, and the failure is r - 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. -- a test must not treat a forced collection timeout as the product behavior, a failure that appears only when cases run together is an isolation bug, and a test must not assume an inlined internal computation expression is reachable through friend visibility from another assembly. +- Distinguish test-owned state from the service state protected by `IncrementalBuilder.updateCurrentState` before classifying a together-only failure as isolation. **Severity:** high. diff --git a/.github/skills/reviewing-vsintegration/vsintegration.md b/.github/skills/reviewing-vsintegration/vsintegration.md index 3676887f645..bf8e6896f7d 100644 --- a/.github/skills/reviewing-vsintegration/vsintegration.md +++ b/.github/skills/reviewing-vsintegration/vsintegration.md @@ -2,7 +2,7 @@ Paths: `vsintegration/**`. Tests: `vsintegration/tests/**`. -Editor, project system, legacy language service, FSI, debug adapter, MEF, and the UI thread. Do not push Visual Studio, MEF, JTF, or Roslyn types into `src/Compiler/Service` or `src/Compiler/Symbols`. +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. @@ -20,11 +20,9 @@ The compiler service must respond to editor keystrokes without unnecessary recom **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**. +**Severity:** Endless recheck loop → **critical**. UI thread block → **critical**. **Hotspots:** `src/Compiler/Service/`, `src/FSharp.Compiler.LanguageServer/`, `vsintegration/` @@ -32,7 +30,7 @@ The compiler service must respond to editor keystrokes without unnecessary recom (Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) -- 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. +- Separate cancellation from logged faults at IDE entry points, as `FSharpNavigation.TryGoToDefinition` and `RoslynHelpers.StartAsyncAsTask` do. Cancellation hotspot: `vsintegration/`. @@ -42,9 +40,9 @@ A host call that requires the UI thread runs off it and deadlocks, or work that **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. -- A newer host version is not evidence that project-system initialization left the UI thread. Keep that work on the UI thread until the host contract says otherwise. -- Package initialization and workspace updates that do not touch UI services must not take the UI thread, and a workspace project context whose host factory is built for the background must not stay on the UI thread. -- Resolve a UI-thread service when the editor part is created, on the main thread, through one main-thread or async lookup. Do not fetch it from a background tooltip or command callback, which deadlocks even when the result is unused. +- 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. @@ -58,10 +56,10 @@ The editor does not compose, another language loads F# visuals, or the host runs **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. -- A MEF view factory must not be visible to other languages. Keep custom visuals on an internal type so another language's tooltip does not load the F# editor. +- 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. -- Editor options must be a composed part that features can import. Expose as little of the editor to MEF as possible: a small factory should choose the host or test store instead of exporting the whole options implementation, and do not keep a static singleton the language service initializes by hand. -- Assign a process-global resource assembly once, from a composition singleton. Assigning it again for each view throws. The options page must open with no solution loaded. +- 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` and create `AbstractOptionPage` views without accessing a solution or assigning `Application.ResourceAssembly`. - 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. @@ -74,10 +72,10 @@ The editor does not compose, another language loads F# visuals, or the host runs 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 the host signals that the solution UI is ready. Do not replace that signal with a custom wait or a fixed delay. +- 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. -- On solution close, run the same cleanup as document close and release documents that are still open, because the workspace does not raise a close for them. Fix a project-close race by ordering disconnect against in-flight work, not by delaying the call. +- 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. @@ -107,24 +105,23 @@ A cache key cannot be found again, so entries accumulate, or an idle cache stays Quick info is unreadable, wraps through a fake divider, shows signature-help text, or is built from a Windows-only element. **CHECK:** -- Quick info and inline hints must show the compiler's existing tooltip text, must not show a parameter value that belongs to signature help, and must not ship a debug tooltip that shows an internal layout tag. -- A section separator must have a fixed length. Do not size it by repeating glyphs or draw it as ASCII art, and do not treat a hand-drawn divider as wrapping. Break long documentation explicitly. -- Quick-info maximum width must scale with the editor font. Documentation may match the signature width only up to a fixed cap, alignment must follow the document reading direction, and documentation must not be fully justified. -- A quick-info section that other languages expose as a setting must be configurable, not always shown. A secondary tooltip must follow the active theme, and a path it displays must be relative to the solution. +- 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. -- Quick-info content must stay a view model another host can translate. Do not build that model out of a Windows-only element. Record and value text shown in tools should keep a single space between braces and fields. -- Do not add a width control that fights the host's own tooltip wrapping. If a setting does not always apply, mark it experimental and explain the effect. Quick info must keep line breaks between sections, and long documentation must still wrap after a host API change. +- `QuickInfoViewProvider.provideContent` must build Roslyn `ClassifiedTextElement` content, not a custom Windows-only element. +- Pass `QuickInfoOptions.DescriptionWidth` to `TryGetToolTip` and preserve section `LineBreak` tokens and `ContainerElementStyle.Wrapped` in `QuickInfoViewProvider`. **Severity:** medium. -**Hotspots:** `vsintegration/src/FSharp.Editor/QuickInfo/**`, `vsintegration/src/FSharp.Editor/Hints/**`, `vsintegration/src/FSharp.Editor/Options/**`, `vsintegration/src/FSharp.UIResources/**` +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 request must return a navigation item and must not open a window itself. Hover must not jump to decompiled metadata because the pointer is over an external symbol. +- A definition provider must return a navigable item; only `FSharpNavigation.NavigateToItem` may 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. @@ -144,9 +141,9 @@ Completion offers items that cannot be declared there, removes items that are st - 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. -- Completion must not invent a method body. Override completion must accept the member name first and the body in a later step, and must not insert the whole override, including a generated body, on one accept. +- `CompletionProvider.GetChangeAsync` must not insert a generated method body. - 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. -- A name suggestion should reuse the completion items for that context when that lookup is cheap. Names the editor shows must stay separate from the representation that guides compiled form. +- Reuse the current `DeclarationListItem` values in `FSharpCompletionProvider`, displaying `NameInList` and committing `NameInCode` through `GetChangeAsync`. **Severity:** high. @@ -159,9 +156,9 @@ A code fix matches a culture-specific message, a host analysis limit is never ap **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. -- A diagnostics exporter must not stay enabled on the analysis path. Left running, it adds overhead that shows up as a compile-time regression. +- 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. -- A host limit or setting must be applied on the override or callback the host actually invokes. A limit that lives only in an initialization method the host no longer calls is never applied. +- 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. @@ -174,8 +171,8 @@ A code fix matches a culture-specific message, a host analysis limit is never ap Editor settings are persisted as one blob, so a new setting comes back as the zero value, or a setting cannot be found. **CHECK:** -- Declare, store, and retrieve each editor setting on its own so a newly added setting keeps its default. Do not persist the whole settings type as one reflected blob. -- An options type the settings page binds must keep the marker that makes its properties settable, and options-page markup must bind to public properties. +- 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. @@ -191,9 +188,9 @@ Editor settings are persisted as one blob, so a new setting comes back as the ze A classification ignores the language role, or a baked-in foreground stays unreadable after the theme changes. **CHECK:** -- Classify a name by its language role. A function must not be colored as a keyword, and a method and a function value must not share a classification when their calling rules differ. +- 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. -- Do not assign a fixed foreground on a classification format. Leave it unset so the host theme supplies a readable color and the user can still customize it. +- 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. @@ -211,7 +208,7 @@ An exception from document check, Quick Info, analysis, or the options page reac - 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 use a process-global resource assembly to locate one extension's dictionaries. Another extension assigning that same property causes a runtime failure. Resolve resources from this extension's install location. +- Do not set a process-global resource assembly; load dictionaries from `FSharp.UIResources` through the `ProvideCodeBase` attribute. **Severity:** critical. @@ -225,7 +222,7 @@ An editor change is tested only through the service API, or only for a flat modu - 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 one MEF composition instead of building a new export provider for every test. Tests must not share a process-wide options part across cases. If desktop tests share one host, a throw from a background thread must still name the case. +- 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. @@ -240,7 +237,7 @@ A keystroke, search, or tooltip starts work that keeps running after the user ha - 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. -- Tooltip navigation must use the host's built-in link and must be cancellable. If the user cancels the slower implementation lookup, fall back to the instant range. Render each tooltip element as icon, then description, then documentation, because the host link parser reads that order. +- 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. From 37f69a22bc412d423c917dd5b708abef4ae334ef Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 16 Oct 2026 10:52:53 +0200 Subject: [PATCH 10/12] Fix review routing that does not match the current tree. Restore the editor routing row, keep LanguageServer out of this corpus, and drop the removed ResourceAssembly symbol. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/skills/reviewing-vsintegration/SKILL.md | 6 +++--- .github/skills/reviewing-vsintegration/vsintegration.md | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/skills/reviewing-vsintegration/SKILL.md b/.github/skills/reviewing-vsintegration/SKILL.md index 3f648343b4e..01297aeb56c 100644 --- a/.github/skills/reviewing-vsintegration/SKILL.md +++ b/.github/skills/reviewing-vsintegration/SKILL.md @@ -25,7 +25,7 @@ Use the Both trees rules only when the diff touches both trees or adds a referen | Path | File | Select | |---|---|---| -For `vsintegration/src/FSharp.Editor/**`, select every section of `vsintegration.md` whose hotspots include the changed path. +| `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 | @@ -37,6 +37,6 @@ For `vsintegration/src/FSharp.Editor/**`, select every section of `vsintegration ## Do not route here -- `src/Compiler/Checking/`, `Optimize/`, `CodeGen/`, `TypedTree/`, `SyntaxTree/`, `Driver/` +- `src/Compiler/Checking/`, `src/Compiler/Optimize/`, `src/Compiler/CodeGen/`, `src/Compiler/TypedTree/`, `src/Compiler/SyntaxTree/`, `src/Compiler/Driver/` - `src/FSharp.Core/**` -- Route `src/FSharp.Compiler.LanguageServer/**` IDE responsiveness here; compiler concurrency stays with `@expert-reviewer`. +- `src/FSharp.Compiler.LanguageServer/**` is not this corpus. Compiler concurrency stays with `@expert-reviewer`. diff --git a/.github/skills/reviewing-vsintegration/vsintegration.md b/.github/skills/reviewing-vsintegration/vsintegration.md index bf8e6896f7d..898769859ed 100644 --- a/.github/skills/reviewing-vsintegration/vsintegration.md +++ b/.github/skills/reviewing-vsintegration/vsintegration.md @@ -59,7 +59,7 @@ The editor does not compose, another language loads F# visuals, or the host runs - 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` and create `AbstractOptionPage` views without accessing a solution or assigning `Application.ResourceAssembly`. +- 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. From b955d2824f3c362afe10627dba0d2fc2f6cf7419 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 16 Oct 2026 11:32:48 +0200 Subject: [PATCH 11/12] Correct review claims that contradict the current compiler. Name the members and host contracts that exist now: PEReference, ProvideBindingPath, GoToDefinition, and the surface-area baseline. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/ide-integration.md | 2 +- .../skills/reviewing-vsintegration/compiler-service.md | 5 ++--- .github/skills/reviewing-vsintegration/vsintegration.md | 8 ++++---- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/.github/agents/ide-integration.md b/.github/agents/ide-integration.md index 8aa1e99c00b..a4204deae23 100644 --- a/.github/agents/ide-integration.md +++ b/.github/agents/ide-integration.md @@ -54,7 +54,7 @@ A request token belongs to the caller that started the work. It must travel with **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 it is a misuse that fails immediately as an internal error in product builds a host can enable, not as a debug assert, and it must not leak an uncaught cancellation or observe another request's token. +- 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. diff --git a/.github/skills/reviewing-vsintegration/compiler-service.md b/.github/skills/reviewing-vsintegration/compiler-service.md index 61adf229fd6..7239c35bf8e 100644 --- a/.github/skills/reviewing-vsintegration/compiler-service.md +++ b/.github/skills/reviewing-vsintegration/compiler-service.md @@ -4,7 +4,7 @@ Paths: `src/Compiler/Service/**`, `src/Compiler/Symbols/**`. Tests: `tests/FShar 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. -- A host must not fork `DelayedILModuleReader`; cross-project IL reads go through `FSharpReferencedProject.ILModuleReference`. +- 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. @@ -15,7 +15,7 @@ Host-neutral API for IDEs: checker, incremental builder, snapshots, workspace, n ### FCS API surface -A public FCS signature change must update `FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl` or be marked experimental. +A public FCS signature change must update `FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl`, including when the API is marked experimental. 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. @@ -102,7 +102,6 @@ An edit reparses unchanged files, or a diagnostics request misses files the buil - 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. -- Do not add a size threshold on the weak parse cache in `IncrementalBuild`. - 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. diff --git a/.github/skills/reviewing-vsintegration/vsintegration.md b/.github/skills/reviewing-vsintegration/vsintegration.md index 898769859ed..b57fad8c99c 100644 --- a/.github/skills/reviewing-vsintegration/vsintegration.md +++ b/.github/skills/reviewing-vsintegration/vsintegration.md @@ -109,7 +109,7 @@ Quick info is unreadable, wraps through a fake divider, shows signature-help tex - 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` must build Roslyn `ClassifiedTextElement` content, not a custom Windows-only element. +- `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. @@ -121,7 +121,7 @@ Use `vsintegration/src/FSharp.Editor/{QuickInfo,DocComments,Hints,Options}/**` a Navigation opens a window itself, jumps to metadata or the wrong linked copy, or offers a link that goes nowhere. **CHECK:** -- A definition provider must return a navigable item; only `FSharpNavigation.NavigateToItem` may open the document. +- 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. @@ -141,7 +141,7 @@ Completion offers items that cannot be declared there, removes items that are st - 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. -- `CompletionProvider.GetChangeAsync` must not insert a generated method body. +- `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`. @@ -208,7 +208,7 @@ An exception from document check, Quick Info, analysis, or the options page reac - 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; load dictionaries from `FSharp.UIResources` through the `ProvideCodeBase` attribute. +- Do not set a process-global resource assembly. Probe `FSharp.UIResources` XAML dictionaries with `ProvideBindingPath`. `ProvideCodeBase` does not load them. **Severity:** critical. From 987ac677e5ce8b2d6c9f9bd7de0c4fb610af4278 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 16 Oct 2026 11:44:23 +0200 Subject: [PATCH 12/12] Drop transplanted outline numbers from the IDE review files. A claim check does not see a copied heading number. The extraction gate now fails that leftover before a panel can call the files clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fc90eb51-8c77-459a-a9eb-63c55bff56ff --- .github/agents/expert-reviewer.md | 81 ++++++++----------- .github/agents/extraction-pipeline.md | 16 ++++ .../skills/reviewing-compiler-prs/SKILL.md | 2 +- .../compiler-service.md | 56 ++----------- .../reviewing-vsintegration/vsintegration.md | 22 +---- 5 files changed, 57 insertions(+), 120 deletions(-) diff --git a/.github/agents/expert-reviewer.md b/.github/agents/expert-reviewer.md index ab030527f81..0ebe22b8ef0 100644 --- a/.github/agents/expert-reviewer.md +++ b/.github/agents/expert-reviewer.md @@ -1,11 +1,11 @@ --- 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), `@ide-integration` (IDE host and compiler service). @@ -17,7 +17,6 @@ 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. -- Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - **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 FSharp.Core must be carefully controlled. FSharp.Core must remain stable. The FCS half of this rule moved to @ide-integration. @@ -28,18 +27,16 @@ 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. The language-service exception moved to @ide-integration. -3. **Internal type leakage** — Moved to @ide-integration. -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** — Moved to @ide-integration. -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. +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,11 +146,7 @@ Optimizer changes must preserve program semantics. Inlining and tail-call change --- -### 7. FCS API Surface Control - -Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - -### 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. @@ -168,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. @@ -184,11 +177,7 @@ Structs have value semantics that differ fundamentally from reference types. Inc --- -### 10. IDE Responsiveness - -Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - -### 11. Overload Resolution Correctness +### 9. Overload Resolution Correctness Overload resolution is one of the most complex and specification-sensitive areas of the compiler. @@ -205,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. @@ -225,7 +214,7 @@ 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. @@ -245,11 +234,7 @@ FCS request cancellation and the `vsintegration/` hotspot moved to @ide-integrat --- -### 14. Incremental Checking Correctness - -Moved to @ide-integration. Invoke it when the change touches vsintegration/, src/Compiler/Service/, or src/Compiler/Symbols/. - -### 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. @@ -265,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. @@ -284,7 +269,7 @@ The language-service boundary bullet moved to @ide-integration. --- -### 17. Diagnostic Quality +### 14. Diagnostic Quality Error and warning messages are the compiler's user interface. They must be precise, consistent, and actionable. @@ -299,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. @@ -314,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. @@ -428,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. IDE impact and incremental checking moved to @ide-integration (Dimensions 10 and 14). -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. IDE impact moved to @ide-integration (Dimension 10). -3. Verify overload resolution correctness if constraint solving changes (Dimension 11). -4. Incremental checking moved to @ide-integration (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/skills/reviewing-compiler-prs/SKILL.md b/.github/skills/reviewing-compiler-prs/SKILL.md index aa0142d1ea5..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 diff --git a/.github/skills/reviewing-vsintegration/compiler-service.md b/.github/skills/reviewing-vsintegration/compiler-service.md index 7239c35bf8e..6d42ad7791e 100644 --- a/.github/skills/reviewing-vsintegration/compiler-service.md +++ b/.github/skills/reviewing-vsintegration/compiler-service.md @@ -17,57 +17,13 @@ Host-neutral API for IDEs: checker, incremental builder, snapshots, workspace, n A public FCS signature change must update `FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl`, including when the API is marked experimental. -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. - +- 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. - -### 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. -- 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/`. - ---- - -### 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. -- Caching must prevent duplicate work per project. - -An endless recheck through `InvalidateConfiguration` is critical. - -**Hotspots:** `src/Compiler/Service/`, `src/FSharp.Compiler.LanguageServer/`, `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**. - -**Hotspots:** `src/Compiler/Service/` - ---- +- 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. diff --git a/.github/skills/reviewing-vsintegration/vsintegration.md b/.github/skills/reviewing-vsintegration/vsintegration.md index b57fad8c99c..c124606927a 100644 --- a/.github/skills/reviewing-vsintegration/vsintegration.md +++ b/.github/skills/reviewing-vsintegration/vsintegration.md @@ -12,28 +12,10 @@ Review `FSharpLanguageDebugInfoService` and `FSharpBreakpointResolutionService` - 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. - -### 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. -- Caching must prevent duplicate work per project. - -**Severity:** Endless recheck loop → **critical**. UI thread block → **critical**. - -**Hotspots:** `src/Compiler/Service/`, `src/FSharp.Compiler.LanguageServer/`, `vsintegration/` - ---- - -(Exception: top-level language service entry points should catch all to prevent IDE crashes — but log, don't silence.) - +- 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. -Cancellation hotspot: `vsintegration/`. - ### 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.