-
Notifications
You must be signed in to change notification settings - Fork 236
Correct various signatures #728
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
Changes from 13 commits
e8c068e
db944e8
11d4357
b0d1618
7db1f8c
dafb591
f3aa102
e22f15c
b43f69c
0672ce9
c430d63
8cb1b4a
3ddf20f
478881a
aae9584
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -493,8 +493,8 @@ class Hash[unchecked out K, unchecked out V] < Object | |||||
| # h.fetch_values("cow", "bird") # raises KeyError | ||||||
| # h.fetch_values("cow", "bird") { |k| k.upcase } #=> ["bovine", "BIRD"] | ||||||
| # | ||||||
| def fetch_values: (*K) -> Array[V] | ||||||
| | [X] (*K) { (K) -> X } -> (V | X) | ||||||
| def fetch_values: (*K) -> ::Array[V] | ||||||
| | [X] (*K) { (K) -> X } -> ::Array[V | X] | ||||||
|
|
||||||
| # Returns a new hash consisting of entries for which the block returns true. | ||||||
| # | ||||||
|
|
@@ -776,7 +776,7 @@ class Hash[unchecked out K, unchecked out V] < Object | |||||
| # h = { "a" => 100, "b" => 200 } | ||||||
| # h.replace({ "c" => 300, "d" => 400 }) #=> {"c"=>300, "d"=>400} | ||||||
| # | ||||||
| def replace: (Hash[K, V]) -> self | ||||||
| def replace: [A, B] (Hash[A, B]) -> Hash[A, B] | ||||||
|
||||||
| def replace: [A, B] (Hash[A, B]) -> Hash[A, B] | |
| def replace: [A, B] (Hash[A, B]) -> Hash[A | K, B | V] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on the reason for having | K/| V please?
AFAIK replace clears the hash first, so there shouldn't be any Ks or Vs left. I could see a reason for having | K/| V for {a: 0, b: 1}.replace({}), however here I think it's better to just use clear which will retain the receiver's type information.
This change is probably related to the collect! discussion too, as it's essentially the same behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I was confusing with #merge.
Will continue at #collect! for in-place updates.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the monomorphic version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll just wait until the discussion on collect! is resolved as it's related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional, since RBS cannot change the type of the receiver by method calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, in such cases, is it really better to just return
self?I understand that it makes the signature describe the method behaviour better, however I think using generics here will help the signature to better describe the type behaviour. To me this is more what type signatures are for, as we can refer to the documentation or implementation if we are interested in learning the side-effects of the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following is the case to detect a type error in my mind.
On the other hand, the following runs without any error == Steep detects a false error.
I'm not sure if we can find a solution to make the both work, and we may have to choose one. My intuition is using
#map!is usually done without assignment because it is in-place.Any thoughts from @mame regarding to TypeProf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, TypeProf performs weak-update of the element type of a container type when its method accepts a new type at a contravariance occurrence, i.e.,
https://mame.github.io/typeprof-playground/#rb=ary+%3D+%5B1%2C+2%2C+3%5D%0Aary.map%21%28%26%3Ato_s%29%0Ap+ary+%23%3D%3E+Array%5BInteger+%7C+String%5D&rbs=
Changing the RBS declaration as proposed will break this behavior of TypeProf.
That being said, I have no strong opinion about the change. I'm unsure how much is used a call to
map!that changes the element type. If there are a lot of such cases, prohibiting them could be very conservative and annoying. @hjwylde do you have any statistics?BTW, I guess the return type of
map!is not important. IMO it could be evenvoid.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I don't have any statistics. I too don't feel strongly about the return type of
map!, so I will revert this one, and make the appropriate changes to the other points in this review - thank you.This is interesting, perhaps we can adopt something similar too. Initially I had disregarded the idea as I thought it would lead to issues with
Array#include?etc., but it seems these types of methods take anuntypedobject and it would be okay.