Implement string interpolation quote linter

This commit implement a linter checking for the usage of '%s' and "%s".
Using manual quoting and bare string interpolation is not safe. We have
a style guide for it here:
https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-q-when-interpolating-strings

4 files changed, 138 insertions, 1 deletions

diff --git a/tools/gitaly-linters/lint.go b/tools/gitaly-linters/lint.go
index c3d9978d8..ffa92428e 100644
--- a/tools/gitaly-linters/lint.go
+++ b/tools/gitaly-linters/lint.go
@@ -7,7 +7,11 @@ import (
 type analyzerPlugin struct{}
 
 func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer {
-	return []*analysis.Analyzer{}
+	return []*analysis.Analyzer{
+		NewQuoteInterpolationAnalyzer([]string{
+			"fmt.*",
+		}),
+	}
 }
 
 // AnalyzerPlugin is a convention of golangci-lint to implement a custom linter. This variable
diff --git a/tools/gitaly-linters/quote.go b/tools/gitaly-linters/quote.go
new file mode 100644
index 000000000..d9648b0b5
--- /dev/null
+++ b/tools/gitaly-linters/quote.go
@@ -0,0 +1,84 @@
+package main
+
+import (
+	"go/ast"
+	"go/token"
+	"regexp"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+// NewQuoteInterpolationAnalyzer returns an analyzer to detect manually quoted string interpolation
+// with '%s' and "%s". Quoting this way doesn't escape special characters such as endline and makes
+// debugging harder later. We encourage to use %q verb instead.
+//
+//   - Bad
+//     return fmt.Errorf("gl_id='%s' is invalid", glID)
+//
+//   - Bad
+//     fmt.Sprintf("fatal: not a git repository: '%s'", repoPath)
+//
+//   - Good
+//     return fmt.Errorf("gl_id=%q is invalid", glID)
+//
+//   - Good
+//     fmt.Sprintf("fatal: not a git repository: %q", repoPath)
+//
+// For more information:
+// https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-q-when-interpolating-strings
+func NewQuoteInterpolationAnalyzer(rules []string) *analysis.Analyzer {
+	return &analysis.Analyzer{
+		Name: "string_interpolation_quote",
+		Doc: `Unless it would lead to incorrect results, always use %q when
+		interpolating strings. For more information:
+		https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-q-when-interpolating-strings`,
+		Run: runStringInterpolationQuoteAnalyzer(rules),
+	}
+}
+
+// offendedFormatPattern matches string interpolation having '%s' and "%s" format
+var offendedFormatPattern = regexp.MustCompile(`['"]%s['"]`)
+
+func analyzeInterpolation(call *ast.CallExpr, pass *analysis.Pass) {
+	if len(call.Args) <= 1 {
+		return
+	}
+
+	if str, ok := call.Args[0].(*ast.BasicLit); ok && str.Kind == token.STRING {
+		value := str.Value
+		if strings.HasPrefix(value, `'`) || strings.HasPrefix(value, `"`) {
+			value = value[1:]
+		}
+		if strings.HasSuffix(value, `'`) || strings.HasSuffix(value, `"`) {
+			value = value[:len(value)-1]
+		}
+		for _, index := range offendedFormatPattern.FindAllIndex([]byte(value), -1) {
+			start := token.Pos(int(str.Pos()) + index[0] + 1)
+			end := token.Pos(int(str.Pos()) + index[1])
+			pass.Report(analysis.Diagnostic{
+				Pos:            start,
+				End:            end,
+				Message:        "wrapping %s verb with quotes is not encouraged, please use %q instead",
+				SuggestedFixes: nil,
+			})
+		}
+	}
+}
+
+func runStringInterpolationQuoteAnalyzer(rules []string) func(*analysis.Pass) (interface{}, error) {
+	return func(pass *analysis.Pass) (interface{}, error) {
+		matcher := NewMatcher(pass)
+		for _, file := range pass.Files {
+			ast.Inspect(file, func(n ast.Node) bool {
+				if call, ok := n.(*ast.CallExpr); ok {
+					if matcher.MatchFunction(call, rules) {
+						analyzeInterpolation(call, pass)
+					}
+				}
+				return true
+			})
+		}
+		return nil, nil
+	}
+}
diff --git a/tools/gitaly-linters/quote_test.go b/tools/gitaly-linters/quote_test.go
new file mode 100644
index 000000000..9ba749288
--- /dev/null
+++ b/tools/gitaly-linters/quote_test.go
@@ -0,0 +1,22 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestQuoteInterpolationAnalyzer(t *testing.T) {
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("Failed to get wd: %s", err)
+	}
+
+	testdata := filepath.Join(wd, "testdata")
+	analyzer := NewQuoteInterpolationAnalyzer([]string{
+		"fmt.*",
+	})
+	analysistest.Run(t, testdata, analyzer, "quote")
+}
diff --git a/tools/gitaly-linters/testdata/src/quote/quote_test.go b/tools/gitaly-linters/testdata/src/quote/quote_test.go
new file mode 100644
index 000000000..d0e832c59
--- /dev/null
+++ b/tools/gitaly-linters/testdata/src/quote/quote_test.go
@@ -0,0 +1,27 @@
+package quote
+
+import "fmt"
+
+// This file is the test fixture for Gitaly linters
+
+func quoteOkay() {
+	fmt.Printf("hello world: %q", "today is a good day")
+	fmt.Printf("hello world: %d", 123)
+	fmt.Printf("%s something", "hello")
+	fmt.Printf("hello world: %s", "this is good")
+	fmt.Printf("hello world: %s something", "this is good")
+}
+
+func quoteNotOkay() {
+	fmt.Printf("hello world: '%s'", "today is a good day") //  want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, "today is a good day") // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, "today is a good day") // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	str := `so is
+tomorrow`
+	fmt.Printf("hello world: '%s'", str) // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, str) // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, str) // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+
+	fmt.Printf("hello world:%s %s '%s'", "today", "is a", "good day") // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf("hello world: %d '%s'", 123, "today is a good day")    // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+}