Improve method implemeations that accept temporal interface parameters

[JeremyBetts]

Some methods that accept an interface such as Temporal will do an “instanceof” check and throw an exception if the two classes do not match. A reasonable effort should be made to honor the interface before throwing an exception. For example ZonedDateTime, Instant, and OffsetDateTime can all represent a point on the timeline. If calculating the amount of time between an Instant and a OffsetDateTime, you’d have to use the “from()” method to cast one of the objects before calculating the time between them. Not doing so will result in an exception being thrown at runtime. Interfaces should be honored as much as possible otherwise poor coding patterns such as the following could emerge in code that uses the java.time api.

if(param instanceof LocalDateTime){ finalParam = param; }else{ finalParam = LocalDateTime.from(param); }

[jodastephen]

AFAICT, this only applies to the until(Temporal,TemporalUnit) methods with currently insist that the input is the same type. This is for good reason, as the amount of time between a LocalDate and a LocalDateTime is far from obvious.

That said, it would be perfectly possible to require that the input argument to that method must use the from(Temporal) method for conversion.

The code sample at the end would not be relevant, as the from(Temporal) method checks for LocalDateTime anyway. Thus the code folds to finalParam = LocalDateTime.from(param).

[jodastephen]

If we do this, the changes will be along the lines of this patch.

I need agreement that we are doing this before completing the patch.

diff --git a/src/share/classes/java/time/LocalDate.java b/src/share/classes/java/time/LocalDate.java
--- a/src/share/classes/java/time/LocalDate.java
+++ b/src/share/classes/java/time/LocalDate.java
@@ -353,6 +353,7 @@
      * @throws DateTimeException if unable to convert to a {@code LocalDate}
      */
     public static LocalDate from(TemporalAccessor temporal) {
+        Objects.requireNonNull(temporal, "temporal");
         LocalDate date = temporal.query(TemporalQuery.localDate());
         if (date == null) {
             throw new DateTimeException("Unable to obtain LocalDate from TemporalAccessor: " + temporal.getClass());
@@ -1541,7 +1542,8 @@
      * objects in terms of a single {@code TemporalUnit}.
      * The start and end points are {@code this} and the specified date.
      * The result will be negative if the end is before the start.
-     * The {@code Temporal} passed to this method must be a {@code LocalDate}.
+     * The {@code Temporal} passed to this method is converted to a
+     * {@code LocalDate} using {@link #from(TemporalAccessor)}.
      * For example, the amount in days between two dates can be calculated
      * using {@code startDate.until(endDate, DAYS)}.
      * <p>
@@ -1572,7 +1574,7 @@
      * <p>
      * This instance is immutable and unaffected by this method call.
      *
-     * @param endDate  the end date, which must be a {@code LocalDate}, not null
+     * @param endExclusive  the end date, which is converted to a {@code LocalDate}, not null
      * @param unit  the unit to measure the amount in, not null
      * @return the amount of time between this date and the end date
      * @throws DateTimeException if the amount cannot be calculated
@@ -1580,13 +1582,8 @@
      * @throws ArithmeticException if numeric overflow occurs
      */
     @Override
-    public long until(Temporal endDate, TemporalUnit unit) {
-        Objects.requireNonNull(unit, "unit");
-        if (endDate instanceof LocalDate == false) {
-            Objects.requireNonNull(endDate, "endDate");
-            throw new DateTimeException("Unable to calculate amount as objects are of two different types");
-        }
-        LocalDate end = (LocalDate) endDate;
+    public long until(Temporal endExclusive, TemporalUnit unit) {
+        LocalDate end = LocalDate.from(endExclusive);
         if (unit instanceof ChronoUnit) {
             switch ((ChronoUnit) unit) {
                 case DAYS: return daysUntil(end);
@@ -1600,7 +1597,7 @@
             }
             throw new UnsupportedTemporalTypeException("Unsupported unit: " + unit);
         }
-        return unit.between(this, endDate);
+        return unit.between(this, end);
     }

     long daysUntil(LocalDate end) {
diff --git a/src/share/classes/java/time/LocalTime.java b/src/share/classes/java/time/LocalTime.java
--- a/src/share/classes/java/time/LocalTime.java
+++ b/src/share/classes/java/time/LocalTime.java
@@ -394,6 +394,7 @@
      * @throws DateTimeException if unable to convert to a {@code LocalTime}
      */
     public static LocalTime from(TemporalAccessor temporal) {
+        Objects.requireNonNull(temporal, "temporal");
         LocalTime time = temporal.query(TemporalQuery.localTime());
         if (time == null) {
             throw new DateTimeException("Unable to obtain LocalTime from TemporalAccessor: " + temporal.getClass());
@@ -1330,7 +1331,8 @@
      * objects in terms of a single {@code TemporalUnit}.
      * The start and end points are {@code this} and the specified time.
      * The result will be negative if the end is before the start.
-     * The {@code Temporal} passed to this method must be a {@code LocalTime}.
+     * The {@code Temporal} passed to this method is converted to a
+     * {@code LocalTime} using {@link #from(TemporalAccessor)}.
      * For example, the amount in hours between two times can be calculated
      * using {@code startTime.until(endTime, HOURS)}.
      * <p>
@@ -1361,7 +1363,7 @@
      * <p>
      * This instance is immutable and unaffected by this method call.
      *
-     * @param endTime  the end time, which must be a {@code LocalTime}, not null
+     * @param endExclusive  the end time, which is converted to a {@code LocalTime}, not null
      * @param unit  the unit to measure the amount in, not null
      * @return the amount of time between this time and the end time
      * @throws DateTimeException if the amount cannot be calculated
@@ -1369,12 +1371,8 @@
      * @throws ArithmeticException if numeric overflow occurs
      */
     @Override
-    public long until(Temporal endTime, TemporalUnit unit) {
-        if (endTime instanceof LocalTime == false) {
-            Objects.requireNonNull(endTime, "endTime");
-            throw new DateTimeException("Unable to calculate amount as objects are of two different types");
-        }
-        LocalTime end = (LocalTime) endTime;
+    public long until(Temporal endExclusive, TemporalUnit unit) {
+        LocalTime end = LocalTime.from(endExclusive);
         if (unit instanceof ChronoUnit) {
             long nanosUntil = end.toNanoOfDay() - toNanoOfDay();  // no overflow
             switch ((ChronoUnit) unit) {
@@ -1388,7 +1386,7 @@
             }
             throw new UnsupportedTemporalTypeException("Unsupported unit: " + unit);
         }
-        return unit.between(this, endTime);
+        return unit.between(this, end);
     }

     /**
diff --git a/src/share/classes/java/time/temporal/IsoFields.java b/src/share/classes/java/time/temporal/IsoFields.java
--- a/src/share/classes/java/time/temporal/IsoFields.java
+++ b/src/share/classes/java/time/temporal/IsoFields.java
@@ -685,6 +685,9 @@

         @Override
         public long between(Temporal temporal1, Temporal temporal2) {
+            if (temporal1.getClass() != temporal2.getClass()) {
+                return temporal1.until(temporal2, this);
+            }
             switch(this) {
                 case WEEK_BASED_YEARS:
                     return Math.subtractExact(temporal2.getLong(WEEK_BASED_YEAR),
diff --git a/src/share/classes/java/time/temporal/Temporal.java b/src/share/classes/java/time/temporal/Temporal.java
--- a/src/share/classes/java/time/temporal/Temporal.java
+++ b/src/share/classes/java/time/temporal/Temporal.java
@@ -374,8 +374,9 @@
      * Calculates the amount of time until another temporal in terms of the specified unit.
      * <p>
      * This calculates the amount of time between two temporal objects
-     * of the same type in terms of a single {@code TemporalUnit}.
+     * in terms of a single {@code TemporalUnit}.
      * The start and end points are {@code this} and the specified temporal.
+     * The end point is converted to be of the same type as the start point if different.
      * The result will be negative if the end is before the start.
      * For example, the period in hours between two temporal objects can be
      * calculated using {@code startTime.until(endTime, HOURS)}.
@@ -415,28 +416,33 @@
      * passing {@code this} as the first argument and the input temporal as
      * the second argument.
      * <p>
-     * In summary, implementations must behave in a manner equivalent to this code:
+     * In summary, implementations must behave in a manner equivalent to this pseudo-code:
      * <pre>
-     *  // check input temporal is the same type as this class
+     *  // convert the end temporal to the same type as this class
      *  if (unit instanceof ChronoUnit) {
      *    // if unit is supported, then calculate and return result
      *    // else throw UnsupportedTemporalTypeException for unsupported units
      *  }
-     *  return unit.between(this, endTemporal);
+     *  return unit.between(this, convertedEndTemporal);
      * </pre>
      * <p>
+     * Note that the unit's {@code between} method must only be invoked if the
+     * two temporal objects have exactly the same type evaluated by {@code getClass()}.
+     * <p>
      * Implementations must ensure that no observable state is altered when this
      * read-only method is invoked.
      *
-     * @param endTemporal  the end temporal, of the same type as this object, not null
+     * @param endTemporalExclusive  the end temporal, converted to be of the
+     *  same type as this object, not null
      * @param unit  the unit to measure the amount in, not null
      * @return the amount of time between this temporal object and the specified one
      *  in terms of the unit; positive if the specified object is later than this one,
      *  negative if it is earlier than this one
-     * @throws DateTimeException if the amount cannot be calculated
+     * @throws DateTimeException if the amount cannot be calculated, or the end
+     *  temporal cannot be converted to the same type as this temporal
      * @throws UnsupportedTemporalTypeException if the unit is not supported
      * @throws ArithmeticException if numeric overflow occurs
      */
-    long until(Temporal endTemporal, TemporalUnit unit);
+    long until(Temporal endTemporalExclusive, TemporalUnit unit);

 }
diff --git a/src/share/classes/java/time/temporal/TemporalUnit.java b/src/share/classes/java/time/temporal/TemporalUnit.java
--- a/src/share/classes/java/time/temporal/TemporalUnit.java
+++ b/src/share/classes/java/time/temporal/TemporalUnit.java
@@ -231,7 +231,9 @@
      * Calculates the amount of time between two temporal objects.
      * <p>
      * This calculates the amount in terms of this unit. The start and end
-     * points are supplied as temporal objects and must be of the same type.
+     * points are supplied as temporal objects and must be of compatible types.
+     * The implementation will convert the second type to be an instance of the
+     * first type before the calculating the amount.
      * The result will be negative if the end is before the start.
      * For example, the amount in hours between two temporal objects can be
      * calculated using {@code HOURS.between(startTime, endTime)}.
@@ -264,15 +266,21 @@
      * If the unit is not supported an {@code UnsupportedTemporalTypeException} must be thrown.
      * Implementations must not alter the specified temporal objects.
      *
+     * @implSpec
+     * Implementations must begin by checking to if the two temporals have the
+     * same type. If they do not, then the result must be obtained by calling
+     * {@code temporal1.until(temporal2Exclusive, this)}.
+     *
      * @param temporal1  the base temporal object, not null
-     * @param temporal2  the other temporal object, not null
+     * @param temporal2Exclusive  the other temporal object, not null
      * @return the amount of time between temporal1 and temporal2 in terms of this unit;
      *  positive if temporal2 is later than temporal1, negative if earlier
-     * @throws DateTimeException if the amount cannot be calculated
+     * @throws DateTimeException if the amount cannot be calculated, or the end
+     *  temporal cannot be converted to the same type as the start temporal
      * @throws UnsupportedTemporalTypeException if the unit is not supported by the temporal
      * @throws ArithmeticException if numeric overflow occurs
      */
-    long between(Temporal temporal1, Temporal temporal2);
+    long between(Temporal temporal1, Temporal temporal2Exclusive);

     //-----------------------------------------------------------------------
     /**

[RogerRiggs]

There should be a fast path throught the *.from methods for the requested type. At the moment, they mostly call TemporalQuery::xxx which recomputes epochDay or NANO_OF_DAY and constructs a new intermediate instance and will decrease performance. The instanceOf checks should move to the from method and only delegate to the Query if it is not already the requested type.

This does not change the signatures and does not make the possible runtime-failures any worse or the exceptions any harder to debug and fix. Seems reasonable to me to proceed.

[jodastephen]

The from methods don't have a fast path because the queries do. See the implementation of query in LocalDate/LocalTime for example.

[jodastephen]

Patch awaiting review https://gist.github.com/jodastephen/6549818

[RogerRiggs]

I was looking at TemporalQuery.localTime() that returns an unoptimized TemporalQueries.LOCAL_TIME version when invoked.

[jodastephen]

But that isn't what from(TemporalAccessor) calls. That method invokes temporal.query(Query) which has special handling for == TemporalQuery.localTime().

[RogerRiggs]

It was more the point that invoking the localTime() method handle doesn't see the optimization. But there are no known performance problems in either case so its not significant.

[jodastephen]

Is the patch itself OK?

[RogerRiggs]

Yes, see comments in the patch.

[jodastephen]

http://hg.openjdk.java.net/threeten/threeten/jdk/rev/055a31aae8b6

[RogerRiggs]

jbs mirror 8024835