Skip to content

keep unimplemented notifications from closing connections#60

Merged
zchee merged 7 commits into
mainfrom
fix/notif-handlers-nil
Jun 27, 2026
Merged

keep unimplemented notifications from closing connections#60
zchee merged 7 commits into
mainfrom
fix/notif-handlers-nil

Conversation

@zchee

@zchee zchee commented Jun 27, 2026

Copy link
Copy Markdown
Member

Summary

This PR fixes the default unimplemented LSP stubs so unhandled notifications are ignored instead of failing the JSON-RPC connection, while preserving method not found responses for unhandled requests.

It also cleans up the generated protocol surface around InitializeParams by flattening private _-prefixed meta-model bases into their public referrers, adds explicit constructors for Nullable[T], and tightens the structure-flattening generator so duplicate inherited fields/embeds are emitted only once.

Why

A JSON-RPC notification has no response slot. Returning a non-nil error from an unimplemented notification handler causes the dispatcher to treat that error as a connection-level failure. That made embedders of UnimplementedServer or UnimplementedClient lose the connection when a peer sent a notification they did not override, such as early window/logMessage traffic.

The LSP metaModel also exposes private helper structures such as _InitializeParams. Emitting those as public Go types leaks implementation detail into the API and adds avoidable indirection on initialize encoding/decoding.

What changed

  • Return nil from unimplemented notification stubs so notifications are ignored per LSP/JSON-RPC semantics.
  • Keep errNotImplemented for request stubs so peers still receive classified method not found responses.
  • Add integration coverage proving an unimplemented client survives a server-sent notification and remains usable for later requests.
  • Flatten _-prefixed meta-model structures into public referrers and emit empty structs as struct{}.
  • Deduplicate inherited fields and embeds during recursive private-base flattening.
  • Add NewNullable(v) and NullNullable[T]() constructors for the value and explicit-null states of Nullable[T].
  • Keep cleanup-only review noise out of the generated/nullable changes without altering generated output.

Impact

Consumers can embed UnimplementedServer or UnimplementedClient and override only the methods they support without unhandled notifications tearing down the connection. Generated initialize types are also cleaner and avoid exposing private meta-model helper types.

Validation

  • go test ./...
  • go vet ./...
  • go tool golangci-lint run ./... (0 issues)

zchee added 4 commits June 23, 2026 08:41
…ection alive

A non-nil error from a notification handler has no response slot, so the
jsonrpc2 dispatcher escalates it to conn.fail and tears the connection
down. UnimplementedServer/UnimplementedClient returned errNotImplemented
uniformly, so an embedder that did not override every notification lost
the connection the instant a peer sent one (e.g. gopls front-loading
window/logMessage right after initialize, before initialized/didOpen).

Return nil from the 26 notification stubs (ignoring the notification, as
the LSP spec prescribes) and keep errNotImplemented only on request
stubs, where it correctly becomes a method-not-found response. Doc
comments now spell out the request-vs-notification distinction so the
stubs are not "consistency-fixed" back.
The LSP metaModel splits InitializeParams into a private
_InitializeParams base plus a WorkspaceFoldersInitializeParams mixin.
Embedding the private base leaked an underscore-named type into the
public API and forced the parent to reach its own fields through an
extra indirection on the hot initialize decode path.

Flatten any extends/mixins reference whose target name begins with
"_" directly into the referrer: inline the base's embeds and rendered
fields (rendered against the public owner so doc hints and hot-field
overrides match), and stop emitting underscore structs as standalone
types. InitializeParams now carries WorkDoneProgressParams and the
eight former base fields itself, and _InitializeParams is gone.

Also collapse zero-field, zero-embed structs to the canonical
struct{} form, since gofmt keeps the multi-line empty body as-is.

Regenerated output is included; the net reduction comes from the
parent no longer delegating codec methods through the dropped embed.
Nullable[T] is tri-state (absent / explicit null / value), so the
single NewOptional-style constructor only covers the value arm.
Callers had to spell struct literals like Nullable[T]{set: true,
null: true} to build the null state, mirroring internal generated
code.

Add NewNullable(v) for the present-value state and NullNullable[T]()
for the explicit-null state. Absent needs no constructor since it is
the zero Nullable[T]{}. Cover all three states and their JSON
round-trip via a real Nullable field.
When recursively flattening private base structures (those prefixed
with an underscore), diamond inheritance or overlapping mixins could
previously produce duplicate struct fields or embedded types. This
would lead to Go compile errors.

This change introduces deduplication tracking in inlineContribution
so each embedded type and property is emitted at most once per
public structure.
@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.77419% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.2%. Comparing base (2d08f2b) to head (dbfd534).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
internal/genlsp/emit.go 96.6% 2 Missing ⚠️
@@          Coverage Diff          @@
##            main     #60   +/-   ##
=====================================
  Coverage   85.1%   85.2%           
=====================================
  Files         27      27           
  Lines       6077    6135   +58     
=====================================
+ Hits        5177    5230   +53     
- Misses       900     905    +5     
Flag Coverage Δ
Linux-ARM64 85.2% <96.7%> (+<0.1%) ⬆️
Linux-X64 87.5% <95.9%> (+<0.1%) ⬆️
Files with missing lines Coverage Δ
nullable.go 90.9% <100.0%> (+0.9%) ⬆️
protocol.go 100.0% <ø> (ø)
internal/genlsp/emit.go 90.9% <96.6%> (+<0.1%) ⬆️

Preserve the behavior-only cleanup as a narrow review aid.
Keep generator traversal allocation-free with unchanged output.
Document the stable NewClient return shape instead of breaking API.
@zchee zchee force-pushed the fix/notif-handlers-nil branch from ff20a21 to 19a4c8b Compare June 27, 2026 11:52
@zchee zchee changed the title [codex] keep unimplemented notifications from closing connections keep unimplemented notifications from closing connections Jun 27, 2026
@zchee zchee marked this pull request as ready for review June 27, 2026 11:53
@zchee

zchee commented Jun 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request flattens underscore-prefixed private base structures (such as _InitializeParams) directly into their public referrers during LSP code generation, eliminating the need to emit them as separate types. It also updates UnimplementedServer and UnimplementedClient notification methods to return nil instead of errNotImplemented to prevent connection teardown when notifications are unhandled, supported by a new integration test. Additionally, helper constructors NewNullable and NullNullable are introduced. The review feedback suggests tracking seenEmbeds and seenFields at the public structure level and passing them into inlineContribution to prevent duplicate fields or embeds when a public structure embeds multiple private bases or defines overlapping fields.

Comment thread internal/genlsp/emit.go
Comment thread internal/genlsp/emit.go Outdated
Share embed and field dedupe state across each public structure.
Let public fields override inherited private-base fields before render.
Cover duplicate private bases and direct public overrides in tests.
@zchee zchee force-pushed the fix/notif-handlers-nil branch from 4b5fd2e to dadabcc Compare June 27, 2026 12:05
@zchee

zchee commented Jun 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request flattens private base structures (prefixed with an underscore) directly into their public referrers within the LSP generator, simplifying the generated Go structs and removing unnecessary private types. Additionally, it updates un-overridden notification methods in UnimplementedServer and UnimplementedClient to return nil instead of errNotImplemented, preventing connection teardowns when unhandled notifications are received. A review comment correctly identifies a potential infinite recursion risk in the recursive flatten function of the generator and suggests adding cycle detection.

Comment thread internal/genlsp/emit.go
Comment on lines +274 to +275
var flatten func(structure *Structure)
flatten = func(structure *Structure) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential infinite recursion and stack overflow in case of cyclic dependencies in the meta-model structures (e.g., if a private base structure directly or indirectly references itself), it is safer to implement cycle detection using a visited map within the recursive flatten function.

	visited := make(map[string]bool)
	var flatten func(structure *Structure)
	flatten = func(structure *Structure) {
		if visited[structure.Name] {
			return
		}
		visited[structure.Name] = true

Stop recursive private-base flattening when a structure repeats.
Cover cyclic private bases so generation cannot recurse forever.
Keep generated output unchanged for the current acyclic meta-model.
@zchee zchee force-pushed the fix/notif-handlers-nil branch from a8c26c4 to dbfd534 Compare June 27, 2026 12:40
@zchee zchee merged commit dbfd534 into main Jun 27, 2026
9 checks passed
@zchee zchee deleted the fix/notif-handlers-nil branch June 27, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant