Velocity
  1. Velocity
  2. VELOCITY-684

Passing a map literal to a macro call forbids altering the map in any way, while maps bound to an actual parameter may be changed

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.6.1
    • Fix Version/s: None
    • Component/s: Engine
    • Labels:
      None

      Description

      For example, with the following macro:

      #macro(changeMap $map)
      Before: $map.someKey
      #set($map.someKey = 'new value')
      After: $map.someKey
      #end

      This call works as expected:

      #set($actualMap =

      {'someKey' : 'old value'}

      )
      #changeMap($actualMap) => old value, then new value

      But this one doesn't:

      #changeMap(

      {'someKey' : 'old value'}

      ) => old value, then again old value

        Activity

        Hide
        Byron Foster added a comment -

        This is correct pass by name behavior, (Agreed this is confusing)

        given your above #changeMap macro definition, then with the following call:

        #changeMap(

        {'someKey' : 'old value'}

        )

        Every $map reference essentially is substituted with

        {'someKey': 'old value'}

        i.e., every reference to $map creates a new Map.

        Show
        Byron Foster added a comment - This is correct pass by name behavior, (Agreed this is confusing) given your above #changeMap macro definition, then with the following call: #changeMap( {'someKey' : 'old value'} ) Every $map reference essentially is substituted with {'someKey': 'old value'} i.e., every reference to $map creates a new Map.
        Hide
        Nathan Bubna added a comment -

        Ugh. Again, even if this is correct pass by name behavior, Velocity was designed to be pass by name in order to behave like this. I would prefer to say that our design goal for macro arguments is not "correct pass by name behavior" but rather something like "a modified, unsurprising form of pass by name behavior". So, when a map literal (or other literal) is passed in, we should probably just assign it to that reference in the local context so that it can be modified as one would expect. So, technical correctness be damned, i want Velocity to be easy to read and write. Surprises like this are bugs, IMHO.

        Show
        Nathan Bubna added a comment - Ugh. Again, even if this is correct pass by name behavior, Velocity was designed to be pass by name in order to behave like this. I would prefer to say that our design goal for macro arguments is not "correct pass by name behavior" but rather something like "a modified, unsurprising form of pass by name behavior". So, when a map literal (or other literal) is passed in, we should probably just assign it to that reference in the local context so that it can be modified as one would expect. So, technical correctness be damned, i want Velocity to be easy to read and write. Surprises like this are bugs, IMHO.
        Hide
        Jarkko Viinamäki added a comment -

        Ahhum... What's the real life use case for this?

        Show
        Jarkko Viinamäki added a comment - Ahhum... What's the real life use case for this?
        Hide
        Byron Foster added a comment -

        I don't mean to sound technically rigid, and I would be for any simple shortcut. An obvious solution is to make ASTMaps constant, but it's not so simple. for example I can have this:

        #macro(foo $map)
        #set($b = "c")
        #set($map.x = "z")
        $map.a $map.x
        #end

        #foo(

        {'a' : $b, 'x' : 'y'}

        )

        A strait forward constant implementation gives: "$map.a z" (in strict mode you get an exception)
        As it stands you get: "c y"

        I'm not sure what the solution is in this context. Also, this same issues applies to ASTObjectArray, ASTIntegerRange, etc...

        Show
        Byron Foster added a comment - I don't mean to sound technically rigid, and I would be for any simple shortcut. An obvious solution is to make ASTMaps constant, but it's not so simple. for example I can have this: #macro(foo $map) #set($b = "c") #set($map.x = "z") $map.a $map.x #end #foo( {'a' : $b, 'x' : 'y'} ) A strait forward constant implementation gives: "$map.a z" (in strict mode you get an exception) As it stands you get: "c y" I'm not sure what the solution is in this context. Also, this same issues applies to ASTObjectArray, ASTIntegerRange, etc...
        Hide
        Nathan Bubna added a comment -

        Yuck. You're right, it's more complicated than i thought. Considering your example, it seems that when a literal of these types is passed in as an argument, we probably should keep it as pass by name until a #set happens that involves that argument reference. Then, we can assume that the user is no longer wanting to treat it as pass-by-name, so we get the value of it and assign it directly in the context (local or global) at that point. Not sure how easy that would be to implement, but i think that would be the simplest, least surprising thing to do for users.

        As for real life use cases, i don't have one myself, but it seems likely that Sergiu does, since he is a user and appears to have expected the behavior i just described, even despite his strict pass-by-name advocacy in VELOCITY-681.

        Show
        Nathan Bubna added a comment - Yuck. You're right, it's more complicated than i thought. Considering your example, it seems that when a literal of these types is passed in as an argument, we probably should keep it as pass by name until a #set happens that involves that argument reference. Then, we can assume that the user is no longer wanting to treat it as pass-by-name, so we get the value of it and assign it directly in the context (local or global) at that point. Not sure how easy that would be to implement, but i think that would be the simplest, least surprising thing to do for users. As for real life use cases, i don't have one myself, but it seems likely that Sergiu does, since he is a user and appears to have expected the behavior i just described, even despite his strict pass-by-name advocacy in VELOCITY-681 .
        Hide
        Sergiu Dumitriu added a comment -

        No, there's no real usecase, I simply found this writing tests for VELOCITY-681 (you can see them commented in test/templates/velocimacro.vm). I just find it surprising and counter-intuitive for a user.

        Show
        Sergiu Dumitriu added a comment - No, there's no real usecase, I simply found this writing tests for VELOCITY-681 (you can see them commented in test/templates/velocimacro.vm). I just find it surprising and counter-intuitive for a user.
        Show
        Sergiu Dumitriu added a comment - See http://wiki.apache.org/velocity/MacroEvaluationStrategy

          People

          • Assignee:
            Unassigned
            Reporter:
            Sergiu Dumitriu
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development