feat: add spancheck linter (#4290)

Co-authored-by: Fernandez Ludovic <ldez@users.noreply.github.com>

.golangci.reference.yml

@@ -1912,6 +1912,24 @@ linters-settings:
     # Default: false
     args-on-sep-lines: true
 
+  spancheck:
+    # Checks to enable.
+    # Options include:
+    # - `end`: check that `span.End()` is called
+    # - `record-error`: check that `span.RecordError(err)` is called when an error is returned
+    # - `set-status`: check that `span.SetStatus(codes.Error, msg)` is called when an error is returned
+    # Default: ["end"]
+    checks:
+      - end
+      - record-error
+      - set-status
+    # A list of regexes for function signatures that silence `record-error` and `set-status` reports
+    # if found in the call path to a returned error.
+    # https://github.com/jjti/go-spancheck#ignore-check-signatures
+    # Default: []
+    ignore-check-signatures:
+      - "telemetry.RecordError"
+
   staticcheck:
     # Deprecated: use the global `run.go` instead.
     go: "1.15"
@@ -2442,6 +2460,7 @@ linters:
     - rowserrcheck
     - scopelint
     - sloglint
+    - spancheck
     - sqlclosecheck
     - staticcheck
     - structcheck
@@ -2562,6 +2581,7 @@ linters:
     - rowserrcheck
     - scopelint
     - sloglint
+    - spancheck
     - sqlclosecheck
     - staticcheck
     - structcheck

go.mod

@@ -57,6 +57,7 @@ require (
 	github.com/jgautheron/goconst v1.7.0
 	github.com/jingyugao/rowserrcheck v1.1.1
 	github.com/jirfag/go-printf-func-name v0.0.0-20200119135958-7558a9eaa5af
+	github.com/jjti/go-spancheck v0.4.2
 	github.com/julz/importas v0.1.0
 	github.com/kisielk/errcheck v1.6.3
 	github.com/kkHAIKE/contextcheck v1.1.4

go.sum

@@ -310,6 +310,8 @@ github.com/jingyugao/rowserrcheck v1.1.1 h1:zibz55j/MJtLsjP1OF4bSdgXxwL1b+Vn7Tjz
 github.com/jingyugao/rowserrcheck v1.1.1/go.mod h1:4yvlZSDb3IyDTUZJUmpZfm2Hwok+Dtp+nu2qOq+er9c=
 github.com/jirfag/go-printf-func-name v0.0.0-20200119135958-7558a9eaa5af h1:KA9BjwUk7KlCh6S9EAGWBt1oExIUv9WyNCiRz5amv48=
 github.com/jirfag/go-printf-func-name v0.0.0-20200119135958-7558a9eaa5af/go.mod h1:HEWGJkRDzjJY2sqdDwxccsGicWEf9BQOZsq2tV+xzM0=
+github.com/jjti/go-spancheck v0.4.2 h1:4MvJOTKRi9ClsPNTxVhHvcSyuInILLSKmstTCJ/oIZI=
+github.com/jjti/go-spancheck v0.4.2/go.mod h1:TBZ1nIcHTtCnBChHcjd+5agCxHBaW0tzw9quzCCNAts=
 github.com/jpillora/backoff v1.0.0/go.mod h1:J/6gKK9jxlEcS3zixgDgUAsiuZ7yrSoa/FX5e0EB2j4=
 github.com/json-iterator/go v1.1.6/go.mod h1:+SdeFBvtyEkXs7REEP0seUULqWtbJapLOCVDaaPEHmU=
 github.com/json-iterator/go v1.1.10/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4=

pkg/config/linters_settings.go

@@ -245,13 +245,14 @@ type LintersSettings struct {
 	Revive           ReviveSettings
 	RowsErrCheck     RowsErrCheckSettings
 	SlogLint         SlogLintSettings
+	Spancheck        SpancheckSettings
 	Staticcheck      StaticCheckSettings
 	Structcheck      StructCheckSettings
 	Stylecheck       StaticCheckSettings
 	TagAlign         TagAlignSettings
 	Tagliatelle      TagliatelleSettings
-	Testifylint      TestifylintSettings
 	Tenv             TenvSettings
+	Testifylint      TestifylintSettings
 	Testpackage      TestpackageSettings
 	Thelper          ThelperSettings
 	Unparam          UnparamSettings
@@ -773,6 +774,11 @@ type SlogLintSettings struct {
 	ArgsOnSepLines bool   `mapstructure:"args-on-sep-lines"`
 }
 
+type SpancheckSettings struct {
+	Checks                []string `mapstructure:"checks"`
+	IgnoreCheckSignatures []string `mapstructure:"ignore-check-signatures"`
+}
+
 type StaticCheckSettings struct {
 	// Deprecated: use the global `run.go` instead.
 	GoVersion string `mapstructure:"go"`

pkg/golinters/spancheck.go

@@ -0,0 +1,29 @@
+package golinters
+
+import (
+	"github.com/jjti/go-spancheck"
+	"golang.org/x/tools/go/analysis"
+
+	"github.com/golangci/golangci-lint/pkg/config"
+	"github.com/golangci/golangci-lint/pkg/golinters/goanalysis"
+)
+
+func NewSpancheck(settings *config.SpancheckSettings) *goanalysis.Linter {
+	cfg := spancheck.NewDefaultConfig()
+
+	if settings != nil {
+		if settings.Checks != nil {
+			cfg.EnabledChecks = settings.Checks
+		}
+
+		if settings.IgnoreCheckSignatures != nil {
+			cfg.IgnoreChecksSignaturesSlice = settings.IgnoreCheckSignatures
+		}
+	}
+
+	a := spancheck.NewAnalyzerWithConfig(cfg)
+
+	return goanalysis.
+		NewLinter(a.Name, a.Doc, []*analysis.Analyzer{a}, nil).
+		WithLoadMode(goanalysis.LoadModeTypesInfo)
+}

pkg/lint/lintersdb/manager.go

@@ -131,6 +131,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
 		reviveCfg           *config.ReviveSettings
 		rowserrcheckCfg     *config.RowsErrCheckSettings
 		sloglintCfg         *config.SlogLintSettings
+		spancheckCfg        *config.SpancheckSettings
 		staticcheckCfg      *config.StaticCheckSettings
 		structcheckCfg      *config.StructCheckSettings
 		stylecheckCfg       *config.StaticCheckSettings
@@ -216,6 +217,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
 		reviveCfg = &m.cfg.LintersSettings.Revive
 		rowserrcheckCfg = &m.cfg.LintersSettings.RowsErrCheck
 		sloglintCfg = &m.cfg.LintersSettings.SlogLint
+		spancheckCfg = &m.cfg.LintersSettings.Spancheck
 		staticcheckCfg = &m.cfg.LintersSettings.Staticcheck
 		structcheckCfg = &m.cfg.LintersSettings.Structcheck
 		stylecheckCfg = &m.cfg.LintersSettings.Stylecheck
@@ -782,6 +784,12 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
 			WithLoadForGoAnalysis().
 			WithURL("https://github.com/ryanrolds/sqlclosecheck"),
 
+		linter.NewConfig(golinters.NewSpancheck(spancheckCfg)).
+			WithSince("v1.56.0").
+			WithLoadForGoAnalysis().
+			WithPresets(linter.PresetBugs).
+			WithURL("https://github.com/jjti/go-spancheck"),
+
 		linter.NewConfig(golinters.NewStaticcheck(staticcheckCfg)).
 			WithEnabledByDefault().
 			WithSince("v1.0.0").

test/linters_test.go

@@ -33,6 +33,7 @@ func TestSourcesFromTestdataSubDir(t *testing.T) {
 		"ginkgolinter",
 		"zerologlint",
 		"protogetter",
+		"spancheck",
 	}
 
 	for _, dir := range subDirs {

test/testdata/spancheck/configs/enable_all.yml

@@ -0,0 +1,8 @@
+linters-settings:
+  spancheck:
+    checks:
+      - "end"
+      - "record-error"
+      - "set-status"
+    ignore-check-signatures:
+      - "recordErr"

test/testdata/spancheck/go.mod

@@ -0,0 +1,14 @@
+module spancheck
+
+go 1.20
+
+require (
+	go.opentelemetry.io/otel v1.21.0
+	go.opentelemetry.io/otel/trace v1.21.0
+)
+
+require (
+	github.com/go-logr/logr v1.4.1 // indirect
+	github.com/go-logr/stdr v1.2.2 // indirect
+	go.opentelemetry.io/otel/metric v1.21.0 // indirect
+)

test/testdata/spancheck/go.sum

@@ -0,0 +1,16 @@
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
+github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
+github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
+github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
+github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
+github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+go.opentelemetry.io/otel v1.21.0 h1:hzLeKBZEL7Okw2mGzZ0cc4k/A7Fta0uoPgaJCr8fsFc=
+go.opentelemetry.io/otel v1.21.0/go.mod h1:QZzNPQPm1zLX4gZK4cMi+71eaorMSGT3A4znnUvNNEo=
+go.opentelemetry.io/otel/metric v1.21.0 h1:tlYWfeo+Bocx5kLEloTjbcDwBuELRrIFxwdQ36PlJu4=
+go.opentelemetry.io/otel/metric v1.21.0/go.mod h1:o1p3CA8nNHW8j5yuQLdc1eeqEaPfzug24uvsyIEJRWM=
+go.opentelemetry.io/otel/trace v1.21.0 h1:WD9i5gzvoUPuXIXH24ZNBudiarZDKuekPqi/E8fpfLc=
+go.opentelemetry.io/otel/trace v1.21.0/go.mod h1:LGbsEB0f9LGjN+OZaQQ26sohbOmiMR+BaslueVtS/qQ=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

test/testdata/spancheck/spancheck_default.go

@@ -0,0 +1,90 @@
+//golangcitest:args -Espancheck
+package spancheck
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/codes"
+)
+
+type testDefaultError struct{}
+
+func (e *testDefaultError) Error() string {
+	return "foo"
+}
+
+// incorrect
+
+func _() {
+	otel.Tracer("foo").Start(context.Background(), "bar")           // want "span is unassigned, probable memory leak"
+	ctx, _ := otel.Tracer("foo").Start(context.Background(), "bar") // want "span is unassigned, probable memory leak"
+	fmt.Print(ctx)
+}
+
+func _() {
+	ctx, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.End is not called on all paths, possible memory leak"
+	print(ctx.Done(), span.IsRecording())
+} // want "return can be reached without calling span.End"
+
+func _() {
+	var ctx, span = otel.Tracer("foo").Start(context.Background(), "bar") // want "span.End is not called on all paths, possible memory leak"
+	print(ctx.Done(), span.IsRecording())
+} // want "return can be reached without calling span.End"
+
+func _() {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.End is not called on all paths, possible memory leak"
+	_, span = otel.Tracer("foo").Start(context.Background(), "bar")
+	fmt.Print(span)
+	defer span.End()
+} // want "return can be reached without calling span.End"
+
+// correct
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	return nil
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	if true {
+		return nil
+	}
+
+	return nil
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	if false {
+		err := errors.New("foo")
+		span.SetStatus(codes.Error, err.Error())
+		span.RecordError(err)
+		return err
+	}
+
+	if true {
+		span.SetStatus(codes.Error, "foo")
+		span.RecordError(errors.New("foo"))
+		return errors.New("bar")
+	}
+
+	return nil
+}
+
+func _() {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	_, span = otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+}

test/testdata/spancheck/spancheck_enable_all.go

@@ -0,0 +1,206 @@
+//golangcitest:config_path configs/enable_all.yml
+//golangcitest:args -Espancheck
+package spancheck
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/codes"
+	"go.opentelemetry.io/otel/trace"
+)
+
+type testError struct{}
+
+func (e *testError) Error() string {
+	return "foo"
+}
+
+// incorrect
+
+func _() {
+	otel.Tracer("foo").Start(context.Background(), "bar")           // want "span is unassigned, probable memory leak"
+	ctx, _ := otel.Tracer("foo").Start(context.Background(), "bar") // want "span is unassigned, probable memory leak"
+	fmt.Print(ctx)
+}
+
+func _() {
+	ctx, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.End is not called on all paths, possible memory leak"
+	print(ctx.Done(), span.IsRecording())
+} // want "return can be reached without calling span.End"
+
+func _() {
+	var ctx, span = otel.Tracer("foo").Start(context.Background(), "bar") // want "span.End is not called on all paths, possible memory leak"
+	print(ctx.Done(), span.IsRecording())
+} // want "return can be reached without calling span.End"
+
+func _() {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.End is not called on all paths, possible memory leak"
+	_, span = otel.Tracer("foo").Start(context.Background(), "bar")
+	fmt.Print(span)
+	defer span.End()
+} // want "return can be reached without calling span.End"
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
+	defer span.End()
+
+	if true {
+		err := errors.New("foo")
+		span.RecordError(err)
+		return err // want "return can be reached without calling span.SetStatus"
+	}
+
+	return nil
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
+	defer span.End()
+
+	if true {
+		span.RecordError(errors.New("foo"))
+		return errors.New("foo") // want "return can be reached without calling span.SetStatus"
+	}
+
+	return nil
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
+	defer span.End()
+
+	if true {
+		span.RecordError(errors.New("foo"))
+		return &testError{} // want "return can be reached without calling span.SetStatus"
+	}
+
+	return nil
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.RecordError is not called on all paths"
+	defer span.End()
+
+	if true {
+		span.SetStatus(codes.Error, "foo")
+		return &testError{} // want "return can be reached without calling span.RecordError"
+	}
+
+	return nil
+}
+
+func _() (string, error) {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
+	defer span.End()
+
+	if true {
+		span.RecordError(errors.New("foo"))
+		return "", &testError{} // want "return can be reached without calling span.SetStatus"
+	}
+
+	return "", nil
+}
+
+func _() (string, error) {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
+	defer span.End()
+
+	if true {
+		span.RecordError(errors.New("foo"))
+		return "", errors.New("foo") // want "return can be reached without calling span.SetStatus"
+	}
+
+	return "", nil
+}
+
+func _() {
+	f := func() error {
+		_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
+		defer span.End()
+
+		if true {
+			span.RecordError(errors.New("foo"))
+			return errors.New("foo") // want "return can be reached without calling span.SetStatus"
+		}
+
+		return nil
+	}
+	fmt.Println(f)
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
+	defer span.End()
+
+	{
+		if true {
+			span.RecordError(errors.New("foo"))
+			return errors.New("foo") // want "return can be reached without calling span.SetStatus"
+		}
+	}
+
+	return nil
+}
+
+// correct
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	return nil
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	if true {
+		return nil
+	}
+
+	return nil
+}
+
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	if false {
+		err := errors.New("foo")
+		span.SetStatus(codes.Error, err.Error())
+		span.RecordError(err)
+		return err
+	}
+
+	if true {
+		span.SetStatus(codes.Error, "foo")
+		span.RecordError(errors.New("foo"))
+		return errors.New("bar")
+	}
+
+	return nil
+}
+
+func _() {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	_, span = otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+}
+
+// ignore error because of matching func sig
+func _() error {
+	_, span := otel.Tracer("foo").Start(context.Background(), "bar")
+	defer span.End()
+
+	err := errors.New("foo")
+	recordError(span, err)
+	return err
+}
+
+func recordError(span trace.Span, err error) {}