fix: gocritic lint issues

Author: alexandear

.golangci.yaml

@@ -7,6 +7,16 @@ linters-settings:
             - standard
             - default
             - prefix(github.com/spf13/viper)
+    gocritic:
+        # Enable multiple checks by tags. See "Tags" section in https://github.com/go-critic/go-critic#usage.
+        enabled-tags:
+            - diagnostic
+            - experimental
+            - opinionated
+            - performance
+            - style
+        disabled-checks:
+            - importShadow
     golint:
         min-confidence: 0
     goimports:
@@ -22,6 +32,7 @@ linters:
         - exhaustive
         - exportloopref
         - gci
+        - gocritic
         - godot
         - gofmt
         - gofumpt
@@ -63,7 +74,6 @@ linters:
         # - gochecknoinits
         # - gocognit
         # - goconst
-        # - gocritic
         # - gocyclo
         # - gosec
         # - gosimple

internal/encoding/dotenv/map_utils.go

@@ -9,7 +9,7 @@ import (
 // flattenAndMergeMap recursively flattens the given map into a new map
 // Code is based on the function with the same name in the main package.
 // TODO: move it to a common place.
-func flattenAndMergeMap(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any {
+func flattenAndMergeMap(shadow, m map[string]any, prefix, delimiter string) map[string]any {
 	if shadow != nil && prefix != "" && shadow[prefix] != nil {
 		// prefix is shadowed => nothing more to flatten
 		return shadow

internal/encoding/ini/codec.go

@@ -19,7 +19,7 @@ type Codec struct {
 	LoadOptions  LoadOptions
 }
 
-func (c Codec) Encode(v map[string]any) ([]byte, error) {
+func (c *Codec) Encode(v map[string]any) ([]byte, error) {
 	cfg := ini.Empty()
 	ini.PrettyFormat = false
 
@@ -62,7 +62,7 @@ func (c Codec) Encode(v map[string]any) ([]byte, error) {
 	return buf.Bytes(), nil
 }
 
-func (c Codec) Decode(b []byte, v map[string]any) error {
+func (c *Codec) Decode(b []byte, v map[string]any) error {
 	cfg := ini.Empty(c.LoadOptions)
 
 	err := cfg.Append(b)
@@ -90,7 +90,7 @@ func (c Codec) Decode(b []byte, v map[string]any) error {
 	return nil
 }
 
-func (c Codec) keyDelimiter() string {
+func (c *Codec) keyDelimiter() string {
 	if c.KeyDelimiter == "" {
 		return "."
 	}

internal/encoding/ini/map_utils.go

@@ -42,7 +42,7 @@ func deepSearch(m map[string]any, path []string) map[string]any {
 // flattenAndMergeMap recursively flattens the given map into a new map
 // Code is based on the function with the same name in the main package.
 // TODO: move it to a common place.
-func flattenAndMergeMap(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any {
+func flattenAndMergeMap(shadow, m map[string]any, prefix, delimiter string) map[string]any {
 	if shadow != nil && prefix != "" && shadow[prefix] != nil {
 		// prefix is shadowed => nothing more to flatten
 		return shadow

internal/encoding/javaproperties/map_utils.go

@@ -42,7 +42,7 @@ func deepSearch(m map[string]any, path []string) map[string]any {
 // flattenAndMergeMap recursively flattens the given map into a new map
 // Code is based on the function with the same name in the main package.
 // TODO: move it to a common place.
-func flattenAndMergeMap(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any {
+func flattenAndMergeMap(shadow, m map[string]any, prefix, delimiter string) map[string]any {
 	if shadow != nil && prefix != "" && shadow[prefix] != nil {
 		// prefix is shadowed => nothing more to flatten
 		return shadow

logger.go

@@ -55,6 +55,7 @@ func (n *discardHandler) Enabled(_ context.Context, _ slog.Level) bool {
 	return false
 }
 
+//nolint:gocritic // hugeParam: _ is heavy (288 bytes); consider passing it by pointer
 func (n *discardHandler) Handle(_ context.Context, _ slog.Record) error {
 	return nil
 }

overrides_test.go

@@ -97,7 +97,7 @@ func overrideFromLayer(l layer, assert *assert.Assertions, firstPath string, fir
 	v := New()
 	firstKeys := strings.Split(firstPath, v.keyDelim)
 	if assert == nil ||
-		len(firstKeys) == 0 || len(firstKeys[0]) == 0 {
+		len(firstKeys) == 0 || firstKeys[0] == "" {
 		return v
 	}
 
@@ -115,7 +115,7 @@ func overrideFromLayer(l layer, assert *assert.Assertions, firstPath string, fir
 
 	// Override and check new value
 	secondKeys := strings.Split(secondPath, v.keyDelim)
-	if len(secondKeys) == 0 || len(secondKeys[0]) == 0 {
+	if len(secondKeys) == 0 || secondKeys[0] == "" {
 		return v
 	}
 	v.Set(secondPath, secondValue)
@@ -129,7 +129,7 @@ func overrideFromLayer(l layer, assert *assert.Assertions, firstPath string, fir
 // configuration map of the given layer, and that the final value equals the one given.
 func deepCheckValue(assert *assert.Assertions, v *Viper, l layer, keys []string, value any) {
 	if assert == nil || v == nil ||
-		len(keys) == 0 || len(keys[0]) == 0 {
+		len(keys) == 0 || keys[0] == "" {
 		return
 	}
 

remote/remote.go

@@ -44,7 +44,7 @@ func (rc remoteConfigProvider) Watch(rp viper.RemoteProvider) (io.Reader, error)
 	return bytes.NewReader(resp), nil
 }
 
-func (rc remoteConfigProvider) WatchChannel(rp viper.RemoteProvider) (<-chan *viper.RemoteResponse, chan bool) {
+func (rc remoteConfigProvider) WatchChannel(rp viper.RemoteProvider) (responseCh <-chan *viper.RemoteResponse, quitCh chan bool) {
 	cm, err := getConfigManager(rp)
 	if err != nil {
 		return nil, nil

viper.go

@@ -345,7 +345,7 @@ func (v *Viper) resetEncoding() {
 	}
 
 	{
-		codec := ini.Codec{
+		codec := &ini.Codec{
 			KeyDelimiter: v.keyDelim,
 			LoadOptions:  v.iniLoadOptions,
 		}
@@ -957,7 +957,8 @@ func (v *Viper) Sub(key string) *Viper {
 	}
 
 	if reflect.TypeOf(data).Kind() == reflect.Map {
-		subv.parents = append(v.parents, strings.ToLower(key))
+		subv.parents = append([]string(nil), v.parents...)
+		subv.parents = append(subv.parents, strings.ToLower(key))
 		subv.automaticEnvApplied = v.automaticEnvApplied
 		subv.envPrefix = v.envPrefix
 		subv.envKeyReplacer = v.envKeyReplacer
@@ -1434,7 +1435,7 @@ func readAsCSV(val string) ([]string, error) {
 func stringToStringConv(val string) any {
 	val = strings.Trim(val, "[]")
 	// An empty string would cause an empty map
-	if len(val) == 0 {
+	if val == "" {
 		return map[string]any{}
 	}
 	r := csv.NewReader(strings.NewReader(val))
@@ -1458,7 +1459,7 @@ func stringToStringConv(val string) any {
 func stringToIntConv(val string) any {
 	val = strings.Trim(val, "[]")
 	// An empty string would cause an empty map
-	if len(val) == 0 {
+	if val == "" {
 		return map[string]any{}
 	}
 	ss := strings.Split(val, ",")
@@ -1506,13 +1507,13 @@ func (v *Viper) SetEnvKeyReplacer(r *strings.Replacer) {
 
 // RegisterAlias creates an alias that provides another accessor for the same key.
 // This enables one to change a name without breaking the application.
-func RegisterAlias(alias string, key string) { v.RegisterAlias(alias, key) }
+func RegisterAlias(alias, key string) { v.RegisterAlias(alias, key) }
 
-func (v *Viper) RegisterAlias(alias string, key string) {
+func (v *Viper) RegisterAlias(alias, key string) {
 	v.registerAlias(alias, strings.ToLower(key))
 }
 
-func (v *Viper) registerAlias(alias string, key string) {
+func (v *Viper) registerAlias(alias, key string) {
 	alias = strings.ToLower(alias)
 	if alias != key && alias != v.realKey(key) {
 		_, exists := v.aliases[alias]
@@ -2181,6 +2182,8 @@ func (v *Viper) SetConfigPermissions(perm os.FileMode) {
 }
 
 // IniLoadOptions sets the load options for ini parsing.
+//
+//nolint:gocritic // hugeParam: in is heavy (114 bytes); consider passing it by pointer
 func IniLoadOptions(in ini.LoadOptions) Option {
 	return optionFunc(func(v *Viper) {
 		v.iniLoadOptions = in

viper_test.go

@@ -234,17 +234,15 @@ func initIni() {
 }
 
 // initDirs makes directories for testing.
-func initDirs(t *testing.T) (string, string) {
-	var (
-		testDirs = []string{`a a`, `b`, `C_`}
-		config   = `improbable`
-	)
+func initDirs(t *testing.T) (root, config string) {
+	testDirs := []string{`a a`, `b`, `C_`}
+	config = `improbable`
 
 	if runtime.GOOS != "windows" {
 		testDirs = append(testDirs, `d\d`)
 	}
 
-	root := t.TempDir()
+	root = t.TempDir()
 
 	for _, dir := range testDirs {
 		innerDir := filepath.Join(root, dir)
@@ -428,7 +426,7 @@ func TestReadInConfig(t *testing.T) {
 		file, err := fs.Create(testutil.AbsFilePath(t, "/etc/viper/config.yaml"))
 		require.NoError(t, err)
 
-		_, err = file.Write([]byte(`key: value`))
+		_, err = file.WriteString(`key: value`)
 		require.NoError(t, err)
 
 		file.Close()
@@ -453,7 +451,7 @@ func TestReadInConfig(t *testing.T) {
 		file, err := fs.Create(testutil.AbsFilePath(t, "/etc/viper/config.yaml"))
 		require.NoError(t, err)
 
-		_, err = file.Write([]byte(`key: value`))
+		_, err = file.WriteString(`key: value`)
 		require.NoError(t, err)
 
 		file.Close()
@@ -936,7 +934,8 @@ func TestUnmarshalWithDecoderOptions(t *testing.T) {
 			if raw == "" {
 				return m, nil
 			}
-			return m, json.Unmarshal([]byte(raw), &m)
+			err := json.Unmarshal([]byte(raw), &m)
+			return m, err
 		},
 	))
 
@@ -2343,21 +2342,21 @@ func doTestCaseInsensitive(t *testing.T, typ, config string) {
 	assert.Equal(t, 5, cast.ToInt(Get("ef.lm.p.q")))
 }
 
-func newViperWithConfigFile(t *testing.T) (*Viper, string) {
+func newViperWithConfigFile(t *testing.T) (v *Viper, configFile string) {
 	watchDir := t.TempDir()
-	configFile := path.Join(watchDir, "config.yaml")
+	configFile = path.Join(watchDir, "config.yaml")
 	err := os.WriteFile(configFile, []byte("foo: bar\n"), 0o640)
 	require.NoError(t, err)
-	v := New()
+	v = New()
 	v.SetConfigFile(configFile)
 	err = v.ReadInConfig()
 	require.NoError(t, err)
 	require.Equal(t, "bar", v.Get("foo"))
 	return v, configFile
 }
 
-func newViperWithSymlinkedConfigFile(t *testing.T) (*Viper, string, string) {
-	watchDir := t.TempDir()
+func newViperWithSymlinkedConfigFile(t *testing.T) (v *Viper, watchDir, configFile string) {
+	watchDir = t.TempDir()
 	dataDir1 := path.Join(watchDir, "data1")
 	err := os.Mkdir(dataDir1, 0o777)
 	require.NoError(t, err)
@@ -2368,11 +2367,11 @@ func newViperWithSymlinkedConfigFile(t *testing.T) (*Viper, string, string) {
 	// now, symlink the tm `data1` dir to `data` in the baseDir
 	os.Symlink(dataDir1, path.Join(watchDir, "data"))
 	// and link the `<watchdir>/datadir1/config.yaml` to `<watchdir>/config.yaml`
-	configFile := path.Join(watchDir, "config.yaml")
+	configFile = path.Join(watchDir, "config.yaml")
 	os.Symlink(path.Join(watchDir, "data", "config.yaml"), configFile)
 	t.Logf("Config file location: %s\n", path.Join(watchDir, "config.yaml"))
 	// init Viper
-	v := New()
+	v = New()
 	v.SetConfigFile(configFile)
 	err = v.ReadInConfig()
 	require.NoError(t, err)
@@ -2612,6 +2611,8 @@ func BenchmarkGetBoolFromMap(b *testing.B) {
 }
 
 // Skip some tests on Windows that kept failing when Windows was added to the CI as a target.
+//
+//nolint:gocritic // sloppyTestFuncName
 func skipWindows(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("Skip test on Windows")