8308167: SequencedMap::firstEntry throws NPE when first entry has null key or value #14278
Conversation
|
👋 Welcome back smarks! A progress list of the required criteria for merging this PR into |
|
@stuart-marks The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
There was a problem hiding this comment.
Changes look good.
An observation:
TreeMap implements SequencedMap, and I see that its firstEntry() and related methods use AbstractMap.SimpleImmutableEntry (via exportEntry()), despite it being serializable. However, this is long-standing code (from 1.6, perhaps?), and TreeMap is itself serializable. So, leaving it as is seems the right thing to do.
| * This class is not exported. Instances are created by various Map implementations | ||
| * when they need a Map.Entry that isn't connected to the Map. | ||
| * | ||
| * <p>This class differs from AbstractMap.SimpleImmutableEntry in that it is not | ||
| * serializable and that it is final. This class differs from java.util.KeyValueHolder | ||
| * in that the key and value are nullable. | ||
| * | ||
| * <p>In principle this class could be a variation on KeyValueHolder. However, | ||
| * making that class selectively support nullable keys and values is quite intricate. | ||
| * Various specifications (such as Map.ofEntries and Map.entry) specify non-nullability | ||
| * of the key and the value. Map.Entry.copyOf also requires non-null keys and values; | ||
| * but it simply passes through KeyValueHolder instances, assuming their keys and values | ||
| * are non-nullable. If a KVH with nullable keys and values were introduced, some way | ||
| * to distinguish it would be necessary. This could be done by introducing a subclass | ||
| * (requiring KVH to be made non-final) or by introducing some kind of "mode" field | ||
| * (potentially increasing the size of every KVH instance, though another field could | ||
| * probably fit into the object's padding in most JVMs.) More critically, a mode field | ||
| * would have to be checked in all the right places to get the right behavior. | ||
| * | ||
| * <p>A longer range possibility is to selectively relax the restrictions against nulls in | ||
| * Map.entry and Map.Entry.copyOf. This would also require some intricate specification | ||
| * changes and corresponding implementation changes (e.g., the implementations backing | ||
| * Map.of might still need to reject nulls, and so would Map.ofEntries) but allowing | ||
| * a Map.Entry itself to contain nulls seems beneficial in general. If this is done, | ||
| * merging KeyValueHolder and NullableKeyValueHolder should be reconsidered. |
There was a problem hiding this comment.
I think having this background and explanation in the docs for this internal class is fine.
IMO, this information would also be useful to have in the bug report.
There was a problem hiding this comment.
I'll add a note to the bug report pointing to this rationale.
|
@stuart-marks This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 90 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks for looking at this. Yeah, the serializability of various Entry objects that are exposed through the APIs is all over the map, so to speak. Note that entries obtained from the entrySet() of HashMap and TreeMap are not serializable. These are actual internal nodes of the respective Map implementations. But TreeMap uses AbstractMap.SimpleImmutableEntry in several places to disconnect the returned entry from the map, and that class is serializable. Oh well. (And yes that code dated back to the beginning of the available OpenJDK history, which puts it at JDK 1.6 or at least certainly before 7.) |
|
/integrate |
|
Going to push as commit 6d155a4.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@stuart-marks Pushed as commit 6d155a4. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Create and use new NullableKeyValueHolder class to accommodate map entries whose key or value might be null.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14278/head:pull/14278$ git checkout pull/14278Update a local copy of the PR:
$ git checkout pull/14278$ git pull https://git.openjdk.org/jdk.git pull/14278/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14278View PR using the GUI difftool:
$ git pr show -t 14278Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14278.diff
Webrev
Link to Webrev Comment