Sounds reasonable, and reminds me of the checks that already take place for fmt.Printf.

The check ticks the correctness vet requirement, since a different number of arguments is definitely a bug. Precision shouldn't be a problem either, as it should be easy to avoid false positives. Frequency is always a bit more hard to prove - do you have any proof of people running into this issue besides your own experience?

/cc @alandonovan @josharian

do you have any proof of people running into this issue besides your own experience?

I don't. Maybe we can spread this bug around and let people +1 or -1 the issue and gauge reaction.

This would be great for an external tool.

I'm not sure it could properly lex SQL server, pg, MySQL, Oracle, both named and indexed(@ $ :), understand comments (vendors support different comment styles), properly handle :: symbols but reading :N vars. It would need to handle T-SQL and PL/SQL. As soon as the sql string was not a constant it would need to ignore the check.

This would be fine in a limited case for an external tool.

The query strings are interpreted by the driver and are database specific. PostgreSQL uses $1, MySQL uses ?, SQLite can use ? but has others. Any check would need to understand SQL and dialects for all common servers in order to know if there's a problem at compile time.

I didn't know that database/sql had to work with many syntaxes - then perhaps it's too complex of a check for vet. I'm suddenly less sure that a precise check would be easy to do :)

I agree that an external tool could be a good start, to gather data about the check's viability and usefulness.

Yeah .. that's why I thought it wouldn't be feasible. Maybe I will create something just for postgres since I need it.

Closing. Thanks for the input everyone.

It seems like a good application for a static checker. SQL is a fairly widely used standard package, and SQL queries would benefit from detection similar to printf format strings. That said, SQL is much less frequently used than printf, and printf misformatting often goes undetected because it does not return an error, whereas a malformed SQL query is destined to return an error and such errors are invariably checked, so your checker will uncover relatively few problems in practice.

If you proceed, I suggest you use the new golang.org/x/tools/go/analysis.Analyzer API so that the checker can be freely incorporated into vet or other tools. Take a look at the existing printf checker as a starting point. You should attempt to quickly skip the analyzer if the package does not import database/sql, or you should attempt to identify and check db.Exec wrapper functions similar to the way we identify and check printf wrapper functions in packages other than fmt.

Awesome, thanks Alan !

FYI - I went ahead and made this https://github.com/agnivade/sqlargs. It works only for postgres queries as of now. @alandonovan - Any feedback will be appreciated !

I didn't use the Fact type as I didn't see any need for it. From what I understand, it is needed when another pass needs info from the current pass, or tests need it. But I was able to run tests by just testing the Reportf output which gets caught by "//want " stanzas. Is this okay ?

P.S. I am thinking of writing a blog post on how to write a vet analyzer pass.

Correct, facts are only needed in the situation you describe. In your scenario, you might want to use facts if you have functions that call db.Exec, and you want to check calls to them as if they were direct calls to db.Exec.

Quick review:

• Consider using the inspector pass; it's faster than ast.Inspect and easier to use.
• sel.Sel == nil can't happen
• Rather than use arg[0].BasicLit and Unquote, use info.Types[arg0].Value, which will give you the constant value directly. Also, it works for any constant, such as k, or "foo"+"bar", not just string literals.
• Style: for "if fnName". use a switch with 'case "Exec", "QueryRow", "Query"'.
• call ptr.Elem.String only once as it alllocates memory. Better still, call it zero times by picking apart the pointer-to-Named and checking that the name is one of (FB, Tx, Stmt) and the package is database/sql.
  This may be an unnecessary optimization here because you've already applied enough conditions, but especially in the early checks of a visit function, avoiding allocation can really make a big difference.
• delete FactTypes, you don't need it.

Thanks for taking the time to review !

info.Types[arg0].Value sounds great !

For the switch case, I had thought of it. Since I am actually checking for the inverse of the condition, doing case "Exec", "QueryRow", "Query" becomes a mess of nested code, which I wanted to avoid.

FactTypes - yes I will remove that.