Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ruleguard: improve FieldList and Field matching #342

Merged
merged 1 commit into from Jan 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 34 additions & 0 deletions analyzer/testdata/src/regression/issue315.go
@@ -0,0 +1,34 @@
package regression

import (
"io"
)

type Issue315_Iface interface {
Example()
}

func Issue315_Func() {
}

func Issue315_FuncErr() error {
return nil
}

func Issue315_Func1() io.Reader { // want `\Qreturn concrete type instead of io.Reader`
return nil
}

func Issue315_Func2() (io.Reader, error) { // want `\Qreturn concrete type instead of io.Reader`
return nil, nil
}

func Issue315_Func3() (int, Issue315_Iface, error) { // want `\Qreturn concrete type instead of Issue315_Iface`
return 0, nil, nil
}

type Issue315_Example struct{}

func (example Issue315_Example) Func1(x int) (Issue315_Iface, error) { // want `\Qreturn concrete type instead of Issue315_Iface`
return nil, nil
}
13 changes: 13 additions & 0 deletions analyzer/testdata/src/regression/rules.go
Expand Up @@ -51,3 +51,16 @@ func issue339(m dsl.Matcher) {
m.Match(`println("339"); println("x")`).Report("pattern1")
m.Match(`println("x"); println("339")`).Report("pattern2")
}

func issue315(m dsl.Matcher) {
m.Match(
`func $name($*_) $arg { $*_ }`,
`func $name($*_) ($arg, $_) { $*_ }`,
`func $name($*_) ($_, $arg, $_) { $*_ }`,
`func ($_ $_) $name($*_) ($arg, $_) { $*_ }`,
).Where(
m["name"].Text.Matches(`^[A-Z]`) &&
m["arg"].Type.Underlying().Is(`interface{ $*_ }`) &&
!m["arg"].Type.Is(`error`),
).Report(`return concrete type instead of $arg`).At(m["name"])
}
4 changes: 2 additions & 2 deletions ruleguard/filters.go
Expand Up @@ -254,7 +254,7 @@ func makeTypeIsFilter(src, varname string, underlying bool, pat *typematch.Patte
return pat.MatchIdentical(params.typeofNode(x).Underlying())
})
}
typ := params.typeofNode(params.subExpr(varname)).Underlying()
typ := params.typeofNode(params.subNode(varname)).Underlying()
if pat.MatchIdentical(typ) {
return filterSuccess
}
Expand All @@ -268,7 +268,7 @@ func makeTypeIsFilter(src, varname string, underlying bool, pat *typematch.Patte
return pat.MatchIdentical(params.typeofNode(x))
})
}
typ := params.typeofNode(params.subExpr(varname))
typ := params.typeofNode(params.subNode(varname))
if pat.MatchIdentical(typ) {
return filterSuccess
}
Expand Down
14 changes: 9 additions & 5 deletions ruleguard/gorule.go
Expand Up @@ -91,12 +91,16 @@ func (params *filterParams) subExpr(name string) ast.Expr {
}

func (params *filterParams) typeofNode(n ast.Node) types.Type {
if e, ok := n.(ast.Expr); ok {
if typ := params.ctx.Types.TypeOf(e); typ != nil {
return typ
}
var e ast.Expr
switch n := n.(type) {
case ast.Expr:
e = n
case *ast.Field:
e = n.Type
}
if typ := params.ctx.Types.TypeOf(e); typ != nil {
return typ
}

return types.Typ[types.Invalid]
}

Expand Down
9 changes: 9 additions & 0 deletions ruleguard/runner.go
Expand Up @@ -9,6 +9,7 @@ import (
"go/printer"
"io/ioutil"
"path/filepath"
"reflect"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -390,6 +391,14 @@ func (rr *rulesRunner) renderMessage(msg string, m matchData, truncate bool) str
if !strings.Contains(msg, key) {
continue
}
// Some captured nodes are typed, but nil.
// We can't really get their text, so skip them here.
// For example, pattern `func $_() $results { $*_ }` may
// match a nil *ast.FieldList for $results if executed
// against a function with no results.
if reflect.ValueOf(n).IsNil() {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this if can be a bit higher, before strings.Contains

buf.Reset()
buf.Write(rr.nodeText(n))
replacement := buf.String()
Expand Down