Unexpected outcomes while trying to merge two instances of type reflect.Value

[akamble-splunk]

https://play.golang.org/p/Jewi51WT-gZ

The expected outcome here is:

printing overlayed
&{b1 [] 1}
&{b2 [{d1 f1 t1} {d2 f2 t2}] 2}

but we get:

printing overlayed
&{b1 [] 1}
&{b2 [{d1 f1 t1} {d2 f2 t2}] 0}

which means if the dest has no value provided, the value from src will be used.

I tried this with a few different versions of the library: specifically 0.3.7, 0.3.8 & 0.3.10. Got the same response in all cases.

Although, with the version 0.3.9, I got a different response, still incorrect

printing overlayed
&{b1 [] 1}
&{b2 [] 2}

The field value from the base is preserved, but the slice value from override is lost.

We don't see this issue if the values are converted to respective structs first and then invoke mergo.Merge: https://play.golang.org/p/t-ND1sobjUb
We get the expected outcome in this case.

[xscode-auto-reply]

Thanks for opening a new issue. The team has been notified and will review it as soon as possible.
For urgent issues and priority support, visit https://xscode.com/imdario/mergo

[darccio]

I'm not sure trying to merge reflect.Value is the safest option. It seems to be mergeable because the struct contains a pointer to the underlying data, but it feels like you are relying on an internal implementation detail for that, which is usually a bad idea.

I would go for the code in your second link, it looks clearer and less magic. Anyway, I will take this into account in the next big version of Mergo, written from scratch.

If you provide a PR for the current version, I won't mind reviewing and merging it.

[bionoren]

The sample code is definitely not going to do what you want as provided, mostly because you don't actually want to merge go's internal reflect.Value structs. You actually want to merge the things in those reflect.Value structs. That's possible, but it would take a fair bit of work. Ultimately, it's imdario's call, but it sounds like he considers this out-of-scope.

In more detail:
You're asking mergo to merge two reflect.Value structs. They do have type struct, and they're even the same type of struct, so technically this package can try to merge them, but reflect.Value doesn't have any exported fields. As the documentation states, "Mergo won't merge unexported (private) fields." This is really a language limitation, and there's not much this library could do about it.

The obvious thing to try would be to call .Interface() on your reflect.Value before passing it into mergo. That could work, but it doesn't for a couple reasons:

1. mergo doesn't accept interface{} as the top level value, only structs and maps (see package documentation) You could solve that by trying to resolve the interface{} to a struct or a map in the library, but I don't know if the author is interested in supporting that. I don't see any obvious technical barriers, but it's definitely outside the documented scope of allowed inputs. It would need to be solved inside mergo though, since I'm not aware of any way to convert reflect.Value to its contained concrete type without knowing what that type is. If you knew what the underlying concrete type was, presumably you wouldn't be trying to work with reflect.Value.
2. If you solve that, you're going to run into the same problem nested non-addressable values cannot be merged #162 is reporting - the value inside your interface isn't addressable, simply because it's in an interface. There are good, if complex, reasons for this that are documented on that issue. Again, this is solvable, but it's not a trivial change.

If you could get all that working, then you could lift it into the package and support reflect.Value as a valid input type. I personally wouldn't go that far, but not my decision

[darccio]

@bionoren Thanks for your insightful comment. This is something I'm willing to support in the next major version, but I don't have a defined ETA for it.

Currently, it's hard to add changes. The cognitive load of the code is above what I can handle properly. This is an undesired byproduct of my liberal policy of accepting PRs, and trying to keep it as flexible as possible.

[rpflynn22]

Just wanted to circle back on this. I was able to change the code in the original post to make it behave as desired. The problem seemed to be making an unnecessary call to the reflect Value's Elem method, which was essentially dereferencing the underlying pointer type.

I would like to point out that, documented or not, this library seems to gladly accept interface{} types in the Merge function as long as the reflect kind is a pointer to a struct (maybe pointer to map works too, I didn't try). Simple illustration here.

In any case, altering the original author's code to pass the non-dereferenced, interface{}-casted values to the Merge function, along with some compensating changes, was enough to get it to behave as desired: https://play.golang.org/p/_r3fc_UikdX