code review 7102050: cmd/vet: don't complain about Error()

cmd/vet: don't complain about Error()
Fixes issue 4598.

[minux1, 2013-01-12 00:47:19]

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

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

[dsymonds, 2013-01-12 01:33:58]

Add a test case?

[r, 2013-01-12 03:45:16]

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

https://codereview.appspot.com/7102050/diff/5001/src/cmd/vet/print.go#newcode256
src/cmd/vet/print.go:256: if *verbose && !isLn && name != "Error" {
this isn't the right fix but will almost always work correctly because calling
Error() with no arguments in any other context is just bizarre. still, add:

// TODO: check that the receiver is of type error.

[minux1, 2013-01-12 08:23:13]

On Sat, Jan 12, 2013 at 9:33 AM, David Symonds <dsymonds@golang.org> wrote:

> Add a test case?
>
Can we make vet output this warning by default?
then I can use existing errchk to write a test case for it.

Or alternatively, we add a flag to suppress the output the "Checking file
somefile.go"?

[minux1, 2013-01-12 08:29:15]

PTAL.

On Sat, Jan 12, 2013 at 11:45 AM, <r@golang.org> wrote:

> https://codereview.appspot.**com/7102050/diff/5001/src/cmd/**
>
vet/print.go#newcode256<https://codereview.appspot.com/7102050/diff/5001/src/cmd/vet/print.go#newcode256>
> src/cmd/vet/print.go:256: if *verbose && !isLn && name != "Error" {
> this isn't the right fix but will almost always work correctly because
>
I agree. I originally think we can delay fixing this until go/types is
ready, however
as Russ pointed out, err.Error() is too common a pattern to ignore.

> calling Error() with no arguments in any other context is just bizarre.
> still, add:
>
> // TODO: check that the receiver is of type error.
>
Done.

I will re-open the issue after submitting and change its summary to
fix this TODO.

[dsymonds, 2013-01-12 08:43:24]

On Sat, Jan 12, 2013 at 7:22 PM, minux <minux.ma@gmail.com> wrote:

> On Sat, Jan 12, 2013 at 9:33 AM, David Symonds <dsymonds@golang.org> wrote:
>>
>> Add a test case?
>
> Can we make vet output this warning by default?
> then I can use existing errchk to write a test case for it.
>
> Or alternatively, we add a flag to suppress the output the "Checking file
> somefile.go"?

Aah, I missed it was conditioned on *verbose. Don't worry about it then.

[minux1, 2013-01-17 10:46:11]

ping?

[r, 2013-01-17 16:46:48]

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

https://codereview.appspot.com/7102050/diff/2002/src/cmd/vet/print.go#newcode257
src/cmd/vet/print.go:257: if *verbose && !isLn && name != "Error" {
the *verbose on line 96 misuses the flag, and so does this.
-v is a shortcut for enabling all the flags. it's a bad idea to also make it be
more chatty because it means i can't ask for printf-checking alone and get all
the output.

please flag line 96 as 'TODO: incorrect use of verbose flag' and delete the
reference to it here. that will make it always print and it's then easy to write
a test.

in a separate CL we can sort out line 96, although i'd LGTM a CL to delete the
condition right now.

[r, 2013-01-17 16:52:53]

disregard previous mail. i conflated -all and -v.

let's try again.

i'm happy with dropping the verbose check here and thereby enabling a test.

[minux1, 2013-01-17 17:05:50]

PTAL.
Removed the *verbose guard for this check and added a
test case.

[r, 2013-01-17 18:20:45]

LGTM

[minux1, 2013-01-17 18:24:26]

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

cmd/vet: don't complain about Error()
Fixes issue 4598.

R=golang-dev, dsymonds, r
CC=golang-dev
https://codereview.appspot.com/7102050