Velocity
  1. Velocity
  2. VELOCITY-285

reference within macro and foreach is incorrect

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other

      Description

      It seems that there is a bug when we have loop into a recursive macro.

      Here is the test I run :

      #macro (test_loop $p)
      call to test_loop ($p)
      #foreach($child in $p)
      in the loop the param should not be changed : ($p)
      #test_loop($child)
      #end
      return
      #end

      #set($l1=["a"])
      #set($l = [$l1])
      #test_loop($l)

      It produce:

      call to test_loop ([[a]])
      in the loop the param should not be changed : ([[a]])
      call to test_loop ([a])
      in the loop the param should not be changed : (a)
      call to test_loop (a)
      return
      return
      return

      IMHO, it should be
      call to test_loop ([[a]])
      in the loop the param should not be changed : ([[a]])
      call to test_loop ([a])
      in the loop the param should not be changed : ([a])
      call to test_loop (a)
      return
      return
      return

      The difference is in the second recusive call. I don't know why, but it seems
      that the instruction #foreach($child in $p) has modified $p that contains the
      value of $child.

        Issue Links

          Activity

          Gilles Scokart created issue -
          Hide
          Gilles Scokart added a comment -

          I have found a workaround : use an intermediate variable :

          #macro (test_loop $p)
          call to test_loop ($p)
          #set( $t=$p)
          #foreach($child in $p)
          in the loop the param should not be changed : ($t)
          #test_loop($child)
          #end
          return
          #end
          #set($l1=["a"])
          #set($l = [$l1])
          #test_loop($l)

          Show
          Gilles Scokart added a comment - I have found a workaround : use an intermediate variable : #macro (test_loop $p) call to test_loop ($p) #set( $t=$p) #foreach($child in $p) in the loop the param should not be changed : ($t) #test_loop($child) #end return #end #set($l1= ["a"] ) #set($l = [$l1] ) #test_loop($l)
          Jeff Turner made changes -
          Field Original Value New Value
          issue.field.bugzillaimportkey 30343 12315155
          Hide
          Will Glass-Husain added a comment -

          Very interesting. The problem appears to be that outside of the #foreach, $p refers to the local $p. Inside of #foreach, $p refers to a global $p.

          This example is a little more instructive:

          #macro (test_loop $p)
          call to test_loop ($p)
          #foreach($child in $p)
          in the loop the param should not be changed : ($p) / ($child)
          #test_loop($child)
          #end
          return
          #end

          #set($l1=["a"])
          #set($l = [$l1])
          #test_loop($l)

          producing:

          call to test_loop ([[a]])
          in the loop the param should not be changed : ([[a]]) / ([a])
          call to test_loop ([a])
          in the loop the param should not be changed : (a) / (a)
          call to test_loop (a)
          return
          return
          return

          Show
          Will Glass-Husain added a comment - Very interesting. The problem appears to be that outside of the #foreach, $p refers to the local $p. Inside of #foreach, $p refers to a global $p. This example is a little more instructive: #macro (test_loop $p) call to test_loop ($p) #foreach($child in $p) in the loop the param should not be changed : ($p) / ($child) #test_loop($child) #end return #end #set($l1= ["a"] ) #set($l = [$l1] ) #test_loop($l) producing: call to test_loop ([ [a] ]) in the loop the param should not be changed : ([ [a] ]) / ( [a] ) call to test_loop ( [a] ) in the loop the param should not be changed : (a) / (a) call to test_loop (a) return return return
          Will Glass-Husain made changes -
          Summary recursion and loop reference within macro and foreach is incorrect
          Bugzilla Id 30343
          Assignee Velocity-Dev List [ velocity-dev@jakarta.apache.org ]
          Fix Version/s 1.5 [ 12310253 ]
          Priority Major [ 3 ] Minor [ 4 ]
          Environment Operating System: other
          Platform: Other
          Operating System: other
          Platform: Other
          Description It seems that there is a bug when we have loop into a recursive macro.

          Here is the test I run :

          #macro (test_loop $p)
          call to test_loop ($p)
          #foreach($child in $p)
              in the loop the param should not be changed : ($p)
          #test_loop($child)
          #end
          return
          #end

          #set($l1=["a"])
          #set($l = [$l1])
          #test_loop($l)


          It produce:

          call to test_loop ([[a]])
          in the loop the param should not be changed : ([[a]])
          call to test_loop ([a])
          in the loop the param should not be changed : (a)
          call to test_loop (a)
          return
          return
          return

          IMHO, it should be
          call to test_loop ([[a]])
          in the loop the param should not be changed : ([[a]])
          call to test_loop ([a])
          in the loop the param should not be changed : ([a])
          call to test_loop (a)
          return
          return
          return

          The difference is in the second recusive call. I don't know why, but it seems
          that the instruction #foreach($child in $p) has modified $p that contains the
          value of $child.
          It seems that there is a bug when we have loop into a recursive macro.

          Here is the test I run :

          #macro (test_loop $p)
          call to test_loop ($p)
          #foreach($child in $p)
              in the loop the param should not be changed : ($p)
          #test_loop($child)
          #end
          return
          #end

          #set($l1=["a"])
          #set($l = [$l1])
          #test_loop($l)


          It produce:

          call to test_loop ([[a]])
          in the loop the param should not be changed : ([[a]])
          call to test_loop ([a])
          in the loop the param should not be changed : (a)
          call to test_loop (a)
          return
          return
          return

          IMHO, it should be
          call to test_loop ([[a]])
          in the loop the param should not be changed : ([[a]])
          call to test_loop ([a])
          in the loop the param should not be changed : ([a])
          call to test_loop (a)
          return
          return
          return

          The difference is in the second recusive call. I don't know why, but it seems
          that the instruction #foreach($child in $p) has modified $p that contains the
          value of $child.
          Henning Schmiedehausen made changes -
          Bugzilla Id 30343
          Component/s Engine [ 12311337 ]
          Component/s Source [ 12310214 ]
          Henning Schmiedehausen made changes -
          Assignee Henning Schmiedehausen [ henning ]
          Henning Schmiedehausen made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Henning Schmiedehausen added a comment -

          Can clearly reproduce it...

          Show
          Henning Schmiedehausen added a comment - Can clearly reproduce it...
          Hide
          Henning Schmiedehausen added a comment -

          Ok, I know what that is. It is the loop variable. Sounds crazy, but here it goes:

          If you set velocimacro.context.localscope = true then it works.

          The problem is, that the loop variable (and also the counter variable) are always considered "local". However if v.c.l = false, then the put instruction in runtime.directive.Foreach, line 415 and 417 put the value right through the context into the global context (see VMContext#put), treating it as global.

          Show
          Henning Schmiedehausen added a comment - Ok, I know what that is. It is the loop variable. Sounds crazy, but here it goes: If you set velocimacro.context.localscope = true then it works. The problem is, that the loop variable (and also the counter variable) are always considered "local". However if v.c.l = false, then the put instruction in runtime.directive.Foreach, line 415 and 417 put the value right through the context into the global context (see VMContext#put), treating it as global.
          Hide
          Henning Schmiedehausen added a comment -

          Hm, that actually is a scary problem. I wonder why loops nested like this ever worked at all. Anyway, I did a minor change to the InternalWrapperContext that allows foreach to force its loop variable to be a part of the local (VM) context. That fixes this problem. Thanks a lot for reporting, sorry that it took us so long to fix it.

          Show
          Henning Schmiedehausen added a comment - Hm, that actually is a scary problem. I wonder why loops nested like this ever worked at all. Anyway, I did a minor change to the InternalWrapperContext that allows foreach to force its loop variable to be a part of the local (VM) context. That fixes this problem. Thanks a lot for reporting, sorry that it took us so long to fix it.
          Henning Schmiedehausen made changes -
          Resolution Fixed [ 1 ]
          Status In Progress [ 3 ] Resolved [ 5 ]
          Hide
          Will Glass-Husain added a comment -

          nice catch and nice fix!

          sometimes it takes an alert user to find a subtle use case that exposes a more general underlying problem.

          Show
          Will Glass-Husain added a comment - nice catch and nice fix! sometimes it takes an alert user to find a subtle use case that exposes a more general underlying problem.
          Hide
          Henning Schmiedehausen added a comment -

          Close all resolved issues for Engine 1.5 release.

          Show
          Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.
          Henning Schmiedehausen made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Will Glass-Husain made changes -
          Link This issue relates to VELOCITY-532 [ VELOCITY-532 ]
          Byron Foster made changes -
          Link This issue is blocked by VELOCITY-630 [ VELOCITY-630 ]
          Mark Thomas made changes -
          Workflow jira [ 12325160 ] Default workflow, editable Closed status [ 12551805 ]
          Mark Thomas made changes -
          Workflow Default workflow, editable Closed status [ 12551805 ] jira [ 12552295 ]

            People

            • Assignee:
              Henning Schmiedehausen
              Reporter:
              Gilles Scokart
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development