Add and update specs for IO methods - #1384
Conversation
Verify that calling IO#binmode on an IO stream disables any active newline conversion (such as universal newline mode) when reading data.
- Add spec for reading a byte with IO#getbyte after calling ungetc on a stream without character conversion.
- Add spec verifying that calling IO#getbyte after ungetc on a stream with character conversion enabled raises IOError ("byte oriented read for character buffered IO").
- Add spec verifying IO#internal_encoding returns nil when external encoding is BINARY. - Add Ruby version-specific specs for setting internal encoding when external encoding is BINARY: Ruby < 3.3 returns the assigned value, while Ruby >= 3.3 ignores the internal encoding and returns nil because binary mode does not support internal character conversion.
Verify that calling IO#pwrite with a negative offset raises Errno::EINVAL.
- Add spec for CRuby Bug #18421 (fixed in 3.0.4) ensuring IO#read_nonblock(0, buffer) clears the output buffer and returns it empty when length is 0.
- Update IOError expectation message check when read_nonblock is called after ungetc with character conversion enabled ("byte oriented read for character buffered IO").
- Add specs verifying that IO#read successfully reads bytes after calling ungetc on streams without character conversion.
- Add specs across various mode combinations verifying that IO#read raises IOError ("byte oriented read for character buffered IO") after ungetc when character conversion is enabled.
- Add spec for IO#readbyte after ungetc on a stream without character conversion.
- Add spec verifying that IO#readbyte raises IOError ("byte oriented read for character buffered IO") after ungetc when character conversion is enabled.
…er conversion
Verify that calling IO#readpartial after ungetc on an IO stream with character conversion enabled raises IOError ("byte oriented read for character buffered IO").
- Add spec verifying IO#reopen raises ArgumentError when passed excess arguments.
- Add specs for reopening an IO with a String path, verifying that the newly opened stream preserves the capabilities and restrictions of the original open mode ('r', 'w', 'r+', 'a').
When ungetc pushes characters into the IO stream's character buffer, calling IO#seek should discard that buffer before seeking to the target offset. This adds a spec for CRuby Bug #20919 fixed in Ruby 3.4.
- Refactor `IO#set_encoding when passed nil, nil` mode blocks ('r', 'rb', 'r+', 'w', 'w+', 'a', 'a+') to reuse shared behaviors `:io_set_encoding_common`, `:io_set_encoding_readonly`, and `:io_set_encoding_write`.
- Add spec for passing "-" as second argument to reset internal_encoding to nil.
- Add validation specs for non-ASCII compatible arguments, nil first argument with non-nil second argument, and invalid newline decorator option.
- Add specs for ASCII-incompatible encodings (handling with binmode, character conversion, write-only streams, and error raising on readable streams).
- Add Ruby 3.2+ spec for newline decorator in binary mode.
- Add Ruby 3.3+ specs for setting internal encoding when second argument is nil.
- Add spec for encoding names containing null bytes.
- Update IOError message expectation when sysread is called after buffered reads.
- Add spec verifying that calling sysread on an IO with a non-empty character buffer raises IOError ("byte oriented read for character buffered IO").
- Update IOError message expectation when sysseek is called after buffered reads.
- Add spec verifying that calling sysseek on an IO with a non-empty character buffer raises IOError ("sysseek for buffered IO").
- Refactor pipe test fixture setup to use before/after blocks for clean tear-down. - Add specs for Ruby < 3.2 verifying that non-blocking syswrite on a full pipe raises Errno::EAGAIN (or Errno::EWOULDBLOCK on Windows) instead of returning partial bytes.
- Add spec for IO#write writing binary data when external encoding is ASCII-8BIT. - Add specs verifying that IO#write performs newline conversion on binary data when no encoding or ASCII-8BIT encoding is specified alongside newline conversion options.
The implementation on Windows does not error in this case
andrykonchin
left a comment
andrykonchin
left a comment
There was a problem hiding this comment.
Will continue review later.
| -> do | ||
| @io.getbyte | ||
| end.should.raise(IOError, "byte oriented read for character buffered IO") | ||
| end |
There was a problem hiding this comment.
minor: this appears to be a CRuby-specific implementation detail (or an intentional design constraint) that other Ruby implementations, such as JRuby, TruffleRuby, or Natalie, might handle differently (e.g., by
permitting byte-oriented reading under these conditions).
CRuby already allows mixing byte-oriented and character-oriented read operations on streams with multibyte encodings (such as UTF-16BE). Semantically, calling ungetc does not make this behavior any worse or introduce additional issues.
There was a problem hiding this comment.
My understanding is that this project documents the behavior of CRuby, and this is indeed a CRuby behavior. I've not tried it on other implementations. Is that a requirement now?
There was a problem hiding this comment.
My understanding is that this project documents the behavior of CRuby
Not exactly, as the README says:
The Ruby Spec Suite, abbreviated ruby/spec, is a test suite for the behavior of the Ruby programming language.
Which notably means avoiding to add specs for things that are bugs in CRuby (and instead report the bug/inconsistency), ask clarification on the CRuby bug tracker for unclear cases and otherwise yes mostly assume CRuby behavior is intended if it looks reasonable.
I think this is a case of "unclear, should ask clarification on the CRuby bug tracker".
Would you mind opening a Misc issue at https://bugs.ruby-lang.org/ to ask the reason for this error and detail there how it's inconsistent?
I think for now it's better to not add these specs in ruby/spec, because the effect would be probably unclear semantics and extra overhead for other Ruby implementations, when that behavior might be undesirable anyway (could probably be even seen as a bug by someone trying to do legitimately).
Could you remove them for this PR?
There was a problem hiding this comment.
Playing a bit in IRB with this and yeah it's weird, I could maybe understand if byte operations are not allowed with UTF-16/32 (which are the only encodings with min-length > 1 byte), although that would be inconsistent with String which doesn't mind.
But it's not even that, that's allowed, it's indeed after ungetc it's not allowed.
That starts to feel like a bug.
There was a problem hiding this comment.
Ah BTW there was already 1 spec about this (it "raises an exception after ungetc with data in the buffer and character conversion enabled" do) let's remove it too
There was a problem hiding this comment.
I will open tickets as requested, but I've had mixed results getting attention on some of them. This ticket is one such which is being ignored. Is there something else I should do to avoid these new tickets also being ignored?
Yes sometimes people answer and sometimes they don't, maybe because nobody knows or remember.
One possibility is to add a ticket to a DevMeeting, then usually someone will take a look at it.
The next dev meeting isn't scheduled yet though.
There was a problem hiding this comment.
Looking at READ_CHAR_PENDING/READ_DATA_PENDING in CRuby and them having different buffers seems very much like accidental complexity to me, not something we'd want to spec, but some implementation artifact from old Ruby versions. The Ruby IO API is also known as one of the most messy.
There was a problem hiding this comment.
Unpopular opinion but maybe ungetc/ungetbyte should be deprecated/removed, it adds significant complexity for little value and little need. I'm not hopeful though.
Read buffering itself is fine and needed e.g. for IO#readline, but being able to mutate the buffer though ungetc/ungetbyte that seems quite brittle.
There was a problem hiding this comment.
That part I don't know so well, when one does ungetc is there any encoding conversion?
No, the bytes passed to ungetc are dropped directly into the internal character buffer, which is separate from the internal byte buffer. The idea is that you get those bytes back when calling getc or any other character oriented read operation. You can even intentionally put an invalid character back via ungetc, so it's not even possible to always reverse the character conversion of the stream in order to drop the bytes into the byte buffer.
In order to attempt a reverse conversion, the behavior of ungetc would have to change to raise a conversion error when the conversion fails. It never does that now, so that would risk breaking existing code in surprising ways. Then there's the cost of creating a new character convertor for the reverse operation and risk of state loss when resetting the state of the forward character convertor after pushing converted bytes back into the byte buffer. This quickly becomes a can of worms.
I think we're approaching the point of identifying these specs as undefined behavior unless the Ruby community sees value in aligning on an implementation.
There was a problem hiding this comment.
Looking at READ_CHAR_PENDING/READ_DATA_PENDING in CRuby and them having different buffers seems very much like accidental complexity to me, not something we'd want to spec, but some implementation artifact from old Ruby versions.
I think it's deliberate complexity given my earlier message. It provides the least surprise in the more common case where people don't mix character and byte oriented operations on the same IO object.
The Ruby IO API is also known as one of the most messy.
I'm far too aware of that mess. I've spent years of effort attempting to make a faithful, pure Ruby implementation of IO operations in the io-like gem. I've been successful, which is why I have all these new specs for weird corner cases.
Unpopular opinion but maybe ungetc/ungetbyte should be deprecated/removed, it adds significant complexity for little value and little need.
People implementing parsers at least would probably disagree with you. It's common to leverage the read buffer as a form of lookahead by putting back bytes or characters that you just peeked at in order to make a decision.
being able to mutate the buffer though ungetc/ungetbyte that seems quite brittle.
Whether brittle or not, I used ungetc in these tests as a way to ensure that the character buffer has predictable content. There may be other ways, but this was the easiest to prove and use. The point is that the character buffer may have content in it. That content is derived from the byte buffer, so bytes were consumed to provide the character buffer content.
The "right" solution for a byte oriented read operation in that condition is debatable, but it's hard to imagine the CRuby maintainers changing their decision at this point. Nothing less than a complete overhaul of the IO class would justify that sort of change.
javanthropus
left a comment
javanthropus
left a comment
There was a problem hiding this comment.
Thanks for taking a look, @andrykonchin.
| -> do | ||
| @io.getbyte | ||
| end.should.raise(IOError, "byte oriented read for character buffered IO") | ||
| end |
There was a problem hiding this comment.
My understanding is that this project documents the behavior of CRuby, and this is indeed a CRuby behavior. I've not tried it on other implementations. Is that a requirement now?
…coding_spec.rb This was suggested as it seemed to apply more with similar existing specs there
This code may have been the result of conflict resolution over the years I've had this in my backlog of work.
|
Thank you! |
|
Thanks for the review and approval, @andrykonchin. When will this be merged? |
|
Hi, thanks for following up! A few items need a quick maintainer discussion before we can merge. We'll update you shortly! |
eregon
left a comment
eregon
left a comment
There was a problem hiding this comment.
Thank you, looks great overall.
As discussed #1384 (comment) I think we shouldn't add specs for byte oriented read for character buffered IO IOError since that seems inconsistent and very low value compatibility to do the same on other Ruby implementations.
Could you remove them?
You could do so in a separate commit so if we'd ever change our mind on this we can easily restore those.
(there is some back-and-forth in these commits, but the commit messages are helpful and it's nice for review so I plan to merge with rebase, i.e. not squashing)
| -> do | ||
| @read.read_nonblock(3) | ||
| end.should.raise(IOError, "byte oriented read for character buffered IO") | ||
| end |
There was a problem hiding this comment.
@eregon, while removing the tests as you requested, this one caught my eye because it's a pre-existing test that does exactly the same thing. All I did here was clean it up and add a check for the error message in addition to the exception type.
In light of this test having already been accepted back in 2022, maybe it's safe to say that other Ruby implementations already have this logic correctly implemented. If you still think that this class of test should be removed, then I think it best to remove this one as well. What do you think?
There was a problem hiding this comment.
I'd remove this one too, like in #1384 (comment).
If it's many of them then yeah let's reconsider but if a handful let's just remove those.
If you miss some it's also not a big deal.
maybe it's safe to say that other Ruby implementations already have this logic correctly implemented
TruffleRuby doesn't have it, Rubinius doesn't have it.
JRuby seems to have some of it due to copying that code from CRuby, but I don't think they have given it much thought.
| it "raises an error when called with a non-empty character buffer" do | ||
| @file.set_encoding(Encoding::UTF_8, Encoding::UTF_16BE) | ||
| @file.ungetc("a".encode(Encoding::UTF_16BE)) | ||
| -> do | ||
| @file.sysread(5) | ||
| end.should.raise(IOError, "byte oriented read for character buffered IO") | ||
| end |
There was a problem hiding this comment.
@eregon, this test seems to be the same class as the others you want removed for now. It feels lower level to me though, so please confirm if this should also be removed.
There was a problem hiding this comment.
Yes I think anything that checks for a byte oriented read for character buffered IO IOError shouldn't be spec'd at this point.
3 participants
This PR adds missing specs, updates error message expectations, refactors redundant test fixtures, and adds coverage for edge cases and version-specific behaviors across
core/io.Summary of Changes
IO#binmode: Added spec verifying that newline conversion is disabled when reading in binary mode.IO#getbyte,IO#read,IO#readbyte,IO#readpartial: Added specs for reading afterungetc, including specs verifying that byte-oriented methods raiseIOError("byte oriented read for character buffered IO") when character conversion is enabled.IO#internal_encoding: Added specs forBINARYexternal encoding, including Ruby 3.3+ behavior where internal encoding assignments are ignored.IO#pwrite: Added spec verifyingErrno::EINVALis raised on negative offsets.IO#read_nonblock: Added spec for CRuby Bug #18421 (fixed in 3.0.4) ensuring 0-length reads clear the buffer, and updatedIOErrorexpectation.IO#reopen: Added specs for excess argument validation and mode preservation (r,w,r+,a) when reopening with a String path.IO#seek: Added spec for CRuby Bug #20919 (fixed in 3.4) verifying character buffer is cleared afterungetc.IO#set_encoding: Refactoredset_encoding nil, nilmode blocks to reuse shared behaviors. Added specs for"-"internal encoding, ASCII compatibility validation, newline options, ASCII-incompatible encodings, and Ruby 3.2+/3.3+ version specific behaviors.IO#sysread,IO#sysseek: UpdatedIOErrorexpectation messages for buffered IO and added specs for non-empty character buffers.IO#syswrite: Refactored pipe test setup and added specs for non-blocking write behavior on full pipes (raisingErrno::EAGAIN/Errno::EWOULDBLOCKon Ruby < 3.2).IO#write: Added specs for writing binary data withASCII-8BITexternal encoding and newline conversion.Verification
Ran specs via
mspec core/io/:103 files, 1582 examples, 2459 expectations, 0 failures, 0 errors, 0 tagged