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

[WIP] Added Try/Catch parsing #263

Closed
wants to merge 7 commits into from
Closed

[WIP] Added Try/Catch parsing #263

wants to merge 7 commits into from

Conversation

Razican
Copy link
Member

@Razican Razican commented Feb 25, 2020

I added parsing functionality for try/catch/finally blocks. Code is still WIP, and it should go to another function at least. Also, currently, if an execution finds a try/catch block, it will just panic, which is not nice. But at least, we are now able to parse the assert.js file of the test262 ECMAScript suite.

The sta.js file is still not parsed due to:

ParsingError: Unexpected keyword 'this' at line 14, col 3

I opened #264 for that.

I did a small idiomatization in the get_token() function, so that it uses idiomatic Rust functions instead of an if block. Functionality should remain the same.

When adding the Display implementation of the try/catch/finally blocks, I also improved the display of other blocks, to give a nicer view. Still, it's not indenting expressions, so there is some work to do there.

Finally, I made some modifications to the CLI so that it will only print the result of the last file. Before, it would print intermediate artifacts such as previously defined functions and more. Still not sure how will multiple file execution actually work.

boa/src/syntax/ast/expr.rs Outdated Show resolved Hide resolved
boa/src/syntax/parser.rs Outdated Show resolved Hide resolved
boa/src/exec.rs Outdated Show resolved Hide resolved
boa/src/exec.rs Outdated
self.realm.environment.initialize_binding(catch_bind, val);
}

todo!("run the catch block");
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if I'm doing this right, @jasonwilliams. The catch_expr here is a (String, ExprDef::Block) tuple, and I'm not sure how can I pass the binding value to the catch block. Am I doing this right? Should the type of expression be different?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like you've done it right to me, the catch_bind is the parameter name right?
That should be initiated on the new block.

You'll also need to pop that environment when you've finished with it but there's a lot of refactoring needed in that anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

About this, the catch block is a Block type expression (as it has been implemented right now) but I need to pass a variable to it. Is this the correct way to do it?

@jasonwilliams
Copy link
Member

Will have another look through his when I catch some time

@Razican
Copy link
Member Author

Razican commented Mar 8, 2020

Leaving the catch block parsing/execution, this PR is ready for review.

I want to note that I did pretty big changes in expression printing. I needed this to properly debug everything, and it seems it's working nicely. It adds proper indenting and pretty formatting.

You can try it out by executing the assert.js file of the test suite. It will return the last function.

@jasonwilliams
Copy link
Member

jasonwilliams commented Mar 14, 2020

Doing a parser rewrite at the moment, so this will most likely need to change.
I will keep this here for now and see how i can get it in.

Hopefully the new parser is more easier to reason with and add features to

@Razican
Copy link
Member Author

Razican commented Apr 1, 2020

Let's close this PR and try it differently with the new parser :) I think maybe we can take some ideas for the execution.

@Razican Razican closed this Apr 1, 2020
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.

2 participants