Bug 65358 - Possible EL Bug — Method Matching with Varargs
Summary: Possible EL Bug — Method Matching with Varargs
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: EL (show other bugs)
Version: 9.0.x
Hardware: PC Mac OS X 10.1
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-05 01:57 UTC by Volodymyr Siedlecki
Modified: 2021-06-07 13:53 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Volodymyr Siedlecki 2021-06-05 01:57:54 UTC
For the following EL expression below, which method should be matched?  

   #{testBean.getMessage(testBean.value1)}.  //  (value1 is a String) 

1) 
    public String getMessage(final EnumInterface enumInstance) {
        return “getMessage(final EnumInterface enumInstance)”;
    }

2) 
    public String getMessage(final String messageKey, final Serializable… arguments) {
        return “getMessage(final String messageKey, final Serializable… arguments)”;
    }

Note that TestBean is of type EnumInterface. I’ve provided a reproducible app with the source on Github. (1)

Prior to the EL varargs patch (2), only the first method was matched. After the patch, the second method is then matched instead. It seems that first method is more appropriate rather than the second?

Prior to 9.0.3:
The first method is an assignableMatch. 
The second method (during the wrapper loop) is skipped since the parameter number check (line 240) evaluated to true.
The first method is therefore the only option available. 

Current Behavior: 
The new varargs checks do not skip the second method this time around. 
Both methods are an assignableMatch, but the second is also a coercibleMatch. 
Therefore, the code selects the second as a better match, from what I gathered? 

I’m a bit stumped. This seems to be a valid scenario, but, then again, maybe it’s not a bug? I would appreciate any feedback. 

Thank you. 


1) https://github.com/volosied/VarargsELBug

2) https://github.com/apache/tomcat/commit/b7ce5679b9e6a073dadbc31e6ecde12ad1e0ede8#diff-f5b7c13f66b3d070a6b1c1713ad4b60fd70b26f579a746edc8fe9f19109243b2L240-L244   -- Added in 9.0.3.
Comment 1 Mark Thomas 2021-06-05 19:40:09 UTC
The EL spec is silent on how methods should be selected when there isn't an exact match between arguments and method parameters.

Generally, the rule Tomcat tries to follow is to pick the same method as the Java compiler would.

In this instance if you call:
testBean.getMessage(testBean.getValue1())

then the Java compiler will call:
getMessage(final EnumInterface enumInstance)

which is what I would expect. I would also expect EL to behave the same way. It doesn't.

I need to dig into what is going on with EL. On the surface it looks like a bug but it needs some further investigation to determine what is going on and why.
Comment 2 Mark Thomas 2021-06-05 20:18:54 UTC
Yes, this is definitely an EL bug. The root cause is the weight given in the method mapping to a potential method match where the potential matching method uses varArgs but the EL expression doesn't specify any varArgs. Currently this is treated as an assignable match for the varargs parameter which is too strong a match.

I need to write some test cases to explore how the Java compiler behaves around this particular edge case. With that information in hand, I'll know how big a change is required to javax.el.Util.findWrapper(). I have a one line fix that addresses the specific issue described here (and doesn't cause any of the existing tests to fail) but I want to make sure there aren't any other related edge cases that also need to be addressed.
Comment 3 Volodymyr Siedlecki 2021-06-05 22:14:53 UTC
Thanks for looking into it, Mark!

Just realized I made two mistakes in the original post.  I should have said
- value1 is of type TestEnum.
- TestEnum of type EnumInterface 

Glad I linked the correct example, at least.
Comment 4 Mark Thomas 2021-06-07 13:53:01 UTC
The one-line fix wasn't sufficient. A broader fix was required that in essence looked at the number of varargs used and preferred the method that matched using fewest varargs.

Fixed in:
- 10.1.x for 10.1.0-M1 onwards
- 10.0.x for 10.0.7 onwards
- 9.0.x for 9.0.47 onwards
- 8.5.x for 8.5.67 onwards