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

go.tools/ssa: fix debug printing

[gri, 2013-05-25 16:03:59]

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

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

[r, 2013-05-25 16:27:14]

LGTM

[gri, 2013-05-25 16:52:07]

*** 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

[adonovan, 2013-05-25 18:25:55]

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.



On 25 May 2013 12:52, <gri@golang.org> wrote:

> *** Submitted as
>
https://code.google.com/p/go/**source/detail?r=7730491448d2&**repo=tools<http...
>
> go.tools/ssa: fix debug printing
>
> R=adonovan, r
> CC=golang-dev
>
https://codereview.appspot.**com/9774043<https://codereview.appspot.com/9774043>
>
>
>
https://codereview.appspot.**com/9774043/<https://codereview.appspot.com/9774...
>

[r, 2013-05-25 18:30:42]

does vet catch it?

-rob

[adonovan, 2013-05-29 16:22:02]

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.

[rsc, 2013-05-29 17:39:10]

vet should catch it (printing a method value) in Printf.
wrappers should be named appropriately (Printf, Logf, Fatalf) so that vet
catches them too.

[adonovan, 2013-05-29 17:51:25]

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



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

> vet should catch it (printing a method value) in Printf.
> wrappers should be named appropriately (Printf, Logf, Fatalf) so that vet
> catches them too.
>
>

[rsc, 2013-05-29 17:54:08]

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

[adonovan, 2013-05-29 17:55:52]

On 29 May 2013 13:54, Russ Cox <rsc@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.

[rsc, 2013-05-29 18:07:54]

On Wed, May 29, 2013 at 1:55 PM, Alan Donovan <adonovan@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

[adonovan, 2013-05-29 18:57:57]

On 29 May 2013 14:07, Russ Cox <rsc@golang.org> wrote:

> On Wed, May 29, 2013 at 1:55 PM, Alan Donovan <adonovan@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?
>

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.

[rsc, 2013-05-29 19:06:14]

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

[adonovan, 2013-05-29 20:28:28]

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.

On 29 May 2013 15:06, Russ Cox <rsc@golang.org> wrote:

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

[rsc, 2013-05-29 21:32:36]

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

[adonovan, 2013-05-29 21:49:30]

On 29 May 2013 17:32, Russ Cox <rsc@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{}) {

[rsc, 2013-05-30 00:18:06]

On Wed, May 29, 2013 at 5:49 PM, Alan Donovan <adonovan@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

[r, 2013-05-30 01:34:17]

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