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

Fix or revert some return type restrictions from #10575 #10857

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) : Int32
def read(slice : Bytes)
read_bytes = io.read(slice)
if @mode.read?
digest_algorithm.update(slice[0, read_bytes])
Expand Down
6 changes: 3 additions & 3 deletions src/enumerable.cr
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ module Enumerable(T)
# ```
# [1, 2, 3, 4].count { |i| i % 2 == 0 } # => 2
# ```
def count
def count(& : T -> _) : Int32
Copy link
Member Author

@straight-shoota straight-shoota Jun 28, 2021

Choose a reason for hiding this comment

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

The non-yielding overload (right below) received a Int32 return type restriction in #10582. But this restriction only holds if the yielding overload returns Int32 as well (which is in fact the case). So I figured we should just add it for consistency.

(The block type annotation is an extra, but it's directly derived from #each.)

count = 0
each { |e| count += 1 if yield e }
count
Expand Down Expand Up @@ -624,7 +624,7 @@ module Enumerable(T)
# ```
#
# Returns `nil` if the block didn't return `true` for any element.
def index
def index(& : T -> _) : Int32?
each_with_index do |e, i|
return i if yield e
end
Expand All @@ -638,7 +638,7 @@ module Enumerable(T)
# ```
#
# Returns `nil` if *obj* is not in the collection.
def index(obj)
def index(obj) : Int32?
Copy link
Member Author

Choose a reason for hiding this comment

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

#10582 added a return type restriction Int32? to the override in Array#index (https://github.com/crystal-lang/crystal/pull/10582/files#diff-a12e3cba63cd83b9e746cd9eab9d85f5ddebd05b696133cd494aa83174a48d16R2252). This method delegates to super, so it can only hold if Enumerable#index returns Int32? as well. It's either both or none. Adding the return type restriction to Enumerable doesn't seem like an issue, so I think it's best to just add the restriction there, too (instead of removing it on Array#index).

index { |e| e == obj }
end

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

def read(slice : Bytes) : Int32
def read(slice : Bytes)
ensure_send_continue
super
end
Expand Down Expand Up @@ -58,7 +58,7 @@ module HTTP
def initialize(@io : IO)
end

def read(slice : Bytes) : Int32
def read(slice : Bytes)
ensure_send_continue
@io.read(slice)
end
Expand All @@ -73,7 +73,7 @@ module HTTP
@io.peek
end

def skip(bytes_count) : Int32?
def skip(bytes_count) : Nil
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) : Int32
def read(slice : Bytes)
ensure_send_continue
count = slice.size
return 0 if count == 0
Expand Down Expand Up @@ -164,7 +164,7 @@ module HTTP
peek
end

def skip(bytes_count) : Int32?
def skip(bytes_count) : Nil
ensure_send_continue
if bytes_count <= @chunk_remaining
@io.skip(bytes_count)
Expand Down
2 changes: 1 addition & 1 deletion src/humanize.cr
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ struct Number
end

# :nodoc:
def self.prefix_index(i, group = 3) : Int32
def self.prefix_index(i : Int32, group : Int32 = 3) : Int32
((i - (i > 0 ? 1 : 0)) // group) * group
end

Expand Down
2 changes: 1 addition & 1 deletion 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) : Int32
def read(slice : Bytes)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually not necessary anymore because of #10828 which already added a type cast to Int32.
That's in fact the proper resolution of these type issues (cf. #10855 (comment)).

We should either remove all restrictions (i.e. revert #10828) or cast all return types to Int32.

Copy link
Member

Choose a reason for hiding this comment

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

Could also turn these restrictions into : Int perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

@oprypin IIRC u can't do this for the return type restrictions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any value in that. Either we remove the potentially erroneous restrictions again or make them true.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm convinced we should have the restriction and cast to Int32. It's a non-breaking change and we had already accepted doing that in #10828. We can easily apply that to all similar methods as well.

Question is only if we do that for 1.1 or 1.2. Technically, it's okay because the restrictions are already in place and the change to make sure they hold under all circumstances is a bug fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I took the liberty to include the type casts in this PR. Hope that's fine with everyone. Otherwise we can revert e8cf88e.
Doing the same in #10845 btw.

Copy link
Member

Choose a reason for hiding this comment

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

Hm I see.
In some way this is the long-term-correct approach.
But in another way, it loses us the ability to use Int64 in these cases, which might be needed one day.
The current state is indeed consistent with the rest, so that's good.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're set on Int32 in many places, so I don't think this is really much of an issue.

first_initialize unless @initialized

if current_io = @current_io
Expand Down
2 changes: 1 addition & 1 deletion src/io/hexdump.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class IO::Hexdump < IO
def initialize(@io : IO, @output : IO = STDERR, @read = false, @write = false)
end

def read(buf : Bytes) : Int32
def read(buf : Bytes)
@io.read(buf).tap do |read_bytes|
buf[0, read_bytes].hexdump(@output) if @read && read_bytes
end
Expand Down
2 changes: 1 addition & 1 deletion src/io/memory.cr
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ class IO::Memory < IO
end

# :nodoc:
def skip(bytes_count)
def skip(bytes_count) : Nil
check_open

available = @bytesize - @pos
Expand Down
2 changes: 1 addition & 1 deletion src/io/sized.cr
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class IO::Sized < IO
@read_remaining = read_size.to_u64
end

def read(slice : Bytes) : Int32
def read(slice : Bytes)
Copy link
Contributor

@caspiano caspiano Jun 29, 2021

Choose a reason for hiding this comment

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

Can confirm this annotation was causing an issue as well.

check_open

count = {slice.size.to_u64, @read_remaining}.min
Expand Down
2 changes: 1 addition & 1 deletion src/io/stapled.cr
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class IO::Stapled < IO
end

# Reads a slice from `reader`.
def read(slice : Bytes) : Int32
def read(slice : Bytes)
check_open

@reader.read(slice)
Expand Down
4 changes: 2 additions & 2 deletions src/string.cr
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ class String
# s[-2..-4] # => ""
# s[-2..1] # => ""
# s[3..-4] # => ""
# ``` : String
# ```
def [](range : Range) : String
self[*Indexable.range_to_index_and_count(range, size) || raise IndexError.new]
end
Expand Down Expand Up @@ -5010,7 +5010,7 @@ class String
# Raises an `ArgumentError` if `self` has null bytes. Returns `self` otherwise.
#
# This method should sometimes be called before passing a `String` to a C function.
def check_no_null_byte(name = nil) : String
def check_no_null_byte(name = nil) : self
if byte_index(0)
name = "`#{name}` " if name
raise ArgumentError.new("String #{name}contains null byte")
Expand Down