Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AtomicReference.compareAndSet: (V?, V%) or (V%, V%)? #401

Open
cpovirk opened this issue Jul 26, 2023 · 4 comments
Open

AtomicReference.compareAndSet: (V?, V%) or (V%, V%)? #401

cpovirk opened this issue Jul 26, 2023 · 4 comments
Labels
library-use-case Question of how a particular library should be annotated; could potentially affect design or docs nullness For issues specific to nullness analysis.

Comments

@cpovirk
Copy link
Collaborator

cpovirk commented Jul 26, 2023

We already decided on and wrote about AtomicReference in general, with emphasis on its no-arg constructor, settling on:

@NullMarked
class AtomicReference<V extends @Nullable Object> {
  AtomicReference(V initialValue) {...}
  AtomicReference() {...} // the value remains `null`!

  V get() {...}
  void set(V newValue) {...}
}

My assumption had been that we would annotate compareAndSet as:

  boolean compareAndSet(V expect, V update) {...}

But @chaoren has proposed an alternative:

  boolean compareAndSet(@Nullable V expect, V update) {...}

That approach makes it easy to use an AtomicReference for cases in which you want to set a value exactly once, as in AtomicReference<Instant> firstWriteTime: You can pass null for the expect value, but you can never pass it for the update value, and so you can transition from null to non-null but not the reverse. (This approach would be combined with "blessing" use of the no-arg constructor and/or making the parameter of the one-arg constructor @Nullable V initialValue.)

I would prefer to force AtomicReference<@Nullable Instant> in that case. My main reason is that I want for calls to get() to have to grapple with the possibility of a null return, since apparently not only is there a period of time during which the value is null but also we expect to "observe" that null by "reading" it as part of a CAS operation. However, I acknowledge that I've taken a harder line against reading nulls than others (in, e.g., #283, #225). So others are likely to feel less pull toward AtomicReference<@Nullable Instant> than I do. (And, of course, unless a tool actually bans use of the no-arg constructor with a non-nullable type, we can't truly force AtomicReference<@Nullable Instant>, anyway.)

@chaoren
Copy link

chaoren commented Jul 26, 2023

Cf. Kotlin's lateinit, which allows you to declare a non-null variable to be initialized later. All reads of the variable are assumed to only happen after it is initialized, so there's no need to check for null. An AtomicReference that could be initialized after its declaration could serve a similar purpose.

Also, even if we ignore the case of a no-arg constructed AtomicReference, having expect be nullable would still be very convenient if you just want to compare the AtomicReference against some variable that happens to be nullable. If you just have some non-nullable variable instead of an AtomicReference, you wouldn't require a null check before being able to compare it against some nullable variable.

This is perfectly fine:

Object foo;
@Nullable Object bar;
Object baz;
...
if (foo == bar) {
  foo = baz;
}

so why should this require a null check on bar?

AtomicReference<Object> foo;
@Nullable Object bar;
Object baz;
...
if (bar != null) {
  foo.compareAndSet(bar, baz);
}

@chaoren
Copy link

chaoren commented Jul 26, 2023

I think in general, requiring that you set and get values with the correct nullness makes perfect sense, but it doesn't make sense to require that you always compare against values with the same nullness since it's always possible to do a == null check. If the API actually throws an NPE when trying to compare against a null, then that's a different discussion.

@cpovirk
Copy link
Collaborator Author

cpovirk commented Nov 28, 2023

I'm still not getting back to this, but I happened to learn that Kotlin has its own AtomicReference. (It uses (V%, V%).)

@cpovirk
Copy link
Collaborator Author

cpovirk commented Dec 6, 2023

Still still not getting back to this in earnest quite yet, but:

Java has already somewhat taken the position that compareAndSet should have a restrictive type for the value to be compared against: It could have chosen the signature boolean compareAndSet(Object expect, V update), but it went for (V, V) instead.

But that comparison is more than a little unfair: It's easy to imagine that someone might accidentally pass, say, a String instead of a UserId for the expect parameter, and Java's current signature can catch that without introducing many practical problems (if any). But if someone is passing null, that person very likely (though not absolutely necessarily; maybe the person should have passed Foo.UNKNOWN or something) intends to pass null.

And anyway, even if we thought that Java got this "wrong," we don't have to follow suit :) My argument here is basically just that a signature of (V?, V%) falls short of being "conceptually pure": The "conceptually pure" version is (IMO) either the very permissive one ((Any?, V%)) or the very restrictive one ((V%, V%)). I can still see plenty of room for disagreement over what's more useful in practice.

@kevinb9n kevinb9n added the design An issue that is resolved by making a decision, about whether and how something should work. label Jan 31, 2024
@kevinb9n kevinb9n added nullness For issues specific to nullness analysis. library-use-case Question of how a particular library should be annotated; could potentially affect design or docs and removed design An issue that is resolved by making a decision, about whether and how something should work. labels Mar 13, 2024
@kevinb9n kevinb9n added this to the 1.0 milestone Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library-use-case Question of how a particular library should be annotated; could potentially affect design or docs nullness For issues specific to nullness analysis.
Projects
None yet
Development

No branches or pull requests

4 participants