-
Notifications
You must be signed in to change notification settings - Fork 25
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
Rewrite sqlvet using the analyzer framework. #8
base: master
Are you sure you want to change the base?
Conversation
Use golang.org/x/tools/go/analysis framework. It's simpler, faster. It also allows analyzing individual packages, not the whole program. As a simple benchmark, running the sqlvet (old or new) on a 100kloc source takes about 10s. Analyzing a single file in that source takes <1s. This PR introduces a few incompatible changes: - The commandline format changes. It's now sqlvet [-f sqlvet.toml] packages... The sqlvet.toml file must be in ".", or its location must be explicitly specified by the new "-f" flag. This commandline format is compatible with by most other analyzers, including govet and staticcheck. The old `sqlvet <dir>` command becomes: cd <dir> && sqlvet ./... - It removes the support for string concatenation. db.Query("SELECT" + " 1") won't work any more. I personally think this isn't that big deal, we can easily rewrite it using a `raw string`.
Thank you @yasushi-saito, the code simplification looks great 👍 I will take a closer look at it tomorrow :) |
Thanks @yasushi-saito again for taking a stab at this. I now remember why I went with the SSA form instead of AST in sqlvet. I think I even read the vet-analyzer article you linked at the time because it predates the project. I initially started with AST, but found out it was too restricted for more advanced code analysis. String concatenation is the simplest application of the SSA call graph, in my todo, I was also planning to do function tracing and recursively resolve variable type and value to reduce false positives. For example, the Analyzer approach is currently causing the following false positives to one of my projects: type Foo struct {
DbName string
}
func (s Foo) Dosomething() {
// ...
dbConn.Exec("QUERY ..." + s.DbName)
} Have you considered the following two workarounds?
|
// log.Printf("QQQQ: %v err=%v", qs.Query, qs.Err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// log.Printf("QQQQ: %v err=%v", qs.Query, qs.Err) |
if shouldIgnoreNode(pass, ignoredNodes, n.Pos()) { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the ignore check could be performed at the beginning of the callback?
Any word on updating this? |
@houqp I guess this is somewhat stale ? If we manage to update this MR or some other version of it; could we have this merged ? It would be a really nice addition to |
@adrianlungu yes, I am open to merging this as long as the limitations in my comments are addressed 👍 |
Use golang.org/x/tools/go/analysis framework. It's simpler, faster. It also
allows analyzing individual packages, not the whole program.
As a simple benchmark, running the sqlvet (old or new) on a 100kloc source takes
about 10s. Analyzing a single file in that source takes <1s.
This PR introduces a few incompatible changes:
The commandline format changes. It's now
sqlvet [-f sqlvet.toml] packages...
The sqlvet.toml file must be in ".", or its location must be explicitly
specified by the new "-f" flag. This commandline format is compatible with by
most other analyzers, including govet and staticcheck.
The old
sqlvet <dir>
command becomes:cd
&& sqlvet ./...It removes the support for string concatenation.
db.Query("SELECT" + " 1")
won't work any more. I personally think this isn't that big deal, we can
easily rewrite it using a
raw string
.