From 6d2eea543038d3d4fee45394e58b6bb7e3b7124e Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 20 Oct 2020 10:17:44 -0400 Subject: [PATCH] internal/typesinternal: use Go 1.16 metadata for go/types errors In Go 1.16 error codes as well as start and end positions are added for go/types errors. This information is temporarily stored in unexported fields, until we're more confident in the correctness of both the API and the underlying data. Read this information using reflection and, if available, use it to set the corresponding field in compiler diagnostics. This establishes a positive feedback loop: in most cases this should improve the gopls diagnostic, and wherever it doesn't we can make a note and fall back on the old heuristics for that error code. For golang/go#42290 Change-Id: I37475189cbd14a0a5bcfde163f599c9a7b957372 Reviewed-on: https://go-review.googlesource.com/c/tools/+/268539 Run-TryBot: Robert Findley TryBot-Result: Go Bot gopls-CI: kokoro Trust: Robert Findley Reviewed-by: Rebecca Stambler --- internal/lsp/cache/errors.go | 51 ++++++++++++++++++++++-------- internal/lsp/diagnostics.go | 13 ++++++-- internal/lsp/source/diagnostics.go | 2 ++ internal/lsp/source/view.go | 6 +++- internal/lsp/testdata/bad/bad0.go | 2 +- internal/lsp/tests/util.go | 30 ++++++++++++++---- internal/span/token.go | 36 +++++++++++++-------- internal/typesinternal/types.go | 17 ++++++++++ 8 files changed, 119 insertions(+), 38 deletions(-) diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index ffc236d26c..10f7d1f014 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -22,6 +22,7 @@ import ( "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" + "golang.org/x/tools/internal/typesinternal" errors "golang.org/x/xerrors" ) @@ -31,6 +32,7 @@ func sourceError(ctx context.Context, snapshot *snapshot, pkg *pkg, e interface{ spn span.Span err error msg, category string + code typesinternal.ErrorCode kind source.ErrorKind fixes []source.SuggestedFix related []source.RelatedInformation @@ -90,7 +92,7 @@ func sourceError(ctx context.Context, snapshot *snapshot, pkg *pkg, e interface{ if !e.Pos.IsValid() { return nil, fmt.Errorf("invalid position for type error %v", e) } - spn, err = typeErrorRange(snapshot, fset, pkg, e.Pos) + code, spn, err = typeErrorData(fset, pkg, e) if err != nil { return nil, err } @@ -101,14 +103,14 @@ func sourceError(ctx context.Context, snapshot *snapshot, pkg *pkg, e interface{ if !perr.Pos.IsValid() { return nil, fmt.Errorf("invalid position for type error %v", e) } - spn, err = typeErrorRange(snapshot, fset, pkg, perr.Pos) + code, spn, err = typeErrorData(fset, pkg, e.primary) if err != nil { return nil, err } for _, s := range e.secondaries { var x source.RelatedInformation x.Message = s.Msg - xspn, err := typeErrorRange(snapshot, fset, pkg, s.Pos) + _, xspn, err := typeErrorData(fset, pkg, s) if err != nil { return nil, fmt.Errorf("invalid position for type error %v", s) } @@ -143,7 +145,7 @@ func sourceError(ctx context.Context, snapshot *snapshot, pkg *pkg, e interface{ if err != nil { return nil, err } - return &source.Error{ + se := &source.Error{ URI: spn.URI(), Range: rng, Message: msg, @@ -151,7 +153,17 @@ func sourceError(ctx context.Context, snapshot *snapshot, pkg *pkg, e interface{ Category: category, SuggestedFixes: fixes, Related: related, - }, nil + } + if code != 0 { + se.Code = code.String() + se.CodeHref = typesCodeHref(snapshot, code) + } + return se, nil +} + +func typesCodeHref(snapshot *snapshot, code typesinternal.ErrorCode) string { + target := snapshot.View().Options().LinkTarget + return fmt.Sprintf("%s/golang.org/x/tools/internal/typesinternal#%s", target, code.String()) } func suggestedAnalysisFixes(snapshot *snapshot, pkg *pkg, diag *analysis.Diagnostic) ([]source.SuggestedFix, error) { @@ -213,18 +225,29 @@ func toSourceErrorKind(kind packages.ErrorKind) source.ErrorKind { } } -func typeErrorRange(snapshot *snapshot, fset *token.FileSet, pkg *pkg, pos token.Pos) (span.Span, error) { - posn := fset.Position(pos) +func typeErrorData(fset *token.FileSet, pkg *pkg, terr types.Error) (typesinternal.ErrorCode, span.Span, error) { + ecode, start, end, ok := typesinternal.ReadGo116ErrorData(terr) + if !ok { + start, end = terr.Pos, terr.Pos + ecode = 0 + } + posn := fset.Position(start) pgf, err := pkg.File(span.URIFromPath(posn.Filename)) if err != nil { - return span.Span{}, err + return 0, span.Span{}, err } - return span.Range{ - FileSet: fset, - Start: pos, - End: analysisinternal.TypeErrorEndPos(fset, pgf.Src, pos), - Converter: pgf.Mapper.Converter, - }.Span() + if !end.IsValid() || end == start { + end = analysisinternal.TypeErrorEndPos(fset, pgf.Src, start) + } + spn, err := parsedGoSpan(pgf, start, end) + if err != nil { + return 0, span.Span{}, err + } + return ecode, spn, nil +} + +func parsedGoSpan(pgf *source.ParsedGoFile, start, end token.Pos) (span.Span, error) { + return span.FileSpan(pgf.Tok, pgf.Mapper.Converter, start, end) } func scannerErrorRange(snapshot *snapshot, pkg *pkg, posn token.Position) (span.Span, error) { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 3b1ab9b011..7e6acb123e 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -445,6 +445,8 @@ func (s *Server) storeErrorDiagnostics(ctx context.Context, snapshot source.Snap Related: e.Related, Severity: protocol.SeverityError, Source: e.Category, + Code: e.Code, + CodeHref: e.CodeHref, } s.storeDiagnostics(snapshot, e.URI, dsource, []*source.Diagnostic{diagnostic}) } @@ -534,7 +536,7 @@ func toProtocolDiagnostics(diagnostics []*source.Diagnostic) []protocol.Diagnost Message: rel.Message, }) } - reports = append(reports, protocol.Diagnostic{ + pdiag := protocol.Diagnostic{ // diag.Message might start with \n or \t Message: strings.TrimSpace(diag.Message), Range: diag.Range, @@ -542,7 +544,14 @@ func toProtocolDiagnostics(diagnostics []*source.Diagnostic) []protocol.Diagnost Source: diag.Source, Tags: diag.Tags, RelatedInformation: related, - }) + } + if diag.Code != "" { + pdiag.Code = diag.Code + } + if diag.CodeHref != "" { + pdiag.CodeDescription = &protocol.CodeDescription{Href: diag.CodeHref} + } + reports = append(reports, pdiag) } return reports } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index a9bffbdb9e..1dd937a543 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -18,6 +18,8 @@ type Diagnostic struct { Range protocol.Range Message string Source string + Code string + CodeHref string Severity protocol.DiagnosticSeverity Tags []protocol.DiagnosticTag diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 8b7673d9d5..ff9dade5fc 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -579,7 +579,11 @@ type Error struct { Kind ErrorKind Message string Category string // only used by analysis errors so far - Related []RelatedInformation + + Related []RelatedInformation + + Code string + CodeHref string // SuggestedFixes is used to generate quick fixes for a CodeAction request. // It isn't part of the Diagnostic type. diff --git a/internal/lsp/testdata/bad/bad0.go b/internal/lsp/testdata/bad/bad0.go index cfde87c87c..36a4e6b95f 100644 --- a/internal/lsp/testdata/bad/bad0.go +++ b/internal/lsp/testdata/bad/bad0.go @@ -18,6 +18,6 @@ type bob struct { //@item(bob, "bob", "struct{...}", "struct") func _() { var q int _ = &bob{ - f: q, //@diag("f", "compiler", "unknown field f in struct literal", "error") + f: q, //@diag("f: q", "compiler", "unknown field f in struct literal", "error") } } diff --git a/internal/lsp/tests/util.go b/internal/lsp/tests/util.go index 9fed4deb77..b46f15980e 100644 --- a/internal/lsp/tests/util.go +++ b/internal/lsp/tests/util.go @@ -129,18 +129,34 @@ func DiffDiagnostics(uri span.URI, want, got []*source.Diagnostic) string { if w.Source != g.Source { return summarizeDiagnostics(i, uri, want, got, "incorrect Source got %v want %v", g.Source, w.Source) } - if protocol.ComparePosition(w.Range.Start, g.Range.Start) != 0 { - return summarizeDiagnostics(i, uri, want, got, "incorrect Start got %v want %v", g.Range.Start, w.Range.Start) - } - if !protocol.IsPoint(g.Range) { // Accept any 'want' range if the diagnostic returns a zero-length range. - if protocol.ComparePosition(w.Range.End, g.Range.End) != 0 { - return summarizeDiagnostics(i, uri, want, got, "incorrect End got %v want %v", g.Range.End, w.Range.End) - } + if !rangeOverlaps(g.Range, w.Range) { + return summarizeDiagnostics(i, uri, want, got, "range %v does not overlap %v", g.Range, w.Range) } } return "" } +// rangeOverlaps reports whether r1 and r2 overlap. +func rangeOverlaps(r1, r2 protocol.Range) bool { + if inRange(r2.Start, r1) || inRange(r1.Start, r2) { + return true + } + return false +} + +// inRange reports whether p is contained within [r.Start, r.End), or if p == +// r.Start == r.End (special handling for the case where the range is a single +// point). +func inRange(p protocol.Position, r protocol.Range) bool { + if protocol.IsPoint(r) { + return protocol.ComparePosition(r.Start, p) == 0 + } + if protocol.ComparePosition(r.Start, p) <= 0 && protocol.ComparePosition(p, r.End) < 0 { + return true + } + return false +} + func summarizeDiagnostics(i int, uri span.URI, want, got []*source.Diagnostic, reason string, args ...interface{}) string { msg := &bytes.Buffer{} fmt.Fprint(msg, "diagnostics failed") diff --git a/internal/span/token.go b/internal/span/token.go index 10b429efd1..6f8b9b570c 100644 --- a/internal/span/token.go +++ b/internal/span/token.go @@ -19,12 +19,16 @@ type Range struct { Converter Converter } +type FileConverter struct { + file *token.File +} + // TokenConverter is a Converter backed by a token file set and file. // It uses the file set methods to work out the conversions, which // makes it fast and does not require the file contents. type TokenConverter struct { + FileConverter fset *token.FileSet - file *token.File } // NewRange creates a new Range from a FileSet and two positions. @@ -40,7 +44,7 @@ func NewRange(fset *token.FileSet, start, end token.Pos) Range { // NewTokenConverter returns an implementation of Converter backed by a // token.File. func NewTokenConverter(fset *token.FileSet, f *token.File) *TokenConverter { - return &TokenConverter{fset: fset, file: f} + return &TokenConverter{fset: fset, FileConverter: FileConverter{file: f}} } // NewContentConverter returns an implementation of Converter for the @@ -49,7 +53,7 @@ func NewContentConverter(filename string, content []byte) *TokenConverter { fset := token.NewFileSet() f := fset.AddFile(filename, -1, len(content)) f.SetLinesForContent(content) - return &TokenConverter{fset: fset, file: f} + return NewTokenConverter(fset, f) } // IsPoint returns true if the range represents a single point. @@ -68,17 +72,23 @@ func (r Range) Span() (Span, error) { if f == nil { return Span{}, fmt.Errorf("file not found in FileSet") } + return FileSpan(f, r.Converter, r.Start, r.End) +} + +// FileSpan returns a span within tok, using converter to translate between +// offsets and positions. +func FileSpan(tok *token.File, converter Converter, start, end token.Pos) (Span, error) { var s Span var err error var startFilename string - startFilename, s.v.Start.Line, s.v.Start.Column, err = position(f, r.Start) + startFilename, s.v.Start.Line, s.v.Start.Column, err = position(tok, start) if err != nil { return Span{}, err } s.v.URI = URIFromPath(startFilename) - if r.End.IsValid() { + if end.IsValid() { var endFilename string - endFilename, s.v.End.Line, s.v.End.Column, err = position(f, r.End) + endFilename, s.v.End.Line, s.v.End.Column, err = position(tok, end) if err != nil { return Span{}, err } @@ -91,13 +101,13 @@ func (r Range) Span() (Span, error) { s.v.Start.clean() s.v.End.clean() s.v.clean() - if r.Converter != nil { - return s.WithOffset(r.Converter) + if converter != nil { + return s.WithOffset(converter) } - if startFilename != f.Name() { - return Span{}, fmt.Errorf("must supply Converter for file %q containing lines from %q", f.Name(), startFilename) + if startFilename != tok.Name() { + return Span{}, fmt.Errorf("must supply Converter for file %q containing lines from %q", tok.Name(), startFilename) } - return s.WithOffset(NewTokenConverter(r.FileSet, f)) + return s.WithOffset(&FileConverter{tok}) } func position(f *token.File, pos token.Pos) (string, int, int, error) { @@ -154,12 +164,12 @@ func (s Span) Range(converter *TokenConverter) (Range, error) { }, nil } -func (l *TokenConverter) ToPosition(offset int) (int, int, error) { +func (l *FileConverter) ToPosition(offset int) (int, int, error) { _, line, col, err := positionFromOffset(l.file, offset) return line, col, err } -func (l *TokenConverter) ToOffset(line, col int) (int, error) { +func (l *FileConverter) ToOffset(line, col int) (int, error) { if line < 0 { return -1, fmt.Errorf("line is not valid") } diff --git a/internal/typesinternal/types.go b/internal/typesinternal/types.go index a5bb408e2f..c3e1a397db 100644 --- a/internal/typesinternal/types.go +++ b/internal/typesinternal/types.go @@ -2,9 +2,12 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. +// Package typesinternal provides access to internal go/types APIs that are not +// yet exported. package typesinternal import ( + "go/token" "go/types" "reflect" "unsafe" @@ -26,3 +29,17 @@ func SetUsesCgo(conf *types.Config) bool { return true } + +func ReadGo116ErrorData(terr types.Error) (ErrorCode, token.Pos, token.Pos, bool) { + var data [3]int + // By coincidence all of these fields are ints, which simplifies things. + v := reflect.ValueOf(terr) + for i, name := range []string{"go116code", "go116start", "go116end"} { + f := v.FieldByName(name) + if !f.IsValid() { + return 0, 0, 0, false + } + data[i] = int(f.Int()) + } + return ErrorCode(data[0]), token.Pos(data[1]), token.Pos(data[2]), true +}