code review 9774043: go.tools/ssa: fix debug printing (issue 9774043)

[g...@golang.org]

Reviewers: adonovan,

Message:
Hello adonovan (cc: golan...@googlegroups.com),

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


Description:
go.tools/ssa: fix debug printing

Please review this at https://codereview.appspot.com/9774043/

Affected files:
M ssa/promote.go


Index: ssa/promote.go
===================================================================
--- a/ssa/promote.go
+++ b/ssa/promote.go
@@ -262,7 +262,7 @@
sig := types.NewSignature(types.NewVar(nil, "recv", typ), old.Params(),
old.Results(), old.IsVariadic())

if prog.mode&LogSource != 0 {
- defer logStack("makeBridgeMethod %s, %s, type %s", typ, cand, &sig)()
+ defer logStack("makeBridgeMethod %s, %s, type %s", typ, cand, sig)()
}

fn := &Function{

[r...@golang.org]

LGTM

https://codereview.appspot.com/9774043/

[g...@golang.org]

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

go.tools/ssa: fix debug printing

R=adonovan, r
CC=golang-dev
https://codereview.appspot.com/9774043


https://codereview.appspot.com/9774043/

[Alan Donovan]

This particular bug pattern has cropped up literally dozens of times since the go/types accessors refactoring.

Specifically:

1) you have a public field named X.

2) you rename it to x and add an X() accessor method.

3) you let the compiler tell you which references to p.X to replace with p.X().

The compiler will fail to report all uses of fmt.Printf("%s", p.X), with the result that a bound-method closure is printed instead of the field's value.  This is particularly pernicious in logging statements since they are almost never covered by tests.

Now if only we had the technology to build a tool to report incorrect (or likely unintended) format specifier/parameter type combinations in calls to printf, including logging wrappers around printf...

I've added it my TODO list.

[Rob Pike]

does vet catch it?

-rob

[Alan Donovan]

On 25 May 2013 14:30, Rob Pike <r...@golang.org> wrote:

does vet catch it?

No, not for Printf, and I certainly would expect it to do so for wrappers around Printf.

[Russ Cox]

vet should catch it (printing a method value) in Printf.

wrappers should be named appropriately (Printf, Logf, Fatalf) so that vet catches them too.

[Alan Donovan]

Am I using the wrong vet tool then?

% cat a.go

package p

import "fmt"

type T struct{}

func (T) m() int {

        return 0

}

func f() int {

        return 0

}

func main() {

   var t T

   fmt.Printf("%d\n", t.m)

   fmt.Printf("%d\n", f)

   panic("oops")

   println(1)  // "unreachable code"

}

% go vet a.go

a.go:20: unreachable code

[Russ Cox]

I meant vet should, not vet does. As in, there's no reason it can't, and it would fit with the other Printf checks it already does (when compiled with go/types support).

Russ

[Alan Donovan]

On 29 May 2013 13:54, Russ Cox <r...@golang.org> wrote:

I meant vet should, not vet does. As in, there's no reason it can't, and it would fit with the other Printf checks it already does (when compiled with go/types support).

Ah, quite different. :)

I'll add it to my todo list.

Arguably any function whose arg list ends with (format string, args ... interface{}) signature should get this checking.

[Russ Cox]

On Wed, May 29, 2013 at 1:55 PM, Alan Donovan <adon...@google.com> wrote:

Arguably any function whose arg list ends with (format string, args ... interface{}) signature should get this checking.

That's not clear to me. What if someone implements a separate printf library with C semantics?

Russ 

[Alan Donovan]

I'd like to experiment with analysing the wrappers directly.  Define a "printf-like function" as fmt.Printf and friends, plus any function that passes its string+[]interface{} arguments to a printf-like function.  It should be pretty easy to identify printf-like functions with the SSA code for a single package and apply the checking at all calls where the format is a literal string.

[Russ Cox]

That only works if you assume complete source to all dependencies. That's a new assumption, at least for vet.

[Alan Donovan]

Not really; like the typechecker, the SSA builder can be run in a mode where it uses gc's object files instead of source to satisfy dependencies.

That does mean you still need object code, and also that you can't do as deep an analysis (e.g. you can't apply the predicate I described for "printf-like functions" to code you can't see), but for calls within a package, it works.

[Russ Cox]

Part of the reason interfaces are great is that they encourage making your code follow the same conventions as other Go code. People writing printf wrappers should use the standard names for them (printf, sprintf, fprintf, errorf, fatalf, fprintf, panicf). 

I certainly agree that you could implement extra analysis to help programmers that are not following standard conventions. However, it's unclear that such behavior should be encouraged. 

Russ

[Alan Donovan]

On 29 May 2013 17:32, Russ Cox <r...@golang.org> wrote:

Part of the reason interfaces are great is that they encourage making your code follow the same conventions as other Go code. People writing printf wrappers should use the standard names for them (printf, sprintf, fprintf, errorf, fatalf, fprintf, panicf). 

I certainly agree that you could implement extra analysis to help programmers that are not following standard conventions. However, it's unclear that such behavior should be encouraged. 

Perhaps you're right and that it's diminishing returns to solve the general case, but just within GOROOT I found the following functions that don't have an 'f' suffix, for example:

pkg/encoding/ascii85/ascii85_test.go:func testEqual(t *testing.T, msg string, args ...interface{}) bool {

pkg/encoding/base32/base32_test.go:func testEqual(t *testing.T, msg string, args ...interface{}) bool {

pkg/encoding/base64/base64_test.go:func testEqual(t *testing.T, msg string, args ...interface{}) bool {

pkg/encoding/gob/debug.go:func (deb *debugger) dump(format string, args ...interface{}) {

pkg/net/smtp/smtp.go:func (c *Client) cmd(expectCode int, format string, args ...interface{}) (int, string, error) {

pkg/net/textproto/textproto.go:func (c *Conn) Cmd(format string, args ...interface{}) (id uint, err error) {

pkg/net/textproto/writer.go:func (w *Writer) PrintfLine(format string, args ...interface{}) error {

pkg/runtime/softfloat64_test.go:func err(t *testing.T, format string, args ...interface{}) {

[Russ Cox]

On Wed, May 29, 2013 at 5:49 PM, Alan Donovan <adon...@google.com> wrote:

Perhaps you're right and that it's diminishing returns to solve the general case, but just within GOROOT I found the following functions that don't have an 'f' suffix, for example:

Okay, you've convinced me. But I think what I'd really like is for vet to handle the unexported ones automatically and to whine about the exported ones, so that it doesn't need to start assuming access to the source code beyond the type information you get from the imported compiled packages.

Russ

[Rob Pike]

define 'handle the imported ones automatically' and 'whine about the
exported ones'. in fact, define 'ones'.