Strange/chaotic/incorrect method resolution with overloaded method signatures

[cjbrooks12]

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 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.

[ebussieres]

The problem seems to be in method findMethod of class com.mitchellbosecke.pebble.attributes.MemberCacheUtils.

For your first point, every primitive type is converted to Boxed version. So this is the current behaviour for that.

For your second point, it uses isAssignableFrom to determine if current method can be used. In your use case, there are 6 candidates methods. Depending on the order of retrieved methods, it can have different behaviour. For example, if method with object parameter is the first one and since object is assignable for any parameter, it's gonna be chosen first.

Maybe we can add a strict mode to use equals insteadof isAssignableFrom. But keep in mind that reflection is very slow and affect performance a lot. So care must be taken here.

[cjbrooks12]

Yeah, this portion of the engine is crucial for performance, so I agree that we shouldn't change it significantly.

I think including the method parameters array (or empty/null array for fields) in the MemberCacheKey is necessary to prevent the exceptions. And for the "perfect match" method candidate, I think we could still get away with using isAssignableFrom if we don't break out of the loop on the first matching method, but continue the loop and see if there is a better match. As this gets cached, hopefully it wouldn't affect performance too much.

[ebussieres]

Will you work on a pull request for this issue ?

[cjbrooks12]

Sure thing