-
Notifications
You must be signed in to change notification settings - Fork 41
Don't wrap StatementExpressions in IIFE in declaration (#202) #1074
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
Conversation
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.
I was wondering how you were going to deal with the precedence of something like x := y++ + switch y ...
but now I see those aren't unwrapped, and compiled as before.
Overall, this definitely seems like a nice first step in this direction! The whole process is a little daunting, but maybe we can chip away at most important cases like this until it's all done.
Maybe it's worth adding a test that demos the #202 feature, like:
x := switch y
<? 'number'
y+1
else
return 'bad'
Note that if ... return
of #202 still isn't supported, because we don't seem to allow expressionizing if
statements... Personally I'd find that more useful than switch
, but hopefully it's not too much harder than what's in this PR. (I guess we should try to parse an IfExpression first, and on failure, parse an IfStatement that worst-case gets IIFE wrapped?)
source/parser.hera
Outdated
children: wrapIIFE([["", s]]), | ||
statement: s | ||
type: "StatementExpression", | ||
statement: statement, |
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 and others could simplify to:
statement: statement, | |
statement, |
source/parser/util.civet
Outdated
@@ -419,6 +422,8 @@ function wrapIIFE(expressions: StatementTuple[], async): ASTNode[] | |||
expressions, | |||
children: ["{", expressions, "}"], | |||
bare: false, | |||
root: false, | |||
parent: undefined, // TODO: get parent from expression |
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.
parent
is set by the makeNode
call below.
source/parser/util.civet
Outdated
@@ -271,6 +273,7 @@ function makeLeftHandSideExpression(expression) | |||
case "SwitchExpression": // wrapIIFE | |||
case "ThrowExpression": // wrapIIFE | |||
case "TryExpression": // wrapIIFE |
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.
If TryExpression
etc. no longer exist (I think they got replaced by StatementExpression
, which seems like a good unification), should remove them here.
I was going to suggest the example from #202 be added as a test, but it turns out it doesn't work (still gets wrapped in an IIFE): compiled :=
try
require('@danielx/civet').compile str, options
catch err
return "error" On the other hand, this spelling does work. Maybe you need to skip over whitespace? compiled := try
require('@danielx/civet').compile str, options
catch err
return "error" |
This is about 50% of #202
Most of the work was refactoring
pushRef
to be more generalassignResults
to handle pushing to a collection or assigning to a ref.Things remaining:
Much later: