Merge pull request #1157 from tamird/static-checker

Add static checkers

Author: sdboyer

.travis.yml

@@ -7,7 +7,7 @@ jobs:
     - stage: test
       go_import_path: github.com/golang/dep
       install:
-        - go get -u honnef.co/go/tools/cmd/{gosimple,staticcheck}
+        - go get -u github.com/golang/lint/golint honnef.co/go/tools/cmd/megacheck
         - npm install -g codeclimate-test-reporter
       env:
         - DEPTESTBYPASS501=1

cmd/dep/ensure.go

@@ -135,7 +135,6 @@ type ensureCommand struct {
 	noVendor   bool
 	vendorOnly bool
 	dryRun     bool
-	overrides  stringSlice
 }
 
 func (cmd *ensureCommand) Run(ctx *dep.Ctx, args []string) error {
@@ -699,19 +698,6 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
 	return errors.Wrapf(f.Close(), "closing %s", dep.ManifestName)
 }
 
-type stringSlice []string
-
-func (s *stringSlice) String() string {
-	if len(*s) == 0 {
-		return "<none>"
-	}
-	return strings.Join(*s, ", ")
-}
-
-func (s *stringSlice) Set(value string) error {
-	*s = append(*s, value)
-	return nil
-}
 func getProjectConstraint(arg string, sm gps.SourceManager) (gps.ProjectConstraint, string, error) {
 	emptyPC := gps.ProjectConstraint{
 		Constraint: gps.Any(), // default to any; avoids panics later

context.go

@@ -83,6 +83,8 @@ func defaultGOPATH() string {
 	return ""
 }
 
+// SourceManager produces an instance of gps's built-in SourceManager
+// initialized to log to the receiver's logger.
 func (c *Ctx) SourceManager() (*gps.SourceMgr, error) {
 	return gps.NewSourceManager(gps.SourceManagerConfig{
 		Cachedir: filepath.Join(c.GOPATH, "pkg", "dep"),

hack/lint.bash

@@ -6,7 +6,7 @@
 # This script will validate code with various linters
 set -e
 
-PKGS=$(go list ./... | grep -v /vendor/)
+PKGS=$(go list ./... | grep -vF /vendor/)
 go vet $PKGS
-staticcheck $PKGS
-gosimple $PKGS
+golint $PKGS
+megacheck -unused.exported -ignore github.com/golang/dep/internal/test/test.go:U1000 $PKGS

internal/gps/bridge.go

@@ -56,12 +56,6 @@ type bridge struct {
 	// held by the solver that it ends up being easier and saner to do this.
 	s *solver
 
-	// Simple, local cache of the root's PackageTree
-	crp *struct {
-		ptree pkgtree.PackageTree
-		err   error
-	}
-
 	// Map of project root name to their available version list. This cache is
 	// layered on top of the proper SourceManager's cache; the only difference
 	// is that this keeps the versions sorted in the direction required by the

internal/gps/cmd.go

@@ -72,14 +72,14 @@ func (c *monitoredCmd) run(ctx context.Context) error {
 	// in at the same time as process completion, but one of the former are
 	// picked first; in such a case, cmd.Process could(?) be nil by the time we
 	// call signal methods on it.
-	var isDone *int32 = new(int32)
+	var isDone int32
 	done := make(chan error, 1)
 
 	go func() {
 		// Wait() can only be called once, so this must act as the completion
 		// indicator for both normal *and* signal-induced termination.
 		done <- c.cmd.Wait()
-		atomic.CompareAndSwapInt32(isDone, 0, 1)
+		atomic.CompareAndSwapInt32(&isDone, 0, 1)
 	}()
 
 	var killerr error
@@ -89,17 +89,17 @@ selloop:
 		case err := <-done:
 			return err
 		case <-ticker.C:
-			if !atomic.CompareAndSwapInt32(isDone, 1, 1) && c.hasTimedOut() {
-				if err := killProcess(c.cmd, isDone); err != nil {
+			if !atomic.CompareAndSwapInt32(&isDone, 1, 1) && c.hasTimedOut() {
+				if err := killProcess(c.cmd, &isDone); err != nil {
 					killerr = &killCmdError{err}
 				} else {
 					killerr = &noProgressError{c.timeout}
 				}
 				break selloop
 			}
 		case <-ctx.Done():
-			if !atomic.CompareAndSwapInt32(isDone, 1, 1) {
-				if err := killProcess(c.cmd, isDone); err != nil {
+			if !atomic.CompareAndSwapInt32(&isDone, 1, 1) {
+				if err := killProcess(c.cmd, &isDone); err != nil {
 					killerr = &killCmdError{err}
 				} else {
 					killerr = ctx.Err()

internal/gps/pkgtree/pkgtree_test.go

@@ -32,9 +32,6 @@ func TestWorkmapToReach(t *testing.T) {
 		return make(map[string]bool)
 	}
 
-	e := struct {
-		Internal, External []string
-	}{}
 	table := map[string]struct {
 		workmap  map[string]wm
 		rm       ReachMap
@@ -49,7 +46,7 @@ func TestWorkmapToReach(t *testing.T) {
 				},
 			},
 			rm: ReachMap{
-				"foo": e,
+				"foo": {},
 			},
 		},
 		"no external": {
@@ -64,8 +61,8 @@ func TestWorkmapToReach(t *testing.T) {
 				},
 			},
 			rm: ReachMap{
-				"foo":     e,
-				"foo/bar": e,
+				"foo":     {},
+				"foo/bar": {},
 			},
 		},
 		"no external with subpkg": {
@@ -85,7 +82,7 @@ func TestWorkmapToReach(t *testing.T) {
 				"foo": {
 					Internal: []string{"foo/bar"},
 				},
-				"foo/bar": e,
+				"foo/bar": {},
 			},
 		},
 		"simple base transitive": {

internal/gps/selection.go

@@ -73,10 +73,6 @@ func (s *selection) depperCount(id ProjectIdentifier) int {
 	return len(s.deps[id.ProjectRoot])
 }
 
-func (s *selection) setDependenciesOn(id ProjectIdentifier, deps []dependency) {
-	s.deps[id.ProjectRoot] = deps
-}
-
 // Compute a list of the unique packages within the given ProjectIdentifier that
 // have dependers, and the number of dependers they have.
 func (s *selection) getRequiredPackagesIn(id ProjectIdentifier) map[string]int {
@@ -93,6 +89,9 @@ func (s *selection) getRequiredPackagesIn(id ProjectIdentifier) map[string]int {
 	return uniq
 }
 
+// Suppress unused warning.
+var _ = (*selection)(nil).getSelectedPackagesIn
+
 // Compute a list of the unique packages within the given ProjectIdentifier that
 // are currently selected, and the number of times each package has been
 // independently selected.

internal/gps/solve_basic_test.go

@@ -1353,14 +1353,6 @@ func (sm *depspecSourceManager) GetManifestAndLock(id ProjectIdentifier, v Versi
 	return nil, nil, fmt.Errorf("Project %s at version %s could not be found", id, v)
 }
 
-func (sm *depspecSourceManager) ExternalReach(id ProjectIdentifier, v Version) (map[string][]string, error) {
-	pid := pident{n: ProjectRoot(id.normalizedSource()), v: v}
-	if m, exists := sm.rm[pid]; exists {
-		return m, nil
-	}
-	return nil, fmt.Errorf("No reach data for %s at version %s", id, v)
-}
-
 func (sm *depspecSourceManager) ListPackages(id ProjectIdentifier, v Version) (pkgtree.PackageTree, error) {
 	pid := pident{n: ProjectRoot(id.normalizedSource()), v: v}
 	if pv, ok := v.(PairedVersion); ok && pv.Revision() == "FAKEREV" {
@@ -1568,10 +1560,6 @@ func (ds depspec) DependencyConstraints() ProjectConstraints {
 
 type fixLock []LockedProject
 
-func (fixLock) SolverVersion() string {
-	return "-1"
-}
-
 // impl Lock interface
 func (fixLock) InputHash() []byte {
 	return []byte("fooooorooooofooorooofoo")
@@ -1584,11 +1572,6 @@ func (l fixLock) Projects() []LockedProject {
 
 type dummyLock struct{}
 
-// impl Lock interface
-func (dummyLock) SolverVersion() string {
-	return "-1"
-}
-
 // impl Lock interface
 func (dummyLock) InputHash() []byte {
 	return []byte("fooooorooooofooorooofoo")

internal/gps/solve_failures.go

@@ -11,17 +11,6 @@ import (
 	"strings"
 )
 
-type errorLevel uint8
-
-// TODO(sdboyer) consistent, sensible way of handling 'type' and 'severity' - or figure
-// out that they're not orthogonal and collapse into just 'type'
-
-const (
-	warning errorLevel = 1 << iota
-	mustResolve
-	cannotResolve
-)
-
 func a2vs(a atom) string {
 	if a.v == rootRev || a.v == nil {
 		return "(root)"

internal/gps/source_manager.go

@@ -213,9 +213,9 @@ func NewSourceManager(c SourceManagerConfig) (*SourceMgr, error) {
 			lasttime = nowtime
 		}
 
-		if _, ok := err.(interface {
+		if t, ok := err.(interface {
 			Temporary() bool
-		}); ok {
+		}); ok && t.Temporary() {
 			time.Sleep(time.Second * 1)
 		} else {
 			return nil, CouldNotCreateLockError{
@@ -677,7 +677,6 @@ const (
 	ctSourcePing
 	ctSourceInit
 	ctSourceFetch
-	ctCheckoutVersion
 	ctExportTree
 )
 

internal/gps/typed_radix.go

@@ -30,6 +30,9 @@ func newDeducerTrie() *deducerTrie {
 	}
 }
 
+// Suppress unused warning.
+var _ = (*deducerTrie)(nil).Delete
+
 // Delete is used to delete a key, returning the previous value and if it was deleted
 func (t *deducerTrie) Delete(s string) (pathDeducer, bool) {
 	t.Lock()

internal/gps/vcs_repo.go

@@ -53,6 +53,10 @@ func getVCSRepo(s vcs.Type, ustr, path string) (r ctxRepo, err error) {
 		var repo *vcs.HgRepo
 		repo, err = vcs.NewHgRepo(ustr, path)
 		r = &hgRepo{repo}
+	case vcs.Svn:
+		var repo *vcs.SvnRepo
+		repo, err = vcs.NewSvnRepo(ustr, path)
+		r = &svnRepo{repo}
 	}
 
 	return
@@ -218,7 +222,7 @@ func (r *svnRepo) get(ctx context.Context) error {
 	return nil
 }
 
-func (r *svnRepo) update(ctx context.Context) error {
+func (r *svnRepo) fetch(ctx context.Context) error {
 	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "svn", "update")
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to update repository", err, string(out))

internal/gps/vcs_repo_test.go

@@ -171,7 +171,7 @@ func testSvnRepo(t *testing.T) {
 	}
 
 	// Perform an update which should take up back to the latest version.
-	err = repo.update(ctx)
+	err = repo.fetch(ctx)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/gps/vcs_source.go

@@ -570,11 +570,6 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) {
 	return vlist, nil
 }
 
-type repo struct {
-	// Object for direct repo interaction
-	r ctxRepo
-}
-
 // This func copied from Masterminds/vcs so we can exec our own commands
 func mergeEnvLists(in, out []string) []string {
 NextVar:

internal/gps/vcs_source_test.go

@@ -25,8 +25,7 @@ func TestSlowVcs(t *testing.T) {
 	t.Run("source-gateway", testSourceGateway)
 	t.Run("bzr-repo", testBzrRepo)
 	t.Run("bzr-source", testBzrSourceInteractions)
-	// TODO(kris-nova) re-enable syn-repo after gps is merged into dep
-	//t.Run("svn-repo", testSvnRepo)
+	t.Run("svn-repo", testSvnRepo)
 	// TODO(sdboyer) svn-source
 	t.Run("hg-repo", testHgRepo)
 	t.Run("hg-source", testHgSourceInteractions)

internal/gps/version.go

@@ -119,6 +119,9 @@ func (r Revision) String() string {
 	return string(r)
 }
 
+// ImpliedCaretString follows the same rules as String(), but in accordance with
+// the Constraint interface will always print a leading "=", as all Versions,
+// when acting as a Constraint, act as exact matches.
 func (r Revision) ImpliedCaretString() string {
 	return r.String()
 }

internal/gps/version_queue_test.go

@@ -5,11 +5,12 @@
 package gps
 
 import (
-	"fmt"
 	"testing"
+
+	"github.com/pkg/errors"
 )
 
-// just need a ListVersions method
+// just need a listVersions method
 type fakeBridge struct {
 	*bridge
 	vl []Version
@@ -27,10 +28,6 @@ func init() {
 	SortForUpgrade(fakevl)
 }
 
-func (fb *fakeBridge) ListVersions(id ProjectIdentifier) ([]PairedVersion, error) {
-	return nil, nil
-}
-
 func (fb *fakeBridge) listVersions(id ProjectIdentifier) ([]Version, error) {
 	// it's a fixture, we only ever do the one, regardless of id
 	return fb.vl, nil
@@ -40,11 +37,7 @@ type fakeFailBridge struct {
 	*bridge
 }
 
-var errVQ = fmt.Errorf("vqerr")
-
-func (fb *fakeFailBridge) ListVersions(id ProjectIdentifier) ([]PairedVersion, error) {
-	return nil, nil
-}
+var errVQ = errors.New("vqerr")
 
 func (fb *fakeFailBridge) listVersions(id ProjectIdentifier) ([]Version, error) {
 	return nil, errVQ
@@ -152,7 +145,7 @@ func TestVersionQueueAdvance(t *testing.T) {
 	}
 
 	for k, v := range fakevl[1:] {
-		err = vq.advance(fmt.Errorf("advancment fail for %s", fakevl[k]))
+		err = vq.advance(errors.Errorf("advancment fail for %s", fakevl[k]))
 		if err != nil {
 			t.Errorf("error on advancing vq from %s to %s", fakevl[k], v)
 			break
@@ -166,7 +159,7 @@ func TestVersionQueueAdvance(t *testing.T) {
 	if vq.isExhausted() {
 		t.Error("should not be exhausted until advancing 'past' the end")
 	}
-	if err = vq.advance(fmt.Errorf("final advance failure")); err != nil {
+	if err = vq.advance(errors.Errorf("final advance failure")); err != nil {
 		t.Errorf("should not error on advance, even past end, but got %s", err)
 	}
 
@@ -191,7 +184,7 @@ func TestVersionQueueAdvance(t *testing.T) {
 		t.Error("can't be exhausted, we aren't even 'allLoaded' yet")
 	}
 
-	err = vq.advance(fmt.Errorf("dequeue lockv"))
+	err = vq.advance(errors.Errorf("dequeue lockv"))
 	if err != nil {
 		t.Error("unexpected error when advancing past lockv", err)
 	} else {
@@ -203,7 +196,7 @@ func TestVersionQueueAdvance(t *testing.T) {
 		}
 	}
 
-	err = vq.advance(fmt.Errorf("dequeue prefv"))
+	err = vq.advance(errors.Errorf("dequeue prefv"))
 	if err != nil {
 		t.Error("unexpected error when advancing past prefv", err)
 	} else {

internal/test/integration/testproj.go

@@ -31,7 +31,6 @@ type RunFunc func(prog string, newargs []string, outW, errW io.Writer, dir strin
 // and content
 type TestProject struct {
 	t          *testing.T
-	h          *test.Helper
 	preImports []string
 	tempdir    string
 	env        []string