[VELOCITY-889] Fix lookahead issue in macro arguments parsing

git-svn-id: https://svn.apache.org/repos/asf/velocity/engine/trunk@1843211 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 196 insertions, 89 deletions

diff --git a/velocity-engine-core/src/main/parser/Parser.jjt b/velocity-engine-core/src/main/parser/Parser.jjt
index bc0e8198..1c41aece 100644
--- a/velocity-engine-core/src/main/parser/Parser.jjt
+++ b/velocity-engine-core/src/main/parser/Parser.jjt
@@ -361,6 +361,64 @@ public class Parser
     }
 
     /**
+     * Check whether there is a right parenthesis with leading optional
+     * whitespaces. This method is used in the semantic look ahead of
+     * Directive method. This is done in code instead of as a production
+     * for simplicity and efficiency.
+     */
+    private boolean isRightParenthesis()
+    {
+        char c;
+        int no = -1;
+        try {
+            while(true)
+            {
+                /**
+                 * Read a character
+                 */
+                 if (no == -1)
+                 {
+                     switch (getToken(1).kind)
+                     {
+                        case RPAREN:
+                            return true;
+                        case WHITESPACE:
+                        case NEWLINE:
+                            no = 0;
+                            break;
+                        default:
+                            return false;
+                     }
+                 }
+                c = velcharstream.readChar();
+                no++;
+                if (c == ')')
+                {
+                    return true;
+                }
+                /**
+                 * if not a white space return
+                 */
+                else if (c != ' ' && c != '\n' && c != '\r' && c != '\t')
+                {
+                    return false;
+                }
+            }
+        }
+        catch(IOException e)
+        {
+        }
+        finally
+        {
+            /**
+             * Backup the stream to the initial state
+             */
+            if (no > 0) velcharstream.backup(no);
+        }
+        return false;
+    }
+
+    /**
      * We use this method in a lookahead to determine if we are in a macro
      * default value assignment.  The standard lookahead is not smart enough.
      * here we look for the equals after the reference.
@@ -1652,70 +1710,73 @@ boolean Directive() :
      /**
       * Look for the pattern [WHITESPACE] <LPAREN>
       */
-    (LOOKAHEAD( { isLeftParenthesis() } )
-    /*
-     *  if this is indeed a token, match the #foo ( arg ) pattern
-     */
-    ((<WHITESPACE> | <NEWLINE>)* <LPAREN> ( LOOKAHEAD(2) (<WHITESPACE> | <NEWLINE>)* [<COMMA> (<WHITESPACE> | <NEWLINE>)*]
-       (
-           [LOOKAHEAD( { isMacro && isAssignment() })
-            DirectiveAssign() (<WHITESPACE> | <NEWLINE>)* <EQUALS> ( <WHITESPACE> | <NEWLINE> )*
-           {
-               argtypes.add(ParserTreeConstants.JJTDIRECTIVEASSIGN);
-           }
-           ]
-           LOOKAHEAD( { getToken(1).kind != RPAREN } )
-           (
-            argType = DirectiveArg()
+    (
+      LOOKAHEAD( { isLeftParenthesis() } )
+      /*
+       *  if this is indeed a token, match the #foo ( arg, arg... ) pattern
+       */
+      (
+        (<WHITESPACE> | <NEWLINE>)* <LPAREN>
+        (
+          LOOKAHEAD({ !isRightParenthesis() }) (<WHITESPACE> | <NEWLINE>)* [<COMMA> (<WHITESPACE> | <NEWLINE>)*]
+          (
+            [
+              LOOKAHEAD( { isMacro && isAssignment() })
+              DirectiveAssign() (<WHITESPACE> | <NEWLINE>)* <EQUALS> ( <WHITESPACE> | <NEWLINE> )*
+              {
+                  argtypes.add(ParserTreeConstants.JJTDIRECTIVEASSIGN);
+              }
+            ]
+            LOOKAHEAD( { !isRightParenthesis() } )
+            (
+              argType = DirectiveArg()
+              {
+                  argtypes.add(argType);
+                  if (d == null && argType == ParserTreeConstants.JJTWORD)
+                  {
+                      if (isVM)
+                      {
+                          throw new MacroParseException("Invalid argument "
+                              + (argPos+1) + " in macro call " + id.image, currentTemplate.getName(), id);
+                      }
+                  }
+                  argPos++;
+              }
+            )
+            |
             {
-                argtypes.add(argType);
-                if (d == null && argType == ParserTreeConstants.JJTWORD)
+                if (!isMacro)
                 {
-                    if (isVM)
-                    {
-                        throw new MacroParseException("Invalid argument "
-                          + (argPos+1) + " in macro call " + id.image, currentTemplate.getName(), id);
-                    }
+                    // We only allow line comments in macro definitions for now
+                    throw new MacroParseException("A Line comment is not allowed in " + id.image
+                        + " arguments", currentTemplate.getName(), id);
                 }
-
-                argPos++;
             }
-           )
-        |
-        {
-          if (!isMacro)
-          {
-              // We only allow line comments in macro definitions for now
-              throw new MacroParseException("A Line comment is not allowed in " + id.image
-                + " arguments", currentTemplate.getName(), id);
-          }
-        }
-
-         <SINGLE_LINE_COMMENT_START> [<SINGLE_LINE_COMMENT>]
+            <SINGLE_LINE_COMMENT_START> [<SINGLE_LINE_COMMENT>]
+          )
+        )* (<WHITESPACE> | <NEWLINE>)* <RPAREN>
       )
-    )* (<WHITESPACE> | <NEWLINE>)* <RPAREN>
-     )
-    |
-    {
-        token_source.stateStackPop();
-    }
+      |
+      {
+          token_source.stateStackPop();
+      }
     )
-     [
-       LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
-       {
-           if (directiveType == Directive.LINE)
-           {
-               jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
-               newlineAtEnd = true;
-           }
-           else
-           {
-               blockPrefix = (t == null ? u.image : t.image + u.image);
-               newlineBeforeStatement = true;
-           }
-           t = u = null;
-       }
-     ]
+    [
+      LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
+      {
+          if (directiveType == Directive.LINE)
+          {
+              jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
+              newlineAtEnd = true;
+          }
+          else
+          {
+              blockPrefix = (t == null ? u.image : t.image + u.image);
+              newlineBeforeStatement = true;
+          }
+          t = u = null;
+      }
+    ]
     {
         if (d != null)
         {
@@ -1729,32 +1790,40 @@ boolean Directive() :
     /*
      *  and the following block if the PD needs it
      */
-    (((
-        LOOKAHEAD( { getToken(1).kind != END && ( !newlineBeforeStatement || getToken(1).kind != WHITESPACE || getToken(2).kind != END ) }) newlineBeforeStatement = Statement(newlineBeforeStatement))*
+    (
+      (
+        (
+          LOOKAHEAD( { getToken(1).kind != END && ( !newlineBeforeStatement || getToken(1).kind != WHITESPACE || getToken(2).kind != END ) }) newlineBeforeStatement = Statement(newlineBeforeStatement)
+        )*
         {
             block = jjtThis;
             block.setPrefix(blockPrefix);
-        })
-        #Block)
-    [ LOOKAHEAD( 1, { newlineBeforeStatement })
-     (t = <WHITESPACE>)
+        }
+      )
+      #Block
+    )
+    [
+      LOOKAHEAD( 1, { newlineBeforeStatement })
+      (t = <WHITESPACE>)
       {
           block.setPostfix(t.image);
           t = null;
       }
     ]
-    ((end = <END>)
-     [ LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
-     {
-         jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
-         t = u = null;
-         newlineAtEnd = true;
-     }
-     ]
-     {
-         int pos = end.image.lastIndexOf('#');
-         if (pos > 0) block.setMorePostfix(end.image.substring(0, pos));
-     }
+    (
+      (end = <END>)
+      [
+        LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
+        {
+            jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
+            t = u = null;
+            newlineAtEnd = true;
+        }
+      ]
+      {
+          int pos = end.image.lastIndexOf('#');
+          if (pos > 0) block.setMorePostfix(end.image.substring(0, pos));
+      }
     )
     {
         /*
@@ -1764,7 +1833,6 @@ boolean Directive() :
          *     as long as things were always defined before use.  This way
          *     we don't have to worry about forward references and such...
          */
-
         if (isMacro)
         {
             // Add the macro name so that we can peform escape processing
@@ -1772,16 +1840,13 @@ boolean Directive() :
             String macroName = jjtThis.jjtGetChild(0).getFirstToken().image;
             macroNames.put(macroName, macroName);
         }
-
         if (d != null)
         {
             d.checkArgs(argtypes, id, currentTemplate.getName());
         }
-
         /*
          *  VM : end
          */
-
         return newlineAtEnd;
     }
 }
diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java
index e5356273..ea5c26d8 100644
--- a/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java
+++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java
@@ -160,15 +160,6 @@ public abstract class BaseTestCase extends TestCase implements TemplateTestBase
         }
     }
 
-    public void testBase()
-    {
-        if (DEBUG && engine != null)
-        {
-            assertSchmoo("");
-            assertSchmoo("abc\n123");
-        }
-    }
-
     /**
      * Compare an expected string with the given loaded template
      */
diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java
new file mode 100755
index 00000000..d4532f84
--- /dev/null
+++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java
@@ -0,0 +1,51 @@
+package org.apache.velocity.test.issues;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.velocity.test.BaseTestCase;
+
+/**
+ * This class tests VELOCITY-589.
+ */
+public class Velocity889TestCase extends BaseTestCase
+{
+    public Velocity889TestCase(String name)
+    {
+       super(name);
+    }
+
+    public void testSpaceBeforeRParen()
+    {
+        assertEvalEquals("#foo(\n)", "#foo(\n)");
+        assertEvalEquals("#foo(\n )", "#foo(\n )");
+    }
+
+    public void testSpaceBeforeRParenWithArg()
+    {
+        assertEvalEquals("#foo(\n$bar\n)", "#foo(\n$bar\n)");
+        assertEvalEquals("#foo(\n $bar\n )", "#foo(\n $bar\n )");
+    }
+
+    public void testSpaceBeforeRParenWithDefaultArg()
+    {
+        assertEvalEquals("", "#macro(\nfoo\n,\n$bar\n=\n'bar')\n#end");
+        assertEvalEquals("", "#macro(\n foo\n ,\n $bar\n =\n 'bar'\n )\n #end");
+    }
+}