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

Add references of default parameters and default values of destructuring #65

Merged

Conversation

mysticatea
Copy link
Contributor

Hello.

I tried to implement default parameters and default values of destructuring assignments.

  • Mainly, I changed PatternVisitor to collect right-hand nodes (readonly subtrees). At Referencer.visitPattern's last, visits the right-hand nodes recursively. This logic adds readable references for identifiers in default values.
  • Referencer.referencingDefaultValue adds writable references for parameters/variables that has its default value. the references are similar to references of VariableDeclarator.init. Patterns can be nested, can have multiple default values, so it might need an utility such as Reference.isInit.
  • I added options (processing right-hand nodes or not) to Referencer.visitPattern, because there are calling Referencer.visitPattern twice for same nodes.

Thanks!

Note: this error has been occurring on this PR version. Umm...

- I changed PatternVisitor to collect right-hand nodes (readonly
subtrees). At Referencer.visitPattern's last, visits the right-hand
nodes recursively. This logic adds readable references for identifiers
in default values.
- Referencer.referencingDefaultValue adds writable references for
parameters/variables that has its default value. It's similar to
VariableDeclarator.init.
- Because there are calling visitPattern twice for same nodes, I added
options (processing right-hand nodes or not) to visitPattern.
@mysticatea
Copy link
Contributor Author

Related: #33 #62 #64

this.rightHandNodes.push(pattern.right);
} finally {
this.assignments.pop();
}
Copy link
Member

Choose a reason for hiding this comment

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

Why these try-finally is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to be exceptions safety.
I'm afraid to leak the resource, so I use finally for a couple of open/close API (or processes of reverting) from long habit.
But I agree that it's redundant here.

@Constellation
Copy link
Member

The patch itself looks good to me. With some nits.

@mysticatea
Copy link
Contributor Author

Thank you for the review!
I will fix it.

@mysticatea
Copy link
Contributor Author

I'm sorry, I might have found a bug.
Please wait...

@mysticatea
Copy link
Contributor Author

I have a wonder on this code:

class Foo { }

The global scope has a variable of Foo, and the Foo's class-body scope has a variable of Foo.
Thus there are two Foo's variables.
True, calls __define twice at https://github.com/estools/escope/blob/master/src/referencer.js#L256-285

This is an expected behavior?

I guess related to eslint/eslint#2545

@michaelficarra
Copy link
Member

Yes, that is expected. Classes create two bindings: a mutable outer binding and an immutable inner binding.

@Constellation
Copy link
Member

@mysticatea, @michaelficarra:

Ah thanks. Yes, that's expected behavior.

@mysticatea
Copy link
Contributor Author

I understood!
Thank you for explain.

So, I think, I completed this PR.

try {
this.visit(node.left);
this.rightHandNodes.push(node.right);
} finally {
Copy link
Member

Choose a reason for hiding this comment

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

Ah, let's drop this try-finally.

The other part looks very nice to me :D
Thank you for your great patch! So after fixing it, I'll merge your PR soon!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry!

@mysticatea mysticatea force-pushed the add-references-of-initializers branch from 52b6167 to 766ce6c Compare May 18, 2015 14:16
@Constellation
Copy link
Member

OK, merging...

Constellation added a commit that referenced this pull request May 18, 2015
Add references of default parameters and default values of destructuring
@Constellation Constellation merged commit b3c8e5e into estools:master May 18, 2015
@Constellation
Copy link
Member

Thank you for your great contribution!!!

@mysticatea
Copy link
Contributor Author

Yeah! Thank you for reviewing and merging!!

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.

3 participants