Skip to content

Commit

Permalink
ruleguard: add support for local functions
Browse files Browse the repository at this point in the history
This feature is useful for rule filters readability improvements.

Instead of copying a complex `Where()` expression several times,
one can now use a local function literal to define that filter
operation and use it inside `Where()` expressions.

Here is an example:

```go
func preferFprint(m dsl.Matcher) {
	isFmtPackage := func(v dsl.Var) bool {
		return v.Text == "fmt" && v.Object.Is(`PkgName`)
	}

	m.Match(`$w.Write([]byte($fmt.Sprint($*args)))`).
		Where(m["w"].Type.Implements("io.Writer") && isFmtPackage(m["fmt"])).
		Suggest("fmt.Fprint($w, $args)").
		Report(`fmt.Fprint($w, $args) should be preferred to the $$`)

	m.Match(`$w.Write([]byte($fmt.Sprintf($*args)))`).
		Where(m["w"].Type.Implements("io.Writer") && isFmtPackage(m["fmt"])).
		Suggest("fmt.Fprintf($w, $args)").
		Report(`fmt.Fprintf($w, $args) should be preferred to the $$`)

	// ...etc
}
```

Note that we used `isFmtPackage` in more than 1 rule.

Functions can accept almost arbitrary params, but there are some
restrictions on what kinds of arguments they can receive right now.

These arguments work:

* Matcher var expressions like `m["varname"]`
* Basic literals like `"foo"`, `104`, `5.2`
* Constants
  • Loading branch information
quasilyte committed Nov 7, 2021
1 parent 34f5283 commit e6ec668
Show file tree
Hide file tree
Showing 11 changed files with 352 additions and 14 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ test-release:
@echo "everything is OK"

lint:
curl -sSfL https://github.com/raw/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH_DIR)/bin v1.30.0
curl -sSfL https://github.com/raw/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH_DIR)/bin v1.43.0
$(GOPATH_DIR)/bin/golangci-lint run ./...
go build -o go-ruleguard ./cmd/ruleguard
./go-ruleguard -debug-imports -rules rules.go ./...
Expand Down
1 change: 1 addition & 0 deletions analyzer/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ var tests = []struct {
{name: "comments"},
{name: "stdlib"},
{name: "uber"},
{name: "localfunc"},
{name: "goversion", flags: map[string]string{"go": "1.16"}},
}

Expand Down
13 changes: 7 additions & 6 deletions analyzer/testdata/src/gocritic/rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,21 +170,22 @@ func appendAssign(m dsl.Matcher) {
//doc:before w.Write([]byte(fmt.Sprintf("%x", 10)))
//doc:after fmt.Fprintf(w, "%x", 10)
func preferFprint(m dsl.Matcher) {
isFmtPackage := func(v dsl.Var) bool {
return v.Text == "fmt" && v.Object.Is(`PkgName`)
}

m.Match(`$w.Write([]byte($fmt.Sprint($*args)))`).
Where(m["w"].Type.Implements("io.Writer") &&
m["fmt"].Text == "fmt" && m["fmt"].Object.Is(`PkgName`)).
Where(m["w"].Type.Implements("io.Writer") && isFmtPackage(m["fmt"])).
Suggest("fmt.Fprint($w, $args)").
Report(`fmt.Fprint($w, $args) should be preferred to the $$`)

m.Match(`$w.Write([]byte($fmt.Sprintf($*args)))`).
Where(m["w"].Type.Implements("io.Writer") &&
m["fmt"].Text == "fmt" && m["fmt"].Object.Is(`PkgName`)).
Where(m["w"].Type.Implements("io.Writer") && isFmtPackage(m["fmt"])).
Suggest("fmt.Fprintf($w, $args)").
Report(`fmt.Fprintf($w, $args) should be preferred to the $$`)

m.Match(`$w.Write([]byte($fmt.Sprintln($*args)))`).
Where(m["w"].Type.Implements("io.Writer") &&
m["fmt"].Text == "fmt" && m["fmt"].Object.Is(`PkgName`)).
Where(m["w"].Type.Implements("io.Writer") && isFmtPackage(m["fmt"])).
Suggest("fmt.Fprintln($w, $args)").
Report(`fmt.Fprintln($w, $args) should be preferred to the $$`)
}
Expand Down
66 changes: 66 additions & 0 deletions analyzer/testdata/src/localfunc/rules.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// +build ignore

package gorules

import "github.com/quasilyte/go-ruleguard/dsl"

func testRules(m dsl.Matcher) {
bothConst := func(x, y dsl.Var) bool {
return x.Const && y.Const
}
m.Match(`test("both const", $x, $y)`).
Where(bothConst(m["x"], m["y"])).
Report(`true`)

intValue := func(x dsl.Var, val int) bool {
return x.Value.Int() == val
}
m.Match(`test("== 10", $x)`).
Where(intValue(m["x"], 10)).
Report(`true`)

isZero := func(x dsl.Var) bool { return x.Value.Int() == 0 }
m.Match(`test("== 0", $x)`).
Where(isZero(m["x"])).
Report(`true`)

// Testing closure-captured m variable.
fmtIsImported := func() bool {
return m.File().Imports(`fmt`)
}
m.Match(`test("fmt is imported")`).
Where(fmtIsImported()).
Report(`true`)

// Testing explicitly passed matcher.
ioutilIsImported := func(m2 dsl.Matcher) bool {
return m2.File().Imports(`io/ioutil`)
}
m.Match(`test("ioutil is imported")`).
Where(ioutilIsImported(m)).
Report(`true`)

// Test precedence after the macro expansion.
isSimpleExpr := func(v dsl.Var) bool {
return v.Const || v.Node.Is(`Ident`)
}
m.Match(`test("check precedence", $x, $y)`).
Where(isSimpleExpr(m["x"]) && m["y"].Text == "err").
Report(`true`)

// Test variables referenced through captured m.
isStringLit := func() bool {
return m["x"].Node.Is(`BasicLit`) && m["x"].Type.Is(`string`)
}
m.Match(`test("is string", $x)`).
Where(isStringLit()).
Report(`true`)

// Test predicate applied to different matcher vars.
isPureCall := func(v dsl.Var) bool {
return v.Node.Is(`CallExpr`) && v.Pure
}
m.Match(`test("is pure call", $x, $y)`).
Where(isPureCall(m["x"]) && isPureCall(m["y"])).
Report(`true`)
}
67 changes: 67 additions & 0 deletions analyzer/testdata/src/localfunc/target.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package localfunc

import (
"fmt"
"io/ioutil"
)

func test(args ...interface{}) {}

func f() interface{} { return nil }

func _() {
fmt.Println("ok")
_ = ioutil.Discard

var i int
var err error

test("both const", 1, 2) // want `true`
test("both const", 1, 2+2) // want `true`
test("both const", i, 2)
test("both const", 1, i)
test("both const", i, i)

test("== 10", 10) // want `true`
test("== 10", 9+1) // want `true`
test("== 10", 11)
test("== 10", i)

test("== 0", 0) // want `true`
test("== 0", 1-1) // want `true`
test("== 0", 11)
test("== 0", i)

test("fmt is imported") // want `true`

test("ioutil is imported") // want `true`

test("check precedence", 1, err) // want `true`
test("check precedence", 1+2, err) // want `true`
test("check precedence", i, err) // want `true`
test("check precedence", err, err) // want `true`
test("check precedence", f(), err)
test("check precedence", 1)
test("check precedence", 1+2)
test("check precedence", i)
test("check precedence", err)
test("check precedence", f())
test("check precedence", 1, nil)
test("check precedence", 1+2, nil)
test("check precedence", i, nil)
test("check precedence", err, nil)
test("check precedence", f(), nil)

test("is string", "yes") // want `true`
test("is string", `yes`) // want `true`
test("is string", 1)
test("is string", i)

test("is pure call", int(0), int(1)) // want `true`
test("is pure call", string("f"), int(1)) // want `true`
test("is pure call", f(), f())
test("is pure call", int(0), 1)
test("is pure call", 0, int(1))
test("is pure call", f(), int(1))
test("is pure call", 1, 1)
}
7 changes: 7 additions & 0 deletions analyzer/testdata/src/localfunc/target2.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package localfunc

func _() {
test("fmt is imported")

test("ioutil is imported")
}
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module github.com/quasilyte/go-ruleguard
go 1.15

require (
github.com/go-toolsmith/astcopy v1.0.0
github.com/go-toolsmith/astequal v1.0.1
github.com/google/go-cmp v0.5.2
github.com/quasilyte/go-ruleguard/dsl v0.3.10
Expand Down
3 changes: 3 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
github.com/go-toolsmith/astcopy v1.0.0 h1:OMgl1b1MEpjFQ1m5ztEO06rz5CUd3oBv9RF7+DyvdG8=
github.com/go-toolsmith/astcopy v1.0.0/go.mod h1:vrgyG+5Bxrnz4MZWPF+pI4R8h3qKRjjyvV/DSez4WVQ=
github.com/go-toolsmith/astequal v1.0.0/go.mod h1:H+xSiq0+LtiDC11+h1G32h7Of5O3CYFJ99GVbS5lDKY=
github.com/go-toolsmith/astequal v1.0.1 h1:JbSszi42Jiqu36Gnf363HWS9MTEAz67vTQLponh3Moc=
github.com/go-toolsmith/astequal v1.0.1/go.mod h1:4oGA3EZXTVItV/ipGiOx7NWkY5veFfcsOJVS2YxltLw=
github.com/go-toolsmith/strparse v1.0.0 h1:Vcw78DnpCAKlM20kSbAyO4mPfJn/lyYA4BJUDxe2Jb4=
Expand Down
14 changes: 14 additions & 0 deletions ruleguard/debug_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,20 @@ func TestDebug(t *testing.T) {
` $x []string: []string{"x"}`,
},
},

`isConst := func(v dsl.Var) bool { return v.Const }; m.Match("_ = $x").Where(isConst(m["x"]) && !m["x"].Type.Is("string"))`: {
`_ = 10`: nil,

`_ = "str"`: {
`input.go:4: [rules.go:5] rejected by !m["x"].Type.Is("string")`,
` $x string: "str"`,
},

`_ = f()`: {
`input.go:4: [rules.go:5] rejected by isConst(m["x"])`,
` $x interface{}: f()`,
},
},
}

loadRulesFromExpr := func(e *Engine, s string) {
Expand Down
Loading

0 comments on commit e6ec668

Please sign in to comment.