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

Implement block scoped variable declarations #173

Merged
merged 3 commits into from
Oct 22, 2019
Merged

Implement block scoped variable declarations #173

merged 3 commits into from
Oct 22, 2019

Conversation

barskern
Copy link
Contributor

const and let are now scoped to the block, while var is scoped to
the surronding function (or global).

NB! Another bigger change is that all tests have been changed to use var
instead of let or const. This is because every time forward is
called with some new Javascript to evaluate, we parse it as a block,
hence variables can only persist across calls to forward if they are
defined using var. I assume it is intentional to parse each call as a
separate block, because a block is the only ExprDef which can contain
multiple statements.

Closes #39

`const` and `let` are now scoped to the block, while `var` is scoped to
the surronding function (or global).

Another bigger change is that all tests have been changed to use `var`
instead of `let` or `const`. This is because every time `forward` is
called with some new Javascript to evaluate, we parse it as a block,
hence variables can only persist across calls to `forward` if they are
defined using `var`. I assume it is intentional to parse each call as a
separate block, because a block is the only `ExprDef` which can contain
multiple statements.

Closes #39
@barskern
Copy link
Contributor Author

@jasonwilliams I spent some time and I managed to got it working, however a few of the tests were failing. After a bit of debugging I found out that I wasn't able to get from an declerative environment to its parent, and then I found out that it unconditionally returns None. Is there a reason for this?

https://github.com/jasonwilliams/boa/blob/a87f04f952cd4623aa096650af08ea87677550be/src/lib/environment/declarative_environment_record.rs#L163-L165

Instead of iterating over the `environment_stack` we rather use
`get_outer_environment` as it will be a better fit when asyncronous
functions are implemented.
@jasonwilliams
Copy link
Member

jasonwilliams commented Oct 20, 2019

That’s weird, only global should return None. I’m not sure, it should return a parent env if there is one.
You can change this

@barskern
Copy link
Contributor Author

@jasonwilliams Ah okay, thats what I was thinking aswell. I have changed it now so that it will return the parent if there is one.

Variables that are defined outside a block should be changeable within
the scope. Just because variable is undefined does not mean it is not
initialized.
Comment on lines +102 to +107
pub fn environments(&self) -> impl Iterator<Item = Environment> {
std::iter::successors(Some(self.get_current_environment_ref().clone()), |env| {
env.borrow().get_outer_environment()
})
}

Copy link
Member

Choose a reason for hiding this comment

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

nice utility function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I was quite happy with it 😄

@jasonwilliams
Copy link
Member

I see what you mean about parse_all() creating a new block each time.
I suppose we do want this, so yeah they will have to change, that's a shame.

@barskern
Copy link
Contributor Author

I see what you mean about parse_all() creating a new block each time.
I suppose we do want this, so yeah they will have to change, that's a shame.

Yeah I see your point. One could rewrite tests to rather defined them as functions and then rather call the functions in the next invocation of forward. Kinda like the following:

let init = r#"
const empty = new Array();
const one = new Array(1);

function ee() {
	return empty.concat(empty);
}
"#;
forward(&mut engine, init);

let _ee = forward(&mut engine, "ee()");

But doing this would require rewriting all the tests and does bring some extra lines per test.

However as you also say it makes sense to parse the top level into a Block no matter what, as one could imagine that one wanted to have subsequent calls to forward be run in a separate block as to not intervene with each other.

@jasonwilliams jasonwilliams merged commit ffcc9ad into boa-dev:master Oct 22, 2019
@barskern barskern deleted the variable-scoping branch October 22, 2019 20:07
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.

Block scope not being used on variable allocation
2 participants