Unnecessary initialization of struct pointers

[na--]

If I have the following code and haven't actually defined any of the environment variables:

package main

import (
	"github.com/davecgh/go-spew/spew"
	"github.com/kelseyhightower/envconfig"
)

type SomeItem struct {
	Blah string
}

type Config struct {
	Data        string
	ItemVal     SomeItem
	ItemPointer *SomeItem
}

func main() {
	conf := Config{}
	envconfig.Process("wtf", &conf)
	spew.Dump(conf)
}

I'd expect to get this:

(main.Config) {
 Data: (string) "",
 ItemVal: (main.SomeItem) {
  Blah: (string) ""
 },
 ItemPointer: (*main.SomeItem)(<nil>)
}

but I actually get:

(main.Config) {
 Data: (string) "",
 ItemVal: (main.SomeItem) {
  Blah: (string) ""
 },
 ItemPointer: (*main.SomeItem)(0xc42000e400)({
  Blah: (string) ""
 })
}

For some reason, envconfig has created an empty SomeItem and assigned its address to conf.ItemPointer.

[rohancme]

@na-- wondering if you ended up using a workaround for this? I'm in a similar situation where I want to validate the values in a sub-struct if at least one property has been set on it.

[na--]

Unfortunately not, but I haven't had the time to investigate this very thoroughly yet... And there's a good chance I won't get to it anyway, since we may have to switch libraries due to some of envconfig's other "features"

[teepark]

I think I have a general handle on the issue, and it's a pretty significant structural one.

envconfig first makes a recursive pass gathering information about types, and returning deep pointers to the fields to populate. This makes the processing phase relatively simple, but requires that those fields all exist in the first place. That's why initializing all these pointers becomes necessary.

I managed to get your example to build the right result, but for the obvious reason this broke the basic functionality of populating nested fields. It'll be a significant refactor to get this right, but I don't have the time to do it.

The first part of the puzzle is to not perform any writes to the spec in gatherInfo:

diff --git a/envconfig.go b/envconfig.go
index 3f16108..2eb549d 100644
--- a/envconfig.go
+++ b/envconfig.go
@@ -85,10 +85,11 @@ func gatherInfo(prefix string, spec interface{}) ([]varInfo, error) {
                                        // nil pointer to a non-struct: leave it alone
                                        break
                                }
-                               // nil pointer to struct: create a zero instance
-                               f.Set(reflect.New(f.Type().Elem()))
+                               // nil pointer to struct: use a new zero instance
+                               f = reflect.New(f.Type().Elem())
+                       } else {
+                               f = f.Elem()
                        }
-                       f = f.Elem()
                }

                // Capture information about the config variable

[savely-krasovsky]

@teepark well, I tried you approatch, but unsuccessfully. Parser should be rewritten with this thought in mind.

Workaround solution is to use the new Go 1.13 function IsZero:

--- envconfig.go	(revision 0b417c4ec4a8a82eecc22a1459a504aa55163d61)
+++ envconfig.go	(date 1572281234355)
@@ -224,9 +224,47 @@
 		}
 	}
 
+	err = removeEmptyStructs(spec)
+	if err != nil {
+		return err
+	}
+
 	return err
 }
 
+func removeEmptyStructs(spec interface{}) error {
+	s := reflect.ValueOf(spec)
+
+	if s.Kind() != reflect.Ptr {
+		return ErrInvalidSpecification
+	}
+	s = s.Elem()
+	if s.Kind() != reflect.Struct {
+		return ErrInvalidSpecification
+	}
+
+	for i := 0; i < s.NumField(); i++ {
+		f := s.Field(i)
+
+		if f.Kind() == reflect.Ptr {
+			if !f.IsNil() {
+				if f.Elem().IsZero() {
+					f.Set(reflect.Zero(f.Type()))
+				}
+			}
+
+			if f.Elem().Kind() == reflect.Struct {
+				err := removeEmptyStructs(f.Elem().Addr().Interface())
+				if err != nil {
+					return err
+				}
+			}
+		}
+	}
+
+	return nil
+}
+
 // MustProcess is the same as Process but panics if an error occurs
 func MustProcess(prefix string, spec interface{}) {
 	if err := Process(prefix, spec); err != nil {

This function recursively sets empty structs to nil.

[hexdigest]

@teepark @L11R Here is the correct version of the removeEmptyStructs that support multiple levels of structs nesting when some levels can be pointers and some not:

https://github.com/hexdigest/envconfig/blob/master/envconfig.go#L230:L263

I also added tests for this func. Hope it helps to everyone who struggle from the same issue.

@kelseyhightower not quite sure that :point_up: thing can be a good candidate for a PR because it's more of a cleanup rather than an actual fix for the env parser even though I don't see how it can break something.

[kevinpark1217]

@hexdigest I think you should still submit a PR! Your change fixed the problem

[hexdigest]

@kevinpark1217 here it is https://github.com/kelseyhightower/envconfig/pull/201