diff --git a/_docs/dsl.md b/_docs/dsl.md index 1ad1b571..d49e4343 100644 --- a/_docs/dsl.md +++ b/_docs/dsl.md @@ -55,6 +55,28 @@ A `Report()` argument string can use `$` notation to interpolate the na There is a special variable `$$` which can be used to inject the entire pattern match into the message (like `$0` in regular expressions). +### Documenting your rules + +It's a good practice to add structured documentation for your rule groups. + +To add such documentation, use special pragmas when commenting a matcher function. + +```go +//doc:summary reports always false/true conditions +//doc:before strings.Count(s, "/") >= 0 +//doc:after strings.Count(s, "/") > 0 +//doc:tags diagnostic exprimental +func badCond(m dsl.Matcher) { + m.Match(`strings.Count($_, $_) >= 0`).Report(`statement always true`) + m.Match(`bytes.Count($_, $_) >= 0`).Report(`statement always true`) +} +``` + +* `summary` - short one sentence description +* `before` - code snippet of code that will violate rule +* `after` - code after a fix (one that complies to the rule) +* `tags` - space separated list of custom tags + ### Filters The rule is matched if: diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index f4490fef..4ba1529b 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -99,7 +99,7 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) { fullMessage := msg if printRuleLocation { fullMessage = fmt.Sprintf("%s: %s (%s:%d)", - info.Group, msg, filepath.Base(info.Filename), info.Line) + info.Group, msg, filepath.Base(info.Group.Filename), info.Line) } diag := analysis.Diagnostic{ Pos: n.Pos(), diff --git a/ruleguard/gorule.go b/ruleguard/gorule.go index 08aee913..35fdc801 100644 --- a/ruleguard/gorule.go +++ b/ruleguard/gorule.go @@ -31,7 +31,7 @@ type goCommentRule struct { } type goRule struct { - group string + group *GoRuleGroup filename string line int pat *gogrep.Pattern diff --git a/ruleguard/parser.go b/ruleguard/parser.go index 94826d49..538622af 100644 --- a/ruleguard/parser.go +++ b/ruleguard/parser.go @@ -13,6 +13,7 @@ import ( "path" "regexp" "strconv" + "strings" "github.com/quasilyte/go-ruleguard/internal/gogrep" "github.com/quasilyte/go-ruleguard/nodetag" @@ -42,7 +43,7 @@ type rulesParser struct { importedPkg string // Package path; only for imported packages filename string - group string + group *GoRuleGroup res *goRuleSet pkg *types.Package types *types.Info @@ -88,7 +89,7 @@ func (p *rulesParser) ParseFile(filename string, r io.Reader) (*goRuleSet, error groups: make(map[string]token.Position), } - parserFlags := parser.Mode(0) + parserFlags := parser.ParseComments f, err := parser.ParseFile(p.ctx.Fset, filename, r, parserFlags) if err != nil { return nil, fmt.Errorf("parse file error: %w", err) @@ -302,18 +303,25 @@ func (p *rulesParser) parseRuleGroup(f *ast.FuncDecl) (err error) { params := f.Type.Params.List matcher := params[0].Names[0].Name - p.group = f.Name.Name + p.group = &GoRuleGroup{ + Name: f.Name.Name, + } if p.prefix != "" { - p.group = p.prefix + "/" + f.Name.Name + p.group.Name = p.prefix + "/" + p.group.Name + } + if f.Doc != nil { + if err := p.parseDocComments(f.Doc); err != nil { + return err + } } - if p.ctx.GroupFilter != nil && !p.ctx.GroupFilter(p.group) { + if p.ctx.GroupFilter != nil && !p.ctx.GroupFilter(p.group.Name) { return nil // Skip this group } - if _, ok := p.res.groups[p.group]; ok { + if _, ok := p.res.groups[p.group.Name]; ok { panic(fmt.Sprintf("duplicated function %s after the typecheck", p.group)) // Should never happen } - p.res.groups[p.group] = token.Position{ + p.res.groups[p.group.Name] = token.Position{ Filename: p.filename, Line: p.ctx.Fset.Position(f.Name.Pos()).Line, } @@ -336,9 +344,49 @@ func (p *rulesParser) parseRuleGroup(f *ast.FuncDecl) (err error) { if err := p.parseCall(matcher, call); err != nil { return err } + } + + return nil +} +func (p *rulesParser) parseDocComments(comment *ast.CommentGroup) error { + knownPragmas := []string{ + "tags", + "summary", + "before", + "after", } + for _, c := range comment.List { + if !strings.HasPrefix(c.Text, "//doc:") { + continue + } + s := strings.TrimPrefix(c.Text, "//doc:") + var pragma string + for i := range knownPragmas { + if strings.HasPrefix(s, knownPragmas[i]) { + pragma = knownPragmas[i] + break + } + } + if pragma == "" { + return p.errorf(c, errors.New("unrecognized 'doc' pragma in comment")) + } + s = strings.TrimPrefix(s, pragma) + s = strings.TrimSpace(s) + switch pragma { + case "summary": + p.group.DocSummary = s + case "before": + p.group.DocBefore = s + case "after": + p.group.DocAfter = s + case "tags": + p.group.DocTags = strings.Fields(s) + default: + panic("unhandled 'doc' pragma: " + pragma) // Should never happen + } + } return nil } diff --git a/ruleguard/ruleguard.go b/ruleguard/ruleguard.go index ba23861a..e51b53bc 100644 --- a/ruleguard/ruleguard.go +++ b/ruleguard/ruleguard.go @@ -76,12 +76,32 @@ type Suggestion struct { } type GoRuleInfo struct { - // Filename is a file that defined this rule. - Filename string - // Line is a line inside a file that defined this rule. Line int - // Group is a function name that contained this rule. - Group string + // Group is a function that contains this rule. + Group *GoRuleGroup +} + +type GoRuleGroup struct { + // Name is a function name associated with this rule group. + Name string + + // Filename is a file that defined this rule group. + Filename string + + // DocTags contains a list of keys from the `gorules:tags` comment. + DocTags []string + + // DocSummary is a short one sentence description. + // Filled from the `gorules:summary` doc content. + DocSummary string + + // DocBefore is a code snippet of code that will violate rule. + // Filled from the `gorules:before` doc content. + DocBefore string + + // DocAfter is a code snippet of fixed code that complies to the rule. + // Filled from the `gorules:after` doc content. + DocAfter string } diff --git a/ruleguard/runner.go b/ruleguard/runner.go index a5d25441..3efb5c3c 100644 --- a/ruleguard/runner.go +++ b/ruleguard/runner.go @@ -193,7 +193,7 @@ func (rr *rulesRunner) runRules(n ast.Node) { } func (rr *rulesRunner) reject(rule goRule, reason string, m matchData) { - if rule.group != rr.ctx.Debug { + if rule.group.Name != rr.ctx.Debug { return // This rule is not being debugged } @@ -261,9 +261,8 @@ func (rr *rulesRunner) handleCommentMatch(rule goCommentRule, m commentMatchData } } info := GoRuleInfo{ - Group: rule.base.group, - Filename: rule.base.filename, - Line: rule.base.line, + Group: rule.base.group, + Line: rule.base.line, } rr.ctx.Report(info, node, message, suggestion) return true @@ -293,9 +292,8 @@ func (rr *rulesRunner) handleMatch(rule goRule, m gogrep.MatchData) bool { } } info := GoRuleInfo{ - Group: rule.group, - Filename: rule.filename, - Line: rule.line, + Group: rule.group, + Line: rule.line, } rr.ctx.Report(info, node, message, suggestion) return true diff --git a/rules/diag.go b/rules/diag.go index e166576d..25ec1223 100644 --- a/rules/diag.go +++ b/rules/diag.go @@ -6,6 +6,10 @@ import ( var Bundle = dsl.Bundle{} +//doc:summary reports always false/true conditions +//doc:before strings.Count(s, "/") >= 0 +//doc:after strings.Count(s, "/") > 0 +//doc:tags diagnostic func badCond(m dsl.Matcher) { m.Match(`strings.Count($_, $_) >= 0`).Report(`statement always true`) m.Match(`bytes.Count($_, $_) >= 0`).Report(`statement always true`) diff --git a/rules/refactor.go b/rules/refactor.go index 7f1e5cae..f46a1e28 100644 --- a/rules/refactor.go +++ b/rules/refactor.go @@ -4,6 +4,10 @@ import ( "github.com/quasilyte/go-ruleguard/dsl" ) +//doc:summary suggests sorting function alternatives +//doc:before sort.Slice(xs, func(i, j int) bool { return xs[i] < xs[j] }) +//doc:after sort.Ints(xs) +//doc:tags refactor func sortFuncs(m dsl.Matcher) { m.Match(`sort.Slice($s, func($i, $j int) bool { return $s[$i] < $s[$j] })`). Where(m["s"].Type.Is(`[]string`)). diff --git a/rules/style.go b/rules/style.go index f5b3029f..fd86f25c 100644 --- a/rules/style.go +++ b/rules/style.go @@ -4,18 +4,30 @@ import ( "github.com/quasilyte/go-ruleguard/dsl" ) +//doc:summary reports redundant parentheses +//doc:before f(x, (y)) +//doc:after f(x, y) +//doc:tags style func exprUnparen(m dsl.Matcher) { m.Match(`$f($*_, ($x), $*_)`). Report(`the parentheses around $x are superfluous`). Suggest(`$f($x)`) } +//doc:summary reports empty declaration blocks +//doc:before var () +//doc:after /* nothing */ +//doc:tags style func emptyDecl(m dsl.Matcher) { m.Match(`var()`).Report(`empty var() block`) m.Match(`const()`).Report(`empty const() block`) m.Match(`type()`).Report(`empty type() block`) } +//doc:summary reports empty errors creation +//doc:before errors.New("") +//doc:after errors.New("can't open the cache file") +//doc:tags style func emptyError(m dsl.Matcher) { m.Match(`fmt.Errorf("")`, `errors.New("")`). Report(`empty errors are hard to debug`)