diff --git a/go.mod b/go.mod index a1fafb61..03714fe0 100644 --- a/go.mod +++ b/go.mod @@ -60,4 +60,4 @@ require ( // See https://github.com/golangci/golangci-lint/issues/995 // Update only after mitigating this issue. // TODO: Enable back tests with skip "Issue955" after update. -require honnef.co/go/tools v0.0.1-2019.2.3 +require honnef.co/go/tools v0.0.1-2020.1.3 diff --git a/go.sum b/go.sum index 4371e0b9..3708fe06 100644 --- a/go.sum +++ b/go.sum @@ -376,6 +376,7 @@ golang.org/x/tools v0.0.0-20190910044552-dd2b5c81c578/go.mod h1:b+2E5dAYhXwXZwtn golang.org/x/tools v0.0.0-20191108193012-7d206e10da11/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20191125144606-a911d9008d1f/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20191130070609-6e064ea0cf2d/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200228224639-71482053b885/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28= golang.org/x/tools v0.0.0-20200324003944-a576cf524670/go.mod h1:Sl4aGygMT6LrqrWclx+PTx3U+LnKx/seiNR+3G19Ar8= golang.org/x/tools v0.0.0-20200331202046-9d5940d49312/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= @@ -420,6 +421,8 @@ gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM= honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg= +honnef.co/go/tools v0.0.1-2020.1.3 h1:sXmLre5bzIR6ypkjXCDI3jHPssRhc8KD/Ome589sc3U= +honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed h1:WX1yoOaKQfddO/mLzdV4wptyWgoH/6hwLs7QHTixo0I= mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed/go.mod h1:Xkxe497xwlCKkIaQYRfC7CSLworTXY9RMqwhhCm+8Nc= mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b h1:DxJ5nJdkhDlLok9K6qO+5290kphDJbHOQO1DFFFTeBo= diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 9ac140c5..ae5b7da1 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -14,7 +14,6 @@ import ( "encoding/hex" "fmt" "io" - "io/ioutil" "os" "path/filepath" "strconv" @@ -24,6 +23,7 @@ import ( "github.com/pkg/errors" "github.com/golangci/golangci-lint/internal/renameio" + "github.com/golangci/golangci-lint/internal/robustio" ) // An ActionID is a cache action key, the hash of a complete description of a @@ -232,7 +232,7 @@ func (c *Cache) GetBytes(id ActionID) ([]byte, Entry, error) { return nil, entry, err } - data, err := ioutil.ReadFile(outputFile) + data, err := robustio.ReadFile(outputFile) if err != nil { return nil, entry, err } diff --git a/internal/renameio/renameio_test.go b/internal/renameio/renameio_test.go index b23b92e8..ae188e58 100644 --- a/internal/renameio/renameio_test.go +++ b/internal/renameio/renameio_test.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//+build !plan9 +// +build !plan9 package renameio @@ -22,8 +22,6 @@ import ( "github.com/golangci/golangci-lint/internal/robustio" ) -const windowsOS = "windows" - func TestConcurrentReadsAndWrites(t *testing.T) { dir, err := ioutil.TempDir("", "renameio") if err != nil { @@ -117,7 +115,7 @@ func TestConcurrentReadsAndWrites(t *testing.T) { } var minWriteSuccesses int64 = attempts - if runtime.GOOS == windowsOS { + if runtime.GOOS == "windows" { // Windows produces frequent "Access is denied" errors under heavy rename load. // As long as those are the only errors and *some* of the writes succeed, we're happy. minWriteSuccesses = attempts / 4 @@ -130,10 +128,18 @@ func TestConcurrentReadsAndWrites(t *testing.T) { } var minReadSuccesses int64 = attempts - if runtime.GOOS == windowsOS { + + switch runtime.GOOS { + case "windows": // Windows produces frequent "Access is denied" errors under heavy rename load. - // As long as those are the only errors and *some* of the writes succeed, we're happy. + // As long as those are the only errors and *some* of the reads succeed, we're happy. minReadSuccesses = attempts / 4 + + case "darwin": + // The filesystem on macOS 10.14 occasionally fails with "no such file or + // directory" errors. See https://golang.org/issue/33041. The flake rate is + // fairly low, so ensure that at least 75% of attempts succeed. + minReadSuccesses = attempts - (attempts / 4) } if readSuccesses < minReadSuccesses { diff --git a/internal/renameio/umask_test.go b/internal/renameio/umask_test.go index 1a471c9e..d75d67c9 100644 --- a/internal/renameio/umask_test.go +++ b/internal/renameio/umask_test.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//+build !nacl,!plan9,!windows,!js +// +build !plan9,!windows,!js package renameio @@ -19,6 +19,7 @@ func TestWriteFileModeAppliesUmask(t *testing.T) { if err != nil { t.Fatalf("Failed to create temporary directory: %v", err) } + defer os.RemoveAll(dir) const mode = 0644 const umask = 0007 @@ -29,7 +30,6 @@ func TestWriteFileModeAppliesUmask(t *testing.T) { if err != nil { t.Fatalf("Failed to write file: %v", err) } - defer os.RemoveAll(dir) fi, err := os.Stat(file) if err != nil { diff --git a/internal/robustio/robustio_darwin.go b/internal/robustio/robustio_darwin.go new file mode 100644 index 00000000..1ac0d10d --- /dev/null +++ b/internal/robustio/robustio_darwin.go @@ -0,0 +1,29 @@ +// Copyright 2019 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package robustio + +import ( + "os" + "syscall" +) + +const errFileNotFound = syscall.ENOENT + +// isEphemeralError returns true if err may be resolved by waiting. +func isEphemeralError(err error) bool { + switch werr := err.(type) { + case *os.PathError: + err = werr.Err + case *os.LinkError: + err = werr.Err + case *os.SyscallError: + err = werr.Err + + } + if errno, ok := err.(syscall.Errno); ok { + return errno == errFileNotFound + } + return false +} diff --git a/internal/robustio/robustio_flaky.go b/internal/robustio/robustio_flaky.go new file mode 100644 index 00000000..e0bf5b9b --- /dev/null +++ b/internal/robustio/robustio_flaky.go @@ -0,0 +1,93 @@ +// Copyright 2019 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build windows darwin + +package robustio + +import ( + "io/ioutil" + "math/rand" + "os" + "syscall" + "time" +) + +const arbitraryTimeout = 500 * time.Millisecond + +const ERROR_SHARING_VIOLATION = 32 + +// retry retries ephemeral errors from f up to an arbitrary timeout +// to work around filesystem flakiness on Windows and Darwin. +func retry(f func() (err error, mayRetry bool)) error { + var ( + bestErr error + lowestErrno syscall.Errno + start time.Time + nextSleep time.Duration = 1 * time.Millisecond + ) + for { + err, mayRetry := f() + if err == nil || !mayRetry { + return err + } + + if errno, ok := err.(syscall.Errno); ok && (lowestErrno == 0 || errno < lowestErrno) { + bestErr = err + lowestErrno = errno + } else if bestErr == nil { + bestErr = err + } + + if start.IsZero() { + start = time.Now() + } else if d := time.Since(start) + nextSleep; d >= arbitraryTimeout { + break + } + time.Sleep(nextSleep) + nextSleep += time.Duration(rand.Int63n(int64(nextSleep))) + } + + return bestErr +} + +// rename is like os.Rename, but retries ephemeral errors. +// +// On windows it wraps os.Rename, which (as of 2019-06-04) uses MoveFileEx with +// MOVEFILE_REPLACE_EXISTING. +// +// Windows also provides a different system call, ReplaceFile, +// that provides similar semantics, but perhaps preserves more metadata. (The +// documentation on the differences between the two is very sparse.) +// +// Empirical error rates with MoveFileEx are lower under modest concurrency, so +// for now we're sticking with what the os package already provides. +func rename(oldpath, newpath string) (err error) { + return retry(func() (err error, mayRetry bool) { + err = os.Rename(oldpath, newpath) + return err, isEphemeralError(err) + }) +} + +// readFile is like ioutil.ReadFile, but retries ephemeral errors. +func readFile(filename string) ([]byte, error) { + var b []byte + err := retry(func() (err error, mayRetry bool) { + b, err = ioutil.ReadFile(filename) + + // Unlike in rename, we do not retry errFileNotFound here: it can occur + // as a spurious error, but the file may also genuinely not exist, so the + // increase in robustness is probably not worth the extra latency. + + return err, isEphemeralError(err) && err != errFileNotFound + }) + return b, err +} + +func removeAll(path string) error { + return retry(func() (err error, mayRetry bool) { + err = os.RemoveAll(path) + return err, isEphemeralError(err) + }) +} diff --git a/internal/robustio/robustio_other.go b/internal/robustio/robustio_other.go index 91ca56cb..a2428856 100644 --- a/internal/robustio/robustio_other.go +++ b/internal/robustio/robustio_other.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//+build !windows +//+build !windows,!darwin package robustio diff --git a/internal/robustio/robustio_windows.go b/internal/robustio/robustio_windows.go index 02f1e8c1..a35237d4 100644 --- a/internal/robustio/robustio_windows.go +++ b/internal/robustio/robustio_windows.go @@ -5,93 +5,22 @@ package robustio import ( - "io/ioutil" - "math/rand" "os" "syscall" - "time" ) -const arbitraryTimeout = 500 * time.Millisecond - -const ERROR_SHARING_VIOLATION = 32 - -// retry retries ephemeral errors from f up to an arbitrary timeout -// to work around spurious filesystem errors on Windows -func retry(f func() (err error, mayRetry bool)) error { - var ( - bestErr error - lowestErrno syscall.Errno - start time.Time - nextSleep time.Duration = 1 * time.Millisecond - ) - for { - err, mayRetry := f() - if err == nil || !mayRetry { - return err - } - - if errno, ok := err.(syscall.Errno); ok && (lowestErrno == 0 || errno < lowestErrno) { - bestErr = err - lowestErrno = errno - } else if bestErr == nil { - bestErr = err - } - - if start.IsZero() { - start = time.Now() - } else if d := time.Since(start) + nextSleep; d >= arbitraryTimeout { - break - } - time.Sleep(nextSleep) - nextSleep += time.Duration(rand.Int63n(int64(nextSleep))) - } - - return bestErr -} - -// rename is like os.Rename, but retries ephemeral errors. -// -// It wraps os.Rename, which (as of 2019-06-04) uses MoveFileEx with -// MOVEFILE_REPLACE_EXISTING. -// -// Windows also provides a different system call, ReplaceFile, -// that provides similar semantics, but perhaps preserves more metadata. (The -// documentation on the differences between the two is very sparse.) -// -// Empirical error rates with MoveFileEx are lower under modest concurrency, so -// for now we're sticking with what the os package already provides. -func rename(oldpath, newpath string) (err error) { - return retry(func() (err error, mayRetry bool) { - err = os.Rename(oldpath, newpath) - return err, isEphemeralError(err) - }) -} - -// readFile is like ioutil.ReadFile, but retries ephemeral errors. -func readFile(filename string) ([]byte, error) { - var b []byte - err := retry(func() (err error, mayRetry bool) { - b, err = ioutil.ReadFile(filename) - - // Unlike in rename, we do not retry ERROR_FILE_NOT_FOUND here: it can occur - // as a spurious error, but the file may also genuinely not exist, so the - // increase in robustness is probably not worth the extra latency. - - return err, isEphemeralError(err) && err != syscall.ERROR_FILE_NOT_FOUND - }) - return b, err -} - -func removeAll(path string) error { - return retry(func() (err error, mayRetry bool) { - err = os.RemoveAll(path) - return err, isEphemeralError(err) - }) -} +const errFileNotFound = syscall.ERROR_FILE_NOT_FOUND // isEphemeralError returns true if err may be resolved by waiting. func isEphemeralError(err error) bool { + switch werr := err.(type) { + case *os.PathError: + err = werr.Err + case *os.LinkError: + err = werr.Err + case *os.SyscallError: + err = werr.Err + } if errno, ok := err.(syscall.Errno); ok { switch errno { case syscall.ERROR_ACCESS_DENIED, diff --git a/pkg/golinters/goanalysis/load/guard.go b/pkg/golinters/goanalysis/load/guard.go index 5695f717..ab7775cc 100644 --- a/pkg/golinters/goanalysis/load/guard.go +++ b/pkg/golinters/goanalysis/load/guard.go @@ -7,9 +7,8 @@ import ( ) type Guard struct { - loadMutexes map[*packages.Package]*sync.Mutex - mutexForExportData sync.Mutex - mutex sync.Mutex + loadMutexes map[*packages.Package]*sync.Mutex + mutex sync.Mutex } func NewGuard() *Guard { @@ -26,10 +25,6 @@ func (g *Guard) MutexForPkg(pkg *packages.Package) *sync.Mutex { return g.loadMutexes[pkg] } -func (g *Guard) MutexForExportData() *sync.Mutex { - return &g.mutexForExportData -} - func (g *Guard) Mutex() *sync.Mutex { return &g.mutex } diff --git a/pkg/golinters/goanalysis/runner.go b/pkg/golinters/goanalysis/runner.go index 64087c28..d0729424 100644 --- a/pkg/golinters/goanalysis/runner.go +++ b/pkg/golinters/goanalysis/runner.go @@ -384,26 +384,6 @@ func extractDiagnostics(roots []*action) (retDiags []Diagnostic, retErrors []err return } -// NeedFacts reports whether any analysis required by the specified set -// needs facts. If so, we must load the entire program from source. -func NeedFacts(analyzers []*analysis.Analyzer) bool { - seen := make(map[*analysis.Analyzer]bool) - var q []*analysis.Analyzer // for BFS - q = append(q, analyzers...) - for len(q) > 0 { - a := q[0] - q = q[1:] - if !seen[a] { - seen[a] = true - if len(a.FactTypes) > 0 { - return true - } - q = append(q, a.Requires...) - } - } - return false -} - // An action represents one unit of analysis work: the application of // one analysis to one package. Actions form a DAG, both within a // package (as different analyzers are applied, either in sequence or @@ -964,7 +944,7 @@ func sizeOfReflectValueTreeBytes(rv reflect.Value, visitedPtrs map[uintptr]struc } } -func (lp *loadingPackage) decUse() { +func (lp *loadingPackage) decUse(canClearTypes bool) { lp.decUseMutex.Lock() defer lp.decUseMutex.Unlock() @@ -1007,11 +987,17 @@ func (lp *loadingPackage) decUse() { return } - lp.pkg.Types = nil + if canClearTypes { + // canClearTypes is set to true if we can discard type + // information after the package and its dependents have been + // processed. This is the case when no whole program checkers (unused) are + // being run. + lp.pkg.Types = nil + } lp.pkg = nil for _, imp := range lp.imports { - imp.decUse() + imp.decUse(canClearTypes) } lp.imports = nil @@ -1047,12 +1033,8 @@ func (lp *loadingPackage) analyze(loadMode LoadMode, loadSem chan struct{}) { <-loadSem }() - defer func() { - if loadMode < LoadModeWholeProgram { - // Save memory on unused more fields. - lp.decUse() - } - }() + // Save memory on unused more fields. + defer lp.decUse(loadMode < LoadModeWholeProgram) if err := lp.loadWithFacts(loadMode); err != nil { werr := errors.Wrapf(err, "failed to load package %s", lp.pkg.Name) @@ -1168,22 +1150,6 @@ func (lp *loadingPackage) loadFromSource(loadMode LoadMode) error { } func (lp *loadingPackage) loadFromExportData() error { - // Because gcexportdata.Read has the potential to create or - // modify the types.Package for each node in the transitive - // closure of dependencies of lpkg, all exportdata operations - // must be sequential. (Finer-grained locking would require - // changes to the gcexportdata API.) - // - // The exportMu lock guards the Package.Pkg field and the - // types.Package it points to, for each Package in the graph. - // - // Not all accesses to Package.Pkg need to be protected by this mutex: - // graph ordering ensures that direct dependencies of source - // packages are fully loaded before the importer reads their Pkg field. - mu := lp.loadGuard.MutexForExportData() - mu.Lock() - defer mu.Unlock() - pkg := lp.pkg // Call NewPackage directly with explicit name. diff --git a/pkg/golinters/gocognit.go b/pkg/golinters/gocognit.go index 8044e3e5..78afff86 100644 --- a/pkg/golinters/gocognit.go +++ b/pkg/golinters/gocognit.go @@ -39,7 +39,7 @@ func NewGocognit() *goanalysis.Linter { return nil, nil } - sort.Slice(stats, func(i, j int) bool { + sort.SliceStable(stats, func(i, j int) bool { return stats[i].Complexity > stats[j].Complexity }) diff --git a/pkg/golinters/gocyclo.go b/pkg/golinters/gocyclo.go index f9c0a799..55f13fcf 100644 --- a/pkg/golinters/gocyclo.go +++ b/pkg/golinters/gocyclo.go @@ -39,7 +39,7 @@ func NewGocyclo() *goanalysis.Linter { return nil, nil } - sort.Slice(stats, func(i, j int) bool { + sort.SliceStable(stats, func(i, j int) bool { return stats[i].Complexity > stats[j].Complexity }) diff --git a/pkg/golinters/unused.go b/pkg/golinters/unused.go index ac5a90d4..bbe1e4b6 100644 --- a/pkg/golinters/unused.go +++ b/pkg/golinters/unused.go @@ -55,7 +55,10 @@ func NewUnused() *goanalysis.Linter { } return issues }).WithContextSetter(func(lintCtx *linter.Context) { - u.WholeProgram = lintCtx.Settings().Unused.CheckExported + if lintCtx.Settings().Unused.CheckExported { + lintCtx.Log.Infof("Using whole program analysis for unused, it can be memory-heavy") + u.WholeProgram = true + } }).WithLoadMode(goanalysis.LoadModeWholeProgram) lnt.UseOriginalPackages() return lnt diff --git a/pkg/lint/lintersdb/enabled_set.go b/pkg/lint/lintersdb/enabled_set.go index 73e3bcfe..37e2166f 100644 --- a/pkg/lint/lintersdb/enabled_set.go +++ b/pkg/lint/lintersdb/enabled_set.go @@ -2,6 +2,7 @@ package lintersdb import ( "sort" + "strings" "github.com/golangci/golangci-lint/pkg/config" "github.com/golangci/golangci-lint/pkg/golinters/goanalysis" @@ -92,6 +93,11 @@ func (es EnabledSet) Get(optimize bool) ([]*linter.Config, error) { resultLinters = append(resultLinters, lc) } + // Make order of execution of linters (go/analysis metalinter and unused) stable. + sort.Slice(resultLinters, func(i, j int) bool { + return strings.Compare(resultLinters[i].Name(), resultLinters[j].Name()) <= 0 + }) + return resultLinters, nil } @@ -122,6 +128,10 @@ func (es EnabledSet) combineGoAnalysisLinters(linters map[string]*linter.Config) delete(linters, lnt.Name()) } + // Make order of execution of go/analysis analyzers stable. + sort.Slice(goanalysisLinters, func(i, j int) bool { + return strings.Compare(goanalysisLinters[i].Name(), goanalysisLinters[j].Name()) <= 0 + }) ml := goanalysis.NewMetaLinter(goanalysisLinters) var presets []string diff --git a/pkg/lint/load.go b/pkg/lint/load.go index de0ab411..e2928a57 100644 --- a/pkg/lint/load.go +++ b/pkg/lint/load.go @@ -98,6 +98,7 @@ func (cl *ContextLoader) makeBuildFlags() ([]string, error) { if len(cl.cfg.Run.BuildTags) != 0 { // go help build buildFlags = append(buildFlags, "-tags", strings.Join(cl.cfg.Run.BuildTags, " ")) + cl.log.Infof("Using build tags: %v", cl.cfg.Run.BuildTags) } mod := cl.cfg.Run.ModulesDownloadMode