internal/lsp: miscellaneous cleanup

CL 212102 contains a few cleanup-type fixes that are unrelated to the
actual content of that CL. Pull them out to make the diffs simpler.

Change-Id: I6b7e34320f2889d05179c8aeb8cb7f8f4c505a3b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212917
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cmd/check.go

@@ -57,15 +57,15 @@ func (c *check) Run(ctx context.Context, args ...string) error {
 	}
 	// now wait for results
 	// TODO: maybe conn.ExecuteCommand(ctx, &protocol.ExecuteCommandParams{Command: "gopls-wait-idle"})
+	ctx, cancel := context.WithTimeout(ctx, time.Second*30)
+	defer cancel()
+
 	for _, file := range checking {
-		select {
-		case <-file.hasDiagnostics:
-		case <-time.After(30 * time.Second):
-			return errors.Errorf("timed out waiting for results from %v", file.uri)
+		diagnostics, err := file.waitForDiagnostics(ctx)
+		if err != nil {
+			return err
 		}
-		file.diagnosticsMu.Lock()
-		defer file.diagnosticsMu.Unlock()
-		for _, d := range file.diagnostics {
+		for _, d := range diagnostics {
 			spn, err := file.mapper.RangeSpan(d.Range)
 			if err != nil {
 				return errors.Errorf("Could not convert position %v for %q", d.Range, d.Message)

internal/lsp/cmd/cmd.go

@@ -406,6 +406,18 @@ func (c *cmdClient) getFile(ctx context.Context, uri span.URI) *cmdFile {
 	return file
 }
 
+func (file *cmdFile) waitForDiagnostics(ctx context.Context) ([]protocol.Diagnostic, error) {
+	select {
+	case <-ctx.Done():
+		return nil, ctx.Err()
+	case <-file.hasDiagnostics:
+	}
+
+	file.diagnosticsMu.Lock()
+	defer file.diagnosticsMu.Unlock()
+	return file.diagnostics, nil
+}
+
 func (c *connection) AddFile(ctx context.Context, uri span.URI) *cmdFile {
 	c.Client.filesMu.Lock()
 	defer c.Client.filesMu.Unlock()

internal/lsp/cmd/references.go

@@ -54,12 +54,10 @@ func (r *references) Run(ctx context.Context, args ...string) error {
 	if file.err != nil {
 		return file.err
 	}
-
 	loc, err := file.mapper.Location(from)
 	if err != nil {
 		return err
 	}
-
 	p := protocol.ReferenceParams{
 		Context: protocol.ReferenceContext{
 			IncludeDeclaration: r.IncludeDeclaration,
@@ -73,7 +71,6 @@ func (r *references) Run(ctx context.Context, args ...string) error {
 	if err != nil {
 		return err
 	}
-
 	if len(locations) == 0 {
 		return tool.CommandLineErrorf("%v: not an identifier", from)
 	}
@@ -94,6 +91,5 @@ func (r *references) Run(ctx context.Context, args ...string) error {
 	for _, s := range spans {
 		fmt.Println(s)
 	}
-
 	return nil
 }

internal/lsp/cmd/test/check.go

@@ -18,6 +18,9 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti
 	if len(want) == 1 && want[0].Message == "" {
 		return
 	}
+	if strings.Contains(uri.Filename(), "circular") {
+		t.Skip("skipping circular diagnostics tests due to golang/go#36265")
+	}
 	fname := uri.Filename()
 	out, _ := r.RunGoplsCmd(t, "check", fname)
 	// parse got into a collection of reports

internal/lsp/source/implementation.go

@@ -106,15 +106,19 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.
 
 			info := pkg.GetTypesInfo()
 			for _, obj := range info.Defs {
+				obj, ok := obj.(*types.TypeName)
 				// We ignore aliases 'type M = N' to avoid duplicate reporting
 				// of the Named type N.
-				if obj, ok := obj.(*types.TypeName); ok && !obj.IsAlias() {
-					// We skip interface types since we only want concrete
-					// implementations.
-					if named, ok := obj.Type().(*types.Named); ok && !isInterface(named) {
-						allNamed = append(allNamed, named)
-					}
+				if !ok || obj.IsAlias() {
+					continue
 				}
+				named, ok := obj.Type().(*types.Named)
+				// We skip interface types since we only want concrete
+				// implementations.
+				if !ok || isInterface(named) {
+					continue
+				}
+				allNamed = append(allNamed, named)
 			}
 		}