Skip to content

refactor(lsp): decompose the 687-line LspTask::run God Function - #2452

Draft
sinelaw wants to merge 4 commits into
masterfrom
claude/epic-turing-2i7cgg
Draft

refactor(lsp): decompose the 687-line LspTask::run God Function#2452
sinelaw wants to merge 4 commits into
masterfrom
claude/epic-turing-2i7cgg

Conversation

@sinelaw

@sinelaw sinelaw commented Jun 22, 2026

Copy link
Copy Markdown
Owner

What & why

LspTask::run in crates/fresh-editor/src/services/lsp/async_handler.rs was the single largest function in the codebase at 687 lines, inside the largest stable source file (~6.3k lines). Its body was one enormous match cmd { … } whose ~20 request-handling arms were near-identical copy-paste:

LspCommand::Hover { request_id, uri, line, character } => {
    if initialized {
        tracing::info!(...);
        spawn_request!(state, pending, |s, p| s.handle_hover(request_id, uri, line, character, &p).await);
    } else {
        let _ = state.async_tx.send(AsyncMessage::LspHover { request_id, /* empty */ });
    }
}

This is both a God Function and a textbook Copy-Paste Specialist — the repetition buried the parts of run that actually carry logic: the task wiring, the stdout-reader handshake, and the init/notification queue-and-replay loop.

The change

  • Moved the ~20 request-dispatch arms (Completion, GotoDefinition, Hover, References, SignatureHelp, CodeActions, InlayHints, all SemanticTokens*, formatting, PluginRequest, …) into a new, documented method LspState::dispatch_request_command.
  • run's match now keeps only the commands that genuinely interact with loop-local state — Initialize (drives the draining/replay), the didOpen/didChange/didClose/didSave/workspace-folder notifications (which queue until initialized), CancelRequest, and Shutdown — plus a single catch-all that delegates everything else:
cmd => state.dispatch_request_command(cmd, &pending),

run shrinks from 687 → ~280 lines and reads end-to-end.

Behaviour

No functional change. Each request still gets its own tokio::spawn (the issue #1679 isolation guarantee is preserved verbatim), and the uninitialized fast-path still answers immediately with the same empty/error AsyncMessage. The only thing that moved is where the code lives. No new traits or generics; the one pre-existing local spawn_request! macro simply moved with the arms it serves.

Conflict avoidance

Selected specifically because it is not touched by any of the 55 open PRs (verified by diffing every open PR branch against their merge-base). async_handler.rs does not appear in any open PR's changeset.

Tooling

  • cargo build -p fresh-editor — clean.
  • cargo fmt — applied.
  • cargo clippy --fix --allow-dirty — applied; the file is clippy-clean (remaining crate warnings are pre-existing and in unrelated files). Per request, the test suite was left to CI.

🤖 Generated with Claude Code


Generated by Claude Code

@sinelaw
sinelaw force-pushed the claude/epic-turing-2i7cgg branch from 71ca301 to a5fd6cd Compare June 23, 2026 04:52
claude added 3 commits June 23, 2026 14:06
`LspTask::run` was the single largest function in the codebase (687
lines). Its body was an enormous `match cmd { … }` whose ~20 request
arms (Completion, GotoDefinition, Hover, SemanticTokens, …) were all
near-identical copy-paste:

    if initialized {
        spawn_request!(state, pending, |s, p| s.handle_xxx(…).await);
    } else {
        let _ = state.async_tx.send(AsyncMessage::LspXxx { … empty … });
    }

That repetition buried the parts of `run` that actually matter — the
task wiring, the stdout-reader handshake, and the init/notification
queueing-and-replay loop.

Move the request-dispatch arms into a new, documented
`LspState::dispatch_request_command` method and delegate to it from a
single catch-all arm. Behaviour is unchanged: same per-request
`tokio::spawn`, same empty/error fast-path when uninitialized. `run`
now keeps only the commands that touch loop-local state (Initialize,
the didOpen/didChange/… queueing notifications, CancelRequest,
Shutdown) and shrinks from 687 to ~280 lines, making the dispatch loop
readable end to end.

No functional change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBpAjG4dxpX3zuSAqx2zdF
The extracted `dispatch_request_command` still repeated the same
skeleton ~20 times: `if initialized { clone; tokio::spawn(handler) }
else { send empty reply }`, plus a per-arm tracing line. Only two
things ever varied — the handler call and the not-initialized fallback
message.

Capture that skeleton once in a small, function-local `dispatch!`
macro (two forms: with and without an `else` fallback) so each arm now
names just the handler invocation and, where applicable, the fallback
`AsyncMessage`. The match reads as a table of intent. `use
LspCommand::*` / `use AsyncMessage::*` drop the noise prefixes from the
arms. The method shrinks 410 -> ~265 lines.

Behaviour is unchanged: same per-request `tokio::spawn` isolation
(#1679), same empty/error fast-path when uninitialized. The only
observable difference is logging — the assorted per-arm `info!/trace!`
lines are replaced by one uniform `trace!("LSP dispatch: <handler>")`,
trimming per-keystroke completion/hover spam from the info log.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBpAjG4dxpX3zuSAqx2zdF
Replaces the `dispatch!` macro with plain code by removing the
duplication it was papering over, rather than hiding it.

Analysis — what every request arm did:
  1. gate on `initialized`                         (identical everywhere)
  2. when ready: clone shared state + spawn handler (identical machinery;
     only the handler call genuinely varies — that's the real routing)
  3. when not ready: send an empty/error reply      (per-command)

Step 3 was not actually unique logic: each `handle_*` already sends the
very same empty/error reply on its own failure path (e.g.
`handle_completion` emits `LspCompletion { items: vec![] }` when its
request errs). The dispatch fallback was a second copy of that
knowledge, which is why the arms looked so repetitive.

So collapse it at the source:
  * `send_request_with_timeout` now fails fast — any request other than
    the `initialize` handshake issued before the server is initialized
    returns `Err` immediately (no frame sent, no timeout wait). Verified
    safe: `initialized` is set true before the queued-command replay, and
    `initialize` is the only request sent pre-init.
  * With that, an uninitialized request simply runs its handler, hits the
    handler's existing error path, and replies there. `dispatch_request_
    command` no longer needs the gate or any fallback: it's a flat table
    that spawns each handler. The shared clone+spawn lives in one small
    generic helper, `spawn_request` — no macro.

Behaviour is unchanged for the commands that had a fallback (same
immediate empty reply, now sourced from the handler). The handful that
had no fallback (formatting, completion/code-action resolve, prepare-
rename) now also get their handler's error reply when uninitialized
instead of being silently dropped — a strict improvement (the editor's
request future resolves instead of lingering).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBpAjG4dxpX3zuSAqx2zdF
@sinelaw
sinelaw force-pushed the claude/epic-turing-2i7cgg branch from a13f609 to b2931e8 Compare June 23, 2026 14:06
Re-inline the request arms into `run`'s `match cmd` and delete the
separate `dispatch_request_command` method. Both the run loop and the
helper previously ended in a catch-all (`cmd => dispatch(..)` and
`other => error!(..)`), so neither match was exhaustive over
`LspCommand` — a newly added command variant would compile and silently
fall through to a runtime log instead of being caught.

Now `run` has one match over all 29 variants with no catch-all, so
adding a variant is a hard compile error (E0004 non-exhaustive)
pointing the implementer straight at the dispatch site. Verified by
temporarily adding a probe variant.

The `spawn_request` helper stays (it's a plain function, not a second
match, so it doesn't weaken exhaustiveness): each request arm is a
one-line `state.spawn_request(&pending, |s, p| s.handle_*(.., &p))`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QBpAjG4dxpX3zuSAqx2zdF
@sinelaw
sinelaw marked this pull request as draft July 22, 2026 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants