Improve annotations on some `java.util.concurrent` collections. #9

cpovirk · 2022-04-28T19:31:30Z

No description provided.

src/java.base/share/classes/java/util/concurrent/LinkedBlockingDeque.java

wmdietl · 2022-05-03T01:24:43Z

src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java

@@ -263,7 +265,7 @@ public int indexOf(@Nullable Object o) {
     *         {@code -1} if the element is not found.
     * @throws IndexOutOfBoundsException if the specified index is negative
     */
-    public int indexOf(E e, int index) {
+    public int indexOf(@Nullable E e, int index) {


Why is this change desirable (same for lastIndexOf below)? If the collection only accepts non-null elements, why allow checking with a null value? It won't throw an NPE, but it will always be false.
The indexOf variant that takes an Object needs to take an @Nullable Object parameter to allow both instantiations of the collection, but that doesn't apply here.

If you think they are indeed correct, having a @CFComment would be useful.

Part of my goal here is consistency, too: contains already has a @Nullable parameter. So let's make them all @Nullable or all not @Nullable.

The broader question is still a good one. As in our retainAll discussion, I've been operating under the principle of "accept the most general type that can't lead to NPE," but of course we're well aware that the principles here are controversial :) (That's not necessarily even my personal favored principle.)

I probably should have sent the poll and peek changes (which probably actually affect users) separately from the more complicated changes. It would be nice to get those in. But for the changes we've been discussing, I don't think we're affected much by what we decide to do, so we can continue discussing or just drop them, as you prefer.

contains takes an Object parameter, so that discussion should go along with the other Object vs. @Nullable Object methods.
For methods that take the type parameter, I don't see a benefit in allowing null. I agree that it wouldn't cause an NPE, but it would just allow calls that will return false. Is there a particular pattern where this would be much more convenient?

Ah, thanks, I had forgotten about the distinction of Object vs. E. I am a bit surprised that they use E here, given the precedent of List.indexOf, but of course there are good arguments for not permitting calls that will always return false (compare https://errorprone.info/bugpattern/CollectionIncompatibleType). And if their goal is to use E to forbid such calls, then it's natural to extend that argument to keeping the type as E instead of @Nullable E.

I might still lean toward @Nullable E if we were writing The Official JSpecify Stubs: The theory would be that, if we didn't, tools could choose to insert runtime null checks for code that calls indexOf on a CopyOnWriteArrayList<@NonNull String>. But my recollection is that the only tool in common use that inserts such checks, Kotlin, does not insert checks in that particular case.

But for Checker Framework users (and for all non-hypothetical use cases), I'm not aware of examples in which @Nullable would improve anyone's life.

wmdietl · 2022-05-03T01:29:59Z

src/java.base/share/classes/java/util/concurrent/LinkedBlockingDeque.java

@@ -579,7 +581,7 @@ public E peekLast() {
        }
    }

-    public boolean removeFirstOccurrence(Object o) {
+    public boolean removeFirstOccurrence(@Nullable Object o) {


Should this change (and the ones to removeLastOccurrence and remove below) go into collection-object-parameters-may-be-null.astub instead?
I see some entries there, e.g. for Deque.

My impression has been that these files haven't quite reached a consistent state.

LinkedBlockingDeque already has @Nullable on its contains method. That method's docs don't mention null explicitly, but they do say...

this deque contains at least one element e such that o.equals(e)

...which at best implies that null is an acceptable input.

The methods I'm annotating in this PR are similar. So I think it would be defensible to remove the annotation on contains, rather than add the ones in this PR. (And as you say, we could update collection-object-parameters-may-be-null.astub to have the annotations for all the methods.)

I can do that, but I'll wait for confirmation just in case. But probably nothing I've said above will come as a surprise :)

It unfortunately looks like there is some inconsistency here about which methods take @Nullable Object here or in collection-object-parameters-may-be-null.astub and I don't see a pattern.
As most of the changes in this PR are related to this, I think it would be great to merge the other changes and separately clean these methods.

wmdietl

I would suggest we remove the @Nullable E parameter and @Nullable Object parameter changes and merge the rest. Making the other signatures consistent is worth separate PRs. What do you think?

wmdietl · 2022-05-04T20:27:22Z

src/java.base/share/classes/java/util/concurrent/LinkedBlockingDeque.java

@@ -579,7 +581,7 @@ public E peekLast() {
        }
    }

-    public boolean removeFirstOccurrence(Object o) {
+    public boolean removeFirstOccurrence(@Nullable Object o) {


It unfortunately looks like there is some inconsistency here about which methods take @Nullable Object here or in collection-object-parameters-may-be-null.astub and I don't see a pattern.
As most of the changes in this PR are related to this, I think it would be great to merge the other changes and separately clean these methods.

wmdietl · 2022-05-04T20:31:09Z

src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java

@@ -263,7 +265,7 @@ public int indexOf(@Nullable Object o) {
     *         {@code -1} if the element is not found.
     * @throws IndexOutOfBoundsException if the specified index is negative
     */
-    public int indexOf(E e, int index) {
+    public int indexOf(@Nullable E e, int index) {


contains takes an Object parameter, so that discussion should go along with the other Object vs. @Nullable Object methods.
For methods that take the type parameter, I don't see a benefit in allowing null. I agree that it wouldn't cause an NPE, but it would just allow calls that will return false. Is there a particular pattern where this would be much more convenient?

Split off from eisop#9, which also contains some changes whose usefulness is unclear.

cpovirk · 2022-05-04T21:29:16Z

I would suggest we remove the @Nullable E parameter and @Nullable Object parameter changes and merge the rest. Making the other signatures consistent is worth separate PRs. What do you think?

I created #16 with the changes that I think are uncontroversial. Once we get what we can merged there, I can merge that into this PR's branch, and we can figure out if/when we want to resume talking annotation philosophy :)

wmdietl · 2022-05-04T23:05:58Z

I created #16 with the changes that I think are uncontroversial. Once we get what we can merged there, I can merge that into this PR's branch, and we can figure out if/when we want to resume talking annotation philosophy :)

Thanks! I've merged #16 and pulled master back in here. As noted there, the removeAll/retainAll changes we discussed https://github.com/eisop/jdk/pull/9/files#r862552217 could also be merged and further minimize the leftovers from this PR.

cpovirk · 2022-05-05T18:19:55Z

Thanks. I might let this (including removeAll/retainAll) drop until I feel like I can spend some more time on forming some opinions, just given that I have some other things I've been neglecting. Thanks for looking at the flurry of PRs. Hopefully I'll end up with another idea or two soon :)

wmdietl · 2022-05-05T19:11:08Z

Thanks. I might let this (including removeAll/retainAll) drop until I feel like I can spend some more time on forming some opinions, just given that I have some other things I've been neglecting. Thanks for looking at the flurry of PRs. Hopefully I'll end up with another idea or two soon :)

Sounds good. Thanks for all the PRs and discussion!

(eisop#9)

Improve annotations on some java.util.concurrent collections.

e13d517

wmdietl reviewed May 2, 2022

View reviewed changes

src/java.base/share/classes/java/util/concurrent/LinkedBlockingDeque.java Show resolved Hide resolved

wmdietl mentioned this pull request May 3, 2022

Overriding methods restricting parameters eisop/checker-framework#232

Open

wmdietl reviewed May 3, 2022

View reviewed changes

wmdietl reviewed May 4, 2022

View reviewed changes

wmdietl assigned cpovirk May 4, 2022

cpovirk added a commit to cpovirk/jdk that referenced this pull request May 4, 2022

Annotate some java.util.concurrent collections.

5f1a06f

Split off from eisop#9, which also contains some changes whose usefulness is unclear.

cpovirk mentioned this pull request May 4, 2022

Annotate some java.util.concurrent collections. #16

Merged

Merge branch 'master' into jucc

4ae27b5

cpovirk added a commit to cpovirk/jdk that referenced this pull request Sep 7, 2023

Annotate java.util.logging.Hander.

c375331

(eisop#9)

Ao-senXiong pushed a commit to Ao-senXiong/jdk that referenced this pull request Apr 21, 2024

Update to EISOP 3.42-eisop1 from 3.41-eisop1 (eisop#9)

c160ca7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve annotations on some `java.util.concurrent` collections. #9

Improve annotations on some `java.util.concurrent` collections. #9

cpovirk commented Apr 28, 2022

wmdietl May 3, 2022

cpovirk May 3, 2022

wmdietl May 4, 2022

cpovirk May 4, 2022

wmdietl May 3, 2022

cpovirk May 3, 2022

wmdietl May 4, 2022

wmdietl left a comment

wmdietl May 4, 2022

wmdietl May 4, 2022

cpovirk commented May 4, 2022

wmdietl commented May 4, 2022

cpovirk commented May 5, 2022

wmdietl commented May 5, 2022

Improve annotations on some java.util.concurrent collections. #9

Are you sure you want to change the base?

Improve annotations on some java.util.concurrent collections. #9

Conversation

cpovirk commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpovirk commented May 4, 2022

wmdietl commented May 4, 2022

cpovirk commented May 5, 2022

wmdietl commented May 5, 2022

Improve annotations on some `java.util.concurrent` collections. #9

Improve annotations on some `java.util.concurrent` collections. #9