Bug 42046

Created attachment 24782 [details]
macro double property expansion fix

I think my patch should fix this issue.
It prevents the replacement of properties in MacroInstances,
which is the unnecessary second (actually first) expansion.

As I said the bug should be fixed by applying the patch I attached.
I've tested it with the given example here and also with the one from
bug #41400 and they work just fine.

Alright. I fixed the bug in a source distribution.
Meanwhile the code in RuntimeConfigurable.java has changed in the trunk, though.

And guess what, this specific bug is no more.
However, bug #41400 still remains, but is still fixed by my updated
patch, which I will attach there.

I can still reproduce the bug with a trunk build of Ant (without your patch, that is).

*** This bug has been marked as a duplicate of bug 41400 ***

*** Bug 41400 has been marked as a duplicate of this bug. ***

I'm again trying to get this resolved.  With your patch now not only propertyhelper-test but also property-test fails.

Interestingly the property-test failure is a good one as the buggy behavior of assertPropertyEquals of bug 41400 hides the actual failure.  See the commented out alternative way of verifying the property value in svn revision 1203226

In the case of properthelper-test I've tracked it down to the point where <equals> says the objects are different.  In fact, <equals> doesn't see any objects but rather the string ${object}.  This ${object} is supposed to be resolved into an Object instance by a custom property helper - and it is inside the error message, but not when passed to <equals>.

So whatever it is that performs the second expansion on macroinstance attributes doesn't seem to consult all propertyhelpers, while the first invocation that your patch removes does.

This is more thinking out loud as I will try to investigate this case further myself, but maybe anybody else sees where we are going wrong before I do.

The resons for the propertyhelper-test failure turned out to be more subtle.  after applying your patch the test was comparing the o.toString() for arg2 to o and of course they were no the same.

I've modified the test to use a property who's value is a string and it passes.

What is happening here is that the loadproperties task doesn't work as expected b the test when expanding properties with non-string values.  I.e. this is another case where the double-expansion of properties cause a false positive unit test.

svn revision 1204961 contains the patch and disabled the failing tests that I'm going to address separately.

Thanks for your patience.

Due to regressions like that described in bug #52621, probably this fix needs to be opt-in, since I can imagine various other scenarios where the former behavior (call by value) is wanted and a change (call by name) would be incompatible.

Suggest introducing an attribute on macrodef like 'expands' which would be true by default; those users who have struggled with double expansion in the past can explicitly set it to false, but others would be unaffected:

  <macrodef name="echotest" expands="false">
    <attribute name="message"/>
    <sequential>
      <echo message="@{message}"/>
    </sequential>
  </macrodef>

RuntimeConfigurable.java would then read:

if (target instanceof MacroInstance &&
    !((MacroInstance) target).getMacroDef().isExpands()) {
  attrValue = value;
} else {
  attrValue = PropertyHelper.getPropertyHelper(p).parseProperties(value);
}

With a slightly more complex patch, the <attribute> nested element could individually have an expansion attribute:

  <macrodef name="echotest">
    <attribute name="message" expands="false"/>
    <sequential>
      <echo message="@{message}"/>
    </sequential>
  </macrodef>

(Not sure if the same consideration applies to <text> and/or <element>.)

The current patch in trunk only applies to the attributes of the macro invocation, nested text or nested elements are not directly affected.  For nested text this doesn't pose any problem as Ant's engine doesn't expand properties there, the task has to do it, so there is no double-expansion.  Nested elements aren't handled in RuntimeConfigurable directly but rather when the nested sequential of the MacroInstance is configured.

What happened before the patch is Ant expanded properties of the attributes of each echotest instance before calling execute on the MacroDef instance backing it, then the MacroInstance created a new UnknownElement for the sequential defined inside the macrodef, replaced @{...} sequences while creating it and the handed it off to Ant's engine to configure - which led to another round of property expansions.

So properties in attributes are expanded in trunk as well, it is only that they are only expanded once rather than twice (which is needed for the macrodef from http://ant.apache.org/faq.html#propertyvalue-as-name-for-property to work).

It boils down to "is ${} expansion performed before and after @{} expansion or only after @{} expansion".  I agree the default has to be "before and after" to avoid regressions.

With an attribute like Jesse suggests we end with a subtle difference that not only is hard to explain for the macrodef manual but also requires macrodef writers to document for their users (who don't necessarily know they are using a macrodef in the first place).  Something like "properties used in the message attribute might get expanded twice so you must use three $ characters rather than two to avoid expanding a property reference".  The same is true for any macrodef used with Ant 1.8.2 and earlier anyway.

I can't say I'm satisfied with the proposed solution but don't see any alternative.  The only other option I see is not fixing this bug but calling it a feature and document it properly (which still leaves macrodef writers to explain the behavior of their tasks to their users).

I'm going to add a FAQ entry documenting the current behavior in the first place.

(In reply to comment #8)
> [a parameter] on macrodef like 'expands' which would be true by default

If discoverability of the #42046 fix is a concern, the taskdef could warn if it were not set one way or the other, as we do for <javac> without source="...". This would address the discoverability issue but at the cost of annoying users with functional scripts, especially if they wish to continue to run on 1.8.2 as well and so cannot define the parameter.

Fix now activated conditionally as discussed in bug #52621.