Change Chronology to be an interface

[jodastephen]

Chronology is currently a class, but might be better as an interface.

While writing tests I found myself writing JapaneseChronology.from(localDate), expecting it to return a JapaneseDate. It didn't because the from method is on Chronology and intended for use in extracting the chronology from a temporal object.

The other static methods, of(String), ofLocale(Locale) and getAvailableChronologies() all suffer from the same static inheritance problem.

Using an interface fits with the package, where other Chrono classes are interfaces. It also allows the same static methods to not be inherited by subclasses, avoiding errors. Making the change would require a ChronologyImpl package scoped class to handle the current behaviour.

[RogerRiggs]

Perhaps using JapaneseDate.from(localDate) would be more natural and gives the desired result. What impact is there on serialization?

[jodastephen]

I played with this for an hour or so. I think the right approach would be as follows:

• Make Chronology into an interface
• Make resolveDate abstract, and other methods on Chronology defaults
• Add new public AbstractChronology class, with the resolveDate defined here
• Define generics as AbstractChronology<D extends ChronoLocalDate>, which allows facory methods like date(..) to no longer need overriding in classes like MingoChronology
• Remove generics from ChronoLocalDate, otherwise there is lots of stupid casting
• Define serialization as only applying to AbstractChronology

I think the end result would be neater, with the extra abstract class simplifying external chronology implementations.

The smaller change would make AbstractChronology non-public, but I suspect thats a missed opportunity.

[RogerRiggs]

That's a very large change just to make a few methods not be inherited.

• I would avoid adding a public class for AbstractChronology; it is extra surface area to test and would likely lead to other refactoring to support other external Chronologies.
• Some of the date(...) classes still need to be overridden to create the MinguoDate instances
• I've been round and round about the generics on ChronoLocalDate and the wildcard generics there are still the best option to accurately reflect consistently generics and wildcard types.

Are there other benefits besides the original observation?

[jodastephen]

This minor patch moves the package scoped methods on Chronology to the impl classes. This should be committed whatever we do with the rest of the issue.

# HG changeset patch
# User scolebourne
# Date 1372347579 -3600
# Node ID 3fca04188e348e351268d8e59ab7b3dd7e20beb8
# Parent  73a95812438c7f7f95eb7b4d26ac74f98474edaa
Move ensureChrono() methods to be static on impl classes
This is a better place for the code
Better casting has also been added
See #321

diff --git a/src/share/classes/java/time/chrono/ChronoDateImpl.java b/src/share/classes/java/time/chrono/ChronoDateImpl.java
--- a/src/share/classes/java/time/chrono/ChronoDateImpl.java
+++ b/src/share/classes/java/time/chrono/ChronoDateImpl.java
@@ -147,6 +147,25 @@
     private static final long serialVersionUID = 6282433883239719096L;

     /**
+     * Casts the {@code Temporal} to {@code ChronoLocalDate} ensuring it bas the specified chronology.
+     *
+     * @param chrono  the chronology to check for, not null
+     * @param temporal  a date-time to cast, not null
+     * @return the date-time checked and cast to {@code ChronoLocalDate}, not null
+     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDate
+     *  or the chronology is not equal this Chronology
+     */
+    static <D extends ChronoLocalDate<D>> D ensureValid(Chronology chrono, Temporal temporal) {
+        @SuppressWarnings("unchecked")
+        D other = (D) temporal;
+        if (chrono.equals(other.getChronology()) == false) {
+            throw new ClassCastException("Chronology mismatch, expected: " + chrono.getId() + ", actual: " + other.getChronology().getId());
+        }
+        return other;
+    }
+
+    //-----------------------------------------------------------------------
+    /**
      * Creates an instance.
      */
     ChronoDateImpl() {
diff --git a/src/share/classes/java/time/chrono/ChronoLocalDate.java b/src/share/classes/java/time/chrono/ChronoLocalDate.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDate.java
+++ b/src/share/classes/java/time/chrono/ChronoLocalDate.java
@@ -384,7 +384,7 @@
      */
     @Override
     default D with(TemporalAdjuster adjuster) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.with(adjuster));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.with(adjuster));
     }

     /**
@@ -398,7 +398,7 @@
         if (field instanceof ChronoField) {
             throw new UnsupportedTemporalTypeException("Unsupported field: " + field);
         }
-        return (D) getChronology().ensureChronoLocalDate(field.adjustInto(this, newValue));
+        return ChronoDateImpl.ensureValid(getChronology(), field.adjustInto(this, newValue));
     }

     /**
@@ -408,7 +408,7 @@
      */
     @Override
     default D plus(TemporalAmount amount) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.plus(amount));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.plus(amount));
     }

     /**
@@ -421,7 +421,7 @@
         if (unit instanceof ChronoUnit) {
             throw new UnsupportedTemporalTypeException("Unsupported unit: " + unit);
         }
-        return (D) getChronology().ensureChronoLocalDate(unit.addTo(this, amountToAdd));
+        return ChronoDateImpl.ensureValid(getChronology(), unit.addTo(this, amountToAdd));
     }

     /**
@@ -431,7 +431,7 @@
      */
     @Override
     default D minus(TemporalAmount amount) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.minus(amount));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.minus(amount));
     }

     /**
@@ -442,7 +442,7 @@
      */
     @Override
     default D minus(long amountToSubtract, TemporalUnit unit) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.minus(amountToSubtract, unit));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.minus(amountToSubtract, unit));
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/ChronoLocalDateTime.java b/src/share/classes/java/time/chrono/ChronoLocalDateTime.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDateTime.java
+++ b/src/share/classes/java/time/chrono/ChronoLocalDateTime.java
@@ -203,7 +203,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> with(TemporalAdjuster adjuster) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.with(adjuster)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.with(adjuster));
     }

     /**
@@ -221,7 +221,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> plus(TemporalAmount amount) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.plus(amount)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.plus(amount));
     }

     /**
@@ -239,7 +239,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> minus(TemporalAmount amount) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.minus(amount)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amount));
     }

     /**
@@ -249,7 +249,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> minus(long amountToSubtract, TemporalUnit unit) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.minus(amountToSubtract, unit)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amountToSubtract, unit));
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java b/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
+++ b/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
@@ -177,6 +177,25 @@
     }

     /**
+     * Casts the {@code Temporal} to {@code ChronoLocalDateTime} ensuring it bas the specified chronology.
+     *
+     * @param chrono  the chronology to check for, not null
+     * @param temporal   a date-time to cast, not null
+     * @return the date-time checked and cast to {@code ChronoLocalDateTime}, not null
+     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDateTimeImpl
+     *  or the chronology is not equal this Chronology
+     */
+    static <D extends ChronoLocalDate<D>> ChronoLocalDateTimeImpl<D> ensureValid(Chronology chrono, Temporal temporal) {
+        @SuppressWarnings("unchecked")
+        ChronoLocalDateTimeImpl<D> other = (ChronoLocalDateTimeImpl<D>) temporal;
+        if (chrono.equals(other.toLocalDate().getChronology()) == false) {
+            throw new ClassCastException("Chronology mismatch, required: " + chrono.getId()
+                    + ", actual: " + other.toLocalDate().getChronology().getId());
+        }
+        return other;
+    }
+
+    /**
      * Constructor.
      *
      * @param date  the date part of the date-time, not null
@@ -202,7 +221,7 @@
             return this;
         }
         // Validate that the new Temporal is a ChronoLocalDate (and not something else)
-        D cd = (D) date.getChronology().ensureChronoLocalDate(newDate);
+        D cd = ChronoDateImpl.ensureValid(date.getChronology(), newDate);
         return new ChronoLocalDateTimeImpl<>(cd, newTime);
     }

@@ -264,9 +283,9 @@
         } else if (adjuster instanceof LocalTime) {
             return with(date, (LocalTime) adjuster);
         } else if (adjuster instanceof ChronoLocalDateTimeImpl) {
-            return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime((ChronoLocalDateTimeImpl<?>) adjuster));
+            return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), (ChronoLocalDateTimeImpl<?>) adjuster);
         }
-        return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime((ChronoLocalDateTimeImpl<?>) adjuster.adjustInto(this)));
+        return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), (ChronoLocalDateTimeImpl<?>) adjuster.adjustInto(this));
     }

     @Override
@@ -279,7 +298,7 @@
                 return with(date.with(field, newValue), time);
             }
         }
-        return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime(field.adjustInto(this, newValue)));
+        return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), field.adjustInto(this, newValue));
     }

     //-----------------------------------------------------------------------
@@ -298,7 +317,7 @@
             }
             return with(date.plus(amountToAdd, unit), time);
         }
-        return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime(unit.addTo(this, amountToAdd)));
+        return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), unit.addTo(this, amountToAdd));
     }

     private ChronoLocalDateTimeImpl<D> plusDays(long days) {
diff --git a/src/share/classes/java/time/chrono/ChronoZonedDateTime.java b/src/share/classes/java/time/chrono/ChronoZonedDateTime.java
--- a/src/share/classes/java/time/chrono/ChronoZonedDateTime.java
+++ b/src/share/classes/java/time/chrono/ChronoZonedDateTime.java
@@ -350,7 +350,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> with(TemporalAdjuster adjuster) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.with(adjuster)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.with(adjuster));
     }

     /**
@@ -368,7 +368,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> plus(TemporalAmount amount) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.plus(amount)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.plus(amount));
     }

     /**
@@ -386,7 +386,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> minus(TemporalAmount amount) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.minus(amount)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amount));
     }

     /**
@@ -396,7 +396,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> minus(long amountToSubtract, TemporalUnit unit) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.minus(amountToSubtract, unit)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amountToSubtract, unit));
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java b/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
--- a/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
+++ b/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
@@ -188,6 +188,25 @@
         return (ChronoZonedDateTimeImpl<D>)ofInstant(toLocalDate().getChronology(), instant, zone);
     }

+    /**
+     * Casts the {@code Temporal} to {@code ChronoZonedDateTimeImpl} ensuring it bas the specified chronology.
+     *
+     * @param chrono  the chronology to check for, not null
+     * @param temporal  a date-time to cast, not null
+     * @return the date-time checked and cast to {@code ChronoZonedDateTimeImpl}, not null
+     * @throws ClassCastException if the date-time cannot be cast to ChronoZonedDateTimeImpl
+     *  or the chronology is not equal this Chronology
+     */
+    static <D extends ChronoLocalDate<D>> ChronoZonedDateTimeImpl<D> ensureValid(Chronology chrono, Temporal temporal) {
+        @SuppressWarnings("unchecked")
+        ChronoZonedDateTimeImpl<D> other = (ChronoZonedDateTimeImpl<D>) temporal;
+        if (chrono.equals(other.toLocalDate().getChronology()) == false) {
+            throw new ClassCastException("Chronology mismatch, required: " + chrono.getId()
+                    + ", actual: " + other.toLocalDate().getChronology().getId());
+        }
+        return other;
+    }
+
     //-----------------------------------------------------------------------
     /**
      * Constructor.
@@ -271,7 +290,7 @@
             }
             return ofBest(dateTime.with(field, newValue), zone, offset);
         }
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(field.adjustInto(this, newValue)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), field.adjustInto(this, newValue));
     }

     //-----------------------------------------------------------------------
@@ -280,7 +299,7 @@
         if (unit instanceof ChronoUnit) {
             return with(dateTime.plus(amountToAdd, unit));
         }
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(unit.addTo(this, amountToAdd)));   /// TODO: Generics replacement Risk!
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), unit.addTo(this, amountToAdd));   /// TODO: Generics replacement Risk!
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/Chronology.java b/src/share/classes/java/time/chrono/Chronology.java
--- a/src/share/classes/java/time/chrono/Chronology.java
+++ b/src/share/classes/java/time/chrono/Chronology.java
@@ -487,60 +487,6 @@

     //-----------------------------------------------------------------------
     /**
-     * Casts the {@code Temporal} to {@code ChronoLocalDate} with the same chronology.
-     *
-     * @param temporal  a date-time to cast, not null
-     * @return the date-time checked and cast to {@code ChronoLocalDate}, not null
-     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDate
-     *  or the chronology is not equal this Chronology
-     */
-    ChronoLocalDate<?> ensureChronoLocalDate(Temporal temporal) {
-        @SuppressWarnings("unchecked")
-        ChronoLocalDate<?> other = (ChronoLocalDate<?>) temporal;
-        if (this.equals(other.getChronology()) == false) {
-            throw new ClassCastException("Chronology mismatch, expected: " + getId() + ", actual: " + other.getChronology().getId());
-        }
-        return other;
-    }
-
-    /**
-     * Casts the {@code Temporal} to {@code ChronoLocalDateTime} with the same chronology.
-     *
-     * @param temporal   a date-time to cast, not null
-     * @return the date-time checked and cast to {@code ChronoLocalDateTime}, not null
-     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDateTimeImpl
-     *  or the chronology is not equal this Chronology
-     */
-    ChronoLocalDateTimeImpl<?> ensureChronoLocalDateTime(Temporal temporal) {
-        @SuppressWarnings("unchecked")
-        ChronoLocalDateTimeImpl<?> other = (ChronoLocalDateTimeImpl<?>) temporal;
-        if (this.equals(other.toLocalDate().getChronology()) == false) {
-            throw new ClassCastException("Chronology mismatch, required: " + getId()
-                    + ", supplied: " + other.toLocalDate().getChronology().getId());
-        }
-        return other;
-    }
-
-    /**
-     * Casts the {@code Temporal} to {@code ChronoZonedDateTimeImpl} with the same chronology.
-     *
-     * @param temporal  a date-time to cast, not null
-     * @return the date-time checked and cast to {@code ChronoZonedDateTimeImpl}, not null
-     * @throws ClassCastException if the date-time cannot be cast to ChronoZonedDateTimeImpl
-     *  or the chronology is not equal this Chronology
-     */
-    ChronoZonedDateTimeImpl<?> ensureChronoZonedDateTime(Temporal temporal) {
-        @SuppressWarnings("unchecked")
-        ChronoZonedDateTimeImpl<?> other = (ChronoZonedDateTimeImpl<?>) temporal;
-        if (this.equals(other.toLocalDate().getChronology()) == false) {
-            throw new ClassCastException("Chronology mismatch, required: " + getId()
-                    + ", supplied: " + other.toLocalDate().getChronology().getId());
-        }
-        return other;
-    }
-
-    //-----------------------------------------------------------------------
-    /**
      * Gets the ID of the chronology.
      * <p>
      * The ID uniquely identifies the {@code Chronology}.
@@ -768,7 +714,7 @@

             } catch (DateTimeException ex1) {
                 @SuppressWarnings("rawtypes")
-                ChronoLocalDateTimeImpl cldt = ensureChronoLocalDateTime(localDateTime(temporal));
+                ChronoLocalDateTimeImpl cldt = ChronoLocalDateTimeImpl.ensureValid(this, localDateTime(temporal));
                 return ChronoZonedDateTimeImpl.ofBest(cldt, zone, null);
             }
         } catch (DateTimeException ex) {

[jodastephen]

http://hg.openjdk.java.net/threeten/threeten/jdk/rev/c8db06948326

[jodastephen]

Patch of the basic proposal added https://gist.github.com/jodastephen/5961060

This involves making Chronology an interface, and adding a public AbstractChronology. If generics are simplified on ChronoLocalDate, then AbstractChronology<D extends ChronoLocalDate> becomes possible to save casting overrides in subclasses.

AbstractChronology has to be public as the resolveDate method is too complex to be a default method on an interface (as we don't have package scoped methods on interfaces yet).

[RogerRiggs]

Finally got back to look at this again with the API freeze deadline looming. It seems like a reasonable change, it is fairly localized though a number of tests will need to be updated also.

[jodastephen]

To confirm, we do the Chronology interface+ AbstractChronology public class. Can I make the change on the threeten repo? Is it up to date enough?

[RogerRiggs]

Hi,

The threaten repository is up to date enough for the change though it is out of date with jdk8-tl to and jdk8.

I would like to transition to working from jdk8-tl as the reviews are easier to merge and the latency will be lower.

Thanks, Roger

On Sep 22, 2013, at 0:27, Stephen Colebourne notifications@github.com wrote:

To confirm, we do the Chronology interface+ AbstractChronology public class. Can I make the change on the threeten repo? Is it up to date enough?

— Reply to this email directly or view it on GitHub.

[RogerRiggs]

JBS issue https://bugs.openjdk.java.net/browse/JDK-8025719

[RogerRiggs]

Is the patch considered mature and ready to use or is an update needed?

[jodastephen]

I haven't checked if the patch still works. However, the basic principles of the patch still look sound. Given the divergence with tl, it may be best to simply prepare a webrev vs there.

[RogerRiggs]

Resolved with http://hg.openjdk.java.net/jdk8/tl/jdk/rev/110107410393