Fixes bugs with yields and awaits inside various let blocks
#1060
Conversation
rhendric
left a comment
rhendric
left a comment
There was a problem hiding this comment.
Nice work! This is almost identical to the solution I found. Fix one minor style quibble and then this is good to go.
| switch child.op | ||
| | \yield \yieldfrom => has-yield := true | ||
| | \await => has-await := true | ||
| return true if has-yield and has-await |
There was a problem hiding this comment.
A function can never legally have both a yield and an await, so this is a little misleading. Could you change this back to something a little closer to the original expression in add-body, where you return as soon as any instance of Yield is found? Instead of returning true you can return child.op, to distinguish between the yield and await cases without mutating a variable in the outer scope.
There was a problem hiding this comment.
I left this in intentionally for async generators, as I would imagine they will come to LS eventually. I thought it would be better to future-proof it, in case someone implementing async generators forgets about awaits and yields in let blocks, as it have happened with async functions.
There was a problem hiding this comment.
That's a good thought! I wasn't aware that async generators had gotten this far in the standardization process before now; they should be very easy to add to LiveScript, so I agree this is a good precaution.
| switch child.op | ||
| | \yield \yieldfrom => has-yield := true | ||
| | \await => has-await := true | ||
| return true if has-yield and has-await |
There was a problem hiding this comment.
That's a good thought! I wasn't aware that async generators had gotten this far in the standardization process before now; they should be very easy to add to LiveScript, so I agree this is a good precaution.
2 participants
Fix for #1019, #1021 and #1023.
This wasn't that hard as I imagined. Did I get it right?
I also noticed a similar behavior is duplicated in
Node::compile-closure. Perhaps it could be rewritten to useCall.letinside.I still don't know how everything works, but more and more I get the hang of it.