Add type restrictions to Digest directory #15696
Conversation
src/digest/adler32.cr
Outdated
| update(data, initial) | ||
| end | ||
|
|
||
| def self.update(data, adler32 : UInt32) : UInt32 | ||
| def self.update(data : Slice(UInt8) | String, adler32 : UInt32) : UInt32 |
There was a problem hiding this comment.
Here is where some sort of trait mechanism would have shined - what really is accepted is slices and anything that can be converted to a slice by implementing to_slice. I don't think it is obvious that the latter should necessary be reduced to just strings.
Aside, not related to PR but to the code in general: Here I'd have preferred to have two method variants, one that is locked down to only slice and don't try to call to_slice on it, and one that is more liberal that try to convert and call the other. (If we ever get endless defs then this is a place where those would be great)
There was a problem hiding this comment.
I agree to both
For this PR, I'd suggest to leave out the type restriction. It's too strict. And we don't have a means to express the actual type requirement accurately (cf. #15682 (comment)).
There was a problem hiding this comment.
We should split the method: one with the implementation that takes a Bytes (Slice(UInt8)) and another that will cast #to_slice?
def self.update(data, adler32 : UInt32)
update(data.to_slice, adler32)
end
def self.update(data : Bytes, adler32 : UInt32)
# actual implementation
endThe advantage is making sure #to_slice does indeed return a Slice(UInt8) not a Slice(UInt16) or Slice(Char).
The same should be applied to other Digest and Crypto and other places in stdlib that expects bytes. Maybe not in this PR, or not, so we have a potentially breaking change in a focused PR.
src/digest/digest.cr
Outdated
| @@ -21,7 +21,7 @@ abstract class Digest | |||
| # The modules adds convenient class methods as `Digest::MD5.digest`, `Digest::MD5.hexdigest`. | |||
| module ClassMethods | |||
| # Returns the hash of *data*. *data* must respond to `#to_slice`. | |||
| def digest(data) : Bytes | |||
| def digest(data : String) : Bytes | |||
There was a problem hiding this comment.
Same here. The comment above it directly contradict the type marker!
There was a problem hiding this comment.
You know, and be a String ;) (will remove restriction)
src/digest/digest.cr
Outdated
| @@ -85,7 +85,7 @@ abstract class Digest | |||
| # | |||
| # Digest::SHA1.base64digest("foo") # => "C+7Hteo/D9vJXQ3UfzxbwnXaijM=" | |||
| # ``` | |||
| def base64digest(data) : String | |||
| def base64digest(data : String) : String | |||
src/digest/crc32.cr
Outdated
| update(data, initial) | ||
| end | ||
|
|
||
| def self.update(data, crc32 : UInt32) : UInt32 | ||
| def self.update(data : Slice(UInt8) | String, crc32 : UInt32) : UInt32 |
There was a problem hiding this comment.
issue: This is the same as #15696 (comment). We should drop the restriction because it's too strict.
| def self.update(data : Slice(UInt8) | String, crc32 : UInt32) : UInt32 | |
| def self.update(data, crc32 : UInt32) : UInt32 |
There was a problem hiding this comment.
This PR is not my lucky PR 😅 Yup, will remove it
src/digest/crc32.cr
Outdated
| @@ -17,16 +17,16 @@ class Digest::CRC32 < ::Digest | |||
| LibZ.crc32(0, nil, 0).to_u32 | |||
| end | |||
|
|
|||
| def self.checksum(data) : UInt32 | |||
| def self.checksum(data : String) : UInt32 | |||
There was a problem hiding this comment.
Even this constraint is too strict: it calls #update internally, and thus can take whatever responds to #to_slice.
This is the output of compiling cr-source-typer and running it with the below incantation:
This is related to #15682 .