search

sophiajt / new-nu-parser

23 stars 10 forks source link

Some parser feedback #21

Open tertsdiepraam opened 8 months ago

tertsdiepraam commented 8 months ago

Since @kubouch asked for feedback on Discord, here's some things I noticed. Most of it is quite clear to me actually, though it might help that I've written a similar parser recently (for another language but in roughly the same style of parsing).

First of all, I was wondering what the reason is that the Parser takes ownership over the Compiler instead of a mutable reference. Is it just to make it so that Parser does not need a lifetime?

If I'm not mistaken, you're using shunting yard for operator precedence. I think it might be nice to document that some more for people who've never seen that before. Maybe you could include a link to the wikipedia page and some nu-specific examples.

On the topic of parsing binary operators, I'm not sure what the nu grammar is exactly, but I'm surprised to see that assignment is in the math_expression function too. Maybe it makes sense to split those up? Or can an assignment happen deep in some expression? As of now, it seems like 1 + 2 = 3 + 4 is allowed, which doesn't feel like it should be? Either I'm confused by the code here or by nu's grammar 😅

There's a recurring pattern like this:

if self.is_something() {
    let something = self.something();
}

It might be possible to change that in many cases to:

if Some(something) = self.try_something() {
    ...
}

However, that is also a matter of preference I suppose. The current version is quite simple and clear, just a bit repetitive.

I think some more docstrings would indeed be nice, especially on the more complex methods such as math_expression, simple_expression or record_or_closure.

kubouch commented 8 months ago

The Parser taking ownership over the Compiler is Sophia's experiment just to try it out. The immediate advantage is not having duplicate API like in Nushell's EngineState and StateWorkingSet. Not sure if we'll be able to keep it that way in the final version, though. Why ownership instead of mutable reference, I don't know, probably to not having to deal with lifetimes, not sure if there would be any benefit in doing a mutable reference.

I haven't heard about shunting yard, not sure if it was Sophia's intention to specifically write a shunting yard, or it just happened to resemble one. But otherwise, documenting the inner workings of the parser in doc comments would be great, agreed.

I think the reason for assignment being a math expression is to allow mutable assignments, like mut x = 10; x = 12, but I guess cases like 1 + 2 = 3 + 4 should be disallowed. Current Nushell reports 1 + 2 as needs to be a variable which I think is good enough, perhaps could say needs to be a mutable variable.

I like the try_something() refactor, it makes the code simpler and it's still self-explanatory.