RFR: 8259922 MethodHandles.collectArguments does not follow its documentation
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. ------------- Commit messages: - Fix tailing whitespace in in MethodHandles.java - Fix copyright yeahr in MethodHandles.java - Fix JDK-8259922 - Fix JDK-8259922. Changes: https://git.openjdk.java.net/jdk/pull/2171/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2171&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259922 Stats: 100 lines in 2 files changed: 99 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2171.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2171/head:pull/2171 PR: https://git.openjdk.java.net/jdk/pull/2171
On Wed, 20 Jan 2021 18:29:00 GMT, Johannes Kuhn <github.com+652983+DasBrain@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
On Thu, 21 Jan 2021 22:54:56 GMT, Mandy Chung <mchung@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.
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.
Yeah, still learning where to put tests. ------------- PR: https://git.openjdk.java.net/jdk/pull/2171
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.
Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Implement suggestions by Mandy Chung. ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2171/files - new: https://git.openjdk.java.net/jdk/pull/2171/files/4f74e2df..f01fefaa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2171&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2171&range=00-01 Stats: 179 lines in 3 files changed: 82 ins; 95 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2171.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2171/head:pull/2171 PR: https://git.openjdk.java.net/jdk/pull/2171
On Fri, 22 Jan 2021 00:33:16 GMT, Johannes Kuhn <github.com+652983+DasBrain@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.
Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision:
Implement suggestions by Mandy Chung.
Looks good. What tests have you ran? ------------- Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2171
On Fri, 22 Jan 2021 00:44:12 GMT, Mandy Chung <mchung@openjdk.org> wrote:
Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision:
Implement suggestions by Mandy Chung.
Looks good. What tests have you ran?
On the latest commit, only my own test. On the previous commit (4f74e2d) I did run the full tier1. Currently a full test tier1 run on my machine and github actions is in progress. ------------- PR: https://git.openjdk.java.net/jdk/pull/2171
On Fri, 22 Jan 2021 00:48:51 GMT, Johannes Kuhn <github.com+652983+DasBrain@openjdk.org> wrote:
Looks good. What tests have you ran?
On the latest commit, only my own test. On the previous commit (4f74e2d) I did run the full tier1.
Currently a full test tier1 run on my machine and github actions is in progress.
Tests did run successfully on my machine (win 64) and github actions. Let me know if there should be additional tests that I should run. ------------- PR: https://git.openjdk.java.net/jdk/pull/2171
On Wed, 20 Jan 2021 18:29:00 GMT, Johannes Kuhn <github.com+652983+DasBrain@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.
This pull request has now been integrated. Changeset: bf5e8015 Author: Johannes Kuhn <info@j-kuhn.de> Committer: Mandy Chung <mchung@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/bf5e8015 Stats: 86 lines in 2 files changed: 86 ins; 0 del; 0 mod 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range Reviewed-by: mchung ------------- PR: https://git.openjdk.java.net/jdk/pull/2171
participants (2)
-
Johannes Kuhn
-
Mandy Chung