From f7a8a58e8d082a049620072678a245a18b8d26c4 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Mon, 29 Oct 2018 18:12:41 -0400 Subject: [PATCH] internal/lsp: use packagestest markers to test diagnostics Add some basic tests for diagnostics using the new go/packages/packagestest framework. Change-Id: I6a7bfba6c392928a9eb123ab71ceb73785c12600 Reviewed-on: https://go-review.googlesource.com/c/145697 Run-TryBot: Rebecca Stambler Reviewed-by: Ian Cottrell --- go/packages/packagestest/export.go | 5 +- internal/lsp/diagnostics.go | 38 ++++---- internal/lsp/diagnostics_test.go | 87 +++++++++++++++++++ internal/lsp/server.go | 5 +- internal/lsp/testdata/diagnostics/bad/bad.go | 19 ++++ .../lsp/testdata/diagnostics/bad/bad_util.go | 6 ++ internal/lsp/testdata/diagnostics/bar/bar.go | 7 ++ internal/lsp/testdata/diagnostics/baz/baz.go | 7 ++ internal/lsp/testdata/diagnostics/foo/foo.go | 3 + .../lsp/testdata/diagnostics/good/good.go | 6 ++ .../testdata/diagnostics/good/good_util.go | 10 +++ .../diagnostics/noparse/noparse.go.in | 11 +++ internal/lsp/view.go | 21 +++-- 13 files changed, 196 insertions(+), 29 deletions(-) create mode 100644 internal/lsp/diagnostics_test.go create mode 100644 internal/lsp/testdata/diagnostics/bad/bad.go create mode 100644 internal/lsp/testdata/diagnostics/bad/bad_util.go create mode 100644 internal/lsp/testdata/diagnostics/bar/bar.go create mode 100644 internal/lsp/testdata/diagnostics/baz/baz.go create mode 100644 internal/lsp/testdata/diagnostics/foo/foo.go create mode 100644 internal/lsp/testdata/diagnostics/good/good.go create mode 100644 internal/lsp/testdata/diagnostics/good/good_util.go create mode 100644 internal/lsp/testdata/diagnostics/noparse/noparse.go.in diff --git a/go/packages/packagestest/export.go b/go/packages/packagestest/export.go index 2086b1c3..c59f7c00 100644 --- a/go/packages/packagestest/export.go +++ b/go/packages/packagestest/export.go @@ -217,7 +217,10 @@ func Copy(source string) Writer { // This will panic if there is any kind of error trying to walk the file tree. func MustCopyFileTree(root string) map[string]interface{} { result := map[string]interface{}{} - if err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err := filepath.Walk(filepath.FromSlash(root), func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } if info.IsDir() { return nil } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 1a38d734..2ca622bb 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -63,23 +63,27 @@ func (v *view) diagnostics(uri protocol.DocumentURI) (map[string][]protocol.Diag } func parseErrorPos(pkgErr packages.Error) (pos token.Position) { - split := strings.Split(pkgErr.Pos, ":") - if len(split) <= 1 { - return pos - } - pos.Filename = split[0] - line, err := strconv.ParseInt(split[1], 10, 64) - if err != nil { - return pos - } - pos.Line = int(line) - if len(split) == 3 { - col, err := strconv.ParseInt(split[2], 10, 64) - if err != nil { - return pos - } - pos.Column = int(col) + remainder1, first, hasLine := chop(pkgErr.Pos) + remainder2, second, hasColumn := chop(remainder1) + if hasLine && hasColumn { + pos.Filename = remainder2 + pos.Line = second + pos.Column = first + } else if hasLine { + pos.Filename = remainder1 + pos.Line = first } return pos - +} + +func chop(text string) (remainder string, value int, ok bool) { + i := strings.LastIndex(text, ":") + if i < 0 { + return text, 0, false + } + v, err := strconv.ParseInt(text[i+1:], 10, 64) + if err != nil { + return text, 0, false + } + return text[:i], int(v), true } diff --git a/internal/lsp/diagnostics_test.go b/internal/lsp/diagnostics_test.go new file mode 100644 index 00000000..9f585844 --- /dev/null +++ b/internal/lsp/diagnostics_test.go @@ -0,0 +1,87 @@ +package lsp + +import ( + "go/token" + "reflect" + "sort" + "strings" + "testing" + + "golang.org/x/tools/go/packages" + "golang.org/x/tools/go/packages/packagestest" + "golang.org/x/tools/internal/lsp/protocol" +) + +func TestDiagnostics(t *testing.T) { + packagestest.TestAll(t, testDiagnostics) +} + +func testDiagnostics(t *testing.T, exporter packagestest.Exporter) { + files := packagestest.MustCopyFileTree("testdata/diagnostics") + // TODO(rstambler): Stop hardcoding this if we have more files that don't parse. + files["noparse/noparse.go"] = packagestest.Copy("testdata/diagnostics/noparse/noparse.go.in") + modules := []packagestest.Module{ + { + Name: "golang.org/x/tools/internal/lsp", + Files: files, + }, + } + exported := packagestest.Export(t, exporter, modules) + defer exported.Cleanup() + + wants := make(map[string][]protocol.Diagnostic) + for _, module := range modules { + for fragment := range module.Files { + if !strings.HasSuffix(fragment, ".go") { + continue + } + filename := exporter.Filename(exported, module.Name, fragment) + wants[filename] = []protocol.Diagnostic{} + } + } + err := exported.Expect(map[string]interface{}{ + "diag": func(pos token.Position, msg string) { + line := float64(pos.Line - 1) + col := float64(pos.Column - 1) + want := protocol.Diagnostic{ + Range: protocol.Range{ + Start: protocol.Position{ + Line: line, + Character: col, + }, + End: protocol.Position{ + Line: line, + Character: col, + }, + }, + Severity: protocol.SeverityError, + Source: "LSP: Go compiler", + Message: msg, + } + if _, ok := wants[pos.Filename]; ok { + wants[pos.Filename] = append(wants[pos.Filename], want) + } else { + t.Errorf("unexpected filename: %v", pos.Filename) + } + }, + }) + if err != nil { + t.Fatal(err) + } + v := newView() + v.config = exported.Config + v.config.Mode = packages.LoadSyntax + for filename, want := range wants { + diagnostics, err := v.diagnostics(filenameToURI(filename)) + if err != nil { + t.Fatal(err) + } + got := diagnostics[filename] + sort.Slice(got, func(i int, j int) bool { + return got[i].Range.Start.Line < got[j].Range.Start.Line + }) + if equal := reflect.DeepEqual(want, got); !equal { + t.Errorf("diagnostics failed for %s: (expected: %v), (got: %v)", filename, want, got) + } + } +} diff --git a/internal/lsp/server.go b/internal/lsp/server.go index c2462707..58cecde6 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -16,9 +16,7 @@ import ( // RunServer starts an LSP server on the supplied stream, and waits until the // stream is closed. func RunServer(ctx context.Context, stream jsonrpc2.Stream, opts ...interface{}) error { - s := &server{ - view: newView(), - } + s := &server{} conn, client := protocol.RunServer(ctx, stream, s, opts...) s.client = client return conn.Wait(ctx) @@ -39,6 +37,7 @@ func (s *server) Initialize(ctx context.Context, params *protocol.InitializePara if s.initialized { return nil, jsonrpc2.NewErrorf(jsonrpc2.CodeInvalidRequest, "server already initialized") } + s.view = newView() s.initialized = true return &protocol.InitializeResult{ Capabilities: protocol.ServerCapabilities{ diff --git a/internal/lsp/testdata/diagnostics/bad/bad.go b/internal/lsp/testdata/diagnostics/bad/bad.go new file mode 100644 index 00000000..abbf57e7 --- /dev/null +++ b/internal/lsp/testdata/diagnostics/bad/bad.go @@ -0,0 +1,19 @@ +package bad + +func stuff() { + x := "heeeeyyyy" + random2(x) //@diag("x", "cannot use x (variable of type string) as int value in argument to random2") + random2(1) + y := 3 //@diag("y", "y declared but not used") +} + +type bob struct { + x int +} + +func _() { + var q int + _ = &bob{ + f: q, //@diag("f", "unknown field f in struct literal") + } +} diff --git a/internal/lsp/testdata/diagnostics/bad/bad_util.go b/internal/lsp/testdata/diagnostics/bad/bad_util.go new file mode 100644 index 00000000..9ab8a73e --- /dev/null +++ b/internal/lsp/testdata/diagnostics/bad/bad_util.go @@ -0,0 +1,6 @@ +package bad + +func random2(y int) int { + x := 6 //@diag("x", "x declared but not used") + return y +} diff --git a/internal/lsp/testdata/diagnostics/bar/bar.go b/internal/lsp/testdata/diagnostics/bar/bar.go new file mode 100644 index 00000000..f9f99af0 --- /dev/null +++ b/internal/lsp/testdata/diagnostics/bar/bar.go @@ -0,0 +1,7 @@ +package bar + +import "golang.org/x/tools/internal/lsp/foo" + +func Bar() { + foo.Foo() +} diff --git a/internal/lsp/testdata/diagnostics/baz/baz.go b/internal/lsp/testdata/diagnostics/baz/baz.go new file mode 100644 index 00000000..3b95ee2f --- /dev/null +++ b/internal/lsp/testdata/diagnostics/baz/baz.go @@ -0,0 +1,7 @@ +package baz + +import "golang.org/x/tools/internal/lsp/bar" + +func Baz() { + bar.Bar() +} diff --git a/internal/lsp/testdata/diagnostics/foo/foo.go b/internal/lsp/testdata/diagnostics/foo/foo.go new file mode 100644 index 00000000..18be5d37 --- /dev/null +++ b/internal/lsp/testdata/diagnostics/foo/foo.go @@ -0,0 +1,3 @@ +package foo + +func Foo() {} diff --git a/internal/lsp/testdata/diagnostics/good/good.go b/internal/lsp/testdata/diagnostics/good/good.go new file mode 100644 index 00000000..14b41e63 --- /dev/null +++ b/internal/lsp/testdata/diagnostics/good/good.go @@ -0,0 +1,6 @@ +package good + +func stuff() { + x := 5 + random2(x) +} diff --git a/internal/lsp/testdata/diagnostics/good/good_util.go b/internal/lsp/testdata/diagnostics/good/good_util.go new file mode 100644 index 00000000..ae71ad43 --- /dev/null +++ b/internal/lsp/testdata/diagnostics/good/good_util.go @@ -0,0 +1,10 @@ +package good + +func random() int { + y := 6 + 7 + return y +} + +func random2(y int) int { + return y +} diff --git a/internal/lsp/testdata/diagnostics/noparse/noparse.go.in b/internal/lsp/testdata/diagnostics/noparse/noparse.go.in new file mode 100644 index 00000000..9dede9c7 --- /dev/null +++ b/internal/lsp/testdata/diagnostics/noparse/noparse.go.in @@ -0,0 +1,11 @@ +package noparse + +func bye(x int) { + hi() +} + +func stuff() { + x := 5 +} + +func .() {} //@diag(".", "expected 'IDENT', found '.'") diff --git a/internal/lsp/view.go b/internal/lsp/view.go index 825981fc..19400d68 100644 --- a/internal/lsp/view.go +++ b/internal/lsp/view.go @@ -13,14 +13,21 @@ import ( type view struct { activeFilesMu sync.Mutex activeFiles map[protocol.DocumentURI][]byte + config *packages.Config fset *token.FileSet } func newView() *view { + fset := token.NewFileSet() return &view{ + config: &packages.Config{ + Mode: packages.LoadSyntax, + Fset: fset, + Tests: true, + }, activeFiles: make(map[protocol.DocumentURI][]byte), - fset: token.NewFileSet(), + fset: fset, } } @@ -55,14 +62,12 @@ func (v *view) clearActiveFile(uri protocol.DocumentURI) { // typeCheck type-checks the package for the given package path. func (v *view) typeCheck(uri protocol.DocumentURI) (*packages.Package, error) { - cfg := &packages.Config{ - Mode: packages.LoadSyntax, - Fset: v.fset, - Overlay: v.overlay(), - Tests: true, - } - pkgs, err := packages.Load(cfg, fmt.Sprintf("file=%s", uriToFilename(uri))) + v.config.Overlay = v.overlay() + pkgs, err := packages.Load(v.config, fmt.Sprintf("file=%s", uriToFilename(uri))) if len(pkgs) == 0 { + if err == nil { + err = fmt.Errorf("no packages found for %s", uri) + } return nil, err } pkg := pkgs[0]