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

Consistent const violations #6100

Merged
merged 3 commits into from Aug 25, 2017

Conversation

Projects
None yet
7 participants
@maurobringolf
Contributor

maurobringolf commented Aug 13, 2017

Q                       A
Fixed Issues
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? no
Tests Added/Pass? no
Spec Compliancy? no
License MIT
Doc PR
Any Dependency Changes?

When I updated the check constants plugin from compile time to runtime errors in #5930, I discovered a weird inconsistency in the way violations are collected. A violating UpdateExpression for example stores the identifier as violation, while assignmentExpression stores itself. As @buunguyen suggested here, this PR cleans up this inconsistency, so its only internal. Because the mechanism for collecting violations relies on getBindingIdentifiers I had to include forXStatement's into this method in babel-types. Is there a specific reason why it is currently not there?

UPDATE: One test case concerning rest parameter optimization is currently failing. The reason seems to be the updated way of how UpdateExpression marks a const violation. I have not figured out the exact reason yet, but I will leave this PR here anyway since I think this change would be a nice cleanup.

@mention-bot

This comment has been minimized.

mention-bot commented Aug 13, 2017

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

@Qantas94Heavy

This comment has been minimized.

Member

Qantas94Heavy commented Aug 22, 2017

That's probably to do with that test being changed recently (#6116). Try rebasing your PR and the CI suite should run successfully.

@@ -140,7 +140,7 @@ const collectorVisitor = {
},
UpdateExpression(path, state) {
state.constantViolations.push(path.get("argument"));
state.constantViolations.push(path);
},
UnaryExpression(path, state) {

This comment has been minimized.

@Qantas94Heavy

Qantas94Heavy Aug 22, 2017

Member

Should this also apply to the delete operator? It appears the actual property is being placed as the violation rather than the operation itself.

This comment has been minimized.

@maurobringolf

maurobringolf Aug 22, 2017

Contributor

As far as I know the delete operator cannot violate the const property ( this would be missing from the current check constants plugin). I am not sure why it is added and therefore did not change it.

Why is a delete node added to constantViolations?

This comment has been minimized.

@jridgewell

jridgewell Aug 23, 2017

Member

Yah, I'm not sure the proper behavior here.

This comment has been minimized.

@maurobringolf

maurobringolf Aug 23, 2017

Contributor

I removed UnaryExpression from the visitor and it did not affect any tests. Should I delete it?

I tried rebasing locally by running git pull --rebase upstream 7.0 and it did fix the broken test. However it says I have a diverging history from my fork and wants me to do another merge commit by pulling from my fork/consistent-violations. Does that sound right or is there a better way?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Aug 23, 2017

Member

However it says I have a diverging history from my fork

You can use git push -f to override your remote fork with your local one.

This comment has been minimized.

@maurobringolf

maurobringolf Aug 23, 2017

Contributor

Thank you @nicolo-ribaudo ! Looks like all worked out.

This comment has been minimized.

@jridgewell

jridgewell Aug 23, 2017

Member

Let's leave deletes in, and update it so it's the UnaryExpression

This comment has been minimized.

@maurobringolf

maurobringolf Aug 23, 2017

Contributor

Can you explain why? I can do it but would like to understand the reason.

This comment has been minimized.

@jridgewell

jridgewell Aug 23, 2017

Member

Because I don't understand all the implications of delete in sloppy code, so best to leave this in if it's already here. And, we need to make it consistent with the other changes you've made.

This comment has been minimized.

@maurobringolf

maurobringolf Aug 23, 2017

Contributor

Okay, done 👍

@@ -140,7 +140,7 @@ const collectorVisitor = {
},
UpdateExpression(path, state) {
state.constantViolations.push(path.get("argument"));
state.constantViolations.push(path);
},
UnaryExpression(path, state) {

This comment has been minimized.

@jridgewell

jridgewell Aug 23, 2017

Member

Yah, I'm not sure the proper behavior here.

@babel-bot

This comment has been minimized.

Collaborator

babel-bot commented Aug 23, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4696/

@babel babel deleted a comment from babel-bot Aug 23, 2017

@xtuc

xtuc approved these changes Aug 24, 2017

@jridgewell jridgewell merged commit d8b4073 into babel:7.0 Aug 25, 2017

4 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.05% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment