diff --git a/lsp/cache/context.go b/lsp/cache/context.go index 8b2d39f..630c877 100644 --- a/lsp/cache/context.go +++ b/lsp/cache/context.go @@ -22,22 +22,12 @@ func NewIncludeDeps() *IncludeDeps { // Includes returns the files file includes directly, sorted ascending by URI. func (c *IncludeDeps) Includes(file uri.URI) []uri.URI { - node := c.graph.Get(file) - if node == nil { - return nil - } - - return node.OutDegree() + return c.graph.Includes(file) } // Includers returns the files that include file directly, in graph order. func (c *IncludeDeps) Includers(file uri.URI) []uri.URI { - node := c.graph.Get(file) - if node == nil { - return nil - } - - return node.InDegree() + return c.graph.Includers(file) } // Register replaces file's include edges, resolving them via resolve the same @@ -58,34 +48,7 @@ func (c *IncludeDeps) Register(file uri.URI, includes []*syntax.Include, resolve // including file itself when it transitively includes itself. The result is // sorted ascending by URI and cycle-safe. func (c *IncludeDeps) Dependents(file uri.URI) []uri.URI { - deps := make([]uri.URI, 0) - seen := make(map[uri.URI]struct{}) - - var walk func(f uri.URI) - - walk = func(f uri.URI) { - node := c.graph.Get(f) - if node == nil { - return - } - - for _, dependent := range node.InDegree() { - if _, ok := seen[dependent]; ok { - continue - } - - seen[dependent] = struct{}{} - deps = append(deps, dependent) - - walk(dependent) - } - } - - walk(file) - - slices.Sort(deps) - - return deps + return c.graph.Dependents(file) } // Forget removes file's edges and returns its former dependents. diff --git a/lsp/cache/graph.go b/lsp/cache/graph.go index a8764e9..8d403e2 100644 --- a/lsp/cache/graph.go +++ b/lsp/cache/graph.go @@ -1,6 +1,7 @@ package cache import ( + "slices" "sort" "strings" "sync" @@ -55,8 +56,78 @@ func NewIncludeGraph() *IncludeGraph { } } +// Includes returns the files file includes directly, sorted ascending by +// URI. The read lock is held during the walk, so no node clones are +// needed. +func (g *IncludeGraph) Includes(file uri.URI) []uri.URI { + g.mu.RLock() + defer g.mu.RUnlock() + + return g.includesLocked(file) +} + +// Includers returns the files that include file directly, in graph order. +func (g *IncludeGraph) Includers(file uri.URI) []uri.URI { + g.mu.RLock() + defer g.mu.RUnlock() + + return g.includersLocked(file) +} + +// Dependents returns every file that directly or transitively includes +// file, including file itself when it transitively includes itself. The +// result is sorted ascending by URI and cycle-safe. +func (g *IncludeGraph) Dependents(file uri.URI) []uri.URI { + g.mu.RLock() + defer g.mu.RUnlock() + + deps := make([]uri.URI, 0) + seen := make(map[uri.URI]struct{}) + + var walk func(f uri.URI) + + walk = func(f uri.URI) { + for _, dependent := range g.includersLocked(f) { + if _, ok := seen[dependent]; ok { + continue + } + + seen[dependent] = struct{}{} + deps = append(deps, dependent) + walk(dependent) + } + } + + walk(file) + slices.Sort(deps) + + return deps +} + +// includesLocked returns file's direct includes; callers must hold g.mu. +func (g *IncludeGraph) includesLocked(file uri.URI) []uri.URI { + node := g.mapper[file] + if node == nil { + return nil + } + + return node.outdegree +} + +// includersLocked returns file's direct includers; callers must hold g.mu. +func (g *IncludeGraph) includersLocked(file uri.URI) []uri.URI { + node := g.mapper[file] + if node == nil { + return nil + } + + return node.indegree +} + // Get returns a copy of file's node. The graph's nodes are mutated in place // by Set and removeWithoutLock, so a live node must never escape the lock. +// Prefer the locked walk methods (Includes, Includers, Dependents), which +// avoid the copy. func (g *IncludeGraph) Get(file uri.URI) *IncludeNode { g.mu.RLock() defer g.mu.RUnlock() diff --git a/lsp/cache/view.go b/lsp/cache/view.go index 8bebb7b..de7b27d 100644 --- a/lsp/cache/view.go +++ b/lsp/cache/view.go @@ -164,7 +164,7 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. for _, uri := range uris { if _, err := newSnapshot.Parse(ctx, uri); err != nil { - slog.Error("parse error", "err", err) + slog.Warn("parse error", "err", err) } } diff --git a/lsp/diagnostic.go b/lsp/diagnostic.go index ace3420..a5fcf3f 100644 --- a/lsp/diagnostic.go +++ b/lsp/diagnostic.go @@ -24,7 +24,7 @@ func (s *Server) diagnostic(ctx context.Context, ss *cache.Snapshot, file uri.UR diagRes, err := diag.Diagnostic(ctx, ss, []uri.URI{file}) if err != nil { - slog.Error("diagnostic failed", "err", err) + logError("diagnostic failed", err) } slog.Debug("publish diagnostic result", "count", len(diagRes)) diff --git a/lsp/errors.go b/lsp/errors.go new file mode 100644 index 0000000..aa8276b --- /dev/null +++ b/lsp/errors.go @@ -0,0 +1,44 @@ +package lsp + +import ( + "errors" + "log/slog" +) + +// expectedError marks an error as part of normal operation: invalid client +// settings, a rejected config file. Logging treats it as a warning, so +// expected failures do not surface as errors in the client's log channel. +type expectedError struct{ err error } + +func (e *expectedError) Error() string { return e.err.Error() } +func (e *expectedError) Unwrap() error { return e.err } + +// Expected wraps err as expected; nil stays nil. +func Expected(err error) error { + if err == nil { + return nil + } + + return &expectedError{err} +} + +// logError logs err: at warning level when expected (or wrapping an +// expected error), error otherwise. msg describes the operation; args are +// the slog key-value pairs. +func logError(msg string, err error, args ...any) { + args = append(args, "err", err) + + if expected(err) { + slog.Warn(msg, args...) + + return + } + + slog.Error(msg, args...) +} + +func expected(err error) bool { + var e *expectedError + + return errors.As(err, &e) +} diff --git a/lsp/impl.go b/lsp/impl.go index 415a4fe..ea259b7 100644 --- a/lsp/impl.go +++ b/lsp/impl.go @@ -3,7 +3,6 @@ package lsp import ( "context" "fmt" - "log/slog" "path" "strings" @@ -184,7 +183,7 @@ func (s *Server) postDiagnostics(ctx context.Context, view *cache.View) func([]u func (s *Server) diagnose(ctx context.Context, ss *cache.Snapshot, affected []uri.URI) { for i := range affected { if err := s.diagnostic(ctx, ss, affected[i]); err != nil { - slog.Error("diagnostic error", "err", err) + logError("diagnostic error", err) } } } diff --git a/lsp/initialize.go b/lsp/initialize.go index c466861..64e7fdf 100644 --- a/lsp/initialize.go +++ b/lsp/initialize.go @@ -48,7 +48,7 @@ func (s *Server) initialize(params *protocol.InitializeParams) (result *protocol // config; didChangeConfiguration updates them later. if len(params.InitializationOptions) > 0 { if patch, err := lspSettings(params.InitializationOptions); err != nil { - slog.Error("initializationOptions rejected", "err", err) + logError("initializationOptions rejected", err) } else { s.setWorkspaceSettings(*patch) } @@ -128,7 +128,7 @@ func (s *Server) walkFoldersThriftFile(folder uri.URI) { Content: []byte{}, From: cache.FileChangeTypeInitialize, }); err != nil { - slog.Error("openFile failed", "err", err) + slog.Warn("openFile failed", "err", err) } // always return nil to continue parse diff --git a/lsp/server.go b/lsp/server.go index 5074d0c..1d5ffb0 100644 --- a/lsp/server.go +++ b/lsp/server.go @@ -73,7 +73,7 @@ func NewServer(c *cache.Cache, client protocol.Client, opts Options) *Server { // settings are rejected and the previous document stays in effect. func (s *Server) setWorkspaceSettings(overlay options.Patch) { if _, err := overlay.Formatter(); err != nil { - slog.Error("workspace settings rejected", "err", err) + logError("workspace settings rejected", err) return } @@ -109,7 +109,7 @@ func (s *Server) viewConfig(folder uri.URI) options.Patch { cfgPath, err := options.FindConfig(folder.FsPath()) if err != nil { - slog.Error("config discovery failed", "dir", folder.FsPath(), "err", err) + logError("config discovery failed", Expected(err), "dir", folder.FsPath()) return s.defaultConfig() } @@ -120,7 +120,7 @@ func (s *Server) viewConfig(folder uri.URI) options.Patch { cfg, err := options.Load(cfgPath) if err != nil { - slog.Error("config file rejected", "path", cfgPath, "err", err) + logError("config file rejected", Expected(err), "path", cfgPath) return s.defaultConfig() } @@ -161,7 +161,7 @@ func (s *Server) formatOptions(view *cache.View) formatter.Options { if err != nil { // Both layers were validated when stored; this is unreachable // unless a view config was corrupted. - slog.Error("formatter options rejected", "err", err) + logError("formatter options rejected", err) fopts, _ = view.Config().Formatter() } @@ -249,7 +249,16 @@ func (s *Server) CompletionResolve(ctx context.Context, params *protocol.Complet } func (s *Server) Declaration(ctx context.Context, params *protocol.DeclarationParams) (result protocol.DeclarationResult, err error) { - return protocol.LocationSlice{}, nil + // Thrift has no separate declaration concept: a declaration is the + // definition. + res, err := s.definition(ctx, &protocol.DefinitionParams{ + TextDocumentPositionParams: params.TextDocumentPositionParams, + }) + if err != nil { + return nil, err + } + + return protocol.LocationSlice(res), nil } func (s *Server) Definition(ctx context.Context, params *protocol.DefinitionParams) (result protocol.DefinitionResult, err error) { @@ -275,7 +284,7 @@ func (s *Server) DidChangeConfiguration(ctx context.Context, params *protocol.Di patch, err := lspSettings(params.Settings) if err != nil { - slog.Error("didChangeConfiguration rejected", "err", err) + logError("didChangeConfiguration rejected", err) return nil } @@ -365,7 +374,9 @@ func (s *Server) Implementation(ctx context.Context, params *protocol.Implementa } func (s *Server) OnTypeFormatting(ctx context.Context, params *protocol.DocumentOnTypeFormattingParams) (result []protocol.TextEdit, err error) { - return []protocol.TextEdit{}, nil + return withFile(ctx, s.session, params.TextDocument.URI, func(ss *cache.Snapshot, fh cache.FileHandle) ([]protocol.TextEdit, error) { + return source.OnTypeFormat(ctx, ss, fh, s.formatOptions(ss.View()), params.Position) + }) } func (s *Server) PrepareRename(ctx context.Context, params *protocol.PrepareRenameParams) (result protocol.PrepareRenameResult, err error) { diff --git a/lsp/settings.go b/lsp/settings.go index 537444e..dc2f198 100644 --- a/lsp/settings.go +++ b/lsp/settings.go @@ -11,18 +11,25 @@ import ( // (initializationOptions or the settings of didChangeConfiguration) into an // options patch. The `path` extension setting is not an options key and is // dropped; unknown keys are rejected so typos fail loudly instead of -// formatting with silently stale options. +// formatting with silently stale options. Parse failures are expected +// errors: the document is client input, and the previous settings stay in +// effect. func lspSettings(data []byte) (*options.Patch, error) { var m map[string]any if err := json.Unmarshal(data, &m); err != nil { - return nil, fmt.Errorf("lsp settings: %w", err) + return nil, Expected(fmt.Errorf("lsp settings: %w", err)) } delete(m, "path") clean, err := json.Marshal(m) if err != nil { - return nil, fmt.Errorf("lsp settings: %w", err) + return nil, Expected(fmt.Errorf("lsp settings: %w", err)) } - return options.Parse(clean) + p, err := options.Parse(clean) + if err != nil { + return nil, Expected(err) + } + + return p, nil } diff --git a/lsp/source/cycle_detect.go b/lsp/source/cycle_detect.go index ee1bfe3..3d8b6d1 100644 --- a/lsp/source/cycle_detect.go +++ b/lsp/source/cycle_detect.go @@ -108,15 +108,15 @@ func cycleDetect(includesMap map[uri.URI][]Include) []CyclePair { func getIncludes(ctx context.Context, ss *cache.Snapshot, file uri.URI, includesMap *map[uri.URI][]Include) error { pf, err := ss.Parse(ctx, file) if err != nil { - slog.Error("parse failed", "file", file, "err", err) - return err } if pf.AST() == nil { - slog.Error("parse ast failed", "errs", pf.AggregatedError()) + // The file does not parse; the Parse checker reports that. Cycle + // detection just skips it — its include edges are unknown. + slog.Debug("cycle check skipped: file does not parse", "file", file) - return pf.AggregatedError() + return nil } includes := pf.AST().Includes() diff --git a/lsp/source/format.go b/lsp/source/format.go index f28adf3..f24c148 100644 --- a/lsp/source/format.go +++ b/lsp/source/format.go @@ -3,6 +3,7 @@ package source import ( "bytes" "context" + "errors" "go.lsp.dev/protocol" @@ -11,6 +12,12 @@ import ( "github.com/karitham/thrift-ls/lsp/mapper" ) +// ErrNotParseable is returned by Format when the document has parse +// errors: it cannot be formatted safely. The Parse checker reports the +// errors to the client, so callers skip formatting (nil edits) instead of +// failing the request. +var ErrNotParseable = errors.New("document does not parse") + // Format returns the whole-document formatting of fh's content. func Format(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, opts formatter.Options) (string, error) { pf, err := ss.Parse(ctx, fh.URI()) @@ -19,7 +26,7 @@ func Format(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, opts f } if len(pf.Errors()) > 0 || pf.AST() == nil { - return "", pf.AggregatedError() + return "", ErrNotParseable } return formatter.Format(pf.AST(), opts) @@ -35,6 +42,9 @@ func FormatDocument(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle } formatted, err := Format(ctx, ss, fh, opts) + if errors.Is(err, ErrNotParseable) { + return nil, nil // the Parse checker reports the errors + } if err != nil { return nil, err } @@ -73,6 +83,9 @@ func FormatRange(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, o } formatted, err := Format(ctx, ss, fh, opts) + if errors.Is(err, ErrNotParseable) { + return nil, nil // the Parse checker reports the errors + } if err != nil { return nil, err } @@ -260,3 +273,44 @@ func lineEnd(content []byte, offset int) int { return len(content) } + +// OnTypeFormat formats the construct whose closing delimiter was just +// typed: the whole struct/union/exception/enum/service block reflows. A +// document that does not parse, or a position outside any construct, +// formats nothing. +func OnTypeFormat(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, opts formatter.Options, pos protocol.Position) ([]protocol.TextEdit, error) { + pf, err := ss.Parse(ctx, fh.URI()) + if err != nil || pf.AST() == nil { + return nil, nil + } + + rng := enclosingConstruct(pf, pos) + if rng == nil { + return nil, nil + } + + return FormatRange(ctx, ss, fh, opts, *rng) +} + +// enclosingConstruct returns the range of the top-level construct +// containing pos — definitions never overlap, so at most one matches. +func enclosingConstruct(pf *cache.ParsedFile, pos protocol.Position) *protocol.Range { + for _, n := range pf.AST().Nodes { + rng := nodeRange(pf, n) + + if rangeContains(rng, pos) { + return &rng + } + } + + return nil +} + +// rangeContains reports whether pos lies within rng, inclusive on the end +// (the typed closing delimiter sits on the construct's last character). +func rangeContains(rng protocol.Range, pos protocol.Position) bool { + afterStart := pos.Line > rng.Start.Line || (pos.Line == rng.Start.Line && pos.Character >= rng.Start.Character) + beforeEnd := pos.Line < rng.End.Line || (pos.Line == rng.End.Line && pos.Character <= rng.End.Character) + + return afterStart && beforeEnd +} diff --git a/lsp/source/index.go b/lsp/source/index.go index ebd11d0..4791b0e 100644 --- a/lsp/source/index.go +++ b/lsp/source/index.go @@ -38,7 +38,7 @@ func parseDefinitionFile(ctx context.Context, ss *cache.Snapshot, file uri.URI) } if len(pf.Errors()) > 0 { - slog.Error("parse error", "errs", pf.Errors()) + slog.Warn("parse error", "errs", pf.Errors()) } if pf.AST() == nil { diff --git a/lsp/source/on_type_format_test.go b/lsp/source/on_type_format_test.go new file mode 100644 index 0000000..2de73f8 --- /dev/null +++ b/lsp/source/on_type_format_test.go @@ -0,0 +1,81 @@ +package source + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.lsp.dev/protocol" + + "github.com/karitham/thrift-ls/formatter" + "github.com/karitham/thrift-ls/lsp/cache" +) + +// TestOnTypeFormat verifies that typing a closing brace formats the +// enclosing construct. +func TestOnTypeFormat(t *testing.T) { + src := "struct S {1: i32 a,2: string b}" + // The closing brace was just typed at the end of the document. + pos := protocol.Position{Line: 0, Character: uint32(len(src) - 1)} + + ss := cache.BuildSnapshotForTest([]*cache.FileChange{ + { + URI: "file:///tmp/f.thrift", + Version: 0, + Content: []byte(src), + From: cache.FileChangeTypeDidOpen, + }, + }) + fh, err := ss.ReadFile(t.Context(), "file:///tmp/f.thrift") + require.NoError(t, err) + + edits, err := OnTypeFormat(t.Context(), ss, fh, formatter.DefaultOptions(), pos) + require.NoError(t, err) + require.Len(t, edits, 1) + + assert.Equal(t, "struct S { 1: i32 a, 2: string b }\n", edits[0].NewText) +} + +// TestOnTypeFormatSkipsBrokenDocument verifies that on-type formatting of +// a document with parse errors formats nothing: the Parse checker reports +// the errors, and the request must not fail. +func TestOnTypeFormatSkipsBrokenDocument(t *testing.T) { + src := "struct S { 1: " + pos := protocol.Position{Line: 0, Character: uint32(len(src))} + + ss := cache.BuildSnapshotForTest([]*cache.FileChange{ + { + URI: "file:///tmp/f.thrift", + Version: 0, + Content: []byte(src), + From: cache.FileChangeTypeDidOpen, + }, + }) + fh, err := ss.ReadFile(t.Context(), "file:///tmp/f.thrift") + require.NoError(t, err) + + edits, err := OnTypeFormat(t.Context(), ss, fh, formatter.DefaultOptions(), pos) + require.NoError(t, err) + assert.Empty(t, edits) +} + +// TestFormatSkipsBrokenDocument verifies that whole-document formatting of +// a file with parse errors returns no edits instead of failing. +func TestFormatSkipsBrokenDocument(t *testing.T) { + src := "struct S { 1: " + + ss := cache.BuildSnapshotForTest([]*cache.FileChange{ + { + URI: "file:///tmp/f.thrift", + Version: 0, + Content: []byte(src), + From: cache.FileChangeTypeDidOpen, + }, + }) + fh, err := ss.ReadFile(t.Context(), "file:///tmp/f.thrift") + require.NoError(t, err) + + edit, err := FormatDocument(t.Context(), ss, fh, formatter.DefaultOptions()) + require.NoError(t, err) + assert.Nil(t, edit) +}