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 IO::ARGF#read should always return i32 #10828

Merged
merged 2 commits into from
Jun 17, 2021

Conversation

stakach
Copy link
Contributor

@stakach stakach commented Jun 17, 2021

seeing errors like this on nightly

image

@stakach
Copy link
Contributor Author

stakach commented Jun 17, 2021

This pull is probably not actually useful as I just made a stab at what needed to be changed

@asterite
Copy link
Member

Probably some IO type outside of the std library is returning an Int64. I don't think that's totally wrong, so maybe we should revert this return type annotation

@stakach
Copy link
Contributor Author

stakach commented Jun 17, 2021

related to this pull #10580
it might be related to the definition of SizeT
https://github.com/crystal-lang/crystal/blob/master/src/lib_c/aarch64-linux-musl/c/stddef.cr#L2

@straight-shoota
Copy link
Member

As long as Slice#size is fixed to Int32, there's no point in returning Int64 on IO#read. It's impossible to read more than Int32::MAX. So IMO the type restriction is good.

The real problem however is that the error message points to the abstract def, not the implementation which breaks the contract by returning Int64. That makes it hard to identify the culprit. I thought there was already an issue about that, but I can't find it.

@asterite
Copy link
Member

But this is breaking someone's code. I don't think we are allowed to do that.

@straight-shoota
Copy link
Member

Returning anything else but Int32 is a bug. Adding that type restriction in the abstract def just makes it visible.
If we want to avoid breaking any code that might be working even though it's incorrect, we can't do any fixes at all because it may break something.

@straight-shoota
Copy link
Member

straight-shoota commented Jun 17, 2021

Ooops, sry I incorrectly identified this error as being caused by adding a return type to an abstract def. But this is really not the case in #10580. It adds a type restriction to the implementation. As @stakach suggested, this might be caused by a missing conversion from ˋSizeTˋ. So maybe it's our own code that's broken. We should have noticed that, though because we run CI against aarch64-musl.

@stakach Can you provide information on the target platform and possibly a reduced reproduction? (ˋ--error-traceˋ would already help as well)

@stakach
Copy link
Contributor Author

stakach commented Jun 17, 2021

This is where I'm seeing the error
https://github.com/PlaceOS/driver/runs/2844680637?check_suite_focus=true

Which looks like the fix is to call .to_i32 on a SizeT?

@asterite
Copy link
Member

We can probably call to_i32 on read_count right before we return it, from ARGF.

@asterite
Copy link
Member

Could you try changing that in this PR and see if all specs pass here, and also pass in that ssh library?

@stakach stakach changed the title fix IO::ARGF#read should return Int32 | Int64 fix IO::ARGF#read should always return i32 Jun 17, 2021
@asterite asterite added this to the 1.1.0 milestone Jun 17, 2021
@asterite asterite merged commit d9e64a6 into crystal-lang:master Jun 17, 2021
@straight-shoota
Copy link
Member

This fix is just a workaround, though.

ARGF#read doesn't introduce a Int64 type. It just receives the result from IO#read on the wrapped IO. And IO#read should return Int32. Unfortunately, there is currently no return type restriction on that abstract def. But adding that would be the correct fix.
We probably can't do that until 2.0, though.

But I think we should add a TODO comment and open an issue to track this.

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

Successfully merging this pull request may close these issues.

4 participants