Page MenuHomePhabricator

Add GoVet Linter
Needs RevisionPublic

Authored by eMxyzptlk on Mar 20 2015, 10:28 PM.

Details

Summary

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.

Test Plan

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

Repository
rARC Arcanist
Branch
govet-linter
Lint
Lint OK
Unit
Unit Test Errors
Build Status
Buildable 5770
Build 5789: [Placeholder Plan] Wait for 30 Seconds

Unit TestsFailed

Excuse: Both tests are also failing on master. See https://gist.github.com/eMxyzptlk/5bee57c5be119f0755d6
TimeTest
526 msArcanistJscsLinterTestCase::testLinter
27 msArcanistXMLLinterTestCase::testLinter
19 msArcanistChmodLinterTestCase::testLinter
0 msArcanistClosureLinterTestCase::testVersion
0 msArcanistCpplintLinterTestCase::testVersion
View Full Test Results (2 Failed · 32 Passed · 23 Skipped)

Event Timeline

sectioneight retitled this revision from to Add GoVet Linter.
sectioneight updated this object.
sectioneight edited the test plan for this revision. (Show Details)

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?

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?

eMxyzptlk edited edge metadata.
  • 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.
  • Lint the code following the automatic code review.

Removing the tab does not break the tests.

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

Revert an accidental change

This is great, thanks for getting the tests passing!

src/lint/linter/__tests__/ArcanistLinterTestCase.php
53

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 :)

eMxyzptlk edited edge metadata.
  • switch to using $config on a per-file basis

Thank you @sectioneight, it's done.

joshuaspence edited edge metadata.
joshuaspence added inline comments.
src/lint/linter/ArcanistGoVetLinter.php
6

Maybe Go Vet

27–46

There's no nice way of handling this... I'd just return go as the default binary.

50

This would be better written as pht('Install Go vet using `%s`.', 'go get golang.org/x/tools/cmd/vet').

53–59

These are the default values, just omit these methods.

62

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')

78

You should maybe use $this->getLintMessageSeverity($code) here to allow the severity to be configurable.

src/lint/linter/__tests__/ArcanistLinterTestCase.php
82

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.

This revision now requires changes to proceed.May 5 2015, 8:44 AM

Oh, you should implement getVersion as well.

eMxyzptlk edited edge metadata.
eMxyzptlk marked 4 inline comments as done.
eMxyzptlk edited edge metadata.
  • implement getVersion

PTAL

src/lint/linter/ArcanistGoVetLinter.php
27–46

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.

78

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
82

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
joshuaspence edited edge metadata.
joshuaspence added inline comments.
src/lint/linter/ArcanistGoVetLinter.php
27–46

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?

78

I think it should be configurable. What you think is a warning, others may say is advice.

src/lint/linter/__tests__/ArcanistLinterTestCase.php
84

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.

This revision now requires changes to proceed.May 5 2015, 9:53 PM
src/lint/linter/ArcanistGoVetLinter.php
27–46

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.

78

Alright I'll get it done tonight and update the differential.

src/lint/linter/__tests__/ArcanistLinterTestCase.php
84

Blessed Reviewers thoughts on this?

Any updates here. I would like to use this. :)

@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

jcox added a subscriber: jcox.