Regression: Ranging over slice from Fast Function

[tooolbox]

I've observed a regression caused by 6d666f9 (#112)

Summary

For this template:

{{sorted := SortApples(myApples)}}
{{range i, apple := sorted}}
	{{i}}: {{apple.GetFlavor()}}
{{end}}

(Where SortApples is a fast function aka jet.Func.)

Jet was able to range over the returned slice up until 6d666f9 at which point the template execution would error with: there is no field or method "GetFlavor" in main.Apple

Example Project

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

• v2.1.2 (working)
• v2.1.3-6d666f94dfe7 (broken)

---

I understand we're now going for a v3 with these commits, so breaking changes might be expected, but this seems like it would hurt fast functions a lot.

cc @sauerbraten

[sauerbraten]

From the example code and the error message, it's clear that the problem is that SortApples returns a Bushel which is a []Apple, meaning apple will be of type Apple inside the loop, which in fact doesn't have the GetFlavor method defined.

Both changing Bushel to mean []*Apple or changing GetFlavor to be defined on (a Apple) fixes the example code.

Now as to why this used to work and doesn't work anymore: I can't really tell. The commit that breaks the example changed the slice ranger implementation to pull the actual value out of the reflect.Value and returns it as interface{}, but the value is rewrapped as a reflect.Value before setting it in the scope so that resolving it in the apple.GetFlavor() call should work just as before.

Here's a small example of that: https://play.golang.org/p/Y0YvB19a0aH

🤔

[sauerbraten]

If I understand jet's lexing and parsing correctly, {{apple.GetFlavor()}} should be parsed as a call expression with a chain expression as base, which in turn has apple as node and GetFlavor as field. apple should then be evaluated as an identifier and resolved from the current runtime scope, correct?

jet/eval.go

Line 1089 in 17c3587

resolved := st.Resolve(node.(*IdentifierNode).Ident)

[tooolbox]

From the example code and the error message, it's clear that the problem is that SortApples returns a Bushel which is a []Apple, meaning apple will be of type Apple inside the loop, which in fact doesn't have the GetFlavor method defined.

Both changing Bushel to mean []*Apple or changing GetFlavor to be defined on (a Apple) fixes the example code.

Yes, that's correct. In a way it is more strict and "correct", but on the other hand Go's normal method semantics allow you to call pointer methods on structs and they magically work, so it seems intuitive that Jet would let you do that.

[tooolbox]

(This comment has been redacted because I made a mistake and referenced the wrong branch, so it's inapplicable 😄 )

[tooolbox]

Okay, I believe I get what's happening now.

Previously, sliceRanger.Range() would return a reflect.Value which contained a main.Apple. When Jet called getValue("GetFlavor", apple) it would call Addr() to get a *main.Apple and then call the method.

Now, sliceRanger.Range() returns an interface{} and concrete values contained in interfaces are not addressable. See https://stackoverflow.com/q/48790663/700471

Thinking about it, this is most definitely desired behavior in terms of the newer ranging style, because when calling .Interface() you are copying the element of the slice and embedding it in the interface. Attempting to then call a pointer-receiver-method (which could modify the receiver) on the concrete value within the interface would fail to work as intended.

Consider if the template looked like this:

{{sorted := SortApples(myApples)}}
{{range i, apple := sorted}}
        {{apple.SetFlavor("ripe")}}
{{end}}
{{range i, apple := sorted}}
	{{i}}: {{apple.GetFlavor()}}
{{end}}

You might expect all apples to have a "ripe" flavor but they wouldn't because you're modifying the copy in the first loop.

Anyway, I'm fairly certain this is what's happening.

---

@sauerbraten I see in #112 you said:

This meant you could not compare e.g. elements of a string slice to a string literal. Also, storing a copy of a certain slice element's index and using it outside of the range did not work, since the copy only pointed to the actual index (which had the value len+1 after the range).

I get the concept, but do you have some code/template examples of where you ran into this previously?

[sauerbraten]

What you say about Addr() being called sounds right, but I can't really see the code path there... in getValue(),

jet/eval.go

Line 1488 in 17c3587

value := v.MethodByName(key)

should already return the method, regardless of wether v is an Apple or *Apple wrapped in an interface, as you can see from the playground example in #122 (comment).

Can you see why that wouldn't happen and instead the if v.CanAddr() path below that would be used?

[sauerbraten]

Two (for us common) use cases regarding my comment in #112 are: checking if a certain string literal is contained in a []string, or finding the index of a certain element of a slice to access it later using an index expression.

[tooolbox]

Can you see why that wouldn't happen and instead the if v.CanAddr() path below that would be used?

I guess it's because we're dealing with an element of a slice, and the method has a pointer receiver.

Check this out: https://play.golang.org/p/suysPI_L6-t

It's the most accurate representation of what we're dealing with. You can see that I can't locate the method until I call Addr() on the slice element, and then once I've put it into an interface{} and re-wrapped it, I can't use Addr() again.

Does that make sense?

[tooolbox]

Two (for us common) use cases regarding my comment in #112 are: checking if a certain string literal is contained in a []string, or finding the index of a certain element of a slice to access it later using an index expression.

Would you be able to show a sample for each of these scenarios?

Like, for example, is this what you mean by the string comparison:

{{range i, n := names}}
  {{if n == "Joe"}}  <!-- This fails? Really? -->
    Hello Joe!
  {{else}}
    Hello person!
  {{end}}
{{end}}

On the index:

{{index := -1}}
{{range i, n := names}}
  {{if CompareString(n, "Joe")}}
    {{index = i}}
  {{end}}
{{end}}

If that's what you mean, I see how that could fail, because it was a pointer to the counter in the sliceRanger:

jet/eval.go

Line 1573 in 2b06453

index = reflect.ValueOf(&s.i).Elem()

Elem() is supposed to get the value within a pointer but I think something is misunderstood there. This playground shows an easy fix, where you could still return a reflect.Value but without it changing later on: https://play.golang.org/p/BTn99ler_l5

[sauerbraten]

Sorry, I meant checking if a string key was present in a map. The values returned by Range() were of the correct type, but when iterating a map[string]interface{}, the key/index type would be *string (because of that line you identified). I guess your way of fixing it would have been enough, yes.

[sauerbraten]

I guess it's because we're dealing with an element of a slice, and the method has a pointer receiver.
Check this out: https://play.golang.org/p/suysPI_L6-t

Ahh, I see: MethodByName automatically dereferences pointer receivers, but doesn't create references of value receivers to call methods defined on pointers to that value's type (which is consistent with Go, btw: https://golang.org/ref/spec#Method_expressions): https://play.golang.org/p/El6o5AMQfuQ

[sauerbraten]

I'm not sure jet should deviate from the Go spec by using reflection. I'd rather it be as strict about the types you work on as Go is.

[tooolbox]

Ahh, I see: MethodByName automatically dereferences pointer receivers, but doesn't create references of value receivers to call methods defined on pointers to that value's type

Yeah, precisely :)

I'm not sure jet should deviate from the Go spec by using reflection. I'd rather it be as strict about the types you work on as Go is.

Yeah, I do get you, but I think since it's the longstanding behavior of getValue() (checking CanAddr() and so on) it's better to keep as-is unless we have strong incentives to change.

Sorry, I meant checking if a string key was present in a map. The values returned by Range() were of the correct type, but when iterating a map[string]interface{}, the key/index type would be *string (because of that line you identified). I guess your way of fixing it would have been enough, yes.

Interesting, okay. Question on that, because I think a mapRanger is a different type, right? And if we look at the code before your original PR (and now reverting back to that with your new PR) we see:

jet/eval.go

Lines 1620 to 1630 in 62edd43

	func (s *mapRanger) Range() (index, value reflect.Value, end bool) {
	if s.i < s.len {
	index = s.keys[s.i]
	value = s.v.MapIndex(index)
	s.i++
	return
	}
	end = true
	pool_mapRanger.Put(s)
	return
	}

I don't actually see how it's returning a *string there...unless reflect.Value.MapKeys() works that way, which it may. I think a playground is in order here. Also if you have an exact template example that was failing, that would be swell.

(I love your new PR and don't want to hold it up, just curious about what you ran into & want to make sure you'll be all good.)

[tooolbox]

Hm, yeah I tried out the map keys: https://play.golang.org/p/JWi9QluRV4a

Seems fine? Well, I might not have it nailed; if you can throw out an example scenario, we can scope it more.

[sauerbraten]

Yeah, everything you say makes sense. I'm not sure anymore what our issue was 🤔

I know that a colleague had a problem with comparisons inside a range not working properly before we did #112. I'm not sure anymore why exactly it happened, sadly.

Since we're changing it back anyway, I don't think it's super important to recreate it.