RFR: 8066870 Add Readable::transferTo(Appendable)

[Patrick Reinhart]

Yes, I meant the Reader.transferTo(Writer), and no I just did a quick search over the JDK code base and I would to takle them as a second step. I would make a couple of different patches for the various places due the fact that they are in different parts of the JDK…

The question would be if I should proceed with the Reader.transferTo(Writer) together with the static method on Readable.transferTo(Appendable)…

-Patrick


> Am 19.11.2017 um 23:14 schrieb Brian Goetz <brian.goetz at oracle.com>:
> 
> I presume you mean Reader.transferTo(Writer)?  This seems fine, and you can have a much better implementation for Reader->Writer than for Readable->Appendable.  (Do we get patches for those 30 places too?)
> 
> 
> 
> On 11/19/2017 3:01 PM, Patrick Reinhart wrote:
>> Having a static method on Appendable instead of a default one seem reasonable to me too.
>> 
>> Would it be a problem to have an additional transferTo() method on Readable as in your overload example though similar to IntputStream? My researches of the JDK code base found at least 30 places where a simple transferTo on a Reader could have been used to transfer all data to a Writer. Also some less usages where a combination of Reader -> CharBuffer or CharBuffer -> Appendable could have been used to. This analysis lead to the idea to have a default method for those use cases instead may be use a separate static method for Readable/Appendable and one on a Reader as on the InputStream
>> 
>> -Patrick
>> 
>> 
>>> Am 19.11.2017 um 14:42 schrieb Brian Goetz <brian.goetz at oracle.com>:
>>> 
>>> Late to this thread, but ...
>>> 
>>> TL;DR: I think this method, as proposed, is fatally flawed; it should be a static method rather than a default method.
>>> 
>>> 
>>> We need to be very, very careful about adding default methods like this to highly abstract interfaces like Readable (the -able suffix is often the giveaway that you're in murky waters wrt default methods).  There are many pitfalls one can run into here; the one that gets us here is that attempted overrides can easily and accidentally become overloads.
>>> 
>>> DIGRESSION -- A LESSON FROM 8
>>> 
>>> By way of background, we added Function.compose(...) in Java 8, when both functional interfaces and default methods were new, and this turned out to be a mistake.  This seemed a no-brainer (in j.l.f.Function), but didn't turn out so well.  We added:
>>> 
>>> intf Function<T,U> {
>>>     <V> Function<T,V> compose(Function<U,V> g);
>>> }
>>> 
>>> But note also that
>>> 
>>> intf BinaryOperator<T> extends Function<T,T> {
>>> }
>>> 
>>> As such, binOp.compose(binOp) yields a Function<T,T>, not a BinOp<T>.  Oops, that's not what we wanted!  If we try and override it:
>>> 
>>> intf BinaryOperator<T> extends Function<T,T> {
>>>     BinaryOperator<T> compose(BinaryOperator<T> g);
>>> }
>>> 
>>> we haven't really overridden compose(), but instead we've overloaded it.  And now we have potential ambiguities in overload resolution because
>>> 
>>>   f.compose(t -> t)
>>> 
>>> is compatible with both overrides of compose().  And since we don't have overloading on return types, we can't even use the return type to disambiguate:
>>> 
>>>   BinaryOperator<T> composed = f.compose(t -> t) // nope
>>> 
>>> The siren song here was the desire for fluency; this was just the wrong place to put a compose() method.  This method interacted with inheritance in two ways; it could be overridden, and it was also desirable for it to have distinguished behavior for distinguished subtypes of its argument types.  This was just too many degrees of freedom, and it painted us into a corner. Function was too general a place to put a default method like compose() that had such potential to interact with inheritance, but we were so excited about the prospects of saying "f.compose(g)" that we didn't think it through.
>>> 
>>> RETURNING TO THE CURRENT ISSUE
>>> 
>>> Now, while you could claim this is not an exact analogy (i.e., transferTo doesn't take a lambda, which was one ingredient of the problems with compose), the reality is that the higher up in the hierarchy you go, the more risky adding defaults is.  There still is a big interaction risk here.
>>> 
>>> In this case, the risk is that both Readable and Appendable are "too general."  In particular, it means that overriding in a subclass won't mean what you might think it means.  Think of the overridings that are actually likely to happen -- those where you know something more about both what kind of Readable and Appendable you've got.
>>> 
>>> Let's say that Reader does:
>>> 
>>>     class Reader {
>>>         long transferTo(Writer out) { ... }
>>>     }
>>> 
>>> This is a totally sensible thing to want to do, since Writer has more flexibility than Appendable, and so such an implementation could be better than the default.
>>> 
>>> The problem is: **this is not an override, but an overload**. Which means that
>>> 
>>>     Reader r = ...
>>>     Writer.w = ...
>>>     r.transferTo(w)
>>> 
>>> will not mean the same thing as
>>> 
>>>     Readable rr = r;
>>>     Appendable ww = w;
>>>     r.transferTo(ww);
>>> 
>>> Because InputStream.transferTo() is not an override of Readable.appendTo().  This is serious puzzler-bait!  Instance method dispatch is not supposed to depend on the static types involved, but that's sure what it looks like is happening here.
>>> 
>>> **IMO, this is a fatal API design error for this method**. Instead, Readable.transferTo() should probably be a static method, so that it does not give the illusion of being overridable.
>>> 
>>> I know default methods on highly abstract interfaces are very tempting, and the siren song of fluency calls to us, but we should be very, very careful about adding default methods to interfaces like Iterable or Readable -- essentially any of the Xxxable interfaces.
>>> 
>>> I think it is much better to make this a static method, and put concrete methods (with better implementations, that can take advantage of bulk write abilities, which Appendable lacks) on Reader as needed.  (InputStream already has this method.)
>>> 
>>> 
>>> On 11/17/2017 1:12 PM, Patrick Reinhart wrote:
>>>> Hi Roger and Alan,
>>>> 
>>>> I incorporated the latest feedback using version 1) from this latest post:
>>>> 
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-November/050004.html
>>>> 
>>>> 
>>>> The actual webrev is here:
>>>> 
>>>> http://cr.openjdk.java.net/~reinhapa/reviews/8066870/webrev.00
>>>> 
>>>> 
>>>> -Patrick
>