-
-
Notifications
You must be signed in to change notification settings - Fork 401
Add and update specs for IO methods #1384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
4a7d459
976d046
6f9fba9
a8dd789
d91bdf7
40e859f
3a69b22
f0a3b5f
46f9838
538111f
950b6cc
09eb335
18069f9
e1b1972
c9449cb
5694592
7008f3b
0749bb7
40f33d2
b9fa9e5
5309d09
68eaaf0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,22 @@ | |
| it "raises an IOError on closed stream" do | ||
| -> { IOSpecs.closed_io.getbyte }.should.raise(IOError) | ||
| end | ||
|
|
||
| it "reads after ungetc without character conversion" do | ||
| @io.set_encoding("utf-8") | ||
| c = @io.getc | ||
| @io.ungetc(c) | ||
| @io.getbyte.should == 86 | ||
| end | ||
|
|
||
| it "raises an exception after ungetc with character conversion" do | ||
| @io.set_encoding("utf-8:utf-16be") | ||
| c = @io.getc | ||
| @io.ungetc(c) | ||
| -> do | ||
| @io.getbyte | ||
| end.should.raise(IOError, "byte oriented read for character buffered IO") | ||
| end | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 CRuby already allows mixing byte-oriented and character-oriented read operations on streams with multibyte encodings (such as UTF-16BE). Semantically, calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not exactly, as the README says:
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". 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).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah BTW there was already 1 spec about this (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes sometimes people answer and sometimes they don't, maybe because nobody knows or remember.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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. |
||
| end | ||
|
|
||
| describe "IO#getbyte" do | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,14 +66,16 @@ | |
| @read.read_nonblock(3).should == "bar" | ||
| end | ||
|
|
||
| it "raises an exception after ungetc with data in the buffer and character conversion enabled" do | ||
| it "raises an exception after ungetc with character conversion enabled" do | ||
| @write.write("foobar") | ||
| @read.set_encoding( | ||
| 'utf-8', universal_newline: true | ||
| ) | ||
| c = @read.getc | ||
| @read.ungetc(c) | ||
| -> { @read.read_nonblock(3).should == "foo" }.should.raise(IOError) | ||
| -> do | ||
| @read.read_nonblock(3) | ||
| end.should.raise(IOError, "byte oriented read for character buffered IO") | ||
| end | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd remove this one too, like in #1384 (comment).
TruffleRuby doesn't have it, Rubinius doesn't have it. |
||
|
|
||
| it "returns less data if that is all that is available" do | ||
|
|
@@ -137,6 +139,14 @@ | |
| -> { @read.read_nonblock(5) }.should.raise(EOFError) | ||
| end | ||
|
|
||
| ruby_bug "#18421", ""..."3.0.4" do | ||
| it "clears and returns the given buffer if the length argument is 0" do | ||
| buffer = String.new("existing content") | ||
| @read.read_nonblock(0, buffer).should == buffer | ||
| buffer.should == "" | ||
| end | ||
| end | ||
|
|
||
| it "preserves the encoding of the given buffer" do | ||
| buffer = ''.encode(Encoding::ISO_8859_1) | ||
| @write.write("abc") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.