[ajitpratap0]

Description

Stacked on #825: the base branch is feat/syntax-highlighting. After #825 merges, this will be rebased onto main.

#825 used the LSP dialect only for semantic tokens. This PR applies it everywhere:

• Diagnostics use dialect-aware parsing with recovery.
• Formatting and document symbols use dialect-aware parsing.
• Completion adds the dialect's own keywords, and hover falls back to them.
• workspace/didChangeConfiguration with {"gosqlx":{"dialect":"mysql"}} switches the dialect at runtime. Invalid values fall back to generic, as at initialize. After a switch:
  • open documents are re-validated;
  • workspace/semanticTokens/refresh is sent, but only if the client advertised refreshSupport.
• An unset or generic dialect keeps exactly the previous behavior: the same gosqlx.Parse, ParseWithRecovery and Format pipeline.
• Concurrency:
  • The dialect is read once per validation, and publishMu serialises the staleness check with the publish.
  • Diagnostics computed under a superseded dialect are dropped.
  • Pending debounce timers are cancelled on a switch, so stale results can't land after the new ones.

Type of Change

• New feature (non-breaking)

Testing

• Per-feature dialect tests, unset-equals-previous-behavior tests, didChangeConfiguration switching and invalid values, a pending-change race test, and -race.
• task check and task test:cbinding
• go-reviewer pass. Its one finding (stale diagnostics during a switch) is fixed, and the related debounce-timer race is covered by TestDialect_DidChangeConfiguration_PendingChange.
• BenchmarkHandler_ValidateDocument: allocations unchanged; time differences within noise.

Breaking Changes

None.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KTj1PEbxzVCETkeYrZKcrE

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 21, 2026 9:58am UTC

[github-actions]

Claude finished @ajitpratap0's task in 30s —— View job

---

Review of PR #828: dialect-aware LSP features

I read the diff for pkg/lsp/* against origin/feat/syntax-highlighting. I did not run the tests or -race here, so this is a read-through only. The change looks solid overall, with a few smaller points below.

What works well

• The default-pipeline escape hatch (usesDefaultPipeline) keeps generic and unset dialects on the old code path. The comment explaining why "generic" passed to ParseWithDialect isn't equivalent (the ILIKE case) is useful.
• dialectFromSettings separates "no setting" (keep the current dialect) from "invalid setting" (fall back to generic), and reuses dialectFromInitOptions so initialize and runtime agree. The null-settings case from vscode-languageclient is handled.
• Pool usage is correct. GetTokenizer/GetParser are paired with deferred Put, and the comments note that Put resets the dialect.
• handleMessage now recognises client responses to server requests instead of answering them with InvalidRequest. This is needed once the server sends requests, and the semanticTokens/refresh request is correctly gated on refreshSupport.
• The staleness check under publishMu closes the main race, where a validation started under the old dialect publishes after the re-validation.

Points to consider

1. A document opened during a switch may get no diagnostics. In revalidateOpenDocuments, snapshots() is taken once. Suppose didOpen stores a document and validates it under the old dialect, and the dialect then switches. If the switch happens after the old-dialect parse but before the snapshot, the document is in the snapshot and is fine. If the validation's publish check drops its result, but the document was added after snapshots() ran, nobody re-validates it until the next edit. The window is tiny, but you could close it by re-validating from validateDocument itself when it drops a result, or by looping until the dialect is stable.

2. publishMu is held across SendNotification (handler.go, validateDocument). This serialises all publishes for all documents, and a slow client write blocks every validation. The lock only needs to cover the dialect check plus the send. If you want to keep the ordering guarantee, that's fine. A comment saying the hold is deliberate would help, and so would confirming that sendMessage can't block on a path that re-enters the handler.

3. revalidateOpenDocuments runs synchronously in the notification handler. With many large open documents, a dialect switch blocks the notification loop for the sum of all parse times. Check whether HandleNotification is already dispatched on a goroutine. If it isn't, consider running the re-validation in one.

4. Small things

   • isDialectKeyword does a linear scan per hover. dialectKeywordCompletions is built once, so a map[SQLDialect]map[string]struct{} built alongside it would make this O(1). It's negligible at current sizes.
   • formatForDialect silently ignores opts.AddSemicolon. The comment documents this, but an unexported options struct, or an assertion in the test that the LSP never sets it, would protect against a later regression.
   • The didChangeConfiguration handler only reads pushed settings. Clients that rely on pull-style workspace/configuration won't be covered. That seems fine to leave out of scope, but it may be worth a note in LSP_GUIDE.md.
   • sendRequest uses an int64 ID. That's valid JSON-RPC, and I saw no collision with client IDs because the two ID spaces are separate.

Tests
The description covers per-feature dialect tests, unset-equals-previous-behavior, invalid values, and the pending-change race. Two cases I'd add:

• A client response ({"id":1,"result":null}) sent to the server isn't answered with an error. I couldn't tell from the diff whether dialect_test.go covers this.
• refreshSupport absent means no semanticTokens/refresh request is sent.

Security
No new concerns. The settings parsing uses safe type assertions, and an unknown dialect falls back to generic.

None of the points above blocks the merge, though I'd take a look at items 1 and 3 first.
· branch feat/lsp-dialect-aware

[github-actions]

Claude finished @ajitpratap0's task in 30s —— View job

---

Review of PR #828: dialect-aware LSP features

I read the diff and pkg/lsp/dialect.go in full. I did not run the tests or -race, so this is a read-through only. I read the surrounding code for handleCompletion's prefix handling and for how HandleNotification is dispatched only partly.

Overall: The design is sound, and none of the points below blocks the merge.

• Generic and unset dialects stay on the old gosqlx.Parse, ParseWithRecovery and Format path. The comment explaining why passing "generic" to ParseWithDialect isn't equivalent (the ILIKE case) is useful.
• dialectFromSettings separates "no setting" (keep the current dialect) from "invalid setting" (fall back to generic), and it reuses dialectFromInitOptions, so initialize and runtime agree.
• Pool usage is correct. Each Get has a deferred Put, and the comments note that Put resets the dialect.
• handleMessage now recognises client responses instead of answering them with InvalidRequest. The semanticTokens/refresh request is gated on refreshSupport.
• Dropping a stale publish under publishMu also behaves correctly for an A→B→A switch. The dialect equals the one parsed under, so the result is published and is correct.

Points to consider

1. A document opened during a switch can end up with no diagnostics (dialect.go:253). revalidateOpenDocuments takes snapshots() once. If didOpen stores a document and its validation parses under the old dialect, the validation's publish is dropped when the dialect switches. If the snapshot was taken before the document was stored, nobody re-validates it until the next edit. The window is tiny. A fix is for validateDocument to re-run itself once when it drops a result. Another is to loop revalidateOpenDocuments until the dialect is unchanged across the pass.

2. publishMu is held across SendNotification (handler.go, validateDocument). A slow client write blocks every document's validation. That may be intended, since it is what gives the ordering guarantee, so a one-line comment saying so would help. Please also confirm that sendMessage can never call back into the handler.

3. revalidateOpenDocuments runs synchronously in the notification path. With many large open documents, a switch blocks other notifications for the sum of all parse times. If HandleNotification isn't already dispatched off the read loop, run the re-validation in a goroutine. The publishMu plus dialect check already makes that safe.

4. Minor

   • isDialectKeyword scans linearly on every hover. A map[SQLDialect]map[string]struct{} built in buildDialectKeywordCompletions would make it O(1). It is negligible at current sizes.
   • formatForDialect silently ignores opts.AddSemicolon. The comment documents this, but a test asserting that the LSP never sets it would guard against a regression.
   • The handler only reads pushed settings, so pull-style workspace/configuration isn't covered. Note this in LSP_GUIDE.md.

Tests: I could not tell from the diff whether dialect_test.go covers these two cases. If it doesn't, add them:

• A client response such as {"id":1,"result":null} is not answered with an error.
• No semanticTokens/refresh request is sent when refreshSupport is absent.

Security: No new concerns. Settings are parsed with safe type assertions, and unknown dialects fall back to generic.
· branch feat/lsp-dialect-aware

[github-actions]

Claude finished @ajitpratap0's task in 22s —— View job

---

Review of PR #828: dialect-aware LSP features

I read the diff for handler.go, server.go and documents.go, and all of dialect.go. I did not run the tests or -race, so this is a read-through only. It repeats most of the earlier reviews, and I re-checked each point against the code.

Verdict: The design is sound and I see no blocking problems.

What works well

• Generic and unset dialects stay on the old gosqlx.Parse, ParseWithRecovery and Format path. The ILIKE explanation in dialect.go is the right reason for that choice.
• dialectFromSettings reuses dialectFromInitOptions. Initialize and runtime therefore validate the same way, and null or missing settings keep the current dialect.
• The tokenizer and parser pool Put calls are deferred, so the dialect is reset.
• Responses from the client are no longer answered with InvalidRequest. semanticTokens/refresh is gated on refreshSupport.
• The dialect check under publishMu makes the A→B→A switch correct.

Points to consider, most important first

1. A document opened during a switch can end up without diagnostics (revalidateOpenDocuments, dialect.go).

   • snapshots() is taken once.
   • A didOpen that stored its document after the snapshot, and that parsed under the old dialect, has its publish dropped by validateDocument.
   • Nothing re-validates that document until the next edit.
   • The window is tiny. One fix is for validateDocument to retry once when it drops a result. Another is to loop until currentDialect() is unchanged across a pass.

2. publishMu is held across SendNotification.

   • A slow client write blocks every document's publish.
   • Holding it there is what guarantees ordering, so I would keep it, but add a comment saying it is deliberate.
   • Please confirm that sendMessage never calls back into the handler.

3. revalidateOpenDocuments runs synchronously in the notification path.

   • Many large open documents will delay later notifications by the total parse time.
   • Check whether HandleNotification already runs off the read loop. If it doesn't, run the re-validation in a goroutine.
   • The dialect check in validateDocument already makes that safe.
   • The refresh request should be sent after the re-validation finishes, or the goroutine should include it.

4. Minor

   • isDialectKeyword scans linearly on every hover. A map[SQLDialect]map[string]struct{} built in buildDialectKeywordCompletions would make it O(1).
   • formatForDialect ignores opts.AddSemicolon. A small test asserting that the LSP never sets it would guard against a regression.
   • Only pushed settings are read. Please say in LSP_GUIDE.md that pull-style workspace/configuration is not supported.

Tests: If dialect_test.go doesn't already cover these, please add:

• A client response such as {"id":1,"result":null} produces no error reply, and neither does a response carrying an error member.
• No workspace/semanticTokens/refresh request is sent when refreshSupport is absent.

Security: I found no new concerns. Settings use safe type assertions, and unknown dialects fall back to generic. The nextRequestID counter is atomic.
· branch feat/lsp-dialect-aware

[ajitpratap0]

Merge train: parent #825 was squash-merged and this PR is now retargeted to main, but it conflicts with current main (squash-merged parent commits). Owning session: please rebase onto current main and resolve conflicts; the train will skip #828 until then.

[ajitpratap0]

Merged origin/main into feat/lsp-dialect-aware (merge commit 45305b4, normal fast-forward push, no force-push, no rebase). Conflicts were resolved by integrating both sides' changes; the main-side dialect-list work (#829, keywords.DialectNames) was merged in alongside this branch's changes, and CHANGELOG.md [Unreleased] hunks kept both sides' entries (union, no rewording). Verification was run before the push.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9

[github-actions]

Claude finished @ajitpratap0's task in 20s —— View job

---

Review of PR #828: status check

Merge status: On this checkout, origin/main is e6f39fc (#825). The branch already contains it through merge commit 45305b4, so git merge-base HEAD origin/main equals origin/main. I found no conflict on this branch. If the merge train still reports one, it was probably computed before that merge commit. Re-run the train. I can't rebase or merge branches from here, and nothing needs to change.

Still open from the earlier reviews. I re-checked these against the code, but I did not run the tests or -race.

1. Lost diagnostics on open during a switch. revalidateOpenDocuments takes snapshots() once. A didOpen stored after that snapshot, and parsed under the old dialect, has its publish dropped. Nothing re-validates it until the next edit. The window is tiny. Retrying once in validateDocument when it drops a result would close it.
2. publishMu held across the send (handler.go:435). This gives the ordering guarantee. Add a comment saying it is deliberate.
3. Synchronous re-validation. The only go statements I found in server.go don't show notifications being dispatched off the read loop. If that's right, a switch with many large open documents delays later notifications. Running re-validation, then the refresh request, in a goroutine is safe because of the dialect check.
4. Minor.
   • isDialectKeyword does a linear scan on every hover. A set would make it O(1).
   • Add a guard test that the LSP never sets AddSemicolon.
   • Note in LSP_GUIDE.md that pull-style workspace/configuration is not supported.

Tests. TestDialect_DidChangeConfiguration_NoRefreshWithoutSupport exists, so that case is covered. I found no test for a client response such as {"id":1,"result":null}, or one with an error member, being left unanswered. I'd add one.

None of this blocks the merge.
· branch feat/lsp-dialect-aware

[github-actions]

Claude finished @ajitpratap0's task in 20s —— View job

---

Review of PR #828: dialect-aware LSP features

I read dialect.go in full and the validateDocument publish path in handler.go. I did not run the tests or -race, so this is a read-through only.

Verdict: The design is sound and nothing here blocks the merge. This repeats most of the earlier reviews, and I re-checked each point against the code.

What works well

• Generic and unset dialects stay on the old gosqlx.Parse, ParseWithRecovery and Format path. The ILIKE explanation in dialect.go:36-44 is the right reason for that choice.
• dialectFromSettings separates "no setting" (keep the current dialect) from "invalid setting" (fall back to generic), and reuses dialectFromInitOptions.
• The tokenizer and parser pool Put calls are deferred, so the dialect is reset.
• The dialect check under publishMu (handler.go:435-440) correctly drops a stale result, and it is also correct for an A→B→A switch.

Points to consider, most important first

1. A document opened during a switch can end up without diagnostics (dialect.go:253-268).

   • snapshots() is taken once.
   • A didOpen that stored its document after the snapshot, and parsed under the old dialect, has its publish dropped at handler.go:437.
   • Nothing re-validates that document until the next edit.
   • The window is tiny. Retrying once in validateDocument when it drops a result would close it, and so would looping until currentDialect() is stable across a pass.

2. revalidateOpenDocuments runs synchronously (dialect.go:237).

   • server.go:405 calls HandleNotification inline, so a switch with many large open documents delays every later message by the total parse time.
   • Running re-validation and then the refresh request in a goroutine would be safe, because validateDocument already drops stale results.
   • If you do this, send the refresh after the re-validation finishes.

3. publishMu is held across SendNotification (handler.go:435).

   • A slow client write blocks every document's publish.
   • The hold is what gives the ordering guarantee, so I would keep it, but add a comment saying it is deliberate.

4. Minor

   • isDialectKeyword (dialect.go:173) scans linearly on every hover. A map[SQLDialect]map[string]struct{} built next to dialectKeywordCompletions would make it O(1).
   • formatForDialect ignores opts.AddSemicolon. A guard test that the LSP never sets it would protect against a regression.
   • Only pushed settings are read. Note in LSP_GUIDE.md that pull-style workspace/configuration is not supported.

Tests

• TestDialect_DidChangeConfiguration_NoRefreshWithoutSupport covers the case where refreshSupport is absent.
• I found no test that a client response ({"id":1,"result":null}, or one with an error member) is left unanswered. I'd add one.

Security: I found no new concerns. Settings use safe type assertions, and unknown dialects fall back to generic.
· branch feat/lsp-dialect-aware