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

Use first binding for multiple var declarations #5745

Merged
merged 6 commits into from
Jul 24, 2017

Conversation

peey
Copy link
Contributor

@peey peey commented May 18, 2017

Q A
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets Fixes #2378
License MIT
Doc PR no
Dependency Changes no

Since var declarations after initial binding have no effect, use the first declaration (instead of last, as we were currently doing). This prevents a param binding from turning into a var binding when a var declaration of the same identifier is made in the function body. Hence it fixes #2378

Since var declarations after initial binding have no effect, use the
first declaration. Fixes babel#2378
@mention-bot
Copy link

@peey, thanks for your PR! By analyzing the history of the files in this pull request, we identified @existentialism and @loganfsmyth to be potential reviewers.

@codecov
Copy link

codecov bot commented May 18, 2017

Codecov Report

Merging #5745 into 7.0 will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##              7.0   #5745      +/-   ##
=========================================
- Coverage   84.63%   84.6%   -0.04%     
=========================================
  Files         282     282              
  Lines        9861    9864       +3     
  Branches     2766    2767       +1     
=========================================
- Hits         8346    8345       -1     
- Misses        997    1001       +4     
  Partials      518     518
Impacted Files Coverage Δ
packages/babel-traverse/src/scope/binding.js 75.6% <100%> (+1.92%) ⬆️
packages/babel-helper-call-delegate/src/index.js 64% <0%> (-4%) ⬇️
packages/babel-traverse/src/path/modification.js 73.78% <0%> (-0.98%) ⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.12% <0%> (-0.86%) ⬇️
packages/babel-traverse/src/path/context.js 86.2% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce6f672...98bef29. Read the comment docs.

@hzoo hzoo requested a review from jridgewell July 21, 2017 00:31
Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

@peey
Copy link
Contributor Author

peey commented Jul 21, 2017

@jridgewell I forgot. I'll have to check.

Also, this shouldn't be merged yet, I just noticed an error in one of the tests: this should also have been renamed to z

I'll patch it up and update here

@peey
Copy link
Contributor Author

peey commented Jul 24, 2017

@jridgewell I don't think we need it, so I have removed it. The reason being that there are only two categories of redeclarations - one we're fine with and one that result in an error.

The redeclarations which result in an error are taken care of here

And rest of them (like var on param or hoisted on var, etc) result in creation of a new Binding where the code in this PR will take care of the bindings.

@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jul 24, 2017
@hzoo hzoo merged commit 2225892 into babel:7.0 Jul 24, 2017
@hzoo hzoo mentioned this pull request Jul 28, 2017
jridgewell pushed a commit that referenced this pull request Sep 9, 2017
* Redeclaring a variable counts as a modification.

Fixes #6217.

* Remove "existing" logic from Binding.

Was added in #5745, but no longer triggered since 6536e60.
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants