From 8ba24d0d5d85bc6933975d340811f30ccd5138ea Mon Sep 17 00:00:00 2001 From: Luke Shumaker Date: Sun, 23 Jun 2019 11:29:33 -0400 Subject: [PATCH 1/3] Add failing testcase for false-positive from 'unused' This false-positive is not present in the upstream stand-alone 'unused' 2019.1.1 program that golangci-lint uses. --- test/run_test.go | 4 ++++ test/testdata_etc/unused_exported/golangci.yml | 7 +++++++ test/testdata_etc/unused_exported/lib/bar_test.go | 1 + test/testdata_etc/unused_exported/lib/foo.go | 13 +++++++++++++ test/testdata_etc/unused_exported/main.go | 9 +++++++++ 5 files changed, 34 insertions(+) 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/test/run_test.go b/test/run_test.go index 0f8a79073f85..88ac071e8226 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 000000000000..a49f3d01e0ce --- /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 000000000000..55c21f80aa65 --- /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 000000000000..97522cfdcc02 --- /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 000000000000..575e34a146bd --- /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() +} From a07dfde06a486c9aa933a816329dc8976fa0e227 Mon Sep 17 00:00:00 2001 From: Luke Shumaker Date: Sun, 23 Jun 2019 15:53:45 -0400 Subject: [PATCH 2/3] megacheck: Tidy up; make correspondence with upstream easier to follow --- pkg/golinters/megacheck.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/golinters/megacheck.go b/pkg/golinters/megacheck.go index 14c66720e78c..af5eaf0e5730 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 } From 479ce877a23125e2f513aa53d1a4f922a9e373f2 Mon Sep 17 00:00:00 2001 From: Luke Shumaker Date: Sun, 23 Jun 2019 16:31:29 -0400 Subject: [PATCH 3/3] Fix the 'unused' false-positive 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/lint/load.go | 39 +++++++++++-------- .../github.com/golangci/go-tools/lint/lint.go | 13 +------ .../golangci/go-tools/stylecheck/lint.go | 7 ---- vendor/modules.txt | 2 +- 6 files changed, 28 insertions(+), 39 deletions(-) diff --git a/go.mod b/go.mod index 1c452be00629..7f01d5b15829 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 58ababec8916..dcdcce94a050 100644 --- a/go.sum +++ b/go.sum @@ -57,8 +57,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/lint/load.go b/pkg/lint/load.go index 63e2076ff415..00ca888d4e84 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 { @@ -279,7 +274,7 @@ func (cl ContextLoader) loadPackages(ctx context.Context, loadMode packages.Load } } - return cl.filterPackages(pkgs), nil + return cl.filterTestMainPackages(pkgs), nil } func (cl ContextLoader) tryParseTestPackage(pkg *packages.Package) (name, testName string, isTest bool) { @@ -291,7 +286,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) @@ -305,11 +315,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, @@ -336,22 +341,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.LoadSyntax { - prog = cl.makeFakeLoaderProgram(pkgs) + prog = cl.makeFakeLoaderProgram(deduplicatedPkgs) } var ssaProg *ssa.Program if loadMode == packages.LoadAllSyntax { - 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/vendor/github.com/golangci/go-tools/lint/lint.go b/vendor/github.com/golangci/go-tools/lint/lint.go index a5a9feef024f..e9aa7933fdb5 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 c0601861a713..620cacf3e5d1 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 0cb94687ff8f..5c3fe80d5597 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