From bdeb7d5c779719346edce8c7818235dab4f89f66 Mon Sep 17 00:00:00 2001 From: karitham Date: Sat, 8 Aug 2026 21:19:30 +0200 Subject: [PATCH] lsp: parallel diagnostics, memoized index resolution --- lsp/impl.go | 20 +++- lsp/source/index.go | 98 ++++++++++++++- lsp/source/index_memo_test.go | 205 ++++++++++++++++++++++++++++++++ lsp/source/semantic_analysis.go | 48 ++++---- 4 files changed, 339 insertions(+), 32 deletions(-) create mode 100644 lsp/source/index_memo_test.go diff --git a/lsp/impl.go b/lsp/impl.go index ea259b7..2a8c334 100644 --- a/lsp/impl.go +++ b/lsp/impl.go @@ -5,6 +5,7 @@ import ( "fmt" "path" "strings" + "sync" "go.lsp.dev/uri" @@ -179,13 +180,22 @@ func (s *Server) postDiagnostics(ctx context.Context, view *cache.View) func([]u } } -// diagnose publishes diagnostics for every affected file. +// diagnose publishes diagnostics for every affected file, in parallel: a +// change to a shared include re-diagnoses all its dependents, and the +// snapshot (and the client connection) are safe for concurrent reads and +// notifications. 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 { - logError("diagnostic error", err) - } + var wg sync.WaitGroup + + for _, file := range affected { + wg.Go(func() { + if err := s.diagnostic(ctx, ss, file); err != nil { + logError("diagnostic error", err) + } + }) } + + wg.Wait() } func (s *Server) completion(ctx context.Context, params *protocol.CompletionParams) (*protocol.CompletionList, error) { diff --git a/lsp/source/index.go b/lsp/source/index.go index 4791b0e..fac96c6 100644 --- a/lsp/source/index.go +++ b/lsp/source/index.go @@ -18,9 +18,13 @@ import ( // resolution and reference search. It composes per-file // cache.FileIndexes over the include graph. // -// An Index is cheap — construct one per request with NewIndex. +// An Index is cheap — construct one per request with NewIndex. Resolutions +// are memoized per (file, name), so a request resolving the same name in +// the same file repeatedly (references, diagnostics) resolves it once. type Index struct { ss *cache.Snapshot + + resolved map[resolveKey]*Resolved } // NewIndex returns an Index for the snapshot. @@ -28,6 +32,40 @@ func NewIndex(ss *cache.Snapshot) *Index { return &Index{ss: ss} } +// resolveKey identifies one resolution: the referencing file, the name as +// written, and the resolver (type, value, or service). +type resolveKey struct { + file uri.URI + name string + kind resolveKind +} + +type resolveKind uint8 + +const ( + resolveType resolveKind = iota + 1 + resolveValue + resolveService +) + +// memoized returns the memoized resolution for (file, name, kind), if any. +func (x *Index) memoized(file uri.URI, name string, kind resolveKind) (*Resolved, bool) { + def, ok := x.resolved[resolveKey{file: file, name: name, kind: kind}] + + return def, ok +} + +// memoize records the resolution of (file, name, kind). A name resolves +// identically everywhere in the same file, so nil (unresolved) results are +// memoized too. +func (x *Index) memoize(file uri.URI, name string, kind resolveKind, def *Resolved) { + if x.resolved == nil { + x.resolved = make(map[resolveKey]*Resolved) + } + + x.resolved[resolveKey{file: file, name: name, kind: kind}] = def +} + // parseDefinitionFile parses the definition file, tolerating parse errors // in the target file (the definitions may still be found in the partial // AST). It returns the parsed file so callers can use its indexes. @@ -74,6 +112,22 @@ func (x *Index) ResolveType(ctx context.Context, from *cache.ParsedFile, ft *syn return nil, nil } + if def, ok := x.memoized(from.URI(), name, resolveType); ok { + return def, nil + } + + def, err := x.resolveType(ctx, from, name) + if err != nil { + return nil, err + } + + x.memoize(from.URI(), name, resolveType, def) + + return def, nil +} + +// resolveType resolves a non-basic type name in from, without memoization. +func (x *Index) resolveType(ctx context.Context, from *cache.ParsedFile, name string) (*Resolved, error) { _, identifier := parseIdent(from.URI(), from.AST().Includes(), name) for _, astFile := range definitionFiles(ctx, x.ss, from.URI(), from.AST(), name) { dst, err := parseDefinitionFile(ctx, x.ss, astFile) @@ -105,10 +159,27 @@ func (x *Index) ResolveValue(ctx context.Context, from *cache.ParsedFile, v *syn return nil, nil } - _, identifier := parseIdent(from.URI(), from.AST().Includes(), v.Text) + if def, ok := x.memoized(from.URI(), v.Text, resolveValue); ok { + return def, nil + } + + def, err := x.resolveValue(ctx, from, v.Text) + if err != nil { + return nil, err + } + + x.memoize(from.URI(), v.Text, resolveValue, def) + + return def, nil +} + +// resolveValue resolves a value-identifier text in from, without +// memoization. +func (x *Index) resolveValue(ctx context.Context, from *cache.ParsedFile, text string) (*Resolved, error) { + _, identifier := parseIdent(from.URI(), from.AST().Includes(), text) identifier = bareName(identifier) - for _, astFile := range definitionFiles(ctx, x.ss, from.URI(), from.AST(), v.Text) { + for _, astFile := range definitionFiles(ctx, x.ss, from.URI(), from.AST(), text) { dst, err := parseDefinitionFile(ctx, x.ss, astFile) if err != nil { return nil, err @@ -133,8 +204,25 @@ func (x *Index) ResolveService(ctx context.Context, from *cache.ParsedFile, iden return nil, nil } - _, identifier := parseIdent(from.URI(), from.AST().Includes(), ident.Text) - for _, astFile := range definitionFiles(ctx, x.ss, from.URI(), from.AST(), ident.Text) { + if def, ok := x.memoized(from.URI(), ident.Text, resolveService); ok { + return def, nil + } + + def, err := x.resolveService(ctx, from, ident.Text) + if err != nil { + return nil, err + } + + x.memoize(from.URI(), ident.Text, resolveService, def) + + return def, nil +} + +// resolveService resolves a service name text in from, without +// memoization. +func (x *Index) resolveService(ctx context.Context, from *cache.ParsedFile, name string) (*Resolved, error) { + _, identifier := parseIdent(from.URI(), from.AST().Includes(), name) + for _, astFile := range definitionFiles(ctx, x.ss, from.URI(), from.AST(), name) { dst, err := parseDefinitionFile(ctx, x.ss, astFile) if err != nil { return nil, err diff --git a/lsp/source/index_memo_test.go b/lsp/source/index_memo_test.go new file mode 100644 index 0000000..6857031 --- /dev/null +++ b/lsp/source/index_memo_test.go @@ -0,0 +1,205 @@ +package source + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.lsp.dev/uri" + + "github.com/karitham/thrift-ls/lsp/cache" + "github.com/karitham/thrift-ls/syntax" +) + +// TestIndexResolutionMemo pins that Index resolutions are memoized per +// (file, name, resolver): repeated resolutions of the same reference are +// identical, unresolved names stay unresolved, and the same name resolved +// as a type versus a value does not collide. +func TestIndexResolutionMemo(t *testing.T) { + ss := cache.BuildSnapshotForTest([]*cache.FileChange{ + { + URI: "file:///base.thrift", + Version: 0, + Content: []byte("struct User { 1: i32 id }\nenum Color { RED = 1 }\nconst i32 MAX = 10\nservice Svc {}\n"), + From: cache.FileChangeTypeDidOpen, + }, + { + URI: "file:///app.thrift", + Version: 0, + Content: []byte("include \"base.thrift\"\nstruct User { 1: string name }\nstruct S { 1: base.User u, 2: i32 x = base.Color.RED }\n"), + From: cache.FileChangeTypeDidOpen, + }, + }) + + ix := NewIndex(ss) + pf := parseOne(t, ss, fu("/app.thrift")) + + tests := []struct { + name string + resolve func() (*Resolved, error) + wantFile string // URI path of the definition file; "" for unresolved + wantName string + wantKind DefinitionKind + }{ + { + name: "qualified type through an include", + resolve: func() (*Resolved, error) { return ix.ResolveType(t.Context(), pf, ft("base.User")) }, + wantFile: "/base.thrift", + wantName: "User", + wantKind: DefinitionStruct, + }, + { + name: "local definition wins over the include", + resolve: func() (*Resolved, error) { return ix.ResolveType(t.Context(), pf, ft("User")) }, + wantFile: "/app.thrift", + wantName: "User", + wantKind: DefinitionStruct, + }, + { + name: "qualified enum value", + resolve: func() (*Resolved, error) { + return ix.ResolveValue(t.Context(), pf, &syntax.ConstValue{Kind: syntax.ValueIdent, Text: "base.Color.RED"}) + }, + wantFile: "/base.thrift", + wantName: "RED", + wantKind: DefinitionEnumValue, + }, + { + name: "qualified service", + resolve: func() (*Resolved, error) { + return ix.ResolveService(t.Context(), pf, &syntax.Identifier{Text: "base.Svc"}) + }, + wantFile: "/base.thrift", + wantName: "Svc", + wantKind: DefinitionService, + }, + { + name: "const value by bare name", + resolve: func() (*Resolved, error) { + return ix.ResolveValue(t.Context(), pf, &syntax.ConstValue{Kind: syntax.ValueIdent, Text: "MAX"}) + }, + wantFile: "/base.thrift", + wantName: "MAX", + wantKind: DefinitionConst, + }, + { + name: "unresolved name memoizes nil", + resolve: func() (*Resolved, error) { return ix.ResolveType(t.Context(), pf, ft("Nope")) }, + wantFile: "", + wantKind: DefinitionNone, + }, + { + name: "a name that is a const is not a type", + resolve: func() (*Resolved, error) { return ix.ResolveType(t.Context(), pf, ft("MAX")) }, + wantFile: "", + wantKind: DefinitionNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Resolve twice: the memoized result must be identical. + first, err := tt.resolve() + require.NoError(t, err) + second, err := tt.resolve() + require.NoError(t, err) + assert.Equal(t, first, second, "memoized resolution must be stable") + + if tt.wantKind == DefinitionNone { + assert.Nil(t, first, "expected unresolved") + + return + } + + require.NotNil(t, first) + assert.Equal(t, tt.wantFile, first.File.Path()) + assert.Equal(t, tt.wantName, first.Name.Text) + assert.Equal(t, tt.wantKind, first.Kind) + }) + } +} + +// TestIndexMemoKeyIsolation pins the memo's key: the same bare name +// resolved from different files never collides, and neither do different +// names from the same file. +func TestIndexMemoKeyIsolation(t *testing.T) { + ss := cache.BuildSnapshotForTest([]*cache.FileChange{ + { + URI: "file:///a.thrift", + Version: 0, + Content: []byte("struct User { 1: i32 id }\n"), + From: cache.FileChangeTypeDidOpen, + }, + { + URI: "file:///b.thrift", + Version: 0, + Content: []byte("struct User { 1: string name }\n"), + From: cache.FileChangeTypeDidOpen, + }, + { + URI: "file:///main.thrift", + Version: 0, + Content: []byte("include \"a.thrift\"\ninclude \"b.thrift\"\nstruct S { 1: a.User x, 2: b.User y }\n"), + From: cache.FileChangeTypeDidOpen, + }, + }) + + ix := NewIndex(ss) + main := parseOne(t, ss, fu("/main.thrift")) + fromB := parseOne(t, ss, fu("/b.thrift")) + + tests := []struct { + name string + from *cache.ParsedFile + text string + wantFile string + }{ + // Same bare name, different referencing files: the memo keys on + // the file, so the two resolutions cannot collide. + {"bare User from main resolves through the first include", main, "User", "/a.thrift"}, + {"bare User from b resolves locally", fromB, "User", "/b.thrift"}, + // Same file, different names. + {"a.User from main", main, "a.User", "/a.thrift"}, + {"b.User from main", main, "b.User", "/b.thrift"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + def, err := ix.ResolveType(t.Context(), tt.from, ft(tt.text)) + require.NoError(t, err) + require.NotNil(t, def) + assert.Equal(t, tt.wantFile, def.File.Path()) + }) + } +} + +// TestSemanticAnalysisSkipsBrokenFile verifies that a file with parse +// errors does not fail the semantic analysis run: the Parse checker owns +// parse errors, and the analysis proceeds (or skips) without erroring. +func TestSemanticAnalysisSkipsBrokenFile(t *testing.T) { + tests := []struct { + name string + content string + }{ + {"unterminated struct", "struct S { 1: "}, + {"garbage tokens", "foo bar baz"}, + {"unclosed annotation", "struct S (x = "}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ss := cache.BuildSnapshotForTest([]*cache.FileChange{ + { + URI: "file:///f.thrift", + Version: 0, + Content: []byte(tt.content), + From: cache.FileChangeTypeDidOpen, + }, + }) + + got, err := (&SemanticAnalysis{}).Diagnostic(t.Context(), ss, []uri.URI{"file:///f.thrift"}) + require.NoError(t, err, "a broken file must not fail the diagnostics run") + assert.NotNil(t, got) + }) + } +} diff --git a/lsp/source/semantic_analysis.go b/lsp/source/semantic_analysis.go index 0a87b42..27c3e33 100644 --- a/lsp/source/semantic_analysis.go +++ b/lsp/source/semantic_analysis.go @@ -2,7 +2,6 @@ package source import ( "context" - "errors" "fmt" "log/slog" @@ -41,30 +40,35 @@ func (s *SemanticAnalysis) diagnostic(ctx context.Context, ss *cache.Snapshot, c } if pf.AST() == nil { - return nil, errors.New("parse ast failed") + // The file does not parse; the Parse checker reports that. + slog.Debug("semantic analysis skipped: file does not parse", "file", changeFile) + + return nil, nil } for _, err := range pf.Errors() { slog.Debug("parse failed", "err", err) } - res := s.checkDefinitionExist(ctx, ss, pf) + // One index per file: resolutions are memoized per (file, name), so + // repeated references resolve once. + res := s.checkDefinitionExist(ctx, ss, NewIndex(ss), pf) return res, nil } // checkDefinitionExist reports field types, const values, and return types // that reference undefined definitions. -func (s *SemanticAnalysis) checkDefinitionExist(ctx context.Context, ss *cache.Snapshot, pf *cache.ParsedFile) []protocol.Diagnostic { +func (s *SemanticAnalysis) checkDefinitionExist(ctx context.Context, ss *cache.Snapshot, ix *Index, pf *cache.ParsedFile) []protocol.Diagnostic { ret := make([]protocol.Diagnostic, 0) processFields := func(fields []*syntax.Field) { for _, field := range fields { - items := s.checkTypeExist(ctx, ss, pf, field.Type) + items := s.checkTypeExist(ctx, ss, ix, pf, field.Type) ret = append(ret, items...) if field.Value != nil { - items := s.checkConstValueExist(ctx, ss, pf, field.Value) + items := s.checkConstValueExist(ctx, ss, ix, pf, field.Value) ret = append(ret, items...) dig := s.checkConstValueMatchType(pf, field) @@ -80,13 +84,13 @@ func (s *SemanticAnalysis) checkDefinitionExist(ctx context.Context, ss *cache.S }) for _, cst := range pf.AST().Consts() { - items := s.checkConstValueExist(ctx, ss, pf, cst.Value) + items := s.checkConstValueExist(ctx, ss, ix, pf, cst.Value) ret = append(ret, items...) } for _, svc := range pf.AST().Services() { for _, fn := range svc.Functions { - items := s.checkTypeExist(ctx, ss, pf, fn.Type) + items := s.checkTypeExist(ctx, ss, ix, pf, fn.Type) ret = append(ret, items...) } } @@ -94,7 +98,7 @@ func (s *SemanticAnalysis) checkDefinitionExist(ctx context.Context, ss *cache.S return ret } -func (s *SemanticAnalysis) checkConstValueExist(ctx context.Context, ss *cache.Snapshot, +func (s *SemanticAnalysis) checkConstValueExist(ctx context.Context, ss *cache.Snapshot, ix *Index, pf *cache.ParsedFile, cst *syntax.ConstValue, ) (res []protocol.Diagnostic) { if cst == nil || cst.Kind != syntax.ValueIdent { @@ -105,7 +109,7 @@ func (s *SemanticAnalysis) checkConstValueExist(ctx context.Context, ss *cache.S return res } - def, err := NewIndex(ss).ResolveValue(ctx, pf, cst) + def, err := ix.ResolveValue(ctx, pf, cst) if err != nil || def == nil { res = append(res, protocol.Diagnostic{ Range: nodeRange(pf, cst), @@ -224,7 +228,7 @@ func typeName(ft *syntax.FieldType) string { return "" } -func (s *SemanticAnalysis) checkTypeExist(ctx context.Context, ss *cache.Snapshot, +func (s *SemanticAnalysis) checkTypeExist(ctx context.Context, ss *cache.Snapshot, ix *Index, pf *cache.ParsedFile, ft *syntax.FieldType, ) (res []protocol.Diagnostic) { if ft == nil { @@ -233,11 +237,11 @@ func (s *SemanticAnalysis) checkTypeExist(ctx context.Context, ss *cache.Snapsho switch ft.Kind { case syntax.TypeMap, syntax.TypeList, syntax.TypeSet: - return s.checkContainerTypeExist(ctx, ss, pf, ft) + return s.checkContainerTypeExist(ctx, ix, ss, pf, ft) case syntax.TypeBase: return nil case syntax.TypeIdent: - def, err := NewIndex(ss).ResolveType(ctx, pf, ft) + def, err := ix.ResolveType(ctx, pf, ft) if err != nil || def == nil { res = append(res, protocol.Diagnostic{ Range: nodeRange(pf, ft.Ident), @@ -252,21 +256,21 @@ func (s *SemanticAnalysis) checkTypeExist(ctx context.Context, ss *cache.Snapsho return res } -func (s *SemanticAnalysis) checkContainerTypeExist(ctx context.Context, +func (s *SemanticAnalysis) checkContainerTypeExist(ctx context.Context, ix *Index, ss *cache.Snapshot, pf *cache.ParsedFile, ft *syntax.FieldType, ) (res []protocol.Diagnostic) { if ft.KeyType != nil { - res = append(res, s.checkTypeExist(ctx, ss, pf, ft.KeyType)...) + res = append(res, s.checkTypeExist(ctx, ss, ix, pf, ft.KeyType)...) if ft.Kind == syntax.TypeMap { - if dig := s.checkMapKeyScalar(ctx, ss, pf, ft.KeyType); dig != nil { + if dig := s.checkMapKeyScalar(ctx, ss, ix, pf, ft.KeyType); dig != nil { res = append(res, *dig) } } } if ft.ValueType != nil { - res = append(res, s.checkTypeExist(ctx, ss, pf, ft.ValueType)...) + res = append(res, s.checkTypeExist(ctx, ss, ix, pf, ft.ValueType)...) } return res @@ -275,8 +279,8 @@ func (s *SemanticAnalysis) checkContainerTypeExist(ctx context.Context, // checkMapKeyScalar returns an error when the map key type is not scalar: // thrift requires map keys to be a base type or an enum. Structs, unions, // exceptions, and containers cannot be keys; typedefs are followed. -func (s *SemanticAnalysis) checkMapKeyScalar(ctx context.Context, ss *cache.Snapshot, pf *cache.ParsedFile, key *syntax.FieldType) *protocol.Diagnostic { - kind := s.mapKeyKind(ctx, ss, pf, key, 0) +func (s *SemanticAnalysis) checkMapKeyScalar(ctx context.Context, ss *cache.Snapshot, ix *Index, pf *cache.ParsedFile, key *syntax.FieldType) *protocol.Diagnostic { + kind := s.mapKeyKind(ctx, ss, ix, pf, key, 0) if kind == "" { return nil } @@ -293,7 +297,7 @@ func (s *SemanticAnalysis) checkMapKeyScalar(ctx context.Context, ss *cache.Snap // mapKeyKind reports why key is not a scalar map key: the container kind, // or the definition kind for struct-like types. "" means scalar: a base // type, an enum, or a typedef chain ending there. -func (s *SemanticAnalysis) mapKeyKind(ctx context.Context, ss *cache.Snapshot, pf *cache.ParsedFile, key *syntax.FieldType, depth int) string { +func (s *SemanticAnalysis) mapKeyKind(ctx context.Context, ss *cache.Snapshot, ix *Index, pf *cache.ParsedFile, key *syntax.FieldType, depth int) string { if key == nil { return "" } @@ -313,7 +317,7 @@ func (s *SemanticAnalysis) mapKeyKind(ctx context.Context, ss *cache.Snapshot, p return "" } - def, err := NewIndex(ss).ResolveType(ctx, pf, key) + def, err := ix.ResolveType(ctx, pf, key) if err != nil || def == nil { return "" } @@ -329,7 +333,7 @@ func (s *SemanticAnalysis) mapKeyKind(ctx context.Context, ss *cache.Snapshot, p return "" } - return s.mapKeyKind(ctx, ss, def.Parsed, td.Type, depth+1) + return s.mapKeyKind(ctx, ss, ix, def.Parsed, td.Type, depth+1) } } -- 2.51.2