internal/lsp: change some comments and variable names in completion code

Also, return diagnostics instead of errors from source.Diagnostics (to
avoid stuck diagnostics).

Change-Id: I56b067273982fd086ed74185e50eda5b72b5fba1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/174400
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

internal/lsp/cache/view.go

@@ -126,6 +126,7 @@ func (v *View) FileSet() *token.FileSet {
 func (v *View) SetContent(ctx context.Context, uri span.URI, content []byte) error {
 	v.mu.Lock()
 	defer v.mu.Unlock()
+
 	// Cancel all still-running previous requests, since they would be
 	// operating on stale data.
 	v.cancel()

internal/lsp/completion.go

@@ -51,25 +51,24 @@ func toProtocolCompletionItems(candidates []source.CompletionItem, prefix string
 		if !strings.HasPrefix(candidate.Label, prefix) {
 			continue
 		}
-
+		insertText := candidate.InsertText
-		insertText := candidate.Insert
 		if insertTextFormat == protocol.SnippetTextFormat {
 			if usePlaceholders && candidate.PlaceholderSnippet != nil {
 				insertText = candidate.PlaceholderSnippet.String()
-			} else if candidate.PlainSnippet != nil {
+			} else if candidate.Snippet != nil {
-				insertText = candidate.PlainSnippet.String()
+				insertText = candidate.Snippet.String()
 			}
 		}
-
+		// If the user has already typed some part of the completion candidate,
+		// don't insert that portion of the text.
 		if strings.HasPrefix(insertText, prefix) {
 			insertText = insertText[len(prefix):]
 		}
-
+		// Don't filter on text that might have snippets in it.
-		filterText := candidate.Insert
+		filterText := candidate.InsertText
 		if strings.HasPrefix(filterText, prefix) {
 			filterText = filterText[len(prefix):]
 		}
-
 		item := protocol.CompletionItem{
 			Label:  candidate.Label,
 			Detail: candidate.Detail,

internal/lsp/source/completion.go

@@ -19,27 +19,43 @@ type CompletionItem struct {
 	// Label is the primary text the user sees for this completion item.
 	Label string
 
-	// Detail is supplemental information to present to the user. This
+	// Detail is supplemental information to present to the user.
-	// often contains the Go type of the completion item.
+	// This often contains the type or return type of the completion item.
 	Detail string
 
-	// Insert is the text to insert if this item is selected. Any already-typed
+	// InsertText is the text to insert if this item is selected.
-	// prefix has not been trimmed. Insert does not contain snippets.
+	// Any of the prefix that has already been typed is not trimmed.
-	Insert string
+	// The insert text does not contain snippets.
+	InsertText string
 
 	Kind CompletionItemKind
 
-	// Score is the internal relevance score. Higher is more relevant.
+	// Score is the internal relevance score.
+	// A higher score indicates that this completion item is more relevant.
 	Score float64
 
-	// PlainSnippet is the LSP snippet to be inserted if not nil and snippets are
+	// Snippet is the LSP snippet for the completion item, without placeholders.
-	// enabled and placeholders are not desired. This can contain tabs stops, but
+	// The LSP specification contains details about LSP snippets.
-	// should not contain placeholder text.
+	// For example, a snippet for a function with the following signature:
-	PlainSnippet *snippet.Builder
+	//
+	//     func foo(a, b, c int)
+	//
+	// would be:
+	//
+	//     foo(${1:})
+	//
+	Snippet *snippet.Builder
 
-	// PlaceholderSnippet is the LSP snippet to be inserted if not nil and
+	// PlaceholderSnippet is the LSP snippet for the completion ite, containing
-	// snippets are enabled and placeholders are desired. This can contain
+	// placeholders. The LSP specification contains details about LSP snippets.
-	// placeholder text.
+	// For example, a placeholder snippet for a function with the following signature:
+	//
+	//     func foo(a, b, c int)
+	//
+	// would be:
+	//
+	//     foo(${1:a int}, ${2: b int}, ${3: c int})
+	//
 	PlaceholderSnippet *snippet.Builder
 }
 
@@ -144,7 +160,7 @@ func (c *completer) found(obj types.Object, weight float64) {
 func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, string, error) {
 	file := f.GetAST(ctx)
 	pkg := f.GetPackage(ctx)
-	if pkg.IsIllTyped() {
+	if pkg == nil || pkg.IsIllTyped() {
 		return nil, "", fmt.Errorf("package for %s is ill typed", f.URI())
 	}
 
@@ -165,7 +181,7 @@ func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, s
 		return nil, "", nil
 	}
 
-	cl, kv, clField := enclosingCompositeLiteral(path, pos)
+	lit, kv, inCompositeLiteralField := enclosingCompositeLiteral(path, pos)
 	c := &completer{
 		types:                     pkg.GetTypes(),
 		info:                      pkg.GetTypesInfo(),
@@ -177,9 +193,9 @@ func Completion(ctx context.Context, f File, pos token.Pos) ([]CompletionItem, s
 		expectedType:              expectedType(path, pos, pkg.GetTypesInfo()),
 		enclosingFunction:         enclosingFunction(path, pos, pkg.GetTypesInfo()),
 		preferTypeNames:           preferTypeNames(path, pos),
-		enclosingCompositeLiteral: cl,
+		enclosingCompositeLiteral: lit,
 		enclosingKeyValue:         kv,
-		inCompositeLiteralField:   clField,
+		inCompositeLiteralField:   inCompositeLiteralField,
 	}
 
 	// Composite literals are handled entirely separately.
@@ -466,12 +482,12 @@ func enclosingFunction(path []ast.Node, pos token.Pos, info *types.Info) *types.
 	return nil
 }
 
-func (c *completer) expectedCompositeLiteralType(cl *ast.CompositeLit, kv *ast.KeyValueExpr) types.Type {
+func (c *completer) expectedCompositeLiteralType(lit *ast.CompositeLit, kv *ast.KeyValueExpr) types.Type {
-	clType, ok := c.info.Types[cl]
+	litType, ok := c.info.Types[lit]
 	if !ok {
 		return nil
 	}
-	switch t := clType.Type.Underlying().(type) {
+	switch t := litType.Type.Underlying().(type) {
 	case *types.Slice:
 		return t.Elem()
 	case *types.Array:
@@ -501,14 +517,14 @@ func (c *completer) expectedCompositeLiteralType(cl *ast.CompositeLit, kv *ast.K
 		// We are in a struct literal, but not a specific key-value pair.
 		// If the struct literal doesn't have explicit field names,
 		// we may still be able to suggest an expected type.
-		for _, el := range cl.Elts {
+		for _, el := range lit.Elts {
 			if _, ok := el.(*ast.KeyValueExpr); ok {
 				return nil
 			}
 		}
 		// The order of the literal fields must match the order in the struct definition.
 		// Find the element that the position belongs to and suggest that field's type.
-		if i := indexExprAtPos(c.pos, cl.Elts); i < t.NumFields() {
+		if i := indexExprAtPos(c.pos, lit.Elts); i < t.NumFields() {
 			return t.Field(i).Type()
 		}
 	}

internal/lsp/source/completion_format.go

@@ -35,8 +35,8 @@ func (c *completer) item(obj types.Object, score float64) CompletionItem {
 			detail = ""
 		} else {
 			val := o.Val().ExactString()
-			if !strings.Contains(val, "\\n") { // skip any multiline constants
+			if !strings.ContainsRune(val, '\n') { // skip any multiline constants
-				label += " = " + o.Val().ExactString()
+				label += " = " + val
 			}
 		}
 		kind = ConstantCompletionItem
@@ -46,24 +46,25 @@ func (c *completer) item(obj types.Object, score float64) CompletionItem {
 		}
 		if o.IsField() {
 			kind = FieldCompletionItem
-			plainSnippet, placeholderSnippet = c.structFieldSnippet(label, detail)
+			plainSnippet, placeholderSnippet = c.structFieldSnippets(label, detail)
 		} else if c.isParameter(o) {
 			kind = ParameterCompletionItem
 		} else {
 			kind = VariableCompletionItem
 		}
 	case *types.Func:
-		if sig, ok := o.Type().(*types.Signature); ok {
+		sig, ok := o.Type().(*types.Signature)
-			params := formatEachParam(sig, c.qf)
+		if !ok {
-			label += formatParamParts(params)
+			break
-			detail = strings.Trim(types.TypeString(sig.Results(), c.qf), "()")
-			kind = FunctionCompletionItem
-			if sig.Recv() != nil {
-				kind = MethodCompletionItem
-			}
-
-			plainSnippet, placeholderSnippet = c.funcCallSnippet(obj.Name(), params)
 		}
+		params := formatEachParam(sig, c.qf)
+		label += formatParamParts(params)
+		detail = strings.Trim(types.TypeString(sig.Results(), c.qf), "()")
+		kind = FunctionCompletionItem
+		if sig.Recv() != nil {
+			kind = MethodCompletionItem
+		}
+		plainSnippet, placeholderSnippet = c.functionCallSnippets(obj.Name(), params)
 	case *types.Builtin:
 		item, ok := builtinDetails[obj.Name()]
 		if !ok {
@@ -82,11 +83,11 @@ func (c *completer) item(obj types.Object, score float64) CompletionItem {
 
 	return CompletionItem{
 		Label:              label,
-		Insert:             insert,
+		InsertText:         insert,
 		Detail:             detail,
 		Kind:               kind,
 		Score:              score,
-		PlainSnippet:       plainSnippet,
+		Snippet:            plainSnippet,
 		PlaceholderSnippet: placeholderSnippet,
 	}
 }

internal/lsp/source/completion_snippet.go

@@ -5,69 +5,66 @@
 package source
 
 import (
+	"fmt"
 	"go/ast"
 
 	"golang.org/x/tools/internal/lsp/snippet"
 )
 
-// structField calculates the plain and placeholder snippets for struct literal
+// structFieldSnippets calculates the plain and placeholder snippets for struct literal field names.
-// field names as in "Foo{Ba<>".
+func (c *completer) structFieldSnippets(label, detail string) (*snippet.Builder, *snippet.Builder) {
-func (c *completer) structFieldSnippet(label, detail string) (*snippet.Builder, *snippet.Builder) {
 	if !c.inCompositeLiteralField {
 		return nil, nil
 	}
 
-	cl := c.enclosingCompositeLiteral
+	lit := c.enclosingCompositeLiteral
 	kv := c.enclosingKeyValue
 
-	// If we aren't in a composite literal or are already in a key/value
+	// If we aren't in a composite literal or are already in a key-value expression,
-	// expression, we don't want a snippet.
+	// we don't want a snippet.
-	if cl == nil || kv != nil {
+	if lit == nil || kv != nil {
 		return nil, nil
 	}
-
+	// First, confirm that we are actually completing a struct field.
-	if len(cl.Elts) > 0 {
+	if len(lit.Elts) > 0 {
-		i := indexExprAtPos(c.pos, cl.Elts)
+		i := indexExprAtPos(c.pos, lit.Elts)
-		if i >= len(cl.Elts) {
+		if i >= len(lit.Elts) {
 			return nil, nil
 		}
-
+		// If the expression is not an identifer, it is not a struct field name.
-		// If our expression is not an identifer, we know it isn't a
+		if _, ok := lit.Elts[i].(*ast.Ident); !ok {
-		// struct field name.
-		if _, ok := cl.Elts[i].(*ast.Ident); !ok {
 			return nil, nil
 		}
 	}
 
-	// It is a multi-line literal if pos is not on the same line as the literal's
+	plain, placeholder := &snippet.Builder{}, &snippet.Builder{}
-	// opening brace.
+	label = fmt.Sprintf("%s: ", label)
-	multiLine := c.fset.Position(c.pos).Line != c.fset.Position(cl.Lbrace).Line
 
-	// Plain snippet will turn "Foo{Ba<>" into "Foo{Bar: <>"
+	// A plain snippet turns "Foo{Ba<>" into "Foo{Bar: <>".
-	plain := &snippet.Builder{}
+	plain.WriteText(label)
-	plain.WriteText(label + ": ")
 	plain.WritePlaceholder(nil)
-	if multiLine {
-		plain.WriteText(",")
-	}
 
-	// Placeholder snippet will turn "Foo{Ba<>" into "Foo{Bar: *int*"
+	// A placeholder snippet turns "Foo{Ba<>" into "Foo{Bar: <*int*>".
-	placeholder := &snippet.Builder{}
+	placeholder.WriteText(label)
-	placeholder.WriteText(label + ": ")
 	placeholder.WritePlaceholder(func(b *snippet.Builder) {
 		b.WriteText(detail)
 	})
-	if multiLine {
+
+	// If the cursor position is on a different line from the literal's opening brace,
+	// we are in a multiline literal.
+	if c.fset.Position(c.pos).Line != c.fset.Position(lit.Lbrace).Line {
+		plain.WriteText(",")
 		placeholder.WriteText(",")
 	}
 
 	return plain, placeholder
 }
 
-// funcCall calculates the plain and placeholder snippets for function calls.
+// functionCallSnippets calculates the plain and placeholder snippets for function calls.
-func (c *completer) funcCallSnippet(funcName string, params []string) (*snippet.Builder, *snippet.Builder) {
+func (c *completer) functionCallSnippets(name string, params []string) (*snippet.Builder, *snippet.Builder) {
 	for i := 1; i <= 2 && i < len(c.path); i++ {
 		call, ok := c.path[i].(*ast.CallExpr)
+
 		// If we are the left side (i.e. "Fun") part of a call expression,
 		// we don't want a snippet since there are already parens present.
 		if ok && call.Fun == c.path[i-1] {
@@ -75,17 +72,18 @@ func (c *completer) funcCallSnippet(funcName string, params []string) (*snippet.
 		}
 	}
 
-	// Plain snippet turns "someFun<>" into "someFunc(<>)"
+	plain, placeholder := &snippet.Builder{}, &snippet.Builder{}
-	plain := &snippet.Builder{}
+	label := fmt.Sprintf("%s(", name)
-	plain.WriteText(funcName + "(")
+
+	// A plain snippet turns "someFun<>" into "someFunc(<>)".
+	plain.WriteText(label)
 	if len(params) > 0 {
 		plain.WritePlaceholder(nil)
 	}
 	plain.WriteText(")")
 
-	// Placeholder snippet turns "someFun<>" into "someFunc(*i int*, s string)"
+	// A placeholder snippet turns "someFun<>" into "someFunc(<*i int*>, *s string*)".
-	placeholder := &snippet.Builder{}
+	placeholder.WriteText(label)
-	placeholder.WriteText(funcName + "(")
 	for i, p := range params {
 		if i > 0 {
 			placeholder.WriteText(", ")

internal/lsp/source/diagnostics.go

@@ -98,12 +98,8 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag
 		}
 	}
 	if len(diags) > 0 {
-		v.Logger().Debugf(ctx, "found parse or type-checking errors for %s, returning", uri)
 		return reports, nil
 	}
-
-	v.Logger().Debugf(ctx, "running `go vet` analyses for %s", uri)
-
 	// Type checking and parsing succeeded. Run analyses.
 	if err := runAnalyses(ctx, v, pkg, func(a *analysis.Analyzer, diag analysis.Diagnostic) error {
 		r := span.NewRange(v.FileSet(), diag.Pos, 0)
@@ -124,11 +120,9 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag
 		})
 		return nil
 	}); err != nil {
-		return nil, err
+		return singleDiagnostic(uri, "unable to run analyses for %s: %v", uri, err), nil
 	}
 
-	v.Logger().Debugf(ctx, "completed reporting `go vet` analyses for %s", uri)
-
 	return reports, nil
 }
 
@@ -208,8 +202,6 @@ func runAnalyses(ctx context.Context, v View, pkg Package, report func(a *analys
 		return err
 	}
 
-	v.Logger().Debugf(ctx, "analyses have completed for %s", pkg.GetTypes().Path())
-
 	// Report diagnostics and errors from root analyzers.
 	for _, r := range roots {
 		for _, diag := range r.diagnostics {