kalbasit / arcanist-go

Phabricator's Arcanist Golang support.
Apache License 2.0
19 stars 10 forks source link

Issue with `go vet` false positives #8

Open derekargueta opened 5 years ago

derekargueta commented 5 years ago

It appears there's a settle difference between go vet and go tool vet - go tool vet can resolve variables that may have been declared in a different file, whereas go vet won't. go vet appears to have the intention of being executed on a package level, not a file level.

Here's a simple example - I have a variable defined in a.go that I use in b.go. Running go vet on b.go results in an "undefined" error, but running go vet on the package is perfectly fine:

➜  demo cat a.go
package main

const foobar string = "hi"
➜  demo cat b.go
package main

import "fmt"

func doSomething() {
  fmt.Println(foobar)
}
➜  demo go vet b.go
# command-line-arguments [command-line-arguments.test]
./b.go:6:15: undefined: foobar
➜  demo go vet .
➜  demo

I don't have a great workaround for Go 1.12 yet (maybe lint the package but try to avoid re-linting the package by keeping track of what files have been checked? And only show vet errors for the changed files?). Perhaps in the meantime we can have a patch that if Go is < 1.12, continue to use go tool vet.

Thoughts @kalbasit ?

Related links: https://github.com/golang/go/issues/23916 https://github.com/w0rp/ale/issues/1358

kalbasit commented 5 years ago

I have not done Arcanist for a while, so I'm not sure how to fix it. It's logical that it operates on the package rather than the file itself. The question is: Can Arcanist pass in a directory containing the file to lint instead of the file itself? If so, this should be an easy fix.