context not passed when `partial` used within `for` loop

[sio4]

When I tried to migrate my old app into the current version of buffalo, I found the following problem.

I guess context not passed to partial when it called within for loop.

Description

When I tried to use partial() within for loop, it cannot find the parent's context variables. It did just work fine with 0.11.0 (as I remember) but now it does not work.

Steps to Reproduce the Problem

parent template:

    <%= for (user) in users { %>
      <%= partial("users/row") %>
    <% } %>

a partial template named _row.html

<%= user %>

Error message:

users/index.html: line 25: users/_row.html: line 2: user: unknown identifier

Additionally,

• If I use <% let user = "something" %>, it passed well to partial but if I use let inside of the loop, it is also not working.
• If I use form_for inside of loop, f also not passed into _form partial.

Expected Behavior

Render content with the parent's value.

Actual Behavior

make error: unknown identifier

Info

``` ### Buffalo Version development ### App Information Pwd=/home/sio4/gowork/src/github.com/hyeoncheon/skel Root=/home/sio4/gowork/src/github.com/hyeoncheon/skel GoPath=/home/sio4/gowork Name=skel Bin=bin/skel PackagePkg=github.com/hyeoncheon/skel ActionsPkg=github.com/hyeoncheon/skel/actions ModelsPkg=github.com/hyeoncheon/skel/models GriftsPkg=github.com/hyeoncheon/skel/grifts VCS=git WithPop=true WithSQLite=false WithDep=false WithWebpack=true WithYarn=true WithDocker=true WithGrifts=true WithModules=false ### Go Version go version go1.11.1 linux/amd64 ### Go Env GOARCH="amd64" GOBIN="" GOCACHE="/home/sio4/.cache/go-build" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOOS="linux" GOPATH="/home/sio4/gowork" GOPROXY="" GORACE="" GOROOT="/opt/google/go" GOTMPDIR="" GOTOOLDIR="/opt/google/go/pkg/tool/linux_amd64" GCCGO="gccgo" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build189751616=/tmp/go-build -gno-record-gcc-switches" ### Node Version v8.12.0 ### NPM Version 6.4.1 ### Yarn Version 1.10.1 ### PostgreSQL Version PostgreSQL Not Found ### MySQL Version mysql Ver 14.14 Distrib 5.7.24, for Linux (x86_64) using EditLine wrapper ### SQLite Version 3.22.0 2018-01-22 18:45:57 0c55d179733b46d8d0ba4d88e01a25e10677046ee3da1d5b1581e86726f2alt1 ### Dep Version could not find a Gopkg.toml file ### Dep Status could not find a Gopkg.toml file ### go.mod module github.com/hyeoncheon/skel ```

[sio4]

Hi @markbates, Thanks for a quick checking for this issue. but sadly, this patch does not solve the problem. I tried to find the point of the problem and I found the following point:

The diff below is not for buffalo but for plush since this issue problem is crossed across these two. Please read the comments on the patch first.

--- a/compiler.go
+++ b/compiler.go
@@ -619,6 +619,13 @@ func (c *compiler) evalCallExpression(node *ast.CallExpression) (interface{}, er
                }
        }

+       // 3. finally it calls `partial()` but it runs on another context.
+       //    can we pass `data` of current context as args?
+       if node.Function.String() == "partial" {
+               fmt.Println("XXX quick and dirty work around!")
+               args = args[0:1]
+               args = append(args, reflect.ValueOf(c.ctx.data))
+       }
        res := rv.Call(args)
        if len(res) > 0 {
                if e, ok := res[len(res)-1].Interface().(error); ok {
@@ -634,6 +641,8 @@ func (c *compiler) evalForExpression(node *ast.ForExpression) (interface{}, erro
        defer func() {
                c.ctx = octx
        }()
+       // 1. create new context for `for` block here.
+       //    anyway this context does not have values form original.
        c.ctx = octx.New()
        iter, err := c.evalExpression(node.Iterable)
        if err != nil {
@@ -661,8 +670,9 @@ func (c *compiler) evalForExpression(node *ast.ForExpression) (interface{}, erro
        case reflect.Slice, reflect.Array:
                for i := 0; i < riter.Len(); i++ {
                        v := riter.Index(i)
-                       c.ctx.Set(node.KeyName, i)
+                       c.ctx.Set(node.KeyName, i) // misc: is it required?
                        c.ctx.Set(node.ValueName, v.Interface())
+                       // 2. it calls evalBlockStatement after set new values
                        res, err := c.evalBlockStatement(node.Block)
                        if err != nil {
                                return nil, errors.WithStack(err)

The starting point of the problem is, as marked as 1, the loop has its own context for isolated loop and at the point of 2, it sets block values on its own context. After that, it calls evalBlockStatement() from this newly generated and filled context.

In mark 3, it calls wrapper function for partial() but the function is not defined in this context. so it just uses parent's data. As a quick and dirty fix for testing, I just re-generate the second argument for partial() with compiler.ctx.data and now it can handle values of loop.

Anyway, I think this will work for the partial but very tricky since there is no reason why plush should know about the present of partial(). Even though it is a family package of buffalo which also has BuffaloRenderer(). I think we need any different approach for this issue, for example start new renderer for loop inside...

Strange thing is, my old app has this "partial within a loop" structure but it works with buffalo 0.11.1.

Is there any good solution?

[markbates]

This patch fixed the code you gave me to reproduce the problem. What error are you getting with this fix?

[sio4]

Oh really? In my env., with buffalo development pulled some hours ago and plush master, the example of partial inside of for not works. (same error)

Then I will make test app for share tomorrow.

[sio4]

Hi @markbates, I added and PRed test cases for this issue as #1411. I think these two test cases can be use for clearfing the issue. Can you confirm the test cases are written well? For now, those are failed with current development branch of buffalo.

I also tried to improve quick patch for plush as following. It does'n affect others but just modify second argument for partial(). Anyway I don't think it is beautiful. :-)

Please check the test cases and patch, and consider some right direction for the improvement.

--- a/compiler.go
+++ b/compiler.go
@@ -504,6 +504,21 @@ func (c *compiler) evalCallExpression(node *ast.CallExpression) (interface{}, er
                                return nil, errors.WithStack(err)
                        }

+                       // 3. if the function is `partial` and user provides second argument,
+                       //    merge it with current context's data which has loop var.
+                       if node.Function.String() == "partial" && pos == 1 {
+                               fmt.Printf("XXX - %T, %v\n", v, v)
+                               data := map[string]interface{}{}
+                               for k, vv := range c.ctx.data {
+                                       data[k] = vv
+                               }
+                               for k, vv := range v.(map[string]interface{}) {
+                                       data[k] = vv
+                               }
+                               v = data
+                               fmt.Printf("XXX - %T, %v\n", v, v)
+                       }
+
                        var ar reflect.Value
                        expectedT := rt.In(pos)
                        if v != nil {
@@ -520,6 +535,12 @@ func (c *compiler) evalCallExpression(node *ast.CallExpression) (interface{}, er
                        args = append(args, ar)
                }

+               // 3.1. if the function is `partial` but there is no second argument,
+               //      just use current context's data as second argument.
+               if node.Function.String() == "partial" && len(node.Arguments) == 1 {
+                       args = append(args, reflect.ValueOf(c.ctx.data))
+               }
+
                hc := func(arg reflect.Type) {
                        if arg.ConvertibleTo(reflect.TypeOf(HelperContext{})) {
                                hargs := HelperContext{
@@ -619,6 +640,7 @@ func (c *compiler) evalCallExpression(node *ast.CallExpression) (interface{}, er
                }
        }

+       // 4. finally it calls `partial()` but it runs on another context.
        res := rv.Call(args)
        if len(res) > 0 {
                if e, ok := res[len(res)-1].Interface().(error); ok {
@@ -634,6 +656,8 @@ func (c *compiler) evalForExpression(node *ast.ForExpression) (interface{}, erro
        defer func() {
                c.ctx = octx
        }()
+       // 1. create new context for `for` block here.
+       //    anyway this context does not have values form original.
        c.ctx = octx.New()
        iter, err := c.evalExpression(node.Iterable)
        if err != nil {
@@ -661,8 +685,9 @@ func (c *compiler) evalForExpression(node *ast.ForExpression) (interface{}, erro
        case reflect.Slice, reflect.Array:
                for i := 0; i < riter.Len(); i++ {
                        v := riter.Index(i)
-                       c.ctx.Set(node.KeyName, i)
+                       c.ctx.Set(node.KeyName, i) // misc: is it required?
                        c.ctx.Set(node.ValueName, v.Interface())
+                       // 2. it calls evalBlockStatement after set new values
                        res, err := c.evalBlockStatement(node.Block)
                        if err != nil {
                                return nil, errors.WithStack(err)

[sio4]

Oh, maybe I found the beginning of the issue. Previously, plush exports its data as a missing argument of function if it meets argument type Data but it removed. (yes, this part is also tricky)

https://github.com/gobuffalo/plush/commit/eb827fe5817ba1e8ac75b6f9476f8a08373f697f#diff-fa2eeef2ce3d6d371a7601bb279e9060L515

I tried to start new Render(string, new_context) for the whole block of for loop but the original template already parsed and I cannot found the way to generate original template block from statements automatically. (Anyway, it will not help because of...)

Currently partial() uses templateRenderer, its holder's local variable data and it cannot access newly filled ctx of compiler. I think partial() should be moved to plush but I also understand it also somewhat bad approach since plush only handle string as input but partial need to access filesystem.

I think the patch above for plush is reasonable for now.

[sio4]

closed by #1411