From 820656b90d86f7cd0130227d522300a4afa67302 Mon Sep 17 00:00:00 2001 From: karitham Date: Sat, 8 Aug 2026 13:43:40 +0200 Subject: [PATCH] lints: include & remove lints & code actions --- README.md | 44 +++++ check_test.go | 12 +- cli_test.go | 8 +- lsp/cache/snapshot.go | 6 + lsp/codeaction.go | 16 ++ lsp/codeaction_test.go | 27 +++ lsp/source/diagnostic.go | 1 + lsp/source/include_action.go | 233 +++++++++++++++++++++++ lsp/source/include_action_test.go | 185 +++++++++++++++++++ lsp/source/semantic_analysis.go | 88 +++++++++ lsp/source/semantic_analysis_test.go | 73 ++++++++ lsp/source/unused_include_check.go | 235 ++++++++++++++++++++++++ lsp/source/unused_include_check_test.go | 97 ++++++++++ tests/made-in-abyss/cycle_a.thrift | 6 +- tests/made-in-abyss/cycle_b.thrift | 4 +- tests/made-in-abyss/lints.thrift | 21 +++ tests/made-in-abyss/unused.thrift | 8 + 17 files changed, 1053 insertions(+), 11 deletions(-) create mode 100644 lsp/source/include_action.go create mode 100644 lsp/source/include_action_test.go create mode 100644 lsp/source/unused_include_check.go create mode 100644 lsp/source/unused_include_check_test.go create mode 100644 tests/made-in-abyss/unused.thrift diff --git a/README.md b/README.md index 4f4485c..f14eedc 100644 --- a/README.md +++ b/README.md @@ -155,6 +155,50 @@ which groups broke and which stayed flat: thrift-ls dump --ir --printWidth 100 path/to/file.thrift ``` +### Diagnostics: `check` + +`thrift-ls check` runs the same diagnostic pipeline the language server +uses — parse, semantic analysis, and lints — over a file or a whole +folder, and reports everything to stdout. It exits 1 when any +error-severity diagnostic is found, so it can gate CI: + +```bash +thrift-ls check path/to/file.thrift # one file +thrift-ls check path/to/folder/ # every *.thrift under the folder +``` + +Output is one line per diagnostic: + +```text +lints.thrift:37:1 warning unused include "unused.thrift" +lints.thrift:132:10 error map key must be a scalar type, found struct +``` + +The checks: + +| Diagnostic | Severity | Meaning | +| ---------- | -------- | ------- | +| parse errors | error | the file does not parse | +| `field id conflict` / invalid field id | error | duplicate or out-of-range field ids | +| `duplicate ` | error | duplicate struct/enum/typedef/const/service names, members, fields, arguments, functions | +| `enum value N duplicates X` | error | two enum members resolve to the same value | +| `duplicate map key` / `duplicate set value` | error | repeated constant keys/values | +| `map key must be a scalar type` | error | struct, union, exception, or container used as a map key | +| `field type doesn't exist` / `default value doesn't exist` | error | unresolved reference | +| `expect X but got Y` | error | default value does not match the field type | +| `unused include "x.thrift"` | warning | no reference in the file resolves into the include | +| `cycle dependency` | warning | the include graph contains a cycle | +| `X has no explicit value` | warning | enum member relies on implicit value | + +Code actions (refactors and quickfixes) fix these from the editor: +- **Make enum values explicit** — fills in the implicit enum values. +- **Make field required / optional** — rewrites the field qualifier. +- **Remove unused include** — deletes the include line (quickfix on the + warning). +- **Add include "x.thrift"** — finds the file defining a missing type + anywhere in the workspace and adds the include (quickfix on + `field type doesn't exist`). + ## Formatter behavior The formatter is **lossless**: comments, `@` annotations, and blank lines diff --git a/check_test.go b/check_test.go index 3c11dbf..2e61ece 100644 --- a/check_test.go +++ b/check_test.go @@ -42,9 +42,9 @@ func Test_CheckMadeInAbyss(t *testing.T) { assert.Contains(t, cycleB[0].Message, "cycle dependency") assert.Contains(t, cycleB[1].Message, "cycle dependency") - // The mistake showcase: 18 errors and 5 warnings. + // The mistake showcase: 19 errors and 6 warnings. lints := diags[corpusAbs(t, "lints.thrift")] - require.Len(t, lints, 23) + require.Len(t, lints, 25) errs, warns := 0, 0 for _, d := range lints { @@ -55,11 +55,12 @@ func Test_CheckMadeInAbyss(t *testing.T) { warns++ } } - assert.Equal(t, 18, errs) - assert.Equal(t, 5, warns) + assert.Equal(t, 19, errs) + assert.Equal(t, 6, warns) // Every intentional mistake is reported. for _, msg := range []string{ + `unused include "unused.thrift"`, "field id conflict", "field id should be a positive integer in [1, 32767]", "duplicate enum Reg", @@ -72,6 +73,7 @@ func Test_CheckMadeInAbyss(t *testing.T) { `duplicate map key "zone1"`, "duplicate set value 4", "field type doesn't exist", + "map key must be a scalar type, found struct", "STAR_COMPASS has no explicit value (implicitly 0)", "UNHEARD_BELL has no explicit value (implicitly 3)", "CROSSED_STILLS has no explicit value (implicitly 5)", @@ -111,7 +113,7 @@ func Test_CollectThriftFiles(t *testing.T) { } assert.Equal(t, []string{ "abyss.thrift", "cycle_a.thrift", "cycle_b.thrift", - "delvers.thrift", "lints.thrift", "orth.thrift", + "delvers.thrift", "lints.thrift", "orth.thrift", "unused.thrift", }, names) // A missing path is an error. diff --git a/cli_test.go b/cli_test.go index 8e45bcd..b25993c 100644 --- a/cli_test.go +++ b/cli_test.go @@ -107,11 +107,11 @@ func Test_CheckCLI_MadeInAbyss(t *testing.T) { errCount := strings.Count(stdout, " error ") warnCount := strings.Count(stdout, " warning ") - assert.Equal(t, 18, errCount, "error diagnostics") - assert.Equal(t, 8, warnCount, "warning diagnostics (5 lints + 3 cycles)") + assert.Equal(t, 19, errCount, "error diagnostics") + assert.Equal(t, 9, warnCount, "warning diagnostics (6 lints + 3 cycles)") - assert.Contains(t, err.Error(), "18 error(s)") - assert.Contains(t, err.Error(), "8 warning(s)") + assert.Contains(t, err.Error(), "19 error(s)") + assert.Contains(t, err.Error(), "9 warning(s)") } // readGolden returns the recorded output of a CLI test case. diff --git a/lsp/cache/snapshot.go b/lsp/cache/snapshot.go index c4cf6de..78a0c49 100644 --- a/lsp/cache/snapshot.go +++ b/lsp/cache/snapshot.go @@ -189,6 +189,12 @@ func (s *Snapshot) Dependents(uri uri.URI) []uri.URI { return s.context.Dependents(uri) } +// View returns the view this snapshot serves: the workspace folder the +// snapshot resolves files under. +func (s *Snapshot) View() *View { + return s.view +} + // Resolver returns a new Resolver instance for this snapshot. // The resolver provides centralized include path resolution. func (s *Snapshot) Resolver() *Resolver { diff --git a/lsp/codeaction.go b/lsp/codeaction.go index 293f9d3..dbc2e1f 100644 --- a/lsp/codeaction.go +++ b/lsp/codeaction.go @@ -39,6 +39,22 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara } actions = append(actions, fieldActions...) + removeInclude, err := source.MakeRemoveUnusedIncludeAction(ctx, ss, fh, params.Range, params.Context.Diagnostics) + if err != nil { + return nil, err + } + if removeInclude != nil { + actions = append(actions, *removeInclude) + } + + addInclude, err := source.MakeAddMissingIncludeAction(ctx, ss, fh, params.Range, params.Context.Diagnostics) + if err != nil { + return nil, err + } + if addInclude != nil { + actions = append(actions, *addInclude) + } + actions = preferQuickFixes(filterCodeActions(actions, params.Context.Only)) out := make([]protocol.CommandOrCodeAction, 0, len(actions)) diff --git a/lsp/codeaction_test.go b/lsp/codeaction_test.go index e25bf9b..86fe69a 100644 --- a/lsp/codeaction_test.go +++ b/lsp/codeaction_test.go @@ -74,6 +74,33 @@ func Test_CodeAction(t *testing.T) { content: "enum E { A = 1, B = 2 }\n", want: map[string]protocol.CodeActionKind{}, }, + { + // An unused include warning offers the removal quickfix on + // the include line. + name: "remove unused include quickfix", + content: "include \"shared.thrift\"\nstruct S { 1: i32 a }\n", + context: protocol.CodeActionContext{ + Diagnostics: []protocol.Diagnostic{{ + Range: protocol.Range{Start: protocol.Position{Line: 0, Character: 0}, End: protocol.Position{Line: 0, Character: 22}}, + Message: protocol.String(`unused include "shared.thrift"`), + }}, + }, + want: map[string]protocol.CodeActionKind{ + `Remove unused include "shared.thrift"`: protocol.CodeActionKindQuickFix, + }, + }, + { + // The same diagnostic elsewhere does not offer the removal. + name: "unused include diagnostic elsewhere", + content: "include \"shared.thrift\"\nstruct S { 1: i32 a }\n", + context: protocol.CodeActionContext{ + Diagnostics: []protocol.Diagnostic{{ + Range: protocol.Range{Start: protocol.Position{Line: 5, Character: 0}, End: protocol.Position{Line: 5, Character: 1}}, + Message: protocol.String(`unused include "shared.thrift"`), + }}, + }, + want: map[string]protocol.CodeActionKind{}, + }, } for _, tt := range tests { diff --git a/lsp/source/diagnostic.go b/lsp/source/diagnostic.go index 4317d8a..882da41 100644 --- a/lsp/source/diagnostic.go +++ b/lsp/source/diagnostic.go @@ -22,6 +22,7 @@ func init() { &FieldIDCheck{}, &DuplicateCheck{}, &EnumValueCheck{}, + &UnusedIncludeCheck{}, &SemanticAnalysis{}, } } diff --git a/lsp/source/include_action.go b/lsp/source/include_action.go new file mode 100644 index 0000000..99f95d5 --- /dev/null +++ b/lsp/source/include_action.go @@ -0,0 +1,233 @@ +package source + +import ( + "context" + "fmt" + "io/fs" + "path" + "path/filepath" + "sort" + "strings" + + "go.lsp.dev/protocol" + "go.lsp.dev/uri" + + "github.com/karitham/thrift-ls/lsp/cache" + "github.com/karitham/thrift-ls/syntax" +) + +// MakeRemoveUnusedIncludeAction returns the quickfix that deletes the +// include line for an "unused include" diagnostic on the selection. It +// returns nil when no such diagnostic overlaps the selection. +func MakeRemoveUnusedIncludeAction(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, rng protocol.Range, diags []protocol.Diagnostic) (*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 + } + + inc := unusedIncludeAt(pf, rng, diags) + if inc == nil { + return nil, nil + } + + // The include statement is a statement of its own: delete from the + // first token to the end of its line. Tokens carry 1-based lines. + start, end := pf.AST().Range(inc) + span := toLSPRange(pf, start, end) + + return &protocol.CodeAction{ + Title: fmt.Sprintf("Remove unused include %q", inc.PathText()), + Kind: new(protocol.CodeActionKindQuickFix), + Edit: &protocol.WorkspaceEdit{ + Changes: map[uri.URI][]protocol.TextEdit{ + fh.URI(): { + { + Range: protocol.Range{ + Start: protocol.Position{Line: span.Start.Line, Character: 0}, + End: protocol.Position{ + Line: span.End.Line + 1, + Character: 0, + }, + }, + NewText: "", + }, + }, + }, + }, + }, nil +} + +// unusedIncludeAt returns the include statement an "unused include" +// diagnostic on the selection refers to, or nil. +func unusedIncludeAt(pf *cache.ParsedFile, rng protocol.Range, diags []protocol.Diagnostic) *syntax.Include { + var target protocol.Range + found := false + + for _, d := range diags { + if strings.HasPrefix(string(d.Message.(protocol.String)), "unused include") && rangesOverlap(rng, d.Range) { + target = d.Range + found = true + + break + } + } + + if !found { + return nil + } + + for _, inc := range pf.AST().Includes() { + if rangesOverlap(target, nodeRange(pf, inc)) { + return inc + } + } + + return nil +} + +// MakeAddMissingIncludeAction returns the quickfix that adds an include of +// the file defining a type flagged "field type doesn't exist" on the +// selection. The definition is searched in every thrift file under the +// workspace folder, so the fix works across the whole project. It returns +// nil when the selection has no such diagnostic, the referenced type is +// not found anywhere, or the current file cannot be edited (parse errors). +func MakeAddMissingIncludeAction(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, rng protocol.Range, diags []protocol.Diagnostic) (*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 + } + + name := missingTypeAt(ctx, ss, fh, rng, diags) + if name == "" { + return nil, nil + } + + defFile, ok := findTypeInFolder(ctx, ss, fh.URI(), name) + if !ok || defFile == fh.URI() { + return nil, nil + } + + incPath, err := filepath.Rel(path.Dir(fh.URI().Path()), defFile.Path()) + if err != nil { + return nil, nil + } + incPath = filepath.ToSlash(incPath) + + // Insert after the last include statement, or at the top of the file. + insert := protocol.Position{} + if includes := pf.AST().Includes(); len(includes) > 0 { + last := includes[len(includes)-1] + _, end := pf.AST().Range(last) + insert = toLSPPosition(pf, end) + insert.Character = 0 + insert.Line++ + } + + return &protocol.CodeAction{ + Title: fmt.Sprintf("Add include %q", incPath), + Kind: new(protocol.CodeActionKindQuickFix), + Edit: &protocol.WorkspaceEdit{ + Changes: map[uri.URI][]protocol.TextEdit{ + fh.URI(): { + { + Range: protocol.Range{Start: insert, End: insert}, + NewText: fmt.Sprintf("include %q\n", incPath), + }, + }, + }, + }, + }, nil +} + +// missingTypeAt returns the type name of a "field type doesn't exist" +// diagnostic on the selection, or "". +func missingTypeAt(ctx context.Context, ss *cache.Snapshot, fh cache.FileHandle, rng protocol.Range, diags []protocol.Diagnostic) string { + overlap := false + + for _, d := range diags { + if string(d.Message.(protocol.String)) == "field type doesn't exist" && rangesOverlap(rng, d.Range) { + overlap = true + + break + } + } + + if !overlap { + return "" + } + + _, target, err := resolveTarget(ctx, ss, fh.URI(), rng.Start) + if err != nil { + return "" + } + + if target.kind != TargetTypeName { + return "" + } + + ft, ok := target.parent.(*syntax.FieldType) + if !ok { + return "" + } + + return typeReferenceName(ft) +} + +// findTypeInFolder searches every thrift file under the workspace folder +// (excluding file) for a definition of name, returning the first match in +// lexical order. +func findTypeInFolder(ctx context.Context, ss *cache.Snapshot, file uri.URI, name string) (uri.URI, bool) { + root := ss.View().Folder().Path() + if root == "" { + return "", false + } + + var files []string + + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return nil + } + + if !d.IsDir() && strings.HasSuffix(d.Name(), ".thrift") && uri.File(p) != file { + files = append(files, p) + } + + return nil + }) + if err != nil { + return "", false + } + + sort.Strings(files) + + for _, p := range files { + pf, err := ss.Parse(ctx, uri.File(p)) + if err != nil || pf.AST() == nil { + continue + } + + if _, ok := pf.Definitions()[name]; ok { + return uri.File(p), true + } + } + + return "", false +} + +// rangesOverlap reports whether two LSP ranges share any position. +func rangesOverlap(a, b protocol.Range) bool { + if a.Start.Line == b.Start.Line && a.End.Line == b.End.Line { + return a.Start.Character < b.End.Character && b.Start.Character < a.End.Character + } + + return a.End.Line >= b.Start.Line && b.End.Line >= a.Start.Line +} diff --git a/lsp/source/include_action_test.go b/lsp/source/include_action_test.go new file mode 100644 index 0000000..7a9e490 --- /dev/null +++ b/lsp/source/include_action_test.go @@ -0,0 +1,185 @@ +package source + +import ( + "os" + "path/filepath" + "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" + "github.com/karitham/thrift-ls/lsp/mapper" +) + +// buildFolderSnapshotForTest builds a snapshot whose view root is folder, +// with the given files opened in the overlay. +func buildFolderSnapshotForTest(t *testing.T, folder string, files []*cache.FileChange) *cache.Snapshot { + t.Helper() + + c := cache.New(nil) + fs := cache.NewOverlayFS(c) + _ = fs.Update(t.Context(), files) + + view := cache.NewView("test", uri.File(folder), fs, nil) + + return cache.NewSnapshot(view, nil) +} + +// writeThrift writes content to a .thrift file under folder. +func writeThrift(t *testing.T, folder, name, content string) string { + t.Helper() + + p := filepath.Join(folder, name) + require.NoError(t, os.WriteFile(p, []byte(content), 0o644)) + + return p +} + +func Test_MakeRemoveUnusedIncludeAction(t *testing.T) { + folder := t.TempDir() + filePath := writeThrift(t, folder, "user.thrift", "include \"shared.thrift\"\nstruct S { 1: i32 a }\n") + + ss := buildFolderSnapshotForTest(t, folder, []*cache.FileChange{ + { + URI: uri.File(filePath), + Version: 0, + Content: []byte("include \"shared.thrift\"\nstruct S { 1: i32 a }\n"), + From: cache.FileChangeTypeDidOpen, + }, + }) + + fh, err := ss.ReadFile(t.Context(), uri.File(filePath)) + require.NoError(t, err) + + // The diagnostic the check produces, as the server would pass it. + diags, err := (&UnusedIncludeCheck{}).diagnostic(t.Context(), ss, uri.File(filePath)) + require.NoError(t, err) + require.Len(t, diags, 1) + + act, err := MakeRemoveUnusedIncludeAction(t.Context(), ss, fh, diags[0].Range, diags) + require.NoError(t, err) + require.NotNil(t, act) + assert.Equal(t, protocol.CodeActionKindQuickFix, *act.Kind) + + edits := act.Edit.Changes[uri.File(filePath)] + got, err := mapper.NewMapper(uri.File(filePath), []byte("include \"shared.thrift\"\nstruct S { 1: i32 a }\n")).ApplyEdits(edits) + require.NoError(t, err) + assert.Equal(t, "struct S { 1: i32 a }\n", string(got)) +} + +func Test_MakeRemoveUnusedIncludeAction_NoDiagnostic(t *testing.T) { + folder := t.TempDir() + filePath := writeThrift(t, folder, "user.thrift", "include \"shared.thrift\"\nstruct S { 1: shared.User u }\n") + + ss := buildFolderSnapshotForTest(t, folder, []*cache.FileChange{ + { + URI: uri.File(filePath), + Version: 0, + Content: []byte("include \"shared.thrift\"\nstruct S { 1: shared.User u }\n"), + From: cache.FileChangeTypeDidOpen, + }, + }) + + fh, err := ss.ReadFile(t.Context(), uri.File(filePath)) + require.NoError(t, err) + + act, err := MakeRemoveUnusedIncludeAction(t.Context(), ss, fh, pointRange(0, 0), nil) + require.NoError(t, err) + assert.Nil(t, act) +} + +func Test_MakeAddMissingIncludeAction(t *testing.T) { + folder := t.TempDir() + _ = writeThrift(t, folder, "shared.thrift", "struct User {\n 1: i32 id,\n}\n") + filePath := writeThrift(t, folder, "user.thrift", "struct S {\n 1: User u,\n}\n") + + ss := buildFolderSnapshotForTest(t, folder, []*cache.FileChange{ + { + URI: uri.File(filePath), + Version: 0, + Content: []byte("struct S {\n 1: User u,\n}\n"), + From: cache.FileChangeTypeDidOpen, + }, + }) + + fh, err := ss.ReadFile(t.Context(), uri.File(filePath)) + require.NoError(t, err) + + // The semantic diagnostic the server would pass, at the type position. + diag := protocol.Diagnostic{ + Range: protocol.Range{Start: protocol.Position{Line: 1, Character: 6}, End: protocol.Position{Line: 1, Character: 10}}, + Message: protocol.String("field type doesn't exist"), + } + + act, err := MakeAddMissingIncludeAction(t.Context(), ss, fh, diag.Range, []protocol.Diagnostic{diag}) + require.NoError(t, err) + require.NotNil(t, act) + assert.Equal(t, protocol.CodeActionKindQuickFix, *act.Kind) + assert.Equal(t, `Add include "shared.thrift"`, act.Title) + + edits := act.Edit.Changes[uri.File(filePath)] + got, err := mapper.NewMapper(uri.File(filePath), []byte("struct S {\n 1: User u,\n}\n")).ApplyEdits(edits) + require.NoError(t, err) + assert.Equal(t, "include \"shared.thrift\"\nstruct S {\n 1: User u,\n}\n", string(got)) +} + +func Test_MakeAddMissingIncludeAction_InsertAfterExistingIncludes(t *testing.T) { + folder := t.TempDir() + _ = writeThrift(t, folder, "shared.thrift", "struct User {\n 1: i32 id,\n}\n") + filePath := writeThrift(t, folder, "user.thrift", "include \"base.thrift\"\n\nstruct S {\n 1: User u,\n}\n") + + ss := buildFolderSnapshotForTest(t, folder, []*cache.FileChange{ + { + URI: uri.File(filePath), + Version: 0, + Content: []byte("include \"base.thrift\"\n\nstruct S {\n 1: User u,\n}\n"), + From: cache.FileChangeTypeDidOpen, + }, + }) + + fh, err := ss.ReadFile(t.Context(), uri.File(filePath)) + require.NoError(t, err) + + diag := protocol.Diagnostic{ + Range: protocol.Range{Start: protocol.Position{Line: 3, Character: 6}, End: protocol.Position{Line: 3, Character: 10}}, + Message: protocol.String("field type doesn't exist"), + } + + act, err := MakeAddMissingIncludeAction(t.Context(), ss, fh, diag.Range, []protocol.Diagnostic{diag}) + require.NoError(t, err) + require.NotNil(t, act) + + edits := act.Edit.Changes[uri.File(filePath)] + got, err := mapper.NewMapper(uri.File(filePath), []byte("include \"base.thrift\"\n\nstruct S {\n 1: User u,\n}\n")).ApplyEdits(edits) + require.NoError(t, err) + assert.Equal(t, "include \"base.thrift\"\ninclude \"shared.thrift\"\n\nstruct S {\n 1: User u,\n}\n", string(got)) +} + +func Test_MakeAddMissingIncludeAction_TypeNotFound(t *testing.T) { + folder := t.TempDir() + filePath := writeThrift(t, folder, "user.thrift", "struct S {\n 1: Ghost u,\n}\n") + + ss := buildFolderSnapshotForTest(t, folder, []*cache.FileChange{ + { + URI: uri.File(filePath), + Version: 0, + Content: []byte("struct S {\n 1: Ghost u,\n}\n"), + From: cache.FileChangeTypeDidOpen, + }, + }) + + fh, err := ss.ReadFile(t.Context(), uri.File(filePath)) + require.NoError(t, err) + + diag := protocol.Diagnostic{ + Range: protocol.Range{Start: protocol.Position{Line: 1, Character: 6}, End: protocol.Position{Line: 1, Character: 11}}, + Message: protocol.String("field type doesn't exist"), + } + + act, err := MakeAddMissingIncludeAction(t.Context(), ss, fh, diag.Range, []protocol.Diagnostic{diag}) + require.NoError(t, err) + assert.Nil(t, act) +} diff --git a/lsp/source/semantic_analysis.go b/lsp/source/semantic_analysis.go index e41d3dc..ed5e231 100644 --- a/lsp/source/semantic_analysis.go +++ b/lsp/source/semantic_analysis.go @@ -255,6 +255,12 @@ func (s *SemanticAnalysis) checkContainerTypeExist(ctx context.Context, ) (res []protocol.Diagnostic) { if ft.KeyType != nil { res = append(res, s.checkTypeExist(ctx, ss, file, pf, ft.KeyType)...) + + if ft.Kind == syntax.TypeMap { + if dig := s.checkMapKeyScalar(ctx, ss, file, pf, ft.KeyType); dig != nil { + res = append(res, *dig) + } + } } if ft.ValueType != nil { @@ -263,3 +269,85 @@ func (s *SemanticAnalysis) checkContainerTypeExist(ctx context.Context, return res } + +// 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, file uri.URI, pf *cache.ParsedFile, key *syntax.FieldType) *protocol.Diagnostic { + kind := s.mapKeyKind(ctx, ss, file, pf.AST(), key, 0) + if kind == "" { + return nil + } + + return &protocol.Diagnostic{ + Range: nodeRange(pf, key), + Severity: protocol.DiagnosticSeverityError, + Source: protocol.NewOptional("thrift-ls"), + Message: protocol.String(fmt.Sprintf("map key must be a scalar type, found %s", kind)), + } +} + +// 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, file uri.URI, ast *syntax.Document, key *syntax.FieldType, depth int) string { + if key == nil { + return "" + } + + switch key.Kind { + case syntax.TypeBase: + return "" + case syntax.TypeMap: + return "map" + case syntax.TypeList: + return "list" + case syntax.TypeSet: + return "set" + case syntax.TypeIdent: + name := typeReferenceName(key) + if name == "" || IsBasicType(name) || depth > 8 { + return "" + } + + dstFile, id, kind, err := FindTypeDefinition(ctx, ss, file, ast, key) + if err != nil || id == nil { + return "" + } + + switch kind { + case DefinitionEnum: + return "" + case DefinitionStruct, DefinitionUnion, DefinitionException: + return kindLabel(kind) + case DefinitionTypedef: + dstPf, err := parseDefinitionFile(ctx, ss, dstFile) + if err != nil { + return "" + } + + td, ok := dstPf.Definitions()[id.Text].(*syntax.Typedef) + if !ok { + return "" + } + + return s.mapKeyKind(ctx, ss, dstFile, dstPf.AST(), td.Type, depth+1) + } + } + + return "" +} + +// kindLabel is the message label of a definition kind. +func kindLabel(k DefinitionKind) string { + switch k { + case DefinitionStruct: + return "struct" + case DefinitionUnion: + return "union" + case DefinitionException: + return "exception" + } + + return "type" +} diff --git a/lsp/source/semantic_analysis_test.go b/lsp/source/semantic_analysis_test.go index 267eee3..25988d2 100644 --- a/lsp/source/semantic_analysis_test.go +++ b/lsp/source/semantic_analysis_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.lsp.dev/protocol" "go.lsp.dev/uri" @@ -261,3 +262,75 @@ struct TestUUID { }) } } + +func Test_SemanticAnalysis_MapKeyScalar(t *testing.T) { + tests := []struct { + name string + content string + want []string // expected messages + }{ + { + name: "base type keys are fine", + content: "struct S {\n 1: map m,\n}\n", + want: nil, + }, + { + name: "struct key", + content: "struct K { 1: i32 a }\nstruct S {\n 1: map m,\n}\n", + want: []string{"map key must be a scalar type, found struct"}, + }, + { + name: "list key", + content: "struct S {\n 1: map, i32> m,\n}\n", + want: []string{"map key must be a scalar type, found list"}, + }, + { + name: "map key", + content: "struct S {\n 1: map, i32> m,\n}\n", + want: []string{"map key must be a scalar type, found map"}, + }, + { + name: "enum key is fine", + content: "enum E { A = 1 }\nstruct S {\n 1: map m,\n}\n", + want: nil, + }, + { + name: "typedef to struct is rejected", + content: "struct K { 1: i32 a }\ntypedef K Alias\nstruct S {\n 1: map m,\n}\n", + want: []string{"map key must be a scalar type, found struct"}, + }, + { + name: "typedef to scalar is fine", + content: "typedef i64 Id\ntypedef Id Id2\nstruct S {\n 1: map m,\n}\n", + want: nil, + }, + { + name: "nested container key is rejected", + content: "struct S {\n 1: map>, i32> m,\n}\n", + want: []string{"map key must be a scalar type, found list"}, + }, + } + + 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 := (&SemanticAnalysis{}).diagnostic(t.Context(), ss, "file:///tmp/user.thrift") + require.NoError(t, err) + + var msgs []string + for _, d := range got { + msgs = append(msgs, string(d.Message.(protocol.String))) + } + + assert.Equal(t, tt.want, msgs) + }) + } +} diff --git a/lsp/source/unused_include_check.go b/lsp/source/unused_include_check.go new file mode 100644 index 0000000..398bd03 --- /dev/null +++ b/lsp/source/unused_include_check.go @@ -0,0 +1,235 @@ +package source + +import ( + "context" + "errors" + "fmt" + "log/slog" + + "go.lsp.dev/protocol" + "go.lsp.dev/uri" + + "github.com/karitham/thrift-ls/lsp/cache" + "github.com/karitham/thrift-ls/syntax" +) + +// UnusedIncludeCheck reports an include no reference in the file resolves +// into. Unused includes bloat the compile and make the dependency graph +// look worse than it is. +type UnusedIncludeCheck struct{} + +func (c *UnusedIncludeCheck) Name() string { + return "UnusedIncludeCheck" +} + +func (c *UnusedIncludeCheck) 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 *UnusedIncludeCheck) 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) + } + + return unusedIncludeDiagnostics(ctx, ss, file, pf), nil +} + +// unusedIncludeDiagnostics warns on every include whose target file never +// receives a resolved reference from this document. A reference is a type +// name, a constant value identifier, or a service extends clause, used +// qualified ("base.Type") or unqualified (resolving through the include +// chain). +func unusedIncludeDiagnostics(ctx context.Context, ss *cache.Snapshot, file uri.URI, pf *cache.ParsedFile) []protocol.Diagnostic { + includes := pf.AST().Includes() + if len(includes) == 0 { + return nil + } + + used := usedIncludes(ctx, ss, file, pf) + + var ret []protocol.Diagnostic + + for _, inc := range includes { + if used[inc] { + continue + } + + ret = append(ret, protocol.Diagnostic{ + Range: nodeRange(pf, inc), + Severity: protocol.DiagnosticSeverityWarning, + Source: protocol.NewOptional("thrift-ls"), + Message: protocol.String(fmt.Sprintf("unused include %q", inc.PathText())), + }) + } + + return ret +} + +// usedIncludes marks every include that at least one reference in the +// document resolves into. Resolution goes through the definition finders, +// which handle both qualified ("base.Type") and unqualified names that +// resolve through the include chain. +func usedIncludes(ctx context.Context, ss *cache.Snapshot, file uri.URI, pf *cache.ParsedFile) map[*syntax.Include]bool { + resolver := ss.Resolver() + includeByFile := make(map[uri.URI]*syntax.Include) + + for _, inc := range pf.AST().Includes() { + if p := inc.PathText(); p != "" { + includeByFile[resolver.ResolveInclude(file, p)] = inc + } + } + + used := make(map[*syntax.Include]bool) + seen := make(map[string]bool) + + for _, name := range referencedNames(pf.AST()) { + if seen[name] { + continue + } + seen[name] = true + + if dst, ok := resolveReferenceFile(ctx, ss, file, pf.AST(), name); ok { + if inc, ok := includeByFile[dst]; ok { + used[inc] = true + } + } + } + + return used +} + +// resolveReferenceFile returns the file a reference name resolves to, or +// false when it resolves nowhere or into the current file. Type, const +// value, and service references are all considered; each finder resolves +// include-qualified names itself. +func resolveReferenceFile(ctx context.Context, ss *cache.Snapshot, file uri.URI, ast *syntax.Document, name string) (uri.URI, bool) { + ft := &syntax.FieldType{Kind: syntax.TypeIdent, Ident: &syntax.Identifier{Text: name}} + if dst, id, _, err := FindTypeDefinition(ctx, ss, file, ast, ft); err == nil && id != nil && dst != file { + return dst, true + } + + cv := &syntax.ConstValue{Kind: syntax.ValueIdent, Text: name} + if dst, id, err := FindConstValueDefinition(ctx, ss, file, ast, cv); err == nil && id != nil && dst != file { + return dst, true + } + + id := &syntax.Identifier{Text: name} + if dst, found, err := FindServiceDefinition(ctx, ss, file, ast, id); err == nil && found != nil && dst != file { + return dst, true + } + + return "", false +} + +// referencedNames collects every identifier used in a reference position: +// field, argument, throws, return, typedef, and const types; const value +// identifiers; and service extends. +func referencedNames(doc *syntax.Document) []string { + var names []string + + addType := func(t *syntax.FieldType) { + walkTypeIdents(t, func(text string) { names = append(names, text) }) + } + addValue := func(v *syntax.ConstValue) { + walkValueIdents(v, func(text string) { names = append(names, text) }) + } + + doc.WalkFieldLists(func(fields []*syntax.Field, _ syntax.FieldListKind) { + for _, f := range fields { + addType(f.Type) + addValue(f.Value) + } + }) + + for _, td := range doc.Typedefs() { + addType(td.Type) + } + + for _, cs := range doc.Consts() { + addType(cs.Type) + addValue(cs.Value) + } + + for _, svc := range doc.Services() { + if svc.Extends != nil { + names = append(names, svc.Extends.Text) + } + + for _, fn := range svc.Functions { + addType(fn.Type) + + for _, arg := range fn.Args { + addType(arg.Type) + } + + if fn.Throws != nil { + for _, f := range fn.Throws.Fields { + addType(f.Type) + addValue(f.Value) + } + } + } + } + + return names +} + +// walkTypeIdents calls f with every identifier of a type reference, +// including nested container types. +func walkTypeIdents(t *syntax.FieldType, f func(string)) { + if t == nil { + return + } + + switch t.Kind { + case syntax.TypeIdent: + if t.Ident != nil { + f(t.Ident.Text) + } + case syntax.TypeMap, syntax.TypeList, syntax.TypeSet: + walkTypeIdents(t.KeyType, f) + walkTypeIdents(t.ValueType, f) + } +} + +// walkValueIdents calls f with every identifier of a constant value, +// descending into maps and lists. +func walkValueIdents(v *syntax.ConstValue, f func(string)) { + if v == nil { + return + } + + switch v.Kind { + case syntax.ValueIdent: + f(v.Text) + case syntax.ValueList: + for _, item := range v.List { + walkValueIdents(item, f) + } + case syntax.ValueMap: + for _, entry := range v.Map { + walkValueIdents(entry.Key, f) + walkValueIdents(entry.Value, f) + } + } +} diff --git a/lsp/source/unused_include_check_test.go b/lsp/source/unused_include_check_test.go new file mode 100644 index 0000000..a2f24fc --- /dev/null +++ b/lsp/source/unused_include_check_test.go @@ -0,0 +1,97 @@ +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" +) + +func Test_UnusedIncludeCheck(t *testing.T) { + tests := []struct { + name string + content string + want []string // full diagnostic messages + }{ + { + name: "no includes", + content: "struct S { 1: i32 a }\n", + want: nil, + }, + { + name: "unused include", + content: "include \"shared.thrift\"\nstruct S { 1: i32 a }\n", + want: []string{`unused include "shared.thrift"`}, + }, + { + name: "used by field type reference", + content: "include \"shared.thrift\"\nstruct S { 1: shared.User u }\n", + want: nil, + }, + { + name: "used by unqualified type reference", + content: "include \"shared.thrift\"\nstruct S { 1: User u }\n", + want: nil, + }, + { + name: "used by nested container type reference", + content: "include \"shared.thrift\"\nstruct S { 1: list us }\n", + want: nil, + }, + { + name: "used by const value identifier", + content: "include \"shared.thrift\"\nconst i32 X = shared.Color.RED\n", + want: nil, + }, + { + name: "used by service extends", + content: "include \"shared.thrift\"\nservice S extends shared.Base {}\n", + want: nil, + }, + { + name: "used by function return and argument types", + content: "include \"shared.thrift\"\nservice S { shared.User get(1: shared.User u) }\n", + want: nil, + }, + { + name: "one used, one unused", + content: "include \"used.thrift\"\ninclude \"unused.thrift\"\nstruct S { 1: used.User u }\n", + want: []string{`unused include "unused.thrift"`}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + folder := t.TempDir() + _ = writeThrift(t, folder, "shared.thrift", "struct User {\n 1: i32 id,\n}\nenum Color { RED = 1 }\nservice Base {}\n") + _ = writeThrift(t, folder, "used.thrift", "struct User {\n 1: i32 id,\n}\n") + _ = writeThrift(t, folder, "unused.thrift", "struct Ghost {}\n") + + filePath := writeThrift(t, folder, "user.thrift", tt.content) + + ss := buildFolderSnapshotForTest(t, folder, []*cache.FileChange{ + { + URI: uri.File(filePath), + Version: 0, + Content: []byte(tt.content), + From: cache.FileChangeTypeDidOpen, + }, + }) + + got, err := (&UnusedIncludeCheck{}).diagnostic(t.Context(), ss, uri.File(filePath)) + require.NoError(t, err) + + var msgs []string + for _, d := range got { + assert.Equal(t, protocol.DiagnosticSeverityWarning, d.Severity) + msgs = append(msgs, string(d.Message.(protocol.String))) + } + + assert.Equal(t, tt.want, msgs) + }) + } +} diff --git a/tests/made-in-abyss/cycle_a.thrift b/tests/made-in-abyss/cycle_a.thrift index 4053c21..1673e2c 100644 --- a/tests/made-in-abyss/cycle_a.thrift +++ b/tests/made-in-abyss/cycle_a.thrift @@ -1,7 +1,11 @@ // CycleCheck — cycle_a.thrift and cycle_b.thrift include each other. +// Each file uses a type from the other, so the includes are used and only +// the cycle diagnostics fire. // Expect the cycle diagnostic on this include while the folder is open. include "cycle_b.thrift" namespace go made -struct FromCycleA {} \ No newline at end of file +struct FromCycleA { + 1: FromCycleB other, +} \ No newline at end of file diff --git a/tests/made-in-abyss/cycle_b.thrift b/tests/made-in-abyss/cycle_b.thrift index 11756dd..cf4e103 100644 --- a/tests/made-in-abyss/cycle_b.thrift +++ b/tests/made-in-abyss/cycle_b.thrift @@ -3,4 +3,6 @@ include "cycle_a.thrift" namespace go made -struct FromCycleB {} \ No newline at end of file +struct FromCycleB { + 1: FromCycleA other, +} \ No newline at end of file diff --git a/tests/made-in-abyss/lints.thrift b/tests/made-in-abyss/lints.thrift index a4dd010..638a27e 100644 --- a/tests/made-in-abyss/lints.thrift +++ b/tests/made-in-abyss/lints.thrift @@ -2,6 +2,9 @@ // above each section names the check that fires and the message to // expect: // +// include "unused.thrift" UnusedIncludeCheck (warning) — "unused +// include \"unused.thrift\"" + the "Remove +// unused include" code action // struct DuplicateFieldID FieldIDCheck — "field id conflict" // struct BadVault FieldIDCheck — "field id should be a // positive integer in [1, 32767]" @@ -23,9 +26,16 @@ // exist" // const CURSED_LOCATIONS DuplicateCheck — "duplicate map key // \"zone1\"", "duplicate set value 4" +// struct KeyedVault SemanticAnalysis — "map key must be a +// scalar type, found struct" (the enum-key +// map stays clean) // // struct Clean at the bottom is fully valid: no diagnostics expected. +// UnusedIncludeCheck — nothing in this file references a type from +// unused.thrift. The "Remove unused include" code action deletes the line. +include "unused.thrift" + // FieldIDCheck — both `1:` tokens fire "field id conflict". struct DuplicateFieldID { 1: i32 weight, @@ -112,6 +122,17 @@ const map> CURSED_LOCATIONS = { "zone1": [7, 8, 9], } +// SemanticAnalysis — KeyStone is a struct, so it cannot be a map key; the +// enum-keyed map on the second field is valid. +struct KeyStone { + 1: i32 depth, +} + +struct KeyedVault { + 1: map by_struct, + 2: map by_enum, +} + // Fully valid — no diagnostics expected here. struct Clean { 1: required i32 id, diff --git a/tests/made-in-abyss/unused.thrift b/tests/made-in-abyss/unused.thrift new file mode 100644 index 0000000..608f491 --- /dev/null +++ b/tests/made-in-abyss/unused.thrift @@ -0,0 +1,8 @@ +// A valid file that nothing includes for real: lints.thrift includes it +// only so UnusedIncludeCheck has something to flag, and nothing in the +// corpus references a type from here. +namespace go unused + +struct Spare { + 1: i32 id, +} -- 2.51.2