internal/lsp: use version numbers in diagnostic messages
This change uses the FileIdentity when reporting an error message, so
that the version number can be propagated to through the
publishDiagnostics notification.
Change-Id: I6a2103e304717ca09895008ea40336e3ace3c66d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208260
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go
index 5c377af..6fdee56 100644
--- a/internal/lsp/cache/errors.go
+++ b/internal/lsp/cache/errors.go
@@ -87,8 +87,12 @@
if err != nil {
return nil, err
}
+ ph, err := pkg.File(spn.URI())
+ if err != nil {
+ return nil, err
+ }
return &source.Error{
- URI: spn.URI(),
+ File: ph.File().Identity(),
Range: rng,
Message: msg,
Kind: kind,
diff --git a/internal/lsp/cmd/test/check.go b/internal/lsp/cmd/test/check.go
index ad260d2..74b27cb 100644
--- a/internal/lsp/cmd/test/check.go
+++ b/internal/lsp/cmd/test/check.go
@@ -51,12 +51,12 @@
got[l] = struct{}{}
}
for _, diag := range want {
- expect := fmt.Sprintf("%v:%v:%v: %v", diag.URI.Filename(), diag.Range.Start.Line+1, diag.Range.Start.Character+1, diag.Message)
+ expect := fmt.Sprintf("%v:%v:%v: %v", diag.File.URI.Filename(), diag.Range.Start.Line+1, diag.Range.Start.Character+1, diag.Message)
if diag.Range.Start.Character == 0 {
- expect = fmt.Sprintf("%v:%v: %v", diag.URI.Filename(), diag.Range.Start.Line+1, diag.Message)
+ expect = fmt.Sprintf("%v:%v: %v", diag.File.URI.Filename(), diag.Range.Start.Line+1, diag.Message)
}
// Skip the badimport test for now, until we do a better job with diagnostic ranges.
- if strings.Contains(diag.URI.Filename(), "badimport") {
+ if strings.Contains(diag.File.URI.Filename(), "badimport") {
continue
}
_, found := got[expect]
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index a77b490..8334d6d 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -55,18 +55,18 @@
s.undeliveredMu.Lock()
defer s.undeliveredMu.Unlock()
- for uri, diagnostics := range reports {
- if err := s.publishDiagnostics(ctx, uri, diagnostics); err != nil {
+ for fileID, diagnostics := range reports {
+ if err := s.publishDiagnostics(ctx, fileID, diagnostics); err != nil {
if s.undelivered == nil {
- s.undelivered = make(map[span.URI][]source.Diagnostic)
+ s.undelivered = make(map[source.FileIdentity][]source.Diagnostic)
}
- s.undelivered[uri] = diagnostics
+ s.undelivered[fileID] = diagnostics
log.Error(ctx, "failed to deliver diagnostic (will retry)", err, telemetry.File)
continue
}
// In case we had old, undelivered diagnostics.
- delete(s.undelivered, uri)
+ delete(s.undelivered, fileID)
}
// Anytime we compute diagnostics, make sure to also send along any
// undelivered ones (only for remaining URIs).
@@ -81,10 +81,11 @@
return nil
}
-func (s *Server) publishDiagnostics(ctx context.Context, uri span.URI, diagnostics []source.Diagnostic) error {
+func (s *Server) publishDiagnostics(ctx context.Context, fileID source.FileIdentity, diagnostics []source.Diagnostic) error {
s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
Diagnostics: toProtocolDiagnostics(ctx, diagnostics),
- URI: protocol.NewURI(uri),
+ URI: protocol.NewURI(fileID.URI),
+ Version: fileID.Version,
})
return nil
}
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index 2895c9e..e4cb28b 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -62,7 +62,7 @@
r := &runner{
server: &Server{
session: session,
- undelivered: make(map[span.URI][]source.Diagnostic),
+ undelivered: make(map[source.FileIdentity][]source.Diagnostic),
},
data: data,
ctx: ctx,
@@ -78,11 +78,12 @@
if err != nil {
t.Fatalf("no file for %s: %v", f, err)
}
+ identity := v.Snapshot().Handle(r.ctx, f).Identity()
results, _, err := source.Diagnostics(r.ctx, v.Snapshot(), f, nil)
if err != nil {
t.Fatal(err)
}
- got := results[uri]
+ got := results[identity]
// A special case to test that there are no diagnostics for a file.
if len(want) == 1 && want[0].Source == "no_diagnostics" {
if len(got) != 0 {
@@ -336,7 +337,9 @@
if err != nil {
t.Fatal(err)
}
- diagnostics, _, err := source.Diagnostics(r.ctx, view.Snapshot(), f, nil)
+ snapshot := view.Snapshot()
+ fileID := snapshot.Handle(r.ctx, f).Identity()
+ diagnostics, _, err := source.Diagnostics(r.ctx, snapshot, f, nil)
if err != nil {
t.Fatal(err)
}
@@ -346,7 +349,7 @@
},
Context: protocol.CodeActionContext{
Only: []protocol.CodeActionKind{protocol.QuickFix},
- Diagnostics: toProtocolDiagnostics(r.ctx, diagnostics[uri]),
+ Diagnostics: toProtocolDiagnostics(r.ctx, diagnostics[fileID]),
},
})
if err != nil {
diff --git a/internal/lsp/server.go b/internal/lsp/server.go
index c13047c..dacfd48 100644
--- a/internal/lsp/server.go
+++ b/internal/lsp/server.go
@@ -14,7 +14,6 @@
"golang.org/x/tools/internal/jsonrpc2"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
- "golang.org/x/tools/internal/span"
)
// NewClientServer
@@ -82,7 +81,7 @@
// undelivered is a cache of any diagnostics that the server
// failed to deliver for some reason.
undeliveredMu sync.Mutex
- undelivered map[span.URI][]source.Diagnostic
+ undelivered map[source.FileIdentity][]source.Diagnostic
// folders is only valid between initialize and initialized, and holds the
// set of folders to build views for when we are ready
diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go
index b112c08..a3c8e8c 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -17,7 +17,7 @@
)
type Diagnostic struct {
- URI span.URI
+ File FileIdentity
Range protocol.Range
Message string
Source string
@@ -39,10 +39,11 @@
Message string
}
-func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyses map[string]struct{}) (map[span.URI][]Diagnostic, string, error) {
+func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyses map[string]struct{}) (map[FileIdentity][]Diagnostic, string, error) {
ctx, done := trace.StartSpan(ctx, "source.Diagnostics", telemetry.File.Of(f.URI()))
defer done()
+ fh := snapshot.Handle(ctx, f)
cphs, err := snapshot.PackageHandles(ctx, f)
if err != nil {
return nil, "", err
@@ -51,7 +52,6 @@
if err != nil {
return nil, "", err
}
-
// If we are missing dependencies, it may because the user's workspace is
// not correctly configured. Report errors, if possible.
var warningMsg string
@@ -64,21 +64,21 @@
pkg, err := cph.Check(ctx)
if err != nil {
log.Error(ctx, "no package for file", err)
- return singleDiagnostic(f.URI(), "%s is not part of a package", f.URI()), "", nil
+ return singleDiagnostic(fh.Identity(), "%s is not part of a package", f.URI()), "", nil
}
// Prepare the reports we will send for the files in this package.
- reports := make(map[span.URI][]Diagnostic)
+ reports := make(map[FileIdentity][]Diagnostic)
for _, fh := range pkg.CompiledGoFiles() {
- clearReports(snapshot, reports, fh.File().Identity().URI)
+ clearReports(snapshot, reports, fh.File().Identity())
}
// Prepare any additional reports for the errors in this package.
- for _, err := range pkg.GetErrors() {
- if err.Kind != ListError {
+ for _, e := range pkg.GetErrors() {
+ if e.Kind != ListError {
continue
}
- clearReports(snapshot, reports, err.URI)
+ clearReports(snapshot, reports, e.File)
}
// Run diagnostics for the package that this URI belongs to.
@@ -96,7 +96,7 @@
return nil, warningMsg, err
}
for _, fh := range pkg.CompiledGoFiles() {
- clearReports(snapshot, reports, fh.File().Identity().URI)
+ clearReports(snapshot, reports, fh.File().Identity())
}
diagnostics(ctx, pkg, reports)
}
@@ -107,25 +107,25 @@
listErrors, parseErrors, typeErrors []*Diagnostic
}
-func diagnostics(ctx context.Context, pkg Package, reports map[span.URI][]Diagnostic) bool {
+func diagnostics(ctx context.Context, pkg Package, reports map[FileIdentity][]Diagnostic) bool {
ctx, done := trace.StartSpan(ctx, "source.diagnostics", telemetry.Package.Of(pkg.ID()))
_ = ctx // circumvent SA4006
defer done()
- diagSets := make(map[span.URI]*diagnosticSet)
- for _, err := range pkg.GetErrors() {
+ diagSets := make(map[FileIdentity]*diagnosticSet)
+ for _, e := range pkg.GetErrors() {
diag := &Diagnostic{
- URI: err.URI,
- Message: err.Message,
- Range: err.Range,
+ File: e.File,
+ Message: e.Message,
+ Range: e.Range,
Severity: protocol.SeverityError,
}
- set, ok := diagSets[diag.URI]
+ set, ok := diagSets[e.File]
if !ok {
set = &diagnosticSet{}
- diagSets[diag.URI] = set
+ diagSets[e.File] = set
}
- switch err.Kind {
+ switch e.Kind {
case ParseError:
set.parseErrors = append(set.parseErrors, diag)
diag.Source = "syntax"
@@ -138,7 +138,7 @@
}
}
var nonEmptyDiagnostics bool // track if we actually send non-empty diagnostics
- for uri, set := range diagSets {
+ for fileID, set := range diagSets {
// Don't report type errors if there are parse errors or list errors.
diags := set.typeErrors
if len(set.parseErrors) > 0 {
@@ -150,15 +150,15 @@
nonEmptyDiagnostics = true
}
for _, diag := range diags {
- if _, ok := reports[uri]; ok {
- reports[uri] = append(reports[uri], *diag)
+ if _, ok := reports[fileID]; ok {
+ reports[fileID] = append(reports[fileID], *diag)
}
}
}
return nonEmptyDiagnostics
}
-func analyses(ctx context.Context, snapshot Snapshot, cph CheckPackageHandle, disabledAnalyses map[string]struct{}, reports map[span.URI][]Diagnostic) error {
+func analyses(ctx context.Context, snapshot Snapshot, cph CheckPackageHandle, disabledAnalyses map[string]struct{}, reports map[FileIdentity][]Diagnostic) error {
var analyzers []*analysis.Analyzer
for _, a := range snapshot.View().Options().Analyzers {
if _, ok := disabledAnalyses[a.Name]; ok {
@@ -181,8 +181,8 @@
if onlyDeletions(e.SuggestedFixes) {
tags = append(tags, protocol.Unnecessary)
}
- addReport(snapshot, reports, Diagnostic{
- URI: e.URI,
+ addReport(ctx, snapshot, reports, Diagnostic{
+ File: e.File,
Range: e.Range,
Message: e.Message,
Source: e.Category,
@@ -195,27 +195,28 @@
return nil
}
-func clearReports(snapshot Snapshot, reports map[span.URI][]Diagnostic, uri span.URI) {
- if snapshot.View().Ignore(uri) {
+func clearReports(snapshot Snapshot, reports map[FileIdentity][]Diagnostic, fileID FileIdentity) {
+ if snapshot.View().Ignore(fileID.URI) {
return
}
- reports[uri] = []Diagnostic{}
+ reports[fileID] = []Diagnostic{}
}
-func addReport(snapshot Snapshot, reports map[span.URI][]Diagnostic, diagnostic Diagnostic) {
- if snapshot.View().Ignore(diagnostic.URI) {
- return
+func addReport(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]Diagnostic, diagnostic Diagnostic) error {
+ if snapshot.View().Ignore(diagnostic.File.URI) {
+ return nil
}
- if _, ok := reports[diagnostic.URI]; ok {
- reports[diagnostic.URI] = append(reports[diagnostic.URI], diagnostic)
+ if _, ok := reports[diagnostic.File]; ok {
+ reports[diagnostic.File] = append(reports[diagnostic.File], diagnostic)
}
+ return nil
}
-func singleDiagnostic(uri span.URI, format string, a ...interface{}) map[span.URI][]Diagnostic {
- return map[span.URI][]Diagnostic{
- uri: []Diagnostic{{
- Source: "LSP",
- URI: uri,
+func singleDiagnostic(fileID FileIdentity, format string, a ...interface{}) map[FileIdentity][]Diagnostic {
+ return map[FileIdentity][]Diagnostic{
+ fileID: []Diagnostic{{
+ File: fileID,
+ Source: "gopls",
Range: protocol.Range{},
Message: fmt.Sprintf(format, a...),
Severity: protocol.SeverityError,
diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go
index 021ec71..51059f3 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -381,8 +381,8 @@
// hasParseErrors returns true if the given file has parse errors.
func hasParseErrors(pkg Package, uri span.URI) bool {
- for _, err := range pkg.GetErrors() {
- if err.URI == uri && err.Kind == ParseError {
+ for _, e := range pkg.GetErrors() {
+ if e.File.URI == uri && e.Kind == ParseError {
return true
}
}
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index fb825a7..b2c0f53 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -71,11 +71,13 @@
if err != nil {
t.Fatal(err)
}
- results, _, err := source.Diagnostics(r.ctx, r.view.Snapshot(), f, nil)
+ snapshot := r.view.Snapshot()
+ fileID := snapshot.Handle(r.ctx, f).Identity()
+ results, _, err := source.Diagnostics(r.ctx, snapshot, f, nil)
if err != nil {
t.Fatal(err)
}
- got := results[uri]
+ got := results[fileID]
// A special case to test that there are no diagnostics for a file.
if len(want) == 1 && want[0].Source == "no_diagnostics" {
if len(got) != 0 {
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index b41b8cf..be5c393 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -287,8 +287,8 @@
Kind FileKind
}
-func (identity FileIdentity) String() string {
- return fmt.Sprintf("%s%f%s%s", identity.URI, identity.Version, identity.Identifier, identity.Kind)
+func (fileID FileIdentity) String() string {
+ return fmt.Sprintf("%s%f%s%s", fileID.URI, fileID.Version, fileID.Identifier, fileID.Kind)
}
// File represents a source file of any type.
@@ -342,7 +342,7 @@
}
type Error struct {
- URI span.URI
+ File FileIdentity
Range protocol.Range
Kind ErrorKind
Message string
@@ -362,7 +362,7 @@
)
func (e *Error) Error() string {
- return fmt.Sprintf("%s:%s: %s", e.URI, e.Range, e.Message)
+ return fmt.Sprintf("%s:%s: %s", e.File, e.Range, e.Message)
}
type BuiltinPackage interface {
diff --git a/internal/lsp/tests/diagnostics.go b/internal/lsp/tests/diagnostics.go
index 92f948c..8ec1c6e 100644
--- a/internal/lsp/tests/diagnostics.go
+++ b/internal/lsp/tests/diagnostics.go
@@ -32,7 +32,7 @@
return summarizeDiagnostics(i, want, got, "incorrect Source got %v want %v", g.Source, w.Source)
}
// Don't check the range on the badimport test.
- if strings.Contains(string(g.URI), "badimport") {
+ if strings.Contains(string(g.File.URI), "badimport") {
continue
}
if protocol.ComparePosition(w.Range.Start, g.Range.Start) != 0 {
@@ -54,7 +54,7 @@
}
func compareDiagnostic(a, b source.Diagnostic) int {
- if r := span.CompareURI(a.URI, b.URI); r != 0 {
+ if r := span.CompareURI(a.File.URI, b.File.URI); r != 0 {
return r
}
if r := protocol.CompareRange(a.Range, b.Range); r != 0 {
@@ -80,11 +80,11 @@
fmt.Fprintf(msg, reason, args...)
fmt.Fprint(msg, ":\nexpected:\n")
for _, d := range want {
- fmt.Fprintf(msg, " %s:%v: %s\n", d.URI, d.Range, d.Message)
+ fmt.Fprintf(msg, " %s:%v: %s\n", d.File.URI, d.Range, d.Message)
}
fmt.Fprintf(msg, "got:\n")
for _, d := range got {
- fmt.Fprintf(msg, " %s:%v: %s\n", d.URI, d.Range, d.Message)
+ fmt.Fprintf(msg, " %s:%v: %s\n", d.File.URI, d.Range, d.Message)
}
return msg.String()
}
diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go
index d7c75ee..6aeebbd 100644
--- a/internal/lsp/tests/tests.go
+++ b/internal/lsp/tests/tests.go
@@ -704,7 +704,9 @@
// This is not the correct way to do this,
// but it seems excessive to do the full conversion here.
want := source.Diagnostic{
- URI: spn.URI(),
+ File: source.FileIdentity{
+ URI: spn.URI(),
+ },
Range: protocol.Range{
Start: protocol.Position{
Line: float64(spn.Start().Line()) - 1,
diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go
index a5efe05..87fa0f4 100644
--- a/internal/lsp/watched_files.go
+++ b/internal/lsp/watched_files.go
@@ -70,7 +70,9 @@
// If this was the only file in the package, clear its diagnostics.
if otherFile == nil {
- if err := s.publishDiagnostics(ctx, uri, []source.Diagnostic{}); err != nil {
+ if err := s.publishDiagnostics(ctx, source.FileIdentity{
+ URI: uri,
+ }, []source.Diagnostic{}); err != nil {
log.Error(ctx, "failed to clear diagnostics", err, telemetry.URI.Of(uri))
}
return nil
