From e87a1cfb83d8591afd5d45efbb178690ec7c5e31 Mon Sep 17 00:00:00 2001 From: Luke Shumaker Date: Mon, 9 Sep 2019 09:10:49 -0400 Subject: [PATCH] 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: https://github.com/golangci/go-tools/commit/af6baa5dc196960e7c24eea96da833727ed58f7f 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 | 2 +- go.sum | 4 +- pkg/golinters/megacheck.go | 18 +++++++-- pkg/lint/load.go | 39 +++++++++++-------- test/run_test.go | 4 ++ .../testdata_etc/unused_exported/golangci.yml | 7 ++++ .../unused_exported/lib/bar_test.go | 1 + test/testdata_etc/unused_exported/lib/foo.go | 13 +++++++ test/testdata_etc/unused_exported/main.go | 9 +++++ .../github.com/golangci/go-tools/lint/lint.go | 13 +------ .../golangci/go-tools/stylecheck/lint.go | 7 ---- vendor/modules.txt | 2 +- 12 files changed, 77 insertions(+), 42 deletions(-) create mode 100644 test/testdata_etc/unused_exported/golangci.yml create mode 100644 test/testdata_etc/unused_exported/lib/bar_test.go create mode 100644 test/testdata_etc/unused_exported/lib/foo.go create mode 100644 test/testdata_etc/unused_exported/main.go diff --git a/go.mod b/go.mod index 93551b3c..d7cb08a5 100644 --- 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 index d6aa1f6c..7c124012 100644 --- 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 index 14c66720..af5eaf0e 100644 --- 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 index 56b64417..01c125e7 100644 --- 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 index 0f8a7907..88ac071e 100644 --- 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 new file mode 100644 index 00000000..a49f3d01 --- /dev/null +++ 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 new file mode 100644 index 00000000..55c21f80 --- /dev/null +++ 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 new file mode 100644 index 00000000..97522cfd --- /dev/null +++ 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 new file mode 100644 index 00000000..575e34a1 --- /dev/null +++ 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 index a5a9feef..e9aa7933 100644 --- a/vendor/github.com/golangci/go-tools/lint/lint.go +++ b/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}) } diff --git a/vendor/github.com/golangci/go-tools/stylecheck/lint.go b/vendor/github.com/golangci/go-tools/stylecheck/lint.go index c0601861..620cacf3 100644 --- a/vendor/github.com/golangci/go-tools/stylecheck/lint.go +++ b/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{} diff --git a/vendor/modules.txt b/vendor/modules.txt index fac62db4..11efbd3e 100644 --- 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