changes to cmd/vet

[Alan Donovan]

The last 2+ months I've been reorganizing vet around the new golang.org/x/tools/go/analysis API, which enables modular static checkers to be expressed independent of any particular application, making it easy for anyone to write vet checks, build a vet-like tool from an a la carte menu of checks, and efficiently apply any vet-like tool to a code base using go vet or similar functionality in other build systems such as Bazel and Blaze, or from within other applications such as IDEs, code review tools, and batch pipelines.

This week I plan to replace cmd/vet with a copy of the driver at cmd/vendor/golang.org/x/tools/go/analysis/cmd/vet-lite. The logic of each checker is essentially unchanged; only the driver differs. My remaining tasks are:

- bring the vet fork up to date w.r.t. the few recent changes in cmd/vet

- ensure that cmd/vet command-line documentation is accurate

- update cmd/vendor, adding a script for this and future updates.

- change cmd/vet/all to use "go vet" instead of "go tool vet",

  and measure any performance impact on the builders

  This requires investigating all deviations in cmd/vet/all diagnostics.

  The main issue here is github.com/golang/go/issues/27665.

  Most of the other differences result from "go vet" being more precise than "go tool vet".

- replace cmd/vet with a copy of vet-lite's short main.go file.

At that point, "go vet" will use the new driver.

Let me know if you have questions/concerns.

cheers

alan

[Russ Cox]

On Mon, Nov 12, 2018 at 10:10 AM 'Alan Donovan' via golang-dev <golan...@googlegroups.com> wrote:

- change cmd/vet/all to use "go vet" instead of "go tool vet",

  and measure any performance impact on the builders

  This requires investigating all deviations in cmd/vet/all diagnostics.

  The main issue here is github.com/golang/go/issues/27665.

  Most of the other differences result from "go vet" being more precise than "go tool vet".

FWIW now that we have vet during go test I'd really like to throw away cmd/vet/all at some point. Probably not this week, but that's what we should be moving toward.

Russ

[Alan Donovan]

cmd/vet/all runs a bigger set of checks than the "curated list" run by vet during go test. But yes, it does seem like the the right direction.

 

[Brad Fitzpatrick]

cmd/vet/all also checks all platforms, which run during trybot time, which it was designed for. If we only have coverage via "go test", we lose presubmit coverage of most platforms.

[Daniel Martí]

On Mon, Nov 12, 2018 at 10:10:22 -0500, 'Alan Donovan' via golang-dev wrote:
> This week I plan to replace cmd/vet with a copy of the driver at cmd/vendor/
> golang.org/x/tools/go/analysis/cmd/vet-lite.

I'm curious; we already have vendor/golang_org/x/{crypto,net,text}, so
why not put the tools repo there too? I understand that we only need the
repository within cmd right now, but I assume we'd prefer all x repos
vendored in a single place following the same method.

[Russ Cox]

The counter-argument is that putting something in $GOROOT/src/vendor means the standard library may accidentally start using it. I don't want to put anything there that doesn't strictly need to be there. And this doesn't. (Similarly, pprof is not there.)

Russ

[Alan Donovan]

https://go-review.googlesource.com/c/go/+/149609 is now submitted. If you build GOROOT from tip, 'go vet' will use the new analysis infrastructure. Please try it out and let me know if it works (or not). 

Also, try writing a new checker. You might find golang.org/x/tools/go/analysis/passes/findcall helpful as an example: all it does is print calls to functions whose name matches its -name flag.  For example, these commands find all calls to recover in the standard library:

$ go install golang.org/x/tools/go/analysis/passes/findcall/cmd/findcall

$ go vet -vettool $GOPATH/bin/findcall -name=recover std

# bytes

bytes/buffer.go:228:13: call of recover(...)

# text/tabwriter

text/tabwriter/tabwriter.go:477:17: call of recover(...)

# fmt

fmt/print.go:542:19: call of recover(...)

fmt/scan.go:249:18: call of recover(...)

fmt/scan.go:1025:17: call of recover(...)

# context

context/context_test.go:635:28: call of recover(...)

...

cheers

alan

[ma...@influxdata.com]

I just built `go version devel +14608976db Thu Nov 15 23:37:20 2018 +0000 darwin/amd64`, and ran

    GO111MODULE=on gotip vet ./...

against my module-enabled project, inside a GOPATH. The output was many lines of

  http/auth_test.go:14:2: CRLF="\r\n" EMPTY="" ERR_OP="-ERR" INFO_OP="INFO" OK_OP="+OK" PONG_OP="PONG" PUB_P="PUB " SPC=" "

  http/auth_test.go:17:2: CRLF="\r\n" EMPTY="" ERR_OP="-ERR" INFO_OP="INFO" OK_OP="+OK" PONG_OP="PONG" PUB_P="PUB " SPC=" "

  http/bucket_test.go:16:2: CRLF="\r\n" EMPTY="" ERR_OP="-ERR" INFO_OP="INFO" OK_OP="+OK" PONG_OP="PONG" PUB_P="PUB " SPC=" "

  http/bucket_test.go:17:2: CRLF="\r\n" EMPTY="" ERR_OP="-ERR" INFO_OP="INFO" OK_OP="+OK" PONG_OP="PONG" PUB_P="PUB " SPC=" "

so it looks like something is being misprinted.

But I did see one properly printed error:

    http/dashboard_service.go:76:2: struct field Cells repeats json tag "cells" also at dashboard_service.go:168

https://github.com/influxdata/platform is the repository if you need a quick repro.

[Alan Donovan]

On 15 November 2018 at 19:09, <ma...@influxdata.com> wrote:

I just built `go version devel +14608976db Thu Nov 15 23:37:20 2018 +0000 darwin/amd64`, and ran

    GO111MODULE=on gotip vet ./...

against my module-enabled project, inside a GOPATH. The output was many lines of

  http/auth_test.go:14:2: CRLF="\r\n" EMPTY="" ERR_OP="-ERR" INFO_OP="INFO" OK_OP="+OK" PONG_OP="PONG" PUB_P="PUB " SPC=" "

Thanks for pointing that out. I left one of the debugging analyzers (pkgfact) in the production tool.

I will remove it presently: https://go-review.googlesource.com/c/tools/+/150039