code review 12811043: cmd/vet: flag redundant invocations of String and Error...

cmd/vet: flag redundant invocations of String and Error methods in printf calls.

[dsymonds, 2013-08-12 22:22:56]

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

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

[r, 2013-08-13 01:07:41]

https://codereview.appspot.com/12811043/diff/6001/cmd/vet/print.go
File cmd/vet/print.go (right):

https://codereview.appspot.com/12811043/diff/6001/cmd/vet/print.go#newcode396
cmd/vet/print.go:396: func (f *File) checkIfArgIsRedundant(arg ast.Expr) {
comment

https://codereview.appspot.com/12811043/diff/6001/cmd/vet/print.go#newcode405
cmd/vet/print.go:405: if n := sel.Sel.Name; n == "String" || n == "Error" {
you need to check the type. it must be string.

[dsymonds, 2013-08-13 01:24:41]

https://codereview.appspot.com/12811043/diff/6001/cmd/vet/print.go
File cmd/vet/print.go (right):

https://codereview.appspot.com/12811043/diff/6001/cmd/vet/print.go#newcode396
cmd/vet/print.go:396: func (f *File) checkIfArgIsRedundant(arg ast.Expr) {
On 2013/08/13 01:07:41, r wrote:
> comment

Done.

https://codereview.appspot.com/12811043/diff/6001/cmd/vet/print.go#newcode405
cmd/vet/print.go:405: if n := sel.Sel.Name; n == "String" || n == "Error" {
On 2013/08/13 01:07:41, r wrote:

> you need to check the type. it must be string.

I also want to use this in situations where type checking fails. Are we that
worried for this purpose that someone might have a method String() returning
int? If it doesn't return a string-ish then matchArgType will flag it.

[r, 2013-08-13 01:33:55]

if type checking fails, you can still complain, but if it doesn't you
should check. Error() for instance is the one i'm more worried about. it's
a fine method name.

[r, 2013-08-13 01:34:53]

matchArgType will complain if it can. it might not be able to.

[dsymonds, 2013-08-13 01:49:40]

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

Please take another look.

[r, 2013-08-13 02:18:54]

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/testdata/print.go
File cmd/vet/testdata/print.go (right):

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/testdata/print.go#...
cmd/vet/testdata/print.go:192: Printf("%s", stringerv.String()) // ERROR
"redundant invocation of String method of stringerv"
an Error one too

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/types.go#newcode287
cmd/vet/types.go:287: func (f *File) doesNotReturnString(sel *ast.SelectorExpr)
bool {
negative true method call: no.

returnsString

put the negation at the call site.

[dsymonds, 2013-08-13 02:27:26]

PTAL

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/testdata/print.go
File cmd/vet/testdata/print.go (right):

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/testdata/print.go#...
cmd/vet/testdata/print.go:192: Printf("%s", stringerv.String()) // ERROR
"redundant invocation of String method of stringerv"
On 2013/08/13 02:18:54, r wrote:

> an Error one too

Done, with an expression to exercise the type checker.

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/12811043/diff/16001/cmd/vet/types.go#newcode287
cmd/vet/types.go:287: func (f *File) doesNotReturnString(sel *ast.SelectorExpr)
bool {
On 2013/08/13 02:18:54, r wrote:

> negative true method call: no.
> 
> returnsString
> 
> put the negation at the call site.

I was concerned about the type checking subtlety here and was trying to cover
the bases, but I guess it's not worth it. Done.

[r, 2013-08-13 03:26:35]

https://codereview.appspot.com/12811043/diff/21001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/12811043/diff/21001/cmd/vet/types.go#newcode287
cmd/vet/types.go:287: func (f *File) returnsString(sel *ast.SelectorExpr) bool {
should just be an Expr so it can be used more broadly. also would be nice to
find a way to share the code one line above

[dsymonds, 2013-08-13 03:32:32]

https://codereview.appspot.com/12811043/diff/21001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/12811043/diff/21001/cmd/vet/types.go#newcode287
cmd/vet/types.go:287: func (f *File) returnsString(sel *ast.SelectorExpr) bool {
On 2013/08/13 03:26:36, r wrote:

> should just be an Expr so it can be used more broadly. also would be nice to
> find a way to share the code one line above

Made this take an ast.Expr.

The subtlety of behaving the desired way if type checking failed makes it
impossible to reuse the previous method.

[r, 2013-08-13 03:39:09]

that's not what i meant. sorry for being unclear. i meant, take that horrible
final line from the last two functions in this file and make it a helper
function

[dsymonds, 2013-08-13 03:40:53]

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

Please take another look.

[r, 2013-08-13 03:44:19]

https://codereview.appspot.com/12811043/diff/23007/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/12811043/diff/23007/cmd/vet/types.go#newcode276
cmd/vet/types.go:276: // Finally the real questions.
s/s//

https://codereview.appspot.com/12811043/diff/23007/cmd/vet/types.go#newcode280
cmd/vet/types.go:280: // returnsString reports whether expr is a function that
returns a string.
s/function/call of a function/

https://codereview.appspot.com/12811043/diff/23007/cmd/vet/types.go#newcode294
cmd/vet/types.go:294: func hasStringReturn(sig *types.Signature) bool {
// hasStringReturn reports whether the function signature has exactly one return
value, of universe type string.

[dsymonds, 2013-08-13 03:45:55]

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

Please take another look.

[r, 2013-08-13 03:47:18]

LGTM

[dsymonds, 2013-08-13 03:48:06]

*** Submitted as
https://code.google.com/p/go/source/detail?r=44387735a77d&repo=tools ***

cmd/vet: flag redundant invocations of String and Error methods in printf calls.

R=r
CC=golang-dev
https://codereview.appspot.com/12811043