RFR: 8259922 MethodHandles.collectArguments does not follow its documentation

[Mandy Chung]

On Wed, 20 Jan 2021 18:29:00 GMT, Johannes Kuhn <github.com+652983+DasBrain at openjdk.org> wrote:

> Add explicit range check to `MethodHandles.collectArgumentsCheck`.  
> Added test case that exercises all cases.
> 
> This is a behavioral change, from throwing an unspecified exception to throwing an IllegalArgumentException, as specified.  
> No spec change needed, as the IllegalArgumentException is already specified to be thrown in those cases.
> 
> Feel free to suggest a better place for the tests.

Thanks for fixing this.  I can sponsor this for you.

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5753:

> 5751:         if (pos < 0 || pos > targetType.parameterCount() ||
> 5752:                 // a filter with void return is fine at pos == targetType.parameterCount();
> 5753:                 (rtype != void.class && pos >= targetType.parameterCount())) {

Nit: adding `rtype == void.class` may make it clearer than the comment as that matches what `@throws` describes

Suggestion:

        if (pos < 0 || (rtype == void.class && pos > targetType.parameterCount()) ||
                             (rtype != void.class && pos >= targetType.parameterCount())) {

test/jdk/java/lang/invoke/8259922/TestMethodHandlesCollectArgs.java line 37:

> 35: import static org.testng.Assert.*;
> 36: 
> 37: public class TestMethodHandlesCollectArgs {

I suggest to rename this test in `test/jdk/java/lang/invoke/MethodHandlesCollectArgsTest.java` matching existing convention and `CollectArgsTest.java` is also fine with me.  The bug ID is already in @bug and I find the directory with bug ID adds noise.

test/jdk/java/lang/invoke/8259922/TestMethodHandlesCollectArgs.java line 51:

> 49:             // expected - pass
> 50:         }
> 51:     }

This can be simplified:

@Test(expected = IllegalArgumentException.class)
public void nonVoidIllegalPos() {
    MethodHandles.collectArguments(TARGET, 2, FILTER_INT);
}

Same applies to other negative test cases.   I suggest to put all negative test cases in a data provider and test them in one single `@Test` like this:
`@Test(dataProvider="illegalPos", expected = IllegalArgumentException.class)`

with the index and filter as the method argument.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2171