Regression: Unexpected dereference of returned struct pointer

[tooolbox]

I've observed a regression caused by https://github.com/CloudyKit/jet/commit/91ac9cb0201cc9d6b4130277c2f7e4b3685850f6 (#97)

Summary

For this template:

{{apple := GetAppleByName("honeycrisp")}}
{{TellFlavor(apple)}}

With this code:

func main() {

    // ...

    apples := map[string]*Apple{
        "honeycrisp": {
            Flavor: "crisp",
        },
        "red-delicious": {
            Flavor: "poor",
        },
        "granny-smith": {
            Flavor: "tart",
        },
    }

    // ...

    vars.SetFunc("GetAppleByName", func(a jet.Arguments) reflect.Value {
        name := a.Get(0).String()
        return reflect.ValueOf(apples[name])
    })

    vars.SetFunc("TellFlavor", func(a jet.Arguments) reflect.Value {
        apple := a.Get(0).Interface().(*Apple)
        flav := apple.GetFlavor()
        return reflect.ValueOf(flav)
    })

    // ...
}

type Apple struct {
    Flavor string
}

func (a *Apple) GetFlavor() string {
    return a.Flavor
}

The expected output would be crisp and this worked fine with v2.1.2

After https://github.com/CloudyKit/jet/commit/91ac9cb0201cc9d6b4130277c2f7e4b3685850f6 the output is:

panic: interface conversion: interface {} is main.Apple, not *main.Apple
// stack trace elided

Example Project

https://github.com/tooolbox/jet-example has two branches that cover this:

• v2.1.2-pointers (working)
• v2.1.3-91ac9cb (broken)

Solution

It seems as though {{apple := GetAppleByName("honeycrisp")}} is returning a *main.Apple but the new code is dereferencing that pointer which is unexpected. I can see where this might be handy for a *string or *int but for a *struct{} I can't think why this would be the desired behavior.

[annismckenzie]

Oh dear, we'll have to get that fixed. I thought we had tests for these and we should add some so this doesn't happen again.

[sauerbraten]

Was there a bug or use case that required #96? Why was that done? It reads to me that reverting that change would fix this issue, but means you can't simply print a *int into a template, correct?

[tooolbox]

Oh dear, we'll have to get that fixed. I thought we had tests for these and we should add some so this doesn't happen again.

Sounds good! 😄

It reads to me that reverting that change would fix this issue, but means you can't simply print a *int into a template, correct?

I think you're right; that seems like the use case, but we should revert and add a test case, and then perhaps @sarge can propose an alternative fix.

[sauerbraten]

Yes, I feel like dereferencing pointers should only happe automatically when evaluating pointers to built-in types, never structs.

On Wed, 8 Jan 2020, 19:35 Matt Mc, notifications@github.com wrote:

Oh dear, we'll have to get that fixed. I thought we had tests for these and we should add some so this doesn't happen again.

Sounds good! 😄

It reads to me that reverting that change would fix this issue, but means you can't simply print a *int into a template, correct?

I think you're right; that seems like the use case, but we should revert and add a test case, and then perhaps @sarge https://github.com/sarge can propose an alternative fix.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CloudyKit/jet/issues/123?email_source=notifications&email_token=AANN5DHMYVT5RCZXA25MVHDQ4YMGPA5CNFSM4KECG73KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINRA5Q#issuecomment-572199030, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANN5DAPGLOGEJW7OTYZWXLQ4YMGPANCNFSM4KECG73A .

[sarge]

Thanks for the heads up @tooolbox super bug report:)

I have moved on from the project that needed this, but from memory @sauerbraten, from the template users perspective, printing the pointer never seems to be useful.

It would be great to get your opinions on this https://github.com/CloudyKit/jet/pull/125

[tooolbox]

Awesome @sarge thanks for looking into this and the great PR, with a test!

From my end, I can confirm that your fix works in v2.1.3-pointers-sarge on my sample project.

Yes, I feel like dereferencing pointers should only happen automatically when evaluating pointers to built-in types, never structs.

Yeah, makes sense to me @sauerbraten. The new PR has done that so I think we have the best of both worlds here.

[sauerbraten]

I like #125, except for https://github.com/CloudyKit/jet/pull/125#discussion_r364649786 :) It's a good fix for this issue besides that!