Repository: spf13/viper
Pull request #1696 (2023-11-30 15:45:45)

fix: gocritic lint issues
This PR enables [`gocritic` ](https://go-critic.com/)linter and fixes found lint issues.

The full log of running golangci-lint:
```sh
❯ golangci-lint run
internal/encoding/ini/map_utils.go:45:1: paramTypeCombine: func(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any could be replaced with func(shadow, m map[string]any, prefix, delimiter string) map[string]any (gocritic)
func flattenAndMergeMap(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any {
^
internal/encoding/javaproperties/map_utils.go:45:1: paramTypeCombine: func(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any could be replaced with func(shadow, m map[string]any, prefix, delimiter string) map[string]any (gocritic)
func flattenAndMergeMap(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any {
^
internal/encoding/dotenv/map_utils.go:12:1: paramTypeCombine: func(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any could be replaced with func(shadow, m map[string]any, prefix, delimiter string) map[string]any (gocritic)
func flattenAndMergeMap(shadow map[string]any, m map[string]any, prefix string, delimiter string) map[string]any {
^
remote/remote.go:47:1: unnamedResult: consider giving a name to these results (gocritic)
func (rc remoteConfigProvider) WatchChannel(rp viper.RemoteProvider) (<-chan *viper.RemoteResponse, chan bool) {
^
viper.go:960:18: appendAssign: append result not assigned to the same slice (gocritic)
                subv.parents = append(v.parents, strings.ToLower(key))
                               ^
viper.go:1412:5: emptyStringTest: replace `len(val) == 0` with `val == ""` (gocritic)
        if len(val) == 0 {
           ^
viper.go:1436:5: emptyStringTest: replace `len(val) == 0` with `val == ""` (gocritic)
        if len(val) == 0 {
           ^
viper.go:1484:1: paramTypeCombine: func(alias string, key string) could be replaced with func(alias, key string) (gocritic)
func RegisterAlias(alias string, key string) { v.RegisterAlias(alias, key) }
^
viper.go:1486:1: paramTypeCombine: func(alias string, key string) could be replaced with func(alias, key string) (gocritic)
func (v *Viper) RegisterAlias(alias string, key string) {
^
viper.go:1490:1: paramTypeCombine: func(alias string, key string) could be replaced with func(alias, key string) (gocritic)
func (v *Viper) registerAlias(alias string, key string) {
^
overrides_test.go:100:26: emptyStringTest: replace `len(firstKeys[0]) == 0` with `firstKeys[0] == ""` (gocritic)
                len(firstKeys) == 0 || len(firstKeys[0]) == 0 {
                                       ^
overrides_test.go:132:21: emptyStringTest: replace `len(keys[0]) == 0` with `keys[0] == ""` (gocritic)
                len(keys) == 0 || len(keys[0]) == 0 {
                                  ^
overrides_test.go:118:29: emptyStringTest: replace `len(secondKeys[0]) == 0` with `secondKeys[0] == ""` (gocritic)
        if len(secondKeys) == 0 || len(secondKeys[0]) == 0 {
                                   ^
viper_test.go:933:14: evalOrder: may want to evaluate json.Unmarshal([]byte(raw), &m) before the return statement (gocritic)
                        return m, json.Unmarshal([]byte(raw), &m)
                                  ^
viper_test.go:431:12: preferStringWriter: file.WriteString(`key: value`) should be preferred to the file.Write([]byte(`key: value`)) (gocritic)
                _, err = file.Write([]byte(`key: value`))
                         ^
viper_test.go:456:12: preferStringWriter: file.WriteString(`key: value`) should be preferred to the file.Write([]byte(`key: value`)) (gocritic)
                _, err = file.Write([]byte(`key: value`))
                         ^
viper_test.go:2509:1: sloppyTestFuncName: function skipWindows should be of form TestXXX(t *testing.T) (gocritic)
func skipWindows(t *testing.T) {
^
viper_test.go:237:1: unnamedResult: consider giving a name to these results (gocritic)
func initDirs(t *testing.T) (string, string) {
^
viper_test.go:2240:1: unnamedResult: consider giving a name to these results (gocritic)
func newViperWithConfigFile(t *testing.T) (*Viper, string) {
^
viper_test.go:2253:1: unnamedResult: consider giving a name to these results (gocritic)
func newViperWithSymlinkedConfigFile(t *testing.T) (*Viper, string, string) {
^
func (c Codec) Encode(v map[string]any) ([]byte, error) {
      ^
internal/encoding/ini/codec.go:65:7: hugeParam: c is heavy (130 bytes); consider passing it by pointer (gocritic)
func (c Codec) Decode(b []byte, v map[string]any) error {
      ^
internal/encoding/ini/codec.go:93:7: hugeParam: c is heavy (130 bytes); consider passing it by pointer (gocritic)
func (c Codec) keyDelimiter() string {
      ^
logger.go:58:52: hugeParam: _ is heavy (288 bytes); consider passing it by pointer (gocritic)
func (n *discardHandler) Handle(_ context.Context, _ slog.Record) error {
                                                   ^
viper.go:2156:21: hugeParam: in is heavy (114 bytes); consider passing it by pointer (gocritic)
func IniLoadOptions(in ini.LoadOptions) Option {
                    ^
```

github-actions[bot]: 👋 Thanks for contributing to Viper! You are awesome! 🎉

A maintainer will take a look at your pull request shortly. 👀

In the meantime: We are working on **Viper v2** and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news,
either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

**Thank you!** ❤️


Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 53d66e2..a127854 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -57,11 +57,11 @@ jobs:
           go-version: ${{ matrix.go }}
 
       - name: Test
-        run: go test -race -v -tags '${{ matrix.tags }}' ./...
+        run: go test -race -v -tags '${{ matrix.tags }}' -shuffle=on ./...
         if: runner.os != 'Windows'
 
       - name: Test (without race detector)
-        run: go test -v -tags '${{ matrix.tags }}' ./...
+        run: go test -v -tags '${{ matrix.tags }}' -shuffle=on ./...
         if: runner.os == 'Windows'
 
   lint:
diff --git a/.golangci.yaml b/.golangci.yaml
index fa28eb3..1faeae4 100644
--- a/.golangci.yaml
+++ b/.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
+            - style
+        disabled-checks:
+            - importShadow
+            - unnamedResult
     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
diff --git a/internal/encoding/dotenv/map_utils.go b/internal/encoding/dotenv/map_utils.go
index 4901d7c..8bfe0a9 100644
--- a/internal/encoding/dotenv/map_utils.go
+++ b/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
diff --git a/internal/encoding/ini/map_utils.go b/internal/encoding/ini/map_utils.go
index 06e703f..490ab59 100644
--- a/internal/encoding/ini/map_utils.go
+++ b/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
diff --git a/internal/encoding/javaproperties/map_utils.go b/internal/encoding/javaproperties/map_utils.go
index 7c7e78a..6e1aff2 100644
--- a/internal/encoding/javaproperties/map_utils.go
+++ b/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
diff --git a/viper.go b/viper.go
index 7de2e78..7f54c26 100644
--- a/viper.go
+++ b/viper.go
@@ -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
@@ -1111,7 +1112,32 @@ func Unmarshal(rawVal any, opts ...DecoderConfigOption) error {
 }
 
 func (v *Viper) Unmarshal(rawVal any, opts ...DecoderConfigOption) error {
-	return decode(v.AllSettings(), defaultDecoderConfig(rawVal, opts...))
+	// TODO: make this optional?
+	structKeys, err := v.decodeStructKeys(rawVal, opts...)
+	if err != nil {
+		return err
+	}
+
+	// TODO: struct keys should be enough?
+	return decode(v.getSettings(append(v.AllKeys(), structKeys...)), defaultDecoderConfig(rawVal, opts...))
+}
+
+func (v *Viper) decodeStructKeys(input any, opts ...DecoderConfigOption) ([]string, error) {
+	var structKeyMap map[string]any
+
+	err := decode(input, defaultDecoderConfig(&structKeyMap, opts...))
+	if err != nil {
+		return nil, err
+	}
+
+	flattenedStructKeyMap := v.flattenAndMergeMap(map[string]bool{}, structKeyMap, "")
+
+	r := make([]string, 0, len(flattenedStructKeyMap))
+	for v := range flattenedStructKeyMap {
+		r = append(r, v)
+	}
+
+	return r, nil
 }
 
 // defaultDecoderConfig returns default mapstructure.DecoderConfig with support
@@ -1409,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))
@@ -1433,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, ",")
@@ -1481,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]
@@ -2098,9 +2124,13 @@ outer:
 func AllSettings() map[string]any { return v.AllSettings() }
 
 func (v *Viper) AllSettings() map[string]any {
+	return v.getSettings(v.AllKeys())
+}
+
+func (v *Viper) getSettings(keys []string) map[string]any {
 	m := map[string]any{}
 	// start from the list of keys, and construct the map one value at a time
-	for _, k := range v.AllKeys() {
+	for _, k := range keys {
 		value := v.Get(k)
 		if value == nil {
 			// should not happen, since AllKeys() returns only keys holding a value,


Test patch:
diff --git a/flags_test.go b/flags_test.go
index 0978439..18a5403 100644
--- a/flags_test.go
+++ b/flags_test.go
@@ -9,6 +9,7 @@ import (
 )
 
 func TestBindFlagValueSet(t *testing.T) {
+	Reset()
 	flagSet := pflag.NewFlagSet("test", pflag.ContinueOnError)
 
 	testValues := map[string]*string{
diff --git a/overrides_test.go b/overrides_test.go
index a8f4cc1..42da3ba 100644
--- a/overrides_test.go
+++ b/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
 	}
 
diff --git a/viper_test.go b/viper_test.go
index 0e416e7..2dd67a1 100644
--- a/viper_test.go
+++ b/viper_test.go
@@ -428,7 +428,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 +453,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()
@@ -471,6 +471,7 @@ func TestReadInConfig(t *testing.T) {
 }
 
 func TestDefault(t *testing.T) {
+	Reset()
 	SetDefault("age", 45)
 	assert.Equal(t, 45, Get("age"))
 
@@ -485,6 +486,7 @@ func TestDefault(t *testing.T) {
 }
 
 func TestUnmarshaling(t *testing.T) {
+	Reset()
 	SetConfigType("yaml")
 	r := bytes.NewReader(yamlExample)
 
@@ -521,6 +523,8 @@ func TestDefaultPost(t *testing.T) {
 }
 
 func TestAliases(t *testing.T) {
+	initConfigs()
+	Set("age", 40)
 	RegisterAlias("years", "age")
 	assert.Equal(t, 40, Get("years"))
 	Set("years", 45)
@@ -528,6 +532,7 @@ func TestAliases(t *testing.T) {
 }
 
 func TestAliasInConfigFile(t *testing.T) {
+	initConfigs()
 	// the config file specifies "beard".  If we make this an alias for
 	// "hasbeard", we still want the old config file to work with beard.
 	RegisterAlias("beard", "hasbeard")
@@ -870,6 +875,7 @@ func TestRecursiveAliases(t *testing.T) {
 }
 
 func TestUnmarshal(t *testing.T) {
+	Reset()
 	SetDefault("port", 1313)
 	Set("name", "Steve")
 	Set("duration", "1s1ms")
@@ -930,7 +936,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
 		},
 	))
 
@@ -948,6 +955,105 @@ func TestUnmarshalWithDecoderOptions(t *testing.T) {
 	}, &C)
 }
 
+func TestUnmarshalWithAutomaticEnv(t *testing.T) {
+	t.Setenv("PORT", "1313")
+	t.Setenv("NAME", "Steve")
+	t.Setenv("DURATION", "1s1ms")
+	t.Setenv("MODES", "1,2,3")
+	t.Setenv("SECRET", "42")
+	t.Setenv("FILESYSTEM_SIZE", "4096")
+
+	type AuthConfig struct {
+		Secret string `mapstructure:"secret"`
+	}
+
+	type StorageConfig struct {
+		Size int `mapstructure:"size"`
+	}
+
+	type Configuration struct {
+		Port     int           `mapstructure:"port"`
+		Name     string        `mapstructure:"name"`
+		Duration time.Duration `mapstructure:"duration"`
+
+		// Infer name from struct
+		Modes []int
+
+		// Squash nested struct (omit prefix)
+		Authentication AuthConfig `mapstructure:",squash"`
+
+		// Different key
+		Storage StorageConfig `mapstructure:"filesystem"`
+
+		// Omitted field
+		Flag bool `mapstructure:"flag"`
+	}
+
+	v := New()
+	v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
+	v.AutomaticEnv()
+
+	t.Run("OK", func(t *testing.T) {
+		var config Configuration
+		if err := v.Unmarshal(&config); err != nil {
+			t.Fatalf("unable to decode into struct, %v", err)
+		}
+
+		assert.Equal(
+			t,
+			Configuration{
+				Name:     "Steve",
+				Port:     1313,
+				Duration: time.Second + time.Millisecond,
+				Modes:    []int{1, 2, 3},
+				Authentication: AuthConfig{
+					Secret: "42",
+				},
+				Storage: StorageConfig{
+					Size: 4096,
+				},
+			},
+			config,
+		)
+	})
+
+	t.Run("Precedence", func(t *testing.T) {
+		var config Configuration
+
+		v.Set("port", 1234)
+		if err := v.Unmarshal(&config); err != nil {
+			t.Fatalf("unable to decode into struct, %v", err)
+		}
+
+		assert.Equal(
+			t,
+			Configuration{
+				Name:     "Steve",
+				Port:     1234,
+				Duration: time.Second + time.Millisecond,
+				Modes:    []int{1, 2, 3},
+				Authentication: AuthConfig{
+					Secret: "42",
+				},
+				Storage: StorageConfig{
+					Size: 4096,
+				},
+			},
+			config,
+		)
+	})
+
+	t.Run("Unset", func(t *testing.T) {
+		var config Configuration
+
+		err := v.Unmarshal(&config, func(config *mapstructure.DecoderConfig) {
+			config.ErrorUnset = true
+		})
+
+		assert.Error(t, err, "expected viper.Unmarshal to return error due to unset field 'FLAG'")
+	})
+}
+
 func TestBindPFlags(t *testing.T) {
 	v := New() // create independent Viper object
 	flagSet := pflag.NewFlagSet("test", pflag.ContinueOnError)
@@ -1242,6 +1348,7 @@ func TestBindPFlagStringToInt(t *testing.T) {
 }
 
 func TestBoundCaseSensitivity(t *testing.T) {
+	initConfigs()
 	assert.Equal(t, "brown", Get("eyes"))
 
 	BindEnv("eYEs", "TURTLE_EYES")
@@ -2506,6 +2613,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")