From eb19bc41c810a58ced6aa25fa2a58348f9b4e884 Mon Sep 17 00:00:00 2001 From: "Iskander (Alex) Sharipov" Date: Sun, 30 Jan 2022 22:27:19 +0300 Subject: [PATCH] ruleguard/typematch: use external matcher state (#374) 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 --- .gitignore | 4 +- Makefile | 3 +- analyzer/analyzer_test.go | 72 +++++++++++++---- analyzer/testdata/src/regression/issue372.go | 6 ++ analyzer/testdata/src/regression/rules.go | 6 ++ ruleguard/filters.go | 8 +- ruleguard/gorule.go | 2 + ruleguard/runner.go | 8 +- ruleguard/typematch/typematch.go | 83 +++++++++++--------- ruleguard/typematch/typematch_test.go | 6 +- 10 files changed, 135 insertions(+), 63 deletions(-) create mode 100644 analyzer/testdata/src/regression/issue372.go diff --git a/.gitignore b/.gitignore index 8f6e5918..aa52fa85 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,5 @@ .idea .vscode -test-ruleguard -test-ruleguard-ir +test-ruleguard.exe +test-ruleguard-ir.exe coverage.txt \ No newline at end of file diff --git a/Makefile b/Makefile index 1cea21a2..551a981b 100644 --- a/Makefile +++ b/Makefile @@ -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" diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 3a1938e8..60089225 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -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].*") @@ -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 @@ -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 { @@ -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() @@ -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() @@ -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") +} diff --git a/analyzer/testdata/src/regression/issue372.go b/analyzer/testdata/src/regression/issue372.go new file mode 100644 index 00000000..520bec65 --- /dev/null +++ b/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` +} diff --git a/analyzer/testdata/src/regression/rules.go b/analyzer/testdata/src/regression/rules.go index 3ee755b0..9ab15ac1 100644 --- a/analyzer/testdata/src/regression/rules.go +++ b/analyzer/testdata/src/regression/rules.go @@ -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") +} diff --git a/ruleguard/filters.go b/ruleguard/filters.go index 607ea27f..cd0b1354 100644 --- a/ruleguard/filters.go +++ b/ruleguard/filters.go @@ -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) @@ -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) diff --git a/ruleguard/gorule.go b/ruleguard/gorule.go index 63907ee2..2574ac90 100644 --- a/ruleguard/gorule.go +++ b/ruleguard/gorule.go @@ -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" ) @@ -60,6 +61,7 @@ type filterParams struct { importer *goImporter gogrepSubState *gogrep.MatcherState + typematchState *typematch.MatcherState match matchData nodePath *nodePath diff --git a/ruleguard/runner.go b/ruleguard/runner.go index 82a40d45..8ad23933 100644 --- a/ruleguard/runner.go +++ b/ruleguard/runner.go @@ -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" ) @@ -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 { diff --git a/ruleguard/typematch/typematch.go b/ruleguard/typematch/typematch.go index 4363e6f0..b7474037 100644 --- a/ruleguard/typematch/typematch.go +++ b/ruleguard/typematch/typematch.go @@ -32,10 +32,32 @@ const ( opNamed ) -type Pattern struct { +type MatcherState struct { typeMatches map[string]types.Type int64Matches map[string]int64 +} + +func NewMatcherState() *MatcherState { + return &MatcherState{ + typeMatches: map[string]types.Type{}, + int64Matches: map[string]int64{}, + } +} + +func (state *MatcherState) reset() { + if len(state.int64Matches) != 0 { + for k := range state.int64Matches { + delete(state.int64Matches, k) + } + } + if len(state.typeMatches) != 0 { + for k := range state.typeMatches { + delete(state.typeMatches, k) + } + } +} +type Pattern struct { root *pattern } @@ -107,9 +129,7 @@ func Parse(ctx *Context, s string) (*Pattern, error) { return nil, fmt.Errorf("can't convert %s type expression", s) } p := &Pattern{ - typeMatches: map[string]types.Type{}, - int64Matches: map[string]int64{}, - root: root, + root: root, } return p, nil } @@ -345,21 +365,12 @@ func parseExpr(ctx *Context, e ast.Expr) *pattern { } // MatchIdentical returns true if the go typ matches pattern p. -func (p *Pattern) MatchIdentical(typ types.Type) bool { - p.reset() - return p.matchIdentical(p.root, typ) -} - -func (p *Pattern) reset() { - if len(p.int64Matches) != 0 { - p.int64Matches = map[string]int64{} - } - if len(p.typeMatches) != 0 { - p.typeMatches = map[string]types.Type{} - } +func (p *Pattern) MatchIdentical(state *MatcherState, typ types.Type) bool { + state.reset() + return p.matchIdentical(state, p.root, typ) } -func (p *Pattern) matchIdenticalFielder(subs []*pattern, f fielder) bool { +func (p *Pattern) matchIdenticalFielder(state *MatcherState, subs []*pattern, f fielder) bool { // TODO: do backtracking. numFields := f.NumFields() @@ -387,7 +398,7 @@ func (p *Pattern) matchIdenticalFielder(subs []*pattern, f fielder) bool { matchAny = false i++ // Lookahead for non-greedy matching. - case i+1 < len(subs) && p.matchIdentical(subs[i+1], f.Field(fieldsMatched).Type()): + case i+1 < len(subs) && p.matchIdentical(state, subs[i+1], f.Field(fieldsMatched).Type()): matchAny = false i += 2 fieldsMatched++ @@ -397,7 +408,7 @@ func (p *Pattern) matchIdenticalFielder(subs []*pattern, f fielder) bool { continue } - if fieldsLeft == 0 || !p.matchIdentical(pat, f.Field(fieldsMatched).Type()) { + if fieldsLeft == 0 || !p.matchIdentical(state, pat, f.Field(fieldsMatched).Type()) { return false } i++ @@ -407,16 +418,16 @@ func (p *Pattern) matchIdenticalFielder(subs []*pattern, f fielder) bool { return numFields == fieldsMatched } -func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { +func (p *Pattern) matchIdentical(state *MatcherState, sub *pattern, typ types.Type) bool { switch sub.op { case opVar: name := sub.value.(string) if name == "_" { return true } - y, ok := p.typeMatches[name] + y, ok := state.typeMatches[name] if !ok { - p.typeMatches[name] = typ + state.typeMatches[name] = typ return true } if y == nil { @@ -432,14 +443,14 @@ func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { if !ok { return false } - return p.matchIdentical(sub.subs[0], typ.Elem()) + return p.matchIdentical(state, sub.subs[0], typ.Elem()) case opSlice: typ, ok := typ.(*types.Slice) if !ok { return false } - return p.matchIdentical(sub.subs[0], typ.Elem()) + return p.matchIdentical(state, sub.subs[0], typ.Elem()) case opArray: typ, ok := typ.(*types.Array) @@ -453,25 +464,25 @@ func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { wantLen = typ.Len() break } - length, ok := p.int64Matches[v] + length, ok := state.int64Matches[v] if ok { wantLen = length } else { - p.int64Matches[v] = typ.Len() + state.int64Matches[v] = typ.Len() wantLen = typ.Len() } case int64: wantLen = v } - return wantLen == typ.Len() && p.matchIdentical(sub.subs[0], typ.Elem()) + return wantLen == typ.Len() && p.matchIdentical(state, sub.subs[0], typ.Elem()) case opMap: typ, ok := typ.(*types.Map) if !ok { return false } - return p.matchIdentical(sub.subs[0], typ.Key()) && - p.matchIdentical(sub.subs[1], typ.Elem()) + return p.matchIdentical(state, sub.subs[0], typ.Key()) && + p.matchIdentical(state, sub.subs[1], typ.Elem()) case opChan: typ, ok := typ.(*types.Chan) @@ -479,7 +490,7 @@ func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { return false } dir := sub.value.(types.ChanDir) - return dir == typ.Dir() && p.matchIdentical(sub.subs[0], typ.Elem()) + return dir == typ.Dir() && p.matchIdentical(state, sub.subs[0], typ.Elem()) case opNamed: typ, ok := typ.(*types.Named) @@ -515,12 +526,12 @@ func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { return false } for i := 0; i < typ.Params().Len(); i++ { - if !p.matchIdentical(params[i], typ.Params().At(i).Type()) { + if !p.matchIdentical(state, params[i], typ.Params().At(i).Type()) { return false } } for i := 0; i < typ.Results().Len(); i++ { - if !p.matchIdentical(results[i], typ.Results().At(i).Type()) { + if !p.matchIdentical(state, results[i], typ.Results().At(i).Type()) { return false } } @@ -535,11 +546,11 @@ func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { params := sub.subs[:numParams] results := sub.subs[numParams:] adapter := tupleFielder{x: typ.Params()} - if !p.matchIdenticalFielder(params, &adapter) { + if !p.matchIdenticalFielder(state, params, &adapter) { return false } adapter.x = typ.Results() - if !p.matchIdenticalFielder(results, &adapter) { + if !p.matchIdenticalFielder(state, results, &adapter) { return false } return true @@ -553,7 +564,7 @@ func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { return false } for i, member := range sub.subs { - if !p.matchIdentical(member, typ.Field(i).Type()) { + if !p.matchIdentical(state, member, typ.Field(i).Type()) { return false } } @@ -564,7 +575,7 @@ func (p *Pattern) matchIdentical(sub *pattern, typ types.Type) bool { if !ok { return false } - if !p.matchIdenticalFielder(sub.subs, typ) { + if !p.matchIdenticalFielder(state, sub.subs, typ) { return false } return true diff --git a/ruleguard/typematch/typematch_test.go b/ruleguard/typematch/typematch_test.go index c616266a..3221c4f3 100644 --- a/ruleguard/typematch/typematch_test.go +++ b/ruleguard/typematch/typematch_test.go @@ -152,13 +152,14 @@ func TestIdentical(t *testing.T) { {`struct{$*_; $x; $*_; $x; $*_}`, structType(intVar, int32Var, stringVar, intVar)}, } + state := NewMatcherState() for _, test := range tests { pat, err := Parse(testContext, test.expr) if err != nil { t.Errorf("parse('%s'): %v", test.expr, err) continue } - if !pat.MatchIdentical(test.typ) { + if !pat.MatchIdentical(state, test.typ) { t.Errorf("identical('%s', %s): expected a match", test.expr, test.typ.String()) } @@ -258,13 +259,14 @@ func TestIdenticalNegative(t *testing.T) { //{`struct{$*x; int; $*x}`, structType(stringVar, intVar, intVar)}, } + state := NewMatcherState() for _, test := range tests { pat, err := Parse(testContext, test.expr) if err != nil { t.Errorf("parse('%s'): %v", test.expr, err) continue } - if pat.MatchIdentical(test.typ) { + if pat.MatchIdentical(state, test.typ) { t.Errorf("identical('%s', %s): unexpected match", test.expr, test.typ.String()) }