Feedback on Records in JavaFX

[Nir Lisker]

>
> Your list of "almosts" corresponds almost exactly to the exploration we
> did during the design process, enumerating examples of patterns which make
> a class unsuitable for being a record.  At some level, it is a positive
> validation that your list did not contain anything that was unknown during
> the design.


Good :)

Instead, perhaps we should ask ourselves: "if I had records when I wrote
> this, could I have used them profitably?"


This is why I wrote "I will also note that I did not do any design changes,
though some of the classes could probably have been refactored to a more
sensible design from which a record could be extracted." I noticed that had
records existed at the time of writing, they would have been a better fit,
but my intention here was not to do redesign, just drop-in replacements.

In the examples you had with abstract classes, did they mostly have fields,
> or no?


No, point 3 is specifically "classes that behave like records, but
they extend an abstract class that does not have any fields".

Also, did you consider the point I made about accessor names of boolean
fields?

On Thu, May 7, 2020 at 5:17 PM Brian Goetz <brian.goetz at oracle.com> wrote:

> I thought I replied when you sent it, but apparently not.
>
> Your list of "almosts" corresponds almost exactly to the exploration we
> did during the design process, enumerating examples of patterns which make
> a class unsuitable for being a record.  At some level, it is a positive
> validation that your list did not contain anything that was unknown during
> the design.
>
> For each of these, we considered -- sometimes agonized over -- what it
> would take to bring these into the fold.  And for each, they either took
> records away from their design center, or greatly increased the complexity
> of the feature, or made it more confusing to users what records were.
>
> Looking at records through the lens of "how many of my classes can I
> convert", while tempting, is not necessarily the right way to judge the
> feature.  Instead, perhaps we should ask ourselves: "if I had records when
> I wrote this, could I have used them profitably?"  (When enums came out, I
> had very few classes lying around that I could just convert to an enum --
> but since then, I've found zillions of uses for enums.  (And yes, I've also
> found many "almost" uses for enums, which is sad.  I expect it will be the
> same way with records.))
>
> To score your list against the design goals (but please, people, this is
> not an invitation to reopen the debate over the design goals):
>
> 1/2 (unwanted accessors.)  These basically say that the class falls
> outside the design center -- a record is a nominal tuple.  Languages with
> tuples don't give you a way to say "but item #3 is private"; that's not
> what tuples are.
>
> 3 (extending abstract classes.)  This is one of the few on the list that
> we could conceivably have covered.  We'd have to have made `Record` an
> interface, and impose some severe restrictions on the superclasses, though,
> and at the time, the complexity of this seemed out of line with the
> benefits.  (The abstract classes would not be allowed to have fields, which
> you might find too harsh a restriction.)  Since then, however, similar work
> in Valhalla has identified what a "stateless" abstract class looks like, so
> its possible we can revisit this before finalizing the feature, but the
> incremental benefit still seems thin.
>
> In the examples you had with abstract classes, did they mostly have
> fields, or no?
>
> 4 (mutability.)  This was obviously the most inconvenient restriction, and
> you could tie yourself in knots trying to accommodate them (which I did,
> for a while.)  In the end, though, we concluded that there was no
> intermediate point between the current design point and "anything goes"
> that was either satisfying or stable.
>
> If you allow mutable state, it is almost inevitable that you will also
> want to exclude some subset of fields (maybe all of them, but maybe not)
> from the canonical constructor, and also exclude some (but a possibly
> different one!) subset of them from the equals/hashCode computation.  Which
> means that you would need syntactic surface for sorting components into
> various buckets (whether they go in the constructor or not, whether they
> are part of equals or not, whether they are part of toSTring or not,
> whether they get getters, whether they get setters, etc).  These each
> clutter the code, and the feature essentially degenerates to being a
> syntax-only, macro generator.  (And if you look at all the options that
> tools like Lombok have grown over the years, we can predict there would be
> an infinite stream of calls for "Can you add this code generation
> feature.")
>
> So while "can I just have mutability" sounds like a simple request, it
> essentially drives you towards building a very different feature, and one
> that arguably does not belong in the language.
>
> 5 (caching.)  This is one we have a lot of sympathy for; this is entirely
> within the design spirit of records.  We looked for a way to do this, but
> all the things we looked at either would slide us back into the hell
> outlined above in (4), or really wanted to be a general feature for all
> classes, not just records.  It is possible that one of these approaches
> might reappear as a general feature (e.g., lazily cached fields), and if
> they do, we would surely consider if records could play in the fun.
>
> 6 (internal state.)  This is another flavor of 4/5, where there is some
> state that is managed by the record but not part of the "record state".
> And again, we looked very hard at whether this could be accomodated, but
> again, there was no stable intermediate point that retained any of the
> semantic benefits of records.  Such internal state would invariably want to
> creep into the calculation of hashCode/equals, for example.  In the end, it
> was too far from the design center of "nominal tuple."
>
> 7 (public API.)  This one goes back to the top point, which is that if you
> evaluate records by "can I convert my existing classes to records", you get
> a different evaluation than "could I have profitably used records when
> writing this class."  Yes, there will be existing public API that can't
> migrate.  But as you build new public API, you will surely consider "can I
> make this a record."
>
>
> Apologies again for not responding.  As I said, we're sympathetic to all
> these issues, and effort was expended attempting to accomodate some of
> them.  In fact, it was the efforts expended trying to bring these into the
> fold that brought us to a better understanding of what the stable design
> center was -- nominal tuples.
>
>
>
> On 5/6/2020 8:23 PM, Nir Lisker wrote:
>
> Hi,
>
> Was this missed or is it not interesting?
>
> - Nir
>
> On Sun, Apr 26, 2020 at 5:34 AM Nir Lisker <nlisker at gmail.com> <nlisker at gmail.com> wrote:
>
>
> Hi,
>
> In order to both prepare JavaFX for the records feature and provide
> feedback on the feature here, I created a branch [1] in my JavaFX fork
> where I test records. I surveyed many (but far from all!) JavaFX classes
> and tried to replace current implementations with records wherever I could.
> I was not very careful with checking toString, hashCode and equals existing
> implementation, and assumed, for the purpose of this test, that the default
> implementation does what the original implementation is supposed to do. I
> will also note that I did not do any design changes, though some of the
> classes could probably have been refactored to a more sensible design from
> which a record could be extracted.
> The branch contains some build related files like .classpath files for
> Eclipse that can be ignored. The code changes are either record
> implementations, or changes to classes that now use records and needed to
> change the accessor name they call.
>
> I hope that this can serve as a concrete example of migration of a large
> production library, and would like your feedback on the changes as well.
>
> Additionally, here is a list of classes that "almost" fit a record for
> various reasons (you can use the GitHub file search to find them).
>
> 1. Generation of unneeded accessors for private classes
> Example classes:
> modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/expression/Expression.java
> (#Token)
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/SelectorPartitioning.java
> (#PartitionKey)
> modules/javafx.web/src/main/java/com/sun/webkit/network/DateParser.java
> (#Time)
>
> These classes behave like records, but accessors will be generated for
> fields that do not need them. Inner classes' private fields are accessible
> to the enclosing class, so many times there is no need for accessors. This
> is a minor point as they can just be ignored, but if there is a cost to
> this, I don't want to pay it.
>
> 2. Generation of unneeded accessors for non-private classes
> Example classes:
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/CascadingStyle.java
> modules/javafx.graphics/src/main/java/javafx/css/Match.java
> modules/javafx.web/src/main/java/com/sun/webkit/ContextMenu.java
> (#ShowContext)
>
> These classes behave like records, but accessors will be generated for
> fields that are not meant to be exposed.
>
> 3. Extending abstract classes
> Example classes:
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/FontFaceImpl.java
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/PseudoClassImpl.java
>
> These classes behave like records, but they extend an abstract class that
> does not have any fields. I remember a discussion for allowing this
> inheritance by making Record an interface,
>
> 4. Mutability
> Example classes:
> modules/javafx.graphics/src/main/java/com/sun/prism/BasicStroke.java
> modules/javafx.graphics/src/main/java/com/sun/javafx/geom/Vec2d.java (and
> similar VecXx)
>
> These classes behave like records, but they require mutable fields.
>
> 5. Caching hash code
> Example classes:
> modules/javafx.web/src/main/java/com/sun/webkit/text/TextBreakIterator.java
> (#CacheKey)
>
> This class behave like records, but caches the hashcode. This is useful in
> cases where hashcode computation is expensive. Since the fields are final,
> this value is *usually* constant, but in case of mutable reference types,
> their hashcode can change externally after they are passed to the record.
> This can be mitigated by defensive copying. There's a lot that can be said
> here, but it might seems reasonable to support this caching in some way
> (I'm waving my hands here).
>
> 6. Internal state
> Example classes:
>
> modules/javafx.web/src/main/java/com/sun/webkit/event/WCInputMethodEvent.java
>
> This class behaves like a record, but has an internal field that is not
> passed through the constructor. Notably, this class also does defensive
> copy of the array both in and out (using Arrays.copyOf and not clone as
> I've seen before).
>
> 7. Public API
> Example classes:
> modules/javafx.graphics/src/main/java/javafx/animation/KeyFrame.java
> modules/javafx.graphics/src/main/java/javafx/animation/KeyValue.java
> ("suffers" from point 6)
> modules/javafx.base/src/main/java/javafx/util/Pair.java
>
> These classes behave like records, but they are public API, so there could
> be migrations issues.
>
> * Additional note about accessors of boolean fields
> Example
> class: modules/javafx.web/src/main/java/javafx/scene/web/PopupFeatures.java
>
> This class uses boolean fields named "menu", "toolbar" etc. to denote if a
> menu or a toolbar exists. In this case, having a "hasX()" accessor makes
> sense while "x()" does not. In my implementation, I changed the field name
> to "hasX" so that the accessor names would match. I think that booleans are
> a special case. Most accessors retrieve a value like "width()" or "name()",
> but booleans usually answer an "is" or "has" question - "isEnlarged()" and
> not "enlarged()". Enforcing the current name of the accessor can
> *sometimes* lead to awkward namings.
>
> ### Conclusions
> There are not that many places where records can be used, though for these
> specific use cases they fit well (nominal tuples); many of them are inner
> classes. From the above list, some stray a lot from the idea of records
> (4), some not so much (1, 3).
> I would cautiously say that the design is too restrictive, especially when
> it comes to internal helper state (5, 6). I would think that, while
> a record component is given to the constructor and exposed through the
> accessor, other internal (final) fields that help with implementations
> would be useful. This reminds me of interfaces - while their public methods
> serve as contract, private methods were made valid just to help with
> internal implementation, and they are invisible to the contract.
> With regards to accessor visibility (1, 2), I know that there was (or is)
> a discussion about allowing to reduce their visibility to that of the class
> (not sure about the constructor). Sometimes there is no need for them at
> all.
>
> I hope that this concept of records can be expanded, either to specialized
> types for other similar use cases, or with changes from other features that
> work well with records, as records alone don't hit enough targets in my
> opinion. I intent to continue to update this branch as I work with whatever
> use cases I find, so by the time JavaFX can use records we would have a
> good starting point.
>
> Best,
> Nir
>
> [1] https://github.com/nlisker/jfx/tree/Records
>
>
>