From 137eb8f2261028b05a290f121b9add59c2b34fdc Mon Sep 17 00:00:00 2001
From: Keegan Carruthers-Smith <keegan.csmith@gmail.com>
Date: Wed, 15 Nov 2023 11:10:04 +0200
Subject: [PATCH] build: ignore out of bound lines from ctags (#694)

universal-ctags sometimes returns lines that are out of bounds. In
practice it seems to only do an off by one. We haven't noticed the
linenum error until a recent change of mine which didn't append an extra
entry to NLS if the file was terminated by "\n". In practice this would
end up being filtered out later on. So we update to just continue rather
than error here.

An example is
https://github.com/sourcegraph/sourcegraph/blob/v5.2.2/client/web-sveltekit/.storybook/main.ts

  $ universal-ctags '--fields=*' --output-format=json main.ts | grep 22
  {"_type": "tag", "name": "config", "path": "main.ts", "pattern": "/^export default config$/", "language": "TypeScript", "line": 22, "kind": "constant", "roles": "def"}

  $ wc -l main.ts
  21 main.ts

  $ tail -n1 main.ts
  export default config

Test Plan: added a unit test
---
 build/ctags.go        |  3 ++-
 build/ctags_test.go   | 53 +++++++++++++++++++++++++++++++++++--------
 build/scoring_test.go | 24 ++++++++------------
 3 files changed, 55 insertions(+), 25 deletions(-)

diff --git a/build/ctags.go b/build/ctags.go
index 28cf88c3b..abdd7dd70 100644
--- a/build/ctags.go
+++ b/build/ctags.go
@@ -134,7 +134,8 @@ func (t *tagsToSections) Convert(content []byte, tags []*ctags.Entry) ([]zoekt.D
 		}
 		lineIdx := t.Line - 1
 		if lineIdx >= len(nls) {
-			return nil, nil, fmt.Errorf("linenum for entry out of range %v", t)
+			// Observed this with a .TS file.
+			continue
 		}
 
 		lineOff := uint32(0)
diff --git a/build/ctags_test.go b/build/ctags_test.go
index 386d07e75..b052eecca 100644
--- a/build/ctags_test.go
+++ b/build/ctags_test.go
@@ -116,16 +116,38 @@ func TestTagsToSectionsEOF(t *testing.T) {
 			Name: "bar",
 			Line: 2,
 		},
-	}
 
-	secs, _, err := (&tagsToSections{}).Convert(c, tags)
-	if err != nil {
-		t.Fatal("tagsToSections", err)
+		// We have seen ctags do this on a JS file
+		{
+			Name: "wat",
+			Line: -1,
+		},
+
+		// We have seen ctags return out of bounds lines
+		{
+			Name: "goliath",
+			Line: 3,
+		},
 	}
 
-	if len(secs) != 1 || secs[0].Start != 17 || secs[0].End != 20 {
-		t.Fatalf("got %#v, want 1 section (17,20)", secs)
+	// We run this test twice. Once with a final \n and without.
+	do := func(t *testing.T, doc []byte) {
+		secs, _, err := (&tagsToSections{}).Convert(doc, tags)
+		if err != nil {
+			t.Fatal("tagsToSections", err)
+		}
+
+		if len(secs) != 1 || secs[0].Start != 17 || secs[0].End != 20 {
+			t.Fatalf("got %#v, want 1 section (17,20)", secs)
+		}
 	}
+
+	t.Run("no final newline", func(t *testing.T) {
+		do(t, c)
+	})
+	t.Run("trailing newline", func(t *testing.T) {
+		do(t, append(c, '\n'))
+	})
 }
 
 func TestOverlaps(t *testing.T) {
@@ -232,9 +254,7 @@ func TestOverlaps(t *testing.T) {
 }
 
 func BenchmarkTagsToSections(b *testing.B) {
-	if checkCTags() == "" {
-		b.Skip("ctags not available")
-	}
+	requireCTags(b)
 
 	file, err := os.ReadFile("./testdata/large_file.cc")
 	parser, err := ctags.NewParser(ctags.UniversalCTags, "universal-ctags")
@@ -268,3 +288,18 @@ func BenchmarkTagsToSections(b *testing.B) {
 		}
 	}
 }
+
+func requireCTags(tb testing.TB) {
+	tb.Helper()
+
+	if checkCTags() != "" {
+		return
+	}
+
+	// On CI we require ctags to be available. Otherwise we skip
+	if os.Getenv("CI") != "" {
+		tb.Fatal("universal-ctags is missing")
+	} else {
+		tb.Skip("universal-ctags is missing")
+	}
+}
diff --git a/build/scoring_test.go b/build/scoring_test.go
index 2f7bb92c5..4254d60b1 100644
--- a/build/scoring_test.go
+++ b/build/scoring_test.go
@@ -27,8 +27,8 @@ import (
 )
 
 type scoreCase struct {
-	fileName           string
-	content            []byte
+	fileName  string
+	content   []byte
 	query     query.Q
 	language  string
 	wantScore float64
@@ -58,7 +58,7 @@ func TestFileNameMatch(t *testing.T) {
 			wantScore: 510,
 		},
 	}
-	
+
 	for _, c := range cases {
 		checkScoring(t, c, ctags.UniversalCTags)
 	}
@@ -165,7 +165,7 @@ func TestJava(t *testing.T) {
 	}
 
 	for _, c := range cases {
-        checkScoring(t, c, ctags.UniversalCTags)
+		checkScoring(t, c, ctags.UniversalCTags)
 	}
 }
 
@@ -174,7 +174,7 @@ func TestKotlin(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	
+
 	cases := []scoreCase{
 		{
 			fileName: "example.kt",
@@ -239,7 +239,7 @@ func TestCpp(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	
+
 	cases := []scoreCase{
 		{
 			fileName: "example.cc",
@@ -489,9 +489,7 @@ func skipIfCTagsUnavailable(t *testing.T, parserType ctags.CTagsParserType) {
 
 	switch parserType {
 	case ctags.UniversalCTags:
-		if checkCTags() == "" {
-			t.Skip("ctags not available")
-		}
+		requireCTags(t)
 	case ctags.ScipCTags:
 		if checkScipCTags() == "" {
 			t.Skip("scip-ctags not available")
@@ -560,9 +558,7 @@ func checkScoring(t *testing.T, c scoreCase, parserType ctags.CTagsParserType) {
 }
 
 func TestDocumentRanks(t *testing.T) {
-	if os.Getenv("CI") == "" && checkCTags() == "" {
-		t.Skip("ctags not available")
-	}
+	requireCTags(t)
 	dir := t.TempDir()
 
 	opts := Options{
@@ -649,9 +645,7 @@ func TestDocumentRanks(t *testing.T) {
 }
 
 func TestRepoRanks(t *testing.T) {
-	if os.Getenv("CI") == "" && checkCTags() == "" {
-		t.Skip("ctags not available")
-	}
+	requireCTags(t)
 	dir := t.TempDir()
 
 	opts := Options{