reduce mem. usage of unused and update staticcheck (#1063)

The primary improvement is in early clearing of
analyzed package's TypeInfo, facts, etc for
whole program analyzers (`unused`). Clear it when it
becomes unused and GC collects them early. Initially this
clearing was performed for all analyzers except `unused`.

Update staticcheck from v0.0.1-2019.2.3 to v0.0.1-2020.1.4

Also in this commit:
  * speed up loading packages from export data (2.5s -> 2.1s for std)
    by not using mutex for export data since it was allowed in
    x/tools#07722704da13
  * make an order of execution of linters stable
  * update renameio and robustio
  * use robustio in caching

Relates: #987, #994, #995, #1011

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

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=

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
 	}

internal/renameio/renameio_test.go

@@ -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 {

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 {

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
+}

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)
+	})
+}

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
 

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 errFileNotFound = syscall.ERROR_FILE_NOT_FOUND
-
-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)
-	})
-}
 
 // 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,

pkg/golinters/goanalysis/load/guard.go

@@ -8,7 +8,6 @@ import (
 
 type Guard struct {
 	loadMutexes map[*packages.Package]*sync.Mutex
-	mutexForExportData sync.Mutex
 	mutex       sync.Mutex
 }
 
@@ -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
 }

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
 	}
 
+	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()
+	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.

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
 			})
 

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
 			})
 

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

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

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