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-08-06 16:37 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
Comment 5 Volodymyr Siedlecki 2021-07-13 16:34:54 UTC
I was looking at this again (on 9.0.50).

I think there's still a bug here? Using the code below: 

Bean bean  = new Bean();

// JVM 
bean.bark(bean.getDog()); // -> bark(Animal animal)

// EL
${ bean.bark(bean.dog) } // - > bark(Dog dog, String... text)

_______

public interface Animal {

}

public class Dog implements Animal {
    
}

public class Bean {

    public String bark(Animal animal){
        return "bark(Animal animal)";
    }

    public String bark(Dog dog, String... text){
        return "bark(Dog dog, String... text)";
    }

    public Animal getDog(){
        System.out.println("Returning Dog of type Animal");
        return new Dog();
    }

}
Comment 6 Mark Thomas 2021-07-13 18:40:00 UTC
Yes, that looks like a bug to me. The JLS states (in rather more precise language) that a method match that doesn't use varargs beats one that does.

I'll work on a fix.
Comment 7 Mark Thomas 2021-07-16 14:54:45 UTC
Fixed in:
- 10.1.x for 10.1.0-M3 onwards
- 10.0.x for 10.0.9 onwards
- 9.0.x for 9.0.51 onwards
- 8.5.x for 8.5.70 onwards
Comment 8 Volodymyr Siedlecki 2021-07-29 16:30:31 UTC
Hey Mark,

I hate to keep following up on this, but I think it's still not quite right.

Please look at these two test cases I added: 

 https://github.com/volosied/tomcat/commit/d6623d814ea30674fa27bda7dcbff3b1c268ba0b


However, this patch I created seems to resolve both scenarios without another other failures.

https://github.com/volosied/tomcat/commit/d876f1e57a265970e0653c2a906b66f18d91784a
Comment 9 Mark Thomas 2021-07-29 18:26:22 UTC
Thanks for the test cases. They demonstrate very well the ambiguity that exists in the current EL 4.0 spec.

The question is, essentially, one of priority of matches. If we look at the Java language to start with, if a set of parameters could match multiple methods then the "best" match is chosen based on these checks in this order (the comparison ends as soon as a rule determines a method is better):
- a non-varargs method is better than a var-args method
- methods that match more exact parameter methods are better
- methods that match more assignable parameters are better
- methods that match fewer parameters using varargs are better

What complicates things for EL is that it adds type coercion.

The tests in the attached patch assume that type coercion is lower priority than "a non-varargs method beats a var-args method". This is not how the code was designed. The code was designed to implement:
- a non-varargs method is better than a var-args method
- methods that match more exact parameter methods are better
- methods that match more assignable parameters are better
- methods that match more coercible parameters are better
- methods that match fewer parameters using varargs are better

The next release of the EL spec will contain a clarification to this effect. Given this bug report, I'll probably add to that text to make clear that non-varargs still beats varargs even when coercion is used.

Therefore, the matches observed in the tests are as expected because coercing an Integer to String is preferred to matching an Integer varargs.
Comment 10 Volodymyr Siedlecki 2021-08-06 16:37:33 UTC
Thank for the detailed explanation, Mark!

I'll leave this issue alone then.