Implement a linter to validate error wrapping without %wqmnguyen0711/fix-custom-linter-offenses

Use %w when wrapping errors with context. Using %w has plenty of
benefits and allows unwrapping underlying error later. After this
feature was introduced, we favor error wrapping to string interpolation.
This commit implements a linter catching such offenses.

For more information
https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-w-when-wrapping-errors

5 files changed, 190 insertions, 0 deletions

diff --git a/tools/gitaly-linters/errorwrap.go b/tools/gitaly-linters/errorwrap.go
new file mode 100644
index 000000000..af3530a60
--- /dev/null
+++ b/tools/gitaly-linters/errorwrap.go
@@ -0,0 +1,95 @@
+package main
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+	"regexp"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+// NewErrorWrapAnalyzer returns an analyzer to detect unexpected error interpolation without %w.
+// After error wrapping was introduced, we encourage wrapping error with %w when constructing a new
+// error. The new error contains the original error able to be unwrapped later.
+//
+//   - Bad
+//     return structerr.NewInvalidArgument("GetRepoPath: %s", err)
+//
+//   - Bad
+//     return structerr.NewCanceled("%v", err)
+//
+//   - Bad
+//     return nil, fmt.Errorf("unmarshalling json: %v", err)
+//
+//   - Good
+//     return structerr.NewCanceled("%w", err)
+//
+//   - Good
+//     return nil, fmt.Errorf("failed unmarshalling json: %w", err)
+//
+// For more information:
+// https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-w-when-wrapping-errors
+func NewErrorWrapAnalyzer(rules []string) *analysis.Analyzer {
+	return &analysis.Analyzer{
+		Name: "error_wrap",
+		Doc: `Always wrap an error with %w:
+	https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-w-when-wrapping-errors`,
+		Run: runErrorWrapAnalyzer(rules),
+	}
+}
+
+var errorType = types.Universe.Lookup("error").Type().Underlying().(*types.Interface)
+
+// Over-simplified pattern to parse interpolation format. This linter targets error wrapping only.
+// Most of the time, %s or %v or %q are used. This pattern is good enough to detect most cases.
+// The std uses a proper parser, which is overkilled for re-implemented:
+// https://github.com/golang/go/blob/518889b35cb07f3e71963f2ccfc0f96ee26a51ce/src/fmt/print.go#L1026
+var formatPattern = regexp.MustCompile(`%.`)
+
+func analyzeErrorInterpolation(pass *analysis.Pass, call *ast.CallExpr) {
+	if len(call.Args) <= 1 {
+		// Irrelevant call, or static format
+		return
+	}
+
+	if str, ok := call.Args[0].(*ast.BasicLit); ok && str.Kind == token.STRING {
+		verbs := formatPattern.FindAllIndex([]byte(str.Value), -1)
+
+		if len(verbs) != len(call.Args)-1 {
+			// Mismatched format verbs and arguments; or our regexp is not correct
+			return
+		}
+		for index, arg := range call.Args[1:] {
+			argType := pass.TypesInfo.Types[arg].Type
+			if types.Implements(argType, errorType) {
+				verb := str.Value[verbs[index][0]:verbs[index][1]]
+				if verb != "%w" {
+					pass.Report(analysis.Diagnostic{
+						Pos:            token.Pos(int(str.Pos()) + verbs[index][0]),
+						End:            token.Pos(int(str.Pos()) + verbs[index][1]),
+						Message:        "please use %w to wrap errors",
+						SuggestedFixes: nil,
+					})
+				}
+			}
+		}
+	}
+}
+
+func runErrorWrapAnalyzer(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) {
+						analyzeErrorInterpolation(pass, call)
+					}
+				}
+				return true
+			})
+		}
+		return nil, nil
+	}
+}
diff --git a/tools/gitaly-linters/errorwrap_test.go b/tools/gitaly-linters/errorwrap_test.go
new file mode 100644
index 000000000..529112ded
--- /dev/null
+++ b/tools/gitaly-linters/errorwrap_test.go
@@ -0,0 +1,22 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestNewErrorWrapAnalyzer(t *testing.T) {
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("Failed to get wd: %s", err)
+	}
+
+	testdata := filepath.Join(wd, "testdata")
+	analyzer := NewErrorWrapAnalyzer([]string{
+		"fmt.Errorf",
+	})
+	analysistest.Run(t, testdata, analyzer, "errorwrap")
+}
diff --git a/tools/gitaly-linters/lint.go b/tools/gitaly-linters/lint.go
index ffa92428e..f03e118eb 100644
--- a/tools/gitaly-linters/lint.go
+++ b/tools/gitaly-linters/lint.go
@@ -11,6 +11,10 @@ func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer {
 		NewQuoteInterpolationAnalyzer([]string{
 			"fmt.*",
 		}),
+		NewErrorWrapAnalyzer([]string{
+			"fmt.Errorf",
+			"gitlab.com/gitlab-org/gitaly/v15/internal/structerr.*",
+		}),
 	}
 }
 
diff --git a/tools/gitaly-linters/testdata/src/errorwrap/errorwrap_alias_test.go b/tools/gitaly-linters/testdata/src/errorwrap/errorwrap_alias_test.go
new file mode 100644
index 000000000..d3e0aba27
--- /dev/null
+++ b/tools/gitaly-linters/testdata/src/errorwrap/errorwrap_alias_test.go
@@ -0,0 +1,32 @@
+package errorwrap
+
+import (
+	f "fmt"
+)
+
+// This file is the test fixture for Gitaly linters
+
+func errorWrapAliasOkay() {
+	err := f.Errorf("test error")
+
+	_ = f.Errorf("error: %s", "something else")
+	_ = f.Errorf("error: %v", "something else")
+	_ = f.Errorf("error: %q", "something else")
+	_ = f.Errorf("error: %s %d", "something else", 5)
+	_ = f.Errorf("error: %w", err)
+	_ = f.Errorf("error: %w", f.Errorf("error: %s", "hello"))
+}
+
+func errorWrapAliasNotOkay() {
+	err := f.Errorf("test error")
+
+	_ = f.Errorf("error: %s", err)                    // want "please use %w to wrap errors"
+	_ = f.Errorf("error: %s", f.Errorf("test error")) // want "please use %w to wrap errors"
+	_ = f.Errorf("error: %v", err)                    // want "please use %w to wrap errors"
+	_ = f.Errorf("error: %v", f.Errorf("test error")) // want "please use %w to wrap errors"
+	_ = f.Errorf("error: %q", err)                    // want "please use %w to wrap errors"
+	_ = f.Errorf("error: %q", f.Errorf("test error")) // want "please use %w to wrap errors"
+	_ = f.Errorf("error number %d: %s", 5, err)       // want "please use %w to wrap errors"
+	_ = f.Errorf("error: %w", err)
+	_ = f.Errorf("error: %w", f.Errorf("error: %s", err)) // want "please use %w to wrap errors"
+}
diff --git a/tools/gitaly-linters/testdata/src/errorwrap/errorwrap_test.go b/tools/gitaly-linters/testdata/src/errorwrap/errorwrap_test.go
new file mode 100644
index 000000000..5f6764e63
--- /dev/null
+++ b/tools/gitaly-linters/testdata/src/errorwrap/errorwrap_test.go
@@ -0,0 +1,37 @@
+package errorwrap
+
+import (
+	"fmt"
+)
+
+// This file is the test fixture for Gitaly linters
+
+func call(format string, err error) {}
+
+func errorWrapOkay() {
+	err := fmt.Errorf("test error")
+
+	_ = fmt.Errorf("error: %s", "something else")
+	_ = fmt.Errorf("error: %v", "something else")
+	_ = fmt.Errorf("error: %q", "something else")
+	_ = fmt.Errorf("error: %s %d", "something else", 5)
+	_ = fmt.Errorf("error: %w", err)
+	_ = fmt.Errorf("error: %w", fmt.Errorf("error: %s", "hello"))
+
+	call("error: %w", fmt.Errorf("error: %s", "hello"))
+	_ = fmt.Sprintf("error: %s", err)
+}
+
+func errorWrapNotOkay() {
+	err := fmt.Errorf("test error")
+
+	_ = fmt.Errorf("error: %s", err)                      // want "please use %w to wrap errors"
+	_ = fmt.Errorf("error: %s", fmt.Errorf("test error")) // want "please use %w to wrap errors"
+	_ = fmt.Errorf("error: %v", err)                      // want "please use %w to wrap errors"
+	_ = fmt.Errorf("error: %v", fmt.Errorf("test error")) // want "please use %w to wrap errors"
+	_ = fmt.Errorf("error: %q", err)                      // want "please use %w to wrap errors"
+	_ = fmt.Errorf("error: %q", fmt.Errorf("test error")) // want "please use %w to wrap errors"
+	_ = fmt.Errorf("error number %d: %s", 5, err)         // want "please use %w to wrap errors"
+	_ = fmt.Errorf("error: %w", err)
+	_ = fmt.Errorf("error: %w", fmt.Errorf("error: %s", err)) // want "please use %w to wrap errors"
+}