Method lookup regression

[opeongo]

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:

method1( single ) {
   print("in method1(single)");
}

method1( String[] list ) {
   for( int i=0; i<list.length; i++)
         method1( list[i] );
}

method1("a");

When run under BeanShell-3.0.0 the following error is raised:

beanshell>java -cp target\bsh-3.0.0-SNAPSHOT.jar  bsh.Interpreter poly.bsh
Evaluation Error: bsh.EvalError: Sourced file: xxxx\poly.bsh : illegal use of undefined variable, class, or 'void' literal : at Line: 6 : in file: xxxx\poly.bsh : ;

Called from method: method1/BlockNameSpace3 : at Line: 10 : in file: xxxx\poly.bsh : method1 ( "a" )

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:

method1( String[] list ) {
   for( int i=0; i<list.length; i++)
         method1( list[i] );
}

method1( single ) {
   print("in method1(single)");
}

method1("a");
method1(new String[] {"a", "b", "c"});

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.

[opeongo]

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: method1(String[]) and method1(UNTYPED). The most specific method finding algorithm will go through the each of the methods in a loop. For my test case the method1(String[]) method was tested first. There are several levels of tests that are applied (Java base types, java box types, and bsh loose conversion). During the beanshell loose conversion test the castObject compares array base types and determines that String can be assigned to String[]. It indicates that method1(String[]) is a potential match. This must be an error, but perhaps other queries depend on this behaviour?

Next it will test if method1(UNTYPED) is a match. This will return true when testing for beanshell loose conversion. Both methods were detected as potential matches, but the algorithm could not detect which one was more specific so it just went with the first that it encountered.

After deciding that the method1(String[]) method was the most specific it called the method with a scalar in the place where it was expecting an array. The error message reflects that evaluation time problem.

When I changed the signature to method1(String) the algorithm detected a exact match and short circuited the detailed tests.

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 fix

It 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 main(String) so the array dims were ignored in the declaration. The root problem here appears to be that the method signature is not being parsed correctly, so then the castObject method was sloppy on purpose to correct for this. The fix was in the wrong place so that sometimes two wrongs made a right.

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.

[nickl-]

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.

Yes it appears the String type is closest, or maybe it was just the last available.

[nickl-]

so we have a testing problem.

Getting more code coverage has gotten hard, I was really hoping to hit 75% but no luck. Shows you what a difference it makes.

The correct fix will be to parse the array dimensions during the method declaration and get the proper signature,

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!

and tighten up the castObject function so that it only tests for exact matches in this clause.

I am wondering if the problem is not with arrayElementType too, should that not check if we are an array...wondering