Repository: PebbleTemplates/pebble (java)

Issue:
Strange/chaotic/incorrect method resolution with overloaded method signatures
I have been noticing some strange behavior when Pebble attempts to resolve methods that are overloaded. I've added a test case [on this branch](https://github.com/cjbrooks12/pebble/blob/method-resolution-bugs/pebble/src/test/java/com/mitchellbosecke/pebble/bugs/OverloadedMethodErrorTest.java) demonstrating some of the strange behavior. 

Here's my general findings. All are based on a single method name `testMethod` in a model class, and all methods have accept a single argument. There are multiple different versions of this method where the type of the argument changes, but the method name does not.

- When both the primitive and boxed versions exist, it seems like the Boxed version is preferred. I would expect the non-boxed version to be preferred.
- When a more generic type exists, it is preferred to the specific type. This goes all the way to the base case where a method with an argument of type `Object` is preferred to all other types and all method calls go to that one method.
- When two methods both extend the same base class, and a method for the base class doesn't exist, I frequently get either a `ClassCastException` or an `IllegalArgumentException` (argument type mismatch).
	- For this situation, by removing any one method, any of them can successfully be called. Having both methods but only calling one of them also seems fine, so I think this particular issue is related to caching. For reference, I've added a few log statements in `DefaultAttributeResolver` that show the selected method accepts a different parameter than what is passed to it
- All of the above behavior seems very chaotic. Adding or removing an overloaded method from the model may make the primitive method selected over the boxed version, and may also cause a `ClassCastException` instead of an `IllegalArgumentException` as it is currently set up to do. This makes me think that the strategy for selecting a method is not deterministic when it probably should be.

Pull request description:
Fixes issues resolving overloaded methods. Closes #367.
# Changes

Searches matching methods for best possible match rather than returning the first one that works. Adds member parameter types to MemberCacheKey so overloaded methods get resolved from cache correctly, fixing issues with ClassCastException.

# Performance:

Run with [PebbleTemplates/pebble-performance-test](https://github.com/PebbleTemplates/pebble-performance-test) on my MacBook Pro, 4-core 2.8 GHz CPU, 16GB memory.

**Current 3.0**
34335.014766 (±1423.690763) ops/s

**Including these changes**
34743.663107 (±533.180988) ops/s

**Total Change**
+1.19% (-62.55%) ops/s

Patch:
diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java
index 050aca6e..76bc32aa 100644
--- a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java
+++ b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java
@@ -26,7 +26,8 @@ public class DefaultAttributeResolver implements AttributeResolver {
     if (instance != null) {
       String attributeName = String.valueOf(attributeNameValue);
 
-      Member member = this.memberCacheUtils.getMember(instance, attributeName);
+      Class<?>[] argumentTypes = this.getArgumentTypes(argumentValues);
+      Member member = this.memberCacheUtils.getMember(instance, attributeName, argumentTypes);
       if (member == null) {
         if (argumentValues == null) {
 
@@ -56,7 +57,7 @@ public class DefaultAttributeResolver implements AttributeResolver {
         }
 
         member = this.memberCacheUtils
-            .cacheMember(instance, attributeName, argumentValues, context, filename, lineNumber);
+            .cacheMember(instance, attributeName, argumentTypes, context, filename, lineNumber);
       }
 
       if (member != null) {
@@ -66,6 +67,24 @@ public class DefaultAttributeResolver implements AttributeResolver {
     return null;
   }
 
+  private Class<?>[] getArgumentTypes(Object[] argumentValues) {
+    if (argumentValues != null) {
+      Class<?>[] argumentTypes = new Class<?>[argumentValues.length];
+
+      for (int i = 0; i < argumentValues.length; i++) {
+        Object o = argumentValues[i];
+        if (o == null) {
+          argumentTypes[i] = null;
+        } else {
+          argumentTypes[i] = o.getClass();
+        }
+      }
+      return argumentTypes;
+    }
+
+    return new Class<?>[0];
+  }
+
   /**
    * Invoke the "Member" that was found via reflection.
    */
diff --git a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/MemberCacheUtils.java b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/MemberCacheUtils.java
index a65e82e6..b8d21b17 100644
--- a/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/MemberCacheUtils.java
+++ b/pebble/src/main/java/com/mitchellbosecke/pebble/attributes/MemberCacheUtils.java
@@ -7,6 +7,7 @@ import java.lang.reflect.AccessibleObject;
 import java.lang.reflect.Member;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 
@@ -15,34 +16,20 @@ class MemberCacheUtils {
   private final ConcurrentHashMap<MemberCacheKey, Member> memberCache = new ConcurrentHashMap<>(100,
       0.9f, 1);
 
-  Member getMember(Object instance, String attributeName) {
-    return this.memberCache.get(new MemberCacheKey(instance.getClass(), attributeName));
+  Member getMember(Object instance, String attributeName, Class<?>[] argumentTypes) {
+    return this.memberCache.get(new MemberCacheKey(instance.getClass(), attributeName, argumentTypes));
   }
 
   Member cacheMember(Object instance,
       String attributeName,
-      Object[] argumentValues,
+      Class<?>[] argumentTypes,
       EvaluationContextImpl context,
       String filename,
       int lineNumber) {
-    if (argumentValues == null) {
-      argumentValues = new Object[0];
-    }
-    Class<?>[] argumentTypes = new Class<?>[argumentValues.length];
-
-    for (int i = 0; i < argumentValues.length; i++) {
-      Object o = argumentValues[i];
-      if (o == null) {
-        argumentTypes[i] = null;
-      } else {
-        argumentTypes[i] = o.getClass();
-      }
-    }
-
     Member member = this.reflect(instance, attributeName, argumentTypes, filename, lineNumber,
         context.getEvaluationOptions());
     if (member != null) {
-      this.memberCache.put(new MemberCacheKey(instance.getClass(), attributeName), member);
+      this.memberCache.put(new MemberCacheKey(instance.getClass(), attributeName, argumentTypes), member);
     }
     return member;
   }
@@ -112,7 +99,9 @@ class MemberCacheUtils {
     List<Method> candidates = this.getCandidates(clazz, name, requiredTypes);
 
     // perfect match
+    Method bestMatch = null;
     for (Method candidate : candidates) {
+      // check if method is even compatible
       boolean compatibleTypes = true;
       Class<?>[] types = candidate.getParameterTypes();
       for (int i = 0; i < types.length; i++) {
@@ -122,10 +111,27 @@ class MemberCacheUtils {
         }
       }
 
+      // if it is compatible, check if it is a better match than the previous best
       if (compatibleTypes) {
-        return candidate;
+        if(bestMatch == null) {
+          bestMatch = candidate;
+        }
+        else {
+          Class<?>[] bestMatchParamTypes = bestMatch.getParameterTypes();
+          for (int i = 0; i < types.length; i++) {
+            // if the current method's param strictly extends the previous best, it is a better match
+            Class<?> widened = this.widen(bestMatchParamTypes[i]);
+            if (widened.isAssignableFrom(types[i]) && !widened.equals(types[i])) {
+              bestMatch = candidate;
+              break;
+            }
+          }
+        }
       }
     }
+    if(bestMatch != null) {
+      return bestMatch;
+    }
 
     // greedy match
     if (evaluationOptions.isGreedyMatchMethod()) {
@@ -202,10 +208,12 @@ class MemberCacheUtils {
 
     private final Class<?> clazz;
     private final String attributeName;
+    private final Class<?>[] methodParameterTypes;
 
-    private MemberCacheKey(Class<?> clazz, String attributeName) {
+    public MemberCacheKey(Class<?> clazz, String attributeName, Class<?>[] methodParameterTypes) {
       this.clazz = clazz;
       this.attributeName = attributeName;
+      this.methodParameterTypes = methodParameterTypes;
     }
 
     @Override
@@ -222,14 +230,18 @@ class MemberCacheUtils {
       if (!this.clazz.equals(that.clazz)) {
         return false;
       }
-      return this.attributeName.equals(that.attributeName);
+      if (!this.attributeName.equals(that.attributeName)) {
+        return false;
+      }
 
+      return Arrays.equals(this.methodParameterTypes, that.methodParameterTypes);
     }
 
     @Override
     public int hashCode() {
       int result = this.clazz.hashCode();
       result = 31 * result + this.attributeName.hashCode();
+      result = 31 * result + Arrays.hashCode(this.methodParameterTypes);
       return result;
     }
   }


Test patch:
diff --git a/pebble/src/test/java/com/mitchellbosecke/pebble/OverloadedMethodTest.java b/pebble/src/test/java/com/mitchellbosecke/pebble/OverloadedMethodTest.java
new file mode 100644
index 00000000..dea44958
--- /dev/null
+++ b/pebble/src/test/java/com/mitchellbosecke/pebble/OverloadedMethodTest.java
@@ -0,0 +1,295 @@
+/*******************************************************************************
+ * This file is part of Pebble.
+ * <p>
+ * Copyright (c) 2014 by Mitchell Bösecke
+ * <p>
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ ******************************************************************************/
+package com.mitchellbosecke.pebble;
+
+import static org.junit.Assert.assertEquals;
+
+import com.mitchellbosecke.pebble.error.PebbleException;
+import com.mitchellbosecke.pebble.loader.StringLoader;
+import com.mitchellbosecke.pebble.template.PebbleTemplate;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.io.Writer;
+import java.util.HashMap;
+import java.util.Map;
+import org.junit.Test;
+
+// These tests verify correct behavior when resolving overloaded methods in a model class. See issue #367
+public class OverloadedMethodTest {
+
+// Verify that an overloaded method will select the correct version of the method to call based on the input type
+//----------------------------------------------------------------------------------------------------------------------
+
+    public static class Model {
+
+        public String testMethod(String input) {
+            return "string input: " + input;
+        }
+
+        public String testMethod(Integer input) {
+            return "Integer input: " + input;
+        }
+
+        public String testMethod(Long input) {
+            return "Long input: " + input;
+        }
+
+        public String testMethod(Object input) {
+            return "other input: " + input.getClass();
+        }
+
+        public String testMethod2(String input1, String input2) {
+            return "string-string inputs";
+        }
+
+        public String testMethod2(Object input1, String input2) {
+            return "object-string inputs";
+        }
+
+        public String testMethod2(String input1, Object input2) {
+            return "string-object inputs";
+        }
+
+        public String testMethod2(Object input1, Object input2) {
+            return "object-object inputs";
+        }
+
+    }
+
+    @Test
+    public void testWithLiteralString() throws PebbleException, IOException {
+        PebbleEngine pebble = new PebbleEngine.Builder()
+                .loader(new StringLoader())
+                .strictVariables(false)
+                .build();
+
+        String input = "{{ model.testMethod(\"one\") }}";
+        String expected = "string input: one";
+
+        Map<String, Object> context = new HashMap<>();
+        context.put("model", new Model());
+
+        PebbleTemplate template = pebble.getTemplate(input);
+
+        Writer writer = new StringWriter();
+        template.evaluate(writer, context);
+        assertEquals(expected, writer.toString());
+    }
+
+    @Test
+    public void testWithContextString() throws PebbleException, IOException {
+        this.testWithModel(new Model(), "one", "string input: one");
+    }
+
+    @Test
+    public void testWithInteger() throws PebbleException, IOException {
+        this.testWithModel(new Model(), 1, "Integer input: 1");
+    }
+
+    @Test
+    public void testWithLong() throws PebbleException, IOException {
+        this.testWithModel(new Model(), 1L, "Long input: 1");
+    }
+
+    @Test
+    public void testWithObject() throws PebbleException, IOException {
+        this.testWithModel(new Model(), this, "other input: " + this.getClass().toString());
+    }
+
+    @Test
+    public void testWithStringString() throws PebbleException, IOException {
+        this.testModelWith2Inputs("", "", "string-string inputs");
+    }
+
+    @Test
+    public void testWithStringObject() throws PebbleException, IOException {
+        this.testModelWith2Inputs("", this, "string-object inputs");
+    }
+
+    @Test
+    public void testWithObjectString() throws PebbleException, IOException {
+        this.testModelWith2Inputs(this, "", "object-string inputs");
+    }
+
+    @Test
+    public void testWithObjectObject() throws PebbleException, IOException {
+        this.testModelWith2Inputs(this, this, "object-object inputs");
+    }
+
+// Verify that multiple overloaded methods can be called in the same template, and the member cache will not return the
+// wrong method and cause a ClassCastException.
+//----------------------------------------------------------------------------------------------------------------------
+
+    public static class Model2 {
+
+        public String testMethod(BaseClass input) {
+            return "BaseClass input";
+        }
+
+        public String testMethod(ChildClass1 input) {
+            return "ChildClass1 input";
+        }
+
+        public String testMethod(ChildClass2 input) {
+            return "ChildClass2 input";
+        }
+
+    }
+
+    public static class BaseClass {
+
+    }
+
+    public static class ChildClass1 {
+
+    }
+
+    public static class ChildClass2 {
+
+    }
+
+    @Test
+    public void testWithMultipleCalls() throws PebbleException, IOException {
+        PebbleEngine pebble = new PebbleEngine.Builder()
+                .loader(new StringLoader())
+                .strictVariables(false)
+                .build();
+
+        String input = "{{ model.testMethod(BaseClass) }}, {{ model.testMethod(ChildClass1) }}, {{ model.testMethod(ChildClass2) }}";
+        String expected = "BaseClass input, ChildClass1 input, ChildClass2 input";
+
+        Map<String, Object> context = new HashMap<>();
+        context.put("model", new Model2());
+        context.put("BaseClass", new BaseClass());
+        context.put("ChildClass1", new ChildClass1());
+        context.put("ChildClass2", new ChildClass2());
+
+        PebbleTemplate template = pebble.getTemplate(input);
+
+        Writer writer = new StringWriter();
+        template.evaluate(writer, context);
+        assertEquals(expected, writer.toString());
+    }
+
+// Check that method resolution works with a deep class hierarchy
+//----------------------------------------------------------------------------------------------------------------------
+
+    public static class Model3 {
+
+        public String testMethod(BaseClass input) {
+            return "BaseClass input";
+        }
+
+        public String testMethod(ChildClass1 input) {
+            return "ChildClass1 input";
+        }
+
+        public String testMethod(ChildClass2 input) {
+            return "ChildClass2 input";
+        }
+
+        public String testMethod(ChildClass3 input) {
+            return "ChildClass3 input";
+        }
+
+        public String testMethod(ChildClass4 input) {
+            return "ChildClass4 input";
+        }
+
+        public String testMethod(ChildClass5 input) {
+            return "ChildClass5 input";
+        }
+
+    }
+
+    public static class ChildClass3 extends ChildClass2 {
+
+    }
+
+    public static class ChildClass4 extends ChildClass3 {
+
+    }
+
+    public static class ChildClass5 extends ChildClass4 {
+
+    }
+
+    @Test
+    public void testWithBaseClass() throws PebbleException, IOException {
+        this.testWithModel(new Model3(), new BaseClass(), "BaseClass input");
+    }
+
+    @Test
+    public void testWithChildClass1() throws PebbleException, IOException {
+        this.testWithModel(new Model3(), new ChildClass1(), "ChildClass1 input");
+    }
+
+    @Test
+    public void testWithChildClass2() throws PebbleException, IOException {
+        this.testWithModel(new Model3(), new ChildClass2(), "ChildClass2 input");
+    }
+
+    @Test
+    public void testWithChildClass3() throws PebbleException, IOException {
+        this.testWithModel(new Model3(), new ChildClass3(), "ChildClass3 input");
+    }
+
+    @Test
+    public void testWithChildClass4() throws PebbleException, IOException {
+        this.testWithModel(new Model3(), new ChildClass4(), "ChildClass4 input");
+    }
+
+    @Test
+    public void testWithChildClass5() throws PebbleException, IOException {
+        this.testWithModel(new Model3(), new ChildClass5(), "ChildClass5 input");
+    }
+
+// test helpers
+//----------------------------------------------------------------------------------------------------------------------
+
+    private void testWithModel(Object model, Object modelInput, String expected) throws PebbleException, IOException {
+        PebbleEngine pebble = new PebbleEngine.Builder()
+                .loader(new StringLoader())
+                .strictVariables(false)
+                .build();
+
+        String input = "{{ model.testMethod(input) }}";
+
+        Map<String, Object> context = new HashMap<>();
+        context.put("model", model);
+        context.put("input", modelInput);
+
+        PebbleTemplate template = pebble.getTemplate(input);
+
+        Writer writer = new StringWriter();
+        template.evaluate(writer, context);
+        assertEquals(expected, writer.toString());
+    }
+
+    private void testModelWith2Inputs(Object modelInput1, Object modelInput2, String expected) throws PebbleException, IOException {
+        PebbleEngine pebble = new PebbleEngine.Builder()
+                .loader(new StringLoader())
+                .strictVariables(false)
+                .build();
+
+        String input = "{{ model.testMethod2(input1, input2) }}";
+
+        Map<String, Object> context = new HashMap<>();
+        context.put("model", new Model());
+        context.put("input1", modelInput1);
+        context.put("input2", modelInput2);
+
+        PebbleTemplate template = pebble.getTemplate(input);
+
+        Writer writer = new StringWriter();
+        template.evaluate(writer, context);
+        assertEquals(expected, writer.toString());
+    }
+
+}