RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown. TIA link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) ------------- Commit messages: - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null Changes: https://git.openjdk.java.net/jdk/pull/5226/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272347 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
+1 On 8/23/2021 2:22 PM, Vicente Romero wrote:
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
-------------
Commit messages: - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
Changes: https://git.openjdk.java.net/jdk/pull/5226/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272347 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226
Actually, it will not NPE if `names` is null and you have selected equals/hashCode as the name. Might be better to do requiresNonNull() up front for all the arguments, just to make such analysis simpler: requireNonNull(methodName); requireNonNull(type); requireNonNull(recordClass); requireNonNull(names); requireNonNull(getters); On 8/23/2021 4:04 PM, Brian Goetz wrote:
+1
On 8/23/2021 2:22 PM, Vicente Romero wrote:
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
-------------
Commit messages: - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
Changes: https://git.openjdk.java.net/jdk/pull/5226/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272347 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226
On 8/23/21 4:07 PM, Brian Goetz wrote:
Actually, it will not NPE if `names` is null and you have selected equals/hashCode as the name. Might be better to do requiresNonNull() up front for all the arguments, just to make such analysis simpler:
requireNonNull(methodName); requireNonNull(type); requireNonNull(recordClass); requireNonNull(names); requireNonNull(getters);
will do, thanks, Vicente
On 8/23/2021 4:04 PM, Brian Goetz wrote:
+1
On 8/23/2021 2:22 PM, Vicente Romero wrote:
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
-------------
Commit messages: - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
Changes: https://git.openjdk.java.net/jdk/pull/5226/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272347 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226
On Mon, 23 Aug 2021 18:08:02 GMT, Vicente Romero <vromero@openjdk.org> wrote:
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
Does the existing `ObjectMethodsTest` test verify all NPE cases? src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 327:
325: * @throws IllegalArgumentException if the bootstrap arguments are invalid 326: * or inconsistent 327: * @throws NullPointerException if any argument but {@code lookup} is {@code null}
`names` may be null if the {@code methodName} is {@code "equals"} or {@code "hashCode"}. This should be captured here. ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
On Mon, 23 Aug 2021 23:13:58 GMT, Mandy Chung <mchung@openjdk.org> wrote:
Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
addressing review comments
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 327:
325: * @throws IllegalArgumentException if the bootstrap arguments are invalid 326: * or inconsistent 327: * @throws NullPointerException if any argument but {@code lookup} is {@code null}
`names` may be null if the {@code methodName} is {@code "equals"} or {@code "hashCode"}. This should be captured here.
Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments. ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
On Tue, 24 Aug 2021 03:03:37 GMT, Vicente Romero <vromero@openjdk.org> wrote:
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 327:
325: * @throws IllegalArgumentException if the bootstrap arguments are invalid 326: * or inconsistent 327: * @throws NullPointerException if any argument but {@code lookup} is {@code null}
`names` may be null if the {@code methodName} is {@code "equals"} or {@code "hashCode"}. This should be captured here.
Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments.
I think you meant requiring `names` to be non-null but `lookup` may still be null. It's okay to change the spec to require `names` to be non-null. Probably better to mention in `@param names` that `names` is ignored when the `methodName` is `equals` or `hashCode`. ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung <mchung@openjdk.org> wrote:
Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments.
I think you meant requiring `names` to be non-null but `lookup` may still be null. It's okay to change the spec to require `names` to be non-null.
Probably better to mention in `@param names` that `names` is ignored when the `methodName` is `equals` or `hashCode`.
no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null. ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
On 8/25/21 12:08 PM, Vicente Romero wrote:
On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung <mchung@openjdk.org> wrote:
Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments. I think you meant requiring `names` to be non-null but `lookup` may still be null. It's okay to change the spec to require `names` to be non-null.
Probably better to mention in `@param names` that `names` is ignored when the `methodName` is `equals` or `hashCode`. no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null.
What I meant is to require it to be non-null but the spec should also mention `names` parameter is ignored if the method name is `equals` and `hashCode`. Mandy
-------------
On 8/25/21 4:45 PM, Mandy Chung wrote:
On 8/25/21 12:08 PM, Vicente Romero wrote:
On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung<mchung@openjdk.org> wrote:
Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments. I think you meant requiring `names` to be non-null but `lookup` may still be null. It's okay to change the spec to require `names` to be non-null.
Probably better to mention in `@param names` that `names` is ignored when the `methodName` is `equals` or `hashCode`. no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null.
What I meant is to require it to be non-null but the spec should also mention `names` parameter is ignored if the method name is `equals` and `hashCode`.
Mandy
I see, I have updated the PR, thanks for your comments
-------------
Vicente
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/b7489b41..89086ca1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=00-01 Stats: 30 lines in 2 files changed: 17 ins; 1 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
On Tue, 24 Aug 2021 03:03:48 GMT, Vicente Romero <vromero@openjdk.org> wrote:
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
addressing review comments
Marked as reviewed by chegar (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: clarifying that the names parameter is ignored in some cases ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/89086ca1..102cd1aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
On Thu, 26 Aug 2021 02:36:47 GMT, Vicente Romero <vromero@openjdk.org> wrote:
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
clarifying that the names parameter is ignored in some cases
Marked as reviewed by mchung (Reviewer). src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 345:
343: Arrays.stream(getters).forEach(Objects::requireNonNull); 344: MethodType methodType; 345: if (type instanceof MethodType)
Since you are modifying this file, do you mind taking Jesper's suggestion [1] posted in another PR to use pattern matching. Suggestion: if (type instanceof MethodType mt) methodType = mt; [1] https://github.com/openjdk/valhalla/pull/528#discussion_r688100918 ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
On Mon, 30 Aug 2021 01:45:49 GMT, Mandy Chung <mchung@openjdk.org> wrote:
Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
clarifying that the names parameter is ignored in some cases
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 345:
343: Arrays.stream(getters).forEach(Objects::requireNonNull); 344: MethodType methodType; 345: if (type instanceof MethodType)
Since you are modifying this file, do you mind taking Jesper's suggestion [1] posted in another PR to use pattern matching.
Suggestion:
if (type instanceof MethodType mt) methodType = mt;
[1] https://github.com/openjdk/valhalla/pull/528#discussion_r688100918
sure I will do this before pushing, thanks ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into JDK-8272347 - adding pattern matching - clarifying that the names parameter is ignored in some cases - addressing review comments - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/102cd1aa..e0a7f5b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=02-03 Stats: 6008 lines in 269 files changed: 4342 ins; 755 del; 911 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
On Mon, 23 Aug 2021 18:08:02 GMT, Vicente Romero <vromero@openjdk.org> wrote:
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown.
TIA
link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852)
This pull request has now been integrated. Changeset: 0609421d Author: Vicente Romero <vromero@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/0609421d4b824c5642ca75d525bad3edd72c... Stats: 32 lines in 2 files changed: 18 ins; 0 del; 14 mod 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null Reviewed-by: mchung, chegar ------------- PR: https://git.openjdk.java.net/jdk/pull/5226
participants (6)
-
Brian Goetz
-
Chris Hegarty
-
Mandy Chung
-
Mandy Chung
-
Vicente Romero
-
Vicente Romero