qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-18 10:04:43 -07:00
Code Releases Activity

internal/lsp/cache: hide type errors if we fix up the AST

Browse Source 
I was curious about why were logging errors during type-checking in
tests, and the answer turned out to be a bit more sinister than I
expected. We were getting type error messages without filepaths, so I
tried to reproduce it in the playground and wasn't able to. I realized
that these errors were coming from were coming from the "fixed" version
of the AST that we pass to the type checker.

Adding fake positions to our fake Cond statements trivially fixes the
logging issue, but it does nothing to handle the fact that the error
makes no sense to the user - because it applies to something that's not
in the source code. I figured we have two options: (1) skip type errors
for all packages with "fixed" ASTs, or (2) add something to the error
messages to indicate that the source code may not match. Starting with
(1) here, and if it becomes a problem, we can move to 2. All ASTs that
we fix have *ast.BadExpr in them, meaning that, by definition they have
parse errors which we will preferentially show those errors to users in
diagnostics (so I'm not sure how to test this).

Change-Id: I17733968aa15f989cdd3e4e7261c4f4fe9b97495
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227557
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This commit is contained in:
Rebecca Stambler 2020-04-07 22:54:42 -04:00
parent c08edad6b3
commit 3bd20875a2
4 changed files with 100 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
internal/lsp/cache/check.go vendored
View File

@ -31,10 +31,7 @@ type packageHandleKey string
type packageHandle struct {
handle *memoize.Handle
goFiles []source.ParseGoHandle
// compiledGoFiles are the ParseGoHandles that compose the package.
compiledGoFiles []source.ParseGoHandle
goFiles, compiledGoFiles []*parseGoHandle
// mode is the mode the the files were parsed in.
mode source.ParseMode
@ -159,7 +156,7 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
return ph, deps, nil
}
func checkPackageKey(id packageID, pghs []source.ParseGoHandle, cfg *packages.Config, deps []packageHandleKey) packageHandleKey {
func checkPackageKey(id packageID, pghs []*parseGoHandle, cfg *packages.Config, deps []packageHandleKey) packageHandleKey {
var depBytes []byte
for _, dep := range deps {
depBytes = append(depBytes, []byte(dep)...)
@ -198,7 +195,11 @@ func (ph *packageHandle) check(ctx context.Context) (*pkg, error) {
}
func (ph *packageHandle) CompiledGoFiles() []source.ParseGoHandle {
return ph.compiledGoFiles
var files []source.ParseGoHandle
for _, f := range ph.compiledGoFiles {
files = append(files, f)
}
return files
}
func (ph *packageHandle) ID() string {
@ -248,19 +249,19 @@ func (ph *packageHandle) cached() (*pkg, error) {
return data.pkg, data.err
}
func (s *snapshot) parseGoHandles(ctx context.Context, files []span.URI, mode source.ParseMode) ([]source.ParseGoHandle, error) {
phs := make([]source.ParseGoHandle, 0, len(files))
func (s *snapshot) parseGoHandles(ctx context.Context, files []span.URI, mode source.ParseMode) ([]*parseGoHandle, error) {
phs := make([]*parseGoHandle, 0, len(files))
for _, uri := range files {
fh, err := s.GetFile(uri)
if err != nil {
return nil, err
}
phs = append(phs, s.view.session.cache.ParseGoHandle(fh, mode))
phs = append(phs, s.view.session.cache.parseGoHandle(fh, mode))
}
return phs, nil
}
func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode source.ParseMode, goFiles []source.ParseGoHandle, compiledGoFiles []source.ParseGoHandle, deps map[packagePath]*packageHandle) (*pkg, error) {
func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode source.ParseMode, goFiles, compiledGoFiles []*parseGoHandle, deps map[packagePath]*packageHandle) (*pkg, error) {
ctx, done := event.StartSpan(ctx, "cache.importer.typeCheck", tag.Package.Of(string(m.id)))
defer done()
@ -293,12 +294,24 @@ func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode sourc
parseErrors = make([]error, len(pkg.compiledGoFiles))
actualErrors = make([]error, len(pkg.compiledGoFiles))
wg sync.WaitGroup
mu sync.Mutex
skipTypeErrors bool
)
for i, ph := range pkg.compiledGoFiles {
wg.Add(1)
go func(i int, ph source.ParseGoHandle) {
files[i], _, _, parseErrors[i], actualErrors[i] = ph.Parse(ctx)
wg.Done()
go func(i int, ph *parseGoHandle) {
defer wg.Done()
data, err := ph.parse(ctx)
if err != nil {
actualErrors[i] = err
return
}
files[i], parseErrors[i], actualErrors[i] = data.ast, data.parseError, data.err
mu.Lock()
skipTypeErrors = skipTypeErrors || data.fixed
mu.Unlock()
}(i, ph)
}
for _, ph := range pkg.goFiles {
@ -340,6 +353,11 @@ func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode sourc
cfg := &types.Config{
Error: func(e error) {
// If we have fixed parse errors in any of the files,
// we should hide type errors, as they may be completely nonsensical.
if skipTypeErrors {
return
}
rawErrors = append(rawErrors, e)
},
Importer: importerFunc(func(pkgPath string) (*types.Package, error) {

4
internal/lsp/cache/errors.go vendored
View File

@ -6,6 +6,7 @@ package cache
import (
"context"
"fmt"
"go/scanner"
"go/token"
"go/types"
@ -85,6 +86,9 @@ func sourceError(ctx context.Context, fset *token.FileSet, pkg *pkg, e interface
case types.Error:
msg = e.Msg
kind = source.TypeError
if !e.Pos.IsValid() {
return nil, fmt.Errorf("invalid position for type error %v", e)
}
spn, err = typeErrorRange(ctx, fset, pkg, e.Pos)
if err != nil {
return nil, err

105
internal/lsp/cache/parse.go vendored
View File

@ -37,14 +37,24 @@ type parseGoHandle struct {
type parseGoData struct {
memoize.NoCopy
src []byte
ast *ast.File
ast *ast.File
mapper *protocol.ColumnMapper
// Source code used to build the AST. It may be different from the
// actual content of the file if we have fixed the AST, in which case,
// fixed will be true.
src []byte
fixed bool
parseError error // errors associated with parsing the file
mapper *protocol.ColumnMapper
err error
err error // any other errors
}
func (c *Cache) ParseGoHandle(fh source.FileHandle, mode source.ParseMode) source.ParseGoHandle {
return c.parseGoHandle(fh, mode)
}
func (c *Cache) parseGoHandle(fh source.FileHandle, mode source.ParseMode) *parseGoHandle {
key := parseKey{
file: fh.Identity(),
mode: mode,
@ -73,14 +83,22 @@ func (pgh *parseGoHandle) Mode() source.ParseMode {
}
func (pgh *parseGoHandle) Parse(ctx context.Context) (*ast.File, []byte, *protocol.ColumnMapper, error, error) {
v := pgh.handle.Get(ctx)
if v == nil {
return nil, nil, nil, nil, errors.Errorf("no parsed file for %s", pgh.File().Identity().URI)
data, err := pgh.parse(ctx)
if err != nil {
return nil, nil, nil, nil, err
}
data := v.(*parseGoData)
return data.ast, data.src, data.mapper, data.parseError, data.err
}
func (pgh *parseGoHandle) parse(ctx context.Context) (*parseGoData, error) {
v := pgh.handle.Get(ctx)
data, ok := v.(*parseGoData)
if !ok {
return nil, errors.Errorf("no parsed file for %s", pgh.File().Identity().URI)
}
return data, nil
}
func (pgh *parseGoHandle) Cached() (*ast.File, []byte, *protocol.ColumnMapper, error, error) {
v := pgh.handle.Cached()
if v == nil {
@ -97,7 +115,7 @@ func hashParseKey(ph source.ParseGoHandle) string {
return hashContents(b.Bytes())
}
func hashParseKeys(phs []source.ParseGoHandle) string {
func hashParseKeys(phs []*parseGoHandle) string {
b := bytes.NewBuffer(nil)
for _, ph := range phs {
b.WriteString(hashParseKey(ph))
@ -123,6 +141,7 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod
}
file, parseError := parser.ParseFile(fset, fh.Identity().URI.Filename(), buf, parserMode)
var tok *token.File
var fixed bool
if file != nil {
tok = fset.File(file.Pos())
if tok == nil {
@ -130,7 +149,7 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod
}
// Fix any badly parsed parts of the AST.
_ = fixAST(ctx, file, tok, buf)
fixed = fixAST(ctx, file, tok, buf)
// Fix certain syntax errors that render the file unparseable.
newSrc := fixSrc(file, tok, buf)
@ -142,7 +161,7 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod
buf = newSrc
tok = fset.File(file.Pos())
_ = fixAST(ctx, file, tok, buf)
fixed = fixAST(ctx, file, tok, buf)
}
}
@ -150,7 +169,6 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod
trimAST(file)
}
}
if file == nil {
// If the file is nil only due to parse errors,
// the parse errors are the actual errors.
@ -165,12 +183,12 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod
Converter: span.NewTokenConverter(fset, tok),
Content: buf,
}
return &parseGoData{
src: buf,
ast: file,
mapper: m,
parseError: parseError,
fixed: fixed,
}
}
@ -213,26 +231,22 @@ func isEllipsisArray(n ast.Expr) bool {
// fixAST inspects the AST and potentially modifies any *ast.BadStmts so that it can be
// type-checked more effectively.
func fixAST(ctx context.Context, n ast.Node, tok *token.File, src []byte) error {
func fixAST(ctx context.Context, n ast.Node, tok *token.File, src []byte) (fixed bool) {
var err error
walkASTWithParent(n, func(n, parent ast.Node) bool {
switch n := n.(type) {
case *ast.BadStmt:
err = fixDeferOrGoStmt(n, parent, tok, src) // don't shadow err
if err == nil {
if fixed = fixDeferOrGoStmt(n, parent, tok, src); fixed {
// Recursively fix in our fixed node.
err = fixAST(ctx, parent, tok, src)
_ = fixAST(ctx, parent, tok, src)
} else {
err = errors.Errorf("unable to parse defer or go from *ast.BadStmt: %v", err)
}
return false
case *ast.BadExpr:
// Don't propagate this error since *ast.BadExpr is very common
// and it is only sometimes due to array types. Errors from here
// are expected and not actionable in general.
if fixArrayType(n, parent, tok, src) == nil {
if fixed = fixArrayType(n, parent, tok, src); fixed {
// Recursively fix in our fixed node.
err = fixAST(ctx, parent, tok, src)
_ = fixAST(ctx, parent, tok, src)
return false
}
@ -266,8 +280,7 @@ func fixAST(ctx context.Context, n ast.Node, tok *token.File, src []byte) error
return true
}
})
return err
return fixed
}
// walkASTWithParent walks the AST rooted at n. The semantics are
@ -556,13 +569,19 @@ func fixInitStmt(bad *ast.BadExpr, parent ast.Node, tok *token.File, src []byte)
return
}
p.Init = stmt
p.Cond = &ast.Ident{Name: "_"}
p.Cond = &ast.Ident{
Name: "_",
NamePos: stmt.End(),
}
case *ast.ForStmt:
if p.Init != nil {
return
}
p.Init = stmt
p.Cond = &ast.Ident{Name: "_"}
p.Cond = &ast.Ident{
Name: "_",
NamePos: stmt.End(),
}
case *ast.SwitchStmt:
if p.Init != nil {
return
@ -598,14 +617,14 @@ func readKeyword(pos token.Pos, tok *token.File, src []byte) string {
// fixArrayType tries to parse an *ast.BadExpr into an *ast.ArrayType.
// go/parser often turns lone array types like "[]int" into BadExprs
// if it isn't expecting a type.
func fixArrayType(bad *ast.BadExpr, parent ast.Node, tok *token.File, src []byte) error {
func fixArrayType(bad *ast.BadExpr, parent ast.Node, tok *token.File, src []byte) bool {
// Our expected input is a bad expression that looks like "[]someExpr".
from := bad.Pos()
to := bad.End()
if !from.IsValid() || !to.IsValid() {
return errors.Errorf("invalid BadExpr from/to: %d/%d", from, to)
return false
}
exprBytes := make([]byte, 0, int(to-from)+3)
@ -626,24 +645,20 @@ func fixArrayType(bad *ast.BadExpr, parent ast.Node, tok *token.File, src []byte
expr, err := parseExpr(from, exprBytes)
if err != nil {
return err
return false
}
cl, _ := expr.(*ast.CompositeLit)
if cl == nil {
return errors.Errorf("expr not compLit (%T)", expr)
return false
}
at, _ := cl.Type.(*ast.ArrayType)
if at == nil {
return errors.Errorf("compLit type not array (%T)", cl.Type)
return false
}
if !replaceNode(parent, bad, at) {
return errors.Errorf("couldn't replace array type")
}
return nil
return replaceNode(parent, bad, at)
}
// precedingToken scans src to find the token preceding pos.
@ -670,7 +685,7 @@ func precedingToken(pos token.Pos, tok *token.File, src []byte) token.Token {
// this statement entirely, and we can't use the type information when completing.
// Here, we try to generate a fake *ast.DeferStmt or *ast.GoStmt to put into the AST,
// instead of the *ast.BadStmt.
func fixDeferOrGoStmt(bad *ast.BadStmt, parent ast.Node, tok *token.File, src []byte) error {
func fixDeferOrGoStmt(bad *ast.BadStmt, parent ast.Node, tok *token.File, src []byte) bool {
// Check if we have a bad statement containing either a "go" or "defer".
s := &scanner.Scanner{}
s.Init(tok, src, nil, 0)
@ -681,7 +696,7 @@ func fixDeferOrGoStmt(bad *ast.BadStmt, parent ast.Node, tok *token.File, src []
)
for {
if tkn == token.EOF {
return errors.Errorf("reached the end of the file")
return false
}
if pos >= bad.From {
break
@ -700,7 +715,7 @@ func fixDeferOrGoStmt(bad *ast.BadStmt, parent ast.Node, tok *token.File, src []
Go: pos,
}
default:
return errors.Errorf("no defer or go statement found")
return false
}
var (
@ -768,11 +783,11 @@ FindTo:
}
if !from.IsValid() || tok.Offset(from) >= len(src) {
return errors.Errorf("invalid from position")
return false
}
if !to.IsValid() || tok.Offset(to) >= len(src) {
return errors.Errorf("invalid to position %d", to)
return false
}
// Insert any phantom selectors needed to prevent dangling "." from messing
@ -792,7 +807,7 @@ FindTo:
expr, err := parseExpr(from, exprBytes)
if err != nil {
return err
return false
}
// Package the expression into a fake *ast.CallExpr and re-insert
@ -810,11 +825,7 @@ FindTo:
stmt.Call = call
}
if !replaceNode(parent, bad, stmt) {
return errors.Errorf("couldn't replace CallExpr")
}
return nil
return replaceNode(parent, bad, stmt)
}
// parseStmt parses the statement in src and updates its position to

10
internal/lsp/cache/pkg.go vendored
View File

@ -21,8 +21,8 @@ type pkg struct {
pkgPath packagePath
mode source.ParseMode
forTest packagePath
goFiles []source.ParseGoHandle
compiledGoFiles []source.ParseGoHandle
goFiles []*parseGoHandle
compiledGoFiles []*parseGoHandle
errors []*source.Error
imports map[packagePath]*pkg
module *packagesinternal.Module
@ -52,7 +52,11 @@ func (p *pkg) PkgPath() string {
}
func (p *pkg) CompiledGoFiles() []source.ParseGoHandle {
return p.compiledGoFiles
var files []source.ParseGoHandle
for _, f := range p.compiledGoFiles {
files = append(files, f)
}
return files
}
func (p *pkg) File(uri span.URI) (source.ParseGoHandle, error) {