Add revive linter (#3755)

denik · web-flow · commit 106c0596fba5 · 2025-10-15T12:39:35.000Z
## Changes - Add revive linter with a subset of rules. - Fix issues found. ## Why Good rules, some stylistic, some catch real bugs, e.g. recursion in IsZero(). ------ https://chatgpt.com/codex/tasks/task_e_68eccd655f1c8325aded897ab7a3bddb Original PR: denik#6
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -10,6 +10,7 @@ linters:
     - intrange
     - mirror
     - perfsprint
+    - revive
     - staticcheck
     - testifylint
     - unconvert
@@ -27,6 +28,24 @@ linters:
         - (*github.com/spf13/cobra.Command).MarkFlagRequired
         - (*github.com/spf13/pflag.FlagSet).MarkDeprecated
         - (*github.com/spf13/pflag.FlagSet).MarkHidden
+    revive:
+      rules:
+        - name: blank-imports
+        - name: context-as-argument
+          exclude:
+            - TEST
+            - "**/integration/**"
+            - "**/internal/testcli/**"
+            - "**/internal/testutil/**"
+            - "**/libs/testdiff/**"
+            - "**/libs/python/pythontest/**"
+        - name: context-keys-type
+        - name: error-return
+        - name: errorf
+        - name: range-val-in-closure
+        - name: waitgroup-by-value
+        - name: unconditional-recursion
+        - name: use-waitgroup-go
     gocritic:
       disable-all: true
       enabled-checks:
diff --git a/bundle/internal/validation/enum.go b/bundle/internal/validation/enum.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"go/format"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -236,9 +237,16 @@ func generateEnumFields(outPath string) error {
 		return fmt.Errorf("failed to execute template: %w", err)
 	}
 
+	// Format the generated source with gofmt-equivalent rules so minor template
+	// whitespace changes don't create noisy diffs in the checked-in artifacts.
+	formatted, err := format.Source(generatedCode.Bytes())
+	if err != nil {
+		return fmt.Errorf("failed to format generated code: %w", err)
+	}
+
 	// Write generated code to file
 	filePath := filepath.Join(outPath, "enum_fields.go")
-	if err := os.WriteFile(filePath, generatedCode.Bytes(), 0o644); err != nil {
+	if err := os.WriteFile(filePath, formatted, 0o644); err != nil {
 		return fmt.Errorf("failed to write generated code: %w", err)
 	}
 
@@ -251,10 +259,6 @@ const enumValidationTemplate = `package generated
 // THIS FILE IS AUTOGENERATED.
 // DO NOT EDIT THIS FILE DIRECTLY.
 
-import (
-	_ "github.com/databricks/cli/libs/dyn"
-)
-
 // EnumFields maps [dyn.Pattern] to valid enum values they should have.
 var EnumFields = map[string][]string{
 {{- range . }}
diff --git a/bundle/internal/validation/generated/enum_fields.go b/bundle/internal/validation/generated/enum_fields.go
diff --git a/bundle/internal/validation/generated/required_fields.go b/bundle/internal/validation/generated/required_fields.go
diff --git a/bundle/internal/validation/required.go b/bundle/internal/validation/required.go
@@ -3,6 +3,7 @@ package main
 import (
 	"bytes"
 	"fmt"
+	"go/format"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -185,9 +186,16 @@ func generateRequiredFields(outPath string) error {
 		return fmt.Errorf("failed to execute template: %w", err)
 	}
 
+	// Format the generated source with gofmt-equivalent rules so minor template
+	// whitespace changes don't create noisy diffs in the checked-in artifacts.
+	formatted, err := format.Source(generatedCode.Bytes())
+	if err != nil {
+		return fmt.Errorf("failed to format generated code: %w", err)
+	}
+
 	// Write generated code to file
 	filePath := filepath.Join(outPath, "required_fields.go")
-	if err := os.WriteFile(filePath, generatedCode.Bytes(), 0o644); err != nil {
+	if err := os.WriteFile(filePath, formatted, 0o644); err != nil {
 		return fmt.Errorf("failed to write generated code: %w", err)
 	}
 
@@ -200,10 +208,6 @@ const validationTemplate = `package generated
 // THIS FILE IS AUTOGENERATED.
 // DO NOT EDIT THIS FILE DIRECTLY.
 
-import (
-	_ "github.com/databricks/cli/libs/dyn"
-)
-
 // RequiredFields maps [dyn.Pattern] to required fields they should have.
 var RequiredFields = map[string][]string{
 {{- range . }}
diff --git a/bundle/mutator_read_only.go b/bundle/mutator_read_only.go
@@ -38,15 +38,13 @@ func ApplyParallel(ctx context.Context, b *Bundle, mutators ...ReadOnlyMutator)
 	}
 
 	for ind, m := range mutators {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
+		wg.Go(func() {
 			// We're not using bundle.ApplyContext here because we don't do copy between typed and dynamic values
 			diags := m.Apply(contexts[ind], b)
 			for _, d := range diags {
 				logdiag.LogDiag(ctx, d)
 			}
-		}()
+		})
 	}
 
 	wg.Wait()
diff --git a/cmd/auth/profiles.go b/cmd/auth/profiles.go
@@ -115,14 +115,12 @@ func newProfilesCommand() *cobra.Command {
 			if profile.IsEmpty() {
 				continue
 			}
-			wg.Add(1)
-			go func() {
+			wg.Go(func() {
 				ctx := cmd.Context()
 				t := time.Now()
 				profile.Load(ctx, iniFile.Path(), skipValidate)
 				log.Debugf(ctx, "Profile %q took %s to load", profile.Name, time.Since(t))
-				wg.Done()
-			}()
+			})
 			profiles = append(profiles, profile)
 		}
 		wg.Wait()
diff --git a/cmd/auth/token.go b/cmd/auth/token.go
@@ -129,8 +129,8 @@ func loadToken(ctx context.Context, args loadTokenArgs) (*oauth2.Token, error) {
 			// This is captured in an acceptance test under "cmd/auth/token".
 			err = errors.New("cache: databricks OAuth is not configured for this host")
 		}
-		if err, ok := auth.RewriteAuthError(ctx, args.authArguments.Host, args.authArguments.AccountID, args.profileName, err); ok {
-			return nil, err
+		if rewritten, rewrittenErr := auth.RewriteAuthError(ctx, args.authArguments.Host, args.authArguments.AccountID, args.profileName, err); rewritten {
+			return nil, rewrittenErr
 		}
 		helpMsg := helpfulError(ctx, args.profileName, oauthArgument)
 		return nil, fmt.Errorf("%w. %s", err, helpMsg)
diff --git a/cmd/root/auth.go b/cmd/root/auth.go
@@ -314,6 +314,8 @@ func emptyHttpRequest(ctx context.Context) *http.Request {
 }
 
 func renderError(ctx context.Context, cfg *config.Config, err error) error {
-	err, _ = auth.RewriteAuthError(ctx, cfg.Host, cfg.AccountID, cfg.Profile, err)
+	if rewritten, newErr := auth.RewriteAuthError(ctx, cfg.Host, cfg.AccountID, cfg.Profile, err); rewritten {
+		return newErr
+	}
 	return err
 }
diff --git a/experimental/ssh/internal/proxy/client_server_test.go b/experimental/ssh/internal/proxy/client_server_test.go
@@ -165,9 +165,7 @@ func TestHandover(t *testing.T) {
 	}
 
 	wg := sync.WaitGroup{}
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
+	wg.Go(func() {
 		handoverCount := 0
 		for {
 			select {
@@ -182,7 +180,7 @@ func TestHandover(t *testing.T) {
 				time.Sleep(time.Millisecond)
 			}
 		}
-	}()
+	})
 
 	wg.Wait()
 
diff --git a/experimental/ssh/internal/proxy/connections_test.go b/experimental/ssh/internal/proxy/connections_test.go
@@ -91,13 +91,11 @@ func TestConnectionsManager_ThreadSafety(t *testing.T) {
 	var wg sync.WaitGroup
 
 	for i := range numGoroutines {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
+		wg.Go(func() {
 			for j := range numOperationsPerGoroutine {
 				cm.TryAdd(fmt.Sprintf("conn-%d-%d", i, j), &proxyConnection{})
 			}
-		}()
+		})
 	}
 
 	wg.Wait()
@@ -108,33 +106,27 @@ func TestConnectionsManager_ThreadSafety(t *testing.T) {
 
 	// Now do concurrent gets, removes, and counts
 	for i := range numGoroutines {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
+		wg.Go(func() {
 			for j := range numOperationsPerGoroutine {
 				cm.Get(fmt.Sprintf("conn-%d-%d", i, j))
 			}
-		}()
+		})
 	}
 
 	for i := range numGoroutines {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
+		wg.Go(func() {
 			for j := range numOperationsPerGoroutine {
 				cm.Remove(fmt.Sprintf("conn-%d-%d", i, j))
 			}
-		}()
+		})
 	}
 
 	for range numGoroutines {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
+		wg.Go(func() {
 			for range numOperationsPerGoroutine {
 				cm.Count()
 			}
-		}()
+		})
 	}
 
 	wg.Wait()
diff --git a/integration/libs/locker/locker_test.go b/integration/libs/locker/locker_test.go
@@ -39,12 +39,10 @@ func TestLock(t *testing.T) {
 
 	var wg sync.WaitGroup
 	for currentIndex := range numConcurrentLocks {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
+		wg.Go(func() {
 			time.Sleep(time.Duration(rand.Intn(100)) * time.Millisecond)
 			lockerErrs[currentIndex] = lockers[currentIndex].Lock(ctx, false)
-		}()
+		})
 	}
 	wg.Wait()
 
diff --git a/internal/testcli/runner.go b/internal/testcli/runner.go
@@ -45,10 +45,8 @@ type Runner struct {
 
 func consumeLines(ctx context.Context, wg *sync.WaitGroup, r io.Reader) <-chan string {
 	ch := make(chan string, 30000)
-	wg.Add(1)
-	go func() {
+	wg.Go(func() {
 		defer close(ch)
-		defer wg.Done()
 		scanner := bufio.NewScanner(r)
 		for scanner.Scan() {
 			// We expect to be able to always send these lines into the channel.
@@ -63,7 +61,7 @@ func consumeLines(ctx context.Context, wg *sync.WaitGroup, r io.Reader) <-chan s
 				panic("line buffer is full")
 			}
 		}
-	}()
+	})
 	return ch
 }
 
@@ -250,11 +248,9 @@ func (r *Runner) Eventually(condition func() bool, waitFor, tick time.Duration,
 	defer wg.Wait()
 
 	// Kick off condition check immediately.
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
+	wg.Go(func() {
 		ch <- condition()
-	}()
+	})
 
 	for tick := ticker.C; ; {
 		select {
@@ -266,11 +262,9 @@ func (r *Runner) Eventually(condition func() bool, waitFor, tick time.Duration,
 			return
 		case <-tick:
 			tick = nil
-			wg.Add(1)
-			go func() {
-				defer wg.Done()
+			wg.Go(func() {
 				ch <- condition()
-			}()
+			})
 		case v := <-ch:
 			if v {
 				return
diff --git a/libs/auth/error.go b/libs/auth/error.go
@@ -9,19 +9,19 @@ import (
 )
 
 // RewriteAuthError rewrites the error message for invalid refresh token error.
-// It returns the rewritten error and a boolean indicating whether the error was rewritten.
-func RewriteAuthError(ctx context.Context, host, accountId, profile string, err error) (error, bool) {
+// It returns whether the error was rewritten and the rewritten error.
+func RewriteAuthError(ctx context.Context, host, accountId, profile string, err error) (bool, error) {
 	target := &u2m.InvalidRefreshTokenError{}
 	if errors.As(err, &target) {
 		oauthArgument, err := AuthArguments{host, accountId}.ToOAuthArgument()
 		if err != nil {
-			return err, false
+			return false, err
 		}
 		msg := `A new access token could not be retrieved because the refresh token is invalid. To reauthenticate, run the following command:
   $ ` + BuildLoginCommand(ctx, profile, oauthArgument)
-		return errors.New(msg), true
+		return true, errors.New(msg)
 	}
-	return err, false
+	return false, err
 }
 
 // BuildLoginCommand builds the login command for the given OAuth argument or profile.
diff --git a/libs/cmdio/render.go b/libs/cmdio/render.go
@@ -227,7 +227,7 @@ func newBufferedFlusher(w io.Writer) writeFlusher {
 	}
 }
 
-func renderWithTemplate(r any, ctx context.Context, outputFormat flags.Output, w io.Writer, headerTemplate, template string) error {
+func renderWithTemplate(ctx context.Context, r any, outputFormat flags.Output, w io.Writer, headerTemplate, template string) error {
 	// TODO: add terminal width & white/dark theme detection
 	switch outputFormat {
 	case flags.OutputJSON:
@@ -260,30 +260,30 @@ func Render(ctx context.Context, v any) error {
 	if _, ok := v.(listingInterface); ok {
 		panic("use RenderIterator instead")
 	}
-	return renderWithTemplate(newRenderer(v), ctx, c.outputFormat, c.out, c.headerTemplate, c.template)
+	return renderWithTemplate(ctx, newRenderer(v), c.outputFormat, c.out, c.headerTemplate, c.template)
 }
 
 func RenderIterator[T any](ctx context.Context, i listing.Iterator[T]) error {
 	c := fromContext(ctx)
-	return renderWithTemplate(newIteratorRenderer(i), ctx, c.outputFormat, c.out, c.headerTemplate, c.template)
+	return renderWithTemplate(ctx, newIteratorRenderer(i), c.outputFormat, c.out, c.headerTemplate, c.template)
 }
 
 func RenderWithTemplate(ctx context.Context, v any, headerTemplate, template string) error {
 	c := fromContext(ctx)
 	if _, ok := v.(listingInterface); ok {
 		panic("use RenderIteratorWithTemplate instead")
 	}
-	return renderWithTemplate(newRenderer(v), ctx, c.outputFormat, c.out, headerTemplate, template)
+	return renderWithTemplate(ctx, newRenderer(v), c.outputFormat, c.out, headerTemplate, template)
 }
 
 func RenderIteratorWithTemplate[T any](ctx context.Context, i listing.Iterator[T], headerTemplate, template string) error {
 	c := fromContext(ctx)
-	return renderWithTemplate(newIteratorRenderer(i), ctx, c.outputFormat, c.out, headerTemplate, template)
+	return renderWithTemplate(ctx, newIteratorRenderer(i), c.outputFormat, c.out, headerTemplate, template)
 }
 
 func RenderIteratorJson[T any](ctx context.Context, i listing.Iterator[T]) error {
 	c := fromContext(ctx)
-	return renderWithTemplate(newIteratorRenderer(i), ctx, c.outputFormat, c.out, c.headerTemplate, c.template)
+	return renderWithTemplate(ctx, newIteratorRenderer(i), c.outputFormat, c.out, c.headerTemplate, c.template)
 }
 
 var renderFuncMap = template.FuncMap{
diff --git a/libs/dyn/time.go b/libs/dyn/time.go
diff --git a/libs/exec/exec_test.go b/libs/exec/exec_test.go
diff --git a/libs/log/logger.go b/libs/log/logger.go
diff --git a/libs/log/sdk.go b/libs/log/sdk.go

Original file line number	Diff line number	Diff line change
`@@ -38,15 +38,13 @@ func ApplyParallel(ctx context.Context, b *Bundle, mutators ...ReadOnlyMutator)`
`38`	`38`	`}`
`39`	`39`
`40`	`40`	`for ind, m := range mutators {`
`41`		`- wg.Add(1)`
`42`		`- go func() {`
`43`		`- defer wg.Done()`
	`41`	`+ wg.Go(func() {`
`44`	`42`	`// We're not using bundle.ApplyContext here because we don't do copy between typed and dynamic values`
`45`	`43`	`diags := m.Apply(contexts[ind], b)`
`46`	`44`	`for _, d := range diags {`
`47`	`45`	`logdiag.LogDiag(ctx, d)`
`48`	`46`	`}`
`49`		`- }()`
	`47`	`+ })`
`50`	`48`	`}`
`51`	`49`
`52`	`50`	`wg.Wait()`
Original file line number	Diff line number	Diff line change
`@@ -129,8 +129,8 @@ func loadToken(ctx context.Context, args loadTokenArgs) (*oauth2.Token, error) {`
`129`	`129`	`// This is captured in an acceptance test under "cmd/auth/token".`
`130`	`130`	`err = errors.New("cache: databricks OAuth is not configured for this host")`
`131`	`131`	`}`
`132`		`- if err, ok := auth.RewriteAuthError(ctx, args.authArguments.Host, args.authArguments.AccountID, args.profileName, err); ok {`
`133`		`- return nil, err`
	`132`	`+ if rewritten, rewrittenErr := auth.RewriteAuthError(ctx, args.authArguments.Host, args.authArguments.AccountID, args.profileName, err); rewritten {`
	`133`	`+ return nil, rewrittenErr`
`134`	`134`	`}`
`135`	`135`	`helpMsg := helpfulError(ctx, args.profileName, oauthArgument)`
`136`	`136`	`return nil, fmt.Errorf("%w. %s", err, helpMsg)`
Original file line number	Diff line number	Diff line change
`@@ -314,6 +314,8 @@ func emptyHttpRequest(ctx context.Context) *http.Request {`
`314`	`314`	`}`
`315`	`315`
`316`	`316`	`func renderError(ctx context.Context, cfg *config.Config, err error) error {`
`317`		`- err, _ = auth.RewriteAuthError(ctx, cfg.Host, cfg.AccountID, cfg.Profile, err)`
	`317`	`+ if rewritten, newErr := auth.RewriteAuthError(ctx, cfg.Host, cfg.AccountID, cfg.Profile, err); rewritten {`
	`318`	`+ return newErr`
	`319`	`+ }`
`318`	`320`	`return err`
`319`	`321`	`}`
Original file line number	Diff line number	Diff line change
`@@ -165,9 +165,7 @@ func TestHandover(t *testing.T) {`
`165`	`165`	`}`
`166`	`166`
`167`	`167`	`wg := sync.WaitGroup{}`
`168`		`- wg.Add(1)`
`169`		`- go func() {`
`170`		`- defer wg.Done()`
	`168`	`+ wg.Go(func() {`
`171`	`169`	`handoverCount := 0`
`172`	`170`	`for {`
`173`	`171`	`select {`
`@@ -182,7 +180,7 @@ func TestHandover(t *testing.T) {`
`182`	`180`	`time.Sleep(time.Millisecond)`
`183`	`181`	`}`
`184`	`182`	`}`
`185`		`- }()`
	`183`	`+ })`
`186`	`184`
`187`	`185`	`wg.Wait()`
`188`	`186`