Factor out the parser into a separate crate #2328
Conversation
|
Bencher Report| Branch | parser |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | microseconds (µs) |
|---|---|---|
| fibonacci 10 | 📈 view plot 🚷 view threshold | 256.92 µs |
| foldl arrays 50 | 📈 view plot 🚷 view threshold | 462.39 µs |
| foldl arrays 500 | 📈 view plot 🚷 view threshold | 3,850.90 µs |
| foldr strings 50 | 📈 view plot 🚷 view threshold | 4,604.50 µs |
| foldr strings 500 | 📈 view plot 🚷 view threshold | 46,802.00 µs |
| generate normal 250 | 📈 view plot 🚷 view threshold | 49,688.00 µs |
| generate normal 50 | 📈 view plot 🚷 view threshold | 1,922.60 µs |
| generate normal unchecked 1000 | 📈 view plot 🚷 view threshold | 2,002.40 µs |
| generate normal unchecked 200 | 📈 view plot 🚷 view threshold | 419.57 µs |
| pidigits 100 | 📈 view plot 🚷 view threshold | 2,327.90 µs |
| pipe normal 20 | 📈 view plot 🚷 view threshold | 686.95 µs |
| pipe normal 200 | 📈 view plot 🚷 view threshold | 6,242.00 µs |
| product 30 | 📈 view plot 🚷 view threshold | 250.76 µs |
| scalar 10 | 📈 view plot 🚷 view threshold | 560.17 µs |
| sum 30 | 📈 view plot 🚷 view threshold | 252.11 µs |
Where is this code/newtype located? I'm trying to focus the review a bit. |
| /// Concrete, recursive definition for an enum row. | ||
| pub type EnumRow = EnumRowF<Box<Type>>; | ||
| #[derive(Clone, PartialEq, Debug)] | ||
| pub struct EnumRow(pub EnumRowF<Box<Type>>); |
There was a problem hiding this comment.
Here (and RecordRow just below) is the newtype wrapper to allow us to implement Display.
There was a problem hiding this comment.
Ah, I feared this would be quite painful, but it doesn't impact the typechecker, only the runtime Type representation. Maybe we should add a comment as to why this one only get the newtype treatment. Another possibility would be to use only the newtype wrapper when we actually need to pretty-print/display, and keep the naked type alias in the actual definition. Though if the newtype wrapper didn't cause much diff,maybe that's not very useful...
| impl CloneTo for typ::EnumRows<'_> { | ||
| type Data<'ast> = typ::EnumRows<'ast>; | ||
| impl<Ty: CloneTo, ERows: CloneTo> CloneTo for typ::EnumRowsF<Ty, ERows> { | ||
| type Data<'ast> = typ::EnumRowsF<Ty::Data<'ast>, ERows::Data<'ast>>; |
There was a problem hiding this comment.
Here's the more generic impl of CloneTo for EnumRowsF so that it covers the type alias in nickel_lang_core
| @@ -0,0 +1,322 @@ | |||
| //! An environment for storing variables with scopes. | |||
There was a problem hiding this comment.
Why do we need to have environment here? It's a bit strange. Maybe another solution would be, middle term, to separate it entirely into its own helper crate.
There was a problem hiding this comment.
It's used in fix_type_vars in the uniterm module.
| /// Concrete, recursive definition for an enum row. | ||
| pub type EnumRow = EnumRowF<Box<Type>>; | ||
| #[derive(Clone, PartialEq, Debug)] | ||
| pub struct EnumRow(pub EnumRowF<Box<Type>>); |
There was a problem hiding this comment.
Ah, I feared this would be quite painful, but it doesn't impact the typechecker, only the runtime Type representation. Maybe we should add a comment as to why this one only get the newtype treatment. Another possibility would be to use only the newtype wrapper when we actually need to pretty-print/display, and keep the naked type alias in the actual definition. Though if the newtype wrapper didn't cause much diff,maybe that's not very useful...
| impl CloneTo for typ::EnumRows<'_> { | ||
| type Data<'ast> = typ::EnumRows<'ast>; | ||
| impl<Ty: CloneTo, ERows: CloneTo> CloneTo for typ::EnumRowsF<Ty, ERows> { | ||
| type Data<'ast> = typ::EnumRowsF<Ty::Data<'ast>, ERows::Data<'ast>>; |
Factors out the parser into a separate crate,
nickel-lang-parser. This is mostly just moving things around, but there were a few other changes.CloneTofor some type aliases innickel_lang_core::typecheck(likeUnifEnumRow) broke the orphan rule. I worked around these by adding some more generic implementations ofCloneToin the parser, so that those type aliases already implementCloneTofrom the implementations innickel_lang_parserDisplayimpls innickel_lang_core::typ(likeEnumRow) broke the orphan rule. I couldn't re-use the same trick from the last point, because of differences in the two differentAllocatorstructs. So I used a newtype wrapper instead.I didn't pay too much attention to exactly where the API boundary is. The main goal was to get it working, and then we can refine it later.
Fixes #2265