From a20955124b7a73cf062d81f2687473c7953fd9aa Mon Sep 17 00:00:00 2001
From: Muir Manders <muir@mnd.rs>
Date: Thu, 16 Jan 2020 15:00:13 -0800
Subject: [PATCH] internal/lsp: consolidate completion sorting

Move completion candidate sorting into internal/lsp/source so
source_test.go and internal/lsp don't have to duplicate the logic.

Change-Id: Ifbe7ca5ad6a5b74020fd1260b4d4f775709968cf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215137
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 internal/lsp/completion.go         |  8 --------
 internal/lsp/source/completion.go  | 16 ++++++++++++++++
 internal/lsp/source/source_test.go |  9 +--------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go
index 560e82c0a5..4ea01b6b86 100644
--- a/internal/lsp/completion.go
+++ b/internal/lsp/completion.go
@@ -7,7 +7,6 @@ package lsp
 import (
 	"context"
 	"fmt"
-	"sort"
 	"strings"
 
 	"golang.org/x/tools/internal/lsp/protocol"
@@ -50,13 +49,6 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara
 	if err != nil {
 		return nil, err
 	}
-	// Sort the candidates by score, then label, since that is not supported by LSP yet.
-	sort.SliceStable(candidates, func(i, j int) bool {
-		if candidates[i].Score != candidates[j].Score {
-			return candidates[i].Score > candidates[j].Score
-		}
-		return candidates[i].Label < candidates[j].Label
-	})
 
 	// When using deep completions/fuzzy matching, report results as incomplete so
 	// client fetches updated completions after every key stroke.
diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go
index 41b43fb14e..7039ab5a7c 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -12,6 +12,7 @@ import (
 	"go/token"
 	"go/types"
 	"math"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -484,6 +485,8 @@ func Completion(ctx context.Context, snapshot Snapshot, fh FileHandle, pos proto
 
 	c.expectedType = expectedType(c)
 
+	defer c.sortItems()
+
 	// If we're inside a comment return comment completions
 	for _, comment := range file.Comments {
 		if comment.Pos() <= rng.Start && rng.Start <= comment.End() {
@@ -566,6 +569,19 @@ func Completion(ctx context.Context, snapshot Snapshot, fh FileHandle, pos proto
 	return c.items, c.getSurrounding(), nil
 }
 
+func (c *completer) sortItems() {
+	sort.SliceStable(c.items, func(i, j int) bool {
+		// Sort by score first.
+		if c.items[i].Score != c.items[j].Score {
+			return c.items[i].Score > c.items[j].Score
+		}
+
+		// Then sort by label so order stays consistent. This also has the
+		// effect of prefering shorter candidates.
+		return c.items[i].Label < c.items[j].Label
+	})
+}
+
 // populateCommentCompletions yields completions for an exported
 // variable immediately preceding comment.
 func (c *completer) populateCommentCompletions(comment *ast.CommentGroup) {
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 401ecd4cf9..dcfe7cc800 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -245,14 +245,7 @@ func (r *runner) callCompletion(t *testing.T, src span.Span, options func(*sourc
 	if surrounding != nil {
 		prefix = strings.ToLower(surrounding.Prefix())
 	}
-	// TODO(rstambler): In testing this out, I noticed that scores are equal,
-	// even when they shouldn't be. This needs more investigation.
-	sort.SliceStable(list, func(i, j int) bool {
-		if list[i].Score != list[j].Score {
-			return list[i].Score > list[j].Score
-		}
-		return list[i].Label < list[j].Label
-	})
+
 	var numDeepCompletionsSeen int
 	var items []source.CompletionItem
 	// Apply deep completion filtering.