Skip to content

Commit

Permalink
ruleguard: add support for structured doc comments (#227)
Browse files Browse the repository at this point in the history
A matcher function can now use 4 special pragmas:

	* `doc:summary` for a short description
	* `doc:before` for an example of a "bad code"
	* `doc:after` for a fixed code example
	* `doc:tags` for a list of custom tags

It's planned to make it possible for the integrating app
to access that information in a convenient way.

This PR is moslty about enabling on a parsing level.

These 4 fields (summary, before, after, tags) are selected
as something that is close to the `go-critic` checker documentation model.
  • Loading branch information
quasilyte authored Apr 28, 2021
1 parent 7e6eba3 commit 545e0d2
Show file tree
Hide file tree
Showing 9 changed files with 129 additions and 21 deletions.
22 changes: 22 additions & 0 deletions _docs/dsl.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,28 @@ A `Report()` argument string can use `$<varname>` 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:
Expand Down
2 changes: 1 addition & 1 deletion analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
2 changes: 1 addition & 1 deletion ruleguard/gorule.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type goCommentRule struct {
}

type goRule struct {
group string
group *GoRuleGroup
filename string
line int
pat *gogrep.Pattern
Expand Down
62 changes: 55 additions & 7 deletions ruleguard/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"path"
"regexp"
"strconv"
"strings"

"github.com/quasilyte/go-ruleguard/internal/gogrep"
"github.com/quasilyte/go-ruleguard/nodetag"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
}
Expand All @@ -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
}

Expand Down
30 changes: 25 additions & 5 deletions ruleguard/ruleguard.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
12 changes: 5 additions & 7 deletions ruleguard/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions rules/diag.go
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand Down
4 changes: 4 additions & 0 deletions rules/refactor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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`)).
Expand Down
12 changes: 12 additions & 0 deletions rules/style.go
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand Down

0 comments on commit 545e0d2

Please sign in to comment.