*: make errcheck work again (pingcap#8795)

.gitignore

@@ -18,3 +18,4 @@ cmd/explaintest/explain-test.out
 cmd/explaintest/explaintest_tidb-server
 _tools/
 *.fail.go
+tools/bin/

Makefile

@@ -59,12 +59,10 @@ dev: checklist test check
 build:
 	$(GOBUILD)
 
-# The retool tools.json is setup from hack/retool-install.sh
-check-setup:
-	@which retool >/dev/null 2>&1 || go get github.com/twitchtv/retool
-	@GO111MODULE=off retool sync
+# Install the check tools.
+check-setup:tools/bin/megacheck tools/bin/revive tools/bin/goword tools/bin/gometalinter tools/bin/gosec
 
-check: check-setup fmt lint vet
+check: fmt errcheck lint
 
 # These need to be fixed before they can be ran regularly
 check-fail: goword check-static check-slow
@@ -73,26 +71,33 @@ fmt:
 	@echo "gofmt (simplify)"
 	@gofmt -s -l -w $(FILES) 2>&1 | $(FAIL_ON_STDOUT)
 
-goword:
-	retool do goword $(FILES) 2>&1 | $(FAIL_ON_STDOUT)
+goword:tools/bin/goword
+	tools/bin/goword $(FILES) 2>&1 | $(FAIL_ON_STDOUT)
 
-check-static:
+gosec:tools/bin/gosec
+	tools/bin/gosec $$($(PACKAGE_DIRECTORIES))
+
+check-static:tools/bin/gometalinter
 	@ # vet and fmt have problems with vendor when ran through metalinter
-	CGO_ENABLED=0 retool do gometalinter.v2 --disable-all --deadline 120s \
+	tools/bin/gometalinter --disable-all --deadline 120s \
 	  --enable misspell \
 	  --enable megacheck \
 	  --enable ineffassign \
 	  $$($(PACKAGE_DIRECTORIES))
 
-check-slow:
-	CGO_ENABLED=0 retool do gometalinter.v2 --disable-all \
+check-slow:tools/bin/gometalinter tools/bin/gosec
+	tools/bin/gometalinter --disable-all \
 	  --enable errcheck \
 	  $$($(PACKAGE_DIRECTORIES))
-	CGO_ENABLED=0 retool do gosec $$($(PACKAGE_DIRECTORIES))
 
-lint:
+errcheck:tools/bin/errcheck
+	@echo "errcheck"
+	@$(GO) mod vendor
+	@tools/bin/errcheck -exclude ./tools/check/errcheck_excludes.txt -blank $(PACKAGES) | grep -v "_test\.go" | awk '{print} END{if(NR>0) {exit 1}}'
+
+lint:tools/bin/revive
 	@echo "linting"
-	@CGO_ENABLED=0 retool do revive -formatter friendly -config revive.toml $(PACKAGES)
+	@tools/bin/revive -formatter friendly -config tools/check/revive.toml $(FILES)
 
 vet:
 	@echo "vet"
@@ -194,3 +199,27 @@ gofail-enable:
 gofail-disable:
 # Restoring gofail failpoints...
 	@$(GOFAIL_DISABLE)
+
+tools/bin/megacheck:
+	cd tools/check; \
+	$go build -o ../bin/megacheck honnef.co/go/tools/cmd/megacheck
+
+tools/bin/revive:
+	cd tools/check; \
+	$(GO) build -o ../bin/revive github.com/mgechev/revive
+
+tools/bin/goword:
+	cd tools/check; \
+	$(GO) build -o ../bin/goword github.com/chzchzchz/goword
+
+tools/bin/gometalinter:
+	cd tools/check; \
+	$(GO) build -o ../bin/gometalinter gopkg.in/alecthomas/gometalinter.v2
+
+tools/bin/gosec:
+	cd tools/check; \
+	$(GO) build -o ../bin/gosec github.com/securego/gosec/cmd/gosec
+
+tools/bin/errcheck:
+	cd tools/check; \
+	$(GO) build -o ../bin/errcheck github.com/kisielk/errcheck

ddl/ddl_api.go

@@ -36,6 +36,7 @@ import (
 	"github.com/pingcap/tidb/sessionctx/variable"
 	"github.com/pingcap/tidb/table"
 	"github.com/pingcap/tidb/types"
+	log "github.com/sirupsen/logrus"
 )
 
 func (d *ddl) CreateSchema(ctx sessionctx.Context, schema model.CIStr, charsetInfo *ast.CharsetOpt) (err error) {
@@ -474,9 +475,14 @@ func setTimestampDefaultValue(c *table.Column, hasDefaultValue bool, setOnUpdate
 	// For timestamp Col, if is not set default value or not set null, use current timestamp.
 	if mysql.HasTimestampFlag(c.Flag) && mysql.HasNotNullFlag(c.Flag) {
 		if setOnUpdateNow {
-			c.SetDefaultValue(types.ZeroDatetimeStr)
+			if err := c.SetDefaultValue(types.ZeroDatetimeStr); err != nil {
+				log.Error(errors.ErrorStack(err))
+			}
 		} else {
-			c.SetDefaultValue(strings.ToUpper(ast.CurrentTimestamp))
+			if err := c.SetDefaultValue(strings.ToUpper(ast.CurrentTimestamp)); err != nil {
+				log.Error(errors.ErrorStack(err))
+			}
+
 		}
 	}
 }

domain/domain.go

@@ -393,7 +393,9 @@ func (do *Domain) infoSyncerKeeper() {
 		select {
 		case <-do.info.Done():
 			log.Info("[ddl] server info syncer need to restart")
-			do.info.Restart(context.Background())
+			if err := do.info.Restart(context.Background()); err != nil {
+				log.Error(err)
+			}
 			log.Info("[ddl] server info syncer restarted.")
 		case <-do.exit:
 			return

executor/aggregate.go

@@ -473,7 +473,9 @@ func (w *HashAggFinalWorker) getFinalResult(sctx sessionctx.Context) {
 	for groupKey := range w.groupSet {
 		partialResults := w.getPartialResult(sctx.GetSessionVars().StmtCtx, []byte(groupKey), w.partialResultMap)
 		for i, af := range w.aggFuncs {
-			af.AppendFinalResult2Chunk(sctx, partialResults[i], result)
+			if err := af.AppendFinalResult2Chunk(sctx, partialResults[i], result); err != nil {
+				log.Error(errors.ErrorStack(err))
+			}
 		}
 		if len(w.aggFuncs) == 0 {
 			result.SetNumVirtualRows(result.NumRows() + 1)
@@ -655,7 +657,9 @@ func (e *HashAggExec) unparallelExec(ctx context.Context, chk *chunk.Chunk) erro
 			chk.SetNumVirtualRows(chk.NumRows() + 1)
 		}
 		for i, af := range e.PartialAggFuncs {
-			af.AppendFinalResult2Chunk(e.ctx, partialResults[i], chk)
+			if err := (af.AppendFinalResult2Chunk(e.ctx, partialResults[i], chk)); err != nil {
+				return err
+			}
 		}
 		if chk.NumRows() == e.maxChunkSize {
 			e.cursor4GroupKey++

executor/executor_test.go

@@ -238,12 +238,12 @@ func (s *testSuite) TestAdmin(c *C) {
 	c.Assert(err, IsNil)
 	err = txn.Commit(context.Background())
 	c.Assert(err, IsNil)
-	r, err_admin := tk.Exec("admin check table admin_test")
-	c.Assert(err_admin, NotNil)
+	r, errAdmin := tk.Exec("admin check table admin_test")
+	c.Assert(errAdmin, NotNil)
 
 	if config.CheckTableBeforeDrop {
 		r, err = tk.Exec("drop table admin_test")
-		c.Assert(err.Error(), Equals, err_admin.Error())
+		c.Assert(err.Error(), Equals, errAdmin.Error())
 
 		// Drop inconsistency index.
 		tk.MustExec("alter table admin_test drop index c1")

executor/explain.go

@@ -81,9 +81,13 @@ func (e *ExplainExec) generateExplainInfo(ctx context.Context) ([][]string, erro
 				break
 			}
 		}
-		e.analyzeExec.Close()
+		if err := e.analyzeExec.Close(); err != nil {
+			return nil, err
+		}
+	}
+	if err := e.explain.RenderResult(); err != nil {
+		return nil, err
 	}
-	e.explain.RenderResult()
 	if e.analyzeExec != nil {
 		e.ctx.GetSessionVars().StmtCtx.RuntimeStatsColl = nil
 	}

executor/index_lookup_join.go

@@ -146,9 +146,12 @@ func (e *IndexLookUpJoin) Open(ctx context.Context) error {
 	// The trick here is `getStartTS` will cache start ts in the dataReaderBuilder,
 	// so even txn is destroyed later, the dataReaderBuilder could still use the
 	// cached start ts to construct DAG.
-	e.innerCtx.readerBuilder.getStartTS()
+	_, err := e.innerCtx.readerBuilder.getStartTS()
+	if err != nil {
+		return err
+	}
 
-	err := e.children[0].Open(ctx)
+	err = e.children[0].Open(ctx)
 	if err != nil {
 		return errors.Trace(err)
 	}

executor/insert_common.go

@@ -205,7 +205,10 @@ func (e *InsertValues) handleErr(col *table.Column, val *types.Datum, rowIdx int
 		return types.ErrWarnDataOutOfRange.GenWithStackByArgs(col.Name.O, rowIdx+1)
 	}
 	if types.ErrTruncated.Equal(err) {
-		valStr, _ := val.ToString()
+		valStr, err1 := val.ToString()
+		if err1 != nil {
+			log.Warn(err1)
+		}
 		return table.ErrTruncatedWrongValueForField.GenWithStackByArgs(types.TypeStr(col.Tp), valStr, col.Name.O, rowIdx+1)
 	}
 	return e.filterErr(err)
@@ -310,7 +313,9 @@ func (e *InsertValues) insertRowsFromSelect(ctx context.Context, exec func(rows
 				if err := exec(rows); err != nil {
 					return errors.Trace(err)
 				}
-				e.ctx.StmtCommit()
+				if err := e.ctx.StmtCommit(); err != nil {
+					return errors.Trace(err)
+				}
 				rows = rows[:0]
 				if err := e.ctx.NewTxn(); err != nil {
 					// We should return a special error for batch insert.

executor/merge_join_test.go

@@ -215,8 +215,8 @@ const plan3 = `[[TableScan_12 {
 } ]]`
 
 func checkMergeAndRun(tk *testkit.TestKit, c *C, sql string) *testkit.Result {
-	explainedSql := "explain " + sql
-	result := tk.MustQuery(explainedSql)
+	explainedSQL := "explain " + sql
+	result := tk.MustQuery(explainedSQL)
 	resultStr := fmt.Sprintf("%v", result.Rows())
 	if !strings.ContainsAny(resultStr, "MergeJoin") {
 		c.Error("Expected MergeJoin in plannercore.")
@@ -225,8 +225,8 @@ func checkMergeAndRun(tk *testkit.TestKit, c *C, sql string) *testkit.Result {
 }
 
 func checkPlanAndRun(tk *testkit.TestKit, c *C, plan string, sql string) *testkit.Result {
-	explainedSql := "explain " + sql
-	result := tk.MustQuery(explainedSql)
+	explainedSQL := "explain " + sql
+	result := tk.MustQuery(explainedSQL)
 	resultStr := fmt.Sprintf("%v", result.Rows())
 	if plan != resultStr {
 		// TODO: Reopen it after refactoring explain.

executor/prepared_test.go

@@ -71,55 +71,55 @@ func (s *testSuite) TestPrepared(c *C) {
 		result := tk.MustQuery("select distinct c1, c2 from prepare_test where c1 = ?", 1)
 		result.Check(testkit.Rows("1 <nil>"))
 
-		// Call Session PrepareStmt directly to get stmtId.
+		// Call Session PrepareStmt directly to get stmtID.
 		query := "select c1, c2 from prepare_test where c1 = ?"
-		stmtId, _, _, err := tk.Se.PrepareStmt(query)
+		stmtID, _, _, err := tk.Se.PrepareStmt(query)
 		c.Assert(err, IsNil)
-		rs, err := tk.Se.ExecutePreparedStmt(ctx, stmtId, 1)
+		rs, err := tk.Se.ExecutePreparedStmt(ctx, stmtID, 1)
 		c.Assert(err, IsNil)
 		tk.ResultSetToResult(rs, Commentf("%v", rs)).Check(testkit.Rows("1 <nil>"))
 
 		tk.MustExec("delete from prepare_test")
 		query = "select c1 from prepare_test where c1 = (select c1 from prepare_test where c1 = ?)"
-		stmtId, _, _, err = tk.Se.PrepareStmt(query)
+		stmtID, _, _, err = tk.Se.PrepareStmt(query)
 		tk1 := testkit.NewTestKitWithInit(c, s.store)
 		tk1.MustExec("insert prepare_test (c1) values (3)")
-		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 3)
+		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 3)
 		c.Assert(err, IsNil)
 		tk.ResultSetToResult(rs, Commentf("%v", rs)).Check(testkit.Rows("3"))
 
 		tk.MustExec("delete from prepare_test")
 		query = "select c1 from prepare_test where c1 = (select c1 from prepare_test where c1 = ?)"
-		stmtId, _, _, err = tk.Se.PrepareStmt(query)
-		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 3)
+		stmtID, _, _, err = tk.Se.PrepareStmt(query)
+		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 3)
 		c.Assert(err, IsNil)
 		tk1.MustExec("insert prepare_test (c1) values (3)")
-		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 3)
+		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 3)
 		c.Assert(err, IsNil)
 		tk.ResultSetToResult(rs, Commentf("%v", rs)).Check(testkit.Rows("3"))
 
 		tk.MustExec("delete from prepare_test")
 		query = "select c1 from prepare_test where c1 in (select c1 from prepare_test where c1 = ?)"
-		stmtId, _, _, err = tk.Se.PrepareStmt(query)
-		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 3)
+		stmtID, _, _, err = tk.Se.PrepareStmt(query)
+		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 3)
 		c.Assert(err, IsNil)
 		tk1.MustExec("insert prepare_test (c1) values (3)")
-		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 3)
+		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 3)
 		c.Assert(err, IsNil)
 		tk.ResultSetToResult(rs, Commentf("%v", rs)).Check(testkit.Rows("3"))
 
 		tk.MustExec("begin")
 		tk.MustExec("insert prepare_test (c1) values (4)")
 		query = "select c1, c2 from prepare_test where c1 = ?"
-		stmtId, _, _, err = tk.Se.PrepareStmt(query)
+		stmtID, _, _, err = tk.Se.PrepareStmt(query)
 		c.Assert(err, IsNil)
 		tk.MustExec("rollback")
-		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 4)
+		rs, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 4)
 		c.Assert(err, IsNil)
 		tk.ResultSetToResult(rs, Commentf("%v", rs)).Check(testkit.Rows())
 
 		// Check that ast.Statement created by executor.CompileExecutePreparedStmt has query text.
-		stmt, err := executor.CompileExecutePreparedStmt(tk.Se, stmtId, 1)
+		stmt, err := executor.CompileExecutePreparedStmt(tk.Se, stmtID, 1)
 		c.Assert(err, IsNil)
 		c.Assert(stmt.OriginText(), Equals, query)
 
@@ -139,19 +139,19 @@ func (s *testSuite) TestPrepared(c *C) {
 		tk.Exec("create table prepare2 (a int)")
 
 		// Should success as the changed schema do not affect the prepared statement.
-		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 1)
+		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 1)
 		c.Assert(err, IsNil)
 
 		// Drop a column so the prepared statement become invalid.
 		query = "select c1, c2 from prepare_test where c1 = ?"
-		stmtId, _, _, err = tk.Se.PrepareStmt(query)
+		stmtID, _, _, err = tk.Se.PrepareStmt(query)
 		tk.MustExec("alter table prepare_test drop column c2")
 
-		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 1)
+		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 1)
 		c.Assert(plannercore.ErrUnknownColumn.Equal(err), IsTrue)
 
 		tk.MustExec("drop table prepare_test")
-		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 1)
+		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 1)
 		c.Assert(plannercore.ErrSchemaChanged.Equal(err), IsTrue)
 
 		// issue 3381
@@ -219,13 +219,13 @@ func (s *testSuite) TestPrepared(c *C) {
 		exec.Close()
 
 		// issue 8065
-		stmtId, _, _, err = tk.Se.PrepareStmt("select ? from dual")
+		stmtID, _, _, err = tk.Se.PrepareStmt("select ? from dual")
 		c.Assert(err, IsNil)
-		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 1)
+		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 1)
 		c.Assert(err, IsNil)
-		stmtId, _, _, err = tk.Se.PrepareStmt("update prepare1 set a = ? where a = ?")
+		stmtID, _, _, err = tk.Se.PrepareStmt("update prepare1 set a = ? where a = ?")
 		c.Assert(err, IsNil)
-		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtId, 1, 1)
+		_, err = tk.Se.ExecutePreparedStmt(ctx, stmtID, 1, 1)
 		c.Assert(err, IsNil)
 	}
 }

executor/set_test.go

@@ -49,24 +49,24 @@ func (s *testSuite) TestSetVar(c *C) {
 	_, err := tk.Exec(testSQL)
 	c.Assert(err, NotNil)
 
-	errTestSql := "SET @@date_format = 1;"
-	_, err = tk.Exec(errTestSql)
+	errTestSQL := "SET @@date_format = 1;"
+	_, err = tk.Exec(errTestSQL)
 	c.Assert(err, NotNil)
 
-	errTestSql = "SET @@rewriter_enabled = 1;"
-	_, err = tk.Exec(errTestSql)
+	errTestSQL = "SET @@rewriter_enabled = 1;"
+	_, err = tk.Exec(errTestSQL)
 	c.Assert(err, NotNil)
 
-	errTestSql = "SET xxx = abcd;"
-	_, err = tk.Exec(errTestSql)
+	errTestSQL = "SET xxx = abcd;"
+	_, err = tk.Exec(errTestSQL)
 	c.Assert(err, NotNil)
 
-	errTestSql = "SET @@global.a = 1;"
-	_, err = tk.Exec(errTestSql)
+	errTestSQL = "SET @@global.a = 1;"
+	_, err = tk.Exec(errTestSQL)
 	c.Assert(err, NotNil)
 
-	errTestSql = "SET @@global.timestamp = 1;"
-	_, err = tk.Exec(errTestSql)
+	errTestSQL = "SET @@global.timestamp = 1;"
+	_, err = tk.Exec(errTestSQL)
 	c.Assert(err, NotNil)
 
 	// For issue 998