From 2bb78cf04ecd899e1e386c18ec73fca800ae2b49 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 23 May 2022 17:45:46 -0400 Subject: [PATCH] internal/lsp: add missing bug reporting, and panic during marker tests Add additional bug reporting related to position invariants, and update marker test runners to panic on encountered bugs. This revealed a panic on older Go versions, where we try to create a range for a missing package name span. Lift the check into the caller for this case, and leave a big comment explaining that checkForOrphanedFile probably shouldn't be so tolerant of invalid calls. Change-Id: Ie16a07afba0f8a5682cca50ad8f04450bfa2da65 Reviewed-on: https://go-review.googlesource.com/c/tools/+/407885 Reviewed-by: Alan Donovan Run-TryBot: Robert Findley TryBot-Result: Gopher Robot gopls-CI: kokoro --- gopls/test/gopls_test.go | 2 ++ internal/lsp/bug/bug.go | 8 ++++++++ internal/lsp/cmd/cmd_test.go | 2 ++ internal/lsp/diagnostics.go | 11 +++++++++++ internal/lsp/lsp_test.go | 2 ++ internal/lsp/source/source_test.go | 2 ++ internal/span/token.go | 12 ++++++------ 7 files changed, 33 insertions(+), 6 deletions(-) diff --git a/gopls/test/gopls_test.go b/gopls/test/gopls_test.go index fde262292c..6282224abb 100644 --- a/gopls/test/gopls_test.go +++ b/gopls/test/gopls_test.go @@ -9,6 +9,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" cmdtest "golang.org/x/tools/internal/lsp/cmd/test" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/tests" @@ -16,6 +17,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true testenv.ExitIfSmallMachine() os.Exit(m.Run()) } diff --git a/internal/lsp/bug/bug.go b/internal/lsp/bug/bug.go index a419a9c3a1..b974e88e17 100644 --- a/internal/lsp/bug/bug.go +++ b/internal/lsp/bug/bug.go @@ -49,6 +49,14 @@ func Reportf(format string, args ...interface{}) { Report(fmt.Sprintf(format, args...), nil) } +// Errorf calls fmt.Errorf for the given arguments, and reports the resulting +// error message as a bug. +func Errorf(format string, args ...interface{}) error { + err := fmt.Errorf(format, args...) + Report(err.Error(), nil) + return err +} + // Report records a new bug encountered on the server. // It uses reflection to report the position of the immediate caller. func Report(description string, data Data) { diff --git a/internal/lsp/cmd/cmd_test.go b/internal/lsp/cmd/cmd_test.go index 29816c83e2..c44bd5722c 100644 --- a/internal/lsp/cmd/cmd_test.go +++ b/internal/lsp/cmd/cmd_test.go @@ -8,12 +8,14 @@ import ( "os" "testing" + "golang.org/x/tools/internal/lsp/bug" cmdtest "golang.org/x/tools/internal/lsp/cmd/test" "golang.org/x/tools/internal/lsp/tests" "golang.org/x/tools/internal/testenv" ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true testenv.ExitIfSmallMachine() os.Exit(m.Run()) } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 3c67ad6760..7ba6a4ad7e 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -439,6 +439,14 @@ func (s *Server) showCriticalErrorStatus(ctx context.Context, snapshot source.Sn // If they cannot and the workspace is not otherwise unloaded, it also surfaces // a warning, suggesting that the user check the file for build tags. func (s *Server) checkForOrphanedFile(ctx context.Context, snapshot source.Snapshot, fh source.VersionedFileHandle) *source.Diagnostic { + // TODO(rfindley): this function may fail to produce a diagnostic for a + // variety of reasons, some of which should probably not be ignored. For + // example, should this function be tolerant of the case where fh does not + // exist, or does not have a package name? + // + // It would be better to panic or report a bug in several of the cases below, + // so that we can move toward guaranteeing we show the user a meaningful + // error whenever it makes sense. if snapshot.View().FileKind(fh) != source.Go { return nil } @@ -454,6 +462,9 @@ func (s *Server) checkForOrphanedFile(ctx context.Context, snapshot source.Snaps if err != nil { return nil } + if !pgf.File.Name.Pos().IsValid() { + return nil + } spn, err := span.NewRange(snapshot.FileSet(), pgf.File.Name.Pos(), pgf.File.Name.End()).Span() if err != nil { return nil diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index ca0985a980..636019f0ea 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -15,6 +15,7 @@ import ( "strings" "testing" + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/cache" "golang.org/x/tools/internal/lsp/command" "golang.org/x/tools/internal/lsp/diff" @@ -27,6 +28,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true testenv.ExitIfSmallMachine() os.Exit(m.Run()) } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 405c35c368..426bffc97b 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -15,6 +15,7 @@ import ( "strings" "testing" + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/cache" "golang.org/x/tools/internal/lsp/diff" "golang.org/x/tools/internal/lsp/diff/myers" @@ -28,6 +29,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true testenv.ExitIfSmallMachine() os.Exit(m.Run()) } diff --git a/internal/span/token.go b/internal/span/token.go index c2b65e7bec..a64e5e88e3 100644 --- a/internal/span/token.go +++ b/internal/span/token.go @@ -8,6 +8,8 @@ import ( "errors" "fmt" "go/token" + + "golang.org/x/tools/internal/lsp/bug" ) // Range represents a source code range in token.Pos form. @@ -29,7 +31,7 @@ type TokenConverter struct { func NewRange(fset *token.FileSet, start, end token.Pos) Range { file := fset.File(start) if file == nil { - // TODO: report a bug here via the bug reporting API, once it is available. + bug.Reportf("nil file") } return Range{ Start: start, @@ -42,7 +44,7 @@ func NewRange(fset *token.FileSet, start, end token.Pos) Range { // token.File. func NewTokenConverter(f *token.File) *TokenConverter { if f == nil { - // TODO: report a bug here using the bug reporting API. + bug.Reportf("nil file") } return &TokenConverter{file: f} } @@ -151,12 +153,10 @@ func (s Span) Range(converter *TokenConverter) (Range, error) { // go/token will panic if the offset is larger than the file's size, // so check here to avoid panicking. if s.Start().Offset() > file.Size() { - // TODO: report a bug here via the future bug reporting API. - return Range{}, fmt.Errorf("start offset %v is past the end of the file %v", s.Start(), file.Size()) + return Range{}, bug.Errorf("start offset %v is past the end of the file %v", s.Start(), file.Size()) } if s.End().Offset() > file.Size() { - // TODO: report a bug here via the future bug reporting API. - return Range{}, fmt.Errorf("end offset %v is past the end of the file %v", s.End(), file.Size()) + return Range{}, bug.Errorf("end offset %v is past the end of the file %v", s.End(), file.Size()) } return Range{ Start: file.Pos(s.Start().Offset()),