Add type restrictions to io#15698
Conversation
src/io.cr
Outdated
|
|
||
| # Same as `pos`. | ||
| def tell | ||
| def tell : Int64 | Int32 |
There was a problem hiding this comment.
This look very odd to me. Especially as pos is noreturn. Where does this union type come from?
There was a problem hiding this comment.
I see Memory#pos return Int32 and Buffered#pos return Int64. The side effect being this wrapping def indirectly inherits the return types of those methods 🤷
Maybe better to omit the return type restriction in this case?
There was a problem hiding this comment.
Either that, or declare it to be typeof(pos), if that is possible 🤔
src/io/file_descriptor.cr
Outdated
| def blocking=(value : Bool) : Int32? | ||
| self.system_blocking = value | ||
| end |
There was a problem hiding this comment.
These combination of input and return types should not be correct. The return type of an assignment should be the value assigned. Does system_blocking= do anything weird?
There was a problem hiding this comment.
system_blocking= may leak the return value of fcntl.
We should probably return value (c.f. #10083).
For this PR, I'd suggest to just set the return type restriction to Nil.
There was a problem hiding this comment.
Per #10083, I could also update this method to return Bool and put value at the end?
Edit: Scratch that, I'll just do what you suggest in this case, and let that issue take care of this :)
There was a problem hiding this comment.
Yes, it's best to keep this PR focused on adding type restrictions, without changing anything else.
| end | ||
|
|
||
| def self.check_invalid(invalid) : Nil | ||
| def self.check_invalid(invalid : Symbol?) : Nil |
There was a problem hiding this comment.
Thought (non-blocking): we should refactor to use an enum instead of a symbol. That would validate the value at comptime, instead of delaying to runtime, and it would be backward compatible, since symbols are automatically transformed into enums.
class IO
struct EncodingOptions
enum Invalid
Skip
end
getter name : String
getter invalid : Invalid?
def initialize(@name : String, @invalid : Invalid?)
end
end
ysbaddaden
left a comment
ysbaddaden
left a comment
There was a problem hiding this comment.
There are still a few details!
`#system_close_on_exec=` (and thus `#close_on_exec=`) returns `Nil` on Windows. But #15698 added a type-restriction to `Bool`. We didn't notice this discrepancy before, because this method was never called on Windows until we enabled the specs for that in #14716. Changing the return value to `false` on Windows should be the correct behaviour. If the parameter is `true`, the method raises. This is a classic integration issue. It could've been avoided by testing each change against current master directly before merging it. But luckily they are pretty rare, so there's no urgent need to do something about it.
7 participants
This is the output of compiling cr-source-typer and running it with the below incantation:
This is related to #15682 .
I also reran the above command but without the
--union-size-threshold 2, and updated several long union types toIOby itself.