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

          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)
          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
          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.
          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.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development