deriveClone should deal with nil better

[deosjr]

Consider the following code

original := map[string]int{}
cloned := deriveClone(s)
if cloned == nil {
  cloned = map[string]int{}
}
cloned["key"] = 42

Right now, if we do not include the nil check, this code will panic if the original map s is nil. In both the slice and the map case, it might be better to return an empty but valid value.

To enforce this intuition, let's look at the code that deriveClone normally replaces in this case:

original := map[string]int{}
cloned := map[string]int{}
for k, v := range original {
  cloned[k] = v
}
cloned["key"] = 42

And likewise for slices. If original is nil we iterate over nothing and immediately return an empty but valid value.

[awalterschulze]

Thank you for reporting, this should be fixed.

It is really weird, since the it seems as if a nil pointer check should be generated https://github.com/awalterschulze/goderive/blob/master/plugin/clone/clone.go#L104-L108 I hope this will be an easy fix

A test can be added here https://github.com/awalterschulze/goderive/blob/master/test/normal/clone_test.go

[deosjr]

The nil pointer check is indeed generated, but simply returns nil. The proposal is to return an empty map/list instead. In the above code, I 'fix' this outside of derive by including another nil check.

I worry about this being a breaking change though. Reported it because you asked me to :) Are you sure this should be fixed? Perhaps this is a variant of deriveClone instead?

[awalterschulze]

Oh I see, I totally misunderstood before. Hmmm yes clone returning something more than you passed in would be weird to explain. I think we should close this case. Sorry for misunderstanding before.

We could rewrite this:

    m := deriveClone(s)
    if m == nil {
        m = map[string]*ast.SExpr{}
    }
    m[x.Name] = v
    return m, true

to prevent m from being nil to start with:

    m := map[string]*ast.SExpr{x.Name: v}
    deriveDeepCopy(s, m)
    return m, true

Reference to pull request where this conversation started https://github.com/awalterschulze/gominikanren/pull/4/files

[deosjr]

Oooh I did not know about DeepCopy, yes that is definitely better. Case closed as far as I'm concerned.