Skip to content

Commit

Permalink
ruleguard/typematch: use external matcher state (#374)
Browse files Browse the repository at this point in the history
This is the same trick we did with gogrep matcher.
The caller is supposed to pass the state that is not
shared between different threads.

For the most use cases, ruleguard has worker/runner based
concurrency, so it's easy to pass this state from the
worker that owns that state and doesn't share it with
other workers.

Also added some E2E tests that compile a ruleguard binary
with `-race` and run it using all test rules over the ruleguard
own source code. If any of these rules cause a data race,
this test fails.

To avoid the slower test times, I removed the `-race` from
the basic test as they do not involve any concurrent behavior
anyway, so it was just a waste of time.

Fixes #372
Fixes #368
  • Loading branch information
quasilyte committed Jan 30, 2022
1 parent a4ee291 commit eb19bc4
Show file tree
Hide file tree
Showing 10 changed files with 135 additions and 63 deletions.
4 changes: 2 additions & 2 deletions .gitignore
@@ -1,5 +1,5 @@
.idea
.vscode
test-ruleguard
test-ruleguard-ir
test-ruleguard.exe
test-ruleguard-ir.exe
coverage.txt
3 changes: 2 additions & 1 deletion Makefile
Expand Up @@ -14,7 +14,8 @@ build-release:
./cmd/ruleguard

test:
go test -count 3 -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic -race ./...
go test -timeout=10m -count=1 -race -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic ./...
go test -count=3 -run=TestE2E ./analyzer
cd rules && go test -v .
@echo "everything is OK"

Expand Down
72 changes: 57 additions & 15 deletions analyzer/analyzer_test.go
Expand Up @@ -51,6 +51,15 @@ var tests = []struct {
{name: "quickfix", quickfixes: true},
}

var ruleguardExe string

func TestMain(m *testing.M) {
ruleguardExe = buildRuleguard()

exitCode := m.Run()
os.Exit(exitCode)
}

func TestDirectiveComments(t *testing.T) {
testdata := analysistest.TestData()
badDirectiveRe := regexp.MustCompile("// want `[^\\\\][^Q].*")
Expand Down Expand Up @@ -114,6 +123,31 @@ func TestAnalyzer(t *testing.T) {
}
}

func TestE2E(t *testing.T) {
wd, err := os.Getwd()
if err != nil {
t.Fatal(err)
}
rootPath := filepath.Join(wd, "..")

for i := range tests {
test := tests[i]
t.Run(test.name, func(t *testing.T) {
rulesFilename := fmt.Sprintf("./analyzer/testdata/src/%s/rules.go", test.name)
cmd := exec.Command(ruleguardExe, "-rules", rulesFilename, "./...") // nolint:gosec
cmd.Dir = rootPath
cmd.Env = append([]string{}, os.Environ()...)
cmd.Env = append(cmd.Env, "GORACE=halt_on_error=1 exitcode=39")
out, err := cmd.CombinedOutput()
if exitError, ok := err.(*exec.ExitError); ok {
if exitError.ExitCode() == 39 {
t.Fatalf("found a data race!\n%s", out)
}
}
})
}
}

func TestPrintIR(t *testing.T) {
analyzerTemplate := `
package main
Expand All @@ -137,18 +171,6 @@ var rulesFile = %s
t.Fatal(err)
}

{
args := []string{
"build",
"-o", "test-ruleguard",
filepath.Join(wd, "..", "cmd", "ruleguard"),
}
out, err := exec.Command("go", args...).CombinedOutput()
if err != nil {
t.Fatalf("build go-ruleguard: %v: %s", err, out)
}
}

for i := range tests {
test := tests[i]
if test.flags != nil {
Expand Down Expand Up @@ -194,14 +216,14 @@ var rulesFile = %s
t.Fatal(err)
}

srcRulesCmd := exec.Command(filepath.Join(wd, "test-ruleguard"), "-rules", rulesFilename, "./...") // nolint:gosec
srcRulesCmd := exec.Command(ruleguardExe, "-rules", rulesFilename, "./...") // nolint:gosec
srcRulesCmd.Dir = filepath.Join(wd, "testdata", "src", test.name)
srcOut, _ := srcRulesCmd.CombinedOutput()

{
args := []string{
"build",
"-o", "test-ruleguard-ir",
"-o", "test-ruleguard-ir.exe",
mainFile.Name(),
}
out, err := exec.Command("go", args...).CombinedOutput()
Expand All @@ -210,7 +232,7 @@ var rulesFile = %s
}
}

irRulesCmd := exec.Command(filepath.Join(wd, "test-ruleguard-ir"), "./...") // nolint:gosec
irRulesCmd := exec.Command(filepath.Join(wd, "test-ruleguard-ir.exe"), "./...") // nolint:gosec
irRulesCmd.Dir = filepath.Join(wd, "testdata", "src", test.name)
irOut, _ := irRulesCmd.CombinedOutput()

Expand All @@ -221,3 +243,23 @@ var rulesFile = %s
})
}
}

func buildRuleguard() string {
wd, err := os.Getwd()
if err != nil {
panic(err)
}

args := []string{
"build",
"-o", "test-ruleguard.exe",
"-race",
filepath.Join(wd, "..", "cmd", "ruleguard"),
}
out, err := exec.Command("go", args...).CombinedOutput()
if err != nil {
panic(fmt.Sprintf("build go-ruleguard: %v: %s", err, out))
}

return filepath.Join(wd, "test-ruleguard.exe")
}
6 changes: 6 additions & 0 deletions analyzer/testdata/src/regression/issue372.go
@@ -0,0 +1,6 @@
package regression

func _() {
_ = map[string]int{} // want `\Qcreating a map`
_ = make(map[int][]string) // want `\Qcreating a map`
}
6 changes: 6 additions & 0 deletions analyzer/testdata/src/regression/rules.go
Expand Up @@ -71,3 +71,9 @@ func issue360(m dsl.Matcher) {
Report(`don't use strings.Compare`).
At(m["s1"])
}

func issue372(m dsl.Matcher) {
m.Match("$x{}", "make($x)").
Where(m["x"].Type.Is("map[$k]$v")).
Report("creating a map")
}
8 changes: 4 additions & 4 deletions ruleguard/filters.go
Expand Up @@ -292,11 +292,11 @@ func makeTypeIsFilter(src, varname string, underlying bool, pat *typematch.Patte
return func(params *filterParams) matchFilterResult {
if list, ok := params.subNode(varname).(gogrep.ExprSlice); ok {
return exprListFilterApply(src, list, func(x ast.Expr) bool {
return pat.MatchIdentical(params.typeofNode(x).Underlying())
return pat.MatchIdentical(params.typematchState, params.typeofNode(x).Underlying())
})
}
typ := params.typeofNode(params.subNode(varname)).Underlying()
if pat.MatchIdentical(typ) {
if pat.MatchIdentical(params.typematchState, typ) {
return filterSuccess
}
return filterFailure(src)
Expand All @@ -306,11 +306,11 @@ func makeTypeIsFilter(src, varname string, underlying bool, pat *typematch.Patte
return func(params *filterParams) matchFilterResult {
if list, ok := params.subNode(varname).(gogrep.ExprSlice); ok {
return exprListFilterApply(src, list, func(x ast.Expr) bool {
return pat.MatchIdentical(params.typeofNode(x))
return pat.MatchIdentical(params.typematchState, params.typeofNode(x))
})
}
typ := params.typeofNode(params.subNode(varname))
if pat.MatchIdentical(typ) {
if pat.MatchIdentical(params.typematchState, typ) {
return filterSuccess
}
return filterFailure(src)
Expand Down
2 changes: 2 additions & 0 deletions ruleguard/gorule.go
Expand Up @@ -7,6 +7,7 @@ import (
"regexp"

"github.com/quasilyte/go-ruleguard/ruleguard/quasigo"
"github.com/quasilyte/go-ruleguard/ruleguard/typematch"
"github.com/quasilyte/gogrep"
"github.com/quasilyte/gogrep/nodetag"
)
Expand Down Expand Up @@ -60,6 +61,7 @@ type filterParams struct {

importer *goImporter
gogrepSubState *gogrep.MatcherState
typematchState *typematch.MatcherState

match matchData
nodePath *nodePath
Expand Down
8 changes: 5 additions & 3 deletions ruleguard/runner.go
Expand Up @@ -17,6 +17,7 @@ import (

"github.com/quasilyte/go-ruleguard/ruleguard/goutil"
"github.com/quasilyte/go-ruleguard/ruleguard/profiling"
"github.com/quasilyte/go-ruleguard/ruleguard/typematch"
"github.com/quasilyte/gogrep"
"github.com/quasilyte/gogrep/nodetag"
)
Expand Down Expand Up @@ -80,9 +81,10 @@ func newRulesRunner(ctx *RunContext, buildContext *build.Context, state *engineS
nodePath: newNodePath(),
truncateLen: ctx.TruncateLen,
filterParams: filterParams{
env: state.env.GetEvalEnv(),
importer: importer,
ctx: ctx,
typematchState: typematch.NewMatcherState(),
env: state.env.GetEvalEnv(),
importer: importer,
ctx: ctx,
},
}
if ctx.TruncateLen == 0 {
Expand Down

0 comments on commit eb19bc4

Please sign in to comment.