Skip to content
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

Fix parsing and evaluation of lambdas with args and operator associativity #151

Merged
merged 11 commits into from
Jan 9, 2022

Conversation

danieldickison
Copy link
Collaborator

@danieldickison danieldickison commented Aug 3, 2021

Turns out parserBehavior.ts already had tests that were being skipped for lambdas with args. It references #65 but I'm not sure how that issue is related. This appears to be the root of issue #146

This should also fix incorrect operator associativity (see comment 894513849 and #153).

@danieldickison
Copy link
Collaborator Author

I've updated the PR with a fix — it parses an arg list in front of => by taking the previously parsed Expression and turning it into a new Parameters node, which extends the context when the lambda is called. This involved adding a new , operator and dropping the => operator precedence to the lowest possible.

It's a little bit hacky, but I think it works well given how the parser is organized. (In actual JS grammar, => is not defined as an operator, so would require some reworking if we want to mirror that parsing.)

@danieldickison danieldickison changed the title Add failing tests for #146: lambdas with args Fix parsing and evaluation of lambdas with args Aug 5, 2021
Rewrite the Node.create_root method using a Shunting Yard algorithm

The old method was erroneously right-associating operators with equal precedence, and also mishandled some deeply nested parse trees. I've updated some of the tests that assumed right-associativity to reflect the correct parse tree.
@danieldickison
Copy link
Collaborator Author

danieldickison commented Aug 6, 2021

In the course of testing this, I discovered another issue with the parser — it was incorrectly right-associating operators with equal precedence, and also mishandling (in a not-totally-clear way) some deeply nested trees. I rewrote Node.create_root to use the shunting yard algorithm to handle precedence and updated tests as appropriate.

@danieldickison danieldickison changed the title Fix parsing and evaluation of lambdas with args Fix parsing and evaluation of lambdas with args and operator associativity Aug 30, 2021
Copy link
Member

@brianmhunt brianmhunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieldickison this looks excellent! I added a few comments (mostly of the linting and documentation variety) but in general I think everything here is fantastic. Thank you!

I'll wait for you to reply on the comments and we'll go from there, but I don't think anything here is a real blocker.

packages/utils.parser/src/Parser.ts Outdated Show resolved Hide resolved
packages/utils.parser/src/Parser.ts Outdated Show resolved Hide resolved
packages/utils.parser/src/Parser.ts Outdated Show resolved Hide resolved
packages/utils.parser/src/Parameters.ts Show resolved Hide resolved
packages/utils.parser/src/Parameters.ts Show resolved Hide resolved
packages/utils.parser/src/Parameters.ts Outdated Show resolved Hide resolved
leaf = leaf.rhs
static create_root (nodes, debug=false) {
// shunting yard algorithm with output to an abstact syntax tree of Nodes
const out = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor note: out and ops are very similarly named, which might make them harder to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like they present fairly distinctively in context, since out has an ascender while ops has a descender, and I can't think of good ones that aren't annoyingly long, but I'm open to other naming ideas!

packages/utils.parser/src/Node.ts Show resolved Hide resolved
packages/utils.parser/spec/parserBehaviors.ts Show resolved Hide resolved
packages/binding.core/src/options.ts Outdated Show resolved Hide resolved
@danieldickison
Copy link
Collaborator Author

@brianmhunt thanks for the review. I addressed each of your comments. Lmk how that looks to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants