Skip to content
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

Add return type restrictions to I/O methods #10580

Merged
merged 2 commits into from
Jun 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/compress/deflate/reader.cr
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,12 @@ class Compress::Deflate::Reader < IO
end

# Always raises `IO::Error` because this is a read-only `IO`.
def unbuffered_write(slice : Bytes)
def unbuffered_write(slice : Bytes) : NoReturn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. The abstract method def has no return type (it should probably be Nil, but that's breaking), so I'd probably not add one here, either. Even if it never returns.

Ditto for similar methods. This is up for debate, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I wouldn't edit the abstract method, of course. And I certainly noticed that this is controversial too. In the end, though, I don't see why not add this. It is directly documented right above that Compress::Deflate::Reader#unbuffered_write will never return. We're not saying anything about base classes.

There is a more general decision to make about return types. For example, the vast majority of <=> methods return Int32, even though other values are possible in some cases. And in the same way, I don't see why those methods shouldn't be documented as such. Due to Hyrum's Law (especially well applicable due to the way that Crystal works), people are already using those values as exactly Int32, so it's better to specify it explicitly so that accidental breaking changes are noticed at least.

raise IO::Error.new "Can't write to Compress::Deflate::Reader"
end

# See `IO#read`.
def unbuffered_read(slice : Bytes)
def unbuffered_read(slice : Bytes) : Int32
check_open

return 0 if slice.empty?
Expand Down Expand Up @@ -125,7 +125,7 @@ class Compress::Deflate::Reader < IO
end
end

def unbuffered_flush
def unbuffered_flush : NoReturn
raise IO::Error.new "Can't flush Compress::Deflate::Reader"
end

Expand Down
4 changes: 2 additions & 2 deletions src/compress/deflate/writer.cr
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class Compress::Deflate::Writer < IO
end

# Always raises `IO::Error` because this is a write-only `IO`.
def read(slice : Bytes)
def read(slice : Bytes) : NoReturn
raise "Can't read from Flate::Writer"
end

Expand Down Expand Up @@ -77,7 +77,7 @@ class Compress::Deflate::Writer < IO
end

# Returns `true` if this IO is closed.
def closed?
def closed? : Bool
@closed
end

Expand Down
4 changes: 2 additions & 2 deletions src/compress/gzip/reader.cr
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class Compress::Gzip::Reader < IO
end

# See `IO#read`.
def unbuffered_read(slice : Bytes)
def unbuffered_read(slice : Bytes) : Int32
check_open

return 0 if slice.empty?
Expand Down Expand Up @@ -133,7 +133,7 @@ class Compress::Gzip::Reader < IO
raise IO::Error.new("Can't write to Compress::Gzip::Reader")
end

def unbuffered_flush
def unbuffered_flush : NoReturn
raise IO::Error.new "Can't flush Compress::Gzip::Reader"
end

Expand Down
2 changes: 1 addition & 1 deletion src/compress/gzip/writer.cr
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class Compress::Gzip::Writer < IO
end

# Always raises `IO::Error` because this is a write-only `IO`.
def read(slice : Bytes)
def read(slice : Bytes) : NoReturn
raise IO::Error.new("Can't read from Gzip::Writer")
end

Expand Down
4 changes: 2 additions & 2 deletions src/compress/zip/checksum_reader.cr
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module Compress::Zip
def initialize(@io : IO, @filename : String, verify @expected_crc32 : UInt32? = nil)
end

def read(slice : Bytes)
def read(slice : Bytes) : Int32
read_bytes = @io.read(slice)
if read_bytes == 0
if (expected_crc32 = @expected_crc32) && crc32 != expected_crc32
Expand All @@ -20,7 +20,7 @@ module Compress::Zip
read_bytes
end

def peek
def peek : Bytes?
@io.peek
end

Expand Down
2 changes: 1 addition & 1 deletion src/compress/zip/checksum_writer.cr
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module Compress::Zip
def initialize(@compute_crc32 = false)
end

def read(slice : Bytes)
def read(slice : Bytes) : NoReturn
raise IO::Error.new "Can't read from Zip::Writer entry"
end

Expand Down
6 changes: 3 additions & 3 deletions src/compress/zlib/reader.cr
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class Compress::Zlib::Reader < IO
end

# See `IO#read`.
def unbuffered_read(slice : Bytes)
def unbuffered_read(slice : Bytes) : Int32
check_open

return 0 if slice.empty?
Expand All @@ -81,11 +81,11 @@ class Compress::Zlib::Reader < IO
end

# Always raises `IO::Error` because this is a read-only `IO`.
def unbuffered_write(slice : Bytes)
def unbuffered_write(slice : Bytes) : NoReturn
raise IO::Error.new "Can't write to Compress::Zlib::Reader"
end

def unbuffered_flush
def unbuffered_flush : NoReturn
raise IO::Error.new "Can't flush Compress::Zlib::Reader"
end

Expand Down
2 changes: 1 addition & 1 deletion src/compress/zlib/writer.cr
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class Compress::Zlib::Writer < IO
end

# Always raises `IO::Error` because this is a write-only `IO`.
def read(slice : Bytes)
def read(slice : Bytes) : NoReturn
raise IO::Error.new("Can't read from Gzip::Writer")
end

Expand Down
2 changes: 1 addition & 1 deletion src/digest/io_digest.cr
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class IO::Digest < IO
def initialize(@io : IO, @digest_algorithm : ::Digest, @mode = DigestMode::Read)
end

def read(slice : Bytes)
def read(slice : Bytes) : Int32
read_bytes = io.read(slice)
if @mode.read?
digest_algorithm.update(slice[0, read_bytes])
Expand Down
4 changes: 2 additions & 2 deletions src/file/preader.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ class File::PReader < IO
bytes_read
end

def unbuffered_write(slice : Bytes)
def unbuffered_write(slice : Bytes) : NoReturn
raise IO::Error.new("Can't write to read-only IO")
end

def unbuffered_flush
def unbuffered_flush : NoReturn
raise IO::Error.new("Can't flush read-only IO")
end

Expand Down
28 changes: 14 additions & 14 deletions src/http/content.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,27 @@ module HTTP
class FixedLengthContent < IO::Sized
include Content

def read(slice : Bytes)
def read(slice : Bytes) : Int32
ensure_send_continue
super
end

def read_byte
def read_byte : UInt8?
ensure_send_continue
super
end

def peek
def peek : Bytes?
ensure_send_continue
super
end

def skip(bytes_count)
def skip(bytes_count) : Nil
ensure_send_continue
super
end

def write(slice : Bytes)
def write(slice : Bytes) : NoReturn
raise IO::Error.new "Can't write to FixedLengthContent"
end
end
Expand All @@ -58,22 +58,22 @@ module HTTP
def initialize(@io : IO)
end

def read(slice : Bytes)
def read(slice : Bytes) : Int32
ensure_send_continue
@io.read(slice)
end

def read_byte
def read_byte : UInt8?
ensure_send_continue
@io.read_byte
end

def peek
def peek : Bytes?
ensure_send_continue
@io.peek
end

def skip(bytes_count)
def skip(bytes_count) : Int32?
ensure_send_continue
@io.skip(bytes_count)
end
Expand Down Expand Up @@ -112,7 +112,7 @@ module HTTP
@received_final_chunk = false
end

def read(slice : Bytes)
def read(slice : Bytes) : Int32
ensure_send_continue
count = slice.size
return 0 if count == 0
Expand All @@ -134,7 +134,7 @@ module HTTP
bytes_read
end

def read_byte
def read_byte : UInt8?
ensure_send_continue
next_chunk
return super if @received_final_chunk
Expand All @@ -148,7 +148,7 @@ module HTTP
end
end

def peek
def peek : Bytes?
ensure_send_continue
next_chunk
return Bytes.empty if @received_final_chunk
Expand All @@ -164,7 +164,7 @@ module HTTP
peek
end

def skip(bytes_count)
def skip(bytes_count) : Int32?
ensure_send_continue
if bytes_count <= @chunk_remaining
@io.skip(bytes_count)
Expand Down Expand Up @@ -233,7 +233,7 @@ module HTTP
raise IO::Error.new "Can't write to ChunkedContent"
end

def closed?
def closed? : Bool
@received_final_chunk || super
end
end
Expand Down
4 changes: 2 additions & 2 deletions src/http/server/response.cr
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class HTTP::Server
end

# :nodoc:
def read(slice : Bytes)
def read(slice : Bytes) : NoReturn
raise "Can't read from HTTP::Server::Response"
end

Expand All @@ -117,7 +117,7 @@ class HTTP::Server
end

# Returns `true` if this response has been closed.
def closed?
def closed? : Bool
@output.closed?
end

Expand Down
2 changes: 1 addition & 1 deletion src/http/web_socket/protocol.cr
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class HTTP::WebSocket::Protocol
end
end

def read(slice : Bytes)
def read(slice : Bytes) : NoReturn
raise "This IO is write-only"
end

Expand Down
18 changes: 9 additions & 9 deletions src/io.cr
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ abstract class IO
# Returns `true` if this `IO` is closed.
#
# `IO` defines returns `false`, but including types may override.
def closed?
def closed? : Bool
false
end

Expand Down Expand Up @@ -389,7 +389,7 @@ abstract class IO
#
# "你".bytes # => [228, 189, 160]
# ```
def read_utf8_byte
def read_utf8_byte : UInt8?
if decoder = decoder()
decoder.read_byte(self)
else
Expand Down Expand Up @@ -464,7 +464,7 @@ abstract class IO
end

# Writes a slice of UTF-8 encoded bytes to this `IO`, using the current encoding.
def write_utf8(slice : Bytes)
def write_utf8(slice : Bytes) : Nil
if encoder = encoder()
encoder.write(self, slice)
else
Expand Down Expand Up @@ -500,7 +500,7 @@ abstract class IO
# slice # => Bytes[49, 50, 51, 52, 53]
# io.read_fully(slice) # raises IO::EOFError
# ```
def read_fully(slice : Bytes)
def read_fully(slice : Bytes) : Int32
read_fully?(slice) || raise(EOFError.new)
end

Expand All @@ -515,7 +515,7 @@ abstract class IO
# slice # => Bytes[49, 50, 51, 52, 53]
# io.read_fully?(slice) # => nil
# ```
def read_fully?(slice : Bytes)
def read_fully?(slice : Bytes) : Int32?
count = slice.size
while slice.size > 0
read_bytes = read slice
Expand Down Expand Up @@ -839,7 +839,7 @@ abstract class IO
# io.write_byte 97_u8
# io.to_s # => "a"
# ```
def write_byte(byte : UInt8)
def write_byte(byte : UInt8) : Nil
x = byte
write Slice.new(pointerof(x), 1)
end
Expand All @@ -858,7 +858,7 @@ abstract class IO
# io.rewind
# io.gets(4) # => "\u{4}\u{3}\u{2}\u{1}"
# ```
def write_bytes(object, format : IO::ByteFormat = IO::ByteFormat::SystemEndian)
def write_bytes(object, format : IO::ByteFormat = IO::ByteFormat::SystemEndian) : Nil
object.to_io(self, format)
end

Expand Down Expand Up @@ -1031,7 +1031,7 @@ abstract class IO
end

# :nodoc:
def has_non_utf8_encoding?
def has_non_utf8_encoding? : Bool
!!@encoding
end

Expand Down Expand Up @@ -1157,7 +1157,7 @@ abstract class IO
# stream2 = IO::Memory.new("123")
# IO.same_content?(stream1, stream2) # => true
# ```
def self.same_content?(stream1 : IO, stream2 : IO)
def self.same_content?(stream1 : IO, stream2 : IO) : Bool
buf1 = uninitialized UInt8[1024]
buf2 = uninitialized UInt8[1024]

Expand Down
6 changes: 3 additions & 3 deletions src/io/argf.cr
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class IO::ARGF < IO
@read_from_stdin = false
end

def read(slice : Bytes)
def read(slice : Bytes) : Int32
first_initialize unless @initialized

if current_io = @current_io
Expand All @@ -29,7 +29,7 @@ class IO::ARGF < IO
end

# :nodoc:
def peek
def peek : Bytes?
first_initialize unless @initialized

if current_io = @current_io
Expand Down Expand Up @@ -57,7 +57,7 @@ class IO::ARGF < IO
raise IO::Error.new "Can't write to ARGF"
end

def path
def path : String
@path || @argv.first? || "-"
end

Expand Down
Loading