Sync from internal

---

Cache Method objects per template rather than per template evaluation.

In a somewhat artificial benchmark, this sped up evaluation by 35%. The benchmark compiles AutoValueTest.java 100 times, and measures how much time was spent by AutoValueProcessor in template evaluation. AutoValueTest.java has 40 @AutoValue classes, and each of those triggers a separate template evaluation. Previously every one of those created a new Method cache (MethodFinder object). Now only the first one (on each iteration of the benchmark) does.

Compilation runs will rarely have as many as 40 @AutoValue classes, but they have often have several, so there is still some benefit.

According to this benchmark, EscapeVelocity and Apache Velocity now have indistinguishable performance.

Internal change: 245835876

---

Avoid excessive reflection overhead by caching the results of method lookups. On an ad-hoc benchmark this improved template evaluation time by 38%. That means that code generators such as AutoValue that use EscapeVelocity should see a substantial speedup.

Internal change: 244671738

---

If $foo is a Map then Velocity interprets $foo.bar the same as $foo["bar"]. Previously EscapeVelocity interpreted it the same as for other objects, by looking for a getBar() method (or boolean isBar()).

It turns out that autoannotation.vm was depending on the old behaviour, so fix that.

Internal change: 244364373

14 files changed, 600 insertions, 231 deletions

diff --git a/README.md b/README.md
index 0f24bc5..e716ecf 100644
--- a/README.md
+++ b/README.md
@@ -14,9 +14,6 @@ the exact same string that Velocity produces. If not, that is a bug.
 EscapeVelocity has no facilities for HTML escaping and it is not appropriate for producing
 HTML output that might include portions of untrusted input.
 
-<!-- MOE:begin_strip -->
-[TOC]
-<!-- MOE:end_strip -->
 
 ## Motivation
 
diff --git a/pom.xml b/pom.xml
index 8f6da5d..076165b 100644
--- a/pom.xml
+++ b/pom.xml
@@ -74,7 +74,7 @@
     <dependency>
       <groupId>com.google.truth</groupId>
       <artifactId>truth</artifactId>
-      <version>0.36</version>
+      <version>0.44</version>
       <scope>test</scope>
     </dependency>
   </dependencies>
diff --git a/src/main/java/com/google/escapevelocity/DirectiveNode.java b/src/main/java/com/google/escapevelocity/DirectiveNode.java
index 2d4decc..fd0cd22 100644
--- a/src/main/java/com/google/escapevelocity/DirectiveNode.java
+++ b/src/main/java/com/google/escapevelocity/DirectiveNode.java
@@ -122,7 +122,7 @@ abstract class DirectiveNode extends Node {
       }
       Runnable undo = context.setVar(var, null);
       StringBuilder sb = new StringBuilder();
-      Iterator<?> it = iterable.iterator();
+      CountingIterator it = new CountingIterator(iterable.iterator());
       Runnable undoForEach = context.setVar("foreach", new ForEachVar(it));
       while (it.hasNext()) {
         context.setVar(var, it.next());
@@ -133,21 +133,50 @@ abstract class DirectiveNode extends Node {
       return sb.toString();
     }
 
+    private static class CountingIterator implements Iterator<Object> {
+      private final Iterator<?> iterator;
+      private int index = -1;
+
+      CountingIterator(Iterator<?> iterator) {
+        this.iterator = iterator;
+      }
+
+      @Override
+      public boolean hasNext() {
+        return iterator.hasNext();
+      }
+
+      @Override
+      public Object next() {
+        Object next = iterator.next();
+        index++;
+        return next;
+      }
+
+      int index() {
+        return index;
+      }
+    }
+
     /**
      *  This class is the type of the variable {@code $foreach} that is defined within
      * {@code #foreach} loops. Its {@link #getHasNext()} method means that we can write
-     * {@code #if ($foreach.hasNext)}.
+     * {@code #if ($foreach.hasNext)} and likewise for {@link #getIndex()}.
      */
     private static class ForEachVar {
-      private final Iterator<?> iterator;
+      private final CountingIterator iterator;
 
-      ForEachVar(Iterator<?> iterator) {
+      ForEachVar(CountingIterator iterator) {
         this.iterator = iterator;
       }
 
       public boolean getHasNext() {
         return iterator.hasNext();
       }
+
+      public int getIndex() {
+        return iterator.index();
+      }
     }
   }
 
diff --git a/src/main/java/com/google/escapevelocity/EvaluationContext.java b/src/main/java/com/google/escapevelocity/EvaluationContext.java
index d2f2914..d40b717 100644
--- a/src/main/java/com/google/escapevelocity/EvaluationContext.java
+++ b/src/main/java/com/google/escapevelocity/EvaluationContext.java
@@ -15,6 +15,8 @@
  */
 package com.google.escapevelocity;
 
+import com.google.common.collect.ImmutableSet;
+import java.lang.reflect.Method;
 import java.util.Map;
 import java.util.TreeMap;
 
@@ -40,11 +42,16 @@ interface EvaluationContext {
    */
   Runnable setVar(final String var, Object value);
 
+  /** See {@link MethodFinder#publicMethodsWithName}. */
+  ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name);
+
   class PlainEvaluationContext implements EvaluationContext {
     private final Map<String, Object> vars;
+    private final MethodFinder methodFinder;
 
-    PlainEvaluationContext(Map<String, ?> vars) {
-      this.vars = new TreeMap<String, Object>(vars);
+    PlainEvaluationContext(Map<String, ?> vars, MethodFinder methodFinder) {
+      this.vars = new TreeMap<>(vars);
+      this.methodFinder = methodFinder;
     }
 
     @Override
@@ -69,5 +76,10 @@ interface EvaluationContext {
       vars.put(var, value);
       return undo;
     }
+
+    @Override
+    public ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name) {
+      return methodFinder.publicMethodsWithName(startClass, name);
+    }
   }
 }
diff --git a/src/main/java/com/google/escapevelocity/EvaluationException.java b/src/main/java/com/google/escapevelocity/EvaluationException.java
index 2457d0a..a1f25a4 100644
--- a/src/main/java/com/google/escapevelocity/EvaluationException.java
+++ b/src/main/java/com/google/escapevelocity/EvaluationException.java
@@ -29,6 +29,6 @@ public class EvaluationException extends RuntimeException {
   }
 
   EvaluationException(String message, Throwable cause) {
-    super(cause);
+    super(message, cause);
   }
 }
diff --git a/src/main/java/com/google/escapevelocity/Macro.java b/src/main/java/com/google/escapevelocity/Macro.java
index b7a547f..afa7bf0 100644
--- a/src/main/java/com/google/escapevelocity/Macro.java
+++ b/src/main/java/com/google/escapevelocity/Macro.java
@@ -17,11 +17,12 @@ package com.google.escapevelocity;
 
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import java.lang.reflect.Method;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
-
 /**
  * A macro definition. Macros appear in templates using the syntax {@code #macro (m $x $y) ... #end}
  * and each one produces an instance of this class. Evaluating a macro involves setting the
@@ -131,5 +132,10 @@ class Macro {
         };
       }
     }
+
+    @Override
+    public ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name) {
+      return originalEvaluationContext.publicMethodsWithName(startClass, name);
+    }
   }
 }
diff --git a/src/main/java/com/google/escapevelocity/MethodFinder.java b/src/main/java/com/google/escapevelocity/MethodFinder.java
new file mode 100644
index 0000000..f8f91f5
--- /dev/null
+++ b/src/main/java/com/google/escapevelocity/MethodFinder.java
@@ -0,0 +1,172 @@
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Licensed 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.
+ */
+package com.google.escapevelocity;
+
+import static com.google.common.reflect.Reflection.getPackageName;
+import static java.util.stream.Collectors.toSet;
+
+import com.google.common.collect.HashBasedTable;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Table;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * Finds public methods in a class. For each one, it determines the public class or interface in
+ * which it is declared. This avoids a problem with reflection, where we get an exception if we call
+ * a {@code Method} in a non-public class, even if the {@code Method} is public and if there is a
+ * public ancestor class or interface that declares it. We need to use the {@code Method} from the
+ * public ancestor.
+ *
+ * <p>Because looking for these methods is relatively expensive, an instance of this class will keep
+ * a cache of methods it previously discovered.
+ */
+class MethodFinder {
+
+  /**
+   * For a given class and name, returns all public methods of that name in the class, as previously
+   * determined by {@link #publicMethodsWithName}. The set of methods for a given class and name is
+   * saved the first time it is searched for, and returned directly thereafter. It may be empty.
+   *
+   * <p>Currently we add the entry for any given (class, name) pair on demand. An alternative would
+   * be to add all the methods for a given class at once. With the current scheme, we may end up
+   * calling {@link Class#getMethods()} several times for the same class, if methods of the
+   * different names are called at different times. With an all-at-once scheme, we might end up
+   * computing and storing information about a bunch of methods that will never be called. Because
+   * the profiling that led to the creation of this class revealed that {@link #visibleMethods} in
+   * particular is quite expensive, it's probably best to avoid calling it unnecessarily.
+   */
+  private final Table<Class<?>, String, ImmutableSet<Method>> methodCache = HashBasedTable.create();
+
+  /**
+   * Returns the set of public methods with the given name in the given class. Here, "public
+   * methods" means public methods in public classes or interfaces. If {@code startClass} is not
+   * itself public, its methods are effectively not public either, but inherited methods may still
+   * appear in the returned set, with the {@code Method} objects belonging to public ancestors. More
+   * than one ancestor may define an appropriate method, but it doesn't matter because invoking any
+   * of those {@code Method} objects will have the same effect.
+   */
+  synchronized ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name) {
+    ImmutableSet<Method> cachedMethods = methodCache.get(startClass, name);
+    if (cachedMethods == null) {
+      cachedMethods = uncachedPublicMethodsWithName(startClass, name);
+      methodCache.put(startClass, name, cachedMethods);
+    }
+    return cachedMethods;
+  }
+
+  private ImmutableSet<Method> uncachedPublicMethodsWithName(Class<?> startClass, String name) {
+    // Class.getMethods() only returns public methods, so no need to filter explicitly for public.
+    Set<Method> methods =
+        Arrays.stream(startClass.getMethods())
+            .filter(m -> m.getName().equals(name))
+            .collect(toSet());
+    if (!classIsPublic(startClass)) {
+      methods =
+          methods.stream()
+              .map(m -> visibleMethod(m, startClass))
+              .filter(Objects::nonNull)
+              .collect(toSet());
+      // It would be a bit simpler to use ImmutableSet.toImmutableSet() here, but there've been
+      // problems in the past with versions of Guava that don't have that method.
+    }
+    return ImmutableSet.copyOf(methods);
+  }
+
+  private static final String THIS_PACKAGE = getPackageName(Node.class) + ".";
+
+  /**
+   * Returns a Method with the same name and parameter types as the given one, but that is in a
+   * public class or interface. This might be the given method, or it might be a method in a
+   * superclass or superinterface.
+   *
+   * @return a public method in a public class or interface, or null if none was found.
+   */
+  static Method visibleMethod(Method method, Class<?> in) {
+    if (in == null) {
+      return null;
+    }
+    Method methodInClass;
+    try {
+      methodInClass = in.getMethod(method.getName(), method.getParameterTypes());
+    } catch (NoSuchMethodException e) {
+      return null;
+    }
+    if (classIsPublic(in) || in.getName().startsWith(THIS_PACKAGE)) {
+      // The second disjunct is a hack to allow us to use the public methods of $foreach without
+      // having to make the ForEachVar class public. We can invoke those methods from the same
+      // package since ForEachVar is package-protected.
+      return methodInClass;
+    }
+    Method methodInSuperclass = visibleMethod(method, in.getSuperclass());
+    if (methodInSuperclass != null) {
+      return methodInSuperclass;
+    }
+    for (Class<?> superinterface : in.getInterfaces()) {
+      Method methodInSuperinterface = visibleMethod(method, superinterface);
+      if (methodInSuperinterface != null) {
+        return methodInSuperinterface;
+      }
+    }
+    return null;
+  }
+
+  /**
+   * Returns whether the given class is public as seen from this class. Prior to Java 9, a class was
+   * either public or not public. But with the introduction of modules in Java 9, a class can be
+   * marked public and yet not be visible, if it is not exported from the module it appears in. So,
+   * on Java 9, we perform an additional check on class {@code c}, which is effectively {@code
+   * c.getModule().isExported(c.getPackageName())}. We use reflection so that the code can compile
+   * on earlier Java versions.
+   */
+  private static boolean classIsPublic(Class<?> c) {
+    return Modifier.isPublic(c.getModifiers()) && classIsExported(c);
+  }
+
+  private static boolean classIsExported(Class<?> c) {
+    if (CLASS_GET_MODULE_METHOD == null) {
+      return true; // There are no modules, so all classes are exported.
+    }
+    try {
+      String pkg = getPackageName(c);
+      Object module = CLASS_GET_MODULE_METHOD.invoke(c);
+      return (Boolean) MODULE_IS_EXPORTED_METHOD.invoke(module, pkg);
+    } catch (Exception e) {
+      return false;
+    }
+  }
+
+  private static final Method CLASS_GET_MODULE_METHOD;
+  private static final Method MODULE_IS_EXPORTED_METHOD;
+
+  static {
+    Method classGetModuleMethod;
+    Method moduleIsExportedMethod;
+    try {
+      classGetModuleMethod = Class.class.getMethod("getModule");
+      Class<?> moduleClass = classGetModuleMethod.getReturnType();
+      moduleIsExportedMethod = moduleClass.getMethod("isExported", String.class);
+    } catch (Exception e) {
+      classGetModuleMethod = null;
+      moduleIsExportedMethod = null;
+    }
+    CLASS_GET_MODULE_METHOD = classGetModuleMethod;
+    MODULE_IS_EXPORTED_METHOD = moduleIsExportedMethod;
+  }
+}
diff --git a/src/main/java/com/google/escapevelocity/Node.java b/src/main/java/com/google/escapevelocity/Node.java
index 485f8a5..a017afa 100644
--- a/src/main/java/com/google/escapevelocity/Node.java
+++ b/src/main/java/com/google/escapevelocity/Node.java
@@ -64,7 +64,6 @@ abstract class Node {
     return new Cons(resourceName, lineNumber, ImmutableList.<Node>of());
   }
 
-
   /**
    * Create a new parse tree node that is the concatenation of the given ones. Evaluating the
    * new node produces the same string as evaluating each of the given nodes and concatenating the
diff --git a/src/main/java/com/google/escapevelocity/Parser.java b/src/main/java/com/google/escapevelocity/Parser.java
index 17c7fba..4416c48 100644
--- a/src/main/java/com/google/escapevelocity/Parser.java
+++ b/src/main/java/com/google/escapevelocity/Parser.java
@@ -15,6 +15,13 @@
  */
 package com.google.escapevelocity;
 
+import com.google.common.base.CharMatcher;
+import com.google.common.base.Verify;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.Iterables;
+import com.google.common.primitives.Chars;
+import com.google.common.primitives.Ints;
 import com.google.escapevelocity.DirectiveNode.SetNode;
 import com.google.escapevelocity.ExpressionNode.BinaryExpressionNode;
 import com.google.escapevelocity.ExpressionNode.NotExpressionNode;
@@ -31,13 +38,6 @@ import com.google.escapevelocity.TokenNode.ForEachTokenNode;
 import com.google.escapevelocity.TokenNode.IfTokenNode;
 import com.google.escapevelocity.TokenNode.MacroDefinitionTokenNode;
 import com.google.escapevelocity.TokenNode.NestedTokenNode;
-import com.google.common.base.CharMatcher;
-import com.google.common.base.Verify;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableListMultimap;
-import com.google.common.collect.Iterables;
-import com.google.common.primitives.Chars;
-import com.google.common.primitives.Ints;
 import java.io.IOException;
 import java.io.LineNumberReader;
 import java.io.Reader;
@@ -419,10 +419,11 @@ class Parser {
   private Node parseParse() throws IOException {
     expect('(');
     skipSpace();
-    if (c != '"') {
+    if (c != '"' && c != '\'') {
       throw parseException("#parse only supported with string literal argument");
     }
-    String nestedResourceName = readStringLiteral();
+    ExpressionNode nestedResourceNameExpression = parseStringLiteral(c, false);
+    String nestedResourceName = nestedResourceNameExpression.evaluate(null).toString();
     expect(')');
     try (Reader nestedReader = resourceOpener.openResource(nestedResourceName)) {
       Parser nestedParser = new Parser(nestedReader, nestedResourceName, resourceOpener);
@@ -438,7 +439,7 @@ class Parser {
    *                           $<id> <macro-parameter-list>
    * }</pre>
    *
-   * <p>Macro parameters are not separated by commas, though method-reference parameters are.
+   * <p>Macro parameters are optionally separated by commas.
    */
   private Node parseMacroDefinition() throws IOException {
     expect('(');
@@ -451,6 +452,10 @@ class Parser {
         next();
         break;
       }
+      if (c == ',') {
+        next();
+        skipSpace();
+      }
       if (c != '$') {
         throw parseException("Macro parameters should look like $name");
       }
@@ -518,15 +523,12 @@ class Parser {
     int startLine = lineNumber();
     int lastC = '\0';
     next();
-    while (!(lastC == '*' && c == '#')) {
-      if (c == EOF) {
-        throw new ParseException(
-            "Unterminated #* - did not see matching *#", resourceName, startLine);
-      }
+    // Consistently with Velocity, we do not make it an error if a #* comment is not closed.
+    while (!(lastC == '*' && c == '#') && c != EOF) {
       lastC = c;
       next();
     }
-    next();
+    next(); // this may read EOF twice, which works
     return new CommentTokenNode(resourceName, startLine);
   }
 
@@ -889,7 +891,6 @@ class Parser {
     }
   }
 
-
   /**
    * Parses an expression containing only literals or references.
    * <pre>{@code
@@ -905,7 +906,9 @@ class Parser {
       next();
       node = parseRequiredReference();
     } else if (c == '"') {
-      node = parseStringLiteral();
+      node = parseStringLiteral(c, true);
+    } else if (c == '\'') {
+      node = parseStringLiteral(c, false);
     } else if (c == '-') {
       // Velocity does not have a negation operator. If we see '-' it must be the start of a
       // negative integer literal.
@@ -922,30 +925,73 @@ class Parser {
     return node;
   }
 
-  private ExpressionNode parseStringLiteral() throws IOException {
-    return new ConstantExpressionNode(resourceName, lineNumber(), readStringLiteral());
-  }
-
-  private String readStringLiteral() throws IOException {
-    assert c == '"';
-    StringBuilder sb = new StringBuilder();
+  /**
+   * Parses a string literal, which may contain references to be expanded. Examples are
+   * {@code "foo"} or {@code "foo${bar}baz"}.
+   * <pre>{@code
+   * <string-literal> -> <double-quote-literal> | <single-quote-literal>
+   * <double-quote-literal> -> " <double-quote-string-contents> "
+   * <double-quote-string-contents> -> <empty> |
+   *                                   <reference> <double-quote-string-contents> |
+   *                                   <character-other-than-"> <double-quote-string-contents>
+   * <single-quote-literal> -> ' <single-quote-string-contents> '
+   * <single-quote-string-contents> -> <empty> |
+   *                                   <character-other-than-'> <single-quote-string-contents>
+   * }</pre>
+   */
+  private ExpressionNode parseStringLiteral(int quote, boolean allowReferences)
+      throws IOException {
+    assert c == quote;
     next();
-    while (c != '"') {
-      if (c == '\n' || c == EOF) {
-        throw parseException("Unterminated string constant");
-      }
-      if (c == '$' || c == '\\') {
-        // In real Velocity, you can have a $ reference expanded inside a "" string literal.
-        // There are also '' string literals where that is not so. We haven't needed that yet
-        // so it's not supported.
-        throw parseException(
-            "Escapes or references in string constants are not currently supported");
+    ImmutableList.Builder<Node> nodes = ImmutableList.builder();
+    StringBuilder sb = new StringBuilder();
+    while (c != quote) {
+      switch (c) {
+        case '\n':
+        case EOF:
+          throw parseException("Unterminated string constant");
+        case '\\':
+          throw parseException(
+              "Escapes in string constants are not currently supported");
+        case '$':
+          if (allowReferences) {
+            if (sb.length() > 0) {
+              nodes.add(new ConstantExpressionNode(resourceName, lineNumber(), sb.toString()));
+              sb.setLength(0);
+            }
+            next();
+            nodes.add(parseReference());
+            break;
+          }
+          // fall through
+        default:
+          sb.appendCodePoint(c);
+          next();
       }
-      sb.appendCodePoint(c);
-      next();
     }
     next();
-    return sb.toString();
+    if (sb.length() > 0) {
+      nodes.add(new ConstantExpressionNode(resourceName, lineNumber(), sb.toString()));
+    }
+    return new StringLiteralNode(resourceName, lineNumber(), nodes.build());
+  }
+
+  private static class StringLiteralNode extends ExpressionNode {
+    private final ImmutableList<Node> nodes;
+
+    StringLiteralNode(String resourceName, int lineNumber, ImmutableList<Node> nodes) {
+      super(resourceName, lineNumber);
+      this.nodes = nodes;
+    }
+
+    @Override
+    Object evaluate(EvaluationContext context) {
+      StringBuilder sb = new StringBuilder();
+      for (Node node : nodes) {
+        sb.append(node.evaluate(context));
+      }
+      return sb.toString();
+    }
   }
 
   private ExpressionNode parseIntLiteral(String prefix) throws IOException {
diff --git a/src/main/java/com/google/escapevelocity/ReferenceNode.java b/src/main/java/com/google/escapevelocity/ReferenceNode.java
index 4b43f77..622388f 100644
--- a/src/main/java/com/google/escapevelocity/ReferenceNode.java
+++ b/src/main/java/com/google/escapevelocity/ReferenceNode.java
@@ -15,16 +15,18 @@
  */
 package com.google.escapevelocity;
 
+import static java.util.stream.Collectors.toList;
+
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.primitives.Primitives;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 
 /**
  * A node in the parse tree that is a reference. A reference is anything beginning with {@code $},
@@ -89,21 +91,27 @@ abstract class ReferenceNode extends ExpressionNode {
       if (lhsValue == null) {
         throw evaluationException("Cannot get member " + id + " of null value");
       }
+      // If this is a Map, then Velocity looks up the property in the map.
+      if (lhsValue instanceof Map<?, ?>) {
+        Map<?, ?> map = (Map<?, ?>) lhsValue;
+        return map.get(id);
+      }
       // Velocity specifies that, given a reference .foo, it will first look for getfoo() and then
       // for getFoo(), and likewise given .Foo it will look for getFoo() and then getfoo().
       for (String prefix : PREFIXES) {
         for (boolean changeCase : CHANGE_CASE) {
           String baseId = changeCase ? changeInitialCase(id) : id;
           String methodName = prefix + baseId;
-          Method method;
-          try {
-            method = lhsValue.getClass().getMethod(methodName);
+          Optional<Method> maybeMethod =
+              context.publicMethodsWithName(lhsValue.getClass(), methodName).stream()
+                  .filter(m -> m.getParameterTypes().length == 0)
+                  .findFirst();
+          if (maybeMethod.isPresent()) {
+            Method method = maybeMethod.get();
             if (!prefix.equals("is") || method.getReturnType().equals(boolean.class)) {
               // Don't consider methods that happen to be called isFoo() but don't return boolean.
               return invokeMethod(method, lhsValue, ImmutableList.of());
             }
-          } catch (NoSuchMethodException e) {
-            // Continue with next possibility
           }
         }
       }
@@ -206,25 +214,35 @@ abstract class ReferenceNode extends ExpressionNode {
       if (lhsValue == null) {
         throw evaluationException("Cannot invoke method " + id + " on null value");
       }
-      List<Object> argValues = new ArrayList<>();
-      for (ExpressionNode arg : args) {
-        argValues.add(arg.evaluate(context));
-      }
-      List<Method> methodsWithName = new ArrayList<>();
-      for (Method method : lhsValue.getClass().getMethods()) {
-        if (method.getName().equals(id) && !method.isSynthetic()) {
-          methodsWithName.add(method);
+      try {
+        return evaluate(context, lhsValue, lhsValue.getClass());
+      } catch (EvaluationException e) {
+        // If this is a Class, try invoking a static method of the class it refers to.
+        // This is what Apache Velocity does. If the method exists as both an instance method of
+        // Class and a static method of the referenced class, then it is the instance method of
+        // Class that wins, again consistent with Velocity.
+        if (lhsValue instanceof Class<?>) {
+          return evaluate(context, null, (Class<?>) lhsValue);
         }
+        throw e;
       }
-      if (methodsWithName.isEmpty()) {
-        throw evaluationException("No method " + id + " in " + lhsValue.getClass().getName());
+    }
+
+    private Object evaluate(EvaluationContext context, Object lhsValue, Class<?> targetClass) {
+      List<Object> argValues = args.stream()
+          .map(arg -> arg.evaluate(context))
+          .collect(toList());
+      ImmutableSet<Method> publicMethodsWithName = context.publicMethodsWithName(targetClass, id);
+      if (publicMethodsWithName.isEmpty()) {
+        throw evaluationException("No method " + id + " in " + targetClass.getName());
       }
-      List<Method> compatibleMethods = new ArrayList<>();
-      for (Method method : methodsWithName) {
-        // TODO(emcmanus): support varargs, if it's useful
-        if (compatibleArgs(method.getParameterTypes(), argValues)) {
-          compatibleMethods.add(method);
-        }
+      List<Method> compatibleMethods = publicMethodsWithName.stream()
+          .filter(method -> compatibleArgs(method.getParameterTypes(), argValues))
+          .collect(toList());
+          // TODO(emcmanus): support varargs, if it's useful
+      if (compatibleMethods.size() > 1) {
+        compatibleMethods =
+            compatibleMethods.stream().filter(method -> !method.isSynthetic()).collect(toList());
       }
       switch (compatibleMethods.size()) {
         case 0:
@@ -253,7 +271,7 @@ abstract class ReferenceNode extends ExpressionNode {
         Object argValue = argValues.get(i);
         if (paramType.isPrimitive()) {
           return primitiveIsCompatible(paramType, argValue);
-        } else if (!paramType.isInstance(argValue)) {
+        } else if (argValue != null && !paramType.isInstance(argValue)) {
           return false;
         }
       }
@@ -267,7 +285,7 @@ abstract class ReferenceNode extends ExpressionNode {
       return primitiveTypeIsAssignmentCompatible(primitive, Primitives.unwrap(value.getClass()));
     }
 
-    private static final ImmutableList<Class<?>> NUMERICAL_PRIMITIVES = ImmutableList.<Class<?>>of(
+    private static final ImmutableList<Class<?>> NUMERICAL_PRIMITIVES = ImmutableList.of(
         byte.class, short.class, int.class, long.class, float.class, double.class);
     private static final int INDEX_OF_INT = NUMERICAL_PRIMITIVES.indexOf(int.class);
 
@@ -300,21 +318,9 @@ abstract class ReferenceNode extends ExpressionNode {
   }
 
   /**
-   * Invoke the given method on the given target with the given arguments. The method is expected
-   * to be public, but the class it is in might not be. In that case we will search up the
-   * hierarchy for an ancestor that is public and has the same method, and use that to invoke the
-   * method. Otherwise we would get an {@link IllegalAccessException}. More than one ancestor might
-   * define the method, but it doesn't matter which one we invoke since ultimately the code that
-   * will run will be the same.
+   * Invoke the given method on the given target with the given arguments.
    */
   Object invokeMethod(Method method, Object target, List<Object> argValues) {
-    if (!classIsPublic(target.getClass())) {
-      method = visibleMethod(method, target.getClass());
-      if (method == null) {
-        throw evaluationException(
-            "Method is not visible in class " + target.getClass().getName() + ": " + method);
-      }
-    }
     try {
       return method.invoke(target, argValues.toArray());
     } catch (InvocationTargetException e) {
@@ -323,98 +329,4 @@ abstract class ReferenceNode extends ExpressionNode {
       throw evaluationException(e);
     }
   }
-
-  private static String packageNameOf(Class<?> c) {
-    String name = c.getName();
-    int lastDot = name.lastIndexOf('.');
-    if (lastDot > 0) {
-      return name.substring(0, lastDot);
-    } else {
-      return "";
-    }
-  }
-
-  private static final String THIS_PACKAGE = packageNameOf(Node.class) + ".";
-
-  /**
-   * Returns a Method with the same name and parameter types as the given one, but that is in a
-   * public class or interface. This might be the given method, or it might be a method in a
-   * superclass or superinterface.
-   *
-   * @return a public method in a public class or interface, or null if none was found.
-   */
-  static Method visibleMethod(Method method, Class<?> in) {
-    if (in == null) {
-      return null;
-    }
-    Method methodInClass;
-    try {
-      methodInClass = in.getMethod(method.getName(), method.getParameterTypes());
-    } catch (NoSuchMethodException e) {
-      return null;
-    }
-    if (classIsPublic(in) || in.getName().startsWith(THIS_PACKAGE)) {
-      // The second disjunct is a hack to allow us to use the methods of $foreach without having
-      // to make the ForEachVar class public. We can invoke those methods from here since they
-      // are in the same package.
-      return methodInClass;
-    }
-    Method methodSuper = visibleMethod(method, in.getSuperclass());
-    if (methodSuper != null) {
-      return methodSuper;
-    }
-    for (Class<?> intf : in.getInterfaces()) {
-      Method methodIntf = visibleMethod(method, intf);
-      if (methodIntf != null) {
-        return methodIntf;
-      }
-    }
-    return null;
-  }
-
-  /**
-   * Returns whether the given class is public as seen from this class. Prior to Java 9, a class
-   * was either public or not public. But with the introduction of modules in Java 9, a class can
-   * be marked public and yet not be visible, if it is not exported from the module it appears in.
-   * So, on Java 9, we perform an additional check on class {@code c}, which is effectively
-   * {@code c.getModule().isExported(c.getPackageName())}. We use reflection so that the code can
-   * compile on earlier Java versions.
-   */
-  private static boolean classIsPublic(Class<?> c) {
-    if (!Modifier.isPublic(c.getModifiers())) {
-      return false;
-    }
-    if (CLASS_GET_MODULE_METHOD != null) {
-      return classIsExported(c);
-    }
-    return true;
-  }
-
-  private static boolean classIsExported(Class<?> c) {
-    try {
-      String pkg = packageNameOf(c);
-      Object module = CLASS_GET_MODULE_METHOD.invoke(c);
-      return (Boolean) MODULE_IS_EXPORTED_METHOD.invoke(module, pkg);
-    } catch (Exception e) {
-      return false;
-    }
-  }
-
-  private static final Method CLASS_GET_MODULE_METHOD;
-  private static final Method MODULE_IS_EXPORTED_METHOD;
-
-  static {
-    Method classGetModuleMethod;
-    Method moduleIsExportedMethod;
-    try {
-      classGetModuleMethod = Class.class.getMethod("getModule");
-      Class<?> moduleClass = classGetModuleMethod.getReturnType();
-      moduleIsExportedMethod = moduleClass.getMethod("isExported", String.class);
-    } catch (Exception e) {
-      classGetModuleMethod = null;
-      moduleIsExportedMethod = null;
-    }
-    CLASS_GET_MODULE_METHOD = classGetModuleMethod;
-    MODULE_IS_EXPORTED_METHOD = moduleIsExportedMethod;
-  }
 }
diff --git a/src/main/java/com/google/escapevelocity/Template.java b/src/main/java/com/google/escapevelocity/Template.java
index 0f4de8f..6bc75c2 100644
--- a/src/main/java/com/google/escapevelocity/Template.java
+++ b/src/main/java/com/google/escapevelocity/Template.java
@@ -31,6 +31,17 @@ import java.util.Map;
 // TODO(emcmanus): spell out exactly what Velocity features are unsupported.
 public class Template {
   private final Node root;
+  
+  /**
+   * Caches {@link Method} objects for public methods accessed through references. The first time
+   * we evaluate {@code $var.property} or {@code $var.method(...)} for a {@code $var} of a given
+   * class and for a given property or method signature, we'll store the resultant {@link Method}
+   * object. Every subsequent time we'll reuse that {@link Method}. The method lookup is quite slow
+   * so caching is useful. The main downside is that we may potentially hold on to {@link Method}
+   * objects that will never be used with this {@link Template} again. But in practice templates
+   * tend to be used repeatedly with the same classes.
+   */
+  private final MethodFinder methodFinder = new MethodFinder();
 
   /**
    * Used to resolve references to resources in the template, through {@code #parse} directives.
@@ -116,7 +127,7 @@ public class Template {
    * @return the string result of evaluating the template.
    */
   public String evaluate(Map<String, ?> vars) {
-    EvaluationContext evaluationContext = new PlainEvaluationContext(vars);
+    EvaluationContext evaluationContext = new PlainEvaluationContext(vars, methodFinder);
     return String.valueOf(root.evaluate(evaluationContext));
   }
 }
diff --git a/src/test/java/com/google/escapevelocity/MethodFinderTest.java b/src/test/java/com/google/escapevelocity/MethodFinderTest.java
new file mode 100644
index 0000000..66b8948
--- /dev/null
+++ b/src/test/java/com/google/escapevelocity/MethodFinderTest.java
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2019 Google, Inc.
+ *
+ * Licensed 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.
+ */
+package com.google.escapevelocity;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.toSet;
+
+import com.google.common.collect.ImmutableMap;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class MethodFinderTest {
+  @Test
+  public void visibleMethodFromClass() throws Exception {
+    Map<String, String> map = Collections.singletonMap("foo", "bar");
+    Class<?> mapClass = map.getClass();
+    assertThat(Modifier.isPublic(mapClass.getModifiers())).isFalse();
+    
+    Method size = mapClass.getMethod("size");
+    Method visibleSize = MethodFinder.visibleMethod(size, mapClass);
+    assertThat(visibleSize.getDeclaringClass().isInterface()).isFalse();
+    assertThat(visibleSize.invoke(map)).isEqualTo(1);
+  }
+
+  @Test
+  public void visibleMethodFromInterface() throws Exception {
+    Map<String, String> map = ImmutableMap.of("foo", "bar");
+    Map.Entry<String, String> entry = map.entrySet().iterator().next();
+    Class<?> entryClass = entry.getClass();
+    assertThat(Modifier.isPublic(entryClass.getModifiers())).isFalse();
+    
+    Method getValue = entryClass.getMethod("getValue");
+    Method visibleGetValue = MethodFinder.visibleMethod(getValue, entryClass);
+    assertThat(visibleGetValue.getDeclaringClass().isInterface()).isTrue();
+    assertThat(visibleGetValue.invoke(entry)).isEqualTo("bar");
+  }
+
+  @Test
+  public void publicMethodsWithName() {
+    List<String> list = Collections.singletonList("foo");
+    Class<?> listClass = list.getClass();
+    assertThat(Modifier.isPublic(listClass.getModifiers())).isFalse();
+    
+    MethodFinder methodFinder = new MethodFinder();
+    Set<Method> methods = methodFinder.publicMethodsWithName(listClass, "remove");
+    // This should find at least remove(int) and remove(Object).
+    assertThat(methods.size()).isAtLeast(2);
+    assertThat(methods.stream().map(Method::getName).collect(toSet())).containsExactly("remove");
+    assertThat(methods.stream().allMatch(MethodFinderTest::isPublic)).isTrue();
+    
+    // We should cache the result, meaning we get back the same result if we ask a second time.
+    Set<Method> methods2 = methodFinder.publicMethodsWithName(listClass, "remove");
+    assertThat(methods2).isSameInstanceAs(methods);
+  }
+
+  @Test
+  public void publicMethodsWithName_Nonexistent() {
+    List<String> list = Collections.singletonList("foo");
+    Class<?> listClass = list.getClass();
+    assertThat(Modifier.isPublic(listClass.getModifiers())).isFalse();
+    
+    MethodFinder methodFinder = new MethodFinder();
+    Set<Method> methods = methodFinder.publicMethodsWithName(listClass, "nonexistentMethod");
+    assertThat(methods).isEmpty();
+  }
+
+  private static boolean isPublic(Method method) {
+    return Modifier.isPublic(method.getModifiers())
+        && Modifier.isPublic(method.getDeclaringClass().getModifiers());
+  }
+}
diff --git a/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java b/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java
index 3e784f6..b1759bd 100644
--- a/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java
+++ b/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java
@@ -17,15 +17,11 @@ package com.google.escapevelocity;
 
 import static com.google.common.truth.Truth.assertThat;
 
-import com.google.escapevelocity.ReferenceNode.MethodReferenceNode;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.primitives.Primitives;
 import com.google.common.truth.Expect;
-import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
-import java.util.Collections;
-import java.util.Map;
+import com.google.escapevelocity.ReferenceNode.MethodReferenceNode;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -85,22 +81,12 @@ public class ReferenceNodeTest {
             MethodReferenceNode.primitiveTypeIsAssignmentCompatible(to, from);
         expect
             .withMessage(from + " assignable to " + to)
-            .that(expected).isEqualTo(actual);
+            .that(actual).isEqualTo(expected);
       }
     }
   }
 
   @Test
-  public void testVisibleMethod() throws Exception {
-    Map<String, String> map = Collections.singletonMap("foo", "bar");
-    Class<?> mapClass = map.getClass();
-    assertThat(Modifier.isPublic(mapClass.getModifiers())).isFalse();
-    Method size = map.getClass().getMethod("size");
-    Method visibleSize = ReferenceNode.visibleMethod(size, mapClass);
-    assertThat(visibleSize.invoke(map)).isEqualTo(1);
-  }
-
-  @Test
   public void testCompatibleArgs() {
     assertThat(MethodReferenceNode.compatibleArgs(
         new Class<?>[]{int.class}, ImmutableList.of((Object) 5))).isTrue();
diff --git a/src/test/java/com/google/escapevelocity/TemplateTest.java b/src/test/java/com/google/escapevelocity/TemplateTest.java
index 04bad8e..0503125 100644
--- a/src/test/java/com/google/escapevelocity/TemplateTest.java
+++ b/src/test/java/com/google/escapevelocity/TemplateTest.java
@@ -16,10 +16,12 @@
 package com.google.escapevelocity;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.truth.Expect;
 import java.io.ByteArrayInputStream;
 import java.io.FileNotFoundException;
@@ -27,6 +29,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.StringReader;
 import java.io.StringWriter;
+import java.io.UncheckedIOException;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -36,6 +39,7 @@ import java.util.function.Supplier;
 import org.apache.commons.collections.ExtendedProperties;
 import org.apache.velocity.VelocityContext;
 import org.apache.velocity.exception.ResourceNotFoundException;
+import org.apache.velocity.exception.VelocityException;
 import org.apache.velocity.runtime.RuntimeConstants;
 import org.apache.velocity.runtime.RuntimeInstance;
 import org.apache.velocity.runtime.log.NullLogChute;
@@ -45,7 +49,6 @@ import org.apache.velocity.runtime.resource.loader.ResourceLoader;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TestName;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -57,7 +60,6 @@ import org.junit.runners.JUnit4;
 public class TemplateTest {
   @Rule public TestName testName = new TestName();
   @Rule public Expect expect = Expect.create();
-  @Rule public ExpectedException thrown = ExpectedException.none();
 
   private RuntimeInstance velocityRuntimeInstance;
 
@@ -128,6 +130,31 @@ public class TemplateTest {
     return writer.toString();
   }
 
+  private void expectParseException(
+      String template,
+      String expectedMessageSubstring) {
+    Exception velocityException = null;
+    try {
+      SimpleNode parsedTemplate =
+          velocityRuntimeInstance.parse(new StringReader(template), testName.getMethodName());
+      VelocityContext velocityContext = new VelocityContext(new TreeMap<>());
+      velocityRuntimeInstance.render(
+          velocityContext, new StringWriter(), parsedTemplate.getTemplateName(), parsedTemplate);
+      fail("Velocity did not throw an exception for this template");
+    } catch (org.apache.velocity.runtime.parser.ParseException | VelocityException expected) {
+      velocityException = expected;
+    }
+    try {
+      Template.parseFrom(new StringReader(template));
+      fail("Velocity generated an exception, but EscapeVelocity did not: " + velocityException);
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
+    } catch (ParseException expected) {
+      assertWithMessage("Got expected exception, but message did not match")
+          .that(expected).hasMessageThat().contains(expectedMessageSubstring);
+    }
+  }
+
   @Test
   public void empty() {
     compare("");
@@ -210,13 +237,6 @@ public class TemplateTest {
     compare("$foo.!", ImmutableMap.of("foo", false));
   }
 
-  /* TODO(emcmanus): make this work.
-  @Test
-  public void substituteNotPropertyId() {
-    compare("$foo.!", ImmutableMap.of("foo", false));
-  }
-  */
-
   @Test
   public void substituteNestedProperty() {
     compare("\n$t.name.empty\n", ImmutableMap.of("t", Thread.currentThread()));
@@ -228,23 +248,64 @@ public class TemplateTest {
   }
 
   @Test
+  public void substituteMethodNoArgsSyntheticOverride() {
+    compare("<$c.isEmpty()>", ImmutableMap.of("c", ImmutableSetMultimap.of()));
+  }
+
+  @Test
   public void substituteMethodOneArg() {
     compare("<$list.get(0)>", ImmutableMap.of("list", ImmutableList.of("foo")));
   }
 
   @Test
+  public void substituteMethodOneNullArg() {
+    // This should evaluate map.containsKey(map.get("absent")), which is map.containsKey(null).
+    compare("<$map.containsKey($map.get(\"absent\"))>", ImmutableMap.of("map", ImmutableMap.of()));
+  }
+
+  @Test
   public void substituteMethodTwoArgs() {
     compare("\n$s.indexOf(\"bar\", 2)\n", ImmutableMap.of("s", "barbarbar"));
   }
 
   @Test
-  public void substituteMethodNoSynthetic() {
+  public void substituteMethodSyntheticOverloads() {
     // If we aren't careful, we'll see both the inherited `Set<K> keySet()` from Map
     // and the overridden `ImmutableSet<K> keySet()` in ImmutableMap.
     compare("$map.keySet()", ImmutableMap.of("map", ImmutableMap.of("foo", "bar")));
   }
 
   @Test
+  public void substituteStaticMethod() {
+    compare("$Integer.toHexString(23)", ImmutableMap.of("Integer", Integer.class));
+  }
+
+  @Test
+  public void substituteStaticMethodAsInstanceMethod() {
+    compare("$i.toHexString(23)", ImmutableMap.of("i", 0));
+  }
+
+  @Test
+  public void substituteClassMethod() {
+    // This is Class.getName().
+    compare("$Integer.getName()", ImmutableMap.of("Integer", Integer.class));
+  }
+
+  /** See {@link #substituteClassOrInstanceMethod}. */
+  public static class GetName {
+    public static String getName() {
+      return "Noddy";
+    }
+  }
+
+  @Test
+  public void substituteClassOrInstanceMethod() {
+    // If the method exists as both an instance method on Class and a static method on the named
+    // class, it's the instance method that wins, so this is still Class.getName().
+    compare("$GetName.getName()", ImmutableMap.of("GetName", GetName.class));
+  }
+
+  @Test
   public void substituteIndexNoBraces() {
     compare("<$map[\"x\"]>", ImmutableMap.of("map", ImmutableMap.of("x", "y")));
   }
@@ -254,6 +315,14 @@ public class TemplateTest {
     compare("<${map[\"x\"]}>", ImmutableMap.of("map", ImmutableMap.of("x", "y")));
   }
 
+  // Velocity allows you to write $map.foo instead of $map["foo"].
+  @Test
+  public void substituteMapProperty() {
+    compare("$map.foo", ImmutableMap.of("map", ImmutableMap.of("foo", "bar")));
+    // $map.empty is always equivalent to $map["empty"], never Map.isEmpty().
+    compare("$map.empty", ImmutableMap.of("map", ImmutableMap.of("empty", "foo")));
+  }
+
   @Test
   public void substituteIndexThenProperty() {
     compare("<$map[2].name>", ImmutableMap.of("map", ImmutableMap.of(2, getClass())));
@@ -291,6 +360,29 @@ public class TemplateTest {
   }
 
   @Test
+  public void substituteInString() {
+    String template =
+        "#foreach ($a in $list)"
+            + "#set ($s = \"THING_${foreach.index}\")"
+            + "$s,$s;"
+            + "#end";
+    compare(template, ImmutableMap.of("list", ImmutableList.of(1, 2, 3)));
+    compare("#set ($s = \"$x\") <$s>", ImmutableMap.of("x", "fred"));
+    compare("#set ($s = \"==$x$y\") <$s>", ImmutableMap.of("x", "fred", "y", "jim"));
+    compare("#set ($s = \"$x$y==\") <$s>", ImmutableMap.of("x", "fred", "y", "jim"));
+  }
+
+  @Test
+  public void stringOperationsOnSubstitution() {
+    compare("#set ($s = \"a${b}c\") $s.length()", ImmutableMap.of("b", 23));
+  }
+
+  @Test
+  public void singleQuoteNoSubstitution() {
+    compare("#set ($s = 'a${b}c') x${s}y", ImmutableMap.of("b", 23));
+  }
+
+  @Test
   public void simpleSet() {
     compare("$x#set ($x = 17)#set ($y = 23) ($x, $y)", ImmutableMap.of("x", 1));
   }
@@ -506,6 +598,18 @@ public class TemplateTest {
   }
 
   @Test
+  public void forEachIndex() {
+    String template =
+        "#foreach ($x in $list)"
+            + "[$foreach.index]"
+            + "#foreach ($y in $list)"
+            + "($foreach.index)==$x.$y=="
+            + "#end"
+            + "#end";
+    compare(template, ImmutableMap.of("list", ImmutableList.of("blim", "blam", "blum")));
+  }
+
+  @Test
   public void setSpacing() {
     // The spacing in the output from #set is eccentric.
     compare("x#set ($x = 0)x");
@@ -550,6 +654,18 @@ public class TemplateTest {
     compare(template, ImmutableMap.of("x", "tiddly"));
   }
 
+  @Test
+  public void macroWithCommaSeparatedArgs() {
+    String template =
+        "$x\n"
+        + "#macro (m, $x, $y)\n"
+        + "  #if ($x < $y) less #else greater #end\n"
+        + "#end\n"
+        + "#m(17 23) #m(23 17) #m(17 17)\n"
+        + "$x";
+    compare(template, ImmutableMap.of("x", "tiddly"));
+  }
+
   /**
    * Tests defining a macro inside a conditional. This proves that macros are not evaluated in the
    * main control flow, but rather are extracted at parse time. It also tests what happens if there
@@ -706,45 +822,36 @@ public class TemplateTest {
   }
 
   @Test
-  public void badBraceReference() throws IOException {
+  public void badBraceReference() {
     String template = "line 1\nline 2\nbar${foo.!}baz";
-    thrown.expect(ParseException.class);
-    thrown.expectMessage("Expected }, on line 3, at text starting: .!}baz");
-    Template.parseFrom(new StringReader(template));
+    expectParseException(template, "Expected }, on line 3, at text starting: .!}baz");
   }
 
   @Test
-  public void undefinedMacro() throws IOException {
+  public void undefinedMacro() {
     String template = "#oops()";
-    thrown.expect(ParseException.class);
-    thrown.expectMessage("#oops is neither a standard directive nor a macro that has been defined");
-    Template.parseFrom(new StringReader(template));
+    expectParseException(
+        template,
+        "#oops is neither a standard directive nor a macro that has been defined");
   }
 
   @Test
-  public void macroArgumentMismatch() throws IOException {
+  public void macroArgumentMismatch() {
     String template =
         "#macro (twoArgs $a $b) $a $b #end\n"
         + "#twoArgs(23)\n";
-    thrown.expect(ParseException.class);
-    thrown.expectMessage("Wrong number of arguments to #twoArgs: expected 2, got 1");
-    Template.parseFrom(new StringReader(template));
+    expectParseException(template, "Wrong number of arguments to #twoArgs: expected 2, got 1");
   }
 
   @Test
-  public void unclosedBlockQuote() throws IOException {
+  public void unclosedBlockQuote() {
     String template = "foo\nbar #[[\nblah\nblah";
-    thrown.expect(ParseException.class);
-    thrown.expectMessage("Unterminated #[[ - did not see matching ]]#, on line 2");
-    Template.parseFrom(new StringReader(template));
+    expectParseException(template, "Unterminated #[[ - did not see matching ]]#, on line 2");
   }
 
   @Test
-  public void unclosedBlockComment() throws IOException {
-    String template = "foo\nbar #*\nblah\nblah";
-    thrown.expect(ParseException.class);
-    thrown.expectMessage("Unterminated #* - did not see matching *#, on line 2");
-    Template.parseFrom(new StringReader(template));
+  public void unclosedBlockComment() {
+    compare("foo\nbar #*\nblah\nblah");
   }
 
   /**