batch of javadoc revisions for clarity and completeness #671

kevinb9n · 2024-09-26T00:47:39Z

I honestly don't remember everything I did here or why :-) so let's just go ahead and discuss how to make it best from here!

google-cla · 2024-09-26T00:47:44Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

netdpb

Thanks, this is good editing!

netdpb · 2024-09-27T15:41:09Z

src/main/java/org/jspecify/annotations/NullMarked.java

- * exclude {@code null} as a value) unless explicitly annotated as {@link Nullable}. However, there
- * are several special cases to address.
+ * <p>Within null-marked code, a type usage is generally considered to be non-null unless explicitly
+ * annotated as {@link Nullable}. However, there are a few special cases to address.


They're not really special cases, are they? They're more like exceptions to the general rule. Can we call them that?

I think "special case" connotes bad complexity, and I don't think that's what we have.

I don't think these all quite fit as being "exceptions". They each seem to have a different kind of flavor, to me. I've left it as is for now pending any new inspirations.

netdpb · 2024-09-27T15:43:48Z

src/main/java/org/jspecify/annotations/NullMarked.java

- *       arguments, then the wildcards seen in {@code List<?>} and {@code List<? super String>} must
- *       include {@code null}, because they have no "upper bound". (<a
- *       href="https://bit.ly/3ppb8ZC">Why?</a>)
+ *   <li>In a <b>wildcard</b> type such as {@code List<?>} or {@code List<? super Integer>}, where


Should you call out the fact that this applies only to wildcards without an explicit upper bound? You don't mean to imply that the wildcard type in List<? extends Number> is nullable.

The previous text did call that out, but not until the very end.

Thanks I think I've made this part much better now.

netdpb · 2024-09-27T15:46:52Z

src/main/java/org/jspecify/annotations/NullMarked.java

- *   <li>Applying this annotation to an instance <b>method</b> of a <i>generic</i> class is
- *       acceptable, but is not recommended because it can lead to some confusing situations.
+ *   <li>To apply these annotations to an entire (single) <b>package</b>, create a <a
+ *       href="https://docs.oracle.com/javase/specs/jls/se19/html/jls-7.html#jls-7.4.1">{@code


Why down to 19?

src/main/java/org/jspecify/annotations/NullMarked.java

netdpb · 2024-09-27T15:48:10Z

src/main/java/org/jspecify/annotations/NullMarked.java

+ *   <li>To apply these annotations to an entire (single) <b>package</b>, create a <a
+ *       href="https://docs.oracle.com/javase/specs/jls/se19/html/jls-7.html#jls-7.4.1">{@code
+ *       package-info.java}</a> file there. This annotation has no effect on "subpackages".
+ *       <b>Warning</b>: if the package does not belong to a module, be very careful: it can easily


Is it odd that this Warning comes in the middle of a paragraph?

I think it's okay...

netdpb · 2024-09-27T16:15:38Z

src/main/java/org/jspecify/annotations/Nullable.java

+ *       is nullable either way. Using {@code @Nullable E} in this way is called <i>nullable
+ *       projection</i> (the more rarely needed <a href="NonNull.html#projection">non-null
+ *       projection</a> is also supported).
+ *   <li>On an <b>array type</b>: The nullness of the array and of its components can both be


Suggested change

* <li>On an array type: The nullness of the array and of its components can both be

* <li>On an array type: The nullness of the array and of its components can each be

netdpb · 2024-09-27T16:20:18Z

src/main/java/org/jspecify/annotations/Nullable.java

+ *       of non-null strings. Note that arrays are treated as covariant: {@code String[]} (array of
+ *       non-null strings) is assignable to {@code @Nullable String[]} (array of nullable strings).


This appears to be the only place in the Javadoc where we mention subtyping at all, let alone covariance. Does it belong here?

My first reaction is that this suggests we should be talking about it more.

We should, but in the Javadoc?

netdpb · 2024-09-27T16:20:57Z

src/main/java/org/jspecify/annotations/Nullable.java

 *   <li>On a <b>nested type</b>: In most examples above, in place of {@code String} we might use a
 *       nested type such as {@code Map.Entry}. The Java syntax for annotating such a type as
 *       nullable looks like {@code Map.@Nullable Entry}.
+ *   <li>On a parameter declaration of variable arity ("varargs"): Use {@code @Nullable String...}
+ *       to indicate that the individual strings may be null.


Please add the other example (String @Nullable ... strings) as well.

netdpb · 2024-09-27T16:21:23Z

src/main/java/org/jspecify/annotations/Nullable.java

 * </ul>
 *
- * <h2 id="applicability">Where it is applicable</h2>
+ * <!-- TODO: move up -->


Up to where?

I was considering switching the order of these entire two giant sections. Thoughts?

Meaning vs applicability? I think meaning is more important and should come first.

netdpb · 2024-09-27T16:23:25Z

src/main/java/org/jspecify/annotations/Nullable.java

 *
 * <p>This annotation and {@link NonNull} are applicable to any <a
 * href="https://github.com/jspecify/jspecify/wiki/type-usages">type usage</a> <b>except</b> the
 * following cases, where they have no defined meaning:
 *
 * <ul>
+ *   <li>On a <b>local variable</b> declaration. According to JSpecicy, the nullness of a local


Do we really need "according to JSpecify" here? I feel like that's implied, or else we'd have to sprinkle it all over the place.

I reworded, but it feels necessary to say that this is a JSpecify-specific stance, particularly since the Java language is probably not going to follow suit here.

netdpb · 2024-09-27T18:26:10Z

src/main/java/org/jspecify/annotations/NullMarked.java

+ *       local variable declaration, is unaffected.
+ *   <li>A <b>wildcard</b> with no upper bound generally represents a nullable type (unless the
+ *       corresponding type parameter has a non-null upper bound itself). (<a
+ *       href="https://bit.ly/3ppb8ZC">Why?</a>) This refers to either an unbounded wildcard like in


Suggested change

* href="https://bit.ly/3ppb8ZC">Why?</a>) This refers to either an unbounded wildcard like in

* href="https://bit.ly/3ppb8ZC">Why?</a>) This can be either an unbounded wildcard like in

A little less likely to confuse people about references.

netdpb · 2024-09-27T18:26:53Z

src/main/java/org/jspecify/annotations/NullMarked.java

+ *       href="https://bit.ly/3ppb8ZC">Why?</a>) This refers to either an unbounded wildcard like in
+ *       {@code List<?>}, or a wildcard with a lower bound like in {@code List<? super Integer>}). 


Suggested change

* href="https://bit.ly/3ppb8ZC">Why?</a>) This refers to either an unbounded wildcard like in

* {@code List<?>}, or a wildcard with a lower bound like in {@code List<? super Integer>}).

* href="https://bit.ly/3ppb8ZC">Why?</a>) This refers to either an unbounded wildcard as in

* {@code List<?>}, or a wildcard with a lower bound as in {@code List<? super Integer>}).

For some reason, changing to as made me feel that commas were newly required, but I did it.

netdpb · 2024-09-27T18:27:49Z

src/main/java/org/jspecify/annotations/NullMarked.java

+ *       upper bound is given explicitly, then {@code Object} is filled in by the compiler. This
+ *       means the example {@code class MyList<E>} is interpreted identically to {@code class
+ *       MyList<E extends Object>}, making the upper bound <em>non-null</em> {@code Object}. (<a
+ *       href="https://bit.ly/3ppb8ZC">Why?</a>)
 *   <li>When a type variable has a nullable upper bound, such as the {@code E} in {@code class


Suggested change

* <li>When a type variable has a nullable upper bound, such as the {@code E} in {@code class

* <li>When a type parameter has a nullable upper bound, such as the {@code E} in {@code class

netdpb · 2024-09-27T18:28:07Z

src/main/java/org/jspecify/annotations/NullMarked.java

- *       <b>parametric nullness</b>. In order to support both nullable and non-null type arguments
- *       safely, the {@code E} type itself must be handled <i>strictly</i>: as if nullable when
- *       "read from", but as if non-null when "written to".
+ *       Foo<E extends @Nullable Bar>}), an unannotated usage of this type variable within that


Suggested change

* Foo<E extends @Nullable Bar>}), an unannotated usage of this type variable within that

* Foo<E extends @Nullable Bar>}), an unannotated usage of the associated type variable within that

I fear this may confuse people but I've reworded slightly

netdpb · 2024-10-10T15:35:10Z

src/main/java/org/jspecify/annotations/Nullable.java

- *       the <i>root type</i> of the variable; other type components (type arguments, array
- *       component types) are annotatable as usual.
+ *   <li>On a <b>local variable</b> declaration. JSpecify treats the nullness of a local variable as
+ *       being a purely dynamic property, not being a fixed declarative property of its type. (<a


Suggested change

* being a purely dynamic property, not being a fixed declarative property of its type. (<a

* being a purely dynamic property, not a fixed declarative property of its type. (<a

Also, maybe "declared" is simpler than "declarative"? And maybe "static" is better than "fixed", to contrast with "dynamic"?

netdpb · 2024-10-10T15:37:19Z

src/main/java/org/jspecify/annotations/NullMarked.java

@@ -33,49 +33,39 @@
 * specified otherwise. Using this annotation avoids the need to write {@link NonNull @NonNull} many
 * times throughout your code.
 *
+ * <p>The effects of this annotation can be negated at a narrower scope using {@link NullUnmarked}.


Suggested change

* The effects of this annotation can be negated at a narrower scope using {@link NullUnmarked}.

* The effects of this annotation can be negated at a narrower scope using {@link NullUnmarked @NullUnmarked}.

netdpb · 2024-10-10T15:37:42Z

src/main/java/org/jspecify/annotations/NullMarked.java

- * exclude {@code null} as a value) unless explicitly annotated as {@link Nullable}. However, there
- * are several special cases to address.
+ * <p>Within null-marked code, a type usage is generally considered to be non-null unless explicitly
+ * annotated as {@link Nullable}. However, there are a few special cases to address.


Suggested change

* annotated as {@link Nullable}. However, there are a few special cases to address.

* annotated as {@link Nullable @Nullable}. However, there are a few special cases to address.

netdpb · 2024-10-10T15:39:50Z

src/main/java/org/jspecify/annotations/Nullable.java

+ *       of non-null strings. Note that arrays are treated as covariant: {@code String[]} (array of
+ *       non-null strings) is assignable to {@code @Nullable String[]} (array of nullable strings).


We should, but in the Javadoc?

netdpb · 2024-10-10T15:40:34Z

src/main/java/org/jspecify/annotations/Nullable.java

 * </ul>
 *
- * <h2 id="applicability">Where it is applicable</h2>
+ * <!-- TODO: move up -->


Meaning vs applicability? I think meaning is more important and should come first.

kevinb9n · 2024-11-18T17:30:44Z

Sorry for the delay; I am still embroiled in the epic journey of getting approval to sign the CLA.

batch of javadoc revisions for clarity and completeness

840e361

kevinb9n requested a review from netdpb September 26, 2024 00:47

netdpb reviewed Sep 27, 2024

View reviewed changes

review feedback from David

9d4fd43

netdpb reviewed Sep 27, 2024

View reviewed changes

review revisions, and AUTHORS

bc77a08

netdpb reviewed Oct 10, 2024

View reviewed changes

	* <li>On an <b>array type</b>: The nullness of the array and of its components can both be
	* <li>On an <b>array type</b>: The nullness of the array and of its components can each be

		* of non-null strings. Note that arrays are treated as covariant: {@code String[]} (array of
		* non-null strings) is assignable to {@code @Nullable String[]} (array of nullable strings).

	* href="https://bit.ly/3ppb8ZC">Why?</a>) This refers to either an unbounded wildcard like in
	* href="https://bit.ly/3ppb8ZC">Why?</a>) This can be either an unbounded wildcard like in

		* href="https://bit.ly/3ppb8ZC">Why?</a>) This refers to either an unbounded wildcard like in
		* {@code List<?>}, or a wildcard with a lower bound like in {@code List<? super Integer>}).

	* <li>When a type variable has a nullable upper bound, such as the {@code E} in {@code class
	* <li>When a type parameter has a nullable upper bound, such as the {@code E} in {@code class

	* Foo<E extends @Nullable Bar>}), an unannotated usage of this type variable within that
	* Foo<E extends @Nullable Bar>}), an unannotated usage of the associated type variable within that

	* being a purely dynamic property, not being a fixed declarative property of its type. (<a
	* being a purely dynamic property, not a fixed declarative property of its type. (<a

	* <p>The effects of this annotation can be negated at a narrower scope using {@link NullUnmarked}.
	* <p>The effects of this annotation can be negated at a narrower scope using {@link NullUnmarked @NullUnmarked}.

	* annotated as {@link Nullable}. However, there are a few special cases to address.
	* annotated as {@link Nullable @Nullable}. However, there are a few special cases to address.

batch of javadoc revisions for clarity and completeness #671

Are you sure you want to change the base?

batch of javadoc revisions for clarity and completeness #671

Uh oh!

Conversation

kevinb9n commented Sep 26, 2024

Uh oh!

google-cla bot commented Sep 26, 2024

Uh oh!

netdpb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinb9n commented Nov 18, 2024

Uh oh!

Uh oh!