Skip to content

Commit

Permalink
all: add support for the File filters in Where clause (#83)
Browse files Browse the repository at this point in the history
A first file filter is Imports(pkgpath) predicate.
It checks whether a file that is currently being checked
contains a certain import.

Example usage: suggest filepath.Separator over os.PathSeparator,
but only if "path/filepath" is imported inside this file.

DSL package is updated and re-generated.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
  • Loading branch information
quasilyte authored Oct 8, 2020
1 parent 37e719c commit 7a3df4b
Show file tree
Hide file tree
Showing 10 changed files with 128 additions and 21 deletions.
11 changes: 10 additions & 1 deletion analyzer/testdata/src/filtertest/f1.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package filtertest

import "time"
import (
"os"
"time"
)

type implementsAll struct{}

Expand Down Expand Up @@ -103,3 +106,9 @@ func detectParensFilter() {
var err error
parensFilterTest(err, "type is error") // want `YES`
}

func fileFilters1() {
// No matches as this file doesn't import "path/filepath".
importsTest(os.PathSeparator, "path/filepath")
importsTest(os.PathListSeparator, "path/filepath")
}
14 changes: 14 additions & 0 deletions analyzer/testdata/src/filtertest/imports_filepath.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package filtertest

import (
"os"
"path/filepath"
)

func fileFilters2() {
importsTest(os.PathSeparator, "path/filepath") // want `YES`
importsTest(filepath.Separator, "path/filepath")

importsTest(os.PathListSeparator, "path/filepath") // want `YES`
importsTest(filepath.ListSeparator, "path/filepath")
}
8 changes: 8 additions & 0 deletions analyzer/testdata/src/filtertest/rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,12 @@ func _(m fluent.Matcher) {
m.Match(`textTest($x, "doesn't match [A-Z]")`).Where(!m["x"].Text.Matches(`[A-Z]`)).Report(`YES`)

m.Match(`parensFilterTest($x, "type is error")`).Where((m["x"].Type.Is(`error`))).Report(`YES`)

m.Match(`importsTest(os.PathSeparator, "path/filepath")`).
Where(m.File().Imports("path/filepath")).
Report(`YES`)

m.Match(`importsTest(os.PathListSeparator, "path/filepath")`).
Where(m.File().Imports("path/filepath")).
Report(`YES`)
}
1 change: 1 addition & 0 deletions analyzer/testdata/src/filtertest/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ func typeTest(args ...interface{}) {}
func pureTest(args ...interface{}) {}
func textTest(args ...interface{}) {}
func parensFilterTest(args ...interface{}) {}
func importsTest(args ...interface{}) {}

func random() int {
return 42
Expand Down
9 changes: 9 additions & 0 deletions dsl/fluent/dsl.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ func (m Matcher) At(v Var) Matcher {
return m
}

// File returns the current file context.
func (m Matcher) File() File { return File{} }

// Var is a pattern variable that describes a named submatch.
type Var struct {
// Pure reports whether expr matched by var is side-effect-free.
Expand Down Expand Up @@ -104,3 +107,9 @@ type MatchedText string

// Matches reports whether the text matches the given regexp pattern.
func (MatchedText) Matches(pattern string) bool { return boolResult }

// File represents the current Go source file.
type File struct{}

// Imports reports whether the current file imports the given path.
func (File) Imports(path string) bool { return boolResult }
2 changes: 1 addition & 1 deletion dslgen/dsl_sources.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
package dslgen

var Fluent = []byte("package fluent\n\n// Matcher is a main API group-level entry point.\n// It's used to define and configure the group rules.\n// It also represents a map of all rule-local variables.\ntype Matcher map[string]Var\n\n// Import loads given package path into a rule group imports table.\n//\n// That table is used during the rules compilation.\n//\n// The table has the following effect on the rules:\n//\t* For type expressions, it's used to resolve the\n//\t full package paths of qualified types, like `foo.Bar`.\n//\t If Import(`a/b/foo`) is called, `foo.Bar` will match\n//\t `a/b/foo.Bar` type during the pattern execution.\nfunc (m Matcher) Import(pkgPath string) {}\n\n// Match specifies a set of patterns that match a rule being defined.\n// Pattern matching succeeds if at least 1 pattern matches.\n//\n// If none of the given patterns matched, rule execution stops.\nfunc (m Matcher) Match(pattern string, alternatives ...string) Matcher {\n\treturn m\n}\n\n// Where applies additional constraint to a match.\n// If a given cond is not satisfied, a match is rejected and\n// rule execution stops.\nfunc (m Matcher) Where(cond bool) Matcher {\n\treturn m\n}\n\n// Report prints a message if associated rule match is successful.\n//\n// A message is a string that can contain interpolated expressions.\n// For every matched variable it's possible to interpolate\n// their printed representation into the message text with $<name>.\n// An entire match can be addressed with $$.\nfunc (m Matcher) Report(message string) Matcher {\n\treturn m\n}\n\n// Suggest assigns a quickfix suggestion for the matched code.\nfunc (m Matcher) Suggest(suggestion string) Matcher {\n\treturn m\n}\n\n// At binds the reported node to a named submatch.\n// If no explicit location is given, the outermost node ($$) is used.\nfunc (m Matcher) At(v Var) Matcher {\n\treturn m\n}\n\n// Var is a pattern variable that describes a named submatch.\ntype Var struct {\n\t// Pure reports whether expr matched by var is side-effect-free.\n\tPure bool\n\n\t// Const reports whether expr matched by var is a constant value.\n\tConst bool\n\n\t// Addressable reports whether the corresponding expression is addressable.\n\t// See https://golang.org/ref/spec#Address_operators.\n\tAddressable bool\n\n\t// Type is a type of a matched expr.\n\tType ExprType\n\n\t// Test is a captured node text as in the source code.\n\tText MatchedText\n}\n\n// ExprType describes a type of a matcher expr.\ntype ExprType struct {\n\t// Size represents expression type size in bytes.\n\tSize int\n}\n\n// AssignableTo reports whether a type is assign-compatible with a given type.\n// See https://golang.org/pkg/go/types/#AssignableTo.\nfunc (ExprType) AssignableTo(typ string) bool { return boolResult }\n\n// ConvertibleTo reports whether a type is conversible to a given type.\n// See https://golang.org/pkg/go/types/#ConvertibleTo.\nfunc (ExprType) ConvertibleTo(typ string) bool { return boolResult }\n\n// Implements reports whether a type implements a given interface.\n// See https://golang.org/pkg/go/types/#Implements.\nfunc (ExprType) Implements(typ string) bool { return boolResult }\n\n// Is reports whether a type is identical to a given type.\nfunc (ExprType) Is(typ string) bool { return boolResult }\n\n// MatchedText represents a source text associated with a matched node.\ntype MatchedText string\n\n// Matches reports whether the text matches the given regexp pattern.\nfunc (MatchedText) Matches(pattern string) bool { return boolResult }\n\n\n\nvar boolResult bool\n\n")
var Fluent = []byte("package fluent\n\n// Matcher is a main API group-level entry point.\n// It's used to define and configure the group rules.\n// It also represents a map of all rule-local variables.\ntype Matcher map[string]Var\n\n// Import loads given package path into a rule group imports table.\n//\n// That table is used during the rules compilation.\n//\n// The table has the following effect on the rules:\n//\t* For type expressions, it's used to resolve the\n//\t full package paths of qualified types, like `foo.Bar`.\n//\t If Import(`a/b/foo`) is called, `foo.Bar` will match\n//\t `a/b/foo.Bar` type during the pattern execution.\nfunc (m Matcher) Import(pkgPath string) {}\n\n// Match specifies a set of patterns that match a rule being defined.\n// Pattern matching succeeds if at least 1 pattern matches.\n//\n// If none of the given patterns matched, rule execution stops.\nfunc (m Matcher) Match(pattern string, alternatives ...string) Matcher {\n\treturn m\n}\n\n// Where applies additional constraint to a match.\n// If a given cond is not satisfied, a match is rejected and\n// rule execution stops.\nfunc (m Matcher) Where(cond bool) Matcher {\n\treturn m\n}\n\n// Report prints a message if associated rule match is successful.\n//\n// A message is a string that can contain interpolated expressions.\n// For every matched variable it's possible to interpolate\n// their printed representation into the message text with $<name>.\n// An entire match can be addressed with $$.\nfunc (m Matcher) Report(message string) Matcher {\n\treturn m\n}\n\n// Suggest assigns a quickfix suggestion for the matched code.\nfunc (m Matcher) Suggest(suggestion string) Matcher {\n\treturn m\n}\n\n// At binds the reported node to a named submatch.\n// If no explicit location is given, the outermost node ($$) is used.\nfunc (m Matcher) At(v Var) Matcher {\n\treturn m\n}\n\n// File returns the current file context.\nfunc (m Matcher) File() File { return File{} }\n\n// Var is a pattern variable that describes a named submatch.\ntype Var struct {\n\t// Pure reports whether expr matched by var is side-effect-free.\n\tPure bool\n\n\t// Const reports whether expr matched by var is a constant value.\n\tConst bool\n\n\t// Addressable reports whether the corresponding expression is addressable.\n\t// See https://golang.org/ref/spec#Address_operators.\n\tAddressable bool\n\n\t// Type is a type of a matched expr.\n\tType ExprType\n\n\t// Text is a captured node text as in the source code.\n\tText MatchedText\n}\n\n// ExprType describes a type of a matcher expr.\ntype ExprType struct {\n\t// Size represents expression type size in bytes.\n\tSize int\n}\n\n// AssignableTo reports whether a type is assign-compatible with a given type.\n// See https://golang.org/pkg/go/types/#AssignableTo.\nfunc (ExprType) AssignableTo(typ string) bool { return boolResult }\n\n// ConvertibleTo reports whether a type is conversible to a given type.\n// See https://golang.org/pkg/go/types/#ConvertibleTo.\nfunc (ExprType) ConvertibleTo(typ string) bool { return boolResult }\n\n// Implements reports whether a type implements a given interface.\n// See https://golang.org/pkg/go/types/#Implements.\nfunc (ExprType) Implements(typ string) bool { return boolResult }\n\n// Is reports whether a type is identical to a given type.\nfunc (ExprType) Is(typ string) bool { return boolResult }\n\n// MatchedText represents a source text associated with a matched node.\ntype MatchedText string\n\n// Matches reports whether the text matches the given regexp pattern.\nfunc (MatchedText) Matches(pattern string) bool { return boolResult }\n\n// File represents the current Go source file.\ntype File struct{}\n\n// Imports reports whether the current file imports the given path.\nfunc (File) Imports(path string) bool { return boolResult }\n\n\n\nvar boolResult bool\n\n")
7 changes: 6 additions & 1 deletion ruleguard/gorule.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ type goRule struct {
msg string
location string
suggestion string
filters map[string]submatchFilter
filter matchFilter
}

type matchFilter struct {
fileImports []string
sub map[string]submatchFilter
}

type submatchFilter struct {
Expand Down
64 changes: 47 additions & 17 deletions ruleguard/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -319,10 +319,11 @@ func (p *rulesParser) parseRule(matcher string, call *ast.CallExpr) error {
}

dst := p.res.universal
filters := map[string]submatchFilter{}
proto := goRule{
filename: p.filename,
filters: filters,
filter: matchFilter{
sub: map[string]submatchFilter{},
},
}
var alternatives []string

Expand All @@ -338,7 +339,7 @@ func (p *rulesParser) parseRule(matcher string, call *ast.CallExpr) error {
}

if whereArgs != nil {
if err := p.walkFilter(filters, (*whereArgs)[0], false); err != nil {
if err := p.walkFilter(&proto.filter, (*whereArgs)[0], false); err != nil {
return err
}
}
Expand Down Expand Up @@ -395,7 +396,7 @@ func (p *rulesParser) parseRule(matcher string, call *ast.CallExpr) error {
return nil
}

func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, negate bool) error {
func (p *rulesParser) walkFilter(dst *matchFilter, e ast.Expr, negate bool) error {
typeAnd := func(x, y func(typeQuery) bool) func(typeQuery) bool {
if x == nil {
return y
Expand Down Expand Up @@ -430,32 +431,44 @@ func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, nega
y := p.types.Types[e.Y].Value
expectedResult := !negate
if operand.path == "Type.Size" && y != nil {
filter := dst[operand.varName]
filter := dst.sub[operand.varName]
filter.typePred = typeAnd(filter.typePred, func(q typeQuery) bool {
x := constant.MakeInt64(q.ctx.Sizes.Sizeof(q.x))
return expectedResult == constant.Compare(x, e.Op, y)
})
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
return nil
}
if operand.path == "Text" && y != nil {
filter := dst[operand.varName]
filter := dst.sub[operand.varName]
filter.textPred = textAnd(filter.textPred, func(s string) bool {
x := constant.MakeString(s)
return expectedResult == constant.Compare(x, e.Op, y)
})
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
return nil
}
}
case *ast.ParenExpr:
return p.walkFilter(dst, e.X, negate)
}

// File-related filters.
fileOperand := p.toFileFilterOperand(e)
switch fileOperand.path {
case "Imports":
pkgPath, ok := p.toStringValue(fileOperand.args[0])
if !ok {
return p.errorf(fileOperand.args[0], "expected a string literal argument")
}
dst.fileImports = append(dst.fileImports, pkgPath)
return nil
}

// TODO(quasilyte): refactor and extend.
operand := p.toFilterOperand(e)
args := operand.args
filter := dst[operand.varName]
filter := dst.sub[operand.varName]
switch operand.path {
default:
return p.errorf(e, "%s is not a valid filter expression", sprintNode(p.fset, e))
Expand All @@ -465,21 +478,21 @@ func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, nega
} else {
filter.pure = bool3true
}
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
case "Const":
if negate {
filter.constant = bool3false
} else {
filter.constant = bool3true
}
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
case "Addressable":
if negate {
filter.addressable = bool3false
} else {
filter.addressable = bool3true
}
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
case "Text.Matches":
patternString, ok := p.toStringValue(args[0])
if !ok {
Expand All @@ -493,7 +506,7 @@ func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, nega
filter.textPred = textAnd(filter.textPred, func(s string) bool {
return wantMatched == re.MatchString(s)
})
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
case "Type.Is":
typeString, ok := p.toStringValue(args[0])
if !ok {
Expand All @@ -508,7 +521,7 @@ func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, nega
filter.typePred = typeAnd(filter.typePred, func(q typeQuery) bool {
return wantIdentical == pat.MatchIdentical(q.x)
})
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
case "Type.ConvertibleTo":
typeString, ok := p.toStringValue(args[0])
if !ok {
Expand All @@ -525,7 +538,7 @@ func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, nega
filter.typePred = typeAnd(filter.typePred, func(q typeQuery) bool {
return wantConvertible == types.ConvertibleTo(q.x, y)
})
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
case "Type.AssignableTo":
typeString, ok := p.toStringValue(args[0])
if !ok {
Expand All @@ -542,7 +555,7 @@ func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, nega
filter.typePred = typeAnd(filter.typePred, func(q typeQuery) bool {
return wantAssignable == types.AssignableTo(q.x, y)
})
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
case "Type.Implements":
typeString, ok := p.toStringValue(args[0])
if !ok {
Expand Down Expand Up @@ -583,7 +596,7 @@ func (p *rulesParser) walkFilter(dst map[string]submatchFilter, e ast.Expr, nega
filter.typePred = typeAnd(filter.typePred, func(q typeQuery) bool {
return wantImplemented == types.Implements(q.x, iface)
})
dst[operand.varName] = filter
dst.sub[operand.varName] = filter
}

return nil
Expand Down Expand Up @@ -621,6 +634,23 @@ func (p *rulesParser) toStringValue(x ast.Node) (string, bool) {
return "", false
}

func (p *rulesParser) toFileFilterOperand(e ast.Expr) filterOperand {
var o filterOperand

call, ok := e.(*ast.CallExpr)
if !ok {
return o
}
selector, ok := call.Fun.(*ast.SelectorExpr)
if !ok {
return o
}

o.args = call.Args
o.path = selector.Sel.Name
return o
}

func (p *rulesParser) toFilterOperand(e ast.Expr) filterOperand {
var o filterOperand

Expand Down
23 changes: 22 additions & 1 deletion ruleguard/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"go/ast"
"go/printer"
"io/ioutil"
"strconv"
"strings"

"github.com/quasilyte/go-ruleguard/internal/mvdan.cc/gogrep"
Expand All @@ -15,6 +16,7 @@ type rulesRunner struct {
rules *GoRuleSet

filename string
imports map[string]struct{}
src []byte
}

Expand Down Expand Up @@ -61,6 +63,7 @@ func (rr *rulesRunner) run(f *ast.File) error {
// TODO(quasilyte): run local rules as well.

rr.filename = rr.ctx.Fset.Position(f.Pos()).Filename
rr.collectImports(f)

for _, rule := range rr.rules.universal.uncategorized {
rule.pat.Match(f, func(m gogrep.MatchData) {
Expand Down Expand Up @@ -88,6 +91,12 @@ func (rr *rulesRunner) run(f *ast.File) error {
}

func (rr *rulesRunner) handleMatch(rule goRule, m gogrep.MatchData) bool {
for _, neededImport := range rule.filter.fileImports {
if _, ok := rr.imports[neededImport]; !ok {
return false
}
}

for name, node := range m.Values {
var expr ast.Expr
switch node := node.(type) {
Expand All @@ -98,7 +107,8 @@ func (rr *rulesRunner) handleMatch(rule goRule, m gogrep.MatchData) bool {
default:
continue
}
filter, ok := rule.filters[name]

filter, ok := rule.filter.sub[name]
if !ok {
continue
}
Expand Down Expand Up @@ -170,6 +180,17 @@ func (rr *rulesRunner) handleMatch(rule goRule, m gogrep.MatchData) bool {
return true
}

func (rr *rulesRunner) collectImports(f *ast.File) {
rr.imports = make(map[string]struct{}, len(f.Imports))
for _, spec := range f.Imports {
s, err := strconv.Unquote(spec.Path.Value)
if err != nil {
continue
}
rr.imports[s] = struct{}{}
}
}

func (rr *rulesRunner) renderMessage(msg string, n ast.Node, nodes map[string]ast.Node, truncate bool) string {
var buf strings.Builder
if strings.Contains(msg, "$$") {
Expand Down
10 changes: 10 additions & 0 deletions rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,16 @@ func _(m fluent.Matcher) {
Suggest(`$a+$b`)
}

func osFilepath(m fluent.Matcher) {
m.Match(`os.PathSeparator`).
Where(m.File().Imports("path/filepath")).
Suggest(`filepath.Separator`)

m.Match(`os.PathListSeparator`).
Where(m.File().Imports("path/filepath")).
Suggest(`filepath.ListSeparator`)
}

// See https://twitter.com/dgryski/status/1281348103505768449
func useMathBits(m fluent.Matcher) {
// RotateLeft
Expand Down

0 comments on commit 7a3df4b

Please sign in to comment.