Skip to content

Add block type restriction to Iterator#each#10905

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
straight-shoota:fix/iterator-each-type-restriction
Jul 12, 2021
Merged

Add block type restriction to Iterator#each#10905
straight-shoota merged 1 commit intocrystal-lang:masterfrom
straight-shoota:fix/iterator-each-type-restriction

Conversation

@straight-shoota
Copy link
Member

Iterator#each implements Enumerable#each, but does not apply the same type restriction on the block argument. The compiler currently does not enforce that (see #10902). This change makes the implementation's signature match the abstract def's.

See #10857 (comment) for details.

There are more implementations of Enumerable#each in stdlib which should get the same type restriction. But I encountered some issues with that, so it'll be deferred. Iterator#each seems to be fine though, at least with stdlib specs.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:collection labels Jul 8, 2021
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.

Looks good! I think we can include this for 1.1, I consider it a bug fix, as you mentioned.

@straight-shoota straight-shoota added this to the 1.1.0 milestone Jul 9, 2021
@straight-shoota straight-shoota merged commit 198b266 into crystal-lang:master Jul 12, 2021
@straight-shoota straight-shoota deleted the fix/iterator-each-type-restriction branch July 12, 2021 11:08
@straight-shoota
Copy link
Member Author

straight-shoota commented Jul 13, 2021

This change caused another build break in crinja: https://app.circleci.com/pipelines/github/straight-shoota/crinja/743/workflows/6a3278a5-8635-417a-96b5-ff7a483ae983/jobs/1307

This time it doesn't seem to be a bug in crinja, but it discovered an issue with the block's return type restriction. It should be empty instead of underscore (see #10467 (comment)). We should really get these compiler semantics straight 🙈

Fix is in #10928. Alternatively, we can revert this PR.

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. topic:stdlib:collection

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants