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

Type signature updates for Enumerable, Module, Range, Array #1921

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

hjwylde
Copy link
Contributor

@hjwylde hjwylde commented Jul 5, 2024

These are some type signature updates, mostly following on from the discussion here: #1226, but also some additional ones on top of it.

I've made the following changes:

  • Enumerable - added a more specific type override for enum_for/to_enum for the default (:each/no-arg) case; this override means that we know the Enumerator returned will be iterating over elements of type Elem. I think that this is a sensible override to provide because of the type from _Each. I wasn't sure if the overrides should go here or in the _Each interface, but it felt strange putting more things into _Each when it's currently clean and simple.

  • Module - improved the type signature for class_exec to be similar to instance_exec. Specifically it now shows that arguments passed are forwarded to the block, and that the block has a self type based on the receiver.

  • Range - added a type signature for % as it was missing. This one is a copy/paste from step, minus the optional parameters.

  • Array - added some more %a{implicitly-returns-nil} annotations. I've only added them in locations that looked similar to the original for [], where they may return nil if an index is out of bounds or the array is empty, as this is typically knowledge that is checked beforehand. Happy to reduce the number added though if it is too many. I haven't added any to Enumerable as there is no size/length/empty methods, so Hash#first will not have this annotation.

Let me know if there's anything I missed or items you'd like me to change.

@hjwylde hjwylde force-pushed the hjwylde/type-signature-updates branch from d5f9d23 to 661d04a Compare July 5, 2024 04:54
`enum_for` and `to_enum` default to creating an Enumerable based on the `each` method. Due to this, we can improve the type signature (make it more specific) for classes that include the Enumerable mixin as their `each` method iterates over elements of type `Elem`.
…exec`

This update ensures the type signature matches the documentation, which specifies that any arguments passed to the method call are forwarded on to the block, as well as a self type of `self`.
…hods that may return `nil`

The methods chosen are only those that are similar to the access method (`[]`); methods that return nil if the array is empty or the index is out of bounds.
@hjwylde hjwylde force-pushed the hjwylde/type-signature-updates branch from 0f3578d to 134d2df Compare July 5, 2024 04:56
@ParadoxV5
Copy link
Contributor

I wasn't sure if the overrides should go here or in the _Each interface, but it felt strange putting more things into _Each when it's currently clean and simple.

The purpose of _Each (and #424 too) was to identify that a type has the method #each.
It is not its concern that enum_for/to_enum has a narrower type, but rather Kernel’s own responsibility to type-narrow (#1822).

@hjwylde
Copy link
Contributor Author

hjwylde commented Jul 8, 2024

The purpose of _Each (and #424 too) was to identify that a type has the method #each.

I agree, better to keep it as such.

It is not its concern that enum_for/to_enum has a narrower type, but rather Kernel’s own responsibility to type-narrow (#1822).

I believe that method contracts do have a purpose in signatures and would be of help, but currently they do not exist and it looks like it's still under discussion. In the meantime, I believe this has value as it is still a standard practice for a subclass in OO languages to provide narrower types. The commit regarding to_enum/enum_for could always be reverted if #1822 is resolved.

As a side note, if there are only concerns with the to_enum/enum_for updates, I can drop that commit so that the PR can just address the other changes (to Range, Array, Module)?

@soutaro soutaro added this to the RBS 3.6 milestone Jul 19, 2024
Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Thanks! Let's try with this version, which makes a deadline to support it in Steep. 😅

@soutaro soutaro added this pull request to the merge queue Jul 19, 2024
Merged via the queue into ruby:master with commit b0f5a72 Jul 19, 2024
19 checks passed
@hjwylde
Copy link
Contributor Author

hjwylde commented Jul 21, 2024

Thanks! Yeah let's see how it goes :), no concerns from me if it needs to be changed/reverted at some point.

@hjwylde hjwylde deleted the hjwylde/type-signature-updates branch July 21, 2024 22:07
@soutaro soutaro added the Released PRs already included in the released version label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

3 participants