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 <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Trust: Peter Weinberger <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: pjweinbgo

internal/lsp/semantic.go

@@ -12,6 +12,7 @@ import (
 	"go/token"
 	"go/types"
 	"log"
+	"path/filepath"
 	"sort"
 	"strings"
 	"time"
@@ -93,7 +94,7 @@ func (s *Server) computeSemanticTokens(ctx context.Context, td protocol.TextDocu
 	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 (s *Server) computeSemanticTokens(ctx context.Context, td protocol.TextDocu
 
 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 @@ const (
 )
 
 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 @@ func (e *encoded) token(start token.Pos, leng int, typ tokenType, mods []string)
 		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 @@ func (e *encoded) strStack() string {
 	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 @@ func (e *encoded) ident(x *ast.Ident) {
 	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 @@ func (e *encoded) importSpec(d *ast.ImportSpec) {
 		}
 		// 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

internal/lsp/testdata/semantic/a.go

@@ -55,6 +55,8 @@ func (a *A) f() bool {
 	w := b[4:]
 	j := len(x)
 	j--
+	q := []interface{}{j, 23i, &y}
+	g(q...)
 	return true
 }
 
@@ -74,5 +76,6 @@ Never:
 	if !ok {
 		switch x := vv[0].(type) {
 		}
+		goto Never
 	}
 }

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
 	}
 }