internal/lsp/semantic.go: repress useless messages and tighten logic
Error messages associated with bad parses and when working at the end
of the file are not useful to users, so they are no longer generated.
The logic around recognizing function calls has been improved, and
a panic sometimes caused by bad imports has been fixed.
The logic is imprecise/incorrect in some cases; there will be
another CL.
Change-Id: I72be3a0a003569fe06d458989e3dbbb46b7a22c0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/332909
Run-TryBot: Peter Weinberger <pjw@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Peter Weinberger <pjw@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/internal/lsp/semantic.go b/internal/lsp/semantic.go
index c0ed972..214fc9a 100644
--- a/internal/lsp/semantic.go
+++ b/internal/lsp/semantic.go
@@ -12,6 +12,7 @@
"go/token"
"go/types"
"log"
+ "path/filepath"
"sort"
"strings"
"time"
@@ -93,7 +94,7 @@
if err != nil {
return nil, err
}
- // don't return errors on pgf.ParseErr. Do what we can.
+ // ignore pgf.ParseErr. Do what we can.
if rng == nil && len(pgf.Src) > maxFullFileSize {
err := fmt.Errorf("semantic tokens: file %s too large for full (%d>%d)",
fh.URI().Filename(), len(pgf.Src), maxFullFileSize)
@@ -122,6 +123,7 @@
func (e *encoded) semantics() {
f := e.pgf.File
+ // may not be in range, but harmless
e.token(f.Package, len("package"), tokKeyword, nil)
e.token(f.Name.NamePos, len(f.Name.Name), tokNamespace, nil)
inspect := func(n ast.Node) bool {
@@ -166,8 +168,11 @@
)
func (e *encoded) token(start token.Pos, leng int, typ tokenType, mods []string) {
- if start == 0 {
- e.unexpected("token at token.NoPos")
+
+ if start == token.NoPos {
+ // This is not worth reporting
+ //e.unexpected("token at token.NoPos")
+ return
}
if start >= e.end || start+token.Pos(leng) <= e.start {
return
@@ -186,10 +191,7 @@
return
}
if lspRange.End.Line != lspRange.Start.Line {
- // abrupt end of file, without \n. TODO(pjw): fix?
- pos := e.fset.PositionFor(start, false)
- msg := fmt.Sprintf("token at %s:%d.%d overflows", pos.Filename, pos.Line, pos.Column)
- event.Log(e.ctx, msg)
+ // this happens if users are typing at the end of the file, but report nothing
return
}
// token is all on one line
@@ -236,12 +238,22 @@
if len(e.stack) > 0 {
loc := e.stack[len(e.stack)-1].Pos()
add := e.pgf.Tok.PositionFor(loc, false)
- msg = append(msg, fmt.Sprintf("(line:%d,col:%d)", add.Line, add.Column))
+ nm := filepath.Base(add.Filename)
+ msg = append(msg, fmt.Sprintf("(%s:%d,col:%d)", nm, add.Line, add.Column))
}
msg = append(msg, "]")
return strings.Join(msg, " ")
}
+func (e *encoded) srcLine(x ast.Node) string {
+ file := e.pgf.Tok
+ line := file.Line(x.Pos())
+ start := file.Offset(file.LineStart(line))
+ end := file.Offset(file.LineStart(line + 1)) // and hope it's not the last line of the file
+ ans := e.pgf.Src[start : end-1]
+ return string(ans)
+}
+
func (e *encoded) inspector(n ast.Node) bool {
pop := func() {
e.stack = e.stack[:len(e.stack)-1]
@@ -409,6 +421,26 @@
use := e.ti.Uses[x]
switch y := use.(type) {
case nil:
+ // In this position we think the identifier is either a function or a variable
+ // and it is possible that it is being defined. The decision has to be made based
+ // on where we are in the parse tree (and all that's known is the parse stack).
+ // The present logic is inadequate, and will be fixed in the next CL:
+ // ExprStmt CallExpr Ident: var [x in a(x)]
+ // ExprStmt CallExpr Ident: function [f()]
+ // CallExpr TypeAssertExpr Ident: type [so not variable nor function]
+ log.SetFlags(log.Lshortfile)
+ log.Printf("%s %s%q", x.Name, e.strStack(), e.srcLine(x))
+ if len(e.stack) >= 3 {
+ n := len(e.stack) - 1
+ if _, ok := e.stack[n-1].(*ast.SelectorExpr); ok {
+ if _, ok = e.stack[n-2].(*ast.CallExpr); ok {
+ log.Print("function")
+ e.token(x.NamePos, len(x.Name), tokFunction, nil)
+ break
+ }
+ }
+ }
+ log.Print("var def")
e.token(x.NamePos, len(x.Name), tokVariable, []string{"definition"})
case *types.Builtin:
e.token(x.NamePos, len(x.Name), tokFunction, []string{"defaultLibrary"})
@@ -642,16 +674,21 @@
}
// and fall through for _
}
- if d.Path.Value == "" {
+ val := d.Path.Value
+ if len(val) < 2 || val[0] != '"' || val[len(val)-1] != '"' {
+ // avoid panics on imports without a properly quoted string
return
}
- nm := d.Path.Value[1 : len(d.Path.Value)-1] // trailing "
+ nm := val[1 : len(val)-1] // remove surrounding "s
v := strings.LastIndex(nm, "/")
if v != -1 {
+ // in import "lib/math", 'math' is the package name
nm = nm[v+1:]
}
start := d.Path.End() - token.Pos(1+len(nm))
e.token(start, len(nm), tokNamespace, nil)
+ // There may be more cases, as import strings are implementation defined.
+ // (E.g., module a.b.c (without a /), the 'a' should be tokNamespace, if we cared.)
}
// log unexpected state
diff --git a/internal/lsp/testdata/semantic/a.go b/internal/lsp/testdata/semantic/a.go
index 756c56e..54d6c8a 100644
--- a/internal/lsp/testdata/semantic/a.go
+++ b/internal/lsp/testdata/semantic/a.go
@@ -55,6 +55,8 @@
w := b[4:]
j := len(x)
j--
+ q := []interface{}{j, 23i, &y}
+ g(q...)
return true
}
@@ -74,5 +76,6 @@
if !ok {
switch x := vv[0].(type) {
}
+ goto Never
}
}
diff --git a/internal/lsp/testdata/semantic/a.go.golden b/internal/lsp/testdata/semantic/a.go.golden
index 512a83e..ba5187d 100644
--- a/internal/lsp/testdata/semantic/a.go.golden
+++ b/internal/lsp/testdata/semantic/a.go.golden
@@ -56,13 +56,15 @@
/*⇒1,variable,[definition]*/w /*⇒2,operator,[]*/:= /*⇒1,variable,[]*/b[/*⇒1,number,[]*/4:]
/*⇒1,variable,[definition]*/j /*⇒2,operator,[]*/:= /*⇒3,function,[defaultLibrary]*/len(/*⇒1,variable,[]*/x)
/*⇒1,variable,[]*/j/*⇒2,operator,[]*/--
+ /*⇒1,variable,[definition]*/q /*⇒2,operator,[]*/:= []/*⇒9,keyword,[]*/interface{}{/*⇒1,variable,[]*/j, /*⇒3,number,[]*/23i, /*⇒1,operator,[]*/&/*⇒1,variable,[]*/y}
+ /*⇒1,function,[]*/g(/*⇒1,variable,[]*/q/*⇒3,operator,[]*/...)
/*⇒6,keyword,[]*/return /*⇒4,variable,[readonly]*/true
}
/*⇒4,keyword,[]*/func /*⇒1,function,[definition]*/g(/*⇒2,parameter,[definition]*/vv /*⇒3,operator,[]*/.../*⇒9,keyword,[]*/interface{}) {
/*⇒2,variable,[definition]*/ff /*⇒2,operator,[]*/:= /*⇒4,keyword,[]*/func() {}
/*⇒5,keyword,[]*/defer /*⇒2,variable,[]*/ff()
- /*⇒2,keyword,[]*/go /*⇒3,namespace,[]*/utf./*⇒9,variable,[definition]*/RuneCount(/*⇒2,string,[]*/"")
+ /*⇒2,keyword,[]*/go /*⇒3,namespace,[]*/utf./*⇒9,function,[]*/RuneCount(/*⇒2,string,[]*/"")
/*⇒2,keyword,[]*/go /*⇒4,namespace,[]*/utf8./*⇒9,function,[]*/RuneCount(/*⇒2,variable,[]*/vv.(/*⇒6,variable,[definition]*/string))
/*⇒2,keyword,[]*/if /*⇒4,variable,[readonly]*/true {
} /*⇒4,keyword,[]*/else {
@@ -75,6 +77,7 @@
/*⇒2,keyword,[]*/if /*⇒1,operator,[]*/!/*⇒2,variable,[]*/ok {
/*⇒6,keyword,[]*/switch /*⇒1,variable,[definition]*/x /*⇒2,operator,[]*/:= /*⇒2,variable,[]*/vv[/*⇒1,number,[]*/0].(/*⇒4,keyword,[]*/type) {
}
+ /*⇒4,keyword,[]*/goto Never
}
}
