go tool vet or "go vet" help detect functional problems in Golang
code. It is useful in addition to golint, which checks for style violations.
Details
Details
- Reviewers
joshuaspence sectioneight - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T6867: Add linter and unit test engine for Go(lang)
- Required Signatures
L28 Phacility Individual Contributor License Agreement
Ran manually on one of my personal repos:
>>> Lint for test.go:
Warning (GOVET)
Possible formatting directive in Println call
3 import "fmt"
4
5 func main() {
>>> 6 fmt.Println("%d", 1)
7 }Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- T6867-go-vet
- Lint
Lint Passed - Unit
Test Failures - Build Status
Buildable 4933 Build 5500: [Placeholder Plan] Wait for 30 Seconds Build 4951: [Placeholder Plan] Wait for 30 Seconds
| Time | Test | |
|---|---|---|
| 31 ms | ArcanistGoVetLinterTestCase::testLinter | |
| 366 ms | ArcanistJscsLinterTestCase::testLinter | |
| 35 ms | ArcanistLibraryTestCase::testLibraryMap | |
| 518 ms | ArcanistCSSLintLinterTestCase::testLinter | |
| 87 ms | ArcanistCSSLintLinterTestCase::testVersion | |
| View Full Test Results (3 Failed · 38 Passed · 15 Skipped) |
Event Timeline
Comment Actions
I also need to detect when the tool is not installed and output the proper warning. Unfortunately it's not as simple as a path check, since it's executed as go tool vet <file> and having go in the path is not enough.
Is the proper strategy to parse the STDERR in parseLinterOutput and throw a new ArcanistMissingLinterException if it matches the proper output?
Comment Actions
I don't know if this is useful, or how to get the tests pass, but I like the functionality of go vet and I like phabricator, so maybe the two can be friends?
Comment Actions
- Add optional argument 'tempFileSuffix' to ArcanistLinterTestCase->executeTestsInDirectory(), this argument is sent as is to ArcanistLinterTestCase->lintFile.
- Add optional argument 'tempFileSuffix' to ArcanistLinterTestCase->lintFile, if present, the temp file created for the test will be given the 'tempFileSuffix' as extension.
Comment Actions
The class now correcly detects the absence of 'go tool vet' by running it and inspecting the exit status.
Reference (of exit status 3): https://github.com/golang/go/blob/5f8423844463b3b77588e46ea57f44d8b69a1564/src/cmd/go/tool.go#L56-L64
Comment Actions
This is great, thanks for getting the tests passing!
| src/lint/linter/__tests__/ArcanistLinterTestCase.php | ||
|---|---|---|
| 54 ↗ | (On Diff #30106) | This might fit better in the $config section below, which is intended to be a flexible JSON hash already, and would allow other tests to opt-in on a per-file basis. I'll let the blessed reviewers decide though :) |
| src/lint/linter/ArcanistGoVetLinter.php | ||
|---|---|---|
| 7 | Maybe Go Vet | |
| 28–47 | There's no nice way of handling this... I'd just return go as the default binary. | |
| 51 | This would be better written as pht('Install Go vet using `%s`.', 'go get golang.org/x/tools/cmd/vet'). | |
| 54–60 | These are the default values, just omit these methods. | |
| 63 | This is PHP 5.4 syntax but we need to be compatible with PHP 5.2, so you'll need to change this to array('tool', 'vet') | |
| 79 | You should maybe use $this->getLintMessageSeverity($code) here to allow the severity to be configurable. | |
| src/lint/linter/__tests__/ArcanistLinterTestCase.php | ||
| 84 ↗ | (On Diff #30123) | We don't want this change... the linter (go vet in this case) should lint whatever path is explicitly passed to it, regardless of the file extension. |
Comment Actions
PTAL
| src/lint/linter/ArcanistGoVetLinter.php | ||
|---|---|---|
| 28–47 | However the presence of the Go binary does not indicate if GoVet is installed or not. Unfortunately, ArcanistExternalLinter::checkBinaryConfiguration is final so I can't overload it. | |
| 79 | So GoVet does not report errors, it only report a possibility of a problem, for instance, running go tool vet on the include test src/lint/linter/__tests__/govet/fmt.lint-test reports /tmp/test.go:6: possible formatting directive in Println call which may or may not be a real problem. Given that, I think we will never report errors but only warnings. Should I still make it configurable? | |
| src/lint/linter/__tests__/ArcanistLinterTestCase.php | ||
| 84 ↗ | (On Diff #30123) | Unfortunately it does not work. Go vet expects the file to end with .go $ go tool vet src/lint/linter/__tests__/govet/fmt.lint-test vet: no files checked |
| src/lint/linter/ArcanistGoVetLinter.php | ||
|---|---|---|
| 28–47 | I installed golang-go on Ubuntu 15 (via apt-get) and it included go tool vet... when would it not be bundled? Is it common for it to not be bundled? In any case, what would happen if getDefaultBinary() returned go and go was installed but not go tool vet? How would it fail? | |
| 79 | I think it should be configurable. What you think is a warning, others may say is advice. | |
| src/lint/linter/__tests__/ArcanistLinterTestCase.php | ||
| 84 ↗ | (On Diff #30556) | If that's the case then I don't think that this linter should be upstreamed just yet. I don't like adding this functionality for only one subclass. Furthermore, I find this behavior to be counterintuitive. If my .arclint file says to lint *.txt files with ArcanistGoVetLinter, it would not be obvious why this wouldn't be working. I think that if we did want this functionality then we should add a getSupportedFileExtensions() method to ArcanistLinter. |
| src/lint/linter/ArcanistGoVetLinter.php | ||
|---|---|---|
| 28–47 | Perhaps it is included by default for Ubuntu (or whichever distro you're on) but AFAIK it is not bundled with the official distribution. If it's not found go tool vet ... will fail with an error message and an exit code 3 $ go tool vet /tmp/test.go; echo $? go tool: no such tool "vet"; to install: go get golang.org/x/tools/cmd/vet 3 The actual linter will not fail but it will print an error for every file: Warning (GOVET)
>>> 1 package main
2
3 import (
4 "flag"
>>> Lint for model/source.go:Not sure where the 'package main' came from, it should be model, but in any case, it fails in an unwanted behavior. | |
| 79 | Alright I'll get it done tonight and update the differential. | |
| src/lint/linter/__tests__/ArcanistLinterTestCase.php | ||
| 84 ↗ | (On Diff #30556) | Blessed Reviewers thoughts on this? |
Comment Actions
@ryansking it looks like this is going to have to wait Blessed Reviewers planning. In the meantime you can use this and all of my Go stuff from https://github.com/kalbasit/arcanist-go