From 5558ba7e70483d313703f1fba9d8ca789413bf88 Mon Sep 17 00:00:00 2001 From: Dominik Roos Date: Fri, 25 Mar 2022 17:26:24 +0100 Subject: [PATCH] lint: move linters from oncilla/gochecks Move the linters from https://github.com/oncilla/gochecks and update them to the updated package paths. Closes #4174 GitOrigin-RevId: 219b897858ff56ff94dd7103ab0d44aad283282e --- .bazelignore | 1 + BUILD.bazel | 1 + .../data/com_github_oncilla_gochecks/LICENSE | 21 --- nogo.bzl | 4 +- nogo.json | 119 +++++++-------- tools/lint.sh | 1 + tools/lint/BUILD.bazel | 9 ++ tools/lint/lint.go | 68 +++++++++ tools/lint/logctxcheck/BUILD.bazel | 22 +++ tools/lint/logctxcheck/analyzer.go | 135 +++++++++++++++++ tools/lint/logctxcheck/analyzer_test.go | 41 +++++ .../logctxcheck/testdata/src/fail/fail.go | 114 ++++++++++++++ .../src/github.com/scionproto/scion/pkg/log | 1 + .../logctxcheck/testdata/src/named/named.go | 33 +++++ tools/lint/serrorscheck/BUILD.bazel | 22 +++ tools/lint/serrorscheck/analyzer.go | 140 ++++++++++++++++++ tools/lint/serrorscheck/analyzer_test.go | 41 +++++ .../serrorscheck/testdata/src/fail/fail.go | 84 +++++++++++ .../serrorscheck/testdata/src/named/named.go | 39 +++++ 19 files changed, 814 insertions(+), 82 deletions(-) delete mode 100644 licenses/data/com_github_oncilla_gochecks/LICENSE create mode 100644 tools/lint/lint.go create mode 100644 tools/lint/logctxcheck/BUILD.bazel create mode 100644 tools/lint/logctxcheck/analyzer.go create mode 100644 tools/lint/logctxcheck/analyzer_test.go create mode 100644 tools/lint/logctxcheck/testdata/src/fail/fail.go create mode 120000 tools/lint/logctxcheck/testdata/src/github.com/scionproto/scion/pkg/log create mode 100644 tools/lint/logctxcheck/testdata/src/named/named.go create mode 100644 tools/lint/serrorscheck/BUILD.bazel create mode 100644 tools/lint/serrorscheck/analyzer.go create mode 100644 tools/lint/serrorscheck/analyzer_test.go create mode 100644 tools/lint/serrorscheck/testdata/src/fail/fail.go create mode 100644 tools/lint/serrorscheck/testdata/src/named/named.go diff --git a/.bazelignore b/.bazelignore index 05b3443ccb..19dbe95779 100644 --- a/.bazelignore +++ b/.bazelignore @@ -1,3 +1,4 @@ bin docker/_build rules_openapi/tools/node_modules +tools/lint/logctxcheck/testdata/src diff --git a/BUILD.bazel b/BUILD.bazel index ee389214e9..d82c95b707 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -15,6 +15,7 @@ load("@cgrindel_bazel_starlib//updatesrc:defs.bzl", "updatesrc_update_all") # gazelle:exclude proto/** # gazelle:exclude doc/** # gazelle:exclude rules_openapi/tools/node_modules/** +# gazelle:exclude tools/lint/**/testdata/src/** gazelle(name = "gazelle") go_lint_config( diff --git a/licenses/data/com_github_oncilla_gochecks/LICENSE b/licenses/data/com_github_oncilla_gochecks/LICENSE deleted file mode 100644 index 06be989a64..0000000000 --- a/licenses/data/com_github_oncilla_gochecks/LICENSE +++ /dev/null @@ -1,21 +0,0 @@ -MIT License - -Copyright (c) 2019 Oncilla - -Permission is hereby granted, free of charge, to any person obtaining a copy -of this software and associated documentation files (the "Software"), to deal -in the Software without restriction, including without limitation the rights -to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -copies of the Software, and to permit persons to whom the Software is -furnished to do so, subject to the following conditions: - -The above copyright notice and this permission notice shall be included in all -copies or substantial portions of the Software. - -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -SOFTWARE. diff --git a/nogo.bzl b/nogo.bzl index 487383d6ee..2dbbcf68d4 100644 --- a/nogo.bzl +++ b/nogo.bzl @@ -1,8 +1,8 @@ nogo_deps = [ "@com_github_scionproto_scion//tools/lint/log:go_default_library", "@com_github_scionproto_scion//tools/lint/maincheck:go_default_library", - "@com_github_oncilla_gochecks//logcheck:go_default_library", - "@com_github_oncilla_gochecks//serrorscheck:go_default_library", + "@com_github_scionproto_scion//tools/lint/serrorscheck:go_default_library", + "@com_github_scionproto_scion//tools/lint/logctxcheck:go_default_library", "@org_golang_x_tools//go/analysis/passes/asmdecl:go_default_library", "@org_golang_x_tools//go/analysis/passes/assign:go_default_library", "@org_golang_x_tools//go/analysis/passes/atomic:go_default_library", diff --git a/nogo.json b/nogo.json index fecc04ef7c..1a9a50acce 100644 --- a/nogo.json +++ b/nogo.json @@ -13,7 +13,8 @@ "/com_github_grpc_ecosystem_grpc_opentracing/go/otgrpc/": "", "/com_github_vishvananda_netlink/": "", "/org_golang_x_tools/internal": "", - "/org_golang_google_protobuf": "" + "/org_golang_google_protobuf": "", + "/org_golang_x_tools/go/analysis/internal/checker": "" } }, "copylocks": { @@ -145,64 +146,64 @@ "/com_github_golang_mock/mockgen": "" } }, - "reflectvaluecompare": { - "exclude_files": { - "in_gopkg_yaml_v2": "", - "com_github_smartystreets_assertions": "" - } - }, - "shadow": { - "exclude_files": { - "org_golang_x_text": "", - "org_golang_x_net": "", - "org_golang_x_sys/unix": "", - "org_golang_x_crypto/": "", - "org_golang_x_tools": "", - "org_golang_google_protobuf": "", - "org_golang_google_grpc": "", - "in_gopkg_yaml_v2/": "", - "in_gopkg_yaml_v3": "", - "in_gopkg_ini_v1": "", - "com_github_google_go_cmp/cmp": "", - "com_github_google_gopacket": "", - "com_github_golang_protobuf": "", - "com_github_matttproud_golang_protobuf_extensions": "", - "com_github_getkin_kin_openapi": "", - "com_github_buildkite_go_buildkite_v2/buildkite": "", - "com_github_deepmap_oapi_codegen/pkg/runtime": "", - "com_github_uber_jaeger_client_go/thrift": "", - "com_github_hashicorp_hcl": "", - "com_github_stretchr_testify/assert": "", - "com_github_spf13_afero": "", - "com_github_marten_seemann_qtls_go1_16": "", - "com_github_marten_seemann_qtls_go1_17": "", - "com_github_lucas_clemente_quic_go": "", - "com_github_lestrrat_go_jwx": "", - "com_github_pelletier_go_toml": "", - "com_github_mdlayher_raw": "", - "com_github_vishvananda_netlink": "", - "com_github_prometheus_common/internal": "", - "com_github_prometheus_procfs": "", - "com_github_prometheus_client_golang": "", - "com_github_uber_jaeger_client_go": "", - "com_github_spf13_viper": "", - "com_github_labstack_echo_v4": "", - "com_github_deepmap_oapi_codegen": "", + "reflectvaluecompare": { + "exclude_files": { + "in_gopkg_yaml_v2": "", + "com_github_smartystreets_assertions": "" + } + }, + "shadow": { + "exclude_files": { + "org_golang_x_text": "", + "org_golang_x_net": "", + "org_golang_x_sys/unix": "", + "org_golang_x_crypto/": "", + "org_golang_x_tools": "", + "org_golang_google_protobuf": "", + "org_golang_google_grpc": "", + "in_gopkg_yaml_v2/": "", + "in_gopkg_yaml_v3": "", + "in_gopkg_ini_v1": "", + "com_github_google_go_cmp/cmp": "", + "com_github_google_gopacket": "", + "com_github_golang_protobuf": "", + "com_github_matttproud_golang_protobuf_extensions": "", + "com_github_getkin_kin_openapi": "", + "com_github_buildkite_go_buildkite_v2/buildkite": "", + "com_github_deepmap_oapi_codegen/pkg/runtime": "", + "com_github_uber_jaeger_client_go/thrift": "", + "com_github_hashicorp_hcl": "", + "com_github_stretchr_testify/assert": "", + "com_github_spf13_afero": "", + "com_github_marten_seemann_qtls_go1_16": "", + "com_github_marten_seemann_qtls_go1_17": "", + "com_github_lucas_clemente_quic_go": "", + "com_github_lestrrat_go_jwx": "", + "com_github_pelletier_go_toml": "", + "com_github_mdlayher_raw": "", + "com_github_vishvananda_netlink": "", + "com_github_prometheus_common/internal": "", + "com_github_prometheus_procfs": "", + "com_github_prometheus_client_golang": "", + "com_github_uber_jaeger_client_go": "", + "com_github_spf13_viper": "", + "com_github_labstack_echo_v4": "", + "com_github_deepmap_oapi_codegen": "", - "pkg/private/serrors/errors.go": "err shadowed", - "pkg/slayers/path/scion/raw.go": "err shadowed", - "pkg/scrypto/cppki/trc.go": "err shadowed", - "private/mgmtapi/jwtauth/jwt.go": "err shadowed", - "dispatcher/underlay.go": "err shadowed" - } - }, - "unusedwrite": { - "exclude_files": { - "org_golang_x_mod": "", - "org_golang_x_crypto/ed25519/internal/edwards25519": "", - "com_github_vishvananda_netlink/nl": "", - "com_github_deepmap_oapi_codegen/pkg/codegen": "", - "control/colibri/reservation/e2e/request_test.go": "" + "pkg/private/serrors/errors.go": "err shadowed", + "pkg/slayers/path/scion/raw.go": "err shadowed", + "pkg/scrypto/cppki/trc.go": "err shadowed", + "private/mgmtapi/jwtauth/jwt.go": "err shadowed", + "dispatcher/underlay.go": "err shadowed" + } + }, + "unusedwrite": { + "exclude_files": { + "org_golang_x_mod": "", + "org_golang_x_crypto/ed25519/internal/edwards25519": "", + "com_github_vishvananda_netlink/nl": "", + "com_github_deepmap_oapi_codegen/pkg/codegen": "", + "control/colibri/reservation/e2e/request_test.go": "" + } } - } } diff --git a/tools/lint.sh b/tools/lint.sh index 07869a364c..b9733ae2f5 100755 --- a/tools/lint.sh +++ b/tools/lint.sh @@ -30,6 +30,7 @@ go_lint() { -a '!' -ipath '*/node_modules/*' \ -a '!' -ipath './scion-pki/certs/certinfo.go' \ -a '!' -ipath './scion-pki/certs/certformat.go' \ + -a '!' -ipath './tools/lint/*/testdata/src/*' \ -a '!' -ipath '*mock_*' > $TMPDIR/gofiles.list lint_step "Building lint tools" diff --git a/tools/lint/BUILD.bazel b/tools/lint/BUILD.bazel index e69de29bb2..bd421a3a21 100644 --- a/tools/lint/BUILD.bazel +++ b/tools/lint/BUILD.bazel @@ -0,0 +1,9 @@ +load("//tools/lint:go.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["lint.go"], + importpath = "github.com/scionproto/scion/tools/lint", + visibility = ["//visibility:public"], + deps = ["@org_golang_x_tools//go/analysis:go_default_library"], +) diff --git a/tools/lint/lint.go b/tools/lint/lint.go new file mode 100644 index 0000000000..d5095cb4db --- /dev/null +++ b/tools/lint/lint.go @@ -0,0 +1,68 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package lint contains helpers for linting and static analysis. +package lint + +import ( + "bytes" + "fmt" + "go/ast" + "go/printer" + "go/token" + "go/types" + "path" + "strings" + + "golang.org/x/tools/go/analysis" +) + +// FindPackageNames returns all the imported packages mapped to their name in +// the file. +func FindPackageNames(file *ast.File) map[string]string { + pkgs := map[string]string{} + for _, imp := range file.Imports { + pkg := strings.Trim(imp.Path.Value, `"`) + name := path.Base(pkg) + if imp.Name != nil { + name = imp.Name.Name + } + pkgs[pkg] = name + } + return pkgs +} + +// IsString checks if the underlying type of the literal is string. +func IsString(pass *analysis.Pass, lit ast.Expr) bool { + t, ok := pass.TypesInfo.TypeOf(lit).Underlying().(*types.Basic) + return ok && t.Info()&types.IsString != 0 +} + +// RenderList renders the list of expressions. +func RenderList(fset *token.FileSet, list []ast.Expr) string { + var p []string + for _, arg := range list { + p = append(p, Render(fset, arg)) + } + return fmt.Sprintf("[%s]", strings.Join(p, ",")) +} + +// Render renders the the expression. +func Render(fset *token.FileSet, x interface{}) string { + var buf bytes.Buffer + if err := printer.Fprint(&buf, fset, x); err != nil { + panic(err) + } + return buf.String() +} diff --git a/tools/lint/logctxcheck/BUILD.bazel b/tools/lint/logctxcheck/BUILD.bazel new file mode 100644 index 0000000000..af2ecd4ccf --- /dev/null +++ b/tools/lint/logctxcheck/BUILD.bazel @@ -0,0 +1,22 @@ +load("//tools/lint:go.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/scionproto/scion/tools/lint/logctxcheck", + visibility = ["//visibility:public"], + deps = [ + "//tools/lint:go_default_library", + "@org_golang_x_tools//go/analysis:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["analyzer_test.go"], + data = glob(["testdata/**"]), + deps = [ + ":go_default_library", + "@org_golang_x_tools//go/analysis/analysistest:go_default_library", + ], +) diff --git a/tools/lint/logctxcheck/analyzer.go b/tools/lint/logctxcheck/analyzer.go new file mode 100644 index 0000000000..4c10c929ec --- /dev/null +++ b/tools/lint/logctxcheck/analyzer.go @@ -0,0 +1,135 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package logctxcheck + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + + "github.com/scionproto/scion/tools/lint" +) + +var importPath = "github.com/scionproto/scion/pkg/log" + +// Analyzer checks all calls on the log package. +var Analyzer = &analysis.Analyzer{ + Name: "logctxcheck", + Doc: "reports invalid log calls", + Run: run, + RunDespiteErrors: true, +} + +func run(pass *analysis.Pass) (interface{}, error) { + for _, file := range pass.Files { + if _, ok := lint.FindPackageNames(file)[importPath]; !ok { + continue + } + + ast.Inspect(file, func(n ast.Node) bool { + ce, ok := n.(*ast.CallExpr) + if !ok { + return true + } + se, ok := ce.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + + if !isTarget(pass, se) { + return true + } + var varargs []ast.Expr + switch se.Sel.Name { + case "Debug", "Info", "Error": + if len(ce.Args) < 2 { + return true + } + varargs = ce.Args[1:] + case "New": + varargs = ce.Args + } + // We cannot check if varargs with ellipsis. + if ce.Ellipsis != token.NoPos { + return true + } + if len(varargs)%2 != 0 { + pass.Reportf( + varargs[0].Pos(), + "context should be even: len=%d ctx=%s expr=%q", + len(varargs), + lint.RenderList(pass.Fset, varargs), + lint.Render(pass.Fset, ce), + ) + } + for i := 0; i < len(varargs); i += 2 { + lit := varargs[i] + if lint.IsString(pass, lit) { + continue + } + pass.Reportf( + lit.Pos(), + "key should be string: type=%q name=%q expr=%q", + pass.TypesInfo.TypeOf(lit), + lint.Render(pass.Fset, lit), + lint.Render(pass.Fset, ce), + ) + } + seen := map[string]bool{} + for i := 0; i < len(varargs); i += 2 { + lit := varargs[i] + k := litKey(lit) + if !seen[k] { + seen[k] = true + continue + } + pass.Reportf( + lit.Pos(), + "duplicate key in context: name=%q expr=%q", + lint.Render(pass.Fset, lit), + lint.Render(pass.Fset, ce), + ) + } + return true + }) + } + return nil, nil +} + +func isTarget(pass *analysis.Pass, se *ast.SelectorExpr) bool { + pkgIdent, _ := se.X.(*ast.Ident) + pkgName, ok := pass.TypesInfo.Uses[pkgIdent].(*types.PkgName) + if ok && pkgName.Imported().Path() == importPath { + return true + } + if typ := pass.TypesInfo.TypeOf(se.X); typ != nil { + return typ.String() == importPath+".Logger" + } + return false +} + +func litKey(lit ast.Expr) string { + switch v := lit.(type) { + case *ast.BasicLit: + return v.Value + case *ast.Ident: + return v.Name + default: + return fmt.Sprint(lit) + } +} diff --git a/tools/lint/logctxcheck/analyzer_test.go b/tools/lint/logctxcheck/analyzer_test.go new file mode 100644 index 0000000000..4f4a60f0db --- /dev/null +++ b/tools/lint/logctxcheck/analyzer_test.go @@ -0,0 +1,41 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package logctxcheck_test + +import ( + "os" + "strings" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/scionproto/scion/tools/lint/logctxcheck" +) + +func Test(t *testing.T) { + if strings.HasSuffix(os.Getenv("TEST_TARGET"), "go_default_test") { + t.Skip("Bazel test not supported: https://github.com/bazelbuild/rules_go/issues/2370") + } + testdata := analysistest.TestData() + analysistest.Run(t, testdata, logctxcheck.Analyzer, "fail") +} + +func TestNamed(t *testing.T) { + if strings.HasSuffix(os.Getenv("TEST_TARGET"), "go_default_test") { + t.Skip("Bazel test not supported: https://github.com/bazelbuild/rules_go/issues/2370") + } + testdata := analysistest.TestData() + analysistest.Run(t, testdata, logctxcheck.Analyzer, "named") +} diff --git a/tools/lint/logctxcheck/testdata/src/fail/fail.go b/tools/lint/logctxcheck/testdata/src/fail/fail.go new file mode 100644 index 0000000000..4ae8ad5dcc --- /dev/null +++ b/tools/lint/logctxcheck/testdata/src/fail/fail.go @@ -0,0 +1,114 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fail + +import ( + "context" + + "github.com/scionproto/scion/pkg/log" +) + +const ( + untyped = "untyped_key" + typed key = "typed_key" +) + +var value = 1 + +var s struct { + logger log.Logger +} + +func validParity() { + log.Debug("message") + log.Info("message") + log.Error("message") + + log.Debug("message", "key", value) + log.Info("message", "key", value) + log.Error("message", "key", value) + + log.Debug("message", "key", value, "key1", value) + log.Info("message", "key", value, "key1", value) + log.Error("message", "key", value, "key1", value) +} + +func validTypes() { + log.Debug("message", "key", value) + log.Debug("message", untyped, value) + log.Debug("message", typed, value) +} + +func invalidParity() { + log.Debug("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + log.Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + log.Error("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + + log.Debug("message", "key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + log.Info("message", "key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + log.Error("message", "key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` +} + +func invalidType() { + log.Info("message", value, value) // want `key should be string: type="int" name="value"` +} + +func loggerNew() { + log.New() + log.New("key", value) + log.New("key", value, "key1", value) + log.New("key") // want `context should be even: len=1 ctx=\["key"\]` + log.New("key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + log.New("key", value, "key", value) // want `duplicate key in context:` + + loggerN := log.New() + loggerN.New() + loggerN.New("key", value) + loggerN.New("key", value, "key1", value) + loggerN.New("key") // want `context should be even: len=1 ctx=\["key"\]` + loggerN.New("key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + loggerN.New("key", value, "key", value) // want `duplicate key in context:` + + loggerC := log.FromCtx(context.Background()) + loggerC.New() + loggerC.New("key", value) + loggerC.New("key", value, "key1", value) + loggerC.New("key") // want `context should be even: len=1 ctx=\["key"\]` + loggerC.New("key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + loggerC.New("key", value, "key", value) // want `duplicate key in context:` + + s.logger.New() + s.logger.New("key", value) + s.logger.New("key", value, "key1", value) + s.logger.New("key") // want `context should be even: len=1 ctx=\["key"\]` + s.logger.New("key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + s.logger.New("key", value, "key", value) // want `duplicate key in context:` +} + +func loggerLog() { + loggerC := log.FromCtx(context.Background()) + loggerC.Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + log.FromCtx(context.Background()).Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + + loggerN := log.New() + loggerN.Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + log.New().Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + + loggerR := log.Root() + loggerR.Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + log.Root().Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` +} + +type key string diff --git a/tools/lint/logctxcheck/testdata/src/github.com/scionproto/scion/pkg/log b/tools/lint/logctxcheck/testdata/src/github.com/scionproto/scion/pkg/log new file mode 120000 index 0000000000..e86440b89c --- /dev/null +++ b/tools/lint/logctxcheck/testdata/src/github.com/scionproto/scion/pkg/log @@ -0,0 +1 @@ +../../../../../../../../../pkg/log \ No newline at end of file diff --git a/tools/lint/logctxcheck/testdata/src/named/named.go b/tools/lint/logctxcheck/testdata/src/named/named.go new file mode 100644 index 0000000000..dc79caafe6 --- /dev/null +++ b/tools/lint/logctxcheck/testdata/src/named/named.go @@ -0,0 +1,33 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package named + +import ( + log "fake/lib/slog" + + slog "github.com/scionproto/scion/pkg/log" +) + +func fakeImportIgnored() { + log.Debug("message", nil) + log.Info("message", "a") + log.Error("messsage", nil) +} + +func invalid() { + slog.Debug("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + slog.Info("message", "key") // want `context should be even: len=1 ctx=\["key"\]` + slog.Error("messsage", "key") // want `context should be even: len=1 ctx=\["key"\]` +} diff --git a/tools/lint/serrorscheck/BUILD.bazel b/tools/lint/serrorscheck/BUILD.bazel new file mode 100644 index 0000000000..76879b9f91 --- /dev/null +++ b/tools/lint/serrorscheck/BUILD.bazel @@ -0,0 +1,22 @@ +load("//tools/lint:go.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/scionproto/scion/tools/lint/serrorscheck", + visibility = ["//visibility:public"], + deps = [ + "//tools/lint:go_default_library", + "@org_golang_x_tools//go/analysis:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["analyzer_test.go"], + data = glob(["testdata/**"]), + deps = [ + ":go_default_library", + "@org_golang_x_tools//go/analysis/analysistest:go_default_library", + ], +) diff --git a/tools/lint/serrorscheck/analyzer.go b/tools/lint/serrorscheck/analyzer.go new file mode 100644 index 0000000000..8088423ae8 --- /dev/null +++ b/tools/lint/serrorscheck/analyzer.go @@ -0,0 +1,140 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package serrorscheck + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + + "github.com/scionproto/scion/tools/lint" +) + +var importPath = "github.com/scionproto/scion/pkg/private/serrors" + +// Analyzer checks all calls on the serrors package. +var Analyzer = &analysis.Analyzer{ + Name: "serrorscheck", + Doc: "reports invalid serrors calls", + Run: run, + RunDespiteErrors: true, +} + +func run(pass *analysis.Pass) (interface{}, error) { + for _, file := range pass.Files { + _, ok := lint.FindPackageNames(file)[importPath] + if !ok { + continue + } + + ast.Inspect(file, func(n ast.Node) bool { + ce, ok := n.(*ast.CallExpr) + if !ok { + return true + } + se, ok := ce.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + + pkgIdent, _ := se.X.(*ast.Ident) + pkgName, ok := pass.TypesInfo.Uses[pkgIdent].(*types.PkgName) + if !ok || pkgName.Imported().Path() != importPath { + return true + } + + var varargs []ast.Expr + switch se.Sel.Name { + case "New": + if len(ce.Args) < 2 { + return true + } + varargs = ce.Args[1:] + case "WithCtx": + if len(ce.Args) < 2 { + pass.Reportf( + ce.Pos(), + "context is missing: expr=%q", + lint.Render(pass.Fset, ce), + ) + return true + } + varargs = ce.Args[1:] + case "Wrap", "WrapStr": + if len(ce.Args) < 3 { + return true + } + varargs = ce.Args[2:] + } + // We cannot check if varargs with ellipsis. + if ce.Ellipsis != token.NoPos { + return true + } + if len(varargs)%2 != 0 { + pass.Reportf( + varargs[0].Pos(), + "context should be even: len=%d ctx=%s expr=%q", + len(varargs), + lint.RenderList(pass.Fset, varargs), + lint.Render(pass.Fset, ce), + ) + } + for i := 0; i < len(varargs); i += 2 { + lit := varargs[i] + if lint.IsString(pass, lit) { + continue + } + pass.Reportf( + lit.Pos(), + "key in context should be string: type=%q name=%q expr=%q", + pass.TypesInfo.TypeOf(lit), + lint.Render(pass.Fset, lit), + lint.Render(pass.Fset, ce), + ) + } + seen := map[string]bool{} + for i := 0; i < len(varargs); i += 2 { + lit := varargs[i] + k := litKey(lit) + if !seen[k] { + seen[k] = true + continue + } + pass.Reportf( + lit.Pos(), + "duplicate key in context: name=%q expr=%q", + lint.Render(pass.Fset, lit), + lint.Render(pass.Fset, ce), + ) + } + return true + }) + } + return nil, nil +} + +func litKey(lit ast.Expr) string { + switch v := lit.(type) { + case *ast.BasicLit: + return v.Value + case *ast.Ident: + return v.Name + default: + return fmt.Sprint(lit) + } +} diff --git a/tools/lint/serrorscheck/analyzer_test.go b/tools/lint/serrorscheck/analyzer_test.go new file mode 100644 index 0000000000..5e9ab37e8c --- /dev/null +++ b/tools/lint/serrorscheck/analyzer_test.go @@ -0,0 +1,41 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package serrorscheck_test + +import ( + "os" + "strings" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/scionproto/scion/tools/lint/serrorscheck" +) + +func Test(t *testing.T) { + if strings.HasSuffix(os.Getenv("TEST_TARGET"), "go_default_test") { + t.Skip("Bazel test not supported: https://github.com/bazelbuild/rules_go/issues/2370") + } + testdata := analysistest.TestData() + analysistest.Run(t, testdata, serrorscheck.Analyzer, "fail") +} + +func TestNamed(t *testing.T) { + if strings.HasSuffix(os.Getenv("TEST_TARGET"), "go_default_test") { + t.Skip("Bazel test not supported: https://github.com/bazelbuild/rules_go/issues/2370") + } + testdata := analysistest.TestData() + analysistest.Run(t, testdata, serrorscheck.Analyzer, "named") +} diff --git a/tools/lint/serrorscheck/testdata/src/fail/fail.go b/tools/lint/serrorscheck/testdata/src/fail/fail.go new file mode 100644 index 0000000000..87de598924 --- /dev/null +++ b/tools/lint/serrorscheck/testdata/src/fail/fail.go @@ -0,0 +1,84 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fail + +import "github.com/scionproto/scion/pkg/private/serrors" + +const ( + untyped = "untyped_key" + typed key = "typed_key" +) + +var ( + errWrap = serrors.New("wrap") + errBase = serrors.New("base") + value = 1 +) + +func validParity() { + serrors.New("some error") + serrors.Wrap(errWrap, errBase) + serrors.WrapStr("wrap", errBase) + + serrors.New("some error", "key", value) + serrors.WithCtx(errBase, "key", value) + serrors.Wrap(errWrap, errBase, "key", value) + serrors.WrapStr("wrap", errBase, "key", value) + + serrors.New("some error", "key", value, "key1", value) + serrors.WithCtx(errBase, "key", value, "key1", value) + serrors.Wrap(errWrap, errBase, "key", value, "key1", value) + serrors.WrapStr("wrap", errBase, "key", value, "key1", value) +} + +func validTypes() { + serrors.New("some error", "key", value) + serrors.New("some error", untyped, value) + serrors.New("some error", typed, value) +} + +func invalidParity() { + serrors.New("some error", "key") // want `context should be even: len=1 ctx=\["key"\]` + serrors.WithCtx(errBase, "key") // want `context should be even: len=1 ctx=\["key"\]` + serrors.Wrap(errWrap, errBase, "key") // want `context should be even: len=1 ctx=\["key"\]` + serrors.WrapStr("wrap", errBase, "key") // want `context should be even: len=1 ctx=\["key"\]` + + serrors.New("some error", "key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + serrors.WithCtx(errBase, "key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + serrors.Wrap(errWrap, errBase, "key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` + serrors.WrapStr("wrap", errBase, "key", value, "key1") // want `context should be even: len=3 ctx=\["key",value,"key1"\]` +} + +func invalidType() { + serrors.New("some error", value, value) // want `key in context should be string: type="int" name="value"` + serrors.WithCtx(errBase, value, value) // want `key in context should be string: type="int" name="value"` + serrors.Wrap(errWrap, errBase, value, value) // want `key in context should be string: type="int" name="value"` + serrors.WrapStr("wrap", errBase, value, value) // want `key in context should be string: type="int" name="value"` +} + +func noCtx() { + serrors.WithCtx(errBase) // want `context is missing:` +} + +func duplicateKey() { + serrors.New("some error", "key", value, "key", value) // want `duplicate key in context:` + serrors.WithCtx(errBase, "key", value, "key", value) // want `duplicate key in context:` + serrors.Wrap(errWrap, errBase, "key", value, "key", value) // want `duplicate key in context:` + serrors.WrapStr("wrap", errBase, "key", value, "key", value) // want `duplicate key in context:` +} + +type key string + +type multiKey key diff --git a/tools/lint/serrorscheck/testdata/src/named/named.go b/tools/lint/serrorscheck/testdata/src/named/named.go new file mode 100644 index 0000000000..c6606c58cc --- /dev/null +++ b/tools/lint/serrorscheck/testdata/src/named/named.go @@ -0,0 +1,39 @@ +// Copyright 2022 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package named + +import ( + serrors "fake/serrors" + + errors "github.com/scionproto/scion/pkg/private/serrors" +) + +var ( + errWrap = errors.New("wrap") + errBase = errors.New("base") + value = 1 +) + +func fakeImportIgnored() { + serrors.New("some error", "key") + serrors.Wrap(errWrap, errBase, "key") +} + +func invalid() { + errors.New("some error", "key") // want `context should be even: len=1 ctx=\["key"\]` + errors.WithCtx(errBase, "key") // want `context should be even: len=1 ctx=\["key"\]` + errors.Wrap(errWrap, errBase, "key") // want `context should be even: len=1 ctx=\["key"\]` + errors.WrapStr("wrap", errBase, "key") // want `context should be even: len=1 ctx=\["key"\]` +}