Hi Reinier,
I see you asked this here as well as on Stack Overflow. I'll repeat my answer from
Stack Overflow here and then continue with some additional discussion.
Quick recap: Collection::toArray has a requirement that the returned array be
"safe", whereas Stream::toArray has no such requirement. In my earlier Stack
Overflow answer (which you quoted) I mentioned something like "if [Stream.toArray]
were to violate its spec", and you asked, what spec?
My answer:
I probably mistakenly assumed that the Stream::toArray spec had the same "safety"
requirements as Collection::toArray. That was our intent, as I recall, but it
didn't make it into the spec.
Now to your questions in this email.
The second question, where to report this, is right here! We can discuss this and
file bugs/PRs etc. if necessary depending on the outcome of the discussion.
The first question has some subtlety to it. Should we not update the specification
for Stream::toArray to have the same "safety" requirement as Collection::toArray?
I guess a change would serve to clarify the specification. The developer of a
clean-room implementation of streams might reasonably think that, if the stream
source is an array, that this array can simply be returned from toArray(). However,
that violates the intent (which is pretty clear in my memory) that the returned
array be freshly created. Usually, adding assertions to interfaces creates a danger
of invalidating existing implementations. However, I don't think returning an
existing array, especially one that might be aliased, was ever considered to be
valid behavior. Thus, a spec change would be more like filling a gap that shouldn't
have been there in the first place, not adding a new semantic requirement.
As a practical matter, there are few third-party implementations, this requirement
is almost impossible to test, and any callers of toArray() won't change. That is, if
they're currently using the returned array, they'll continue using it; but if they
want 100% assurance the array isn't aliased, they'll continue to make a defensive
copy. So, it doesn't seem like changing the spec will have any practical impact.
Have I answered your questions?
s'marks
On 12/14/23 5:46 AM, Reinier Zwitserloot wrote:
Hi core libs team,
I think I found a rather inconsequential and esoteric bug, though the term is
somewhat less objectively defined when the problem exists solely in how complete
some method’s javadoc is.
Two questions: Is there a plausible argument that the javadoc as is, shouldn’t be
updated? If not, what’s the right place to report this javadoc ‘bug’?
The issue:
A snippet of the javadoc of Collection.toArray, from current HEAD jdk22u:
<p>The returned array will be "safe" in that no references to it are
* maintained by this collection. (In other words, this method must
* allocate a new array even if this collection is backed by an array).
* The caller is thus free to modify the returned array.
The javadoc of Stream.toArray, from current HEAD jdk22u - has no such rider
anywhere in its documentation nor on the javadoc of the Stream interface, nor on
the package javadoc. The javadoc is quite short; the complete docs on toArray():
/**
* Returns an array containing the elements of this stream.
*
* <p>This is a <a href="package-summary.html#StreamOps">terminal
* operation</a>.
*
* @return an array, whose {@linkplain Class#getComponentType runtime
component
* type} is {@code Object}, containing the elements of this stream
*/
The more usually used variant taking a function that makes the array has slightly
longer javadoc, but it similarly makes no mention whatsoever about the contract
stipulation that any implementors must not keep a reference.
A snippet from Stuart Marks on a stack overflow question about a to the asker
weird choice about stream’s toList()’s default implementation
(https://stackoverflow.com/questions/77473755/is-it-necessary-to-copy-a-list-to-be-safe/77474199?noredirect=1#comment136909551_77474199):
@Holger The extra step in the default implementation is there to force a defensive
copy if |this.toArray| were to violate its spec and keep a reference to the
returned array. Without the defensive copy, it would be possible to modify the
list returned from the default |toList| implementation.
Which leads to the obvious question: Where is that ’spec’ that Stuart is referring
to? Either the javadoc is the spec and therefore the javadoc is buggy, in that it
fails to mention this stipulation, or the spec is elsewhere, in which case surely
the javadoc should link to it or copy it.
Possibly this is jus filed away as: “Unlike with Collection, Stream instances are
disposable; after a terminal operation (and toArray is terminal) has been invoked
on it, that stream object has ceased being useful. Therefore, there is no
perceived value to caching any created array, therefore, it doesn’t need mentioning".
Except, as per Stuart’s comment, actual OpenJDK code is written partly to deal
with any violators of this invisible spec, and the discrepancy (where
Collection.toArray explicitly mentions the contract stipulation that toArray()
must make safe arrays, but Stream’s toArray() does not) suggests a fundamental
difference where none exists (in fact, literally: Apparently its a spec violation
if your Stream implementation return a non-safe array from toArray!)
--Reinier Zwitserloot
