From 0cc118a7ae047f50881d0c7713482101e0fa0cbf Mon Sep 17 00:00:00 2001 From: Iskander Sharipov Date: Thu, 14 Jan 2021 01:16:21 +0300 Subject: [PATCH] ruleguard: allow FQN in Implements() filter --- _test/regress/issue103/expected2.txt | 14 +++++ _test/regress/issue103/rules2.go | 24 ++++++++ _test/regress/issue103/test.bash | 3 + dsl/dsl.go | 14 ++++- dsl/filter.go | 9 +-- ruleguard/parser.go | 87 +++++++++++++++------------- ruleguard/ruleguard_error_test.go | 9 ++- 7 files changed, 110 insertions(+), 50 deletions(-) create mode 100644 _test/regress/issue103/expected2.txt create mode 100644 _test/regress/issue103/rules2.go diff --git a/_test/regress/issue103/expected2.txt b/_test/regress/issue103/expected2.txt new file mode 100644 index 00000000..22f59e6c --- /dev/null +++ b/_test/regress/issue103/expected2.txt @@ -0,0 +1,14 @@ +/root/target.go:20:2: logrus: logger.WithError(err).Error(...) (rules2.go:10) +20 logger.Error(err) +/root/target.go:22:2: logrus: logger.WithError(fmt.Errorf("wrap: %w", err)).Error(...) (rules2.go:10) +22 logger.Error(fmt.Errorf("wrap: %w", err)) +/root/target.go:25:2: loggerType: testLoggerType called with *logrus.Logger (rules2.go:21) +25 testLoggerType(logger) +/root/target.go:29:2: logrus: loggerEntry.WithError(err).Error(...) (rules2.go:10) +29 loggerEntry.Error(err) +/root/target.go:31:2: logrus: loggerEntry.WithError(fmt.Errorf("wrap: %w", err)).Error(...) (rules2.go:10) +31 loggerEntry.Error(fmt.Errorf("wrap: %w", err)) +/root/target.go:38:2: logrus: loggerIface.WithError(err).Error(...) (rules2.go:10) +38 loggerIface.Error(err) +/root/target.go:40:2: logrus: loggerIface.WithError(fmt.Errorf("wrap: %w", err)).Error(...) (rules2.go:10) +40 loggerIface.Error(fmt.Errorf("wrap: %w", err)) diff --git a/_test/regress/issue103/rules2.go b/_test/regress/issue103/rules2.go new file mode 100644 index 00000000..4e78fe93 --- /dev/null +++ b/_test/regress/issue103/rules2.go @@ -0,0 +1,24 @@ +// +build ignore + +package gorules + +import ( + "github.com/quasilyte/go-ruleguard/dsl" +) + +func logrus(m dsl.Matcher) { + m.Match( + `$log.Error($*_, $err, $*_)`, + `$log.Errorf($*_, $err, $*_)`, + ). + Where(m["err"].Type.Is(`error`) && m["log"].Type.Implements(`github.com/sirupsen/logrus.FieldLogger`)). + Report(`$log.WithError($err).Error(...)`) +} + +func loggerType(m dsl.Matcher) { + m.Import("github.com/sirupsen/logrus") + + m.Match(`testLoggerType($x)`). + Where(m["x"].Type.Is(`*logrus.Logger`)). + Report("testLoggerType called with *logrus.Logger") +} diff --git a/_test/regress/issue103/test.bash b/_test/regress/issue103/test.bash index c33a3606..8302a79f 100755 --- a/_test/regress/issue103/test.bash +++ b/_test/regress/issue103/test.bash @@ -15,4 +15,7 @@ go get -v -u github.com/sirupsen/logrus ./ruleguard -c 0 -rules /root/rules.go /root/target.go &> actual.txt || true diff -u actual.txt /root/expected.txt +./ruleguard -c 0 -rules /root/rules2.go /root/target.go &> actual.txt || true +diff -u actual.txt /root/expected2.txt + echo SUCCESS diff --git a/dsl/dsl.go b/dsl/dsl.go index 7b301e92..99271f95 100644 --- a/dsl/dsl.go +++ b/dsl/dsl.go @@ -131,7 +131,7 @@ func (ExprType) ConvertibleTo(typ string) bool { return boolResult } // Implements reports whether a type implements a given interface. // See https://golang.org/pkg/go/types/#Implements. -func (ExprType) Implements(typ string) bool { return boolResult } +func (ExprType) Implements(typ typeName) bool { return boolResult } // Is reports whether a type is identical to a given type. func (ExprType) Is(typ string) bool { return boolResult } @@ -160,3 +160,15 @@ type File struct { // Imports reports whether the current file imports the given path. func (File) Imports(path string) bool { return boolResult } + +// typeName is a helper type used to document function params better. +// +// A type name can be: +// - builtin type name: `error`, `string`, etc. +// - qualified name from a standard library: `io.Reader`, etc. +// - fully-qualified type name, like `github.com/username/pkgname.TypeName` +// +// typeName is also affected by a local import table, which can override +// how qualified names are interpreted. +// See `Matcher.Import` for more info. +type typeName = string \ No newline at end of file diff --git a/dsl/filter.go b/dsl/filter.go index e201408c..cef88009 100644 --- a/dsl/filter.go +++ b/dsl/filter.go @@ -16,14 +16,9 @@ type VarFilterContext struct { func (*VarFilterContext) SizeOf(x types.Type) int { return 0 } // GetType finds a type value by a given name. -// -// A name can be: -// - builtin type name, like `error` or `string` -// - fully-qualified type name, like `github.com/username/pkgname.TypeName` -// // If a type can't be found (or a name is malformed), this function panics. -func (*VarFilterContext) GetType(name string) types.Type { return nil } +func (*VarFilterContext) GetType(name typeName) types.Type { return nil } // GetInterface finds a type value that represents an interface by a given name. // Works like `types.AsInterface(ctx.GetType(name))`. -func (*VarFilterContext) GetInterface(name string) *types.Interface { return nil } +func (*VarFilterContext) GetInterface(name typeName) *types.Interface { return nil } diff --git a/ruleguard/parser.go b/ruleguard/parser.go index e0d2c318..d6e6aaff 100644 --- a/ruleguard/parser.go +++ b/ruleguard/parser.go @@ -603,6 +603,7 @@ func (p *rulesParser) parseFilterExpr(e ast.Expr) matchFilter { result.fn = makeCustomVarFilter(result.src, operand.varName, userFn) case "Type.Is", "Type.Underlying.Is": + // TODO(quasilyte): add FQN support? typeString, ok := p.toStringValue(args[0]) if !ok { panic(p.errorf(args[0], "expected a string literal argument")) @@ -624,45 +625,7 @@ func (p *rulesParser) parseFilterExpr(e ast.Expr) matchFilter { result.fn = makeTypeAssignableToFilter(result.src, operand.varName, dstType) case "Type.Implements": - typeString, ok := p.toStringValue(args[0]) - if !ok { - panic(p.errorf(args[0], "expected a string literal argument")) - } - n, err := parser.ParseExpr(typeString) - if err != nil { - panic(p.errorf(args[0], "parse type expr: %v", err)) - } - var iface *types.Interface - switch n := n.(type) { - case *ast.Ident: - if n.Name != `error` { - panic(p.errorf(n, "only `error` unqualified type is recognized")) - } - iface = types.Universe.Lookup("error").Type().Underlying().(*types.Interface) - case *ast.SelectorExpr: - pkgName, ok := n.X.(*ast.Ident) - if !ok { - panic(p.errorf(n.X, "invalid package name")) - } - pkgPath, ok := p.itab.Lookup(pkgName.Name) - if !ok { - panic(p.errorf(n.X, "package %s is not imported", pkgName.Name)) - } - pkg, err := p.importer.Import(pkgPath) - if err != nil { - panic(p.errorf(n, "can't load %s: %v", pkgPath, err)) - } - obj := pkg.Scope().Lookup(n.Sel.Name) - if obj == nil { - panic(p.errorf(n, "%s is not found in %s", n.Sel.Name, pkgPath)) - } - iface, ok = obj.Type().Underlying().(*types.Interface) - if !ok { - panic(p.errorf(n, "%s is not an interface type", n.Sel.Name)) - } - default: - panic(p.errorf(args[0], "only qualified names (and `error`) are supported")) - } + iface := p.toInterfaceValue(args[0]) result.fn = makeTypeImplementsFilter(result.src, operand.varName, iface) case "Text.Matches": @@ -687,6 +650,52 @@ func (p *rulesParser) parseFilterExpr(e ast.Expr) matchFilter { return result } +func (p *rulesParser) toInterfaceValue(x ast.Node) *types.Interface { + typeString, ok := p.toStringValue(x) + if !ok { + panic(p.errorf(x, "expected a string literal argument")) + } + + typ, err := p.state.FindType(p.importer, p.pkg, typeString) + if err == nil { + iface, ok := typ.Underlying().(*types.Interface) + if !ok { + panic(p.errorf(x, "%s is not an interface type", typeString)) + } + return iface + } + + n, err := parser.ParseExpr(typeString) + if err != nil { + panic(p.errorf(x, "parse type expr: %v", err)) + } + qn, ok := n.(*ast.SelectorExpr) + if !ok { + panic(p.errorf(x, "can't resolve %s type; try a fully-qualified name", typeString)) + } + pkgName, ok := qn.X.(*ast.Ident) + if !ok { + panic(p.errorf(qn.X, "invalid package name")) + } + pkgPath, ok := p.itab.Lookup(pkgName.Name) + if !ok { + panic(p.errorf(qn.X, "package %s is not imported", pkgName.Name)) + } + pkg, err := p.importer.Import(pkgPath) + if err != nil { + panic(p.errorf(n, "can't load %s: %v", pkgPath, err)) + } + obj := pkg.Scope().Lookup(qn.Sel.Name) + if obj == nil { + panic(p.errorf(n, "%s is not found in %s", qn.Sel.Name, pkgPath)) + } + iface, ok := obj.Type().Underlying().(*types.Interface) + if !ok { + panic(p.errorf(n, "%s is not an interface type", qn.Sel.Name)) + } + return iface +} + func (p *rulesParser) toStringValue(x ast.Node) (string, bool) { switch x := x.(type) { case *ast.BasicLit: diff --git a/ruleguard/ruleguard_error_test.go b/ruleguard/ruleguard_error_test.go index 00d3370f..cac284b1 100644 --- a/ruleguard/ruleguard_error_test.go +++ b/ruleguard/ruleguard_error_test.go @@ -257,12 +257,15 @@ func TestParseFilterError(t *testing.T) { { `m["x"].Type.Implements("foo")`, - "only `error` unqualified type is recognized", + `can't resolve foo type; try a fully-qualified name`, }, - { `m["x"].Type.Implements("func()")`, - "only qualified names (and `error`) are supported", + `can't resolve func() type; try a fully-qualified name`, + }, + { + `m["x"].Type.Implements("bytes.Buffer")`, + `bytes.Buffer is not an interface type`, }, {