Suggestion: add Addr keyword to get pointer type from non-pointer type
#129
Comments
|
Sorry for the late response. The common approach for your kind of issue is to use 2 patterns: Matcher(`&$p.$x{$_*}`,
`$p.$x{$_*}`).
Where(m["x"].Type.Implements(`mypkg.MyInterface`))It also looks like Matcher(`&$x{$_*}`,
`$x{$_*}`).
Where(m["x"].Type.Implements(`mypkg.MyInterface`))I'll try your example tomorrow and see if I can provide a better answer. |
|
With the following pattern: That will match against code BTW, coming laterally from the concept of greedy/non greedy regular expression, it would be great to explain how greedy the matches are. For example, with the code |
You're right. On the other hand, we may allow using The AST pattern matching details rely on the gogrep. I may try to document important things and send a PR to Daniel so the gogrep users can use it more easily, but I believe such details are "implementation-defined" and complex patterns may or may not work as you might intend them to work. The best solution here is to test rules and see whether they behave correctly. #91 can help here. |
I understand how that would solve this scenario: |
|
Could you share an example of a warning text for this pattern so I can understand it better? |
Sure. An example would be Replace struct initialization mypackage.Xyz{} with call to factory method mypackage.NewXyz(). |
sebastien-rosset
changed the title
PtrTo keyword or equivalentAddr keyword or equivalent
sebastien-rosset
changed the title
Addr keyword or equivalentAddr keyword to get pointer type from non-pointer type
|
I see the picture now, thank you. So, you want to match literals that "may" implement an interface even if they don't in the way they're written inside the code. This means we're adding a concept of types manipulation to the engine. This is a novel concept, hence it took some time for me to grasp it. We can add new methods to the If we could write some kinds of custom predicates, this would be a more general solution: func implementsStringer(ctx dsl.Context, typ types.Type) bool {
return ctx.Implements(typ, `fmt.Stringer`) ||
ctx.Implements(types.NewPointer(typ), `fmt.Stringer`)
}
func stringerLiteral(m dsl.Matcher) {
m.Match(`$x{$*_}`).
Where(m["x"].Type.Check(implementsStringer)).
Report(`$x implements fmt.Stringer`)
}Where The problem is: there is no Go interpreter inside the ruleguard. I thought about using yaegi, but I'm not 100% convinced yet. Not that the performance is the only concern (it would make custom filters many times slower), but also it's a big dependency that I'm not to keen to take right now. I may be wrong here. If ruleguard rules were 100% normal Go code and were executed with Having custom extension filters looks like a must-have to create serious checks. The current mechanisms only good enough for simple things and it was, honestly speaking, supposed to be this way. Complicated checks were to be written as normal analyzers/linters. Not saying I wouldn't like to extend what ruleguard should be capable of though. |
|
In principle, it would be nice to be able to invoke a go interpreter or compiled go code through reflection, though as you mention one downside is performance and memory utilization. While running ruleguard I've noticed memory utilization can be high, I need to spend some time understanding why. Presumably it would be harder to debug memory usage if/when arbitrary functions can be invoked. Or, how about supporting go plugins? For this specific need, the changes (PR #137 137) are small and localized. |
|
How about supporting the following grammar: where "foo.so" is a go plugin and symbol_name is the name of a exported Similarly, there could be |
|
Plugins could be one way to solve it. I don't dislike them that much, but they might be too much for some simple cases. I started to work on a solution, but I need a week or two to show some results. |
|
On my local branch, this code can now be used: package gorules
import (
"github.com/quasilyte/go-ruleguard/dsl"
"github.com/quasilyte/go-ruleguard/dsl/types"
)
// Note: the exact dsl.VarFilterContext API may change; for example, Type()
// may become a field instead of a method.
func implementsStringer(ctx *dsl.VarFilterContext) bool {
// GetInterface() returns *types.Interface for a given FQN
stringer := ctx.GetInterface(`fmt.Stringer`)
return types.Implements(ctx.Type(), stringer) ||
types.Implements(types.NewPointer(ctx.Type()), stringer)
}
func stringerLiterals(m dsl.Matcher) {
m.Match(`$x{$*_}`).
Where(m["x"].Filter(implementsStringer)).
Report("$x implements stringer")
}
Custom filter functions are more low level, but they still can use helpers like If we run it on this Go file: package target
type byValue struct{}
type byPtr struct{}
func (*byPtr) String() string { return "" }
func (byValue) String() string { return "" }
func f() {
_ = &byValue{}
_ = byValue{}
_ = &byPtr{}
_ = byPtr{} // does not implement fooer, but we still want it
}We'll get this output: Under the hood, there is a simple bytecode interpreter for a Go subset. It should be good enough for filters. Performance-wise, it's better than yaegi and the memory footprint is quite low. Calling "native" Go functions from the interpreted code (benchmark does 2 function calls): The exact numbers are irrelevant, but it shows that it's quite good on the performance side even though I did 0 optimization efforts and don't use I need a few more days to finalize it (if nothing distracts me from it, of course). |
`dsl/types` is a wrapper around `go/types` that provides the minimal interface that is useful for custom filter. We may add `dsl/ast` later to mimic `go/ast` as well for node-related custom filters. See #129 (comment) to get more context on what this is about.
`dsl/types` is a wrapper around `go/types` that provides the minimal interface that is useful for custom filter. We may add `dsl/ast` later to mimic `go/ast` as well for node-related custom filters. See #129 (comment) to get more context on what this is about.
`dsl/types` is a wrapper around `go/types` that provides the minimal interface that is useful for custom filter. We may add `dsl/ast` later to mimic `go/ast` as well for node-related custom filters. See #129 (comment) to get more context on what this is about.
Custom filters are simple Go functions defined in ruleguard rule files that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when matched rule needs to apply its filters. The performance is good enough for now (faster than yaegi) and can be further improved in future. There are various limitations in what can be used in custom filters and what not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Type.Check(implementsStringer)). Report(`$x implements fmt.Stringer`) } Custom filters are more flexible predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review.
that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject).
that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject).
that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject).
that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject).
that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject). Also fixes #167 and #170
that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject). Also fixes #167 and #170
that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject). Also fixes #167 and #170
#173) that can be used as Where clause predicates. They're compiled to a bytecode that is then interpreted when the matched rule needs to apply its filters. The performance is good enough for now (faster than `yaegi`) and can be further improved in the future. There are various limitations in what can be used in custom filters and what's not. It's not well-documented right now, but the compile errors try to be helpful. What can be compiled will be executed like a normal Go would. One use case for this new feature can be found in #129 Here is the solution to #129 which is not possible: ```go func implementsStringer(ctx *dsl.VarFilterContext) bool { stringer := ctx.GetInterface(`fmt.Stringer`) return types.Implements(ctx.Type, stringer) || types.Implements(types.NewPointer(ctx.Type), stringer) } func stringerLiteral(m dsl.Matcher) { m.Match(`$x{$*_}`). Where(m["x"].Filter(implementsStringer)). Report(`$x implements fmt.Stringer`) } ``` Custom filters are more flexible than predefined filters, but they generally require more coding. As a rule of thumb: they should be used only when there is no matching builtin filter. Note: right now many simple operations, like integer multiplication, are not implemented. They can be added trivially, but I wanted to present a working concept with a minimal amount of code for this first PR. Upcoming changes that extend the supported features set are going to be easier to review. It's also possible to allow calling byte-compiled functions from other byte-compiled functions. But it's not there yet (I also need examples where it can be applied to get a better understanding of the subject). Also fixes #167 and #170
|
See https://github.com/quasilyte/go-ruleguard/blob/master/analyzer/testdata/src/quasigo/rules.go for some extra examples. |
|
Thanks. Let me test with my use case and then I can close both the issue and the PR. |
|
How should I test the latest (unreleased) software? Sorry this seems like a trivial question and previously I was able to test, but now I'm getting errors. I've tried: GO111MODULE=on go get -v -u github.com/quasilyte/go-ruleguard/...@masterWhen I run ruleguard I get the following error: I know previously it was I've also tried: git clone [email protected]:quasilyte/go-ruleguard.git
cd go-ruleguard
go install ./...Then when I run ruleguard I get the following error: The printed Here is the // +build ignore
package gorules
import (
"github.com/quasilyte/go-ruleguard/dsl"
"github.com/quasilyte/go-ruleguard/dsl/types"
)
func implementsStringer(ctx *dsl.VarFilterContext) bool {
stringer := ctx.GetInterface(`fmt.Stringer`)
return types.Implements(ctx.Type, stringer) ||
types.Implements(types.NewPointer(ctx.Type), stringer)
} |
|
You can try the latest release binaries (v0.3.0). In order to make ruleguard find
Running the You can check out the _test/install to see the supported installation options.
That error may be related to the fact that we get different |
|
I've recompiled go and then used it to compile ruleguard. I've added debugging info in https://github.com/golang/go/blob/master/src/go/types/predicates.go#L147. When ruleguard executes,
This happens when ruleguard compares two I suspect this has to do with vendoring. If I run ruleguard with a basic test, everything works. But in my real project, there is vendoring. I will add more debug info. Here is the stack trace when the comparison fails: |
|
Actually I'm not so sure about vendoring being the problem. I discovered |
|
I would start my attempt by trying to create a docker test that gives the universal reproducer in clean environment. If it can't be reproduced there, it must be an environment issue which would require us to guess what needs to be fixed (and maybe add these fixing steps to the documentation or some kind of FAQ). Could you share your I'm planning to give it a try this weekend. |
|
Here: https://github.com/quasilyte/go-ruleguard/blob/master/go.mod#L7 As an aside, |
|
I have created a few Dockerfiles here: https://github.com/sebastien-rosset/ruleguard-test. For example: FROM golang:1.15.6
WORKDIR /go/src/app
COPY . .
# Install ruleguard as documented at https://github.com/quasilyte/go-ruleguard
RUN GO111MODULE=on go get -v github.com/quasilyte/go-ruleguard/...
CMD [ "ruleguard", "-rules", "rules.go", "."]> docker build -t ruleguard-test . ; docker run --rm ruleguard-test
ruleguard: load rules: parse rules file: typechecker error: rules.go:6:3: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")
```__ |
That flag comes from the analysis framework (https://github.com/golang/tools/tree/master/go/analysis/internal/analysisflags). I have no idea why it doesn't work or what it's supposed to do. :) I think it expects an argument like
In theory, yes. But it's probably only relevant for running the tests. The DSL package should be a dependency of a package that is being checked (because we do a run-time based package importing, the package lookup depends on what I'll investigate your reproducer today. Thank you for the efforts! It really helps. |
|
OK, now I see what's going on. There are 2 main ways to use ruleguard:
The installation instruction should be updated I guess. |
|
I've updated the installation instructions. |
|
Thank you! Now I'll try to reproduce |
|
It looks like the installation instructions for gocritic need to be updated too. With I have updated the Dockerfiles for test purpose at https://github.com/sebastien-rosset/ruleguard-test |
|
I've noticed if I run Dockerfile: |
|
@quasilyte , I have narrowed the problem to help troubleshoot I've noticed the following:
- ruleguard -rules $(echo gorules/*.go | tr ' ' ',') ./...
+ ruleguard -rules rules.go ./...In parser.go I've added log statements to list the files in the UPDATE: I was able to provide a minimal test to reproduces the issue. It turns out the order of the rule file names on the command line matters (everything else being the same). - ruleguard -rules log-rule.go,worker-rule.go,string-rule.go . # FAIL
+ ruleguard -rules worker-rule.go,string-rule.go,log-rule.go . # SUCCESSHere is the Dockerfile that outputs "wrong type" error: https://github.com/sebastien-rosset/ruleguard-test/blob/master/Dockerfile.wrongtype |
|
This thread has become too long. |
Good idea. 👍 |
|
Even with custom filters, it's still somewhat cumbersome to solve this issue. |
Suppose we want to match a struct initialization statement, but only when the struct implements interface
mypkg.MyInterface. For example:Here is a first attempt at writing a ruleguard checker:
However this does not work because
ExprType.Implements()return true only if the ExprType is a pointer to a type. Is there a way to convert to a pointer, or do we need to add a new keyword to get the pointer to a type given anExprType? Maybe something like this?As a side note, my observation is that
Matcher("$p.$x{$_*}")orMatcher("$x{$_*}")patterns are not greedy, which makes sense but is not necessarily obvious. In thea := &mypkg.Type1{}example, "m[x].Type" is the typemypkg.Type1, not*mypkg.Type1.The text was updated successfully, but these errors were encountered: