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

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jun 28, 2021

This PR fixes some issues with return type restrictions added in the first batch of #10575.

A couple of IO#read implementations needed to remove Int32 because we can't be sure that the wrapped IO implementations return Int32 (continuation of #10855). The restrictions stay, we're casting the value instead.

Return type restrictions are added to a couple of Enumerable methods because those are delegated to from other methods which already have that restriction. This makes sure to honor that.

IO#skip always returns Nil. One implementation of an HTTP IO leaked an Int32 into other implementations as well. Adding Nil everywhere avoids that.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works labels Jun 28, 2021
@@ -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).

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

src/enumerable.cr Outdated Show resolved Hide resolved
src/io/sized.cr Outdated
@@ -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.

src/io/argf.cr Outdated
@@ -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.

@straight-shoota straight-shoota mentioned this pull request Jun 29, 2021
@caspiano
Copy link
Contributor

Could we please merge this? It would be great to get it in before the next nightly build

@straight-shoota
Copy link
Member Author

straight-shoota commented Jul 1, 2021

@crystal-lang/crystallers I'd like to get another approval to make sure we agree on e8cf88e6 (see code comment thread).

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

I'm unsure about adding these return types, I feel it's a breaking change, but let's try it. We can always revert if it breaks someone's code.

@beta-ziliani
Copy link
Member

beta-ziliani commented Jul 1, 2021

But @asterite in that case the code will break anyway since it's adding the restrictions that were planned for 1.1.0. Right?

I skimmed the new code with casts, and I have the feeling that some of those casts can be avoided. But that's not a real issue in the end, since I assume the optimizer will get read of useless casts anyway.

So let's merge it.

@@ -60,7 +60,7 @@ module HTTP

def read(slice : Bytes) : Int32
ensure_send_continue
@io.read(slice)
@io.read(slice).to_i32
Copy link
Member

Choose a reason for hiding this comment

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

Are there @io.read that does not return Int32? It seems odd to add all the to_i32 on methods that should already be returning Int32 at a first glance.

Copy link
Member

Choose a reason for hiding this comment

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

Yes: if a shard inherits IO and returns another integer type.

Copy link
Member

Choose a reason for hiding this comment

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

(this was the main issue that happened, and that's why the original type restrictions had to be reverted... that's why I'm still not convinced that forcing return types here isn't going to break something else, but we'll see)

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, it could technically break code that was explicitly expecting an Int64 return type, for example. But I don't think that's likely. Almost all source implementations in stdlib return Int32. So it would need to be very specific and selective code that doesn't at least have Int32 return type as a union member.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ok, the abstract IO#read does not have a return type restriction and so implementors can choose any return type. Eventually on 2.0 we can make that return type explicit and the to_i32 can go away since the compiler will ensure that all implementations honor the return type.

👍 then.

@straight-shoota
Copy link
Member Author

But that's not a real issue in the end, since I assume the optimizer will get read of useless casts anyway.

Yes, those casts should be no-op if the type already matches.

@straight-shoota straight-shoota added this to the 1.1.0 milestone Jul 1, 2021
@straight-shoota straight-shoota merged commit 7e94d52 into crystal-lang:master Jul 3, 2021
@straight-shoota straight-shoota deleted the fix/return-type-restrictions branch July 3, 2021 09:38
@straight-shoota
Copy link
Member Author

The added type restiction on ˋEnumerable#count(& : T ->_)ˋ seems to cause an error in crinja:https://app.circleci.com/pipelines/github/straight-shoota/crinja/721/workflows/6cd5eb17-4e34-4e00-a2a7-441d2f4f70ad/jobs/1285

This change should really be fine, because ˋEnumerable#each(& : T ->_)ˋ uses the same restriction. So, I believe the error is cause by an actual bug in the code. But it's unclear to me why this wasn't caught without the type restriction on ˋ#countˋ. It appears that some ˋ#eachˋ implementation already yields an incorrect type. It's also pretty difficult to figure out what exactly is causing this. I've already narrowed it down to an ˋIterator(Value)ˋ type and I'll keep digging.
If my assumptions about the cause are correct, this might indicate a compiler error (because it should have complained about invalid semantics before).

@straight-shoota
Copy link
Member Author

I managed to identify the origin of the error: An Iterator(Foo) had a #next method return Bar. The Iterator interface actually does not enforce a return type on #next. It certainly should, though. It would've prevented this bug. Luckily, it was discovered through adding a type restriction to Enumerable#size.
Iterator#each overrides Enumerable#each without a type restriction on the yielded block. But id does not override Enumerable#count which expects the correct type restrictions.

The correct solution would be to add a type restriction to Iterator#next. It must return T | ::Iterator::Stop. Anything else breaks the iterator contract.
Other Iterator methods should also receive appropriate type restrictions. For Iterator#each it is particularly important to match the parent interface.

This is technically a change that can break code, but it only identifies already broken code. So it would be fine to merge in a minor release.
Question is, should we introduce this change now, shortly before the planned release of 1.1?
If not, we might want to revert the changes to Enumerable in this PR.

IMO that's not really helpful though, it contributes to hiding potential bugs in user code. And some of the other added type restriction might break broken code anyways.

In any case, I'll prepare a PR to apply the necessary type restrictions on Iterator.

@straight-shoota
Copy link
Member Author

straight-shoota commented Jul 8, 2021

Unfortunately, adding a return type restriction to an abstract method requires all implementations to have a compatible type restriction as well. That would be a major breaking change for Iterator#next because basically no iterator implementation defines a return type restriction there. 😢
So, we might not be able to do that before 2.0, at least not at such short notice.
Maybe there could be other solutions, for example, abstract method implementations could inherit the return type restriction from the abstract def by default (#10904).
I've created a separate issue to track Iterator#next: #10903

For now, I suppose we can just add type restrictions to Iterator#each to match Enumerable#each (#10905). I think this should also be enforced by the compiler (#10902).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants