Merge pull request etcd-io#16608 from fuweid/fix-shadow-vet

*: fix govet-shadow lint

Makefile

@@ -64,7 +64,8 @@ fuzz:
 
 verify: verify-gofmt verify-bom verify-lint verify-dep verify-shellcheck verify-goword \
 	verify-govet verify-license-header verify-receiver-name verify-mod-tidy verify-shellcheck \
-	verify-shellws verify-proto-annotations verify-genproto verify-goimport verify-yamllint
+	verify-shellws verify-proto-annotations verify-genproto verify-goimport verify-yamllint \
+	verify-govet-shadow
 fix: fix-goimports fix-bom fix-lint fix-yamllint
 	./scripts/fix.sh
 
@@ -140,6 +141,10 @@ fix-goimports:
 verify-yamllint:
 	yamllint --config-file tools/.yamllint .
 
+.PHONY: verify-govet-shadow
+verify-govet-shadow:
+	PASSES="govet_shadow" ./scripts/test.sh
+
 YAMLFMT_VERSION = $(shell cd tools/mod && go list -m -f '{{.Version}}' github.com/google/yamlfmt)
 
 .PHONY: fix-yamllint

client/v3/lease.go

@@ -409,9 +409,9 @@ func (l *lessor) keepAliveOnce(ctx context.Context, id LeaseID) (karesp *LeaseKe
 	}
 
 	defer func() {
-		if err := stream.CloseSend(); err != nil {
+		if cerr := stream.CloseSend(); cerr != nil {
 			if ferr == nil {
-				ferr = toErr(ctx, err)
+				ferr = toErr(ctx, cerr)
 			}
 			return
 		}

client/v3/maintenance.go

@@ -244,18 +244,19 @@ func (m *maintenance) SnapshotWithVersion(ctx context.Context) (*SnapshotRespons
 	}
 	go func() {
 		// Saving response is blocking
-		err = m.save(resp, pw)
+		err := m.save(resp, pw)
 		if err != nil {
 			m.logAndCloseWithError(err, pw)
 			return
 		}
 		for {
-			resp, err := ss.Recv()
+			sresp, err := ss.Recv()
 			if err != nil {
 				m.logAndCloseWithError(err, pw)
 				return
 			}
-			err = m.save(resp, pw)
+
+			err = m.save(sresp, pw)
 			if err != nil {
 				m.logAndCloseWithError(err, pw)
 				return
@@ -267,7 +268,7 @@ func (m *maintenance) SnapshotWithVersion(ctx context.Context) (*SnapshotRespons
 		Header:   resp.GetHeader(),
 		Snapshot: &snapshotReadCloser{ctx: ctx, ReadCloser: pr},
 		Version:  resp.GetVersion(),
-	}, err
+	}, nil
 }
 
 func (m *maintenance) Snapshot(ctx context.Context) (io.ReadCloser, error) {
@@ -293,7 +294,7 @@ func (m *maintenance) Snapshot(ctx context.Context) (io.ReadCloser, error) {
 			}
 		}
 	}()
-	return &snapshotReadCloser{ctx: ctx, ReadCloser: pr}, err
+	return &snapshotReadCloser{ctx: ctx, ReadCloser: pr}, nil
 }
 
 func (m *maintenance) logAndCloseWithError(err error, pw *io.PipeWriter) {

etcdutl/snapshot/v3_snapshot.go

@@ -144,16 +144,16 @@ func (s *v3Manager) Status(dbPath string) (ds Status, err error) {
 			if b == nil {
 				return fmt.Errorf("cannot get hash of bucket %s", string(next))
 			}
-			if _, err := h.Write(next); err != nil {
+			if _, err = h.Write(next); err != nil {
 				return fmt.Errorf("cannot write bucket %s : %v", string(next), err)
 			}
 			iskeyb := (string(next) == "key")
-			if err := b.ForEach(func(k, v []byte) error {
-				if _, err := h.Write(k); err != nil {
-					return fmt.Errorf("cannot write to bucket %s", err.Error())
+			if err = b.ForEach(func(k, v []byte) error {
+				if _, herr := h.Write(k); herr != nil {
+					return fmt.Errorf("cannot write to bucket %s", herr.Error())
 				}
-				if _, err := h.Write(v); err != nil {
-					return fmt.Errorf("cannot write to bucket %s", err.Error())
+				if _, herr := h.Write(v); herr != nil {
+					return fmt.Errorf("cannot write to bucket %s", herr.Error())
 				}
 				if iskeyb {
 					rev := bytesToRev(k)

pkg/expect/expect_test.go

@@ -53,8 +53,7 @@ func TestExpectFuncTimeout(t *testing.T) {
 	go func() {
 		// It's enough to have "talkative" process to stuck in the infinite loop of reading
 		for {
-			err := ep.Send("new line\n")
-			if err != nil {
+			if serr := ep.Send("new line\n"); serr != nil {
 				return
 			}
 		}
@@ -67,7 +66,7 @@ func TestExpectFuncTimeout(t *testing.T) {
 
 	require.ErrorAs(t, err, &context.DeadlineExceeded)
 
-	if err := ep.Stop(); err != nil {
+	if err = ep.Stop(); err != nil {
 		t.Fatal(err)
 	}
 
@@ -111,7 +110,7 @@ func TestExpectFuncExitFailureStop(t *testing.T) {
 	require.Equal(t, 1, exitCode)
 	require.NoError(t, err)
 
-	if err := ep.Stop(); err != nil {
+	if err = ep.Stop(); err != nil {
 		t.Fatal(err)
 	}
 	err = ep.Close()

pkg/traceutil/trace.go

@@ -181,33 +181,33 @@ func (t *Trace) logInfo(threshold time.Duration) (string, []zap.Field) {
 	var steps []string
 	lastStepTime := t.startTime
 	for i := 0; i < len(t.steps); i++ {
-		step := t.steps[i]
+		tstep := t.steps[i]
 		// add subtrace common fields which defined at the beginning to each sub-steps
-		if step.isSubTraceStart {
+		if tstep.isSubTraceStart {
 			for j := i + 1; j < len(t.steps) && !t.steps[j].isSubTraceEnd; j++ {
-				t.steps[j].fields = append(step.fields, t.steps[j].fields...)
+				t.steps[j].fields = append(tstep.fields, t.steps[j].fields...)
 			}
 			continue
 		}
 		// add subtrace common fields which defined at the end to each sub-steps
-		if step.isSubTraceEnd {
+		if tstep.isSubTraceEnd {
 			for j := i - 1; j >= 0 && !t.steps[j].isSubTraceStart; j-- {
-				t.steps[j].fields = append(step.fields, t.steps[j].fields...)
+				t.steps[j].fields = append(tstep.fields, t.steps[j].fields...)
 			}
 			continue
 		}
 	}
 	for i := 0; i < len(t.steps); i++ {
-		step := t.steps[i]
-		if step.isSubTraceStart || step.isSubTraceEnd {
+		tstep := t.steps[i]
+		if tstep.isSubTraceStart || tstep.isSubTraceEnd {
 			continue
 		}
-		stepDuration := step.time.Sub(lastStepTime)
+		stepDuration := tstep.time.Sub(lastStepTime)
 		if stepDuration > threshold {
 			steps = append(steps, fmt.Sprintf("trace[%d] '%v' %s (duration: %v)",
-				traceNum, step.msg, writeFields(step.fields), stepDuration))
+				traceNum, tstep.msg, writeFields(tstep.fields), stepDuration))
 		}
-		lastStepTime = step.time
+		lastStepTime = tstep.time
 	}
 
 	fs := []zap.Field{zap.String("detail", writeFields(t.fields)),

scripts/test.sh

@@ -379,13 +379,45 @@ function govet_pass {
   run_for_modules generic_checker run go vet
 }
 
-function govet_shadow_pass {
-  # TODO: we should ignore the generated packages?
+function govet_shadow_per_package {
+  local shadow
+  shadow=$1
+
+  # skip grpc_gateway packages because
   #
   # stderr: etcdserverpb/gw/rpc.pb.gw.go:2100:3: declaration of "ctx" shadows declaration at line 2005
+  local skip_pkgs=(
+    "go.etcd.io/etcd/api/v3/etcdserverpb/gw"
+    "go.etcd.io/etcd/server/v3/etcdserver/api/v3lock/v3lockpb/gw"
+    "go.etcd.io/etcd/server/v3/etcdserver/api/v3election/v3electionpb/gw"
+  )
+
+  local pkgs=()
+  while IFS= read -r line; do
+    local in_skip_pkgs="false"
+
+    for pkg in "${skip_pkgs[@]}"; do
+      if [ "${pkg}" == "${line}" ]; then
+        in_skip_pkgs="true"
+        break
+      fi
+    done
+
+    if [ "${in_skip_pkgs}" == "true" ]; then
+      continue
+    fi
+
+    pkgs+=("${line}")
+  done < <(go list ./...)
+
+  run go vet -all -vettool="${shadow}" "${pkgs[@]}"
+}
+
+function govet_shadow_pass {
   local shadow
   shadow=$(tool_get_bin "golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow")
-  run_for_modules generic_checker run go vet -all -vettool="${shadow}"
+
+  run_for_modules generic_checker govet_shadow_per_package "${shadow}"
 }
 
 function unparam_pass {

server/embed/config_logging.go

@@ -246,7 +246,7 @@ func (logRotationConfig) Sync() error { return nil }
 
 // setupLogRotation initializes log rotation for a single file path target.
 func setupLogRotation(logOutputs []string, logRotateConfigJSON string) error {
-	var logRotationConfig logRotationConfig
+	var logRotationCfg logRotationConfig
 	outputFilePaths := 0
 	for _, v := range logOutputs {
 		switch v {
@@ -265,7 +265,7 @@ func setupLogRotation(logOutputs []string, logRotateConfigJSON string) error {
 		return ErrLogRotationInvalidLogOutput
 	}
 
-	if err := json.Unmarshal([]byte(logRotateConfigJSON), &logRotationConfig); err != nil {
+	if err := json.Unmarshal([]byte(logRotateConfigJSON), &logRotationCfg); err != nil {
 		var unmarshalTypeError *json.UnmarshalTypeError
 		var syntaxError *json.SyntaxError
 		switch {
@@ -278,8 +278,8 @@ func setupLogRotation(logOutputs []string, logRotateConfigJSON string) error {
 		}
 	}
 	zap.RegisterSink("rotate", func(u *url.URL) (zap.Sink, error) {
-		logRotationConfig.Filename = u.Path[1:]
-		return &logRotationConfig, nil
+		logRotationCfg.Filename = u.Path[1:]
+		return &logRotationCfg, nil
 	})
 	return nil
 }

server/embed/etcd.go

@@ -231,9 +231,9 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
 
 	if srvcfg.ExperimentalEnableDistributedTracing {
 		tctx := context.Background()
-		tracingExporter, err := newTracingExporter(tctx, cfg)
-		if err != nil {
-			return e, err
+		tracingExporter, terr := newTracingExporter(tctx, cfg)
+		if terr != nil {
+			return e, terr
 		}
 		e.tracingExporterShutdown = func() {
 			tracingExporter.Close(tctx)

server/embed/serve.go

@@ -153,7 +153,7 @@ func (sctx *serveCtx) serve(
 				Handler:  createAccessController(sctx.lg, s, httpmux),
 				ErrorLog: logger, // do not log user error
 			}
-			if err := configureHttpServer(srv, s.Cfg); err != nil {
+			if err = configureHttpServer(srv, s.Cfg); err != nil {
 				sctx.lg.Error("Configure http server failed", zap.Error(err))
 				return err
 			}

server/etcdserver/api/v3compactor/periodic_test.go

@@ -216,7 +216,7 @@ func TestPeriodicSkipRevNotChange(t *testing.T) {
 			fc.Advance(tb.getRetryInterval())
 		}
 
-		_, err := compactable.Wait(1)
+		_, err = compactable.Wait(1)
 		if err == nil {
 			t.Fatal(errors.New("should not compact since the revision not change"))
 		}

server/etcdserver/api/v3discovery/discovery.go

@@ -211,7 +211,7 @@ func (d *discovery) joinCluster(config string) (string, error) {
 		return "", err
 	}
 
-	if err := d.registerSelf(config); err != nil {
+	if err = d.registerSelf(config); err != nil {
 		return "", err
 	}
 

server/etcdserver/bootstrap_test.go

@@ -190,7 +190,7 @@ func TestBootstrapBackend(t *testing.T) {
 			}
 
 			if tt.prepareData != nil {
-				if err := tt.prepareData(cfg); err != nil {
+				if err = tt.prepareData(cfg); err != nil {
 					t.Fatalf("failed to prepare data, unexpected error: %v", err)
 				}
 			}

server/etcdserver/corrupt.go

@@ -477,8 +477,10 @@ func (s *EtcdServer) getPeerHashKVs(rev int64) []*peerHashKVResp {
 		respsLen := len(resps)
 		var lastErr error
 		for _, ep := range p.eps {
+			var resp *pb.HashKVResponse
+
 			ctx, cancel := context.WithTimeout(context.Background(), s.Cfg.ReqTimeout())
-			resp, lastErr := HashByRev(ctx, s.cluster.ID(), cc, ep, rev)
+			resp, lastErr = HashByRev(ctx, s.cluster.ID(), cc, ep, rev)
 			cancel()
 			if lastErr == nil {
 				resps = append(resps, &peerHashKVResp{peerInfo: p, resp: resp, err: nil})
@@ -535,7 +537,7 @@ func (h *hashKVHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	req := &pb.HashKVRequest{}
-	if err := json.Unmarshal(b, req); err != nil {
+	if err = json.Unmarshal(b, req); err != nil {
 		h.lg.Warn("failed to unmarshal request", zap.Error(err))
 		http.Error(w, "error unmarshalling request", http.StatusBadRequest)
 		return

server/storage/wal/testing/waltesting.go

@@ -49,7 +49,9 @@ func NewTmpWAL(t testing.TB, reqs []etcdserverpb.InternalRaftRequest) (*wal.WAL,
 		if err != nil {
 			t.Fatalf("Failed to open WAL: %v", err)
 		}
-		_, state, _, err := w.ReadAll()
+
+		var state raftpb.HardState
+		_, state, _, err = w.ReadAll()
 		if err != nil {
 			t.Fatalf("Failed to read WAL: %v", err)
 		}

server/storage/wal/version.go

@@ -161,7 +161,7 @@ func visitMessageDescriptor(md protoreflect.MessageDescriptor, visitor Visitor)
 
 	enums := md.Enums()
 	for i := 0; i < enums.Len(); i++ {
-		err := visitEnumDescriptor(enums.Get(i), visitor)
+		err = visitEnumDescriptor(enums.Get(i), visitor)
 		if err != nil {
 			return err
 		}

tests/common/alarm_test.go

@@ -61,7 +61,7 @@ func TestAlarm(t *testing.T) {
 		}
 
 		// check that Put is rejected when alarm is on
-		if err := cc.Put(ctx, "3rd_test", smallbuf, config.PutOptions{}); err != nil {
+		if err = cc.Put(ctx, "3rd_test", smallbuf, config.PutOptions{}); err != nil {
 			if !strings.Contains(err.Error(), "etcdserver: mvcc: database space exceeded") {
 				t.Fatal(err)
 			}

tests/e2e/corrupt_test.go

@@ -128,7 +128,7 @@ func TestInPlaceRecovery(t *testing.T) {
 	oldCc, err := e2e.NewEtcdctl(epcOld.Cfg.Client, epcOld.EndpointsGRPC())
 	assert.NoError(t, err)
 	for i := 0; i < 10; i++ {
-		err := oldCc.Put(ctx, testutil.PickKey(int64(i)), fmt.Sprint(i), config.PutOptions{})
+		err = oldCc.Put(ctx, testutil.PickKey(int64(i)), fmt.Sprint(i), config.PutOptions{})
 		assert.NoError(t, err, "error on put")
 	}
 
@@ -203,7 +203,7 @@ func TestPeriodicCheckDetectsCorruption(t *testing.T) {
 
 	cc := epc.Etcdctl()
 	for i := 0; i < 10; i++ {
-		err := cc.Put(ctx, testutil.PickKey(int64(i)), fmt.Sprint(i), config.PutOptions{})
+		err = cc.Put(ctx, testutil.PickKey(int64(i)), fmt.Sprint(i), config.PutOptions{})
 		assert.NoError(t, err, "error on put")
 	}
 
@@ -249,7 +249,7 @@ func TestCompactHashCheckDetectCorruption(t *testing.T) {
 
 	cc := epc.Etcdctl()
 	for i := 0; i < 10; i++ {
-		err := cc.Put(ctx, testutil.PickKey(int64(i)), fmt.Sprint(i), config.PutOptions{})
+		err = cc.Put(ctx, testutil.PickKey(int64(i)), fmt.Sprint(i), config.PutOptions{})
 		assert.NoError(t, err, "error on put")
 	}
 	members, err := cc.MemberList(ctx, false)
@@ -318,7 +318,7 @@ func TestCompactHashCheckDetectCorruptionInterrupt(t *testing.T) {
 	t.Log("putting 10 values to the identical key...")
 	cc := epc.Etcdctl()
 	for i := 0; i < 10; i++ {
-		err := cc.Put(ctx, "key", fmt.Sprint(i), config.PutOptions{})
+		err = cc.Put(ctx, "key", fmt.Sprint(i), config.PutOptions{})
 		require.NoError(t, err, "error on put")
 	}
 

tests/e2e/ctl_v3_snapshot_test.go

@@ -405,8 +405,8 @@ func TestRestoreCompactionRevBump(t *testing.T) {
 	t.Log("Ensuring the restored member has the correct data...")
 	hasKVs(t, ctl, kvs, currentRev, baseRev)
 	for i := range unsnappedKVs {
-		v, err := ctl.Get(context.Background(), unsnappedKVs[i].Key, config.GetOptions{})
-		require.NoError(t, err)
+		v, gerr := ctl.Get(context.Background(), unsnappedKVs[i].Key, config.GetOptions{})
+		require.NoError(t, gerr)
 		require.Equal(t, int64(0), v.Count)
 	}