Repository: alecthomas/gometalinter (go)

Issue:
errcheck is called incorrectly and silently fails
I typically call `gometalinter` via `vim-go` which runs a command like this:

```
gometalinter --disable-all --enable=vet --enable=golint --enable=errcheck \
/home/vagrant/go/src/github.com/horgh/test
```

I noticed today after updating `gometalinter` that errors it normally would catch were not being reported. It appears there was a change to how it calls `errcheck` and the above command now fails the `errcheck` call.

If I revert to an earlier `gometalinter` commit, linting with `errcheck` works again.

I've updated using `gometalinter --install` as well.

I used `git bisect` and found the first commit with an issue is `849d41c4e4702571db3c9b8c646a7ff21d48bdc8`.

Here is debug output of the problem case (`b4ebfc554d8f36bfef1f180ad0aaaaac99b430d5`):

```
vagrant@ct100-test:~/go/src/github.com/horgh/test$ gometalinter --debug --disable-all --enable=vet --enable=golint --enable=errcheck /home/vagrant/go/src/github.com/horgh/test
DEBUG: PATH=/home/vagrant/bin:/home/vagrant/bin/ratpoison-git/src:/home/vagrant/go/bin:/home/vagrant/bin:/home/vagrant/bin/ratpoison-git/src:/home/vagrant/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/home/vagrant/.fzf/bin
DEBUG: GOPATH=/home/vagrant/go
DEBUG: GOBIN=
DEBUG: linting path /home/vagrant/go/src/github.com/horgh/test
DEBUG: executing /usr/bin/go tool vet /home/vagrant/go/src/github.com/horgh/test/test.go
DEBUG: executing /home/vagrant/go/bin/golint -min_confidence 0.800000 /home/vagrant/go/src/github.com/horgh/test
DEBUG: golint hits 0: ^(?P<path>.*?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: golint linter took 11.488242ms
DEBUG: executing /home/vagrant/go/bin/errcheck -abspath /home/vagrant/go/src/github.com/horgh/test
DEBUG: warning: /home/vagrant/go/bin/errcheck returned exit status 2: import "/home/vagrant/go/src/github.com/horgh/test": cannot import absolute path
error: failed to check packages: could not type check: no initial packages were loaded

DEBUG: errcheck hits 0: ^(?P<path>.*?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: errcheck linter took 9.964403ms
DEBUG: vet hits 0: ^(?:vet:.*?\.go:\s+(?P<path>.*?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*))|(?:(?P<path>.*?\.go):(?P<line>\d+):\s*(?P<message>.*))$
DEBUG: vet linter took 50.874178ms
DEBUG: total elapsed time 57.944781ms
```

Here is debug output of the working case (`1bcfd85`):

```
vagrant@ct100-test:~/go/src/github.com/horgh/test$ gometalinter --debug --disable-all --enable=vet --enable=golint --enable=errcheck /home/vagrant/go/src/github.com/horgh/test
DEBUG: PATH=/home/vagrant/bin:/home/vagrant/bin/ratpoison-git/src:/home/vagrant/go/bin:/home/vagrant/bin:/home/vagrant/bin/ratpoison-git/src:/home/vagrant/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/home/vagrant/.fzf/bin
DEBUG: GOPATH=/home/vagrant/go
DEBUG: GOBIN=
DEBUG: linting path /home/vagrant/go/src/github.com/horgh/test
DEBUG: linting with golint: golint -min_confidence {min_confidence} {path} (on /home/vagrant/go/src/github.com/horgh/test)
DEBUG: executing /home/vagrant/go/bin/golint ["-min_confidence" "0.800000" "."]
DEBUG: linting with errcheck: errcheck -abspath {path} (on /home/vagrant/go/src/github.com/horgh/test)
DEBUG: executing /home/vagrant/go/bin/errcheck ["-abspath" "."]
DEBUG: golint hits 0: ^(?P<path>.*?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: golint linter took 18.766998ms
DEBUG: linting with vet: go tool vet {path}/*.go (on /home/vagrant/go/src/github.com/horgh/test)
DEBUG: executing /usr/bin/go ["tool" "vet" "test.go"]
DEBUG: vet hits 0: ^(?:vet:.*?\.go:\s+(?P<path>.*?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*))|(?:(?P<path>.*?\.go):(?P<line>\d+):\s*(?P<message>.*))$
DEBUG: vet linter took 153.805345ms
DEBUG: warning: errcheck -abspath . returned exit status 2
DEBUG: errcheck hits 2: ^(?P<path>.*?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: errcheck linter took 656.594344ms
DEBUG: nolint: parsing /home/vagrant/go/src/github.com/horgh/test/test.go for directives
DEBUG: nolint: parsing /home/vagrant/go/src/github.com/horgh/test/test.go took 617.432µs
/home/vagrant/go/src/github.com/horgh/test/test.go:19:6:warning: error return value not checked (cannot convert 5 (untyped int constant) to string) (errcheck)
/home/vagrant/go/src/github.com/horgh/test/test.go:18:2:warning: error return value not checked (a declared but not used) (errcheck)
DEBUG: total elapsed time 672.846156ms
```

The silent failure seems related to #310. But I don't see an open issue for the change in the call to `errcheck`.

Thanks for your assistance!

Pull request:
Convert absolute paths to package paths for some linters
Fixes #318

Some linters don't accept absolute paths, so convert them to package paths.

Patch:
diff --git a/config.go b/config.go
index 4bc1c00..e9ce8e4 100644
--- a/config.go
+++ b/config.go
@@ -92,6 +92,24 @@ var (
 
 	linterTakesFilesGroupedByPackage = newStringSet("vet", "vetshadow")
 
+	linterTakesPackagePaths = newStringSet(
+		"errcheck",
+		"aligncheck",
+		"errcheck",
+		"gosimple",
+		"interfacer",
+		"megacheck",
+		"safesql",
+		"staticcheck",
+		"structcheck",
+		"test",
+		"testify",
+		"unconvert",
+		"unparam",
+		"unused",
+		"varcheck",
+	)
+
 	// Linter definitions.
 	linterDefinitions = map[string]string{
 		"aligncheck":  `aligncheck:^(?:[^:]+: )?(?P<path>.*?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.+)$`,
diff --git a/execute.go b/execute.go
index a306112..de22e4e 100644
--- a/execute.go
+++ b/execute.go
@@ -14,9 +14,8 @@ import (
 	"sync"
 	"time"
 
-	kingpin "gopkg.in/alecthomas/kingpin.v2"
-
 	"github.com/google/shlex"
+	"gopkg.in/alecthomas/kingpin.v3-unstable"
 )
 
 type Vars map[string]string
@@ -83,7 +82,10 @@ func (l *linterState) Partitions() ([][]string, error) {
 	if err != nil {
 		return nil, err
 	}
-	parts := l.Linter.partitionStrategy(cmdArgs, l.paths)
+	parts, err := l.Linter.partitionStrategy(cmdArgs, l.paths)
+	if err != nil {
+		return nil, err
+	}
 	if len(parts) == 0 {
 		return nil, fmt.Errorf("%s: no files to lint", l.Name)
 	}
diff --git a/linters.go b/linters.go
index bd19e0c..366b056 100644
--- a/linters.go
+++ b/linters.go
@@ -57,7 +57,7 @@ func LinterFromName(name string) *Linter {
 		SeverityOverride:  Severity(config.Severity[name]),
 		MessageOverride:   config.MessageOverride[name],
 		regex:             re,
-		partitionStrategy: getParitionStrategy(name),
+		partitionStrategy: getPartitionStrategy(name),
 	}
 }
 
diff --git a/main.go b/main.go
index 3fa755d..62df2e6 100644
--- a/main.go
+++ b/main.go
@@ -392,7 +392,7 @@ func replaceWithMegacheck(enabled []string) []string {
 }
 
 func findVendoredLinters() string {
-	gopaths := strings.Split(getGoPath(), string(os.PathListSeparator))
+	gopaths := getGoPathList()
 	for _, home := range vendoredSearchPaths {
 		for _, p := range gopaths {
 			joined := append([]string{p, "src"}, home...)
@@ -416,6 +416,10 @@ func getGoPath() string {
 	return path
 }
 
+func getGoPathList() []string {
+	return strings.Split(getGoPath(), string(os.PathListSeparator))
+}
+
 // addPath appends p to paths and returns it if:
 // 1. p is not a blank string
 // 2. p doesn't already exist in paths
@@ -434,7 +438,7 @@ func addPath(p string, paths []string) []string {
 
 // Ensure all "bin" directories from GOPATH exists in PATH, as well as GOBIN if set.
 func configureEnvironment() {
-	gopaths := strings.Split(getGoPath(), string(os.PathListSeparator))
+	gopaths := getGoPathList()
 	paths := strings.Split(os.Getenv("PATH"), string(os.PathListSeparator))
 	gobin := os.Getenv("GOBIN")
 
diff --git a/parition.go b/parition.go
deleted file mode 100644
index a58e10e..0000000
--- a/parition.go
+++ /dev/null
@@ -1,100 +0,0 @@
-package main
-
-import (
-	"path/filepath"
-)
-
-// MaxCommandBytes is the maximum number of bytes used when executing a command
-const MaxCommandBytes = 32000
-
-type partitionStrategy func([]string, []string) [][]string
-
-func getParitionStrategy(name string) partitionStrategy {
-	switch {
-	case linterTakesFiles.contains(name):
-		return partitionToMaxArgSizeWithFileGlobs
-	case linterTakesFilesGroupedByPackage.contains(name):
-		return partitionToPackageFileGlobs
-	}
-	return partitionToMaxArgSize
-}
-
-func packagesToFileGlobs(paths []string) []string {
-	filePaths := []string{}
-	for _, dir := range paths {
-		// ignore error because the glob pattern is hardcoded
-		paths, _ := filepath.Glob(filepath.Join(dir, "*.go"))
-		filePaths = append(filePaths, paths...)
-	}
-	return filePaths
-}
-
-func partitionToMaxArgSize(cmdArgs []string, paths []string) [][]string {
-	return partitionToMaxSize(cmdArgs, paths, MaxCommandBytes)
-}
-
-func partitionToMaxSize(cmdArgs []string, paths []string, maxSize int) [][]string {
-	partitions := newSizeParitioner(cmdArgs, maxSize)
-	for _, path := range paths {
-		partitions.add(path)
-	}
-	return partitions.end()
-}
-
-type sizePartitioner struct {
-	base    []string
-	parts   [][]string
-	current []string
-	size    int
-	max     int
-}
-
-func newSizeParitioner(base []string, max int) *sizePartitioner {
-	p := &sizePartitioner{base: base, max: max}
-	p.new()
-	return p
-}
-
-func (p *sizePartitioner) add(arg string) {
-	if p.size+len(arg)+1 > p.max {
-		p.new()
-	}
-	p.current = append(p.current, arg)
-	p.size += len(arg) + 1
-}
-
-func (p *sizePartitioner) new() {
-	p.end()
-	p.size = 0
-	p.current = []string{}
-	for _, arg := range p.base {
-		p.add(arg)
-	}
-}
-
-func (p *sizePartitioner) end() [][]string {
-	if len(p.current) > 0 {
-		p.parts = append(p.parts, p.current)
-	}
-	return p.parts
-}
-
-func partitionToMaxArgSizeWithFileGlobs(cmdArgs []string, paths []string) [][]string {
-	filePaths := packagesToFileGlobs(paths)
-	if len(filePaths) == 0 {
-		return nil
-	}
-	return partitionToMaxArgSize(cmdArgs, filePaths)
-}
-
-func partitionToPackageFileGlobs(cmdArgs []string, paths []string) [][]string {
-	parts := [][]string{}
-	for _, path := range paths {
-		filePaths := packagesToFileGlobs([]string{path})
-		if len(filePaths) == 0 {
-			continue
-		}
-		parts = append(parts, append(cmdArgs, filePaths...))
-	}
-	return parts
-}
diff --git a/partition.go b/partition.go
new file mode 100644
index 0000000..33355df
--- /dev/null
+++ b/partition.go
@@ -0,0 +1,143 @@
+package main
+
+import (
+	"fmt"
+	"path/filepath"
+)
+
+// MaxCommandBytes is the maximum number of bytes used when executing a command
+const MaxCommandBytes = 32000
+
+type partitionStrategy func([]string, []string) ([][]string, error)
+
+func getPartitionStrategy(name string) partitionStrategy {
+	switch {
+	case linterTakesFiles.contains(name):
+		return partitionToMaxArgSizeWithFileGlobs
+	case linterTakesFilesGroupedByPackage.contains(name):
+		return partitionToPackageFileGlobs
+	case linterTakesPackagePaths.contains(name):
+		return partitionToMaxArgSizeWithPackagePaths
+	}
+	return partitionToMaxArgSize
+}
+
+func pathsToFileGlobs(paths []string) ([]string, error) {
+	filePaths := []string{}
+	for _, dir := range paths {
+		paths, err := filepath.Glob(filepath.Join(dir, "*.go"))
+		if err != nil {
+			return nil, err
+		}
+		filePaths = append(filePaths, paths...)
+	}
+	return filePaths, nil
+}
+
+func partitionToMaxArgSize(cmdArgs []string, paths []string) ([][]string, error) {
+	return partitionToMaxSize(cmdArgs, paths, MaxCommandBytes), nil
+}
+
+func partitionToMaxSize(cmdArgs []string, paths []string, maxSize int) [][]string {
+	partitions := newSizePartitioner(cmdArgs, maxSize)
+	for _, path := range paths {
+		partitions.add(path)
+	}
+	return partitions.end()
+}
+
+type sizePartitioner struct {
+	base    []string
+	parts   [][]string
+	current []string
+	size    int
+	max     int
+}
+
+func newSizePartitioner(base []string, max int) *sizePartitioner {
+	p := &sizePartitioner{base: base, max: max}
+	p.new()
+	return p
+}
+
+func (p *sizePartitioner) add(arg string) {
+	if p.size+len(arg)+1 > p.max {
+		p.new()
+	}
+	p.current = append(p.current, arg)
+	p.size += len(arg) + 1
+}
+
+func (p *sizePartitioner) new() {
+	p.end()
+	p.size = 0
+	p.current = []string{}
+	for _, arg := range p.base {
+		p.add(arg)
+	}
+}
+
+func (p *sizePartitioner) end() [][]string {
+	if len(p.current) > 0 {
+		p.parts = append(p.parts, p.current)
+	}
+	return p.parts
+}
+
+func partitionToMaxArgSizeWithFileGlobs(cmdArgs []string, paths []string) ([][]string, error) {
+	filePaths, err := pathsToFileGlobs(paths)
+	if err != nil || len(filePaths) == 0 {
+		return nil, err
+	}
+	return partitionToMaxArgSize(cmdArgs, filePaths)
+}
+
+func partitionToPackageFileGlobs(cmdArgs []string, paths []string) ([][]string, error) {
+	parts := [][]string{}
+	for _, path := range paths {
+		filePaths, err := pathsToFileGlobs([]string{path})
+		if err != nil {
+			return nil, err
+		}
+		if len(filePaths) == 0 {
+			continue
+		}
+		parts = append(parts, append(cmdArgs, filePaths...))
+	}
+	return parts, nil
+}
+
+func partitionToMaxArgSizeWithPackagePaths(cmdArgs []string, paths []string) ([][]string, error) {
+	packagePaths, err := pathsToPackagePaths(paths)
+	if err != nil || len(packagePaths) == 0 {
+		return nil, err
+	}
+	return partitionToMaxArgSize(cmdArgs, packagePaths)
+}
+
+func pathsToPackagePaths(paths []string) ([]string, error) {
+	packages := []string{}
+
+	for _, path := range paths {
+		pkg, err := packageNameFromPath(path)
+		if err != nil {
+			return nil, err
+		}
+		packages = append(packages, pkg)
+	}
+	return packages, nil
+}
+
+func packageNameFromPath(path string) (string, error) {
+	if !filepath.IsAbs(path) {
+		return path, nil
+	}
+	for _, gopath := range getGoPathList() {
+		rel, err := filepath.Rel(filepath.Join(gopath, "src"), path)
+		if err != nil {
+			continue
+		}
+		return rel, nil
+	}
+	return "", fmt.Errorf("%s not in GOPATH", path)
+}


Test patch:
diff --git a/execute_test.go b/execute_test.go
index 4454310..5a75108 100644
--- a/execute_test.go
+++ b/execute_test.go
@@ -29,9 +29,9 @@ func TestSortedIssues(t *testing.T) {
 	require.Equal(t, expected, actual)
 }
 
-func TestLinterStateParitions(t *testing.T) {
-	noPartitions := func(_ []string, _ []string) [][]string {
-		return nil
+func TestLinterStatePartitions(t *testing.T) {
+	noPartitions := func(_ []string, _ []string) ([][]string, error) {
+		return nil, nil
 	}
 
 	state := &linterState{
diff --git a/parition_test.go b/partition_test.go
similarity index 68%
rename from parition_test.go
rename to partition_test.go
index bdba527..07ac305 100644
--- a/parition_test.go
+++ b/partition_test.go
@@ -10,7 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestParitionToMaxSize(t *testing.T) {
+func TestPartitionToMaxSize(t *testing.T) {
 	cmdArgs := []string{"/usr/bin/foo", "-c"}
 	paths := []string{"one", "two", "three", "four"}
 
@@ -38,7 +38,8 @@ func TestPartitionToPackageFileGlobs(t *testing.T) {
 		mkGoFile(t, dir, "other.go")
 	}
 
-	parts := partitionToPackageFileGlobs(cmdArgs, paths)
+	parts, err := partitionToPackageFileGlobs(cmdArgs, paths)
+	require.NoError(t, err)
 	expected := [][]string{
 		append(cmdArgs, packagePaths(paths[0], "file.go", "other.go")...),
 		append(cmdArgs, packagePaths(paths[1], "file.go", "other.go")...),
@@ -61,7 +62,8 @@ func TestPartitionToPackageFileGlobsNoFiles(t *testing.T) {
 
 	cmdArgs := []string{"/usr/bin/foo", "-c"}
 	paths := []string{filepath.Join(tmpdir, "one"), filepath.Join(tmpdir, "two")}
-	parts := partitionToPackageFileGlobs(cmdArgs, paths)
+	parts, err := partitionToPackageFileGlobs(cmdArgs, paths)
+	require.NoError(t, err)
 	assert.Len(t, parts, 0)
 }
 
@@ -72,6 +74,26 @@ func TestPartitionToMaxArgSizeWithFileGlobsNoFiles(t *testing.T) {
 
 	cmdArgs := []string{"/usr/bin/foo", "-c"}
 	paths := []string{filepath.Join(tmpdir, "one"), filepath.Join(tmpdir, "two")}
-	parts := partitionToMaxArgSizeWithFileGlobs(cmdArgs, paths)
+	parts, err := partitionToMaxArgSizeWithFileGlobs(cmdArgs, paths)
+	require.NoError(t, err)
 	assert.Len(t, parts, 0)
 }
+
+func TestPathsToPackagePaths(t *testing.T) {
+	root := "/fake/root"
+	defer fakeGoPath(t, root)()
+
+	packagePaths, err := pathsToPackagePaths([]string{
+		filepath.Join(root, "src", "example.com", "foo"),
+		"./relative/package",
+	})
+	require.NoError(t, err)
+	expected := []string{"example.com/foo", "./relative/package"}
+	assert.Equal(t, expected, packagePaths)
+}
+
+func fakeGoPath(t *testing.T, path string) func() {
+	oldpath := os.Getenv("GOPATH")
+	require.NoError(t, os.Setenv("GOPATH", path))
+	return func() { require.NoError(t, os.Setenv("GOPATH", oldpath)) }
+}