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

Nested scope closure #127

Merged
merged 2 commits into from May 11, 2018
Merged

Nested scope closure #127

merged 2 commits into from May 11, 2018

Conversation

efaust
Copy link
Collaborator

@efaust efaust commented May 11, 2018

There are two bugs here:

First, catch blocks had mismatched stack pushes and pops, leading to total confusion.
Second, we were blurring all seen names into flat levels, meaning that we couldn't tell sibling uses and bindings from nested uses and bindings.

Previously,

var a;
(function () {
    a;
    {
        let a;
    }
})

did not mark the var as closed over, even though it clearly is, since it saw the sibling binding.

let is_bound = bindings.iter()
.find(|container| container.contains(&name))
.is_some();
if !is_bound {
self.free_names_in_nested_functions.insert(name);
let parent_free = self.free_names_in_block_stack.last_mut();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

if let Some(mut parent_free) = self.free_names-in_block_stack.last_mut() {
  // ...
}

and get rid of unwrapped.

let parent_free = self.free_names_in_block_stack.last_mut();
if parent_free.is_some() {
let mut unwrapped = parent_free.unwrap();
if unwrapped.get(&name) == Some(&true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Here, you should inspect a value, rather than create a new one for comparison purposes.

if let Some(&true) = unwrapped.get(&name) {
  // ...
}

Except in this case, it looks like you want to update the entry, so it's rather something along the lines of

let new_is_cross_function = old_cross_function || function_scope;

unwrapped.entry(&name)
  // Modify the entry if it is already present.
  .and_modify(new_is_cross_function)
  // Or insert a new value.
  .or_insert(new_is_cross_function);

fn pop_free_names(&mut self, bindings: &[&HashSet<String>]) {
let mut free_names_in_current_function = self.free_names_in_function_stack.pop().unwrap();
for name in free_names_in_current_function.drain() {
fn pop_free_names(&mut self, bindings: &[&HashSet<String>], function_scope: bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Any chance you could replace function_scope with an enum? This would make things a bit more readable for callers.

Also, if it's a boolean, I'd rather call it is_function_scope.

let is_bound = bindings.iter()
.find(|container| container.contains(&name))
.is_some();
if !is_bound {
self.free_names_in_nested_functions.insert(name);
if let Some(mut parent_free) = self.free_names_in_block_stack.last_mut() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: A comment explaining what you're doing here would be nice.

@efaust efaust merged commit e427f1b into binast:master May 11, 2018
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