TemporalQuery.queryFrom should return @Nullable R

[chaoren]

The documentation says "may return null to indicate not found":

jdk/src/java.base/share/classes/java/time/temporal/TemporalQuery.java

Line 143 in 2ada91b

* @return the queried value, may return null to indicate not found

Also, maybe R shouldn't extends @Nullable Object:

jdk/src/java.base/share/classes/java/time/temporal/TemporalQuery.java

Line 107 in 2ada91b

public interface TemporalQuery<R extends @Nullable Object> {

since the meaning of a null return value would be ambiguous if R itself is @Nullable.

[cpovirk]

The understanding I came to is that there are "TemporalQuery instances that can return null" and "TemporalQuery instances that cannot return null," in much the same way as there are Function instances that can return null and others that can't. (I base this on the constants in TemporalQueries.) That would make it useful to represent that distinction in the type system—TemporaryQuery<Foo> vs. TemporaryQuery<@Nullable Foo>. I agree that the Javadoc for TemporalQuery itself does not even hint at this, but does that sound right in light of the TemporalQueries class?

[chaoren]

does that sound right in light of the TemporalQueries class?

It seems every single type argument to TemporalQuery in TemporalQueries is @Nullable.

"TemporalQuery instances that cannot return null,"

Where do you see these?

[chaoren]

TemporalAccessor.query(TemporalQuery) also says "null may be returned (defined by the query)":

jdk/src/java.base/share/classes/java/time/temporal/TemporalAccessor.java

Lines 308 to 312 in 2ada91b

	* @return the query result, null may be returned (defined by the query)
	* @throws DateTimeException if unable to query
	* @throws ArithmeticException if numeric overflow occurs
	*/
	default <R extends @Nullable Object> R query(TemporalQuery<R> query) {

[cpovirk]

...huh :) I may have gotten confused when I saw a few entries like "A query for LocalDate returning null if not found" and then a few entries without "returning null if not found." And then presumably I didn't reevaluate when I actually looked at all the methods as part of annotating them.

Oh, maybe what I was actually going on was this...?

The most common implementations are method references, such as LocalDate::from and ZoneId::from.

And now I see your message about TemporalAccessor [edit: "defined by the query," which I read as supportive of the idea that the nullness is a property of the instance's type]. Maybe that?

But I'd need to look into how instances are actually used in order to understand what that's about....

OK, here are a couple factory methods (which I won't pretend that I have any memory of) that do appear to be claiming that they always return non-null values:

• DateTimeFormatter.parsedExcessDays
• DateTimeFormatter.parsedLeapSecond

[chaoren]

Some implementations of TemporalAccessor.query(TemporalQuery) (including the default implementation) actually explicitly return null. I'd take that as a signal that it should be returning @Nullable R instead of R. Are we running the nullness analyzer on the annotated JDK itself? It would be complaining about the explicit return nulls without a @Nullable R return type, right?

[cpovirk]

We don't run nullness checked on the annotated JDK. (We also haven't made much effort to annotate anything beyond publicly visible APIs, though we're inconsistent about that.) I agree that a checker would produce an error for...

jdk/src/java.base/share/classes/java/time/temporal/TemporalAccessor.java

Lines 312 to 316 in 2ada91b

	default <R extends @Nullable Object> R query(TemporalQuery<R> query) {
	if (query == TemporalQueries.zoneId()
	|| query == TemporalQueries.chronology()
	|| query == TemporalQueries.precision()) {
	return null;

...but I think that's one of those very rare cases (like in AbstractFuture) in which the code is correct and we'd just need to suppress: The default implementation directly returns null only for cases in which the given query is one of the TemporalQuery<@Nullable Foo> instances.

[chaoren]

Documentation on DateTimeFormatter.parsedExcessDays actually seems to imply that returning null would be expected, but they're doing something unusual. There's no reason to say "with a zero period returned instead of null" if you're not expecting it to return null in the first place.

jdk/src/java.base/share/classes/java/time/format/DateTimeFormatter.java

Lines 1376 to 1377 in 2ada91b

	* access to additional information from the parse. The query always returns
	* a non-null period, with a zero period returned instead of null.

[cpovirk]

As for how these things are even used: It looks like a large fraction of the usages of TemporyQuery in our depot involve parsing, either formatter.parse(text).query(query) or the (equivalent, I hope?) formatter.parse(text, query). In that case, we very much want for formatter.parse(text, LocalDate::from) to return a non-null LocalDate.

(And now I'm confused as to how formatter.parse(text, query) could promise to return "the parsed date-time, not null" if callers can pass a TemporalQuery that can return null....)

[cpovirk]

(And now I'm confused as to how formatter.parse(text, query) could promise to return "the parsed date-time, not null" if callers can pass a TemporalQuery that can return null....)

(Answer: The doc is wrong: ISO_DATE.parse("2025-02-19", zone())) returns null :))