Record parse #6640
Record parse #6640
Conversation
crates/compiler/parse/src/expr.rs
Outdated
| } | ||
|
|
||
| #[macro_export] | ||
| macro_rules! both { |
There was a problem hiding this comment.
There's already an and! macro - how is both! different?
There was a problem hiding this comment.
I agree, this is not ideal, or idiomatic to the rest of parsing. I would like to find a better solution. The difference between and and both is that both is more conservative on returning Ok. I'm still a bit fuzzy on the reasoning between Progress and Ok/Err.
I was getting error because the first optional would make some progress, but still return an Err and eat part of the state that the second optional parser would need. I would also get a similar issue with one_of.
Do you have a suggestion on a different way to go about this? I'm not sure how much experience you've got with the parser. I'd love a better way to do this!
There was a problem hiding this comment.
I tried out backtrackable and that seems like it does what I wanted, but I still feel like this parser doesn't quite fit in with the style of the rest of them.
| spaces_before(specialize_err_ref(ERecord::Expr, loc_expr(false))) | ||
| ) | ||
| optional(and!( | ||
| and!(backtrackable(spaces()), byte(b':', ERecord::Colon)), |
There was a problem hiding this comment.
Hmm, I'm surprised you need to do backtrackable(spaces()). The only errors that spaces will give is for things like having a tab character, or a carriage return without a newline, etc. And backtrackable doesn't do anything if the inner parser returns Ok. Do you have an example of where the backtrackable helps?
There was a problem hiding this comment.
Yeah, when I take out the backtrackable, the default test fails The crux of it comes down to the optional.
optional(and!(
and!(spaces(), byte(b':', ERecord::Colon)),
record_field_expr()
))When parsing x ? 0.0, spaces() returns Ok(MadeProgress), but byte returns Err(NoProgress), so and! returns Err(MadeProgress). This ends up getting passed back until record_field returns that same Err. When backtrackable is used backtrackable(spaces()) always returns Ok(NoProgress). When combined in the and!, the result is now Err(NoProgress), which optional converts to Ok(None).
There was a problem hiding this comment.
Ahhhh; interesting. I didn't realize that backtrackable returned NoProgress even in the success case. That makes sense now.
This looks reasonable to me.
|
Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked |
should fix #6532
As of this draft, I'm happy with the parser change and would love some feedback if anyone has other ideas on it. I need to work on canonicalization, so I'll be doing that, too.