Actual fix will be submitted to x/tools and vendored.
This is just an end-to-end test for vet after that is done.
Update #35264
Change-Id: I1a63f607e7cfa7aafee23c2c081086c276d3c38c
Reviewed-on: https://go-review.googlesource.com/c/go/+/204538
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
This pulls in the x/tools fix from
https://go-review.googlesource.com/c/tools/+/202041
so that cmd/vet won't flag %x/%X verbs incorrectly for floating-point
and complex types.
Fixes#34993
Change-Id: I68d89a19d95fe6ad336e87d12d56f03556974086
Reviewed-on: https://go-review.googlesource.com/c/go/+/202083
Run-TryBot: Caleb Spare <cespare@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
vet_test currently uses a custom GOPATH for each test, but it turns
out not to be necessary.
Updates #30228
Change-Id: Id7a7bf6d759bd94adccf44e197be1728c2f23575
Reviewed-on: https://go-review.googlesource.com/c/163038
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change deletes the legacy implementation of vet, replacing it
with a short main.go that merely selects the desired analyzers and
calls into the "unitchecker" implementation vendored from
golang.org/x/tools/go/analysis.
Unlike the full vet checker (x/tools/go/analysis/cmd/vet), the 'lite'
unitchecker cannot also be run standalone (as 'go tool vet' or
cmd/vet); it must be invoked by 'go vet'.
This design was chosen to avoid vendoring many
additional dependencies into GOROOT, in particular go/packages. If
go/packages should someday become part of the standard library, there
will be considerable opportunity for simplification.
This change also patches the vendored analysisflag package
(by adding patch.go) so that it fully supports the build
system's -V flag protocol.
Also:
- remove stale internal/unitchecker/ tree
(belonged in https://go-review.googlesource.com/c/149778).
- move vet legacy flags (-all, -v, -source, -tags) into analysisflags
as all drivers will need them, not just unitchecker.
I will upstream this change.
A sampling of tests from the cmd/vet testsuite have been preserved as
a smoke test, to ensure that each analyzer is being run, and for
convenience when evaluating changes. Comprehensive tests for each
analyzer live upstream in x/tools. The tests have been heavily reduced
and reorganized so that they conform to the structure required by 'go
vet'.
Change-Id: I84b38caeef733e65deb95234b3b87b5f61046def
Reviewed-on: https://go-review.googlesource.com/c/149609
Reviewed-by: Russ Cox <rsc@golang.org>
Now that vet can rely on go/types, there's no reason to do extra work to
avoid using it. The rewrite lets us get rid of the field list flattening
code, as well as the slight verbosity that comes with go/printer.
While at it, make the testdata/method.go expected errors be more
specific, to make sure that we're not breaking the warnings that are
printed.
Finally, update whitelist/all.txt, since the reported errors now include
qualified types.
Change-Id: I760a1b3b1f60e4a478c9dc43bd7f584a8459593e
Reviewed-on: https://go-review.googlesource.com/c/148919
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Alan Donovan <adonovan@google.com>
When main.main returns, the process exits, so there's no need to cancel contexts.
This change was initially reviewed as
https://go-review.googlesource.com/c/go/+/106915/4
but somehow I messed up and committed patchset 5, which was
effectively empty.
Change-Id: Ic4250eb6563af9bc734e429aafc7081ca7d0e012
Reviewed-on: https://go-review.googlesource.com/c/148758
Reviewed-by: Alan Donovan <adonovan@google.com>
Pointers to compound objects (structs, slices, arrays, maps) are only
followed by fmt if the pointer is at the top level of an argument. This
is to minimise the chances of fmt running into loops.
However, vet did not follow this rule. It likely doesn't help that fmt
does not document that restriction well, which is being tracked in
#28625.
Updates #27672.
Change-Id: Ie9bbd9b974eda5ab9a285986d207ef92fca4453e
Reviewed-on: https://go-review.googlesource.com/c/147997
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
fmt's godoc reads:
For compound objects, the elements are printed using these
rules, recursively, laid out like this:
struct: {field0 field1 ...}
array, slice: [elem0 elem1 ...]
maps: map[key1:value1 key2:value2 ...]
pointer to above: &{}, &[], &map[]
That is, a pointer to a struct, array, slice, or map, can be correctly
printed by fmt if the type pointed to can be printed without issues.
vet was only following this rule for pointers to structs, omitting
arrays, slices, and maps. Fix that, and add tests for all the
combinations.
Updates #27672.
Change-Id: Ie61ebe1fffc594184f7b24d7dbf72d7d5de78309
Reviewed-on: https://go-review.googlesource.com/c/147758
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Checks usage of Unmarshal and Decode functions in json, gob and
xml packages to detect attempts to decode into non-pointer types.
Fixes#27564
Change-Id: I07bbd5be82d61834ffde9af9937329d7fb1f05d0
Reviewed-on: https://go-review.googlesource.com/c/139997
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
For type switches using a short variable declaration of the form
switch t := x.(type) {
case T1:
...
go/types doesn't declare the symbolic variable (t in this example)
with the switch; thus such variables are not found in types.Info.Defs.
Instead they are implicitly declared with each type switch case,
and can be found in types.Info.Implicits.
Adjust the shadowing code accordingly.
Added a test case to verify that the issue is fixed, and a test
case verifying that the shadowing code now considers implicitly
declared variables introduces in type switch cases.
While at it, also fixed the (internal) error reporting to provide
more accurate information.
Fixe #26725.
Change-Id: If408ed9e692bf47c640f81de8f46bf5eb43415b0
Reviewed-on: https://go-review.googlesource.com/135117
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Alan Donovan <adonovan@google.com>
We can no longer use the field's position for the duplicate field tag
warning - since we now check embedded tags, the positions may belong to
copmletely different packages.
Instead, keep track of the lowest field that's still part of the
top-level struct type that we are checking.
Finally, be careful to not repeat the independent struct field warnings
when checking fields again because they are embedded into another
struct. To do this, separate the duplicate tag value logic into a func
that recurses into embedded fields on a per-encoding basis.
Fixes#25593.
Change-Id: I3bd6e01306d8ec63c0314d25e3136d5e067a9517
Reviewed-on: https://go-review.googlesource.com/115677
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
The code was fixed in CL 108559 but the testing TODO was not implemented.
Updates #22936
Change-Id: I20a703260a181bbcf5f87609d6fb8221a182be1a
Reviewed-on: https://go-review.googlesource.com/125038
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
This lets us simplify the code considerably. For example, unquoting the
tag is no longer necessary, and we can get the field name with a single
method call.
While at it, fix a typechecking error in testdata/structtag.go, which
hadn't been caught since vet still skips past go/types errors in most
cases.
Using go/types will also let us expand the structtag check more easily
if we want to, for example to allow it to check for duplicates in
embedded fields.
Finally, update one of the test cases to check for regressions when we
output invalid tag strings. We also checked that these two changes to
testdata/structtag.go didn't fail with the old structtag check.
For #25593.
Change-Id: Iea4906d0f30a67f36b28c21d8aa96251aae653f5
Reviewed-on: https://go-review.googlesource.com/115676
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Rob Pike <r@golang.org>
It is possible to write a function that seems to wrap a print/printf
call, but then doesn't. For example, if the string parameter we thought
was the format is used as another argument.
One option would be to make vet's print analysis smarter, to detect when
format strings are indeed used like we initially suspected.
However, I've opted for a simpler solution - check if the print/printf
call is already using more than one variadic argument, in which case
using an ellipsis in the last one would break the program:
// too many arguments in call to fmt.Printf
fmt.Printf(format, arg0, args...)
Fixes#26979.
Change-Id: I39371f1cec8483cfd2770a91670c1e80cbb9efdf
Reviewed-on: https://go-review.googlesource.com/129575
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Move badf helper into top-level function so that prints from buildtag.go
are once again themselves printf-format-checked by vet.
Also, fix implementation, which was missing a ... in the Sprintf call and
produced messages like:
/Users/rsc/x_test.go:1: +build comment must appear before package clause and be followed by a blank line%!(EXTRA []interface {}=[])
These were introduced in CL 111415.
Change-Id: I000af3a4e01dc99fc79c9146aa68a71dace1460f
Reviewed-on: https://go-review.googlesource.com/121300
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
This CL takes advantage of the ability to record vet-specific export data,
added in CL 108558, to save information about observed printf wrappers.
Then calls to those wrappers from other packages can be format-checked.
This found a few real mistakes using previously-unrecognized printf
wrappers in cmd/compile. It will no doubt find real mistakes in external code.
Change-Id: I9c29c92d89bbdc984571a174a96e6054585e9cd4
Reviewed-on: https://go-review.googlesource.com/108559
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
vet's buildtag check looks for malformed build tag comments. Since these
can appear in Go files as well as non-Go files (such as assembly files),
it must read the file line by line instead of using go/token or go/ast
directly.
However, this method runs into false positives if there are any lines in
the code that look like comments, but are not. For example:
$ cat f.go
package main
const foo = `
//+build ignore
`
$ go vet f.go
./f.go:3: +build comment must appear before package clause and be followed by a blank line
This bug has been popping up more frequently since vet started being run
with go test, so it is important to make the check as precise as
possible.
To avoid the false positive, when checking a Go file, cross-check that a
line that looks like a comment actually corresponds to a comment in the
go/ast syntax tree. Since vet already obtains the syntax trees for all
the Go files, it checks, this change means very little extra work for
the check.
While at it, add a badf helper function to simplify the code that
reports warnings in the buildtag check.
Fixes#13533.
Change-Id: I484a16da01363b409ec418c313634171bf85250b
Reviewed-on: https://go-review.googlesource.com/111415
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This change uses errorCheck and wantedErrors functions copied from
the test/run.go to eliminate use of the test/errchk perl script.
Tests' error messages that contained full filenames were changed to
have base filenames because the errorCheck function processes output
from "go vet" in the same way.
Fixes#20032.
Change-Id: Ieb7be67c2d7281b9648171c698398449b7e2d4dd
Reviewed-on: https://go-review.googlesource.com/114176
Run-TryBot: Yury Smolsky <yury@smolsky.by>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
That was the intention with the existing code, but it was buggy; builtin
functions aren't treated as values by types.TypeAndVal. Thus, we should
use the IsBuiltin method instead of IsValue.
Teaching vet what builtin funcs are pure is already being tracked as a
separate issue, #22851.
While at it, also add a test with methods, just to be sure that the
current logic doesn't break with that edge case either.
Fixes#25303.
Change-Id: Ic18402b22cceeabf76641c02f575b194b9a536cc
Reviewed-on: https://go-review.googlesource.com/112177
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
In hasSideEffects, vet has to be taught whether or not a CallExpr is an
actual function call, or just a type conversion.
The previous code knew how to differentiate fn(arg) from int(arg), but
it incorrectly saw (func(T))(fn) as a func call. This edge case is
slightly tricky, since the CallExpr.Fun has a func signature type, just
like in func calls.
However, the difference is that in this case the Fun is a type, not a
value. This information is in types.TypeAndValue, so use it.
Change-Id: I18bb8b23abbe7decc558b726ff2dc31fae2f13d6
Reviewed-on: https://go-review.googlesource.com/111416
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Some warnings were being missed, because vet's regex that finds
formatting directives was missing the 'T' verb.
Fixes#24646.
Change-Id: I2f6f9ed19e7daf9a07175199f428a62e94799ea9
Reviewed-on: https://go-review.googlesource.com/111357
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Rob Pike <r@golang.org>
fmt's %d, %x, and %X all accept pointer arguments. However, in cmd/vet's
printVerbs table, they were defined as if they did not accept pointer
arguments.
This inconsistency with fmt did not manifest to users since the vet
codebase worked around it. In particular, pointer arguments were usually
allowed for verbs that accepted integers, as the *types.Pointer argument
type case read the following:
t&(argInt|argPointer) != 0
As a result, using the %q verb with a pointer resulted in a bug in
cmd/vet:
$ go run f.go
%!q(*int=0xc000014140)
$ go vet f.go
[no warning]
As documented, fmt's %q verb only accepts runes (integers), strings, and
byte slices. It should not accept pointers, and it does not. But since
vet mixed integers and pointers, it wasn't properly warning about the
misuse of fmt.
This patch surfaced another bug with fmt.Printf("%p", nil):
$ go run f.go
%!p(<nil>)
$ go vet f.go
[no warning]
As documented, fmt's %p verb only accepts pointers, and untyped nil is
not a valid pointer. But vet did not warn about it, which is another
inconsistency with fmt's documented rules. Fix that too, with a test,
also getting rid of the TODO associated with the code.
As a result of those changes, fix a wrong use of the fmt format verbs in
the standard library, now correctly spotted by vet.
Fixes#25233.
Change-Id: Id0ad31fbc25adfe1c46c6b6879b8d02b23633b3a
Reviewed-on: https://go-review.googlesource.com/111284
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
If the vetted function supplies zero arguments, previously you would
get an error message like this:
Printf format %v reads arg #1, but call has only 0 args
"has only 0 args" is an odd construction, and "has 0 args" sounds
better. Getting rid of "only" in all cases simplifies the code and
reads just as well.
Change-Id: I4706dfe4a75f13bf4db9c0650e459ca676710752
Reviewed-on: https://go-review.googlesource.com/109457
Run-TryBot: Kevin Burke <kev@inburke.com>
Run-TryBot: David Symonds <dsymonds@golang.org>
Reviewed-by: David Symonds <dsymonds@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
A switch statement without a tag requires case values to be bools, but
the parser does not enforce that, so AST-walking code needs to take
care.
Change-Id: I7d9abbb0324314e02a37813c2d2f6adb0d6af5e7
Reviewed-on: https://go-review.googlesource.com/107375
Reviewed-by: Rob Pike <r@golang.org>
The go/printer (and thus gofmt) uses a heuristic to determine
whether to break alignment between elements of an expression
list which is spread across multiple lines. The heuristic only
kicked in if the entry sizes (character length) was above a
certain threshold (20) and the ratio between the previous and
current entry size was above a certain value (4).
This heuristic worked reasonably most of the time, but also
led to unfortunate breaks in many cases where a single entry
was suddenly much smaller (or larger) then the previous one.
The behavior of gofmt was sufficiently mysterious in some of
these situations that many issues were filed against it.
The simplest solution to address this problem is to remove
the heuristic altogether and have a programmer introduce
empty lines to force different alignments if it improves
readability. The problem with that approach is that the
places where it really matters, very long tables with many
(hundreds, or more) entries, may be machine-generated and
not "post-processed" by a human (e.g., unicode/utf8/tables.go).
If a single one of those entries is overlong, the result
would be that the alignment would force all comments or
values in key:value pairs to be adjusted to that overlong
value, making the table hard to read (e.g., that entry may
not even be visible on screen and all other entries seem
spaced out too wide).
Instead, we opted for a slightly improved heuristic that
behaves much better for "normal", human-written code.
1) The threshold is increased from 20 to 40. This disables
the heuristic for many common cases yet even if the alignment
is not "ideal", 40 is not that many characters per line with
todays screens, making it very likely that the entire line
remains "visible" in an editor.
2) Changed the heuristic to not simply look at the size ratio
between current and previous line, but instead considering the
geometric mean of the sizes of the previous (aligned) lines.
This emphasizes the "overall picture" of the previous lines,
rather than a single one (which might be an outlier).
3) Changed the ratio from 4 to 2.5. Now that we ignore sizes
below 40, a ratio of 4 would mean that a new entry would have
to be 4 times bigger (160) or smaller (10) before alignment
would be broken. A ratio of 2.5 seems more sensible.
Applied updated gofmt to all of src and misc. Also tested
against several former issues that complained about this
and verified that the output for the given examples is
satisfactory (added respective test cases).
Some of the files changed because they were not gofmt-ed
in the first place.
For #644.
For #7335.
For #10392.
(and probably more related issues)
Fixes#22852.
Change-Id: I5e48b3d3b157a5cf2d649833b7297b33f43a6f6e
Make the hasSideEffects func use type information to see if a CallExpr
is a type conversion or not. In case it is, there cannot be any side
effects.
Now that vet always has type information, we can afford to use it here.
Update the tests and remove the TODO there too.
Change-Id: I74fdacf830aedf2371e67ba833802c414178caf1
Reviewed-on: https://go-review.googlesource.com/79536
Reviewed-by: Robert Griesemer <gri@golang.org>
Simply checking if a name is "atomic" isn't enough, as that might be a
var or another imported package. Now that vet requires type information,
we can do better. And add a simple regression test.
Change-Id: Ibd2004428374e3628cd3cd0ffb5f37cedaf448ea
Reviewed-on: https://go-review.googlesource.com/91795
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Robert Griesemer <gri@golang.org>
We did warn on them in some cases, but not others. In particular, if one
used a slice composite literal with struct pointer elements, and omitted
the type of an element's composite literal, it would not get any warning
even if it should get one.
The issue is that typ.Underlying() can be of type *types.Pointer. Skip
those levels of indirection before checking for a *types.Struct
underlying type.
isLocalType also needed a bit of tweaking to ignore dereferences.
Perhaps that can be rewritten now that we have type info, but let's
leave it for another time.
Fixes#23539.
Change-Id: I727a497284df1325b70d47a756519f5db1add25d
Reviewed-on: https://go-review.googlesource.com/89715
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
For example, the following program is valid:
type T struct {
f interface{}
}
func main() {
fmt.Printf("%s", T{"foo"}) // prints {foo}
}
Since the field is of type interface{}, we might have any value in it.
For example, if we had T{3}, fmt would complain. However, not knowing
what the type under the interface is, we must be conservative.
However, as shown in #17798, we should issue an error if the field's
type is statically known to implement the error or fmt.Stringer
interfaces. In those cases, the user likely wanted the %s format to call
those methods. Keep the vet error in those cases.
While at it, add more field type test cases, such as custom error types,
and interfaces that extend the error interface.
Fixes#23563.
Change-Id: I063885955555917c59da000391b603f0d6dce432
Reviewed-on: https://go-review.googlesource.com/90516
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
For example, the following program is valid:
func main() {
fmt.Printf("%[1]d", 1, 2, 3)
}
If any of the formats are indexed, fmt will not complain about unused
extra arguments. See #22867 for more detail.
Make vet follow the same logic, to avoid erroring on programs that would
run without fmt complaining.
Fixes#23564.
Change-Id: Ic9dede5d4c37d1cd4fa24714216944897b5bb7cc
Reviewed-on: https://go-review.googlesource.com/90495
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
vet was quiet for []stringer, but not for [N]stringer. The source of the
problem was how the recursive call used .Elem().Underlying() for arrays,
but .Elem() for slices. In the first case, the named type is dropped,
thus losing all information of attached methods.
Be consistent across slices and arrays, by dropping the Underlying call
that is causing trouble. Add regression tests too, including cases where
the element type does not implement fmt.Stringer.
Fixes#23552.
Change-Id: I0fde07d101f112d5768be0a79207ef0b3dc45f2e
Reviewed-on: https://go-review.googlesource.com/90455
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
vet recorded what types had String methods defined on them, but it did
not record whether the receivers were pointer types. That information is
important, as the following program is valid:
type T string
func (t *T) String() string {
return fmt.Sprint(&t) // prints address
}
Teach vet that, if *T is Stringer, **T is not.
Fixes#23550.
Change-Id: I1062e60e6d82e789af9cca396546db6bfc3541e8
Reviewed-on: https://go-review.googlesource.com/90417
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
The problem is that vet complains about 0 as a Printf flag in some
situations where fmt allows it but probably shouldn't. The two
need to be brought in line, but it's too late in the release cycle.
The situation is messy and should be resolved properly in 1.11. This
CL is a simple fix to disable a spurious complaint for 1.10 that will be
resolved in a more thorough way in 1.11.
The workaround is just to be silent about flag 0, as suggested in
issue 23605.
Fixes#23605
Update #23498
Change-Id: Ice1a4f4d86845d70c1340a0a6430d74e5de9afd4
Reviewed-on: https://go-review.googlesource.com/90695
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
The name-based heuristics fail too often to be on during "go test",
but we really want the printf vet check in "go test", so change to
a list of exactly which standard library functions are print-like.
For a later release we'd like to bring back checking for user-defined
wrappers, but in a completely precise way. Not for Go 1.10, though.
The new, more precise list includes t.Skipf, which caught some
mistakes in standard library tests.
Fixes#22936.
Change-Id: I110448e3f6b75afd4327cf87b6abb4cc2021fd0d
Reviewed-on: https://go-review.googlesource.com/83838
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Values must not be copied after the first use.
Using noCopy makes vet complain about copies
even before the first use, which is incorrect
and very frustrating.
Drop it.
Fixes#21504.
Change-Id: Icd3a5ac3fe11e84525b998e848ed18a5d996f45a
Reviewed-on: https://go-review.googlesource.com/80836
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
In golang.org/cl/74352, the print rules were overhauled to give better
error messages. This also meant adding a regex to find and extract the
used formatting verbs.
However, %v was missed. Add it to the expression, and add a test too.
Fixes#22847.
Change-Id: If117cc364db0cb91373742239b8a626c137642b0
Reviewed-on: https://go-review.googlesource.com/79455
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The old code only worked for double-quoted strings, and only checked
that the end of the literal value was \n". This worked most of the time,
except for some strings like "foo\\n", which doesn't actually translate
into a trailing newline when unquoted.
To fix this, unquote the string first and look for a real newline at the
end of it. Ignore errors, as we don't have anything to do with string
literals using back quotes.
Fixes#22613.
Change-Id: I7cf96916dd578b7068216c2051ec2622cce0b740
Reviewed-on: https://go-review.googlesource.com/76194
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Every time I see an error that begins `missing argument for Fprintf("%s")`
my mental type-checker goes off, since obviously "%s" is not a valid first
argument to Fprintf. Writing Printf("%s") to report an error in Printf("hello %s")
is almost as confusing.
This CL rewords the errors reported by vet's printf check to be more
consistent with each other, avoid placing context like "in printf call"
in the middle of the message, and to avoid the imprecisions above by
not quoting the format string at all.
Before:
bad.go:9: no formatting directive in Printf call
bad.go:10: missing argument for Printf("%s"): format reads arg 1, have only 0 args
bad.go:11: wrong number of args for format in Printf call: 1 needed but 2 args
bad.go:12: bad syntax for printf argument index: [1]
bad.go:13: index value [0] for Printf("%[0]s"); indexes start at 1
bad.go:14: missing argument for Printf("%[2]s"): format reads arg 2, have only 1 args
bad.go:15: bad syntax for printf argument index: [abc]
bad.go:16: unrecognized printf verb 'z'
bad.go:17: arg "hello" for * in printf format not of type int
bad.go:18: arg fmt.Sprint in printf call is a function value, not a function call
bad.go:19: arg fmt.Sprint in Print call is a function value, not a function call
bad.go:20: arg "world" for printf verb %d of wrong type: string
bad.go:21: missing argument for Printf("%q"): format reads arg 2, have only 1 args
bad.go:22: first argument to Print is os.Stderr
bad.go:23: Println call ends with newline
bad.go:32: arg r in Sprint call causes recursive call to String method
bad.go:34: arg r for printf causes recursive call to String method
After:
bad.go:9: Printf call has arguments but no formatting directives
bad.go:10: Printf format %s reads arg #1, but have only 0 args
bad.go:11: Printf call needs 1 args but has 2 args
bad.go:12: Printf format %[1 is missing closing ]
bad.go:13: Printf format has invalid argument index [0]
bad.go:14: Printf format has invalid argument index [2]
bad.go:15: Printf format has invalid argument index [abc]
bad.go:16: Printf format %.234z has unknown verb z
bad.go:17: Printf format %.*s uses non-int "hello" as argument of *
bad.go:18: Printf format %s arg fmt.Sprint is a func value, not called
bad.go:19: Print arg fmt.Sprint is a func value, not called
bad.go:20: Printf format %d has arg "world" of wrong type string
bad.go:21: Printf format %q reads arg #2, but have only 1 args
bad.go:22: Print does not take io.Writer but has first arg os.Stderr
bad.go:23: Println args end with redundant newline
bad.go:32: Sprint arg r causes recursive call to String method
bad.go:34: Sprintf format %s with arg r causes recursive String method call
Change-Id: I5719f0fb9f2cd84df8ad4c7754ab9b79c691b060
Reviewed-on: https://go-review.googlesource.com/74352
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
The existing logic for whether the left and right parts of an assignment
were equal only checked that the gofmt representation of the two was
equal. This only checks that the ASTs were equal.
However, that method is flawed. For example, if either of the
expressions contains a function call, the expressions may actually be
different even if their ASTs are the same. An obvious case is a func
call to math/rand to get a random integer, such as the one added in the
test.
If either of the expressions may have side effects, simply skip the
check. Reuse the logic from bool.go's hasSideEffects.
Fixes#22174.
Change-Id: Ied7f7543dc2bb8852e817230756c6d23bc801d90
Reviewed-on: https://go-review.googlesource.com/69116
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
The change in https://golang.org/cl/43295 added warning about spaces in
struct tags. However, in XML tags it is expected that there will be a
space between the namespace and the local name.
Change-Id: Ic31c3bdae30797f406f25c737b83bbe2de1ed1db
Reviewed-on: https://go-review.googlesource.com/62570
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
+ Test
Change-Id: I7b42ff70f26a58e1cf58cbbc53d02a65623456ae
Reviewed-on: https://go-review.googlesource.com/62371
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
According to CL 31817, fmt cannot invoke String or Error methods
on unexported struct fields.
Fixes#17798.
Change-Id: I0d516577298bc36daa9a94313c3874d64dc079e6
Reviewed-on: https://go-review.googlesource.com/44831
Reviewed-by: Rob Pike <r@golang.org>
Keith Randall
Caleb Spare
Bryan C. Mills
Alan Donovan
Daniel Martí
Brad Fitzpatrick
Ian Davis
Igor Zhilianin
Robert Griesemer
Ian Lance Taylor
Russ Cox
Yury Smolsky
Kevin Burke
David Symonds
Austin Clements
Rob Pike
Chris Hines
Wei Congrui
Sam Whited
Aliaksandr Valialkin