diff --git a/lsp/codeaction.go b/lsp/codeaction.go index c15f995..1c9fc53 100644 --- a/lsp/codeaction.go +++ b/lsp/codeaction.go @@ -2,6 +2,7 @@ package lsp import ( "context" + "strings" "go.lsp.dev/protocol" @@ -9,19 +10,115 @@ import ( "github.com/karitham/thrift-ls/lsp/source" ) -// codeAction returns the quickfixes for the document: formatting the whole -// document when the range covers it, or the range when it is a selection. +// codeAction returns the code actions for the document: the refactors for +// the code at the selection. An action that fixes a reported diagnostic is +// also offered as a quickfix. Actions are filtered to the kinds the client +// requested. func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionParams) ([]protocol.CommandOrCodeAction, error) { return withFile(ctx, s.session, params.TextDocument.URI, func(ss *cache.Snapshot, fh cache.FileHandle) ([]protocol.CommandOrCodeAction, error) { - action, err := source.FormatDocumentAction(ctx, ss, fh, s.formatOptions()) + var actions []protocol.CodeAction + + enum, err := source.MakeEnumValuesExplicitAction(ctx, ss, fh, params.Range) if err != nil { return nil, err } - - if action == nil { - return nil, nil + if enum != nil { + // A diagnostic on the selection makes the action a quickfix + // for it; the rewrite stays for kind-filtered requests. + if diagnosticOverlaps(params.Context.Diagnostics, params.Range) { + fix := *enum + fix.Kind = new(protocol.CodeActionKindQuickFix) + actions = append(actions, fix) + } + actions = append(actions, *enum) } - return []protocol.CommandOrCodeAction{action}, nil + actions = preferQuickFixes(filterCodeActions(actions, params.Context.Only)) + + out := make([]protocol.CommandOrCodeAction, 0, len(actions)) + for i := range actions { + out = append(out, &actions[i]) + } + return out, nil }) } + +// diagnosticOverlaps reports whether any diagnostic shares a position with +// rng: the client presents a problem there, so the action is a quickfix for +// it. +func diagnosticOverlaps(diags []protocol.Diagnostic, rng protocol.Range) bool { + for _, d := range diags { + if rangesOverlap(rng, d.Range) { + return true + } + } + + return false +} + +// positionBefore reports a <= b. +func positionBefore(a, b protocol.Position) bool { + if a.Line != b.Line { + return a.Line < b.Line + } + + return a.Character <= b.Character +} + +// rangesOverlap reports whether two ranges share at least one position, +// degenerate single-point ranges included. +func rangesOverlap(a, b protocol.Range) bool { + return positionBefore(a.Start, b.End) && positionBefore(b.Start, a.End) +} + +// filterCodeActions keeps only the actions whose kind falls under one of +// the requested kinds. An empty request keeps everything. +func filterCodeActions(actions []protocol.CodeAction, kinds []protocol.CodeActionKind) []protocol.CodeAction { + if len(kinds) == 0 { + return actions + } + + var out []protocol.CodeAction + + for _, act := range actions { + if act.Kind == nil { + continue + } + + for _, kind := range kinds { + if strings.HasPrefix(string(*act.Kind), string(kind)) { + out = append(out, act) + break + } + } + } + + return out +} + +// preferQuickFixes drops a refactor.rewrite action when a quickfix action +// with the same title is also offered, so clients without kind grouping +// show the action once, as the fix. +func preferQuickFixes(actions []protocol.CodeAction) []protocol.CodeAction { + quickfix := make(map[string]bool) + for _, act := range actions { + if act.Kind != nil && *act.Kind == protocol.CodeActionKindQuickFix { + quickfix[act.Title] = true + } + } + + if len(quickfix) == 0 { + return actions + } + + var out []protocol.CodeAction + + for _, act := range actions { + if act.Kind != nil && *act.Kind == protocol.CodeActionKindRefactorRewrite && quickfix[act.Title] { + continue + } + out = append(out, act) + } + + return out +} diff --git a/lsp/codeaction_test.go b/lsp/codeaction_test.go new file mode 100644 index 0000000..e25bf9b --- /dev/null +++ b/lsp/codeaction_test.go @@ -0,0 +1,113 @@ +package lsp + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.lsp.dev/protocol" + "go.lsp.dev/uri" +) + +func Test_CodeAction(t *testing.T) { + ctx := t.Context() + fileURI, err := uri.Parse("file:///tmp/user.thrift") + require.NoError(t, err) + + tests := []struct { + name string + content string + context protocol.CodeActionContext + want map[string]protocol.CodeActionKind // title -> kind + }{ + { + // Already formatted: only the enum rewrite applies. + name: "enum refactor", + content: "enum E { A, B = 1 }\n", + want: map[string]protocol.CodeActionKind{ + "Make enum values explicit": protocol.CodeActionKindRefactorRewrite, + }, + }, + { + // A reported diagnostic turns the enum refactor into the + // quickfix for it. + name: "enum quickfix", + content: "enum E { A, B = 1 }\n", + context: protocol.CodeActionContext{Diagnostics: []protocol.Diagnostic{{ + Range: protocol.Range{Start: protocol.Position{Character: 10}, End: protocol.Position{Character: 11}}, + Message: protocol.String("A has no explicit value (implicitly 0)"), + }}}, + want: map[string]protocol.CodeActionKind{ + "Make enum values explicit": protocol.CodeActionKindQuickFix, + }, + }, + { + name: "only quickfix", + content: "enum E { A, B = 1 }\n", + context: protocol.CodeActionContext{ + Diagnostics: []protocol.Diagnostic{{ + Range: protocol.Range{Start: protocol.Position{Character: 10}, End: protocol.Position{Character: 11}}, + Message: protocol.String("A has no enum value (implicitly 0)"), + }}, + Only: []protocol.CodeActionKind{protocol.CodeActionKindQuickFix}, + }, + want: map[string]protocol.CodeActionKind{ + "Make enum values explicit": protocol.CodeActionKindQuickFix, + }, + }, + { + name: "only refactor drops the quickfix", + content: "enum E { A, B = 1 }\n", + context: protocol.CodeActionContext{ + Diagnostics: []protocol.Diagnostic{{ + Range: protocol.Range{Start: protocol.Position{Character: 10}, End: protocol.Position{Character: 11}}, + Message: protocol.String("A has no enum value (implicitly 0)"), + }}, + Only: []protocol.CodeActionKind{protocol.CodeActionKindRefactorRewrite}, + }, + want: map[string]protocol.CodeActionKind{ + "Make enum values explicit": protocol.CodeActionKindRefactorRewrite, + }, + }, + { + name: "no applicable actions", + content: "enum E { A = 1, B = 2 }\n", + want: map[string]protocol.CodeActionKind{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newTestServer(nil) + + err := srv.DidOpen(ctx, &protocol.DidOpenTextDocumentParams{ + TextDocument: protocol.TextDocumentItem{ + URI: fileURI, + LanguageID: "thrift", + Version: 0, + Text: tt.content, + }, + }) + require.NoError(t, err) + + params := &protocol.CodeActionParams{ + TextDocument: protocol.TextDocumentIdentifier{URI: fileURI}, + Range: protocol.Range{Start: protocol.Position{Line: 0, Character: 10}, End: protocol.Position{Line: 0, Character: 10}}, + Context: tt.context, + } + + actions, err := srv.codeAction(ctx, params) + require.NoError(t, err) + + got := make(map[string]protocol.CodeActionKind) + for _, a := range actions { + ca, ok := a.(*protocol.CodeAction) + require.True(t, ok, "expected a code action, got %T", a) + require.NotNil(t, ca.Kind) + got[ca.Title] = *ca.Kind + } + + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/lsp/diag_new_include_test.go b/lsp/diag_new_include_test.go index 4839150..139d6fc 100644 --- a/lsp/diag_new_include_test.go +++ b/lsp/diag_new_include_test.go @@ -185,10 +185,15 @@ func Test_DiagIncludeCreatedFullSession(t *testing.T) { _, err := srv.Initialize(t.Context(), &protocol.InitializeParams{}) require.NoError(t, err) + // The watcher is registered on Initialized, not during the + // initialize handshake: the server must not send requests to the + // client before answering initialize. + require.NoError(t, srv.Initialized(t.Context(), &protocol.InitializedParams{})) + // The server must ask the client to watch thrift files; without it, // disk-created includes never reach the server. watchers := client.watchers() - require.NotEmpty(t, watchers, "file watcher must be registered at initialize") + require.NotEmpty(t, watchers, "file watcher must be registered on Initialized") assert.Contains(t, watchers, "**/*.thrift") openDocument(t, srv, aURI, aContent) diff --git a/lsp/format_range_server_test.go b/lsp/format_range_server_test.go index 0a40f01..6f18cf3 100644 --- a/lsp/format_range_server_test.go +++ b/lsp/format_range_server_test.go @@ -1,7 +1,6 @@ package lsp import ( - "strings" "testing" "github.com/stretchr/testify/assert" @@ -10,6 +9,7 @@ import ( "go.lsp.dev/uri" "github.com/karitham/thrift-ls/lsp/cache" + "github.com/karitham/thrift-ls/lsp/mapper" "github.com/karitham/thrift-ls/options" ) @@ -38,14 +38,12 @@ struct C { 3: i64 c } }, })) - // applyEdits applies the edits to the given text. + // applyEdits applies the edits to the given text via the mapper. applyEdits := func(text string, edits []protocol.TextEdit) string { - out := text - for _, e := range edits { - out = applyEdit(out, e) - } + got, err := mapper.NewMapper(fileURI, []byte(text)).ApplyEdits(edits) + require.NoError(t, err) - return out + return string(got) } formatting := func() string { @@ -157,25 +155,3 @@ struct D { assert.Equal(t, "struct A { 1: string a }\n\nstruct D { 4: i64 d }\n", applyEdits(unsaved, edits)) }) } - -// applyEdit applies a single text edit to text, resolving the range's -// line/character positions to byte offsets. -func applyEdit(text string, edit protocol.TextEdit) string { - start := offsetAt(text, edit.Range.Start) - end := offsetAt(text, edit.Range.End) - - return text[:start] + edit.NewText + text[end:] -} - -// offsetAt resolves a position to a byte offset within text. -func offsetAt(text string, pos protocol.Position) int { - offset := 0 - - for range pos.Line { - if i := strings.IndexByte(text[offset:], '\n'); i >= 0 { - offset += i + 1 - } - } - - return offset + int(pos.Character) -} diff --git a/lsp/impl_test.go b/lsp/impl_test.go index 4e3ee3e..d221343 100644 --- a/lsp/impl_test.go +++ b/lsp/impl_test.go @@ -472,9 +472,10 @@ func symbolNames(syms protocol.SymbolInformationSlice) []string { } // Test_InitializeDefersTheWorkspaceWalk pins the startup flow: initialize -// returns without blocking on the workspace, the walk runs asynchronously -// from initialize (not the Initialized notification), and registers every -// thrift file under the workspace folder. +// returns without touching the workspace (the client must have answered the +// handshake before the server sends anything), and the walk runs +// asynchronously from the Initialized notification, registering every thrift +// file under the workspace folder. func Test_InitializeDefersTheWorkspaceWalk(t *testing.T) { synctest.Test(t, func(t *testing.T) { dir := t.TempDir() @@ -491,6 +492,12 @@ func Test_InitializeDefersTheWorkspaceWalk(t *testing.T) { }) require.NoError(t, err) + // Nothing runs during the handshake: no views until the client + // sends Initialized. + require.Empty(t, srv.session.Views()) + + require.NoError(t, srv.Initialized(t.Context(), &protocol.InitializedParams{})) + synctest.Wait() // The walk registered the folder as a view and marked both files @@ -513,58 +520,6 @@ func Test_InitializeDefersTheWorkspaceWalk(t *testing.T) { }) } -// Test_CodeActionFormatDocument pins the format code action: an -// unformatted document yields a source.fixAll action with the full-document -// edit, and a formatted document yields no actions. -func Test_CodeActionFormatDocument(t *testing.T) { - tests := []struct { - name string - content string - want bool // whether an action is expected - }{ - {"unformatted document offers formatting", "struct S{\n1:i32 a\n}", true}, - {"formatted document offers nothing", "struct S { 1: i32 a }\n", false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - fileURI := uri.File("/tmp/format.thrift") - - srv := NewServer(cache.New(nil), nil, options.Patch{}) - require.NoError(t, srv.DidOpen(t.Context(), &protocol.DidOpenTextDocumentParams{ - TextDocument: protocol.TextDocumentItem{ - URI: fileURI, - LanguageID: "thrift", - Version: 0, - Text: tt.content, - }, - })) - - actions, err := srv.CodeAction(t.Context(), &protocol.CodeActionParams{ - TextDocument: protocol.TextDocumentIdentifier{URI: fileURI}, - }) - require.NoError(t, err) - - if !tt.want { - assert.Empty(t, actions) - - return - } - - require.Len(t, actions, 1) - action, ok := actions[0].(*protocol.CodeAction) - require.True(t, ok) - require.NotNil(t, action) - assert.Equal(t, protocol.CodeActionKindSourceFixAll, *action.Kind) - require.NotNil(t, action.Edit) - - edits := action.Edit.Changes[fileURI] - require.Len(t, edits, 1) - assert.Contains(t, edits[0].NewText, "struct S {") - }) - } -} - // Test_CompletionQualifiedType pins qualified type completion: in a type // position, typing an include name followed by a dot suggests the // include's types, qualified. diff --git a/lsp/initialize.go b/lsp/initialize.go index b33977c..2368004 100644 --- a/lsp/initialize.go +++ b/lsp/initialize.go @@ -56,20 +56,12 @@ func (s *Server) initialize(ctx context.Context, params *protocol.InitializePara } } - // Kick off the workspace walk immediately, off the request path, so - // the workspace is indexed by the time the client makes its first - // request. The walk is async (it parses every thrift file) and the - // once-guard keeps it from running twice. - s.workspaceWalkOnce.Do(func() { - go func() { - for _, folder := range s.folders { - s.walkFoldersThriftFile(folder) - } - }() - }) - - s.registerFileWatcher(ctx) - + // The workspace walk and the file watcher registration run on the + // Initialized notification, not here: the spec forbids the server from + // sending requests or notifications to the client before responding to + // initialize. Helix deadlocks on the registerCapability request during + // the handshake and discards (or stalls on) notifications from an + // uninitialized server. return initializeResult(), nil } @@ -244,7 +236,13 @@ func initializeResult() *protocol.InitializeResult { Label: new("thrift-ls"), }, CodeActionProvider: &protocol.CodeActionOptions{ - CodeActionKinds: []protocol.CodeActionKind{protocol.CodeActionKindSourceFixAll}, + // Keep in sync with the kinds codeAction returns: + // quickfix (fixes for reported diagnostics) and + // refactor.rewrite (rewrites at the selection). + CodeActionKinds: []protocol.CodeActionKind{ + protocol.CodeActionKindQuickFix, + protocol.CodeActionKindRefactorRewrite, + }, ResolveProvider: new(false), }, CodeLensProvider: &protocol.CodeLensOptions{ diff --git a/lsp/mapper/apply.go b/lsp/mapper/apply.go new file mode 100644 index 0000000..9f0d049 --- /dev/null +++ b/lsp/mapper/apply.go @@ -0,0 +1,60 @@ +package mapper + +import ( + "fmt" + "sort" + + "go.lsp.dev/protocol" + + "github.com/karitham/thrift-ls/lsp/types" +) + +// ApplyEdits returns the mapped content with the edits applied. Edits must +// not overlap; they may arrive in any order. Positions are resolved from +// LSP (UTF-16) coordinates via the mapper, and later edits apply first so +// earlier offsets stay valid. +func (m *Mapper) ApplyEdits(edits []protocol.TextEdit) ([]byte, error) { + type pending struct { + start, end int + text string + } + + all := make([]pending, 0, len(edits)) + + for _, e := range edits { + start, err := m.offsetAt(e.Range.Start) + if err != nil { + return nil, err + } + + end, err := m.offsetAt(e.Range.End) + if err != nil { + return nil, err + } + + all = append(all, pending{start, end, e.NewText}) + } + + // Later edits apply first so earlier offsets stay valid. + sort.Slice(all, func(i, j int) bool { return all[i].start > all[j].start }) + + buf := m.content + + for _, e := range all { + if e.start < 0 || e.end > len(buf) || e.start > e.end { + return nil, fmt.Errorf("invalid edit range [%d:%d], total content: %d", e.start, e.end, len(buf)) + } + + buf = append(append(buf[:e.start:e.start], e.text...), buf[e.end:]...) + } + + return buf, nil +} + +// offsetAt resolves an LSP (UTF-16) position to a byte offset in the mapped +// content. +func (m *Mapper) offsetAt(pos protocol.Position) (int, error) { + p, err := m.LSPPosToParserPosition(types.Position{Line: pos.Line, Character: pos.Character}) + + return p.Offset, err +} diff --git a/lsp/server.go b/lsp/server.go index 3e28527..39dc7e1 100644 --- a/lsp/server.go +++ b/lsp/server.go @@ -98,11 +98,22 @@ func (s *Server) Initialize(ctx context.Context, params *protocol.InitializePara } func (s *Server) Initialized(ctx context.Context, params *protocol.InitializedParams) (err error) { - // The workspace walk starts at the end of initialize, not here: this - // method is a notification, which is fire-and-forget, while initialize - // is a request — a client that drops the notification (or never sends - // it) would otherwise leave the workspace unindexed until the first - // edit. + // The workspace walk and the file watcher registration run here, not + // in initialize: the client only sends Initialized after receiving + // the initialize response, so nothing the server emits at this + // point races the handshake. Sending the registerCapability request + // or diagnostics any earlier violates the spec — Helix deadlocks on + // a client request that arrives before initialize is answered. + s.workspaceWalkOnce.Do(func() { + go func() { + for _, folder := range s.folders { + s.walkFoldersThriftFile(folder) + } + }() + }) + + s.registerFileWatcher(ctx) + return nil } diff --git a/lsp/source/diagnostic.go b/lsp/source/diagnostic.go index 293c4df..1686bdc 100644 --- a/lsp/source/diagnostic.go +++ b/lsp/source/diagnostic.go @@ -20,6 +20,7 @@ func init() { &CycleCheck{}, &Parse{}, &FieldIDCheck{}, + &EnumValueCheck{}, &SemanticAnalysis{}, } } diff --git a/lsp/source/enum_value_action.go b/lsp/source/enum_value_action.go new file mode 100644 index 0000000..f6e28e5 --- /dev/null +++ b/lsp/source/enum_value_action.go @@ -0,0 +1,88 @@ +package source + +import ( + "context" + "strconv" + + "go.lsp.dev/protocol" + "go.lsp.dev/uri" + + "github.com/karitham/thrift-ls/lsp/cache" + "github.com/karitham/thrift-ls/syntax" +) + +// MakeEnumValuesExplicitAction returns the code action that appends an +// explicit value to every member of the enum under rng, mirroring the +// auto-incremented constants the compiler would assign: 0 for the first +// member, one greater than the preceding member's value otherwise. +// +// It returns nil when the selection is outside every enum, the enum is +// already fully explicit, the implicit values cannot be computed (an +// unparseable explicit constant), or the document has parse errors. +func MakeEnumValuesExplicitAction(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, rng protocol.Range) (*protocol.CodeAction, error) { + pf, err := ss.Parse(ctx, fh.URI()) + if err != nil { + return nil, err + } + + if pf.AST() == nil || len(pf.Errors()) > 0 { + return nil, nil + } + + enum := enumAt(pf, rng) + if enum == nil { + return nil, nil + } + + edits, ok := enumValueEdits(pf, enum) + if !ok || len(edits) == 0 { + return nil, nil + } + + return &protocol.CodeAction{ + Title: "Make enum values explicit", + Kind: new(protocol.CodeActionKindRefactorRewrite), + Edit: &protocol.WorkspaceEdit{ + Changes: map[uri.URI][]protocol.TextEdit{ + fh.URI(): edits, + }, + }, + }, nil +} + +// enumAt returns the enum declaration containing the selection start, or +// nil when it lies outside every enum. +func enumAt(pf *cache.ParsedFile, rng protocol.Range) *syntax.Enum { + pos, err := pf.Mapper().LSPPosToParserPosition(lspPosition(rng.Start)) + if err != nil { + return nil + } + + for _, enum := range pf.AST().Enums() { + if pf.AST().Contains(enum, pos) { + return enum + } + } + + return nil +} + +// enumValueEdits appends " = N" to every member without an explicit value. +// ok is false when the implicit values cannot be computed; the caller must +// then not edit the enum, as the inserted values would be wrong. +func enumValueEdits(pf *cache.ParsedFile, enum *syntax.Enum) (edits []protocol.TextEdit, ok bool) { + for _, im := range enumImplicitValues(enum) { + if !im.known { + return nil, false + } + + insertAt := pf.AST().TokenEndPosition(im.member.Name.TokStart()) + + edits = append(edits, protocol.TextEdit{ + Range: toLSPRange(pf, insertAt, insertAt), + NewText: " = " + strconv.FormatInt(im.value, 10), + }) + } + + return edits, true +} diff --git a/lsp/source/enum_value_action_test.go b/lsp/source/enum_value_action_test.go new file mode 100644 index 0000000..0d077da --- /dev/null +++ b/lsp/source/enum_value_action_test.go @@ -0,0 +1,127 @@ +package source + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.lsp.dev/protocol" + + "github.com/karitham/thrift-ls/lsp/cache" + "github.com/karitham/thrift-ls/lsp/mapper" +) + +func Test_MakeEnumValuesExplicitAction(t *testing.T) { + tests := []struct { + name string + content string + rng protocol.Range + want string // resulting content; empty means no action + }{ + { + name: "fills implicit member values", + content: `enum Color { + RED, + GREEN = 2, + BLUE, + ALPHA = 0x10, + OMEGA, +} +`, + rng: pointRange(1, 2), + want: `enum Color { + RED = 0, + GREEN = 2, + BLUE = 3, + ALPHA = 0x10, + OMEGA = 17, +} +`, + }, + { + name: "fully explicit enum is a no-op", + content: "enum E {\n A = 1,\n B = 2\n}\n", + rng: pointRange(1, 2), + want: "", + }, + { + name: "selection outside every enum is a no-op", + content: `struct S { + 1: i32 a, +} + +enum E { + A, +} +`, + rng: pointRange(1, 5), + want: "", + }, + { + name: "unparseable precedent is a no-op", + content: "enum E {\n A = 08,\n B\n}\n", + rng: pointRange(2, 2), + want: "", + }, + { + name: "parse errors are a no-op", + content: "enum E {\n A,\n", + rng: pointRange(1, 2), + want: "", + }, + { + name: "only the enum under the cursor is edited", + content: `enum A { + X, +} + +enum B { + Y, +} +`, + rng: pointRange(5, 2), + want: "enum A {\n X,\n}\n\nenum B {\n Y = 0,\n}\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ss := buildSnapshotForTest(t, []*cache.FileChange{ + { + URI: "file:///tmp/user.thrift", + Version: 0, + Content: []byte(tt.content), + From: cache.FileChangeTypeDidOpen, + }, + }) + + fh, err := ss.ReadFile(t.Context(), "file:///tmp/user.thrift") + require.NoError(t, err) + + act, err := MakeEnumValuesExplicitAction(t.Context(), ss, fh, tt.rng) + require.NoError(t, err) + + if tt.want == "" { + assert.Nil(t, act) + return + } + + require.NotNil(t, act) + assert.Equal(t, "Make enum values explicit", act.Title) + assert.Equal(t, protocol.CodeActionKindRefactorRewrite, *act.Kind) + + edits := act.Edit.Changes["file:///tmp/user.thrift"] + + got, err := mapper.NewMapper("file:///tmp/user.thrift", []byte(tt.content)).ApplyEdits(edits) + require.NoError(t, err) + assert.Equal(t, tt.want, string(got)) + }) + } +} + +// pointRange is a collapsed selection: the cursor at line/col, 0-based. +func pointRange(line, character uint32) protocol.Range { + pos := protocol.Position{Line: line, Character: character} + + return protocol.Range{Start: pos, End: pos} +} diff --git a/lsp/source/enum_value_check.go b/lsp/source/enum_value_check.go new file mode 100644 index 0000000..cc5ecc0 --- /dev/null +++ b/lsp/source/enum_value_check.go @@ -0,0 +1,124 @@ +package source + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strconv" + + "go.lsp.dev/protocol" + "go.lsp.dev/uri" + + "github.com/karitham/thrift-ls/lsp/cache" + "github.com/karitham/thrift-ls/syntax" +) + +type EnumValueCheck struct{} + +// EnumValueCheck warns on enum members that lack an explicit value. +// +// The compiler auto-increments implicit members: 0 for the first member, +// one greater than the preceding member's value otherwise. Their on-wire +// value therefore follows their position; inserting, removing, or +// reordering members silently changes serialized data. +func (c *EnumValueCheck) Diagnostic(ctx context.Context, ss *cache.Snapshot, changeFiles []uri.URI) (DiagnosticResult, error) { + res := make(DiagnosticResult) + + for _, file := range changeFiles { + items, err := c.diagnostic(ctx, ss, file) + if err != nil { + return nil, err + } + + res[file] = items + } + + return res, nil +} + +func (c *EnumValueCheck) Name() string { + return "EnumValueCheck" +} + +func (c *EnumValueCheck) diagnostic(ctx context.Context, ss *cache.Snapshot, file uri.URI) ([]protocol.Diagnostic, error) { + pf, err := ss.Parse(ctx, file) + if err != nil { + return nil, err + } + + if pf.AST() == nil { + return nil, errors.New("parse ast failed") + } + + for _, err := range pf.Errors() { + slog.Debug("parse failed", "err", err) + } + + var ret []protocol.Diagnostic + + for _, enum := range pf.AST().Enums() { + for _, im := range enumImplicitValues(enum) { + msg := fmt.Sprintf("%s has no explicit value", im.member.Name.Text) + if im.known { + msg = fmt.Sprintf("%s has no explicit value (implicitly %d)", im.member.Name.Text, im.value) + } + + ret = append(ret, protocol.Diagnostic{ + Range: tokenRange(pf, enumValueNameToken(pf, im.member)), + Severity: protocol.DiagnosticSeverityWarning, + Source: protocol.NewOptional("thrift-ls"), + Message: protocol.String(msg), + }) + } + } + + return ret, nil +} + +// enumValueNameToken returns the name token of an enum member. +func enumValueNameToken(pf *cache.ParsedFile, v *syntax.EnumValue) *syntax.Token { + return &pf.AST().Tokens[v.Name.TokStart()] +} + +// enumImplicitValue is an enum member that lacks an explicit value, with +// the int constant the compiler auto-increments for it. +type enumImplicitValue struct { + member *syntax.EnumValue + value int64 + known bool // false when the preceding value is broken, so value is unknowable +} + +// enumImplicitValues reports the members of an enum that carry no explicit +// value, together with the value the compiler would auto-increment: 0 for +// the first member, one greater than the preceding member's value +// otherwise. Members after an unparseable explicit constant report +// known=false until the next parseable constant settles the chain. +func enumImplicitValues(enum *syntax.Enum) []enumImplicitValue { + var out []enumImplicitValue + + // A virtual value of -1 precedes the first member so the first + // implicit member auto-increments to 0, mirroring the compiler. + val, known := int64(-1), true + + for _, member := range enum.Values { + if member.Value == nil { + im := enumImplicitValue{member: member, known: known} + if known { + val++ + im.value = val + } + out = append(out, im) + continue + } + + v, err := strconv.ParseInt(member.Value.Text, 0, 64) + if err != nil { + known = false + continue + } + val, known = v, true + } + + return out +} diff --git a/lsp/source/enum_value_check_test.go b/lsp/source/enum_value_check_test.go new file mode 100644 index 0000000..15b5ff7 --- /dev/null +++ b/lsp/source/enum_value_check_test.go @@ -0,0 +1,106 @@ +package source + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.lsp.dev/protocol" + "go.lsp.dev/uri" + + "github.com/karitham/thrift-ls/lsp/cache" +) + +func Test_EnumValueCheck_Diagnostic(t *testing.T) { + tests := []struct { + name string + content string + want []protocol.Diagnostic + }{ + { + name: "explicit values only", + content: `enum Color { + RED = 0, + GREEN = 2, +} +`, + want: nil, + }, + { + name: "implicit members auto-increment", + content: `enum Color { + RED, + GREEN = 2, + BLUE, + ALPHA = 0x10, + OMEGA, +} +`, + want: []protocol.Diagnostic{ + { + Range: protocol.Range{ + Start: protocol.Position{Line: 1, Character: 2}, + End: protocol.Position{Line: 1, Character: 5}, + }, + Severity: protocol.DiagnosticSeverityWarning, + Source: protocol.NewOptional("thrift-ls"), + Message: protocol.String("RED has no explicit value (implicitly 0)"), + }, + { + Range: protocol.Range{ + Start: protocol.Position{Line: 3, Character: 2}, + End: protocol.Position{Line: 3, Character: 6}, + }, + Severity: protocol.DiagnosticSeverityWarning, + Source: protocol.NewOptional("thrift-ls"), + Message: protocol.String("BLUE has no explicit value (implicitly 3)"), + }, + { + Range: protocol.Range{ + Start: protocol.Position{Line: 5, Character: 2}, + End: protocol.Position{Line: 5, Character: 7}, + }, + Severity: protocol.DiagnosticSeverityWarning, + Source: protocol.NewOptional("thrift-ls"), + Message: protocol.String("OMEGA has no explicit value (implicitly 17)"), + }, + }, + }, + { + name: "unparseable explicit value breaks the chain", + content: `enum E { + A = 08, + B, +} +`, + want: []protocol.Diagnostic{ + { + Range: protocol.Range{ + Start: protocol.Position{Line: 2, Character: 2}, + End: protocol.Position{Line: 2, Character: 3}, + }, + Severity: protocol.DiagnosticSeverityWarning, + Source: protocol.NewOptional("thrift-ls"), + Message: protocol.String("B has no explicit value"), + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ss := buildSnapshotForTest(t, []*cache.FileChange{ + { + URI: "file:///tmp/user.thrift", + Version: 0, + Content: []byte(tt.content), + From: cache.FileChangeTypeDidOpen, + }, + }) + + got, err := (&EnumValueCheck{}).Diagnostic(t.Context(), ss, []uri.URI{"file:///tmp/user.thrift"}) + assert.NoError(t, err) + + assert.Equal(t, DiagnosticResult{"file:///tmp/user.thrift": tt.want}, got) + }) + } +} diff --git a/lsp/source/format.go b/lsp/source/format.go index bedb8f4..e7d6fcc 100644 --- a/lsp/source/format.go +++ b/lsp/source/format.go @@ -5,7 +5,6 @@ import ( "context" "go.lsp.dev/protocol" - "go.lsp.dev/uri" "github.com/karitham/thrift-ls/formatter" "github.com/karitham/thrift-ls/lsp/cache" @@ -129,32 +128,6 @@ func FormatRange(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, o return result, nil } -// FormatDocumentAction returns the source.fixAll code action that formats -// the document, mirroring the formatting request. It returns nil when the -// document is already formatted. -func FormatDocumentAction(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, opts formatter.Options) (*protocol.CodeAction, error) { - edit, err := FormatDocument(ctx, ss, fh, opts) - if err != nil { - return nil, err - } - - if edit == nil { - return nil, nil - } - - file := fh.URI() - - return &protocol.CodeAction{ - Title: "Format document", - Kind: new(protocol.CodeActionKindSourceFixAll), - Edit: &protocol.WorkspaceEdit{ - Changes: map[uri.URI][]protocol.TextEdit{ - file: {*edit}, - }, - }, - }, nil -} - // lspPosition converts a protocol position to the internal position type. func lspPosition(p protocol.Position) types.Position { return types.Position{