-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add return expression #712
Conversation
beb1749
to
34d5b20
Compare
The checks should work now |
Hi @mladedav, I am traveling these days, I will take a look next week.
Adding another pass just for this check seems like an overkill to me. Ideally this should be a hard error during compilation since it's unsupported syntax.
Error codes are messy, I wanted to clean them up at some point. As long as you pick an unused error code number, it is ok. |
No worries, take your time and enjoy your vacation if that's the reason for travel. I've updated it so that the parser itself rejects the returns, but now pretty much everything in the parser is a macro since expressions can be almost everywhere and we need to remember if we're inside a closure or not and the error seems a lot more cryptic since it's just a generic Is there a way for the parser to output better errors? This was my first time using lalrpop so maybe it can also be solved in a better way than just passing With this change the new error code also goes away. But honestly I'd rather it existed in some form so that it is clear why the return keyword is not allowed there. |
# 3 │ return 42 | ||
# │ ^^^^^^ | ||
# │ │ | ||
# │ unexpected syntax token: "Return" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct that the only case we can encounter this error is if the return
was inside a closure? If this is true, we can improve the error to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I might suggest allowing it through the "lexer" but return a more specific error (return cannot be used within closures
) when "compiling".
src/parser/parser.lalrpop
Outdated
Exprs: Vec<Node<Expr>> = { | ||
Expr => vec![<>], | ||
<v:(<Expr> EndOfExpression)+> <e:(<Expr>)?> => match e { | ||
Exprs<T>: Vec<Node<Expr>> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this change required a lot of changes to the parser. Is this because you introduced ExprWithoutReturn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. Basically, if we want the parser to already refuse the code, we need the parser to know whether it is inside a closure, leading to two sets of Exprs
, Block
s and so on.
The other two options are checking the AST after parsing in another pass (what was here previously) or doing this bottom-up probably during compile
where the Expr
struct (and probably others, I'm not sure) would have an added field with the information of whether there is a return inside and failing the compilation when a closure finds out it contains a return.
Or a decision could be made how the returns should behave inside the closure. Then none of these would be needed. Any of these three options would add code and complexity that would be ultimately deleted when returns in closures are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this https://github.com/vectordotdev/vector/pull/19828/files#r1488734971, do you think it is viable to implement option 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I think this is the desirable path forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mladedav, just double checking, is this something you plan to implement in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm still planing to. It might be a bit more difficult than I originally thought because the closure must be guaranteed to only return correct types (e.g. bool for filter
) but I didn't have enough time to deep dive into this yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be working now, if you have any feedback, it would be appreciated.
I'm somewhat uneasy with the changes because I think I might have very easily overlooked a place where the returns should be propagated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is absolutely fantastic, thank you @mladedav.
Just noticed that some typedef tests are failing. To reproduce locally: |
Co-authored-by: Jesse Szwedko <jesse@szwedko.me>
The checks passed for me locally, can you try the pipeline again please? |
Thank you @mladedav, this is a great PR! |
Implements RFC 7496 - return expressions.
I think there are two potential issues:
I used
ExpressionError
to propagate the return, same as abort. I think that is the most straightforward solution since we do not want the rest of the expressions to be evaluated at all. However, the naming ofExpressionError
seems a bit off, but I didn't want to change it since it seems it is also used as part of a compilation error. So if I understand it correctly, it can be raised both as a compilation and runtime error? I personally think these two cases should be split if possible, but I did not dig deeper into this.I have made a check for usage of returns inside closures with the visitor pattern, similar as the check for unused variables. This is not strictly needed, but it greatly simplifies the parser definitions since there would be a lot of duplication with stuff like
Block
andBlockWithoutReturn
,Exprs
andExprsWithoutReturn
, Exprand
ExprWithoutReturn` and so on. I tried that but it just didn't feel right especially since it's probable returns in closures will be added sooner or later. I did not benchamark if there is issue with the additional pass.I've also added an error code 901, similar to 900 for unused variable warning. I don't know where to document it and I've noticed that 900 is not on the errors page.