build: Enable gometalinter "gosimple" check, improve build.go

build.go

@@ -27,7 +27,6 @@ import (
 	"runtime"
 	"strconv"
 	"strings"
-	"syscall"
 	"text/template"
 	"time"
 )
@@ -154,6 +153,39 @@ var targets = map[string]target{
 	},
 }
 
+var (
+	// fast linters complete in a fraction of a second and might as well be
+	// run always as part of the build
+	fastLinters = []string{
+		"deadcode",
+		"golint",
+		"ineffassign",
+		"vet",
+	}
+
+	// slow linters take several seconds and are run only as part of the
+	// "metalint" command.
+	slowLinters = []string{
+		"gosimple",
+		"staticcheck",
+		"structcheck",
+		"unused",
+		"varcheck",
+	}
+
+	// Which parts of the tree to lint
+	lintDirs = []string{".", "./lib/...", "./cmd/..."}
+
+	// Messages to ignore
+	lintExcludes = []string{
+		".pb.go",
+		"should have comment",
+		"protocol.Vector composite literal uses unkeyed fields",
+		"Use DialContext instead",   // Go 1.7
+		"os.SEEK_SET is deprecated", // Go 1.7
+	}
+)
+
 func init() {
 	// The "syncthing" target includes a few more files found in the "etc"
 	// and "extra" dirs.
@@ -203,8 +235,6 @@ func main() {
 	// which is what you want for maximum error checking during development.
 	if flag.NArg() == 0 {
 		runCommand("install", targets["all"])
-		runCommand("vet", target{})
-		runCommand("lint", target{})
 	} else {
 		// with any command given but not a target, the target is
 		// "syncthing". So "go run build.go install" is "go run build.go install
@@ -242,6 +272,7 @@ func runCommand(cmd string, target target) {
 			tags = []string{"noupgrade"}
 		}
 		install(target, tags)
+		metalint(fastLinters, lintDirs)
 
 	case "build":
 		var tags []string
@@ -249,6 +280,7 @@ func runCommand(cmd string, target target) {
 			tags = []string{"noupgrade"}
 		}
 		build(target, tags)
+		metalint(fastLinters, lintDirs)
 
 	case "test":
 		test("./lib/...", "./cmd/...")
@@ -284,28 +316,14 @@ func runCommand(cmd string, target target) {
 		clean()
 
 	case "vet":
-		vet("build.go")
-		vet("cmd", "lib")
+		metalint(fastLinters, lintDirs)
 
 	case "lint":
-		lint(".")
-		lint("./cmd/...")
-		lint("./lib/...")
+		metalint(fastLinters, lintDirs)
 
 	case "metalint":
-		if isGometalinterInstalled() {
-			dirs := []string{".", "./cmd/...", "./lib/..."}
-			ok := gometalinter("deadcode", dirs, "test/util.go")
-			ok = gometalinter("structcheck", dirs) && ok
-			ok = gometalinter("varcheck", dirs) && ok
-			ok = gometalinter("ineffassign", dirs) && ok
-			ok = gometalinter("unused", dirs) && ok
-			ok = gometalinter("staticcheck", dirs) && ok
-			ok = gometalinter("unconvert", dirs) && ok
-			if !ok {
-				os.Exit(1)
-			}
-		}
+		metalint(fastLinters, lintDirs)
+		metalint(slowLinters, lintDirs)
 
 	case "version":
 		fmt.Println(getVersion())
@@ -373,6 +391,7 @@ func setup() {
 		"github.com/tsenart/deadcode",
 		"golang.org/x/net/html",
 		"golang.org/x/tools/cmd/cover",
+		"honnef.co/go/simple/cmd/gosimple",
 		"honnef.co/go/staticcheck/cmd/staticcheck",
 		"honnef.co/go/unused/cmd/unused",
 	}
@@ -1009,48 +1028,6 @@ func zipFile(out string, files []archiveFile) {
 	}
 }
 
-func vet(dirs ...string) {
-	params := []string{"tool", "vet", "-all"}
-	params = append(params, dirs...)
-	bs, err := runError("go", params...)
-
-	if len(bs) > 0 {
-		log.Printf("%s", bs)
-	}
-
-	if err != nil {
-		if exitStatus(err) == 3 {
-			// Exit code 3, the "vet" tool is not installed
-			return
-		}
-
-		// A genuine error exit from the vet tool.
-		log.Fatal(err)
-	}
-}
-
-func lint(pkg string) {
-	bs, err := runError("golint", pkg)
-	if err != nil {
-		log.Println(`- No golint, not linting. Try "go get -u github.com/golang/lint/golint".`)
-		return
-	}
-
-	analCommentPolicy := regexp.MustCompile(`exported (function|method|const|type|var) [^\s]+ should have comment`)
-	for _, line := range strings.Split(string(bs), "\n") {
-		if line == "" {
-			continue
-		}
-		if analCommentPolicy.MatchString(line) {
-			continue
-		}
-		if strings.Contains(line, ".pb.go:") {
-			continue
-		}
-		log.Println(line)
-	}
-}
-
 func macosCodesign(file string) {
 	if pass := os.Getenv("CODESIGN_KEYCHAIN_PASS"); pass != "" {
 		bs, err := runError("security", "unlock-keychain", "-p", pass)
@@ -1070,14 +1047,16 @@ func macosCodesign(file string) {
 	}
 }
 
-func exitStatus(err error) int {
-	if err, ok := err.(*exec.ExitError); ok {
-		if ws, ok := err.ProcessState.Sys().(syscall.WaitStatus); ok {
-			return ws.ExitStatus()
+func metalint(linters []string, dirs []string) {
+	ok := true
+	if isGometalinterInstalled() {
+		if !gometalinter(linters, dirs, lintExcludes...) {
+			ok = false
 		}
 	}
-
-	return -1
+	if !ok {
+		log.Fatal("Build succeeded, but there were lint warnings")
+	}
 }
 
 func isGometalinterInstalled() bool {
@@ -1088,10 +1067,12 @@ func isGometalinterInstalled() bool {
 	return true
 }
 
-func gometalinter(linter string, dirs []string, excludes ...string) bool {
-	params := []string{"--disable-all"}
-	params = append(params, fmt.Sprintf("--deadline=%ds", 60))
-	params = append(params, "--enable="+linter)
+func gometalinter(linters []string, dirs []string, excludes ...string) bool {
+	params := []string{"--disable-all", "--concurrency=2", "--deadline=300s"}
+
+	for _, linter := range linters {
+		params = append(params, "--enable="+linter)
+	}
 
 	for _, exclude := range excludes {
 		params = append(params, "--exclude="+exclude)
@@ -1104,14 +1085,19 @@ func gometalinter(linter string, dirs []string, excludes ...string) bool {
 	bs, _ := runError("gometalinter", params...)
 
 	nerr := 0
+	lines := make(map[string]struct{})
 	for _, line := range strings.Split(string(bs), "\n") {
 		if line == "" {
 			continue
 		}
-		if strings.Contains(line, ".pb.go:") {
+		if _, ok := lines[line]; ok {
 			continue
 		}
 		log.Println(line)
+		if strings.Contains(line, "executable file not found") {
+			log.Println(` - Try "go run build.go setup" to install missing tools`)
+		}
+		lines[line] = struct{}{}
 		nerr++
 	}
 

cmd/stcli/cmd_general.go

@@ -62,9 +62,9 @@ func generalID(c *cli.Context) {
 
 func generalStatus(c *cli.Context) {
 	response := httpGet(c, "system/config/insync")
-	status := make(map[string]interface{})
+	var status struct{ ConfigInSync bool }
 	json.Unmarshal(responseToBArray(response), &status)
-	if status["configInSync"] != true {
+	if !status.ConfigInSync {
 		die("Config out of sync")
 	}
 	fmt.Println("Config in sync")

cmd/stgenfiles/main.go

@@ -85,12 +85,7 @@ func generateOneFile(fd io.ReadSeeker, p1 string, s int64) error {
 	_ = os.Chmod(p1, os.FileMode(rand.Intn(0777)|0400))
 
 	t := time.Now().Add(-time.Duration(rand.Intn(30*86400)) * time.Second)
-	err = os.Chtimes(p1, t, t)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return os.Chtimes(p1, t, t)
 }
 
 func randomName() string {

cmd/syncthing/gui.go

@@ -381,10 +381,8 @@ func (s *apiService) String() string {
 }
 
 func (s *apiService) VerifyConfiguration(from, to config.Configuration) error {
-	if _, err := net.ResolveTCPAddr("tcp", to.GUI.Address()); err != nil {
-		return err
-	}
-	return nil
+	_, err := net.ResolveTCPAddr("tcp", to.GUI.Address())
+	return err
 }
 
 func (s *apiService) CommitConfiguration(from, to config.Configuration) bool {

lib/config/folderconfiguration.go

@@ -99,10 +99,7 @@ func (f *FolderConfiguration) CreateMarker() error {
 
 func (f *FolderConfiguration) HasMarker() bool {
 	_, err := os.Stat(filepath.Join(f.Path(), ".stfolder"))
-	if err != nil {
-		return false
-	}
-	return true
+	return err == nil
 }
 
 func (f FolderConfiguration) Description() string {

lib/db/blockmap_test.go

@@ -58,10 +58,7 @@ func setup() (*Instance, *BlockFinder) {
 func dbEmpty(db *Instance) bool {
 	iter := db.NewIterator(util.BytesPrefix([]byte{KeyTypeBlock}), nil)
 	defer iter.Release()
-	if iter.Next() {
-		return false
-	}
-	return true
+	return !iter.Next()
 }
 
 func TestBlockMapAddUpdateWipe(t *testing.T) {

lib/db/concurrency_test.go

@@ -4,7 +4,8 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this file,
 // You can obtain one at http://mozilla.org/MPL/2.0/.
 
-// +build ignore // this  is a really tedious test for an old issue
+// this  is a really tedious test for an old issue
+// +build ignore
 
 package db_test
 

lib/discover/local.go

@@ -51,7 +51,7 @@ func NewLocal(id protocol.DeviceID, addr string, addrList AddressLister) (Finder
 		Supervisor:      suture.NewSimple("local"),
 		myID:            id,
 		addrList:        addrList,
-		localBcastTick:  time.Tick(BroadcastInterval),
+		localBcastTick:  time.NewTicker(BroadcastInterval).C,
 		forcedBcastTick: make(chan time.Time),
 		localBcastStart: time.Now(),
 		cache:           newCache(),

lib/events/events_test.go

@@ -247,22 +247,23 @@ func BenchmarkBufferedSub(b *testing.B) {
 	}
 
 	// Receive the events
-	done := make(chan struct{})
+	done := make(chan error)
 	go func() {
-		defer close(done)
 		recv := 0
 		var evs []Event
 		for i := 0; i < b.N; {
 			evs = bs.Since(recv, evs[:0])
 			for _, ev := range evs {
 				if ev.GlobalID != recv+1 {
-					b.Fatal("skipped event", ev.GlobalID, recv)
+					done <- fmt.Errorf("skipped event %v %v", ev.GlobalID, recv)
+					return
 				}
 				recv = ev.GlobalID
 				coord <- struct{}{}
 			}
 			i += len(evs)
 		}
+		done <- nil
 	}()
 
 	// Send the events
@@ -276,7 +277,9 @@ func BenchmarkBufferedSub(b *testing.B) {
 		<-coord
 	}
 
-	<-done
+	if err := <-done; err != nil {
+		b.Error(err)
+	}
 	b.ReportAllocs()
 }
 

lib/ignore/cache_test.go

@@ -15,7 +15,7 @@ func TestCache(t *testing.T) {
 	c := newCache(nil)
 
 	res, ok := c.get("nonexistent")
-	if res.IsIgnored() || res.IsDeletable() || ok != false {
+	if res.IsIgnored() || res.IsDeletable() || ok {
 		t.Errorf("res %v, ok %v for nonexistent item", res, ok)
 	}
 
@@ -25,12 +25,12 @@ func TestCache(t *testing.T) {
 	c.set("false", 0)
 
 	res, ok = c.get("true")
-	if !res.IsIgnored() || !res.IsDeletable() || ok != true {
+	if !res.IsIgnored() || !res.IsDeletable() || !ok {
 		t.Errorf("res %v, ok %v for true item", res, ok)
 	}
 
 	res, ok = c.get("false")
-	if res.IsIgnored() || res.IsDeletable() || ok != true {
+	if res.IsIgnored() || res.IsDeletable() || !ok {
 		t.Errorf("res %v, ok %v for false item", res, ok)
 	}
 
@@ -41,12 +41,12 @@ func TestCache(t *testing.T) {
 	// Same values should exist
 
 	res, ok = c.get("true")
-	if !res.IsIgnored() || !res.IsDeletable() || ok != true {
+	if !res.IsIgnored() || !res.IsDeletable() || !ok {
 		t.Errorf("res %v, ok %v for true item", res, ok)
 	}
 
 	res, ok = c.get("false")
-	if res.IsIgnored() || res.IsDeletable() || ok != true {
+	if res.IsIgnored() || res.IsDeletable() || !ok {
 		t.Errorf("res %v, ok %v for false item", res, ok)
 	}
 

lib/model/util.go

@@ -37,6 +37,7 @@ func (d *deadlockDetector) Watch(name string, mut sync.Locker) {
 
 			go func() {
 				mut.Lock()
+				_ = 1 // empty critical section
 				mut.Unlock()
 				ok <- true
 			}()

lib/osutil/sync.go

@@ -22,10 +22,7 @@ func SyncFile(path string) error {
 	}
 	defer fd.Close()
 	// MacOS and Windows do not flush the disk cache
-	if err := fd.Sync(); err != nil {
-		return err
-	}
-	return nil
+	return fd.Sync()
 }
 
 func SyncDir(path string) error {

lib/protocol/bufferpool.go

@@ -17,10 +17,10 @@ func (p *bufferPool) get(size int) []byte {
 		return p.new(size)
 	}
 
-	bs := intf.([]byte)
+	bs := *intf.(*[]byte)
 	if cap(bs) < size {
 		// Buffer was too small, leave it for someone else and allocate.
-		p.put(bs)
+		p.pool.Put(intf)
 		return p.new(size)
 	}
 
@@ -43,7 +43,7 @@ func (p *bufferPool) upgrade(bs []byte, size int) []byte {
 
 // put returns the buffer to the pool
 func (p *bufferPool) put(bs []byte) {
-	p.pool.Put(bs)
+	p.pool.Put(&bs)
 }
 
 // new creates a new buffer of the requested size, taking the minimum

lib/protocol/hello.go

@@ -105,11 +105,7 @@ func readHello(c io.Reader) (HelloResult, error) {
 		if err := hello.UnmarshalXDR(buf); err != nil {
 			return HelloResult{}, err
 		}
-		res := HelloResult{
-			DeviceName:    hello.DeviceName,
-			ClientName:    hello.ClientName,
-			ClientVersion: hello.ClientVersion,
-		}
+		res := HelloResult(hello)
 		return res, ErrTooOldVersion13
 
 	case 0x00010001, 0x00010000:

lib/protocol/protocol.go

@@ -516,7 +516,7 @@ func (c *rawConnection) handleRequest(req Request) {
 	var done chan struct{}
 
 	if usePool {
-		buf = c.pool.Get().([]byte)[:size]
+		buf = (*c.pool.Get().(*[]byte))[:size]
 		done = make(chan struct{})
 	} else {
 		buf = make([]byte, size)
@@ -539,7 +539,7 @@ func (c *rawConnection) handleRequest(req Request) {
 
 	if usePool {
 		<-done
-		c.pool.Put(buf)
+		c.pool.Put(&buf)
 	}
 }
 
@@ -762,11 +762,12 @@ func (c *rawConnection) close(err error) {
 // results in an effecting ping interval of somewhere between
 // PingSendInterval/2 and PingSendInterval.
 func (c *rawConnection) pingSender() {
-	ticker := time.Tick(PingSendInterval / 2)
+	ticker := time.NewTicker(PingSendInterval / 2)
+	defer ticker.Stop()
 
 	for {
 		select {
-		case <-ticker:
+		case <-ticker.C:
 			d := time.Since(c.cw.Last())
 			if d < PingSendInterval/2 {
 				l.Debugln(c.id, "ping skipped after wr", d)
@@ -786,11 +787,12 @@ func (c *rawConnection) pingSender() {
 // but we expect pings in the absence of other messages) within the last
 // ReceiveTimeout. If not, we close the connection with an ErrTimeout.
 func (c *rawConnection) pingReceiver() {
-	ticker := time.Tick(ReceiveTimeout / 2)
+	ticker := time.NewTicker(ReceiveTimeout / 2)
+	defer ticker.Stop()
 
 	for {
 		select {
-		case <-ticker:
+		case <-ticker.C:
 			d := time.Since(c.cr.Last())
 			if d > ReceiveTimeout {
 				l.Debugln(c.id, "ping timeout", d)

lib/sync/sync.go

@@ -80,7 +80,7 @@ func (h holder) String() string {
 	if h.at == "" {
 		return "not held"
 	}
-	return fmt.Sprintf("at %s goid: %d for %s", h.at, h.goid, time.Now().Sub(h.time))
+	return fmt.Sprintf("at %s goid: %d for %s", h.at, h.goid, time.Since(h.time))
 }
 
 type loggedMutex struct {
@@ -95,7 +95,7 @@ func (m *loggedMutex) Lock() {
 
 func (m *loggedMutex) Unlock() {
 	currentHolder := m.holder.Load().(holder)
-	duration := time.Now().Sub(currentHolder.time)
+	duration := time.Since(currentHolder.time)
 	if duration >= threshold {
 		l.Debugf("Mutex held for %v. Locked at %s unlocked at %s", duration, currentHolder.at, getHolder().at)
 	}
@@ -147,7 +147,7 @@ func (m *loggedRWMutex) Lock() {
 
 func (m *loggedRWMutex) Unlock() {
 	currentHolder := m.holder.Load().(holder)
-	duration := time.Now().Sub(currentHolder.time)
+	duration := time.Since(currentHolder.time)
 	if duration >= threshold {
 		l.Debugf("RWMutex held for %v. Locked at %s unlocked at %s", duration, currentHolder.at, getHolder().at)
 	}
@@ -201,7 +201,7 @@ type loggedWaitGroup struct {
 func (wg *loggedWaitGroup) Wait() {
 	start := time.Now()
 	wg.WaitGroup.Wait()
-	duration := time.Now().Sub(start)
+	duration := time.Since(start)
 	if duration >= threshold {
 		l.Debugf("WaitGroup took %v at %s", duration, getHolder())
 	}

lib/util/utils_test.go

@@ -22,7 +22,7 @@ func TestSetDefaults(t *testing.T) {
 		t.Error("int failed")
 	} else if x.C != 0 {
 		t.Errorf("float failed")
-	} else if x.D != false {
+	} else if x.D {
 		t.Errorf("bool failed")
 	}
 
@@ -36,7 +36,7 @@ func TestSetDefaults(t *testing.T) {
 		t.Error("int failed")
 	} else if x.C != 2.2 {
 		t.Errorf("float failed")
-	} else if x.D != true {
+	} else if !x.D {
 		t.Errorf("bool failed")
 	}
 }

lib/versioner/staggered.go

@@ -73,11 +73,14 @@ func NewStaggered(folderID, folderPath string, params map[string]string) Version
 	l.Debugf("instantiated %#v", s)
 
 	go func() {
+		// TODO: This should be converted to a Serve() method.
 		s.clean()
 		if testCleanDone != nil {
 			close(testCleanDone)
 		}
-		for range time.Tick(time.Duration(cleanInterval) * time.Second) {
+		tck := time.NewTicker(time.Duration(cleanInterval) * time.Second)
+		defer tck.Stop()
+		for range tck.C {
 			s.clean()
 		}
 	}()

test/sync_test.go

@@ -17,6 +17,8 @@ import (
 	"testing"
 	"time"
 
+	"io"
+
 	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/rc"