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

LukeShu · jirfag · commit e87a1cfb83d8 · 2019-09-09T16:10:49.000+03:00
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: golangci/go-tools@af6baa5 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.
diff --git a/go.mod b/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
diff --git a/go.sum b/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=
diff --git a/pkg/golinters/megacheck.go b/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
 	}
diff --git a/pkg/lint/load.go b/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
 	}
diff --git a/test/run_test.go b/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).
diff --git a/test/testdata_etc/unused_exported/golangci.yml b/test/testdata_etc/unused_exported/golangci.yml
@@ -0,0 +1,7 @@
+linters:
+  disable-all: true
+  enable:
+    - unused
+linters-settings:
+  unused:
+    check-exported: true
diff --git a/test/testdata_etc/unused_exported/lib/bar_test.go b/test/testdata_etc/unused_exported/lib/bar_test.go
@@ -0,0 +1 @@
+package lib
diff --git a/test/testdata_etc/unused_exported/lib/foo.go b/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")
+}
diff --git a/test/testdata_etc/unused_exported/main.go b/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()
+}	
diff --git a/vendor/github.com/golangci/go-tools/lint/lint.go b/vendor/github.com/golangci/go-tools/lint/lint.go
diff --git a/vendor/github.com/golangci/go-tools/stylecheck/lint.go b/vendor/github.com/golangci/go-tools/stylecheck/lint.go
diff --git a/vendor/modules.txt b/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
