From 7c906f525a3709132c561e08e111e76630b99a00 Mon Sep 17 00:00:00 2001 From: ~wispem-wantex Date: Wed, 14 Oct 2026 13:03:05 -0700 Subject: [PATCH] initial commit: ifinit and boolnames --- .custom-gcl.yaml | 11 + .gitignore | 1 + .golangci.yaml | 129 +++++++++++ boolnames/analyzer.go | 119 +++++++++++ boolnames/analyzer_test.go | 22 ++ boolnames/plugin.go | 31 +++ boolnames/settings.go | 16 ++ boolnames/testdata/src/cases/boolnames.go | 56 +++++ .../testdata/src/casescustom/boolnames.go | 12 ++ build.sh | 37 ++++ go.mod | 13 ++ go.sum | 10 + ifinit/analyzer.go | 200 ++++++++++++++++++ ifinit/analyzer_test.go | 13 ++ ifinit/plugin.go | 24 +++ ifinit/testdata/src/cases/scopes.go | 88 ++++++++ ifinit/testdata/src/cases/scopes.go.golden | 91 ++++++++ ifinit/testdata/src/cases/structural.go | 113 ++++++++++ .../testdata/src/cases/structural.go.golden | 118 +++++++++++ root.go | 6 + 20 files changed, 1110 insertions(+) create mode 100644 .custom-gcl.yaml create mode 100644 .gitignore create mode 100644 .golangci.yaml create mode 100644 boolnames/analyzer.go create mode 100644 boolnames/analyzer_test.go create mode 100644 boolnames/plugin.go create mode 100644 boolnames/settings.go create mode 100644 boolnames/testdata/src/cases/boolnames.go create mode 100644 boolnames/testdata/src/casescustom/boolnames.go create mode 100755 build.sh create mode 100644 go.mod create mode 100644 go.sum create mode 100644 ifinit/analyzer.go create mode 100644 ifinit/analyzer_test.go create mode 100644 ifinit/plugin.go create mode 100644 ifinit/testdata/src/cases/scopes.go create mode 100644 ifinit/testdata/src/cases/scopes.go.golden create mode 100644 ifinit/testdata/src/cases/structural.go create mode 100644 ifinit/testdata/src/cases/structural.go.golden create mode 100644 root.go diff --git a/.custom-gcl.yaml b/.custom-gcl.yaml new file mode 100644 index 0000000..84f83c4 --- /dev/null +++ b/.custom-gcl.yaml @@ -0,0 +1,11 @@ +# Builds a golangci-lint binary with our own linters (ops/customlint) embedded. +# +# The version below MUST stay in lockstep with the golangci-lint version pinned +# in .github/workflows/ci.yaml: plugins link against golangci-lint internals, so +# a mismatch is a link error, not a graceful degradation. +version: v2.13.1 +name: golint +destination: ./bin +plugins: + - module: git.offline-twitter.com/offline-labs/go-lint + path: . diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..e660fd9 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +bin/ diff --git a/.golangci.yaml b/.golangci.yaml new file mode 100644 index 0000000..fc7cea2 --- /dev/null +++ b/.golangci.yaml @@ -0,0 +1,129 @@ +version: "2" + +linters: + default: none + enable: + # Defaults + - errcheck + - govet + - ineffassign + - staticcheck + - unused + + # Extras + - depguard + - errorlint + - forbidigo + - godox + - lll + - nolintlint + - sqlclosecheck + - whitespace + - modernize + + # Custom linters + - ifinit + - boolnames + + settings: + custom: + ifinit: + type: module + description: if-statement init clauses must be on a single line + boolnames: + type: module + description: boolean variables and functions should use "is", "has", "was" etc + settings: + whitelist: + - ok + forbidigo: + forbid: + - pattern: ^sync\.Map$ + msg: sync.Map loses type safety — use MutexMap instead + analyze-types: true + depguard: + rules: + main: + files: + - $all + deny: + - pkg: io/ioutil + desc: Replace with the matching functions from `io` or `os` packages + - pkg: github.com/pkg/errors + desc: Should be replaced by standard lib errors package + errcheck: + # report about not checking of errors in type assertions: `a := b.(MyStruct)`; + # default is false: such cases aren't reported by default. + check-type-assertions: true + errorlint: + errorf: true # Ensure Errorf only uses %w (not %v or %s etc) for errors + asserts: true # Require errors.As instead of type-asserting + comparison: true # Require errors.Is instead of equality-checking + godox: + # report any comments starting with keywords, this is useful for TODO or FIXME comments that + # might be left in the code accidentally and should be resolved before merging + keywords: # default keywords are TODO, BUG, and FIXME, these can be overwritten by this setting + - XXX + govet: + enable-all: true + disable: + - fieldalignment + lll: + line-length: 140 + tab-width: 4 + nolintlint: + require-explanation: true + require-specific: true + allow-unused: false + staticcheck: + checks: + - all + - -ST1000 # Re-enable this once we have docstrings + dot-import-whitelist: [] + wrapcheck: + report-internal-errors: false + extra-ignore-sigs: + - "json.Marshal(" + exclusions: + rules: + - path: _test.go + linters: + - errcheck + # Vendored from github.com/aldy505/sentry-integration/sqltracer — track + # upstream style, don't wrap errors (would break errors.Is/As across the + # transparent driver wrapper). + - path: pkg/db/sentrysql/ + linters: + - wrapcheck + generated: lax # Don't lint generated files + paths: [] + +formatters: + enable: + - gci + - gofmt + - gofumpt + settings: + gci: + sections: + - standard + - default + - localmodule + gofmt: + simplify: true + gofumpt: + module-path: git.offline-twitter.com/offline-labs/go-lint + extra-rules: false + exclusions: + generated: lax + +run: + build-tags: + - integration + - liveKeycloak + - staging + +issues: + max-same-issues: 0 + max-issues-per-linter: 0 + uniq-by-line: false diff --git a/boolnames/analyzer.go b/boolnames/analyzer.go new file mode 100644 index 0000000..68344ad --- /dev/null +++ b/boolnames/analyzer.go @@ -0,0 +1,119 @@ +package boolnames + +import ( + "fmt" + "go/ast" + "go/types" + "slices" + "strings" + "unicode" + "unicode/utf8" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +var defaultBoolNamePrefixes = []string{"is", "has", "can", "was"} + +var Analyzer = NewAnalyzer(Settings{}) + +func NewAnalyzer(settings Settings) *analysis.Analyzer { + if len(settings.AllowedPrefixes) == 0 { + settings.AllowedPrefixes = defaultBoolNamePrefixes + } + return &analysis.Analyzer{ + Name: "boolnames", + Doc: `checks that boolean values and functions use appropriate names`, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: func(pass *analysis.Pass) (any, error) { + result, ok := pass.ResultOf[inspect.Analyzer] + if !ok { + return nil, fmt.Errorf("inspect analyzer result is unavailable") + } + + insp, ok := result.(*inspector.Inspector) + if !ok { + return nil, fmt.Errorf("inspect analyzer returned %T, want *inspector.Inspector", result) + } + + insp.Preorder([]ast.Node{(*ast.Ident)(nil)}, func(n ast.Node) { + ident, ok := n.(*ast.Ident) + if !ok { + return + } + obj := pass.TypesInfo.Defs[ident] + if obj == nil || ident.Name == "_" { + return + } + + switch obj.(type) { + case *types.Var, *types.Const, *types.Func: + default: + return + } + + if !hasBoolType(obj.Type()) { + return + } + + if settings.IsAcceptableBoolName(ident.Name) { + return + } + + pass.Reportf(ident.Pos(), + "boolean name %q must start with %s", + ident.Name, + strings.Join(settings.AllowedPrefixes, ", "), + ) + }) + + return nil, nil + }, + } +} + +func hasBoolType(t types.Type) bool { + isBool := func(t types.Type) bool { + basic, ok := types.Unalias(t).Underlying().(*types.Basic) + if !ok { + return false + } + + return basic.Kind() == types.Bool || basic.Kind() == types.UntypedBool + } + if isBool(t) { + return true + } + + sig, ok := types.Unalias(t).Underlying().(*types.Signature) + if !ok { + return false + } + + results := sig.Results() + return results.Len() == 1 && isBool(results.At(0).Type()) +} + +func (settings Settings) IsAcceptableBoolName(s string) bool { + if slices.Contains(settings.Whitelist, s) { + return true + } + + for _, pfix := range settings.AllowedPrefixes { + if pfix == "" || len(s) <= len(pfix) || s[1:len(pfix)] != pfix[1:] { + continue + } + + first := s[0] + if first != pfix[0] && first != pfix[0]-'a'+'A' { + continue + } + + sfix := s[len(pfix):] + r, _ := utf8.DecodeRuneInString(sfix) + return unicode.IsUpper(r) + } + + return false +} diff --git a/boolnames/analyzer_test.go b/boolnames/analyzer_test.go new file mode 100644 index 0000000..33da11f --- /dev/null +++ b/boolnames/analyzer_test.go @@ -0,0 +1,22 @@ +package boolnames_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "git.offline-twitter.com/offline-labs/go-lint/boolnames" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), boolnames.Analyzer, "cases") +} + +func TestAnalyzerWithCustomSettings(t *testing.T) { + analyzer := boolnames.NewAnalyzer(boolnames.Settings{ + Whitelist: []string{"ok"}, + AllowedPrefixes: []string{"truthy"}, + }) + + analysistest.Run(t, analysistest.TestData(), analyzer, "casescustom") +} diff --git a/boolnames/plugin.go b/boolnames/plugin.go new file mode 100644 index 0000000..53266f4 --- /dev/null +++ b/boolnames/plugin.go @@ -0,0 +1,31 @@ +package boolnames + +import ( + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +func init() { + register.Plugin("boolnames", New) +} + +type plugin struct { + settings Settings +} + +func New(rawSettings any) (register.LinterPlugin, error) { + settings, err := decodeSettings(rawSettings) + if err != nil { + return nil, err + } + + return &plugin{settings: settings}, nil +} + +func (p *plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{NewAnalyzer(p.settings)}, nil +} + +func (p *plugin) GetLoadMode() string { + return register.LoadModeTypesInfo +} diff --git a/boolnames/settings.go b/boolnames/settings.go new file mode 100644 index 0000000..a09f226 --- /dev/null +++ b/boolnames/settings.go @@ -0,0 +1,16 @@ +package boolnames + +import "github.com/golangci/plugin-module-register/register" + +type Settings struct { + Whitelist []string `json:"whitelist"` + AllowedPrefixes []string `json:"allowed_prefixes"` +} + +func decodeSettings(rawSettings any) (Settings, error) { + if rawSettings == nil { + return Settings{}, nil + } + + return register.DecodeSettings[Settings](rawSettings) +} diff --git a/boolnames/testdata/src/cases/boolnames.go b/boolnames/testdata/src/cases/boolnames.go new file mode 100644 index 0000000..89af511 --- /dev/null +++ b/boolnames/testdata/src/cases/boolnames.go @@ -0,0 +1,56 @@ +package cases + +var isPackageReady bool +var packageReady bool // want `boolean name "packageReady" must` + +const hasPackageValue = true +const packageValue = true // want `boolean name "packageValue" must` + +type FeatureFlag bool + +type Predicate func() bool + +var canEvaluate Predicate +var evaluate Predicate // want `boolean name "evaluate" must` + +type Config struct { + HasOwner bool + owner bool // want `boolean name "owner" must` + + CanRun func() bool + run func() bool // want `boolean name "run" must` +} + +func IsReady() bool { return true } + +func ready() bool { return true } // want `boolean name "ready" must` + +func Find() (int, bool) { return 0, true } +func FindPair() (bool, bool) { return true, false } + +func consume(active bool, hasActive bool, predicate func() bool, canCheck func() bool) (ok bool) { // want `boolean name "consume" must` `boolean name "active" must` `boolean name "predicate" must` `boolean name "ok" must` + ready := true // want `boolean name "ready" must` + isReady := true + + var enabled bool // want `boolean name "enabled" must` + var wasEnabled bool + + var namedFlag FeatureFlag // want `boolean name "namedFlag" must` + var isNamedFlag FeatureFlag + + checker := func() bool { return true } // want `boolean name "checker" must` + hasChecker := func() bool { return true } + + _ = func(active bool) (ok bool) { // want `boolean name "active" must` `boolean name "ok" must` + return ok + } + + return ready || isReady || enabled || wasEnabled || bool(namedFlag) || bool(isNamedFlag) || checker() || hasChecker() || active || hasActive || predicate() || canCheck() +} + +func (flag FeatureFlag) String() string { // want `boolean name "flag" must` + if flag { + return "true" + } + return "false" +} diff --git a/boolnames/testdata/src/casescustom/boolnames.go b/boolnames/testdata/src/casescustom/boolnames.go new file mode 100644 index 0000000..c70eae7 --- /dev/null +++ b/boolnames/testdata/src/casescustom/boolnames.go @@ -0,0 +1,12 @@ +package casescustom + +var ok bool +var truthyValue bool + +var ready bool // want `boolean name "ready" must start with truthy` +var isReady bool // want `boolean name "isReady" must start with truthy` +var hasOwner bool // want `boolean name "hasOwner" must start with truthy` +var truthy bool // want `boolean name "truthy" must start with truthy` + +func truthyCheck() bool { return true } +func canCheck() bool { return true } // want `boolean name "canCheck" must start with truthy` diff --git a/build.sh b/build.sh new file mode 100755 index 0000000..4b77b00 --- /dev/null +++ b/build.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env sh + +set -e + +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) +cd "$SCRIPT_DIR" # .custom-gcl.yaml is only discovered in the current directory + +BIN="bin/golint" +STAMP="bin/.golint.stamp" + +# Everything that changes the compiled binary: the linter sources, and the build +# config that pins the golangci-lint version the plugin links against. testdata +# is excluded -- fixtures are not compiled into the binary. +current_stamp() { + { + find . -type f ! -path '*/testdata/*' | sort | xargs shasum -a 256 + shasum -a 256 .custom-gcl.yaml + } | shasum -a 256 | cut -d' ' -f1 +} + +if [ "$1" = "--if-needed" ] && + [ -x "$BIN" ] && + [ -f "$STAMP" ] && + [ "$(cat "$STAMP")" = "$(current_stamp)" ]; then + exit 0 +fi + +if ! [ -x "$(command -v golangci-lint)" ]; then + echo "golangci-lint is not installed. Please install it from https://golangci-lint.run/welcome/install/" + exit 1 +fi + +echo "Building golint (golangci-lint + custom linters)..." +golangci-lint custom +current_stamp > "$STAMP" + +echo "Done: $SCRIPT_DIR/$BIN" diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..e4750ee --- /dev/null +++ b/go.mod @@ -0,0 +1,13 @@ +module git.offline-twitter.com/offline-labs/go-lint + +go 1.26.3 + +require ( + github.com/golangci/plugin-module-register v0.1.2 + golang.org/x/tools v0.50.0 +) + +require ( + golang.org/x/mod v0.41.0 // indirect + golang.org/x/sync v0.23.0 // indirect +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..8f5ecf0 --- /dev/null +++ b/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.2 h1:e5WM6PO6NIAEcij3B053CohVp3HIYbzSuP53UAYgOpg= +github.com/golangci/plugin-module-register v0.1.2/go.mod h1:1+QGTsKBvAIvPvoY/os+G5eoqxWn70HYDm2uvUyGuVw= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.41.0 h1:qJmnOUb4YB+FsEuM3HcWucdZASCPGhsX6uljO6pog0c= +golang.org/x/mod v0.41.0/go.mod h1:Ek9pY8RKWXwsWvd3rQiHYtMqkjSUV+s1Rj7j4H5Ur6o= +golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= +golang.org/x/sync v0.23.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= +golang.org/x/tools v0.50.0 h1:c2ifzfcuY7L90lZ2aKd8S4K2NpASF08SZx9ZuJkHmSU= +golang.org/x/tools v0.50.0/go.mod h1:7ulVMw3831Mwi5EZD6RomGyffr4VFjuNYXf2BbCEAV0= diff --git a/ifinit/analyzer.go b/ifinit/analyzer.go new file mode 100644 index 0000000..5542562 --- /dev/null +++ b/ifinit/analyzer.go @@ -0,0 +1,200 @@ +// Package ifinit reports if-statement init clauses that span more than one line. +package ifinit + +import ( + "fmt" + "go/ast" + "go/token" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +const doc = `checks that if-statement init clauses occupy a single line + +A multi-line init clause pushes the condition being tested far away from the +"if" keyword, so the reader has to scan past the setup to find out what is +actually being decided. The fix hoists the init clause into its own statement +above the if.` + +var Analyzer = &analysis.Analyzer{ + Name: "ifinit", + Doc: doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + result, ok := pass.ResultOf[inspect.Analyzer] + if !ok { + return nil, fmt.Errorf("inspect analyzer result is unavailable") + } + + insp, ok := result.(*inspector.Inspector) + if !ok { + return nil, fmt.Errorf("inspect analyzer returned %T, want *inspector.Inspector", result) + } + + insp.WithStack([]ast.Node{(*ast.IfStmt)(nil)}, func(n ast.Node, isPush bool, stack []ast.Node) bool { + if !isPush { + return false + } + + stmt, ok := n.(*ast.IfStmt) + if !ok { + return true + } + if stmt.Init == nil { + return true + } + + start := pass.Fset.Position(stmt.Init.Pos()) + end := pass.Fset.Position(stmt.Init.End()) + if start.Line == end.Line { + return true + } + + diag := analysis.Diagnostic{ + Pos: stmt.Init.Pos(), + End: stmt.Init.End(), + Message: fmt.Sprintf("if statement init clause spans %d lines; it must be on a single line", end.Line-start.Line+1), + } + + if fix, ok := hoist(pass, stmt, stack); ok { + diag.SuggestedFixes = []analysis.SuggestedFix{fix} + } + + pass.Report(diag) + + return true + }) + + return nil, nil +} + +// hoist rewrites "if ; {" into "" on its own line followed by +// "if {". The init clause's own text is left untouched, so comments and +// internal line breaks survive. +func hoist(pass *analysis.Pass, stmt *ast.IfStmt, stack []ast.Node) (analysis.SuggestedFix, bool) { + if !isHoistable(stmt, stack) { + return analysis.SuggestedFix{}, false + } + + if !isShadowSafe(pass, stmt) { + return analysis.SuggestedFix{}, false + } + + indent, ok := lineIndent(pass, stmt.Pos()) + if !ok { + return analysis.SuggestedFix{}, false + } + + return analysis.SuggestedFix{ + Message: "Hoist the init clause above the if statement", + TextEdits: []analysis.TextEdit{ + // Drop the leading "if ". + {Pos: stmt.Pos(), End: stmt.Init.Pos()}, + // Turn the "; " separator into a line break and a fresh "if ". + {Pos: stmt.Init.End(), End: stmt.Cond.Pos(), NewText: []byte("\n" + indent + "if ")}, + }, + }, true +} + +func isHoistable(stmt *ast.IfStmt, stack []ast.Node) bool { + if len(stack) < 2 { + return false + } + + switch parent := stack[len(stack)-2].(type) { + case *ast.IfStmt: + // An "else if": the init clause cannot move above the chain without + // restructuring the whole thing into a nested else block. + return parent.Else != ast.Stmt(stmt) + case *ast.LabeledStmt: + // Hoisting would leave the label pointing at the init clause instead. + return false + } + + return true +} + +// shadowSafe reports whether hoisting stmt's init clause out of the if's scope +// leaves every name it binds meaning the same thing. +// +// Only ":=" declares anything, and only in the if statement's own scope. Moving +// that declaration up one level is safe exactly when none of the names it binds +// already resolve to something in the enclosing scope: otherwise the hoisted +// ":=" either assigns to that existing object instead of declaring a fresh one, +// or collides with a later declaration in the same block. +// +// The check is deliberately conservative. It refuses whenever a name resolves at +// all -- including to a package-level or universe name -- rather than trying to +// prove that nothing reads the shadowed object afterwards. +func isShadowSafe(pass *analysis.Pass, stmt *ast.IfStmt) bool { + assign, ok := stmt.Init.(*ast.AssignStmt) + if !ok || assign.Tok != token.DEFINE { + return true + } + + scope := pass.TypesInfo.Scopes[stmt] + if scope == nil { + return false + } + + enclosing := scope.Parent() + if enclosing == nil { + return false + } + + for _, lhs := range assign.Lhs { + ident, ok := lhs.(*ast.Ident) + if !ok { + return false + } + + if ident.Name == "_" { + continue + } + + // token.NoPos makes the lookup ignore declaration order, so a name + // declared later in the same block counts as a conflict too. + if _, obj := enclosing.LookupParent(ident.Name, token.NoPos); obj != nil { + return false + } + } + + return true +} + +// lineIndent returns the whitespace preceding pos on its line, and reports +// whether pos is the first non-whitespace token on that line. +func lineIndent(pass *analysis.Pass, pos token.Pos) (string, bool) { + if pass.ReadFile == nil { + return "", false + } + + file := pass.Fset.File(pos) + if file == nil { + return "", false + } + + src, err := pass.ReadFile(file.Name()) + if err != nil { + return "", false + } + + lineStart := file.Offset(file.LineStart(file.Line(pos))) + offset := file.Offset(pos) + if offset > len(src) { + return "", false + } + + prefix := string(src[lineStart:offset]) + if strings.TrimLeft(prefix, " \t") != "" { + return "", false + } + + return prefix, true +} diff --git a/ifinit/analyzer_test.go b/ifinit/analyzer_test.go new file mode 100644 index 0000000..ee191ab --- /dev/null +++ b/ifinit/analyzer_test.go @@ -0,0 +1,13 @@ +package ifinit_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "git.offline-twitter.com/offline-labs/go-lint/ifinit" +) + +func TestAnalyzer(t *testing.T) { + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), ifinit.Analyzer, "cases") +} diff --git a/ifinit/plugin.go b/ifinit/plugin.go new file mode 100644 index 0000000..7df9097 --- /dev/null +++ b/ifinit/plugin.go @@ -0,0 +1,24 @@ +package ifinit + +import ( + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +func init() { + register.Plugin("ifinit", New) +} + +type plugin struct{} + +func New(any) (register.LinterPlugin, error) { + return &plugin{}, nil +} + +func (p *plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{Analyzer}, nil +} + +func (p *plugin) GetLoadMode() string { + return register.LoadModeTypesInfo +} diff --git a/ifinit/testdata/src/cases/scopes.go b/ifinit/testdata/src/cases/scopes.go new file mode 100644 index 0000000..964c8e8 --- /dev/null +++ b/ifinit/testdata/src/cases/scopes.go @@ -0,0 +1,88 @@ +package cases + +func do(args ...int) error { return nil } + +var errState error + +// Safe: both names are fresh in the block that would receive the hoisted statement. +func freshNames() { + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } +} + +// Safe: "_" declares nothing and err is fresh. +func blankAndFresh() { + if _, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = err + } +} + +// Safe: "=" assigns to an existing variable; it declares nothing at all. +func plainAssign() error { + var err error + + if err = do( // want `init clause spans 3 lines` + 1, + ); err != nil { + return err + } + + return nil +} + +// Unsafe: err already exists in the enclosing block. Hoisting the ":=" would +// assign to that err rather than declaring a new one, so the returned error +// would silently change. +func shadowsExistingLocal() error { + v, err := multi(1) + if err != nil { + return err + } + _ = v + + if v2, err := multi( // want `init clause spans 3 lines` + 2, + ); err != nil { + _ = v2 + } + + return err +} + +// Unsafe: the same names are redeclared later in this block, so hoisting turns +// the later ":=" into "no new variables on left side of :=". +func declaredLaterInBlock() { + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } + + v, err := multi(2) + _, _ = v, err +} + +// Unsafe: err is a parameter, so hoisting would rebind it for the rest of the body. +func shadowsParam(err error) { + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } +} + +// Conservatively refused: shadowing a package-level name. Nothing reads errState +// after the if here, so this one would in fact be safe -- the check does not try +// to prove that. +func shadowsPackageLevel() { + if v, errState := multi( // want `init clause spans 3 lines` + 1, + ); errState != nil { + _ = v + } +} diff --git a/ifinit/testdata/src/cases/scopes.go.golden b/ifinit/testdata/src/cases/scopes.go.golden new file mode 100644 index 0000000..ce3c815 --- /dev/null +++ b/ifinit/testdata/src/cases/scopes.go.golden @@ -0,0 +1,91 @@ +package cases + +func do(args ...int) error { return nil } + +var errState error + +// Safe: both names are fresh in the block that would receive the hoisted statement. +func freshNames() { + v, err := multi( // want `init clause spans 3 lines` + 1, + ) + if err != nil { + _ = v + } +} + +// Safe: "_" declares nothing and err is fresh. +func blankAndFresh() { + _, err := multi( // want `init clause spans 3 lines` + 1, + ) + if err != nil { + _ = err + } +} + +// Safe: "=" assigns to an existing variable; it declares nothing at all. +func plainAssign() error { + var err error + + err = do( // want `init clause spans 3 lines` + 1, + ) + if err != nil { + return err + } + + return nil +} + +// Unsafe: err already exists in the enclosing block. Hoisting the ":=" would +// assign to that err rather than declaring a new one, so the returned error +// would silently change. +func shadowsExistingLocal() error { + v, err := multi(1) + if err != nil { + return err + } + _ = v + + if v2, err := multi( // want `init clause spans 3 lines` + 2, + ); err != nil { + _ = v2 + } + + return err +} + +// Unsafe: the same names are redeclared later in this block, so hoisting turns +// the later ":=" into "no new variables on left side of :=". +func declaredLaterInBlock() { + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } + + v, err := multi(2) + _, _ = v, err +} + +// Unsafe: err is a parameter, so hoisting would rebind it for the rest of the body. +func shadowsParam(err error) { + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } +} + +// Conservatively refused: shadowing a package-level name. Nothing reads errState +// after the if here, so this one would in fact be safe -- the check does not try +// to prove that. +func shadowsPackageLevel() { + if v, errState := multi( // want `init clause spans 3 lines` + 1, + ); errState != nil { + _ = v + } +} diff --git a/ifinit/testdata/src/cases/structural.go b/ifinit/testdata/src/cases/structural.go new file mode 100644 index 0000000..8156e39 --- /dev/null +++ b/ifinit/testdata/src/cases/structural.go @@ -0,0 +1,113 @@ +package cases + +func multi(args ...int) (int, error) { return 0, nil } + +func single() (int, error) { return 0, nil } + +func count(args ...int) int { return 0 } + +func clean() { + if v, err := single(); err != nil { + _ = v + } + + x := count( + 1, + 2, + ) + if x > 0 { + _ = x + } +} + +func multiLineInit() { + if v, err := multi( // want `init clause spans 4 lines` + 1, + 2, + ); err != nil { + _ = v + } +} + +func elseIfChain() { + if x := count(1); x > 0 { + _ = x + } else if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } +} + +func nestedInsideBody() { + if true { + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } + } +} + +// Only "if" init clauses are covered; for and switch are deliberately untouched. +func otherInitClauses() { + for i := count( + 1, + ); i < 10; i++ { + _ = i + } + + switch v := count( + 1, + ); v { + case 1: + } +} + +func doSomething(args ...int) error { return nil } + +func setup(args ...int) {} + +// The line break falls after ":=" rather than inside the call's arguments. +func brokenAfterAssignOp() { + if result, err := // want `init clause spans 2 lines` + multi(1, 2); err != nil { + _ = result + } +} + +// Assignment to an existing variable rather than a definition. +func assignNotDefine() error { + var err error + if err = doSomething( // want `init clause spans 4 lines` + 1, + 2, + ); err != nil { + return err + } + + return nil +} + +// The init clause is a bare call expression, not an assignment at all. +func exprStmtInit(condition bool) { + if setup( // want `init clause spans 4 lines` + 1, + 2, + ); condition { + _ = condition + } +} + +// gofmt joins "else" and "if", but the parser accepts them on separate lines -- +// and then the "if" really is the first token on its line. Hoisting here would +// emit "} v, err := multi(...)", which does not compile. +func elseOnItsOwnLine() { + if true { + } else + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } +} diff --git a/ifinit/testdata/src/cases/structural.go.golden b/ifinit/testdata/src/cases/structural.go.golden new file mode 100644 index 0000000..c19d97d --- /dev/null +++ b/ifinit/testdata/src/cases/structural.go.golden @@ -0,0 +1,118 @@ +package cases + +func multi(args ...int) (int, error) { return 0, nil } + +func single() (int, error) { return 0, nil } + +func count(args ...int) int { return 0 } + +func clean() { + if v, err := single(); err != nil { + _ = v + } + + x := count( + 1, + 2, + ) + if x > 0 { + _ = x + } +} + +func multiLineInit() { + v, err := multi( // want `init clause spans 4 lines` + 1, + 2, + ) + if err != nil { + _ = v + } +} + +func elseIfChain() { + if x := count(1); x > 0 { + _ = x + } else if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } +} + +func nestedInsideBody() { + if true { + v, err := multi( // want `init clause spans 3 lines` + 1, + ) + if err != nil { + _ = v + } + } +} + +// Only "if" init clauses are covered; for and switch are deliberately untouched. +func otherInitClauses() { + for i := count( + 1, + ); i < 10; i++ { + _ = i + } + + switch v := count( + 1, + ); v { + case 1: + } +} + +func doSomething(args ...int) error { return nil } + +func setup(args ...int) {} + +// The line break falls after ":=" rather than inside the call's arguments. +func brokenAfterAssignOp() { + result, err := // want `init clause spans 2 lines` + multi(1, 2) + if err != nil { + _ = result + } +} + +// Assignment to an existing variable rather than a definition. +func assignNotDefine() error { + var err error + err = doSomething( // want `init clause spans 4 lines` + 1, + 2, + ) + if err != nil { + return err + } + + return nil +} + +// The init clause is a bare call expression, not an assignment at all. +func exprStmtInit(condition bool) { + setup( // want `init clause spans 4 lines` + 1, + 2, + ) + if condition { + _ = condition + } +} + +// gofmt joins "else" and "if", but the parser accepts them on separate lines -- +// and then the "if" really is the first token on its line. Hoisting here would +// emit "} v, err := multi(...)", which does not compile. +func elseOnItsOwnLine() { + if true { + } else + if v, err := multi( // want `init clause spans 3 lines` + 1, + ); err != nil { + _ = v + } +} diff --git a/root.go b/root.go new file mode 100644 index 0000000..dff0412 --- /dev/null +++ b/root.go @@ -0,0 +1,6 @@ +package customlint + +import ( + _ "git.offline-twitter.com/offline-labs/go-lint/boolnames" + _ "git.offline-twitter.com/offline-labs/go-lint/ifinit" +)