From 003381c863419b11cb4287f73eaf907ce74f2b57 Mon Sep 17 00:00:00 2001 From: karitham Date: Fri, 7 Aug 2026 23:51:31 +0200 Subject: [PATCH] lsp: fix n-cycle detection, enum-qualified rename, prepare rename Include cycle detection only caught 2-cycles; an include edge now reports when its target transitively includes the including file, so cycles of any length (and self-includes) are found. Renaming an enum left references in value positions untouched: user.Test3.TWO kept the old enum name. Enum renames now also scan value positions and rewrite just the enum segment, keeping the value. The pinned rename/reference tests that enshrined the gap are updated to the correct behavior. PrepareRename rejected type references that Rename handles, and the server advertised PrepareProvider: false while implementing the method. PrepareRename now accepts type references (rejecting basic types like Rename) and the capability is advertised. Dead code dropped: FilesMap.Destroy, Snapshot.Graph, IsContainerType, ParsedFile.DumpAST, and the view.go invariant comment describing a destroy/shutdown machinery that never existed. DebugHandler now returns the panic as an error instead of a silent success. --- lsp/cache/didchange_test.go | 60 ++++++++++++++ lsp/cache/file.go | 11 +-- lsp/cache/parse.go | 11 --- lsp/cache/view.go | 15 ++-- lsp/debug.go | 8 +- lsp/initialize.go | 2 +- lsp/source/cycle_detect.go | 43 +++++++--- lsp/source/cycle_detect_test.go | 11 +-- lsp/source/reference.go | 136 +++++++++++++++++++++++++++----- lsp/source/reference_test.go | 15 ++++ lsp/source/rename.go | 11 +++ lsp/source/rename_enum_test.go | 94 ++++++++++++++++++++++ lsp/source/rename_test.go | 51 ++++++++++++ lsp/source/utils.go | 13 --- 14 files changed, 400 insertions(+), 81 deletions(-) create mode 100644 lsp/cache/didchange_test.go create mode 100644 lsp/source/rename_enum_test.go diff --git a/lsp/cache/didchange_test.go b/lsp/cache/didchange_test.go new file mode 100644 index 0000000..74a5619 --- /dev/null +++ b/lsp/cache/didchange_test.go @@ -0,0 +1,60 @@ +package cache + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.lsp.dev/protocol" +) + +// TestFileChangeFromLSPDidChange pins whole-document sync semantics: the +// server advertises TextDocumentSyncKindFull, so whole-document events map +// one-to-one and incremental (partial) events are skipped. +func TestFileChangeFromLSPDidChange(t *testing.T) { + tests := []struct { + name string + content []protocol.TextDocumentContentChangeEvent + wantContent []byte + want int // number of file changes produced + }{ + { + name: "whole document", + content: []protocol.TextDocumentContentChangeEvent{ + &protocol.TextDocumentContentChangeWholeDocument{Text: "struct HTT {}"}, + }, + wantContent: []byte("struct HTT {}"), + want: 1, + }, + { + name: "incremental change is skipped", + content: []protocol.TextDocumentContentChangeEvent{ + &protocol.TextDocumentContentChangePartial{ + Range: protocol.Range{ + Start: protocol.Position{Line: 0, Character: 7}, + End: protocol.Position{Line: 0, Character: 10}, + }, + Text: "HoukagoTeaTime", + }, + }, + want: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + changes := FileChangeFromLSPDidChange(&protocol.DidChangeTextDocumentParams{ + TextDocument: protocol.VersionedTextDocumentIdentifier{ + TextDocumentIdentifier: protocol.TextDocumentIdentifier{URI: "file:///tmp/song.thrift"}, + }, + ContentChanges: tt.content, + }) + + require.Len(t, changes, tt.want) + + if tt.want == 1 { + assert.Equal(t, tt.wantContent, changes[0].Content) + } + }) + } +} diff --git a/lsp/cache/file.go b/lsp/cache/file.go index 7e70460..fc40c46 100644 --- a/lsp/cache/file.go +++ b/lsp/cache/file.go @@ -177,11 +177,6 @@ func (m *FilesMap) copyOnWrite() { m.shared = false } -func (m *FilesMap) Destroy() { - m.files = nil - m.overlays = nil -} - type FileChangeType string const ( @@ -204,13 +199,15 @@ func (f *FileChange) FullContent(base []byte) []byte { return f.Content } +// FileChangeFromLSPDidChange converts a didChange payload into file +// changes. The server advertises whole-document sync, so whole-document +// events map one-to-one; incremental (partial) events are a client +// protocol violation and are skipped. func FileChangeFromLSPDidChange(params *protocol.DidChangeTextDocumentParams) []*FileChange { changes := make([]*FileChange, 0, len(params.ContentChanges)) for i := range params.ContentChanges { event, ok := params.ContentChanges[i].(*protocol.TextDocumentContentChangeWholeDocument) if !ok { - // Incremental changes are not supported; fall back to full reload - // semantics using the current full content. continue } diff --git a/lsp/cache/parse.go b/lsp/cache/parse.go index aefb7b3..3fac1aa 100644 --- a/lsp/cache/parse.go +++ b/lsp/cache/parse.go @@ -1,7 +1,6 @@ package cache import ( - "encoding/json" "fmt" "log/slog" "maps" @@ -311,16 +310,6 @@ func (p *ParsedFile) AggregatedError() error { return fmt.Errorf("aggregated error: %v", p.errs) } -// DumpAST is for debug. -func (p *ParsedFile) DumpAST() { - if p.ast == nil { - return - } - - data, _ := json.MarshalIndent(p.ast, "", " ") - fmt.Println(string(data)) -} - // Parse lexes and parses the file content into a ParsedFile. func Parse(fh FileHandle) (*ParsedFile, error) { content, err := fh.Content() diff --git a/lsp/cache/view.go b/lsp/cache/view.go index 1b5aca6..185d4e4 100644 --- a/lsp/cache/view.go +++ b/lsp/cache/view.go @@ -28,16 +28,11 @@ type View struct { includePaths []string - // Track the latest snapshot via the snapshot field, guarded by snapshotMu. - // - // Invariant: whenever the snapshot field is overwritten, destroy(snapshot) - // is called on the previous (overwritten) snapshot while snapshotMu is held, - // incrementing snapshotWG. During shutdown the final snapshot is - // overwritten with nil and destroyed, guaranteeing that all observed - // snapshots have been destroyed via the destroy method, and snapshotWG may - // be waited upon to let these destroy operations complete. + // Track the latest snapshot via the snapshot field, guarded by + // snapshotMu. The swap in FileChange releases the previous snapshot's + // ref under the same lock. snapshotMu sync.Mutex - snapshot *Snapshot // latest snapshot; nil after shutdown has been called + snapshot *Snapshot // latest snapshot snapshotRelease func() } @@ -131,10 +126,12 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. v.snapshotMu.Lock() newSnapshot, release := v.snapshot.clone() v.snapshotRelease() + v.snapshot = newSnapshot for _, change := range changes { newSnapshot.ForgetFile(change.URI) } + v.snapshotRelease = release v.snapshotMu.Unlock() diff --git a/lsp/debug.go b/lsp/debug.go index 8045763..1c9018b 100644 --- a/lsp/debug.go +++ b/lsp/debug.go @@ -2,13 +2,16 @@ package lsp import ( "context" + "fmt" "log/slog" + "runtime/debug" "go.lsp.dev/jsonrpc2" ) // DebugHandler wraps a jsonrpc2.Handler with request/response debug logging -// and panic recovery. +// and panic recovery. A recovered panic becomes an error response, so the +// client never sees a success for a request that crashed. func DebugHandler(handler jsonrpc2.Handler) jsonrpc2.Handler { return func(ctx context.Context, req *jsonrpc2.Request) (result any, err error) { if req != nil { @@ -17,7 +20,8 @@ func DebugHandler(handler jsonrpc2.Handler) jsonrpc2.Handler { defer func() { if r := recover(); r != nil { - slog.Error("recovered from panic", "panic", r) + slog.Error("recovered from panic", "panic", r, "stack", string(debug.Stack())) + err = fmt.Errorf("panic: %v", r) } }() diff --git a/lsp/initialize.go b/lsp/initialize.go index aeb1232..19b2732 100644 --- a/lsp/initialize.go +++ b/lsp/initialize.go @@ -218,7 +218,7 @@ func initializeResult() *protocol.InitializeResult { MoreTriggerCharacter: []string{}, }, RenameProvider: &protocol.RenameOptions{ - PrepareProvider: new(false), + PrepareProvider: new(true), }, ExecuteCommandProvider: protocol.ExecuteCommandOptions{ Commands: []string{}, diff --git a/lsp/source/cycle_detect.go b/lsp/source/cycle_detect.go index f5dae96..32b3e3b 100644 --- a/lsp/source/cycle_detect.go +++ b/lsp/source/cycle_detect.go @@ -60,18 +60,43 @@ type CyclePair struct { include Include } +// cycleDetect returns every include edge that closes a cycle: the pair +// (file, include file->Y) is reported when Y transitively includes file. +// Cycles of any length are caught, including self-includes. func cycleDetect(includesMap *map[uri.URI][]Include) []CyclePair { + // reaches reports whether from can reach target via include edges, + // cycle-safe via the seen set. + var reaches func(from, target uri.URI, seen map[uri.URI]bool) bool + + reaches = func(from, target uri.URI, seen map[uri.URI]bool) bool { + if from == target { + return true + } + + if seen[from] { + return false + } + + seen[from] = true + + for _, inc := range (*includesMap)[from] { + if reaches(inc.file, target, seen) { + return true + } + } + + return false + } + cyclePairs := make([]CyclePair, 0) - for uri, includes := range *includesMap { - for _, incI := range includes { - for _, incJ := range (*includesMap)[incI.file] { - if uri == incJ.file { - cyclePairs = append(cyclePairs, CyclePair{ - file: uri, - include: incI, - }) - } + for file, includes := range *includesMap { + for _, inc := range includes { + if reaches(inc.file, file, make(map[uri.URI]bool)) { + cyclePairs = append(cyclePairs, CyclePair{ + file: file, + include: inc, + }) } } } diff --git a/lsp/source/cycle_detect_test.go b/lsp/source/cycle_detect_test.go index f259680..36ad4da 100644 --- a/lsp/source/cycle_detect_test.go +++ b/lsp/source/cycle_detect_test.go @@ -98,15 +98,6 @@ func Test_cycleDetectN(t *testing.T) { club = "/clubroom.thrift" ) - include := func(from uri.URI, tos ...uri.URI) map[uri.URI][]Include { - m := make(map[uri.URI][]Include, len(tos)) - for _, to := range tos { - m[from] = append(m[from], Include{file: to}) - } - - return m - } - tests := []struct { name string graph map[uri.URI][]Include @@ -183,7 +174,9 @@ func Test_cycleDetectN(t *testing.T) { {file: club, include: Include{file: git}}, {file: tea, include: Include{file: bass}}, {file: git, include: Include{file: drum}}, + {file: bass, include: Include{file: drum}}, {file: bass, include: Include{file: club}}, + {file: drum, include: Include{file: tea}}, }, }, } diff --git a/lsp/source/reference.go b/lsp/source/reference.go index 23feb39..d68c756 100644 --- a/lsp/source/reference.go +++ b/lsp/source/reference.go @@ -156,7 +156,25 @@ func searchDefinitionReferences(ctx context.Context, ss *cache.Snapshot, file ur typeName := fmt.Sprintf("%s.%s", includeNameOf(file), id.Text) - return searchIdentifierReferences(ctx, ss, file, typeName, kind) + typeRefs, err := searchIdentifierReferences(ctx, ss, file, typeName, kind) + if err != nil { + return res, err + } + + res = append(res, typeRefs...) + + // Enum renames also touch value positions: identifiers like + // songs.Song.FUWA_FUWA_TIME reference the enum by name. + if kind == DefinitionEnum { + valueRefs, err := searchEnumQualifiedValueReferences(ctx, ss, file, id.Text) + if err != nil { + return res, err + } + + res = append(res, valueRefs...) + } + + return res, err } func grandparent(path []syntax.Node) syntax.Node { @@ -475,42 +493,120 @@ func searchConstValueIdentifierReference(ctx context.Context, ss *cache.Snapshot return res, err } - jumpValue := func(v *syntax.ConstValue) { - if v != nil && v.Kind == syntax.ValueIdent && bareName(v.Text) == bareName(valueName) { + walkValueIdentifiers(pf.AST(), func(v *syntax.ConstValue) { + if v.Kind == syntax.ValueIdent && bareName(v.Text) == bareName(valueName) { res = append(res, referenceHit{loc: jump(file, pf, v), text: v.Text}) } + }) + + return res, err +} + +// searchEnumQualifiedValueReferences finds references to an enum in value +// positions: field defaults and const values qualified with the enum name, +// e.g. songs.Song.FUWA_FUWA_TIME or Song.FUWA_FUWA_TIME. Each hit covers +// only the enum segment of the identifier, so the rename rewrites the +// qualifier and keeps the value. +func searchEnumQualifiedValueReferences(ctx context.Context, ss *cache.Snapshot, file uri.URI, enumName string) (res []referenceHit, err error) { + locations, err := searchEnumQualifiedValueReference(ctx, ss, file, enumName) + if err != nil { + return nil, err + } + + res = append(res, locations...) + + for _, referenceFile := range referenceFiles(ss, file) { + locations, err := searchEnumQualifiedValueReference(ctx, ss, referenceFile, enumName) + if err != nil { + return nil, err + } + + res = append(res, locations...) } - processStructLike := func(fields []*syntax.Field) { + + return res, err +} + +func searchEnumQualifiedValueReference(ctx context.Context, ss *cache.Snapshot, file uri.URI, enumName string) (res []referenceHit, err error) { + pf, err := ss.Parse(ctx, file) + if err != nil || pf.AST() == nil { + return res, err + } + + // qualifier returns the enum segment of a value identifier and its + // byte offset within the identifier, when the identifier is + // . or .. with the enum name. + qualifier := func(text string) (seg string, off int, ok bool) { + items := strings.Split(text, ".") + if len(items) == 2 && items[0] == enumName { + return items[0], 0, true + } + + if len(items) == 3 && items[1] == enumName { + return items[1], len(items[0]) + 1, true + } + + return "", 0, false + } + + walkValueIdentifiers(pf.AST(), func(v *syntax.ConstValue) { + if v.Kind != syntax.ValueIdent { + return + } + + seg, off, ok := qualifier(v.Text) + if !ok { + return + } + + start, _ := pf.AST().Range(v) + segStart := toLSPPosition(pf, syntax.Position{Line: start.Line, Col: start.Col, Offset: start.Offset + off}) + segEnd := toLSPPosition(pf, syntax.Position{Line: start.Line, Col: start.Col, Offset: start.Offset + off + len(seg)}) + + res = append(res, referenceHit{ + loc: protocol.Location{URI: file, Range: protocol.Range{Start: segStart, End: segEnd}}, + text: seg, + }) + }) + + return res, err +} + +// walkValueIdentifiers visits every constant value in a value position: +// field defaults, const values, and service argument and throws defaults. +// Positions without a default are skipped. +func walkValueIdentifiers(doc *syntax.Document, fn func(v *syntax.ConstValue)) { + process := func(fields []*syntax.Field) { for _, field := range fields { - jumpValue(field.Value) + if field.Value != nil { + fn(field.Value) + } } } - for _, st := range pf.AST().Structs() { - processStructLike(st.Fields) + for _, st := range doc.Structs() { + process(st.Fields) } - for _, st := range pf.AST().Unions() { - processStructLike(st.Fields) + for _, st := range doc.Unions() { + process(st.Fields) } - for _, st := range pf.AST().Exceptions() { - processStructLike(st.Fields) + for _, st := range doc.Exceptions() { + process(st.Fields) } - for _, cst := range pf.AST().Consts() { - jumpValue(cst.Value) + for _, cst := range doc.Consts() { + fn(cst.Value) } - for _, svc := range pf.AST().Services() { - for _, fn := range svc.Functions { - processStructLike(fn.Args) + for _, svc := range doc.Services() { + for _, fnx := range svc.Functions { + process(fnx.Args) - if fn.Throws != nil { - processStructLike(fn.Throws.Fields) + if fnx.Throws != nil { + process(fnx.Throws.Fields) } } } - - return res, err } diff --git a/lsp/source/reference_test.go b/lsp/source/reference_test.go index 23a7ecf..07d68e5 100644 --- a/lsp/source/reference_test.go +++ b/lsp/source/reference_test.go @@ -310,6 +310,21 @@ const UserKind kind = "1" }, }, }, + { + // The enum-qualified value reference in the default + // value position. + URI: "file:///tmp/api.thrift", + Range: protocol.Range{ + Start: protocol.Position{ + Line: 3, + Character: 55, + }, + End: protocol.Position{ + Line: 3, + Character: 60, + }, + }, + }, }, assertion: assert.NoError, }, diff --git a/lsp/source/rename.go b/lsp/source/rename.go index c183d69..8b3e5ae 100644 --- a/lsp/source/rename.go +++ b/lsp/source/rename.go @@ -24,6 +24,17 @@ func PrepareRename(ctx context.Context, ss *cache.Snapshot, file uri.URI, pos pr case TargetDefinition, TargetConstValue, TargetService: rg := nodeRange(pf, target.node) + return &rg, nil + case TargetTypeName: + // Rename supports type references; basic types are the only + // position it rejects, so prepare must reject them too. + ft := target.parent.(*syntax.FieldType) + if typeReferenceName(ft) == "" || IsBasicType(typeReferenceName(ft)) { + return nil, fmt.Errorf("rename not supported for basic types") + } + + rg := nodeRange(pf, target.node) + return &rg, nil } diff --git a/lsp/source/rename_enum_test.go b/lsp/source/rename_enum_test.go new file mode 100644 index 0000000..3e14dcd --- /dev/null +++ b/lsp/source/rename_enum_test.go @@ -0,0 +1,94 @@ +package source + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.lsp.dev/protocol" + "go.lsp.dev/uri" + + "github.com/karitham/thrift-ls/lsp/cache" +) + +// TestRenameEnumQualifiedValues pins that renaming an enum also updates +// references in value positions (field defaults, const values), where the +// enum name appears as the qualifier of an enum value: songs.Song.FUWA +// becomes songs.NewName.FUWA, keeping the value part. +func TestRenameEnumQualifiedValues(t *testing.T) { + tests := []struct { + name string + files map[string]string + cursor uri.URI + pos protocol.Position + newName string + // want maps each file to the ordered NewTexts of its edits. + want map[string][]string + }{ + { + name: "cross-file qualified value references", + files: map[string]string{ + "file:///tmp/songs.thrift": "enum Song {\n FUWA_FUWA_TIME = 1,\n MY_SONG = 2\n}\nconst Song favorite = Song.FUWA_FUWA_TIME\n", + "file:///tmp/club.thrift": "include \"songs.thrift\"\nstruct Club {\n 1: optional songs.Song favorite = songs.Song.FUWA_FUWA_TIME\n}\n", + }, + cursor: "file:///tmp/songs.thrift", + pos: protocol.Position{Line: 0, Character: 5}, // 'S' of Song + newName: "HoukagoTeaTime", + want: map[string][]string{ + "file:///tmp/songs.thrift": { + "HoukagoTeaTime", // the const's Song type reference + "HoukagoTeaTime", // the same-file Song.FUWA_FUWA_TIME qualifier + "HoukagoTeaTime", // the enum definition under the cursor + }, + "file:///tmp/club.thrift": { + "songs.HoukagoTeaTime", // the type reference + "HoukagoTeaTime", // the songs.Song.FUWA_FUWA_TIME qualifier + }, + }, + }, + { + name: "type rename leaves value references untouched", + files: map[string]string{ + "file:///tmp/songs.thrift": "enum Song {\n FUWA_FUWA_TIME = 1\n}\n", + "file:///tmp/club.thrift": "include \"songs.thrift\"\nstruct Club {\n 1: optional songs.Song favorite = songs.Song.FUWA_FUWA_TIME\n}\n", + }, + cursor: "file:///tmp/club.thrift", + pos: protocol.Position{Line: 2, Character: 14}, // 'S' of the songs.Song type reference + newName: "Track", + want: map[string][]string{ + "file:///tmp/club.thrift": { + "songs.Track", // the type reference only + "Track", // the identifier under the cursor + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + changes := make([]*cache.FileChange, 0, len(tt.files)) + for file, src := range tt.files { + changes = append(changes, &cache.FileChange{ + URI: uri.URI(file), + Version: 0, + Content: []byte(src), + From: cache.FileChangeTypeDidOpen, + }) + } + + ss := cache.BuildSnapshotForTest(changes) + + edit, err := Rename(t.Context(), ss, tt.cursor, tt.pos, tt.newName) + require.NoError(t, err) + + for file, wantNewTexts := range tt.want { + var got []string + for _, te := range edit.Changes[uri.URI(file)] { + got = append(got, te.NewText) + } + + assert.Equal(t, wantNewTexts, got, "edits for %s", file) + } + }) + } +} diff --git a/lsp/source/rename_test.go b/lsp/source/rename_test.go index b5b46de..cf26f15 100644 --- a/lsp/source/rename_test.go +++ b/lsp/source/rename_test.go @@ -172,6 +172,42 @@ const UserKind kind = "1" }, assertion: assert.NoError, }, + { + name: "case type reference", + args: args{ + ctx: t.Context(), + ss: ss, + file: "file:///tmp/api.thrift", + pos: protocol.Position{ + Line: 2, + Character: 7, // 'T' of user.Test + }, + }, + wantRes: &protocol.Range{ + Start: protocol.Position{ + Line: 2, + Character: 2, + }, + End: protocol.Position{ + Line: 2, + Character: 11, + }, + }, + assertion: assert.NoError, + }, + { + name: "case basic type reference", + args: args{ + ctx: t.Context(), + ss: ss, + file: "file:///tmp/api.thrift", + pos: protocol.Position{ + Line: 3, + Character: 68, // 's' of string + }, + }, + assertion: assert.Error, + }, { name: "typedef", args: args{ @@ -450,6 +486,21 @@ const UserKind kind = "1" }, NewText: "user.newtext", }, + { + // The enum-qualified value reference: only the + // enum segment is rewritten. + Range: protocol.Range{ + Start: protocol.Position{ + Line: 3, + Character: 55, + }, + End: protocol.Position{ + Line: 3, + Character: 60, + }, + }, + NewText: "newtext", + }, }, }, }, diff --git a/lsp/source/utils.go b/lsp/source/utils.go index ee76ec0..20ade9d 100644 --- a/lsp/source/utils.go +++ b/lsp/source/utils.go @@ -88,13 +88,6 @@ func IsBasicType(t string) bool { return ok } -// IsContainerType reports whether t is a container keyword. -func IsContainerType(t string) bool { - _, ok := containerType[t] - - return ok -} - // typeReferenceName returns the referenced type name of a FieldType, or "" // for base types and containers. func typeReferenceName(ft *syntax.FieldType) string { @@ -134,12 +127,6 @@ var basicType = map[string]struct{}{ "uuid": {}, } -var containerType = map[string]struct{}{ - "list": {}, - "map": {}, - "set": {}, -} - // definitionMatches reports whether the node has the expected definition // kind. func definitionMatches(n syntax.Node, kind DefinitionKind) bool { -- 2.51.2