internal/lsp: distinguish parse errors from actual errors

Parse errors need to be treated separately from actual errors when
parsing a file. Parse errors are treated more like values, whereas
actual errors should not be propagated to the user. This enables us to
delete some of the special handling for context.Canceled errors.

Change-Id: I93a02f22b3f54beccbd6bcf26f04bb8da0202c25
Reviewed-on: https://go-review.googlesource.com/c/tools/+/195997
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Cottrell <[email protected]>

Author: stamblerre

internal/lsp/cache/builtin.go

@@ -43,8 +43,8 @@ func (view *view) buildBuiltinPackage(ctx context.Context) error {
 		fh := view.session.GetFile(span.FileURI(filename))
 		ph := view.session.cache.ParseGoHandle(fh, source.ParseFull)
 		view.builtin.files = append(view.builtin.files, ph)
-		file, _, err := ph.Parse(ctx)
-		if file == nil {
+		file, _, _, err := ph.Parse(ctx)
+		if err != nil {
 			return err
 		}
 		files[filename] = file

internal/lsp/cache/check.go

@@ -258,15 +258,12 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle, m *
 		go func(i int, ph source.ParseGoHandle) {
 			defer wg.Done()
 
-			files[i], _, parseErrors[i] = ph.Parse(ctx)
+			files[i], _, parseErrors[i], _ = ph.Parse(ctx)
 		}(i, ph)
 	}
 	wg.Wait()
 
 	for _, err := range parseErrors {
-		if err == context.Canceled {
-			return nil, errors.Errorf("parsing files for %s: %v", m.pkgPath, err)
-		}
 		if err != nil {
 			imp.view.session.cache.appendPkgError(pkg, err)
 		}
@@ -350,9 +347,9 @@ func (imp *importer) cachePerFile(ctx context.Context, gof *goFile, ph source.Pa
 	}
 	gof.cphs[cph.m.id] = cph
 
-	file, _, err := ph.Parse(ctx)
-	if file == nil {
-		return errors.Errorf("no AST for %s: %v", ph.File().Identity().URI, err)
+	file, _, _, err := ph.Parse(ctx)
+	if err != nil {
+		return err
 	}
 	gof.imports = file.Imports
 	return nil

internal/lsp/cache/load.go

@@ -88,25 +88,33 @@ func (view *view) load(ctx context.Context, f *goFile, fh source.FileHandle) ([]
 
 // checkMetadata determines if we should run go/packages.Load for this file.
 // If yes, update the metadata for the file and its package.
-func (v *view) checkMetadata(ctx context.Context, f *goFile, fh source.FileHandle) ([]*metadata, error) {
+func (v *view) checkMetadata(ctx context.Context, f *goFile, fh source.FileHandle) (metadata []*metadata, err error) {
 	// Check if we need to re-run go/packages before loading the package.
-	f.mu.Lock()
-	runGopackages := v.shouldRunGopackages(ctx, f, fh)
-	metadata := f.metadata()
-	f.mu.Unlock()
+	var runGopackages bool
+	func() {
+		f.mu.Lock()
+		defer f.mu.Unlock()
+
+		runGopackages, err = v.shouldRunGopackages(ctx, f, fh)
+		metadata = f.metadata()
+	}()
+	if err != nil {
+		return nil, err
+	}
 
 	// The package metadata is correct as-is, so just return it.
 	if !runGopackages {
 		return metadata, nil
 	}
 
-	// Check if the context has been canceled before calling packages.Load.
+	// Don't bother running go/packages if the context has been canceled.
 	if ctx.Err() != nil {
 		return nil, ctx.Err()
 	}
 
 	ctx, done := trace.StartSpan(ctx, "packages.Load", telemetry.File.Of(f.filename()))
 	defer done()
+
 	pkgs, err := packages.Load(v.Config(ctx), fmt.Sprintf("file=%s", f.filename()))
 	if len(pkgs) == 0 {
 		if err == nil {
@@ -185,40 +193,36 @@ func sameSet(x, y map[packagePath]struct{}) bool {
 // shouldRunGopackages reparses a file's package and import declarations to
 // determine if they have changed.
 // It assumes that the caller holds the lock on the f.mu lock.
-func (v *view) shouldRunGopackages(ctx context.Context, f *goFile, fh source.FileHandle) (result bool) {
+func (v *view) shouldRunGopackages(ctx context.Context, f *goFile, fh source.FileHandle) (result bool, err error) {
 	if len(f.meta) == 0 || len(f.missingImports) > 0 {
-		return true
+		return true, nil
 	}
 	// Get file content in case we don't already have it.
-	parsed, _, err := v.session.cache.ParseGoHandle(fh, source.ParseHeader).Parse(ctx)
-	if err == context.Canceled {
-		log.Error(ctx, "parsing file header", err, tag.Of("file", f.URI()))
-		return false
-	}
-	if parsed == nil {
-		return true
+	parsed, _, _, err := v.session.cache.ParseGoHandle(fh, source.ParseHeader).Parse(ctx)
+	if err != nil {
+		return false, err
 	}
 	// Check if the package's name has changed, by checking if this is a filename
 	// we already know about, and if so, check if its package name has changed.
 	for _, m := range f.meta {
 		for _, uri := range m.files {
 			if span.CompareURI(uri, f.URI()) == 0 {
 				if m.name != parsed.Name.Name {
-					return true
+					return true, nil
 				}
 			}
 		}
 	}
 	// If the package's imports have changed, re-run `go list`.
 	if len(f.imports) != len(parsed.Imports) {
-		return true
+		return true, nil
 	}
 	for i, importSpec := range f.imports {
 		if importSpec.Path.Value != parsed.Imports[i].Path.Value {
-			return true
+			return true, nil
 		}
 	}
-	return false
+	return false, nil
 }
 
 type importGraph struct {

internal/lsp/cache/parse.go

@@ -41,9 +41,10 @@ type parseGoHandle struct {
 type parseGoData struct {
 	memoize.NoCopy
 
-	ast    *ast.File
-	mapper *protocol.ColumnMapper
-	err    error
+	ast        *ast.File
+	parseError error // errors associated with parsing the file
+	mapper     *protocol.ColumnMapper
+	err        error
 }
 
 func (c *cache) ParseGoHandle(fh source.FileHandle, mode source.ParseMode) source.ParseGoHandle {
@@ -53,18 +54,7 @@ func (c *cache) ParseGoHandle(fh source.FileHandle, mode source.ParseMode) sourc
 	}
 	h := c.store.Bind(key, func(ctx context.Context) interface{} {
 		data := &parseGoData{}
-		data.ast, data.err = parseGo(ctx, c, fh, mode)
-		tok := c.FileSet().File(data.ast.Pos())
-		if tok == nil {
-			return data
-		}
-		uri := fh.Identity().URI
-		content, _, err := fh.Read(ctx)
-		if err != nil {
-			data.err = err
-			return data
-		}
-		data.mapper = newColumnMapper(uri, c.FileSet(), tok, content)
+		data.ast, data.mapper, data.parseError, data.err = parseGo(ctx, c, fh, mode)
 		return data
 	})
 	return &parseGoHandle{
@@ -73,13 +63,6 @@ func (c *cache) ParseGoHandle(fh source.FileHandle, mode source.ParseMode) sourc
 		mode:   mode,
 	}
 }
-func newColumnMapper(uri span.URI, fset *token.FileSet, tok *token.File, content []byte) *protocol.ColumnMapper {
-	return &protocol.ColumnMapper{
-		URI:       uri,
-		Converter: span.NewTokenConverter(fset, tok),
-		Content:   content,
-	}
-}
 
 func (h *parseGoHandle) File() source.FileHandle {
 	return h.file
@@ -89,22 +72,22 @@ func (h *parseGoHandle) Mode() source.ParseMode {
 	return h.mode
 }
 
-func (h *parseGoHandle) Parse(ctx context.Context) (*ast.File, *protocol.ColumnMapper, error) {
+func (h *parseGoHandle) Parse(ctx context.Context) (*ast.File, *protocol.ColumnMapper, error, error) {
 	v := h.handle.Get(ctx)
 	if v == nil {
-		return nil, nil, ctx.Err()
+		return nil, nil, nil, ctx.Err()
 	}
 	data := v.(*parseGoData)
-	return data.ast, data.mapper, data.err
+	return data.ast, data.mapper, data.parseError, data.err
 }
 
-func (h *parseGoHandle) Cached(ctx context.Context) (*ast.File, *protocol.ColumnMapper, error) {
+func (h *parseGoHandle) Cached(ctx context.Context) (*ast.File, *protocol.ColumnMapper, error, error) {
 	v := h.handle.Cached()
 	if v == nil {
-		return nil, nil, errors.Errorf("no cached value for %s", h.file.Identity().URI)
+		return nil, nil, nil, errors.Errorf("no cached AST for %s", h.file.Identity().URI)
 	}
 	data := v.(*parseGoData)
-	return data.ast, data.mapper, data.err
+	return data.ast, data.mapper, data.parseError, data.err
 }
 
 func hashParseKey(ph source.ParseGoHandle) string {
@@ -122,35 +105,51 @@ func hashParseKeys(phs []source.ParseGoHandle) string {
 	return hashContents(b.Bytes())
 }
 
-func parseGo(ctx context.Context, c *cache, fh source.FileHandle, mode source.ParseMode) (*ast.File, error) {
+func parseGo(ctx context.Context, c *cache, fh source.FileHandle, mode source.ParseMode) (file *ast.File, mapper *protocol.ColumnMapper, parseError error, err error) {
 	ctx, done := trace.StartSpan(ctx, "cache.parseGo", telemetry.File.Of(fh.Identity().URI.Filename()))
 	defer done()
 
 	buf, _, err := fh.Read(ctx)
 	if err != nil {
-		return nil, err
+		return nil, nil, nil, err
 	}
 	parseLimit <- struct{}{}
 	defer func() { <-parseLimit }()
 	parserMode := parser.AllErrors | parser.ParseComments
 	if mode == source.ParseHeader {
 		parserMode = parser.ImportsOnly | parser.ParseComments
 	}
-	ast, err := parser.ParseFile(c.fset, fh.Identity().URI.Filename(), buf, parserMode)
-	if ast != nil {
+	file, parseError = parser.ParseFile(c.fset, fh.Identity().URI.Filename(), buf, parserMode)
+	if file != nil {
 		if mode == source.ParseExported {
-			trimAST(ast)
+			trimAST(file)
 		}
 		// Fix any badly parsed parts of the AST.
-		tok := c.fset.File(ast.Pos())
-		if err := fix(ctx, ast, tok, buf); err != nil {
+		tok := c.fset.File(file.Pos())
+		if err := fix(ctx, file, tok, buf); err != nil {
 			log.Error(ctx, "failed to fix AST", err)
 		}
 	}
-	if ast == nil {
-		return nil, err
+	// If the file is nil only due to parse errors,
+	// the parse errors are the actual errors.
+	if file == nil {
+		return nil, nil, parseError, parseError
+	}
+	tok := c.FileSet().File(file.Pos())
+	if tok == nil {
+		return nil, nil, parseError, err
+	}
+	uri := fh.Identity().URI
+	content, _, err := fh.Read(ctx)
+	if err != nil {
+		return nil, nil, parseError, err
+	}
+	m := &protocol.ColumnMapper{
+		URI:       uri,
+		Converter: span.NewTokenConverter(c.FileSet(), tok),
+		Content:   content,
 	}
-	return ast, err
+	return file, m, parseError, nil
 }
 
 // trimAST clears any part of the AST not relevant to type checking

internal/lsp/cache/pkg.go

@@ -151,11 +151,20 @@ func (pkg *pkg) Files() []source.ParseGoHandle {
 	return pkg.files
 }
 
+func (pkg *pkg) File(uri span.URI) (source.ParseGoHandle, error) {
+	for _, ph := range pkg.Files() {
+		if ph.File().Identity().URI == uri {
+			return ph, nil
+		}
+	}
+	return nil, errors.Errorf("no ParseGoHandle for %s", uri)
+}
+
 func (pkg *pkg) GetSyntax(ctx context.Context) []*ast.File {
 	var syntax []*ast.File
 	for _, ph := range pkg.files {
-		file, _, _ := ph.Cached(ctx)
-		if file != nil {
+		file, _, _, err := ph.Cached(ctx)
+		if err == nil {
 			syntax = append(syntax, file)
 		}
 	}

internal/lsp/diagnostics.go

@@ -23,6 +23,7 @@ func (s *Server) diagnostics(view source.View, uri span.URI) error {
 	defer done()
 
 	ctx = telemetry.File.With(ctx, uri)
+
 	f, err := view.GetFile(ctx, uri)
 	if err != nil {
 		return err
@@ -45,8 +46,9 @@ func (s *Server) diagnostics(view source.View, uri span.URI) error {
 			if s.undelivered == nil {
 				s.undelivered = make(map[span.URI][]source.Diagnostic)
 			}
-			log.Error(ctx, "failed to deliver diagnostic (will retry)", err, telemetry.File)
 			s.undelivered[uri] = diagnostics
+
+			log.Error(ctx, "failed to deliver diagnostic (will retry)", err, telemetry.File)
 			continue
 		}
 		// In case we had old, undelivered diagnostics.
@@ -58,6 +60,7 @@ func (s *Server) diagnostics(view source.View, uri span.URI) error {
 		if err := s.publishDiagnostics(ctx, uri, diagnostics); err != nil {
 			log.Error(ctx, "failed to deliver diagnostic for (will not retry)", err, telemetry.File)
 		}
+
 		// If we fail to deliver the same diagnostics twice, just give up.
 		delete(s.undelivered, uri)
 	}

internal/lsp/link.go

@@ -28,8 +28,8 @@ func (s *Server) documentLink(ctx context.Context, params *protocol.DocumentLink
 		return nil, err
 	}
 	fh := f.Handle(ctx)
-	file, m, err := view.Session().Cache().ParseGoHandle(fh, source.ParseFull).Parse(ctx)
-	if file == nil {
+	file, m, _, err := view.Session().Cache().ParseGoHandle(fh, source.ParseFull).Parse(ctx)
+	if err != nil {
 		return nil, err
 	}
 	var links []protocol.DocumentLink

internal/lsp/source/completion.go

@@ -376,14 +376,12 @@ func Completion(ctx context.Context, view View, f GoFile, pos protocol.Position,
 	if err != nil {
 		return nil, nil, err
 	}
-	var ph ParseGoHandle
-	for _, h := range pkg.Files() {
-		if h.File().Identity().URI == f.URI() {
-			ph = h
-		}
+	ph, err := pkg.File(f.URI())
+	if err != nil {
+		return nil, nil, err
 	}
-	file, m, err := ph.Cached(ctx)
-	if file == nil {
+	file, m, _, err := ph.Cached(ctx)
+	if err != nil {
 		return nil, nil, err
 	}
 	spn, err := m.PointSpan(pos)

internal/lsp/source/completion_format.go

@@ -123,14 +123,13 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
 	if !pos.IsValid() {
 		return item, nil
 	}
-
 	uri := span.FileURI(pos.Filename)
 	ph, pkg, err := c.pkg.FindFile(c.ctx, uri)
 	if err != nil {
 		return CompletionItem{}, err
 	}
-	file, _, err := ph.Cached(c.ctx)
-	if file == nil {
+	file, _, _, err := ph.Cached(c.ctx)
+	if err != nil {
 		return CompletionItem{}, err
 	}
 	if !(file.Pos() <= obj.Pos() && obj.Pos() <= file.End()) {