diff --git a/CHANGELOG.md b/CHANGELOG.md index aecd6f7..53dd763 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- add rule `AZG012`: in a body of more than five statements, an `if`/`for`/`switch`/`select` must be followed by a blank line before the next step; short bodies and runs of one-line guards are left alone; fixable with `-fix`; `statements` and `lines` set what counts as short ([#71](https://github.com/katbyte/azproviderlint/pull/71)) - add rule `AZT003`: test functions named `TestAcc` that never run an acceptance test, so are scheduled as one without touching Azure; report only, no fix ([#64](https://github.com/katbyte/azproviderlint/pull/64)) ## v0.11.0 (2026-09-28) diff --git a/README.md b/README.md index ba48d62..694a648 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ Rules are named `AZ`, a category letter, and a number. The letters follow [tfpro | [AZG008](checks/AZG/AZG008_unchecked_nil_dereference) | pointer dereferences must have a nil guard or use pointer.From | | [AZG009](checks/AZG/AZG009_pointer_from_enum_conversion) | use pointer.FromEnum for enum conversions | | [AZG010](checks/AZG/AZG010_unneeded_import_alias) | drop import aliases that are not needed | +| [AZG012](checks/AZG/AZG012_missing_blank_line_between_steps) | separate the steps of a longer function body with blank lines | ### AZP - Provider-Wide Conventions diff --git a/checks/AZG/AZG.go b/checks/AZG/AZG.go index 741df84..3d90ae3 100644 --- a/checks/AZG/AZG.go +++ b/checks/AZG/AZG.go @@ -14,6 +14,7 @@ import ( AZG008 "github.com/katbyte/azproviderlint/checks/AZG/AZG008_unchecked_nil_dereference" AZG009 "github.com/katbyte/azproviderlint/checks/AZG/AZG009_pointer_from_enum_conversion" AZG010 "github.com/katbyte/azproviderlint/checks/AZG/AZG010_unneeded_import_alias" + AZG012 "github.com/katbyte/azproviderlint/checks/AZG/AZG012_missing_blank_line_between_steps" ) // Checks contains all AZG (general Go style & readability) analyzers. @@ -28,4 +29,5 @@ var Checks = []*analysis.Analyzer{ AZG008.Analyzer, AZG009.Analyzer, AZG010.Analyzer, + AZG012.Analyzer, } diff --git a/checks/AZG/AZG012_missing_blank_line_between_steps/AZG012.go b/checks/AZG/AZG012_missing_blank_line_between_steps/AZG012.go new file mode 100644 index 0000000..802dae9 --- /dev/null +++ b/checks/AZG/AZG012_missing_blank_line_between_steps/AZG012.go @@ -0,0 +1,131 @@ +// Package AZG012 defines an analyzer that reports a block statement run straight into the +// next step of a longer function body without a blank line between them. +package AZG012 + +import ( + "flag" + "go/ast" + "go/token" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + + "github.com/katbyte/azproviderlint/lib/astx" +) + +// Analyzer checks that in a body with more than a few statements, an if, for, switch or select +// is separated from the statement that follows it by a blank line, so a reader sees where one +// step ends and the next begins. A short body is one group and is left alone, as is a run of +// one-line ifs, which read as a single set of guards. +var Analyzer = &analysis.Analyzer{ + Name: "AZG012", + Doc: "check that the steps of a longer function body are separated by blank lines", + URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZG/AZG012_missing_blank_line_between_steps/README.md", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +// A body with at most this many statements, or spanning at most this many lines, is one +// group: no blank lines are asked for inside it. +var ( + maxStatements = 5 + maxLines = 10 +) + +func init() { + Analyzer.Flags.Init("AZG012", flag.ContinueOnError) + Analyzer.Flags.IntVar(&maxStatements, "statements", maxStatements, "bodies with this many statements or fewer are one group") + Analyzer.Flags.IntVar(&maxLines, "lines", maxLines, "bodies spanning this many lines or fewer are one group") +} + +func run(pass *analysis.Pass) (any, error) { + insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, nil + } + + line := func(p token.Pos) int { return pass.Fset.Position(p).Line } + check := func(stmts []ast.Stmt, open, closing token.Pos) { + if len(stmts) <= maxStatements || line(closing)-line(open)+1 <= maxLines { + return + } + + for i := 0; i+1 < len(stmts); i++ { + a, s := stmts[i], stmts[i+1] + kind := blockKind(a) + if kind == "" { + continue + } + + switch s.(type) { + case *ast.CaseClause, *ast.CommClause: + continue + } + + if _, isIf := s.(*ast.IfStmt); isIf && oneLiner(a) && oneLiner(s) { + continue + } + + // a comment between them belongs to the next step, so the blank goes before it + at := s.Pos() + for _, cg := range astx.EnclosingFile(pass, s.Pos()).Comments { + if cg.Pos() > a.End() && cg.End() < s.Pos() && line(cg.Pos()) > line(a.End()) && line(cg.Pos()) < line(at) { + at = cg.Pos() + } + } + + if line(at) != line(a.End())+1 { + continue + } + + tf := pass.Fset.File(at) + start := tf.LineStart(tf.Line(at)) + pass.Report(analysis.Diagnostic{ + Pos: at, + Message: "a blank line should separate this from the " + kind + " above, which ends a step", + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Insert a blank line", + TextEdits: []analysis.TextEdit{{Pos: start, End: start, NewText: []byte("\n")}}, + }}, + }) + } + } + + insp.Preorder([]ast.Node{(*ast.BlockStmt)(nil), (*ast.CaseClause)(nil), (*ast.CommClause)(nil)}, func(n ast.Node) { + switch n := n.(type) { + case *ast.BlockStmt: + check(n.List, n.Lbrace, n.Rbrace) + case *ast.CaseClause: + check(n.Body, n.Pos(), n.End()) + case *ast.CommClause: + check(n.Body, n.Pos(), n.End()) + } + }) + + return nil, nil +} + +// blockKind names a statement with a body, or is empty for anything else: only these end a +// step. A literal assignment that happens to close with a brace does not. +func blockKind(s ast.Stmt) string { + switch s.(type) { + case *ast.IfStmt: + return "if" + case *ast.ForStmt, *ast.RangeStmt: + return "for" + case *ast.SwitchStmt, *ast.TypeSwitchStmt: + return "switch" + case *ast.SelectStmt: + return "select" + case *ast.BlockStmt: + return "block" + } + return "" +} + +// oneLiner reports whether s is an if without else whose body is a single statement: a guard. +func oneLiner(s ast.Stmt) bool { + i, ok := s.(*ast.IfStmt) + return ok && i.Else == nil && len(i.Body.List) == 1 +} diff --git a/checks/AZG/AZG012_missing_blank_line_between_steps/AZG012_test.go b/checks/AZG/AZG012_missing_blank_line_between_steps/AZG012_test.go new file mode 100644 index 0000000..14bb540 --- /dev/null +++ b/checks/AZG/AZG012_missing_blank_line_between_steps/AZG012_test.go @@ -0,0 +1,22 @@ +package AZG012 + +import ( + "path/filepath" + "runtime" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAZG012(t *testing.T) { + t.Parallel() + + _, filename, _, _ := runtime.Caller(0) + dir := filepath.Join(filepath.Dir(filename), "testdata") + + analysistest.RunWithSuggestedFixes(t, dir, Analyzer, "azg012") + + maxStatements, maxLines = 2, 4 + analysistest.Run(t, dir, Analyzer, "azg012short") + maxStatements, maxLines = 5, 10 +} diff --git a/checks/AZG/AZG012_missing_blank_line_between_steps/README.md b/checks/AZG/AZG012_missing_blank_line_between_steps/README.md new file mode 100644 index 0000000..3e1329f --- /dev/null +++ b/checks/AZG/AZG012_missing_blank_line_between_steps/README.md @@ -0,0 +1,86 @@ +# AZG012 - separate the steps of a longer function body with blank lines + +AZG012 reports an `if`, `for`, `switch` or `select` in a longer body that is run straight into the statement after it, with no blank line between. + +A body of any length is read as a sequence of steps. A block ends one, and the statement after it starts the next: get something, check it, use it. A blank line between them is what shows the boundary. Without it a twenty-line loop body reads as one undifferentiated run. + +## Flagged Code + +```go +for _, decl := range file.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Body == nil { + continue + } + fn, ok := pass.TypesInfo.Defs[fd.Name].(*types.Func) + if !ok || runs[fn] != nil { + continue + } + rest := strings.TrimPrefix(fn.Name(), "TestAcc") + if rest != "" && unicode.IsLower(rune(rest[0])) { + pass.Reportf(fd.Name.Pos(), "...") + continue + } + pass.Reportf(fd.Name.Pos(), "...") +} +``` + +## Passing Code + +```go +for _, decl := range file.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Body == nil { + continue + } + + fn, ok := pass.TypesInfo.Defs[fd.Name].(*types.Func) + if !ok || runs[fn] != nil { + continue + } + + rest := strings.TrimPrefix(fn.Name(), "TestAcc") + if rest != "" && unicode.IsLower(rune(rest[0])) { + pass.Reportf(fd.Name.Pos(), "...") + continue + } + + pass.Reportf(fd.Name.Pos(), "...") +} +``` + +## What counts + +Only a statement with a body ends a step: `if`, `for`, `switch`, `select`, or a bare block. A literal assignment that happens to close with a brace does not. Closures are checked like any other body, and so are the bodies of `case` clauses. + +Not reported: + +- a short body, which is one group: five statements or fewer, or ten lines or fewer, see [Options](#options) +- two one-line `if`s in a row, which read as a single set of guards +- a block followed by the next `case` or `default` +- anywhere a blank line is already there + +A comment on the line after the block belongs to the next step, so the report, and the blank line, go before the comment. + +## The fix + +`-fix` inserts the blank line. + +## Options + +| Option | Default | Effect | +|---|---|---| +| `statements` | 5 | bodies with this many statements or fewer are one group | +| `lines` | 10 | bodies spanning this many lines or fewer are one group | + +A body is left alone when either holds. Set with `-AZG012.