Fix #5014: Implement java.io.BufferedWriter #5015
Conversation
Implementation copied from Scala-native
|
Completed the CLA process |
|
When we port code from Scala.js to Scala Native, we typically put the following to give some context and a starting point. Not sure if Scala.js wants to use this convention or not. Note: we don't have a special header but this is verbatim from // Ported from Scala.js commit: 2253950 dated: 2022-10-02
// Note: this file has differences noted below
/*
* Scala.js (https://www.scala-js.org/)
*
* Copyright EPFL.
*
* Licensed under Apache License 2.0
* (https://www.apache.org/licenses/LICENSE-2.0).
*
* See the NOTICE file distributed with this work for
* additional information regarding copyright ownership.
*/ |
| def flush(): Unit = { | ||
| ensureOpen() | ||
| out.write(buffer, 0, pos) | ||
| out.flush() |
There was a problem hiding this comment.
I cannot infer this behavior from the JDK documentation. Maybe we should add an explicit test to ensure the behavior is aligned?
There was a problem hiding this comment.
I read
Thus one flush() invocation will flush all the buffers in a chain of Writers and OutputStreams
| override def write(s: String, off: Int, len: Int): Unit = | ||
| write(s.toCharArray, off, len) | ||
|
|
||
| def write(cbuf: Array[Char], off: Int, len: Int): Unit = { |
There was a problem hiding this comment.
From the JDK doc:
If the requested length is at least as large as the buffer, however, then this method will flush the buffer and write the characters directly to the underlying stream.
IIUC this behavior isn't implemented.
There was a problem hiding this comment.
I reimplemented the method
| if (available >= len) { | ||
| System.arraycopy(cbuf, off, buffer, pos, len) | ||
| pos += len | ||
| if (pos == sz) flush() |
There was a problem hiding this comment.
This will also flush the underlying stream. Are you sure this is the right behavior?
There was a problem hiding this comment.
Also (IIRC) Scala.js style guide dictates that side-effecting calls are on their own line.
There was a problem hiding this comment.
If the flush action is correctly flushing the whole chain then flushing on write when write directly to the chain writer makes sense. Note: the new code is a bit different but still flushes the chained writer when writing directly to it.
| pos += len | ||
| if (pos == sz) flush() | ||
| } else { | ||
| write(cbuf, off, available) |
There was a problem hiding this comment.
Instead of calling the method recursively, have you considered a helper:
val available = sz - pos
if (available >= len) {
writeToBuf(cbuf, off, buffer, len)
} else {
writeToBuf(cbuf, off, buffer, available)
writeToBuf(cbuf, off + available, len - available)
}
(probably necessary anyways to build the "flush the buffer and write directly" behavior.
There was a problem hiding this comment.
I don't think it needs recursion. Reimplemented it to where it write to the buffer or it flushes the buffer and writes the all to the chained writer (as this is the behaviour I interpret from the JDK spec).
Ordinarily this method stores characters from the given array into this stream's buffer, flushing the buffer to the underlying stream as needed. If the requested length is at least as large as the buffer, however, then this method will flush the buffer and write the characters directly to the underlying stream. Thus redundant BufferedWriters will not copy data unnecessarily.
| write(System.lineSeparator(), 0, System.lineSeparator().length) | ||
|
|
||
| override def write(c: Int): Unit = | ||
| write(Array(c.toChar), 0, 1) |
There was a problem hiding this comment.
IIUC there is always at least one spot free in the buffer (otherwise it would be flushed).
So couldn't this be:
buffer(pos) = c.toChar
pos += 1
if (pos == len)
flush()
test-suite/shared/src/test/scala/org/scalajs/testsuite/javalib/io/BufferedWriterTest.scala
Outdated
Show resolved
Hide resolved
|
|
||
| import org.scalajs.testsuite.utils.AssertThrows.assertThrows | ||
|
|
||
| class BufferedWriterTest { |
There was a problem hiding this comment.
We'll probably have to add more tests, notably (probably not a complete list, sorry):
- operations on closed writers throw
- index out of bounds use cases
- corner cases pointed out in the implementation review (notably flushing behavior)
- writing to a buffered writer multiple times (with different buffer boundary conditions).
There was a problem hiding this comment.
I have added more test cases.
|
@gzm0 thanks for your extensive review of this code. As I mentioned the code is copied from Scala-native, it is 1:1 the same. I will go through your comments and see if the code can be improved. But I have some concern that if we make changes, the behaviour could be different than that of Scala-native. It would be nice if the can both benefit from the same implementation. Would it be possible Scala-js and Scala-native somehow share sources for these things? |
That doesn't matter. The reference is the JVM implementation. Both Scala.js and Scala Native must follow the specced behavior of the JDK. Scala.js, being more mature, is more demanding in terms of adhering to it.
No, that's not possible nor desirable. Copy-pasting is the way to go. Usually copying from JS to Native will be fine. Copying in the other direction tends to meet the higher quality standards of Scala.js, as you are experiencing now. |
|
@gzm0 I have done another iteration, ready to review again. |
|
did sign the CLA (again) |
|
The AppVeyer CI seems be pass but if I check the logs than I see many 'failed'.... |
|
The Jenkins CI jobs show the command that failed at the end of the log. In this case it seems the JVM test suite is failing: You should be able to replicate locally with |
|
Interesting, so the JDK spec says write nothing where the JVM implementation throws an StringIndexOutOfBoundsException exception. |
|
If the JVM implementation is in direct contradiction with its (latest) spec, it might be a bug on the JVM. Consider searching the JDK bug database. However, recall that you are not allowed to even look at the JDK implementation nor their tests.. If it's a known issue on the JVM we can exclude the test on the JVM with a comment to the JDK bug. |
|
Looks like it the was was reported before and 'fixed', so JVM should not throw for negative values but it still does 🤔 |
|
But if we suspect a bug in the JVM code we can also not look at the source code and try to identify/fix it? Sounds silly... |
|
The JVM test also deviates in writing contents largen than the (available) buffer partially to the underlying stream. So it unnecessary splits/copies data. The is not what I interpret from the docs. |
|
I get the feeling the JDK is quite open to interpretation (or implemented wrong or not updated often) 🤣 . This also does make me a little sad 😢 |
This is actually quite rare. The JDK is remarkably well specified overall. If the latest JVM implementation still contradicts the spec, it might be worth opening a new issue, if only so that we can reference it in our tests to justify why we disagree with the JVM.
Such is our life. You consider it silly; we see potential lawsuits if we don't play by the rules. We can report bugs, but not fix them nor investigate from the source. Essentially we have to treat the JDK as (free-as-in-beer) proprietary software. |
|
Silly from their perspective I mean, as in it discourages people from contributing. I understand the position of Scala-js. |
|
But for this PR.. will these differences be a blocker for now? Ignore those tests? Or should we replicate the JVM behaviour? |
|
It's not a blocker, but it should be documented as a JDK bug. See for example and for examples. |
|
I think I will not continue this PR. I think it is better to skip to jdk classes and build pure Scala alternatives. It is also not satisfying that whatever was going to be implemented here would always be wrong because the spec is ambiguous or by not matching other openjdk implementations behaviour. |
Implementation copied from Scala-native
I excluded any copyright headers Scala-js uses in all files because this code comes from Scala-native