Run source-typer on crystal standard library, give or take#15682
Run source-typer on crystal standard library, give or take#15682Vici37 wants to merge 1 commit intocrystal-lang:masterfrom
Conversation
ysbaddaden
left a comment
ysbaddaden
left a comment
There was a problem hiding this comment.
We can't merge as-is it will introduce countless breaking changes, yet, I think it's interesting to notice some complex unions/tuples that could get simplified (less types), as well as optimization opportunities (expect slice vs passing static array).
| end | ||
|
|
||
| private def strict_encode(data, alphabet, pad = false) | ||
| private def strict_encode(data : Slice(UInt8) | StaticArray(UInt8, 5) | String | StaticArray(UInt8, 20) | StaticArray(UInt8, 16), alphabet : String, pad : Bool = false) : String |
There was a problem hiding this comment.
The static arrays are interesting: they outline that the method should really just take a slice, and there are opportunities for optimizations.
Even the string in the other methods are interesting. There could be a specific overload that would call the slice method.
There was a problem hiding this comment.
Ideally, we should be able to express a type restriction for types that have a #to_slice : Bytes method (cf. #934).
There was a problem hiding this comment.
I'm not sure about runtime optimizations because private methods are typically inlined anyway, so the static array here likely won't be copied around.
But for sure the compiler would have an easier time if it has to deal with only one method accepting Bytes instead of a multitude of different StaticArray instance types.
| end | ||
|
|
||
| private def self.select_impl(ops : Indexable(SelectAction), non_blocking) | ||
| private def self.select_impl(ops : Indexable(SelectAction), non_blocking : Bool) : Tuple(Int32, Channel::NotReady | Int32) | Tuple(Int32, Channel::NotReady | Nil) | Tuple(Int32, Channel::NotReady | String) | Tuple(Int32, Bool | Channel::NotReady | String) | Tuple(Int32, Channel::NotReady | String | Nil) | Tuple(Int32, Bool | Channel::NotReady | String | Nil) | Tuple(Int32, Bool | Channel::NotReady) | Tuple(Int32, Bool | Channel::NotReady | Nil) | Tuple(Int32, Channel::NotReady | Int32 | Nil) | Tuple(Int32, Channel::NotReady | Exception | Nil) |
There was a problem hiding this comment.
Urgh, tuples are nice but they quickly create lots of types. It might be interesting to introduce a type, here 🤔
There was a problem hiding this comment.
Seeing the size of the union is a good indication there's confusion on what's expected of this method. Could also be a good opportunity to see why some of the types show up like this as well.
|
Related:
There are lots of trivial type enhancements which should be straightforward to merge in. Following the example of #10575, it would be helpful to split these changes into semantic groups. That makes it easier to review and merge. Once the simple cases are resolved, we can more clearly make out and discuss the more complex ones. Let me know if you want to proceed with that, or need any help. |
|
Agreed! I could add some filtering options or thresholds - add restrictions if the number of types in the union is less than N, say. I'll work on making some smaller PRs over some smaller clusters of related code. I also thought seeing the sheer mass of these changes amusing enough to want to share them in all their glory 😁 |
This is the output of compiling [cr-source-typer](https://github.com/Vici37/cr-source-typer) and running it with the below incantation: ``` CRYSTAL_PATH="./src" ./typer spec/std_spec.cr \ --error-trace --exclude src/crystal/ \ --stats --progress \ --union-size-threshold 2 \ --ignore-private-defs \ src/process ``` This is related to crystal-lang#15682 .
This is the output of compiling [cr-source-typer](https://github.com/Vici37/cr-source-typer) and running it with the below incantation: ``` CRYSTAL_PATH="./src" ./typer spec/std_spec.cr \ --error-trace --exclude src/crystal/ \ --stats --progress \ --union-size-threshold 2 \ --ignore-private-defs \ src/regex ``` This is related to crystal-lang#15682 .
…Vici37/cr-source-typer) and running it with the below incantation: ``` CRYSTAL_PATH="./src" ./typer spec/std_spec.cr \ --error-trace --exclude src/crystal/ \ --stats --progress \ --union-size-threshold 2 \ --ignore-private-defs \ src/spec ``` This is related to crystal-lang#15682 .
…Vici37/cr-source-typer) and running it with the below incantation: ``` CRYSTAL_PATH="./src" ./typer spec/std_spec.cr \ --error-trace --exclude src/crystal/ \ --stats --progress \ --union-size-threshold 2 \ --ignore-private-defs \ src/spec ``` This is related to crystal-lang#15682 .
3 participants
This runs the https://github.com/Vici37/cr-source-typer project on the crystal source code with command:
Followed by repeated:
Until specs passed and a basic
makecall all work.I have done no additional fixing beyond the above - the way that the standard lib is loosely coupled with itself means that the order of
requiresisn't well structured, and so some types were present in restrictions before the relevantrequirewas hit. Some methods have hilariously long type restrictions as well.This PR should not be as accepted as it is, but represents an interesting view of the internals of the compiler and standard library that I wanted to share.