Fix a false-positive from 'unused' (#585)

This false-positive is not present in the upstream stand-alone 'unused'
2019.1.1 program that golangci-lint uses.

pkg/lint.ContextLoader.filterPackages() did two things:
 1. It removed synthetic "testmain" packages (packages with .Name=="main"
    and .PkgPath ending with ".test")
 2. It removed pruned subsumed copies of packages; if a package with files
    "a.go" and "a_test.go", it results in packages.Load giving us two
    packages:
      - ID=".../a" GoFiles=[a.go]
      - ID=".../a [.../a.test]" GoFiles=[a.go a_test.go]
    The first package is subsumed in the second, and leaving it around
    results in duplicated work, and confuses the 'deadcode' linter.

However, the 'unused' linter relies on both the ".../a" and
".../a [.../a.test]" packages being present.  Pruning them causes it to
panic in some situations, which lead to this workaround:
af6baa5dc1

While that workaround got it to not panic, it causes incorrect results.

So, split filterPackages() in to two functions: filterTestMainPackages()
and filterDuplicatePackages().  The linter.Context.Packages list only
gets filterTestMainPackages() called on it, while linter.Context.Program
and linter.Context.SSAProgram get both filters applied.

With the source of the panic fixed, roll back a few now-unnecessary
commits in go-tools.

go.mod

@@ -14,7 +14,7 @@ require (
 	github.com/golangci/dupl v0.0.0-20180902072040-3e9179ac440a
 	github.com/golangci/errcheck v0.0.0-20181003203344-ef45e06d44b6
 	github.com/golangci/go-misc v0.0.0-20180628070357-927a3d87b613
-	github.com/golangci/go-tools v0.0.0-20180109140146-af6baa5dc196
+	github.com/golangci/go-tools v0.0.0-20190318055746-e32c54105b7c
 	github.com/golangci/goconst v0.0.0-20180610141641-041c5f2b40f3
 	github.com/golangci/gocyclo v0.0.0-20180528134321-2becd97e67ee
 	github.com/golangci/gofmt v0.0.0-20181105071733-0b8337e80d98

go.sum

@@ -59,8 +59,8 @@ github.com/golangci/errcheck v0.0.0-20181003203344-ef45e06d44b6 h1:i2jIkQFb8RG45
 github.com/golangci/errcheck v0.0.0-20181003203344-ef45e06d44b6/go.mod h1:DbHgvLiFKX1Sh2T1w8Q/h4NAI8MHIpzCdnBUDTXU3I0=
 github.com/golangci/go-misc v0.0.0-20180628070357-927a3d87b613 h1:9kfjN3AdxcbsZBf8NjltjWihK2QfBBBZuv91cMFfDHw=
 github.com/golangci/go-misc v0.0.0-20180628070357-927a3d87b613/go.mod h1:SyvUF2NxV+sN8upjjeVYr5W7tyxaT1JVtvhKhOn2ii8=
-github.com/golangci/go-tools v0.0.0-20180109140146-af6baa5dc196 h1:9rtVlONXLF1rJZzvLt4tfOXtnAFUEhxCJ64Ibzj6ECo=
-github.com/golangci/go-tools v0.0.0-20180109140146-af6baa5dc196/go.mod h1:unzUULGw35sjyOYjUt0jMTXqHlZPpPc6e+xfO4cd6mM=
+github.com/golangci/go-tools v0.0.0-20190318055746-e32c54105b7c h1:/7detzz5stiXWPzkTlPTzkBEIIE4WGpppBJYjKqBiPI=
+github.com/golangci/go-tools v0.0.0-20190318055746-e32c54105b7c/go.mod h1:unzUULGw35sjyOYjUt0jMTXqHlZPpPc6e+xfO4cd6mM=
 github.com/golangci/goconst v0.0.0-20180610141641-041c5f2b40f3 h1:pe9JHs3cHHDQgOFXJJdYkK6fLz2PWyYtP4hthoCMvs8=
 github.com/golangci/goconst v0.0.0-20180610141641-041c5f2b40f3/go.mod h1:JXrF4TWy4tXYn62/9x8Wm/K/dm06p8tCKwFRDPZG/1o=
 github.com/golangci/gocyclo v0.0.0-20180528134321-2becd97e67ee h1:J2XAy40+7yz70uaOiMbNnluTg7gyQhtGqLQncQh+4J8=

pkg/golinters/megacheck.go

@@ -237,10 +237,11 @@ func (m megacheck) runMegacheck(workingPkgs []*packages.Package, checkExportedUn
 		// TODO: support Ignores option
 	}
 
-	return runMegacheckCheckers(checkers, opts, workingPkgs)
+	return runMegacheckCheckers(checkers, workingPkgs, opts)
 }
 
-// parseIgnore is a copy from megacheck code just to not fork megacheck
+// parseIgnore is a copy from megacheck honnef.co/go/tools/lint/lintutil.parseIgnore
+// just to not fork megacheck.
 func parseIgnore(s string) ([]lint.Ignore, error) {
 	var out []lint.Ignore
 	if s == "" {
@@ -258,17 +259,28 @@ func parseIgnore(s string) ([]lint.Ignore, error) {
 	return out, nil
 }
 
-func runMegacheckCheckers(cs []lint.Checker, opt *lintutil.Options, workingPkgs []*packages.Package) ([]lint.Problem, error) {
+// runMegacheckCheckers is like megacheck honnef.co/go/tools/lint/lintutil.Lint,
+// but takes a list of already-parsed packages instead of a list of
+// package-paths to parse.
+func runMegacheckCheckers(cs []lint.Checker, workingPkgs []*packages.Package, opt *lintutil.Options) ([]lint.Problem, error) {
 	stats := lint.PerfStats{
 		CheckerInits: map[string]time.Duration{},
 	}
 
+	if opt == nil {
+		opt = &lintutil.Options{}
+	}
 	ignores, err := parseIgnore(opt.Ignores)
 	if err != nil {
 		return nil, err
 	}
 
+	// package-parsing elided here
+	stats.PackageLoading = 0
+
 	var problems []lint.Problem
+	// populating 'problems' with parser-problems elided here
+
 	if len(workingPkgs) == 0 {
 		return problems, nil
 	}

pkg/lint/load.go

@@ -88,11 +88,6 @@ func (cl ContextLoader) makeFakeLoaderPackageInfo(pkg *packages.Package) *loader
 	}
 }
 
-func shouldSkipPkg(pkg *packages.Package) bool {
-	// it's an implicit testmain package
-	return pkg.Name == "main" && strings.HasSuffix(pkg.PkgPath, ".test")
-}
-
 func (cl ContextLoader) makeFakeLoaderProgram(pkgs []*packages.Package) *loader.Program {
 	var createdPkgs []*loader.PackageInfo
 	for _, pkg := range pkgs {
@@ -296,7 +291,7 @@ func (cl ContextLoader) loadPackages(ctx context.Context, loadMode packages.Load
 		return nil, err
 	}
 
-	return cl.filterPackages(pkgs), nil
+	return cl.filterTestMainPackages(pkgs), nil
 }
 
 func (cl ContextLoader) tryParseTestPackage(pkg *packages.Package) (name, testName string, isTest bool) {
@@ -308,7 +303,22 @@ func (cl ContextLoader) tryParseTestPackage(pkg *packages.Package) (name, testNa
 	return matches[1], matches[2], true
 }
 
-func (cl ContextLoader) filterPackages(pkgs []*packages.Package) []*packages.Package {
+func (cl ContextLoader) filterTestMainPackages(pkgs []*packages.Package) []*packages.Package {
+	var retPkgs []*packages.Package
+	for _, pkg := range pkgs {
+		if pkg.Name == "main" && strings.HasSuffix(pkg.PkgPath, ".test") {
+			// it's an implicit testmain package
+			cl.debugf("skip pkg ID=%s", pkg.ID)
+			continue
+		}
+
+		retPkgs = append(retPkgs, pkg)
+	}
+
+	return retPkgs
+}
+
+func (cl ContextLoader) filterDuplicatePackages(pkgs []*packages.Package) []*packages.Package {
 	packagesWithTests := map[string]bool{}
 	for _, pkg := range pkgs {
 		name, _, isTest := cl.tryParseTestPackage(pkg)
@@ -322,11 +332,6 @@ func (cl ContextLoader) filterPackages(pkgs []*packages.Package) []*packages.Pac
 
 	var retPkgs []*packages.Package
 	for _, pkg := range pkgs {
-		if shouldSkipPkg(pkg) {
-			cl.debugf("skip pkg ID=%s", pkg.ID)
-			continue
-		}
-
 		_, _, isTest := cl.tryParseTestPackage(pkg)
 		if !isTest && packagesWithTests[pkg.PkgPath] {
 			// If tests loading is enabled,
@@ -353,22 +358,24 @@ func (cl ContextLoader) Load(ctx context.Context, linters []*linter.Config) (*li
 		return nil, err
 	}
 
-	if len(pkgs) == 0 {
+	deduplicatedPkgs := cl.filterDuplicatePackages(pkgs)
+
+	if len(deduplicatedPkgs) == 0 {
 		return nil, exitcodes.ErrNoGoFiles
 	}
 
 	var prog *loader.Program
 	if loadMode&packages.NeedTypes != 0 {
-		prog = cl.makeFakeLoaderProgram(pkgs)
+		prog = cl.makeFakeLoaderProgram(deduplicatedPkgs)
 	}
 
 	var ssaProg *ssa.Program
 	if loadMode&packages.NeedDeps != 0 {
-		ssaProg = cl.buildSSAProgram(pkgs)
+		ssaProg = cl.buildSSAProgram(deduplicatedPkgs)
 	}
 
 	astLog := cl.log.Child("astcache")
-	astCache, err := astcache.LoadFromPackages(pkgs, astLog)
+	astCache, err := astcache.LoadFromPackages(deduplicatedPkgs, astLog)
 	if err != nil {
 		return nil, err
 	}

test/run_test.go

@@ -117,6 +117,10 @@ func TestIdentifierUsedOnlyInTests(t *testing.T) {
 	testshared.NewLintRunner(t).Run("--no-config", "--disable-all", "-Eunused", getTestDataDir("used_only_in_tests")).ExpectNoIssues()
 }
 
+func TestUnusedCheckExported(t *testing.T) {
+	testshared.NewLintRunner(t).Run("-c", "testdata_etc/unused_exported/golangci.yml", "testdata_etc/unused_exported/...").ExpectNoIssues()
+}
+
 func TestConfigFileIsDetected(t *testing.T) {
 	checkGotConfig := func(r *testshared.RunResult) {
 		r.ExpectExitCode(exitcodes.Success).

test/testdata_etc/unused_exported/golangci.yml

@@ -0,0 +1,7 @@
+linters:
+  disable-all: true
+  enable:
+    - unused
+linters-settings:
+  unused:
+    check-exported: true

test/testdata_etc/unused_exported/lib/bar_test.go

@@ -0,0 +1 @@
+package lib

test/testdata_etc/unused_exported/lib/foo.go

@@ -0,0 +1,13 @@
+package lib
+
+import (
+	"fmt"
+)
+
+func PublicFunc() {
+	privateFunc()
+}
+
+func privateFunc() {
+	fmt.Println("side effect")
+}

test/testdata_etc/unused_exported/main.go

@@ -0,0 +1,9 @@
+package main
+
+import (
+	"github.com/golangci/golangci-lint/test/testdata_etc/unused_exported/lib"
+)
+
+func main() {
+	lib.PublicFunc()
+}	

vendor/github.com/golangci/go-tools/lint/lint.go

@@ -9,17 +9,16 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"runtime/debug"
 	"sort"
 	"strings"
 	"sync"
 	"time"
 	"unicode"
 
+	"golang.org/x/tools/go/packages"
 	"github.com/golangci/go-tools/config"
 	"github.com/golangci/go-tools/ssa"
 	"github.com/golangci/go-tools/ssa/ssautil"
-	"golang.org/x/tools/go/packages"
 )
 
 type Job struct {
@@ -30,7 +29,6 @@ type Job struct {
 	problems []Problem
 
 	duration time.Duration
-	panicErr error
 }
 
 type Ignore interface {
@@ -453,11 +451,6 @@ func (l *Linter) Lint(initial []*packages.Package, stats *PerfStats) []Problem {
 	for _, j := range jobs {
 		wg.Add(1)
 		go func(j *Job) {
-			defer func() {
-				if panicErr := recover(); panicErr != nil {
-					j.panicErr = fmt.Errorf("panic: %s: %s", panicErr, string(debug.Stack()))
-				}
-			}()
 			defer wg.Done()
 			sem <- struct{}{}
 			defer func() { <-sem }()
@@ -473,10 +466,6 @@ func (l *Linter) Lint(initial []*packages.Package, stats *PerfStats) []Problem {
 	wg.Wait()
 
 	for _, j := range jobs {
-		if j.panicErr != nil {
-			panic(j.panicErr)
-		}
-
 		if stats != nil {
 			stats.Jobs = append(stats.Jobs, JobStat{j.check.ID, j.duration})
 		}

vendor/github.com/golangci/go-tools/stylecheck/lint.go

@@ -252,9 +252,6 @@ func (c *Checker) CheckUnexportedReturn(j *lint.Job) {
 
 func (c *Checker) CheckReceiverNames(j *lint.Job) {
 	for _, pkg := range j.Program.InitialPackages {
-		if pkg.SSA == nil {
-			continue
-		}
 		for _, m := range pkg.SSA.Members {
 			if T, ok := m.Object().(*types.TypeName); ok && !T.IsAlias() {
 				ms := typeutil.IntuitiveMethodSet(T.Type(), nil)
@@ -279,10 +276,6 @@ func (c *Checker) CheckReceiverNames(j *lint.Job) {
 
 func (c *Checker) CheckReceiverNamesIdentical(j *lint.Job) {
 	for _, pkg := range j.Program.InitialPackages {
-		if pkg.SSA == nil {
-			continue
-		}
-
 		for _, m := range pkg.SSA.Members {
 			names := map[string]int{}
 

vendor/modules.txt

@@ -61,7 +61,7 @@ github.com/golangci/errcheck/golangci
 github.com/golangci/errcheck/internal/errcheck
 # github.com/golangci/go-misc v0.0.0-20180628070357-927a3d87b613
 github.com/golangci/go-misc/deadcode
-# github.com/golangci/go-tools v0.0.0-20180109140146-af6baa5dc196
+# github.com/golangci/go-tools v0.0.0-20190318055746-e32c54105b7c
 github.com/golangci/go-tools/config
 github.com/golangci/go-tools/lint
 github.com/golangci/go-tools/lint/lintutil