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

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Apr 3, 2021

No description provided.

@@ -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.

@oprypin
Copy link
Member Author

oprypin commented Jun 4, 2021

This is not approved BTW

@straight-shoota straight-shoota merged commit 9c5390d into crystal-lang:master Jun 7, 2021
@asterite
Copy link
Member

Can we revert this one? It introduced a breaking change because some shards (or maybe the std?) is returning Int64 instead of Int32 for the unbuffered_read method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants