code review 7378061: cmd/vet: check argument types in printf formats

cmd/vet: check argument types in printf formats
Fixes issue 4404.

[r, 2013-02-23 18:12:26]

Hello gri (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2013-02-23 18:28:54]

If you care.

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go
File src/cmd/vet/print.go (right):

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode18
src/cmd/vet/print.go:18: "unsafe" // just for test case printing unsafe.Pointer
If you move this into a separate print_bad.go file with a // +build ignore tag,
you can keep it from being required for building vet.

[r, 2013-02-23 18:43:11]

Hello gri@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2013-02-23 18:44:00]

PTAL

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go
File src/cmd/vet/print.go (right):

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode18
src/cmd/vet/print.go:18: "unsafe" // just for test case printing unsafe.Pointer
On 2013/02/23 18:28:54, rsc wrote:
> If you move this into a separate print_bad.go file with a // +build ignore
tag,
> you can keep it from being required for building vet.

Done.

[gri, 2013-02-23 18:51:12]

LGTM

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/atomic.go
File src/cmd/vet/atomic.go (right):

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/atomic.go#newcode13
src/cmd/vet/atomic.go:13: // checkAtomicAssignment walks the assignment
statement checking for common
comomon is cute, though

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go
File src/cmd/vet/print.go (right):

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode110
src/cmd/vet/print.go:110: func (f *File) checkPrintf(call *ast.CallExpr, name
string, skip int) {
Can we rename skip - I find it confusing here. It seems like argNo would be
clearer.

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode111
src/cmd/vet/print.go:111: if len(call.Args) <= skip {
if argNo >= len(call.Args) {
   return
}

seems clearer

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode316
src/cmd/vet/print.go:316: case types.Uint, types.Uint8, types.Uint16,
types.Uint32, types.Uint64, types.Uintptr:
2 lines instead of two cases? fallthrough? (see comment below as well)

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode334
src/cmd/vet/print.go:334: case uint, uint8, uint16, uint32, uint64:
just use a 2nd line rather than a 2nd case? then you don't need the integral bit
either, you can just do the return. Alternative, use a fallthrough and return.

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode337
src/cmd/vet/print.go:337: if integral {
can go into the switch

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode540
src/cmd/vet/print.go:540: // TODO
add some random strings as well  - all these cases are very regular. empty
format string?

[r, 2013-02-23 18:51:29]

Hello gri@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2013-02-23 19:08:17]

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go
File src/cmd/vet/print.go (right):

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode110
src/cmd/vet/print.go:110: func (f *File) checkPrintf(call *ast.CallExpr, name
string, skip int) {
On 2013/02/23 18:51:12, gri wrote:
> Can we rename skip - I find it confusing here. It seems like argNo would be
> clearer.

Done.

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode316
src/cmd/vet/print.go:316: case types.Uint, types.Uint8, types.Uint16,
types.Uint32, types.Uint64, types.Uintptr:
On 2013/02/23 18:51:12, gri wrote:
> 2 lines instead of two cases? fallthrough? (see comment below as well)
see below, although here (unlike with the type switch) i can use a fallthrough.

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode334
src/cmd/vet/print.go:334: case uint, uint8, uint16, uint32, uint64:
done, but i like keeping int and uint separately like this. makes it easier for
me to see i have all the cases.

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode337
src/cmd/vet/print.go:337: if integral {
On 2013/02/23 18:51:12, gri wrote:
> can go into the switch

Done.

https://codereview.appspot.com/7378061/diff/2001/src/cmd/vet/print.go#newcode540
src/cmd/vet/print.go:540: // TODO
added a couple. there are plenty of printf tests here.

[r, 2013-02-23 19:08:29]

Hello gri@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2013-02-23 19:29:39]

LGTM

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go
File src/cmd/vet/print.go (right):

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go#newcode80
src/cmd/vet/print.go:80: lit.Value = strconv.Quote(x + y)
you are changing the literal in place in the AST here - not very clean. would be
better to create a new literal

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go#newcod...
src/cmd/vet/print.go:108: func (f *File) checkPrintf(call *ast.CallExpr, name
string, formatIndex int) {
much better

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go#newcod...
src/cmd/vet/print.go:123: if err != nil {
that should never happen if the parser returned no errors

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go#newcod...
src/cmd/vet/print.go:126: firstArg := formatIndex + 1
add a comment?

// format arguments follow the format immediately

[r, 2013-02-23 23:08:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=8eb896294200 ***

cmd/vet: check argument types in printf formats
Fixes issue 4404.

R=gri, rsc
CC=golang-dev
https://codereview.appspot.com/7378061

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go
File src/cmd/vet/print.go (right):

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go#newcod...
src/cmd/vet/print.go:123: if err != nil {
On 2013/02/23 19:29:40, gri wrote:
> that should never happen if the parser returned no errors

Done.

https://codereview.appspot.com/7378061/diff/15001/src/cmd/vet/print.go#newcod...
src/cmd/vet/print.go:126: firstArg := formatIndex + 1
On 2013/02/23 19:29:40, gri wrote:
> add a comment?
> 
> // format arguments follow the format immediately

Done.