Specialize exprs merge #7243
Specialize exprs merge #7243
Conversation
This reverts commit ae28556.
There was a problem hiding this comment.
I looked through every file except for 5 or 6 200+ line files in specialize_types, this looks generally OK to me. I left some minor comments, you're good to ignore all of them for now since this is just a "get stuff in the main branch" PR.
The one thing that we may want to handle before merging is something @rtfeldman mentioned to me he wanted to do at some point: move stuff from the compiler folder to two folders, check for parsing, typechecking, mono, etc. and build for codegen-related crates. Before we have to move a bunch of files around in the future, I'd vote for moving specialize_types to check/specialize_types preemptively. Utility crates like test_compile can stay in the crates root folder.
| pub fn as_str_plural(&self) -> &'static str { | ||
| match self { | ||
| AliasKind::Structural => "aliases", | ||
| AliasKind::Opaque => "opaques", |
There was a problem hiding this comment.
nitpick: "aliases" is a sensible plural, but "opaques" seems weird. maybe we can do "opaque types" ?
| .append(alloc.reflow(alias_kind.as_str())) | ||
| .append(alloc.reflow("es is only allowed if recursion happens behind a tagged union, at least one variant of which is not recursive.")); | ||
| .append(alloc.reflow(alias_kind.as_str_plural())) | ||
| .append(alloc.reflow(" is only allowed if recursion happens behind a tagged union, at least one variant of which is not recursive.")); |
There was a problem hiding this comment.
| .append(alloc.reflow(" is only allowed if recursion happens behind a tagged union, at least one variant of which is not recursive.")); | |
| .append(alloc.reflow(" are only allowed if recursion happens behind a tagged union, at least one variant of which is not recursive.")); |
|
|
||
| #[allow(dead_code)] | ||
| #[derive(Default)] | ||
| pub struct ConstrainedExpr { |
There was a problem hiding this comment.
nitpick: I think we should use past tense for output post-compiler phase, e.g. ConstrainedExprOut or in my opinion ConstrainedExpr, and present tense for before the phase, e.g. ConstrainExpr. The same would apply to the other compiler phases in this crate.
| impl Default for ParseExpr { | ||
| fn default() -> Self { | ||
| Self { | ||
| arena: Bump::with_capacity(4096), |
There was a problem hiding this comment.
Consider moving this magic number to a constant. Why 4kb? Page size?
| for line in input | ||
| .lines() | ||
| // Keep skipping until we hit a line that is neither empty nor all spaces. | ||
| .skip_while(|line| line.chars().all(|ch| ch == ' ')) |
There was a problem hiding this comment.
Should we also ignore tabs?
There was a problem hiding this comment.
I think tabs aren't currently allowed by the parser
| } | ||
| } | ||
|
|
||
| pub fn into_nonempty_slice(&self) -> Option<NonEmptySlice<T>> { |
There was a problem hiding this comment.
nitpick: the into_ prefix in Rust usually takes ownership, I'd use as_ for non-consuming APIs.
| @@ -7,6 +8,8 @@ use core::{ | |||
|
|
|||
| use crate::soa_slice::Slice; | |||
|
|
|||
| pub type Id<T> = Index<T>; | |||
There was a problem hiding this comment.
nitpick: This seems unnecessarily opaque
| U128(u128), | ||
| F32(f32), | ||
| F64(f64), | ||
| Dec(f64), |
There was a problem hiding this comment.
To avoid floating point errors, should this be u64/u128?
| /// [ A, .., B ] -> patterns = [A, B], rest = 1 | ||
| /// [ A, B, .. ] -> patterns = [A, B], rest = 2 | ||
| /// Optionally, the rest pattern can be named - e.g. `[ A, B, ..others ]` | ||
| opt_rest: Option<(u16, Option<IdentId>)>, |
There was a problem hiding this comment.
For future-proofing ourselves, it may be a good idea to use a list type here, e.g.
rests: Slice<(u16, Option<IdentId>)>
In case we end up supporting multiple in the future.
I understand this was copied from the other file, so NBD.
There was a problem hiding this comment.
Why do you think we need to future proof? Personally, I prefer types to model the current reality. We can change it later and just follow the compiler.
There was a problem hiding this comment.
I was thinking of how easy it was to implement early returns (barring some minor bugs) in the mono code + codegen. Though I don't know if early returns were ever planned, the refcounting code Just Worked when I converted all early returns can::Exprs to Stmt::Ret naively.
With how complex Roc is at 300k LoC and growing, it's nice to make code work in a more generic way to avoid future contributors needing to figure out how the whole thing works before making changes.
That said, it's minor, and I can see why we'd rather them need to understand how things work before making changes. I'm totally fine if we don't go for the "future-proof" approach.
| pub struct MonoModule { | ||
| mono_types: MonoTypes, | ||
| foreign_symbols: ForeignSymbols, | ||
| interned_strings: Vec<String>, |
There was a problem hiding this comment.
Maybe add a TODO here to use a string interner?
Merges the WIP new mono implementation so we can collaborate on it without drifting much from
main. This shouldn't affect the behavior of the compiler.