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

Replace environment_stack with current_env #1197

Closed
jasonwilliams opened this issue Apr 5, 2021 · 1 comment
Closed

Replace environment_stack with current_env #1197

jasonwilliams opened this issue Apr 5, 2021 · 1 comment
Labels
E-Easy Easy good first issue Good for newcomers

Comments

@jasonwilliams
Copy link
Member

jasonwilliams commented Apr 5, 2021

Now that environment traversal has been refactored, we no longer need to hold environments in a Vec. Initially we used this to traverse through and get bindings.

This means we will only ever care about the last environment in the Vec. See examples here:
https://github.com/boa-dev/boa/blob/master/boa/src/environment/lexical_environment.rs#L154-L168

We only ever get the back environment.

Task

Instead of using a Vec<Environment> just use a single Environment placeholder to hold the current env.

  • Re-implement get_current_environment_ref and get_current_environment using self.env instead of the vector.
  • push and pop should be renamed to get and set and overwrite the current env or return it
@0x7D2B
Copy link
Contributor

0x7D2B commented Apr 9, 2021

I'm pretty sure we can't do this because it would bring back #989.

Consider the code from #989:

function f1() {
    return a
}
function f2() {
    let a = 1
    return f1()
}

f2()

f1() getting called in f2() will have a new environment with the global scope as its parent - it would not have access to a declared inside of f1, for example. The environment stack of f1 would be: Global -> f1. This means that we can't assume that a new environment will have the previous environment as its parent.

The issue in #989 was that resolving bindings was not being done correctly: it was done by traversing through the stack instead of traversing through parent environments. This created a situation where the environment stack for f1 would look like this: Global -> f2 -> f1, creating the problem of variables in f2 being visible in f1.

If we get rid of the stack and use parent environments to return to the previous environment, there will be a different problem: we won't be able to come back to f2's environment, since the parent stack of f1 would be Global.

I think it's still possible to get rid of the environment stack by manually managing environments in parts of the code that currently push and pop environments (implicitly using function calls in Rust as the stack, I suppose), but we can't replace it with traversing parent environments.

@0x7D2B 0x7D2B closed this as completed Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-Easy Easy good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants