From d23c35470b899af8de151db9f960a60c92bcf08a Mon Sep 17 00:00:00 2001 From: Joshua Timmons Date: Tue, 2 Jan 2024 22:24:30 -0500 Subject: [PATCH] feat: add `spancheck` linter (#4290) Co-authored-by: Fernandez Ludovic --- .golangci.reference.yml | 20 ++ go.mod | 1 + go.sum | 2 + pkg/config/linters_settings.go | 8 +- pkg/golinters/spancheck.go | 29 +++ pkg/lint/lintersdb/manager.go | 8 + test/linters_test.go | 1 + .../testdata/spancheck/configs/enable_all.yml | 8 + test/testdata/spancheck/go.mod | 14 ++ test/testdata/spancheck/go.sum | 16 ++ test/testdata/spancheck/spancheck_default.go | 90 ++++++++ .../spancheck/spancheck_enable_all.go | 206 ++++++++++++++++++ 12 files changed, 402 insertions(+), 1 deletion(-) create mode 100644 pkg/golinters/spancheck.go create mode 100644 test/testdata/spancheck/configs/enable_all.yml create mode 100644 test/testdata/spancheck/go.mod create mode 100644 test/testdata/spancheck/go.sum create mode 100644 test/testdata/spancheck/spancheck_default.go create mode 100644 test/testdata/spancheck/spancheck_enable_all.go diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 4b219ee882c4..c21108ba81e8 100644 --- a/.golangci.reference.yml +++ b/.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 diff --git a/go.mod b/go.mod index 291258824606..92a81fb903b4 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 7d7b4121ded9..0b979a22d37e 100644 --- a/go.sum +++ b/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= diff --git a/pkg/config/linters_settings.go b/pkg/config/linters_settings.go index a79a2a61e681..fd7c5f80e796 100644 --- a/pkg/config/linters_settings.go +++ b/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"` diff --git a/pkg/golinters/spancheck.go b/pkg/golinters/spancheck.go new file mode 100644 index 000000000000..934124477767 --- /dev/null +++ b/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) +} diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index c4e222493d5e..e30d48e4f10c 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/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"). diff --git a/test/linters_test.go b/test/linters_test.go index 75d4f44adf1d..1019a1f09c42 100644 --- a/test/linters_test.go +++ b/test/linters_test.go @@ -33,6 +33,7 @@ func TestSourcesFromTestdataSubDir(t *testing.T) { "ginkgolinter", "zerologlint", "protogetter", + "spancheck", } for _, dir := range subDirs { diff --git a/test/testdata/spancheck/configs/enable_all.yml b/test/testdata/spancheck/configs/enable_all.yml new file mode 100644 index 000000000000..1ea3518098bb --- /dev/null +++ b/test/testdata/spancheck/configs/enable_all.yml @@ -0,0 +1,8 @@ +linters-settings: + spancheck: + checks: + - "end" + - "record-error" + - "set-status" + ignore-check-signatures: + - "recordErr" diff --git a/test/testdata/spancheck/go.mod b/test/testdata/spancheck/go.mod new file mode 100644 index 000000000000..de347098a901 --- /dev/null +++ b/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 +) diff --git a/test/testdata/spancheck/go.sum b/test/testdata/spancheck/go.sum new file mode 100644 index 000000000000..e09fe7a75588 --- /dev/null +++ b/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= diff --git a/test/testdata/spancheck/spancheck_default.go b/test/testdata/spancheck/spancheck_default.go new file mode 100644 index 000000000000..e1fff76da927 --- /dev/null +++ b/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() +} diff --git a/test/testdata/spancheck/spancheck_enable_all.go b/test/testdata/spancheck/spancheck_enable_all.go new file mode 100644 index 000000000000..382e714043e4 --- /dev/null +++ b/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) {}