This macro uses the mutli-line comment hack to gobble whitespace (so that none appears in the link text, whilst preserving some kind of readability.

This works fine in 1.4, but throws a ParseException in 1.5 (stacktrace below)

– stacktrace –

2007-04-03 16:05:38,712 - VelocimacroManager.parseTree() : exception makeLink
org.apache.velocity.runtime.parser.ParseException: Lexical error: org.apache.velocity.runtime.parser.TokenMgrError: Lexical error at line 1, column 535. Encountered: <EOF> after : ""
at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:124)
at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1042)
at org.apache.velocity.runtime.directive.VelocimacroProxy.parseTree(VelocimacroProxy.java:342)
at org.apache.velocity.runtime.directive.VelocimacroProxy.setupMacro(VelocimacroProxy.java:322)
at org.apache.velocity.runtime.directive.VelocimacroProxy.init(VelocimacroProxy.java:309)
at org.apache.velocity.runtime.parser.node.ASTDirective.init(ASTDirective.java:134)
at org.apache.velocity.runtime.parser.node.SimpleNode.init(SimpleNode.java:285)
at org.apache.velocity.Template.initDocument(Template.java:199)
at org.apache.velocity.Template.process(Template.java:121)
at org.apache.velocity.runtime.resource.ResourceManagerImpl.loadResource(ResourceManagerImpl.java:415)
at org.apache.velocity.runtime.resource.ResourceManagerImpl.getResource(ResourceManagerImpl.java:335)
at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1102)
at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1077)
at org.apache.velocity.app.VelocityEngine.getTemplate(VelocityEngine.java:528)
at org.apache.velocity.tools.view.servlet.VelocityViewServlet.getTemplate(VelocityViewServlet.java:667)
at org.apache.velocity.tools.view.servlet.VelocityViewServlet.handleRequest(VelocityViewServlet.java:601)
at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doRequest(VelocityViewServlet.java:541)
at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doGet(VelocityViewServlet.java:507)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:689)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:802)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:252)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:173)
at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:213)
at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:178)
at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:126)
at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:105)
at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:107)
at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:148)
at org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:869)
at org.apache.coyote.http11.Http11BaseProtocol$Http11ConnectionHandler.processConnection(Http11BaseProtocol.java:664)
at org.apache.tomcat.util.net.PoolTcpEndpoint.processSocket(PoolTcpEndpoint.java:527)
at org.apache.tomcat.util.net.LeaderFollowerWorkerThread.runIt(LeaderFollowerWorkerThread.java:80)
at org.apache.tomcat.util.threads.ThreadPool$ControlRunnable.run(ThreadPool.java:684)
at java.lang.Thread.run(Thread.java:595)

Activity

The exception does not occur until you try to use the macro, and it can be reproduced with a macro as simple as:

#macro( test537a )test#*
*##end

Change the macro to any of these:

#macro( test537b )test
#**##end

#macro( test537c )
test#**##end

#macro( test537d )#**#test#end

#macro( test537e )
#**#test#end

#macro( test537f )#*
*#test#end

And the error no longer occurs. However, #test537f() does not produce the expected output:

#test537f()

produces:

*#test

Which is baffling, to say the least!

Something's very rotten here. The worst part is that i could not find any simple change to Christopher's macro that would make it work as expected. Instead i got increasingly bizarre results. Did anyone who uses the common whitespace removal/management hacks test their code against 1.5? It's not looking that way to me...

Nathan Bubna
added a comment - 03/Apr/07 19:35 The exception does not occur until you try to use the macro, and it can be reproduced with a macro as simple as:
#macro( test537a )test#*
*##end
Change the macro to any of these:
#macro( test537b )test
#**##end
#macro( test537c )
test#**##end
#macro( test537d )#**#test#end
#macro( test537e )
#**#test#end
#macro( test537f )#*
*#test#end
And the error no longer occurs. However, #test537f() does not produce the expected output:
#test537f()
produces:
*#test
Which is baffling, to say the least!
Something's very rotten here. The worst part is that i could not find any simple change to Christopher's macro that would make it work as expected. Instead i got increasingly bizarre results. Did anyone who uses the common whitespace removal/management hacks test their code against 1.5? It's not looking that way to me...

At Atlassian we're looking to upgrade Confluence to use Velocity 1.5 and I've just run into this problem as well.

The funny thing is, if you add an argument to the macro definition it then parses fine. I'd love to dig into the Velocity parser but I'm not that familiar with the tools used to generate it. We might just have to work around it.

Christopher Owen
added a comment - 19/Dec/07 06:25 At Atlassian we're looking to upgrade Confluence to use Velocity 1.5 and I've just run into this problem as well.
The funny thing is, if you add an argument to the macro definition it then parses fine. I'd love to dig into the Velocity parser but I'm not that familiar with the tools used to generate it. We might just have to work around it.

Will Glass-Husain
added a comment - 19/Dec/07 07:56 I did a little work on this earlier in the year. Something tricky is going on.
Let me take another stab at this – I have a little downtime during the holidays. Kind of bizarre issue. The "adding an argument fixes the issue" is a good clue.

I found a solution to this problem (and the underlying issue as mentioned in issue VELOCITY-580:

The problem is when method getASTAsStringArray(Node rootNode) of class Macro (package org.apache.velocity.runtime.directive) is called to produce a List of Strings, for some reason a string "#" appears. This string is coming from a ASTComment node which has a token whose image is "#". I don't know if ASTComment is parsed invalid or the image of the token is wrong, but when changing method NodeUtils.tokenLiteral( Token t ) (in package org.apache.velocity.runtime.parser.node) to this:

Marnix van Bochove
added a comment - 21/Feb/08 10:28 I found a solution to this problem (and the underlying issue as mentioned in issue VELOCITY-580 :
The problem is when method getASTAsStringArray(Node rootNode) of class Macro (package org.apache.velocity.runtime.directive) is called to produce a List of Strings, for some reason a string " #" appears. This string is coming from a ASTComment node which has a token whose image is " #". I don't know if ASTComment is parsed invalid or the image of the token is wrong, but when changing method NodeUtils.tokenLiteral( Token t ) (in package org.apache.velocity.runtime.parser.node) to this:
public static String tokenLiteral( Token t )
{
String result;
// Issue: VELOCITY-580
// Marnix 2008-02-20: Look at king of token and return "" when it's a multiline comment
if (t.kind == ParserConstants.MULTI_LINE_COMMENT)
{
result = "";
}
else
{
result = specialText( t ) + t.image;
}
return result;
}
the problem of "*#" in macro output will disappear.
Another solution could be, to change the image of the first token inside a ASTComment Node.

I'm no expert in how Velocity parses and executes templates, but this seems like the kind of thing that should be fixed in the parser itself, not an AST walker. I'm not sure how the team feels, but I'm not sure there's a reason to do anything with comments but completely ignore them during the building of the AST.

From the stack trace, though, it looks like this is actually /during/ parsing, rather than evaluation.

That would indicate to me that the problem /is/ with the parser (probably the grammar). Is the method indicated in the previous comment automatically generated by javacc/antlr/jlex+cup or whatever compiler compiler we use? If so, the opportunities for fixing this bug in an auto-generated method are quite limited.

Christopher Schultz
added a comment - 21/Feb/08 15:00 Marnix,
I'm no expert in how Velocity parses and executes templates, but this seems like the kind of thing that should be fixed in the parser itself, not an AST walker. I'm not sure how the team feels, but I'm not sure there's a reason to do anything with comments but completely ignore them during the building of the AST.
From the stack trace, though, it looks like this is actually /during/ parsing, rather than evaluation.
That would indicate to me that the problem /is/ with the parser (probably the grammar). Is the method indicated in the previous comment automatically generated by javacc/antlr/jlex+cup or whatever compiler compiler we use? If so, the opportunities for fixing this bug in an auto-generated method are quite limited.

Marnix van Bochove
added a comment - 21/Feb/08 17:34 Christopher,
I think the ASTComment node should not be in the list, so I agree it's probably a problem with the parser. I don't know how the tokenLiteral method is generated or not.

NodeUtils.tokenLiteral() is not a generated class/method. But it does seem odd to have the fix here when the problem seems to be rooted from elsewhere. Still, if it fixes it and no one steps up to dig for the root issue, then i'd support including this fix. Better a workaround than nothing! Marnix, do you think you could whip up a test case to go with this patch (well, it's sort of a patch ?

Nathan Bubna
added a comment - 21/Feb/08 18:53 NodeUtils.tokenLiteral() is not a generated class/method. But it does seem odd to have the fix here when the problem seems to be rooted from elsewhere. Still, if it fixes it and no one steps up to dig for the root issue, then i'd support including this fix. Better a workaround than nothing! Marnix, do you think you could whip up a test case to go with this patch (well, it's sort of a patch ?

I use velocity to produce a application which consists of 385 generated java files. With the patch the resulting formatted java code produced by Velocity 4 is the same as produced by Velocity 5 (with patch). What's the best way to proof this patch for the project? I could make a unit test which fails without the patch and succeeds with the patch.

Marnix van Bochove
added a comment - 21/Feb/08 20:10 I used this template to produce the problem:
#macro(someMacro )
#*
*#
#end
#someMacro()
I use velocity to produce a application which consists of 385 generated java files. With the patch the resulting formatted java code produced by Velocity 4 is the same as produced by Velocity 5 (with patch). What's the best way to proof this patch for the project? I could make a unit test which fails without the patch and succeeds with the patch.

Ah, so this is really a test for VELOCITY-580. I'm not seeing any parse exceptions, just an extra *# in the output. I suppose that makes sense since NodeUtils doesn't have to do with parsing . I see your original patch comment refers to 580, not 537; i clearly wasn't paying sufficient attention earlier. I'll rename the test case to match. Anyway, it's great to have either dealt with, even if it's just a workaround. I'll probably hold off on committing the code for now, in hopes someone addresses the root cause and this won't be necessary. Thanks, this certainly resolves 580 nicely!

Nathan Bubna
added a comment - 21/Feb/08 23:21 Ah, so this is really a test for VELOCITY-580 . I'm not seeing any parse exceptions, just an extra *# in the output. I suppose that makes sense since NodeUtils doesn't have to do with parsing . I see your original patch comment refers to 580, not 537; i clearly wasn't paying sufficient attention earlier. I'll rename the test case to match. Anyway, it's great to have either dealt with, even if it's just a workaround. I'll probably hold off on committing the code for now, in hopes someone addresses the root cause and this won't be necessary. Thanks, this certainly resolves 580 nicely!

I agree that the issue needs to be fixed in the parser itself, because the change to NodeUtils is just masking out one symptom of the problem.
The parser, or rather the lexer is seriously going haywire. Depending on the number and configuration of the comments ParseExceptions occur, or comment boundaries get mismatched resulting in cropped literal text and ommitted logic. I'll try to provide more testcases showcasing the problems. Too bad that JavaCC is an absolute b*tch to debug. It would be most helpful if the folks that developed this particular parser could take a look at this...

Rafal Krzewski
added a comment - 16/May/08 12:29 I agree that the issue needs to be fixed in the parser itself, because the change to NodeUtils is just masking out one symptom of the problem.
The parser, or rather the lexer is seriously going haywire. Depending on the number and configuration of the comments ParseExceptions occur, or comment boundaries get mismatched resulting in cropped literal text and ommitted logic. I'll try to provide more testcases showcasing the problems. Too bad that JavaCC is an absolute b*tch to debug. It would be most helpful if the folks that developed this particular parser could take a look at this...

agreed. i've tried to throw a little time into investigating this myself here and there, but i'm out of my depth, as i have no experience with JavaCC at this point in my life. the time it would take me to figure this out is probably an order of magnitude higher than it is for some others of you who have played with the parser already.

this is a really unfortunate regression in 1.5 and is really the biggest thing holding me back from advocating for a 1.6 release. Anyone more parser savvy got a few cycles to spare on this? Henning? Geir? Will? Supun? Christoph? (guessing at names of people likely to be more apt for this problem)

Nathan Bubna
added a comment - 16/May/08 16:24 agreed. i've tried to throw a little time into investigating this myself here and there, but i'm out of my depth, as i have no experience with JavaCC at this point in my life. the time it would take me to figure this out is probably an order of magnitude higher than it is for some others of you who have played with the parser already.
this is a really unfortunate regression in 1.5 and is really the biggest thing holding me back from advocating for a 1.6 release. Anyone more parser savvy got a few cycles to spare on this? Henning? Geir? Will? Supun? Christoph? (guessing at names of people likely to be more apt for this problem)

So, if i get no further on this, we can at least hack-patch VELOCITY-580 with Marnix's NodeUtils' patch and propose the insertion of $!null at the end as a workaround for this. That won't make me happy, but it's better than the "sorry, no options to fix this" we currently have. And perhaps this smaller test case can help more parser-savvy folks figure out the real problem. I will start looking now, but i can't spend long on this and know little about JavaCC. Help would be great...

Nathan Bubna
added a comment - 30/Jun/08 23:39 Found some time today to investigate this a bit further. The lexical error can be replicated with a template as simple as:
#macro( test )#*
*#end
#test()
If you add a quiet null reference at the end, the error disappears:
#macro( test )#*
*#$!null#end
#test()
but then VELOCITY-580 manifests and the output is
*#
So, if i get no further on this, we can at least hack-patch VELOCITY-580 with Marnix's NodeUtils' patch and propose the insertion of $!null at the end as a workaround for this. That won't make me happy, but it's better than the "sorry, no options to fix this" we currently have. And perhaps this smaller test case can help more parser-savvy folks figure out the real problem. I will start looking now, but i can't spend long on this and know little about JavaCC. Help would be great...

Nathan Bubna
added a comment - 30/Jun/08 23:57 Actually, it's better than i thought; Marnix's patch for NodeUtils fixes both VELOCITY-580 and VELOCITY-537 . No *# in the output and no lexical errors in my unit tests for either issue.
I realize that it is not the ideal fix, but a true fix it appears to be. Thoughts, anyone?

To aid others in investigation of this (e.g. Will), i've checked in the unit tests for VELOCITY-580 and VELOCITY-537. Also, here is the relevant stack from the TokenMgrError that is at the heart of VELOCITY-537 (and is wrapped by the ParseException shown in the original description):

org.apache.velocity.runtime.parser.TokenMgrError: Lexical error at line 1, column 3. Encountered: <EOF> after : ""
at org.apache.velocity.runtime.parser.ParserTokenManager.getNextToken(ParserTokenManager.java:4093)
at org.apache.velocity.runtime.parser.Parser.jj_ntk(Parser.java:3343)
at org.apache.velocity.runtime.parser.Parser.process(Parser.java:275)
at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105)
at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078)
at org.apache.velocity.runtime.directive.VelocimacroProxy.parseTree(VelocimacroProxy.java:355)
at org.apache.velocity.runtime.directive.VelocimacroProxy.setupMacro(VelocimacroProxy.java:335)
at org.apache.velocity.runtime.directive.VelocimacroProxy.init(VelocimacroProxy.java:322)
at org.apache.velocity.runtime.directive.RuntimeMacro.render(RuntimeMacro.java:218)
at org.apache.velocity.runtime.parser.node.ASTDirective.render(ASTDirective.java:178)
at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:317)
at org.apache.velocity.Template.merge(Template.java:324)
at org.apache.velocity.Template.merge(Template.java:232)
at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:96)

I also traced the callstack of the relevant call to NodeUtils.tokenLiteral() (where Marnix's patch is applied) to try and figure out why his patch worked. The stack is:

at org.apache.velocity.runtime.parser.node.NodeUtils.tokenLiteral(NodeUtils.java:159)
at org.apache.velocity.runtime.directive.Macro.getASTAsStringArray(Macro.java:313)
at org.apache.velocity.runtime.directive.Macro.processAndRegister(Macro.java:188)
at org.apache.velocity.runtime.parser.Parser.Directive(Parser.java:902)
at org.apache.velocity.runtime.parser.Parser.Statement(Parser.java:365)
at org.apache.velocity.runtime.parser.Parser.process(Parser.java:303)
at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105)
at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078)
at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1008)
at org.apache.velocity.Template.process(Template.java:121)
at org.apache.velocity.runtime.resource.ResourceManagerImpl.loadResource(ResourceManagerImpl.java:434)
at org.apache.velocity.runtime.resource.ResourceManagerImpl.getResource(ResourceManagerImpl.java:350)
at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1342)
at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1317)
at org.apache.velocity.runtime.RuntimeSingleton.getTemplate(RuntimeSingleton.java:303)
at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:88)

I'm not sure anymore how much this is a JavaCC grammar/parser problem. The lexical error is only thrown when you try to use the macro in question. It appears that the content is parsed a second time. I wonder if it is being mangled in between...

Nathan Bubna
added a comment - 01/Jul/08 01:05 To aid others in investigation of this (e.g. Will), i've checked in the unit tests for VELOCITY-580 and VELOCITY-537 . Also, here is the relevant stack from the TokenMgrError that is at the heart of VELOCITY-537 (and is wrapped by the ParseException shown in the original description):
org.apache.velocity.runtime.parser.TokenMgrError: Lexical error at line 1, column 3. Encountered: <EOF> after : ""
at org.apache.velocity.runtime.parser.ParserTokenManager.getNextToken(ParserTokenManager.java:4093)
at org.apache.velocity.runtime.parser.Parser.jj_ntk(Parser.java:3343)
at org.apache.velocity.runtime.parser.Parser.process(Parser.java:275)
at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105)
at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078)
at org.apache.velocity.runtime.directive.VelocimacroProxy.parseTree(VelocimacroProxy.java:355)
at org.apache.velocity.runtime.directive.VelocimacroProxy.setupMacro(VelocimacroProxy.java:335)
at org.apache.velocity.runtime.directive.VelocimacroProxy.init(VelocimacroProxy.java:322)
at org.apache.velocity.runtime.directive.RuntimeMacro.render(RuntimeMacro.java:218)
at org.apache.velocity.runtime.parser.node.ASTDirective.render(ASTDirective.java:178)
at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:317)
at org.apache.velocity.Template.merge(Template.java:324)
at org.apache.velocity.Template.merge(Template.java:232)
at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:96)
I also traced the callstack of the relevant call to NodeUtils.tokenLiteral() (where Marnix's patch is applied) to try and figure out why his patch worked. The stack is:
at org.apache.velocity.runtime.parser.node.NodeUtils.tokenLiteral(NodeUtils.java:159)
at org.apache.velocity.runtime.directive.Macro.getASTAsStringArray(Macro.java:313)
at org.apache.velocity.runtime.directive.Macro.processAndRegister(Macro.java:188)
at org.apache.velocity.runtime.parser.Parser.Directive(Parser.java:902)
at org.apache.velocity.runtime.parser.Parser.Statement(Parser.java:365)
at org.apache.velocity.runtime.parser.Parser.process(Parser.java:303)
at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105)
at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078)
at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1008)
at org.apache.velocity.Template.process(Template.java:121)
at org.apache.velocity.runtime.resource.ResourceManagerImpl.loadResource(ResourceManagerImpl.java:434)
at org.apache.velocity.runtime.resource.ResourceManagerImpl.getResource(ResourceManagerImpl.java:350)
at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1342)
at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1317)
at org.apache.velocity.runtime.RuntimeSingleton.getTemplate(RuntimeSingleton.java:303)
at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:88)
I'm not sure anymore how much this is a JavaCC grammar/parser problem. The lexical error is only thrown when you try to use the macro in question. It appears that the content is parsed a second time. I wonder if it is being mangled in between...

Played with different examples. The problem seems to be that the first #* is being skipped by the parser. (also shown in Nathan's examples). All marnix's patch does is skip the # which is generating an error due to the missing #.

I haven't tried it, but I'm guessing that

#macro( test537f )#*
abc*#test#end

will incorrectly show "abc" with Marnix's patch. So I don't think this is the way to go.

Will Glass-Husain
added a comment - 01/Jul/08 13:36 Played with different examples. The problem seems to be that the first #* is being skipped by the parser. (also shown in Nathan's examples). All marnix's patch does is skip the # which is generating an error due to the missing # .
I haven't tried it, but I'm guessing that
#macro( test537f )#*
abc*#test#end
will incorrectly show "abc" with Marnix's patch. So I don't think this is the way to go.
Let me look at this further again later today.

Actually, i believe it's that the parser is errantly leaving in the *# token when it directly precedes the #end

The start token and bodies of the multi-line comments never get to tokenLiteral() or getASTAsStringArray() in my testing. Changing the tests for 537 and 580 to have text in the comments does not make any difference, either with or without Marnix's patch. Really, i've tried most variations i can think of. Marnix's patch does seem to fix them all. Though ideally, the parser should eat the *# token along with the rest of the comment.

Nathan Bubna
added a comment - 01/Jul/08 14:43 Actually, i believe it's that the parser is errantly leaving in the *# token when it directly precedes the #end
The start token and bodies of the multi-line comments never get to tokenLiteral() or getASTAsStringArray() in my testing. Changing the tests for 537 and 580 to have text in the comments does not make any difference, either with or without Marnix's patch. Really, i've tried most variations i can think of. Marnix's patch does seem to fix them all. Though ideally, the parser should eat the *# token along with the rest of the comment.

FYI - I've got a fix to the parser for this issue. Almost. Just trying to make the unit tests pass.

I spent about 6 hours working through this (surprisingly complex!). If we want to keep track of comments with the parser, then Marnix's patch is probably the best solution. But I think we should take the comments out in the tokenizer and only track the actual legit VTL. I think I have that, just need to make sure all edge cases are covered.

Will Glass-Husain
added a comment - 08/Jul/08 03:23 FYI - I've got a fix to the parser for this issue. Almost. Just trying to make the unit tests pass.
I spent about 6 hours working through this (surprisingly complex!). If we want to keep track of comments with the parser, then Marnix's patch is probably the best solution. But I think we should take the comments out in the tokenizer and only track the actual legit VTL. I think I have that, just need to make sure all edge cases are covered.

Hi - I spent another few hours on this. Tried to remove comments in the lexer. Turns out I re-introduced hitting a number of the comment-related bugs in the earlier versions of Velocity. (Hooray for regression tests). Specifically, if you change the comment tokens to SKIP instead of TOKEN, the MORE for a $ reference never ends. This means that the test

Taking a step back, here's why this is all happening. When a comment is encountered a ASTComment node is included in the parse tree. (instead of just being skipped at the token level like you'd expect). When a page is rendered, comments do not render. So far, so good. But when a comment is included in a macro, the nodes of the parse tree are strung back together into text so that the actual string body of the macro is stored. The problem is that the full comment is not stored, just the ending "#". Consequently, any multiline comment in a macro results in an extra "#" being included in the macro. This may cause an error (e.g. the original VELOCITY-537) or just display "*#" literally.

There's two approaches to solving this. Marnix's patch simply ignores the ASTComment token when creating the body for the macro. This makes the most sense to me. We could also save the complete comments and include in the macro. But since it's eventually ignored anyway this is pointless.

Bottom line-- I've applied Marnix's patch and like it. (Good work). All unit tests pass, including the breaking ones by Nathan (thanks!) and one more I added.

Will Glass-Husain
added a comment - 08/Jul/08 06:11 Hi - I spent another few hours on this. Tried to remove comments in the lexer. Turns out I re-introduced hitting a number of the comment-related bugs in the earlier versions of Velocity. (Hooray for regression tests). Specifically, if you change the comment tokens to SKIP instead of TOKEN, the MORE for a $ reference never ends. This means that the test
$## comment
something
which should produce "$something" actually outputs "something". instead.
So I gave up on that approach.
Taking a step back, here's why this is all happening. When a comment is encountered a ASTComment node is included in the parse tree. (instead of just being skipped at the token level like you'd expect). When a page is rendered, comments do not render. So far, so good. But when a comment is included in a macro, the nodes of the parse tree are strung back together into text so that the actual string body of the macro is stored. The problem is that the full comment is not stored, just the ending " #". Consequently, any multiline comment in a macro results in an extra " #" being included in the macro. This may cause an error (e.g. the original VELOCITY-537 ) or just display "*#" literally.
There's two approaches to solving this. Marnix's patch simply ignores the ASTComment token when creating the body for the macro. This makes the most sense to me. We could also save the complete comments and include in the macro. But since it's eventually ignored anyway this is pointless.
Bottom line-- I've applied Marnix's patch and like it. (Good work). All unit tests pass, including the breaking ones by Nathan (thanks!) and one more I added.
WILL