vet: various cleanups

- Use staticcheck for pkg comments instead of special code.
- Remove golint use (it is deprecated & very slow).
- Re-add deprecation warnings and update exclusions.
- Add all checkers to staticcheck (e.g. mismatched receivers, underscores).
- Add a manual check for misspelled test functions that prevent running.

balancer/rls/internal/adaptive/adaptive_test.go

@@ -25,13 +25,13 @@ import (
 )
 
 // stats returns a tuple with accepts, throttles for the current time.
-func (th *Throttler) stats() (int64, int64) {
+func (t *Throttler) stats() (int64, int64) {
 	now := timeNowFunc()
 
-	th.mu.Lock()
-	a, t := th.accepts.sum(now), th.throttles.sum(now)
-	th.mu.Unlock()
-	return a, t
+	t.mu.Lock()
+	a, th := t.accepts.sum(now), t.throttles.sum(now)
+	t.mu.Unlock()
+	return a, th
 }
 
 // Enums for responses.

benchmark/benchmain/main.go

@@ -61,7 +61,6 @@ import (
 
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/benchmark"
-	bm "google.golang.org/grpc/benchmark"
 	"google.golang.org/grpc/benchmark/flags"
 	"google.golang.org/grpc/benchmark/latency"
 	"google.golang.org/grpc/benchmark/stats"
@@ -378,11 +377,11 @@ func makeClients(bf stats.Features) ([]testgrpc.BenchmarkServiceClient, func())
 		}))
 	}
 	lis = nw.Listener(lis)
-	stopper := bm.StartServer(bm.ServerInfo{Type: "protobuf", Listener: lis}, sopts...)
+	stopper := benchmark.StartServer(benchmark.ServerInfo{Type: "protobuf", Listener: lis}, sopts...)
 	conns := make([]*grpc.ClientConn, bf.Connections)
 	clients := make([]testgrpc.BenchmarkServiceClient, bf.Connections)
 	for cn := 0; cn < bf.Connections; cn++ {
-		conns[cn] = bm.NewClientConn("" /* target not used */, opts...)
+		conns[cn] = benchmark.NewClientConn("" /* target not used */, opts...)
 		clients[cn] = testgrpc.NewBenchmarkServiceClient(conns[cn])
 	}
 
@@ -430,7 +429,7 @@ func makeFuncStream(bf stats.Features) (rpcCallFunc, rpcCleanupFunc) {
 		if bf.EnablePreloader {
 			req = preparedMsg[cn][pos]
 		} else {
-			pl := bm.NewPayload(testpb.PayloadType_COMPRESSABLE, reqSizeBytes)
+			pl := benchmark.NewPayload(testpb.PayloadType_COMPRESSABLE, reqSizeBytes)
 			req = &testpb.SimpleRequest{
 				ResponseType: pl.Type,
 				ResponseSize: int32(respSizeBytes),
@@ -488,7 +487,7 @@ func setupStream(bf stats.Features, unconstrained bool) ([][]testgrpc.BenchmarkS
 		}
 	}
 
-	pl := bm.NewPayload(testpb.PayloadType_COMPRESSABLE, bf.ReqSizeBytes)
+	pl := benchmark.NewPayload(testpb.PayloadType_COMPRESSABLE, bf.ReqSizeBytes)
 	req := &testpb.SimpleRequest{
 		ResponseType: pl.Type,
 		ResponseSize: int32(bf.RespSizeBytes),
@@ -515,13 +514,13 @@ func prepareMessages(streams [][]testgrpc.BenchmarkService_StreamingCallClient,
 // Makes a UnaryCall gRPC request using the given BenchmarkServiceClient and
 // request and response sizes.
 func unaryCaller(client testgrpc.BenchmarkServiceClient, reqSize, respSize int) {
-	if err := bm.DoUnaryCall(client, reqSize, respSize); err != nil {
+	if err := benchmark.DoUnaryCall(client, reqSize, respSize); err != nil {
 		logger.Fatalf("DoUnaryCall failed: %v", err)
 	}
 }
 
 func streamCaller(stream testgrpc.BenchmarkService_StreamingCallClient, req any) {
-	if err := bm.DoStreamingRoundTripPreloaded(stream, req); err != nil {
+	if err := benchmark.DoStreamingRoundTripPreloaded(stream, req); err != nil {
 		logger.Fatalf("DoStreamingRoundTrip failed: %v", err)
 	}
 }

vet.sh

@@ -77,6 +77,9 @@ fi
 not grep 'func Test[^(]' *_test.go
 not grep 'func Test[^(]' test/*.go
 
+# - Check for typos in test function names
+git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test'
+
 # - Do not import x/net/context.
 not git grep -l 'x/net/context' -- "*.go"
 
@@ -94,7 +97,7 @@ git grep -l -e 'grpclog.I' --or -e 'grpclog.W' --or -e 'grpclog.E' --or -e 'grpc
 not git grep "\(import \|^\s*\)\"github.com/golang/protobuf/ptypes/" -- "*.go"
 
 # - Ensure all usages of grpc_testing package are renamed when importing.
-not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- "*.go" 
+not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- "*.go"
 
 # - Ensure all xds proto imports are renamed to *pb or *grpc.
 git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "'
@@ -110,7 +113,6 @@ for MOD_FILE in $(find . -name 'go.mod'); do
   go vet -all ./... | fail_on_output
   gofmt -s -d -l . 2>&1 | fail_on_output
   goimports -l . 2>&1 | not grep -vE "\.pb\.go"
-  golint ./... 2>&1 | not grep -vE "/grpc_testing_not_regenerate/.*\.pb\.go:"
 
   go mod tidy -compat=1.19
   git status --porcelain 2>&1 | fail_on_output || \
@@ -119,94 +121,73 @@ for MOD_FILE in $(find . -name 'go.mod'); do
 done
 
 # - Collection of static analysis checks
-#
-# TODO(dfawley): don't use deprecated functions in examples or first-party
-# plugins.
-# TODO(dfawley): enable ST1019 (duplicate imports) but allow for protobufs.
 SC_OUT="$(mktemp)"
-staticcheck -go 1.19 -checks 'inherit,-ST1015,-ST1019,-SA1019' ./... > "${SC_OUT}" || true
-# Error if anything other than deprecation warnings are printed.
-not grep -v "is deprecated:.*SA1019" "${SC_OUT}"
-# Only ignore the following deprecated types/fields/functions.
-not grep -Fv '.CredsBundle
-.HeaderMap
-.Metadata is deprecated: use Attributes
-.NewAddress
-.NewServiceConfig
-.Type is deprecated: use Attributes
-BuildVersion is deprecated
-balancer.ErrTransientFailure
-balancer.Picker
-extDesc.Filename is deprecated
-github.com/golang/protobuf/jsonpb is deprecated
-grpc.CallCustomCodec
-grpc.Code
-grpc.Compressor
-grpc.CustomCodec
-grpc.Decompressor
-grpc.MaxMsgSize
-grpc.MethodConfig
-grpc.NewGZIPCompressor
-grpc.NewGZIPDecompressor
-grpc.RPCCompressor
-grpc.RPCDecompressor
-grpc.ServiceConfig
-grpc.WithCompressor
-grpc.WithDecompressor
-grpc.WithDialer
-grpc.WithMaxMsgSize
-grpc.WithServiceConfig
-grpc.WithTimeout
-http.CloseNotifier
-info.SecurityVersion
-proto is deprecated
-proto.InternalMessageInfo is deprecated
-proto.EnumName is deprecated
-proto.ErrInternalBadWireType is deprecated
-proto.FileDescriptor is deprecated
-proto.Marshaler is deprecated
-proto.MessageType is deprecated
-proto.RegisterEnum is deprecated
-proto.RegisterFile is deprecated
-proto.RegisterType is deprecated
-proto.RegisterExtension is deprecated
-proto.RegisteredExtension is deprecated
-proto.RegisteredExtensions is deprecated
-proto.RegisterMapType is deprecated
-proto.Unmarshaler is deprecated
+staticcheck -go 1.19 -checks 'all' ./... > "${SC_OUT}" || true
+
+# Error for anything other than checks that need exclusions.
+grep -v "(ST1000)" "${SC_OUT}" | grep -v "(SA1019)" | grep -v "(ST1003)" | not grep -v "(ST1019)\|\(other import of\)"
+
+# Exclude underscore checks for generated code.
+grep "(ST1003)" "${SC_OUT}" | not grep -Fv '.pb.go:' 'code_string_test.go:'
+
+# Error for duplicate imports not including grpc protos.
+grep "(ST1019)\|\(other import of\)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
+channelz/grpc_channelz_v1"
+go-control-plane/envoy
+grpclb/grpc_lb_v1"
+health/grpc_health_v1"
+interop/grpc_testing"
+orca/v3"
+proto/grpc_gcp"
+proto/grpc_lookup_v1"
+reflection/grpc_reflection_v1"
+reflection/grpc_reflection_v1alpha"
+XXXXX PleaseIgnoreUnused'
+
+# Error for any package comments not in generated code.
+grep "(ST1000)" "${SC_OUT}" | not grep -v "\.pb\.go:"
+
+# Only ignore the following deprecated types/fields/functions and exclude
+# generated code.
+grep "(SA1019)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
+XXXXX Protobuf related deprecation errors:
+"github.com/golang/protobuf
+.pb.go:
+: ptypes.
+proto.RegisterType
+XXXXX gRPC internal usage deprecation errors:
+"google.golang.org/grpc
+: grpc.
+: v1alpha.
+: v1alphareflectionpb.
+BalancerAttributes is deprecated:
+CredsBundle is deprecated:
+Metadata is deprecated: use Attributes instead.
+NewSubConn is deprecated:
+OverrideServerName is deprecated:
+RemoveSubConn is deprecated:
+SecurityVersion is deprecated:
 Target is deprecated: Use the Target field in the BuildOptions instead.
-xxx_messageInfo_
-' "${SC_OUT}"
-
-# - special golint on package comments.
-lint_package_comment_per_package() {
-  # Number of files in this go package.
-  fileCount=$(go list -f '{{len .GoFiles}}' $1)
-  if [ ${fileCount} -eq 0 ]; then
-    return 0
-  fi
-  # Number of package errors generated by golint.
-  lintPackageCommentErrorsCount=$(golint --min_confidence 0 $1 | grep -c "should have a package comment")
-  # golint complains about every file that's missing the package comment. If the
-  # number of files for this package is greater than the number of errors, there's
-  # at least one file with package comment, good. Otherwise, fail.
-  if [ ${fileCount} -le ${lintPackageCommentErrorsCount} ]; then
-    echo "Package $1 (with ${fileCount} files) is missing package comment"
-    return 1
-  fi
-}
-lint_package_comment() {
-  set +ex
-
-  count=0
-  for i in $(go list ./...); do
-    lint_package_comment_per_package "$i"
-    ((count += $?))
-  done
-
-  set -ex
-  return $count
-}
-lint_package_comment
+UpdateAddresses is deprecated:
+UpdateSubConnState is deprecated:
+balancer.ErrTransientFailure is deprecated:
+grpc/reflection/v1alpha/reflection.proto
+XXXXX xDS deprecated fields we support
+.ExactMatch
+.PrefixMatch
+.SafeRegexMatch
+.SuffixMatch
+GetContainsMatch
+GetExactMatch
+GetMatchSubjectAltNames
+GetPrefixMatch
+GetSafeRegexMatch
+GetSuffixMatch
+GetTlsCertificateCertificateProviderInstance
+GetValidationContextCertificateProviderInstance
+XXXXX TODO: Remove the below deprecation usages:
+CloseNotifier
+Roots.Subjects
+XXXXX PleaseIgnoreUnused'
 
 echo SUCCESS

xds/internal/balancer/outlierdetection/callcounter_test.go

@@ -38,19 +38,19 @@ func (b1 *bucket) Equal(b2 *bucket) bool {
 	return b1.numFailures == b2.numFailures
 }
 
-func (cc1 *callCounter) Equal(cc2 *callCounter) bool {
-	if cc1 == nil && cc2 == nil {
+func (cc *callCounter) Equal(cc2 *callCounter) bool {
+	if cc == nil && cc2 == nil {
 		return true
 	}
-	if (cc1 != nil) != (cc2 != nil) {
+	if (cc != nil) != (cc2 != nil) {
 		return false
 	}
-	ab1 := (*bucket)(atomic.LoadPointer(&cc1.activeBucket))
+	ab1 := (*bucket)(atomic.LoadPointer(&cc.activeBucket))
 	ab2 := (*bucket)(atomic.LoadPointer(&cc2.activeBucket))
 	if !ab1.Equal(ab2) {
 		return false
 	}
-	return cc1.inactiveBucket.Equal(cc2.inactiveBucket)
+	return cc.inactiveBucket.Equal(cc2.inactiveBucket)
 }
 
 // TestClear tests that clear on the call counter clears (everything set to 0)

xds/internal/resolver/xds_resolver_test.go

@@ -67,7 +67,6 @@ import (
 	v3discoverypb "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
 
 	_ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer" // Register the cds LB policy
-	_ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer" // To parse LB config
 	_ "google.golang.org/grpc/xds/internal/httpfilter/router"    // Register the router filter
 )
 

xds/internal/xdsclient/authority_test.go

@@ -31,7 +31,6 @@ import (
 	"google.golang.org/grpc/xds/internal"
 
 	"google.golang.org/grpc/xds/internal/testutils"
-	xdstestutils "google.golang.org/grpc/xds/internal/testutils"
 	"google.golang.org/grpc/xds/internal/xdsclient/bootstrap"
 	"google.golang.org/grpc/xds/internal/xdsclient/xdsresource"
 	"google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version"
@@ -66,7 +65,7 @@ func setupTest(ctx context.Context, t *testing.T, opts e2e.ManagementServerOptio
 	}
 
 	a, err := newAuthority(authorityArgs{
-		serverCfg: xdstestutils.ServerConfigForAddress(t, ms.Address),
+		serverCfg: testutils.ServerConfigForAddress(t, ms.Address),
 		bootstrapCfg: &bootstrap.Config{
 			NodeProto: &v3corepb.Node{Id: nodeID},
 		},

xds/internal/xdsclient/tests/cds_watchers_test.go

@@ -633,7 +633,7 @@ func (s) TestCDSWatch_ValidResponseCancelsExpiryTimerBehavior(t *testing.T) {
 //  2. An update to other resource should result in the invocation of the watch
 //     callback associated with that resource.  It should not result in the
 //     invocation of the watch callback associated with the deleted resource.
-func (s) TesCDSWatch_ResourceRemoved(t *testing.T) {
+func (s) TestCDSWatch_ResourceRemoved(t *testing.T) {
 	mgmtServer, nodeID, bootstrapContents, _, cleanup := e2e.SetupManagementServer(t, e2e.ManagementServerOptions{})
 	defer cleanup()