-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Method lookup regression #731
Comments
I traced through the code to see where the problem is and the mixup is happening in Types.castObject line 559. This problem was introduced to the code base 5 years ago, so we have a testing problem. The castObject function is called by the code that is determining the most specific function to match to the arguments at the calling site. There are two potential methods: Next it will test if After deciding that the When I changed the signature to When I changed the declaration order the comparison order changed, and it turned out that the first one encountered was the correct selection and was evaluated correctly. Potential fixIt seems like the problem is in the test in Types.castObject // assignment to loose type, void type, or exactly same type
if ( toType == null || arrayElementType(toType) == arrayElementType(fromType) )
return checkOnly ? VALID_CAST :
fromValue; This is not test for an exact match, but some kind of loose match where if either element is an array type then the base will be used. I tried changing this to only do an exact match if ( toType == null || toType == fromType )
return checkOnly ? VALID_CAST :
fromValue; This seems to work with most of the tests except only a handful. The ones that are failing are like main(String args[]) {
assertArrayEquals({"foo", "bar", "baz"}, args);
}
main({"foo", "bar", "baz"}); Notice that the formal parameter declaration is using that horrible old syntax where the array dimensions are on the variable name and not on the type. In this case the method ends up getting a signature of The correct fix will be to parse the array dimensions during the method declaration and get the proper signature, and tighten up the castObject function so that it only tests for exact matches in this clause. |
…a partial fix because each change uncovered another subtle issue. Still work to be done.
Yes it appears the |
Getting more code coverage has gotten hard, I was really hoping to hit 75% but no luck. Shows you what a difference it makes.
That sounds about right, we had to make so many workarounds for that method of array declaration it makes sense that we missed something. Well spotted!! Missed your thorough testing over the holidays, glad you're back!
I am wondering if the problem is not with |
…a partial fix because each change uncovered another subtle issue. Still work to be done.
Here is an example of a regression of a method lookup that previously worked with BeanShell 2.0b5 and no longer works in BeanShell 3.0.
This following code works in BeanShell-2.0b5, but fails in BeanShell-3.0.0:
When run under BeanShell-3.0.0 the following error is raised:
There is a simple workaround to this issue which is to change the first method declaration to
method1(String single) {
.Also, if the single string version of
method1
is declared after the array version this will also work. This works correctly:I don't consider this to be a significant problem for me because I always use formal parameters with declared types. I am bringing this up as an issue because it may relate to other lookup problems that have been showing up lately. Also, it is odd to me that the error is occurring at line 6 which should not be on the execution path. It appears as though the wrong code block is being executed.
The text was updated successfully, but these errors were encountered: