Add and improve type restrictions of block arguments#10467
Conversation
|
might want to check |
|
|
| # | ||
| # See also: `Array#select`. | ||
| def select! | ||
| def select!(& : T ->) : self |
There was a problem hiding this comment.
| def select!(& : T ->) : self | |
| def select!(& : T -> Bool) : self |
Is there a reason this and like reject(!) shouldn't be forced to return a Bool?
There was a problem hiding this comment.
That would definitely be a breaking change (so should be brought up in a different issue). Right now, the value just needs to be interpreted as truthy. And I think that's probably good. No need to force users to a bool conversion.
There was a problem hiding this comment.
Wouldn't T -> mean Nil return value type?
There was a problem hiding this comment.
Nope. It means there's no restriction on the return type. Basically like a free var.
There was a problem hiding this comment.
I think @Sija is right. You need to use underscore to mean any type
There was a problem hiding this comment.
It's equivalent to T -> _, which means "allow any return type".
EDIT: Oops, didn't refresh 😬.
There was a problem hiding this comment.
Yes, it's different for captured blocks.
There was a problem hiding this comment.
Ideally we should probably improve the semantics, maybe make empty invalid for non-captured blocks and require an underscore since that's more explicit and fits with the use of underscore in the type grammar.
But for now, it's better to use empty because it's not entirely equivalent to underscore: https://forum.crystal-lang.org/t/difference-between-underscore-and-empty-as-block-return-value/2998
There was a problem hiding this comment.
Based on #11210 we never constrain the return type to Bool in contexts like this.
There's an unintended subtle difference for yielding blocks and underscore return value. See crystal-lang#10467 (comment)
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
src/slice.cr
Outdated
| {% raise "expected block to return Int32 or Nil, not #{U}" %} | ||
| {% end %} | ||
|
|
||
| def sort(&block : T, T -> Int32?) : Slice(T) |
There was a problem hiding this comment.
I wonder if this change would break something. I can't come up with a concrete example of a U that's a subtype of Int32?.
There was a problem hiding this comment.
Int32 and Nil are subtypes, but they both work with both variants.
The macro solution was introduced in #6611 with the following comment by @asterite
Note: I replaced an
Int32?restriction withU forall Uto allow forsort's block to be of typeInt32orInt32?, for performance reasons. Ideally we should be able to doU < Int32?or some other condition over a free variable, but that's currently not supported. Eventually we can improve that code.
I don't know why that's more performant (and if that's still true).
There was a problem hiding this comment.
The sort code raises an exception if the block returns nil. If we make the return type to be Int32 | Nil, checking for nil will always happen, regardless of whether the block can actually return nil. If we don't restrict the block's type and it only returns Int32, the check nil goes away.
That said, I'm not finding any performance difference now, or my benchmark is flawed, so it's probably fine to change this 🤷
There was a problem hiding this comment.
OK, let's leave it as is until we have better evidence.
There was a problem hiding this comment.
That is, @straight-shoota restore these lines for now.
6 participants
There is probably a lot more places where better type restrictions could improve usability, especially error reporting as reported in #10457. This is already a huge patch, focusing on yielding collection methods.
Resolves #10457