From 5bf02b21f123b83264fa2cf2d4b7aab4759d2d3c Mon Sep 17 00:00:00 2001
From: Muir Manders <muir@mnd.rs>
Date: Thu, 6 Aug 2020 14:30:27 -0700
Subject: [PATCH] internal/lsp/source: fix bug in deep completion score
 tracking

We keep track of the N highest seen scores so we can quickly skip deep
completions not in the top N. Our logic for maintaining the top N list
wasn't quite right, resulting in certain cases where we would let
non-high scoring candidates through. I don't think the bug impacted
correctness.

Change-Id: Ic105617523c82f0e71e4f95ef0ee216182a84252
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247418
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
---
 internal/lsp/source/deep_completion.go      | 19 +++++-------
 internal/lsp/source/deep_completion_test.go | 33 +++++++++++++++++++++
 2 files changed, 41 insertions(+), 11 deletions(-)
 create mode 100644 internal/lsp/source/deep_completion_test.go

diff --git a/internal/lsp/source/deep_completion.go b/internal/lsp/source/deep_completion.go
index 2b5778265b..2afff8b080 100644
--- a/internal/lsp/source/deep_completion.go
+++ b/internal/lsp/source/deep_completion.go
@@ -73,22 +73,19 @@ func (s *deepCompletionState) isHighScore(score float64) bool {
 	// Invariant: s.highScores is sorted with highest score first. Unclaimed
 	// positions are trailing zeros.
 
-	// First check for an unclaimed spot and claim if available.
+	// If we beat an existing score then take its spot.
 	for i, deepScore := range s.highScores {
-		if deepScore == 0 {
-			s.highScores[i] = score
-			return true
+		if score <= deepScore {
+			continue
 		}
-	}
 
-	// Otherwise, if we beat an existing score then take its spot and scoot
-	// all lower scores down one position.
-	for i, deepScore := range s.highScores {
-		if score > deepScore {
+		if deepScore != 0 && i != len(s.highScores)-1 {
+			// If this wasn't an empty slot then we need to scooch everyone
+			// down one spot.
 			copy(s.highScores[i+1:], s.highScores[i:])
-			s.highScores[i] = score
-			return true
 		}
+		s.highScores[i] = score
+		return true
 	}
 
 	return false
diff --git a/internal/lsp/source/deep_completion_test.go b/internal/lsp/source/deep_completion_test.go
new file mode 100644
index 0000000000..47d5179c65
--- /dev/null
+++ b/internal/lsp/source/deep_completion_test.go
@@ -0,0 +1,33 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+	"testing"
+)
+
+func TestDeepCompletionIsHighScore(t *testing.T) {
+	// Test that deepCompletionState.isHighScore properly tracks the top
+	// N=MaxDeepCompletions scores.
+
+	var s deepCompletionState
+
+	if !s.isHighScore(1) {
+		// No other scores yet, anything is a winner.
+		t.Error("1 should be high score")
+	}
+
+	// Fill up with higher scores.
+	for i := 0; i < MaxDeepCompletions; i++ {
+		if !s.isHighScore(10) {
+			t.Error("10 should be high score")
+		}
+	}
+
+	// High scores should be filled with 10s so 2 is not a high score.
+	if s.isHighScore(2) {
+		t.Error("2 shouldn't be high score")
+	}
+}