vet: run staticcheck for all sub modules (#7155)

channelz/internal/protoconv/sockopt_linux.go

@@ -21,9 +21,10 @@ package protoconv
 import (
 	"time"
 
-	channelzpb "google.golang.org/grpc/channelz/grpc_channelz_v1"
 	"google.golang.org/grpc/grpclog"
 	"google.golang.org/grpc/internal/channelz"
+
+	channelzpb "google.golang.org/grpc/channelz/grpc_channelz_v1"
 	"google.golang.org/protobuf/types/known/anypb"
 	"google.golang.org/protobuf/types/known/durationpb"
 )

examples/features/customloadbalancer/client/customroundrobin/customroundrobin.go

@@ -16,6 +16,7 @@
  *
  */
 
+// Package customroundrobin provides an example for the custom roundrobin balancer.
 package customroundrobin
 
 import (

gcp/observability/observability_test.go

@@ -39,7 +39,6 @@ import (
 	"google.golang.org/grpc/internal/leakcheck"
 	"google.golang.org/grpc/internal/stubserver"
 	"google.golang.org/grpc/internal/testutils"
-	"google.golang.org/grpc/metadata"
 
 	testgrpc "google.golang.org/grpc/interop/grpc_testing"
 	testpb "google.golang.org/grpc/interop/grpc_testing"
@@ -64,21 +63,16 @@ func init() {
 }
 
 var (
-	defaultTestTimeout        = 10 * time.Second
-	testHeaderMetadata        = metadata.MD{"header": []string{"HeADer"}}
-	testTrailerMetadata       = metadata.MD{"trailer": []string{"TrAileR"}}
-	testOkPayload             = []byte{72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100}
-	testErrorPayload          = []byte{77, 97, 114, 116, 104, 97}
-	testErrorMessage          = "test case injected error"
-	infinitySizeBytes   int32 = 1024 * 1024 * 1024
-	defaultRequestCount       = 24
+	defaultTestTimeout  = 10 * time.Second
+	testOkPayload       = []byte{72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100}
+	defaultRequestCount = 24
 )
 
 const (
-	TypeOpenCensusViewDistribution string = "distribution"
-	TypeOpenCensusViewCount               = "count"
-	TypeOpenCensusViewSum                 = "sum"
-	TypeOpenCensusViewLastValue           = "last_value"
+	TypeOpenCensusViewDistribution = "distribution"
+	TypeOpenCensusViewCount        = "count"
+	TypeOpenCensusViewSum          = "sum"
+	TypeOpenCensusViewLastValue    = "last_value"
 )
 
 type fakeOpenCensusExporter struct {
@@ -504,6 +498,9 @@ func (s) TestCustomTagsTracingMetrics(t *testing.T) {
 	}`
 
 	cleanup, err := createTmpConfigInFileSystem(configJSON)
+	if err != nil {
+		t.Fatalf("failed to create config in file system: %v", err)
+	}
 	defer cleanup()
 
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)

scripts/vet.sh

@@ -1,13 +1,20 @@
 #!/bin/bash
 
-set -ex  # Exit on error; debugging enabled.
-set -o pipefail  # Fail a pipe if any sub-command fails.
+set -ex         # Exit on error; debugging enabled.
+set -o pipefail # Fail a pipe if any sub-command fails.
 
 source "$(dirname $0)/vet-common.sh"
 
 # Check to make sure it's safe to modify the user's git repo.
 git status --porcelain | fail_on_output
 
+# noret_grep will return 0 if zero or more lines were selected, and >1 if an
+# error occurred. Suppresses grep's return code of 1 when there are no matches
+# (for eg, empty file).
+noret_grep() {
+  grep "$@" || [[ $? == 1 ]]
+}
+
 # Undo any edits made by this script.
 cleanup() {
   git reset --hard HEAD
@@ -40,34 +47,31 @@ 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'
-git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example'
-
-# - Do not import x/net/context.
-not git grep -l 'x/net/context' -- "*.go"
+git grep --quiet 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test'
+git grep --quiet 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example'
 
 # - Do not use time.After except in tests.  It has the potential to leak the
 #   timer since there is no way to stop it early.
-git grep -l 'time.After(' -- "*.go" | not grep -v '_test.go\|test_utils\|testutils'
+git grep --quiet -l 'time.After(' -- "*.go" | not grep -v '_test.go\|test_utils\|testutils'
 
 # - Do not import math/rand for real library code.  Use internal/grpcrand for
 #   thread safety.
-git grep -l '"math/rand"' -- "*.go" 2>&1 | not grep -v '^examples\|^interop/stress\|grpcrand\|^benchmark\|wrr_test'
+git grep --quiet -l '"math/rand"' -- "*.go" 2>&1 | not grep -v '^examples\|^interop/stress\|grpcrand\|^benchmark\|wrr_test'
 
 # - Do not use "interface{}"; use "any" instead.
-git grep -l 'interface{}' -- "*.go" 2>&1 | not grep -v '\.pb\.go\|protoc-gen-go-grpc\|grpc_testing_not_regenerate'
+git grep --quiet -l 'interface{}' -- "*.go" 2>&1 | not grep -v '\.pb\.go\|protoc-gen-go-grpc\|grpc_testing_not_regenerate'
 
 # - Do not call grpclog directly. Use grpclog.Component instead.
-git grep -l -e 'grpclog.I' --or -e 'grpclog.W' --or -e 'grpclog.E' --or -e 'grpclog.F' --or -e 'grpclog.V' -- "*.go" | not grep -v '^grpclog/component.go\|^internal/grpctest/tlogger_test.go'
+git grep --quiet -l -e 'grpclog.I' --or -e 'grpclog.W' --or -e 'grpclog.E' --or -e 'grpclog.F' --or -e 'grpclog.V' -- "*.go" | not grep -v '^grpclog/component.go\|^internal/grpctest/tlogger_test.go'
 
 # - Ensure that the deprecated protobuf dependency is not used.
-not git grep "\"github.com/golang/protobuf/*" -- "*.go" ':(exclude)reflection/test/grpc_testing_not_regenerate/*'
+not git grep --quiet "\"github.com/golang/protobuf/*" -- "*.go" ':(exclude)reflection/test/grpc_testing_not_regenerate/*'
 
 # - 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 --quiet "\(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 "'
+git grep --quiet '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "'
 
 misspell -error .
 
@@ -83,21 +87,19 @@ for MOD_FILE in $(find . -name 'go.mod'); do
   go mod tidy -compat=1.19
   git status --porcelain 2>&1 | fail_on_output || \
     (git status; git --no-pager diff; exit 1)
-  popd
-done
+
+  # - Collection of static analysis checks
+  SC_OUT="$(mktemp)"
+  staticcheck -go 1.19 -checks 'all' ./... >"${SC_OUT}" || true
 
-# - Collection of static analysis checks
-SC_OUT="$(mktemp)"
-staticcheck -go 1.19 -checks 'all' ./... > "${SC_OUT}" || true
+  # Error for anything other than checks that need exclusions.
+  noret_grep -v "(ST1000)" "${SC_OUT}" | noret_grep -v "(SA1019)" | noret_grep -v "(ST1003)" | noret_grep -v "(ST1019)\|\(other import of\)" | not grep -v "(SA4000)"
 
-# 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.
+  noret_grep "(ST1003)" "${SC_OUT}" | not grep -v '\(.pb.go:\)\|\(code_string_test.go:\)\|\(grpc_testing_not_regenerate\)'
 
-# Exclude underscore checks for generated code.
-grep "(ST1003)" "${SC_OUT}" | not grep -v '\(.pb.go:\)\|\(code_string_test.go:\)\|\(grpc_testing_not_regenerate\)'
-
-# Error for duplicate imports not including grpc protos.
-grep "(ST1019)\|\(other import of\)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
+  # Error for duplicate imports not including grpc protos.
+  noret_grep "(ST1019)\|\(other import of\)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
 channelz/grpc_channelz_v1"
 go-control-plane/envoy
 grpclb/grpc_lb_v1"
@@ -106,16 +108,21 @@ interop/grpc_testing"
 orca/v3"
 proto/grpc_gcp"
 proto/grpc_lookup_v1"
+examples/features/proto/echo"
 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:"
+  # Error for any package comments not in generated code.
+  noret_grep "(ST1000)" "${SC_OUT}" | not grep -v "\.pb\.go:"
+
+  # Ignore a false positive when operands have side affectes.
+  # TODO(https://github.com/dominikh/go-tools/issues/54): Remove this once the issue is fixed in staticcheck.
+  noret_grep "(SA4000)" "${SC_OUT}" | not grep -ev "crl.go:\d*:\d*: identical expressions on the left and right side of the '||' operator (SA4000)"
 
-# Only ignore the following deprecated types/fields/functions and exclude
-# generated code.
-grep "(SA1019)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
+  # Only ignore the following deprecated types/fields/functions and exclude
+  # generated code.
+  noret_grep "(SA1019)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused
 XXXXX Protobuf related deprecation errors:
 "github.com/golang/protobuf
 .pb.go:
@@ -154,5 +161,7 @@ GetSuffixMatch
 GetTlsCertificateCertificateProviderInstance
 GetValidationContextCertificateProviderInstance
 XXXXX PleaseIgnoreUnused'
+  popd
+done
 
 echo SUCCESS

security/advancedtls/crl_test.go

@@ -224,7 +224,6 @@ qsSIp8gfxSyzkJP+Ngkm2DdLjlJQCZ9R0MZP9Xj4
 		t.Fatalf("parseRevocationList(dummyCrlFile) failed: %v", err)
 	}
 	crlExt := &CRL{certList: crl}
-	var crlIssuer pkix.Name = crl.Issuer
 
 	var revocationTests = []struct {
 		desc    string
@@ -290,7 +289,7 @@ qsSIp8gfxSyzkJP+Ngkm2DdLjlJQCZ9R0MZP9Xj4
 		{
 			desc: "Single unrevoked Issuer",
 			in: x509.Certificate{
-				Issuer:                crlIssuer,
+				Issuer:                crl.Issuer,
 				SerialNumber:          big.NewInt(2),
 				CRLDistributionPoints: []string{"test"},
 			},

security/authorization/engine/engine.go

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+// Package engine provides a CEL-based authorization engine for gRPC.
 package engine
 
 import (
@@ -28,7 +29,6 @@ import (
 	"github.com/google/cel-go/interpreter"
 	expr "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
 	"google.golang.org/grpc/grpclog"
-	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/peer"
 	"google.golang.org/protobuf/proto"
 )
@@ -71,7 +71,6 @@ func (activation activationImpl) Parent() interpreter.Activation {
 
 // AuthorizationArgs is the input of the CEL-based authorization engine.
 type AuthorizationArgs struct {
-	md         metadata.MD
 	peerInfo   *peer.Peer
 	fullMethod string
 }
@@ -211,7 +210,7 @@ func exprToProgram(condition *expr.Expr, env *cel.Env) (cel.Program, error) {
 		return nil, iss.Err()
 	}
 	// Check that the expression will evaluate to a boolean.
-	if !proto.Equal(ast.ResultType(), decls.Bool) {
+	if ot, _ := cel.TypeToExprType(ast.OutputType()); !proto.Equal(ot, decls.Bool) {
 		return nil, fmt.Errorf("expected boolean condition")
 	}
 	// Build the program plan.

security/authorization/engine/util.go

@@ -40,7 +40,7 @@ func compileCel(env *cel.Env, expr string) (*cel.Ast, error) {
 		return nil, iss.Err()
 	}
 	// Check the result type is a Boolean.
-	if !proto.Equal(checked.ResultType(), decls.Bool) {
+	if ot, _ := cel.TypeToExprType(checked.OutputType()); !proto.Equal(ot, decls.Bool) {
 		return nil, errors.New("failed to compile CEL string: get non-bool value")
 	}
 	return checked, nil

security/authorization/engine/util_test.go

@@ -29,9 +29,9 @@ import (
 
 func (s) TestStringConvert(t *testing.T) {
 	declarations := []*expr.Decl{
-		decls.NewIdent("request.url_path", decls.String, nil),
-		decls.NewIdent("request.host", decls.String, nil),
-		decls.NewIdent("connection.uri_san_peer_certificate", decls.String, nil),
+		decls.NewConst("request.url_path", decls.String, nil),
+		decls.NewConst("request.host", decls.String, nil),
+		decls.NewConst("connection.uri_san_peer_certificate", decls.String, nil),
 	}
 	env, err := cel.NewEnv()
 	if err != nil {