nogo: add missing go vet checks (#6218)

BUILD

@@ -35,32 +35,41 @@ write_file(
                     },
                 }
                 for analyzer in ANALYZERS + [
+                    "appends",
                     "asmdecl",
                     "assign",
                     "atomicalign",
+                    "bools",
                     "buildtag",
-                    "cgocall",
+                    # "cgocall",
                     "composites",
                     "copylocks",
                     "deepequalerrors",
+                    "defers",
+                    "directive",
                     "errorsas",
+                    # Noisy and is not part of 'go vet'
                     # "fieldalignment",
                     "framepointer",
                     "httpresponse",
                     "ifaceassert",
                     "loopclosure",
                     "lostcancel",
-                    # template methods currently cause this analyzer to panic
-                    # "nilness",
+                    "nilfunc",
+                    "nilness",
+                    "printf",
                     # Everyone shadows `err`
                     # "shadow",
                     "shift",
+                    "sigchanyzer",
+                    "slog",
                     "sortslice",
                     "stdmethods",
                     "stringintconv",
                     "structtag",
-                    "tests",
                     "testinggoroutine",
+                    "tests",
+                    "timeformat",
                     "unmarshal",
                     "unreachable",
                     "unsafeptr",
@@ -74,34 +83,43 @@ write_file(
 nogo(
     name = "vet",
     config = ":nogo_config.json",
-    vet = True,
     visibility = ["//visibility:public"],
     deps = [
+        "@org_golang_x_tools//go/analysis/passes/appends",
         "@org_golang_x_tools//go/analysis/passes/asmdecl",
         "@org_golang_x_tools//go/analysis/passes/assign",
+        "@org_golang_x_tools//go/analysis/passes/atomic",
         "@org_golang_x_tools//go/analysis/passes/atomicalign",
+        "@org_golang_x_tools//go/analysis/passes/bools",
+        "@org_golang_x_tools//go/analysis/passes/buildtag",
         # "@org_golang_x_tools//go/analysis/passes/cgocall",
         "@org_golang_x_tools//go/analysis/passes/composite",
         "@org_golang_x_tools//go/analysis/passes/copylock",
         "@org_golang_x_tools//go/analysis/passes/deepequalerrors",
+        "@org_golang_x_tools//go/analysis/passes/defers",
+        "@org_golang_x_tools//go/analysis/passes/directive",
         "@org_golang_x_tools//go/analysis/passes/errorsas",
         # "@org_golang_x_tools//go/analysis/passes/fieldalignment",
         "@org_golang_x_tools//go/analysis/passes/framepointer",
         "@org_golang_x_tools//go/analysis/passes/httpresponse",
         "@org_golang_x_tools//go/analysis/passes/ifaceassert",
         "@org_golang_x_tools//go/analysis/passes/loopclosure",
         "@org_golang_x_tools//go/analysis/passes/lostcancel",
-        # template methods currently cause this analyzer to panic
-        # "@org_golang_x_tools//go/analysis/passes/nilness",
+        "@org_golang_x_tools//go/analysis/passes/nilfunc",
+        "@org_golang_x_tools//go/analysis/passes/nilness",
+        "@org_golang_x_tools//go/analysis/passes/printf",
         # Everyone shadows `err`
         # "@org_golang_x_tools//go/analysis/passes/shadow",
         "@org_golang_x_tools//go/analysis/passes/shift",
+        "@org_golang_x_tools//go/analysis/passes/sigchanyzer",
+        "@org_golang_x_tools//go/analysis/passes/slog",
         "@org_golang_x_tools//go/analysis/passes/sortslice",
         "@org_golang_x_tools//go/analysis/passes/stdmethods",
         "@org_golang_x_tools//go/analysis/passes/stringintconv",
         "@org_golang_x_tools//go/analysis/passes/structtag",
-        "@org_golang_x_tools//go/analysis/passes/tests",
         "@org_golang_x_tools//go/analysis/passes/testinggoroutine",
+        "@org_golang_x_tools//go/analysis/passes/tests",
+        "@org_golang_x_tools//go/analysis/passes/timeformat",
         "@org_golang_x_tools//go/analysis/passes/unmarshal",
         "@org_golang_x_tools//go/analysis/passes/unreachable",
         "@org_golang_x_tools//go/analysis/passes/unsafeptr",

cli/remotebazel/remotebazel.go

@@ -680,7 +680,7 @@ func Run(ctx context.Context, opts RunOpts, repoConfig *RepoConfig) (int, error)
 	log.Debugf("Invocation ID: %s", iid)
 
 	// If the remote bazel process is canceled or killed, cancel the remote run
-	sigChan := make(chan os.Signal)
+	sigChan := make(chan os.Signal, 1)
 	go func() {
 		<-sigChan
 		_, err = bbClient.CancelExecutions(ctx, &inpb.CancelExecutionsRequest{

enterprise/server/githubapp/githubapp.go

@@ -629,16 +629,13 @@ func (a *GitHubApp) GetLinkedGitHubRepos(ctx context.Context) (*ghpb.GetLinkedRe
 		WHERE group_id = ?
 		ORDER BY repo_url ASC
 	`, u.GetGroupID())
-	if err != nil {
-		return nil, status.InternalErrorf("failed to query repo rows: %s", err)
-	}
 	res := &ghpb.GetLinkedReposResponse{}
 	err = db.ScanEach(rq, func(ctx context.Context, row *tables.GitRepository) error {
 		res.RepoUrls = append(res.RepoUrls, row.RepoURL)
 		return nil
 	})
 	if err != nil {
-		return nil, err
+		return nil, status.InternalErrorf("failed to query repo rows: %s", err)
 	}
 	return res, nil
 }

enterprise/server/raft/replica/replica.go

@@ -436,7 +436,7 @@ func (sm *Replica) updatePartitionMetadata(wb pebble.Batch, key, val []byte, fil
 			// Skip increment on duplicate write.
 			return closer.Close()
 		}
-		if err != nil && err != pebble.ErrNotFound {
+		if err != pebble.ErrNotFound {
 			return err
 		}
 		pm.TotalCount++

enterprise/server/remote_execution/dirtools/dirtools.go

@@ -198,9 +198,6 @@ func newDirToUpload(instanceName string, digestFunction repb.DigestFunction_Valu
 		return nil, err
 	}
 	r := digest.NewResourceName(d, instanceName, rspb.CacheType_CAS, digestFunction)
-	if err != nil {
-		return nil, err
-	}
 
 	return &fileToUpload{
 		fullFilePath: filepath.Join(parentDir, info.Name()),

enterprise/server/remote_execution/snaploader/snaploader.go

@@ -869,7 +869,7 @@ func groupID(ctx context.Context, env environment.Env) (string, error) {
 	u, err := env.GetAuthenticator().AuthenticatedUser(ctx)
 	if err == nil {
 		gid = u.GetGroupID()
-	} else if err != nil && !authutil.IsAnonymousUserError(err) && !*container.DebugEnableAnonymousRecycling {
+	} else if !authutil.IsAnonymousUserError(err) && !*container.DebugEnableAnonymousRecycling {
 		return "", err
 	}
 	return gid, nil

enterprise/server/remote_execution/soci_store/soci_store_linux.go

@@ -278,9 +278,11 @@ func streamingStoreArg(path string) string {
 
 func getArtifacts(ctx context.Context, client socipb.SociArtifactStoreClient, env environment.Env, image string, credentials oci.Credentials) error {
 	startTime := time.Now()
-	defer metrics.PodmanGetSociArtifactsLatencyUsec.
-		With(prometheus.Labels{metrics.ContainerImageTag: image}).
-		Observe(float64(time.Since(startTime).Microseconds()))
+	defer func() {
+		metrics.PodmanGetSociArtifactsLatencyUsec.
+			With(prometheus.Labels{metrics.ContainerImageTag: image}).
+			Observe(float64(time.Since(startTime).Microseconds()))
+	}()
 
 	ctx, err := prefix.AttachUserPrefixToContext(ctx, env)
 	if err != nil {

enterprise/server/scheduling/task_leaser/task_leaser.go

@@ -184,10 +184,8 @@ func (t *TaskLeaser) Close(ctx context.Context, taskErr error, retry bool) {
 		req.Finalize = true
 	} else {
 		req.ReEnqueue = true
-		if taskErr != nil {
-			s, _ := gstatus.FromError(taskErr)
-			req.ReEnqueueReason = s.Proto()
-		}
+		s, _ := gstatus.FromError(taskErr)
+		req.ReEnqueueReason = s.Proto()
 	}
 	if err := t.stream.Send(req); err != nil {
 		log.CtxWarningf(ctx, "Could not send request: %s", err)

enterprise/server/secrets/secrets.go

@@ -76,7 +76,7 @@ func (s *SecretService) listSecretsIncludingValues(ctx context.Context) (*skpb.L
 		return nil, err
 	}
 	dbHandle := s.env.GetDBHandle()
-	if err != nil {
+	if dbHandle == nil {
 		return nil, status.FailedPreconditionError("A database is required")
 	}
 
@@ -119,7 +119,7 @@ func (s *SecretService) UpdateSecret(ctx context.Context, req *skpb.UpdateSecret
 		return nil, false, err
 	}
 	dbHandle := s.env.GetDBHandle()
-	if err != nil {
+	if dbHandle == nil {
 		return nil, false, status.FailedPreconditionError("A database is required")
 	}
 
@@ -202,7 +202,7 @@ func (s *SecretService) DeleteSecret(ctx context.Context, req *skpb.DeleteSecret
 		return nil, err
 	}
 	dbHandle := s.env.GetDBHandle()
-	if err != nil {
+	if dbHandle == nil {
 		return nil, status.FailedPreconditionError("A database is required")
 	}
 

enterprise/server/sociartifactstore/sociartifactstore.go

@@ -149,7 +149,7 @@ func (s *SociArtifactStore) GetArtifacts(ctx context.Context, req *socipb.GetArt
 	if err == nil {
 		return resp, nil
 	}
-	if err != nil && !status.IsNotFoundError(err) {
+	if !status.IsNotFoundError(err) {
 		return nil, err
 	}
 

server/buildbuddy_server/buildbuddy_server.go

@@ -505,18 +505,14 @@ func (s *BuildBuddyServer) UpdateGroup(ctx context.Context, req *grpb.UpdateGrou
 	if userDB == nil {
 		return nil, status.UnimplementedError("Not Implemented")
 	}
-	var group *tables.Group
-	var err error
-
-	if group == nil {
-		if req.GetRequestContext().GetGroupId() == "" {
-			return nil, status.InvalidArgumentError("Missing organization identifier.")
-		}
-		group, err = userDB.GetGroupByID(ctx, req.GetRequestContext().GetGroupId())
-		if err != nil {
-			return nil, err
-		}
+	if req.GetRequestContext().GetGroupId() == "" {
+		return nil, status.InvalidArgumentError("Missing organization identifier.")
 	}
+	group, err := userDB.GetGroupByID(ctx, req.GetRequestContext().GetGroupId())
+	if err != nil {
+		return nil, err
+	}
+
 	group.Name = req.GetName()
 	if ui := strings.TrimSpace(req.GetUrlIdentifier()); ui != "" {
 		group.URLIdentifier = ui

server/util/claims/claims.go

@@ -129,9 +129,7 @@ func ParseClaims(token string) (*Claims, error) {
 		if errors.As(err, &validationErr) && validationErr.Errors&jwt.ValidationErrorSignatureInvalid != 0 {
 			continue
 		}
-		if err != nil {
-			return nil, err
-		}
+		return nil, err
 	}
 	return nil, lastErr
 }

server/util/clickhouse/clickhouse.go

@@ -109,8 +109,8 @@ func (r *rawQuery) IterateRaw(fn func(ctx context.Context, row *sql.Rows) error)
 			return err
 		}
 	}
-	if err != nil {
-		return err
+	if rows.Err() != nil {
+		return rows.Err()
 	}
 	return nil
 }

server/util/db/db.go

@@ -1018,8 +1018,8 @@ func (r *rawQuery) IterateRaw(fn func(ctx context.Context, row *sql.Rows) error)
 			return err
 		}
 	}
-	if err != nil {
-		return err
+	if rows.Err() != nil {
+		return rows.Err()
 	}
 	return nil
 }

server/util/flagutil/yaml/yaml.go

@@ -719,7 +719,7 @@ func populateFlagsFromYAML(a any, prefix []string, node *yaml.Node, setFlags map
 			if node != nil {
 				// Ensure that we populate flags in the order they are specified in the
 				// YAML data if the node structure data was provided.
-				for ok := false; node != nil && !ok; i++ {
+				for ok := false; !ok; i++ {
 					k = node.Content[2*i].Value
 					n = node.Content[2*i+1]
 					v, ok = m[k]

server/util/healthcheck/healthcheck.go

@@ -66,7 +66,7 @@ func NewHealthChecker(serverType string) *HealthChecker {
 		checkers:      make(map[string]interfaces.Checker, 0),
 		lastStatus:    make([]*serviceStatus, 0),
 	}
-	sigTerm := make(chan os.Signal)
+	sigTerm := make(chan os.Signal, 1)
 	go func() {
 		<-sigTerm
 		hc.Shutdown()

server/util/ioutil/ioutil.go

@@ -76,7 +76,7 @@ func (c *CustomCommitWriteCloser) Close() error {
 	// Close may free resources, so all Close functions should be called.
 	// The first error encountered will be returned.
 	if closer, ok := c.w.(io.Closer); ok {
-		if err := closer.Close(); err != nil && firstErr == nil {
+		if err := closer.Close(); err != nil {
 			firstErr = err
 		}
 	}

tools/metrics/grafana/grafana.go

@@ -174,9 +174,14 @@ func exportNormalizedDashboard(d *Dashboard) error {
 		return err
 	}
 	outPath := filepath.Join(dashboardsDir, fileName)
+	oldContent, err := os.ReadFile(outPath)
+	if err != nil && !os.IsNotExist(err) {
+		return err
+	}
+
 	// Write file only if different, to avoid updating mtime and triggering file
 	// watchers etc.
-	if b, _ := os.ReadFile(outPath); err == nil && !bytes.Equal(b, normalized.Bytes()) {
+	if oldContent == nil || !bytes.Equal(oldContent, normalized.Bytes()) {
 		return os.WriteFile(outPath, normalized.Bytes(), 0644)
 	}
 	return nil