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

Spec compliancy of check-es2015-constants plugin #5930

Merged
merged 7 commits into from Jul 18, 2017

Conversation

Projects
None yet
8 participants
@maurobringolf
Contributor

maurobringolf commented Jul 8, 2017

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

I started working on the proposed change to the check-es2015-constants plugin: Instead of throwing a compile time error when const is violated, Babel should insert a throw statement before the violation. This will make it a runtime error and violations in code that is not executed are fine. I added the example from the issue as a test case and changed existing test cases.

I am not sure if this covers all possible violations besides modules. Is there a list of all possible violation types somewhere? Variable declarations via module import statements are not yet covered by this PR. Let me know if this is going into the right direction or if I should take a different route.

@maurobringolf

This comment has been minimized.

Contributor

maurobringolf commented Jul 8, 2017

Actually I just realized that inserting before the violation will break in many many many cases. I think the throwStatement needs to replace the violation instead.

@jridgewell

This comment has been minimized.

Member

jridgewell commented Jul 9, 2017

Replacing won't work either. You'll need to find the statement parent and insert before that.

@maurobringolf

This comment has been minimized.

Contributor

maurobringolf commented Jul 9, 2017

Agreed, finding a parent statement seems more reasonable. Do you have an example where my current implementation fails? I tried using path.getStatementParent and inserting before that. It does not behave correctly in the example from the issue:

Input

const a = "foo";
if (false) a = "false";

Output

const a = "foo";
throw new Error("\"a\" is read-only");
if (false) a = "false";

I checked and indeed, violation.getStatementParent returns the IfStatement in this case. This is because of the shorthand IfStatement. When using curly braces around the violation, it works correctly because then getStatementParent will return the actual if clause and not the complete IfStatement.

The same happens with all loop constructs that can either have a single expression or a block as body. Do I have to manually work around these or is there a better way than getStatementParent?

EDIT: Found more cases where the statement parent is not the right place:

const a = 3
const b = () => a = 4
@jridgewell

This comment has been minimized.

Member

jridgewell commented Jul 10, 2017

const before = path.findParent(p => p.isStatement() || p.parentPath.isArrowExpression());
var MULTIPLIER = 5;
throw new Error("\"MULTIPLIER\" is read-only");
for (MULTIPLIER in arr) {}

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 12, 2017

Member

This should be

var MULTIPLIER = 5;

for (MULTIPLIER in arr) {
  throw new Error("\"MULTIPLIER\" is read-only");
}

Because if arr is an empty object, MULTIPLIER is never reassigned and thus it shouldn't throw.

This comment has been minimized.

@maurobringolf

maurobringolf Jul 12, 2017

Contributor

Ah true, thank you! I will change the forXStatement case then.

@@ -44,7 +44,15 @@ export default function({ messages, types: t }) {
]),
);
} else if (violation.parentPath.isForXStatement()) {
violation.parentPath.insertBefore(throwNode);
// Transform single statement body into BlockStatement
if (!violation.parentPath.get("body").isBlockStatement()) {

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 12, 2017

Member

I think you can use violation.parentPath.ensureBlock() (I'm not 100% sure because I've never used it, but I found it while looking at random babel files 😆 )

This comment has been minimized.

@maurobringolf

maurobringolf Jul 12, 2017

Contributor

Ah cool! As far as I can tell it uses this converter function, so it is probably appropriate:

export function toBlock(node: Object, parent: Object): Object {
if (t.isBlockStatement(node)) {
return node;
}
if (t.isEmptyStatement(node)) {
node = [];
}
if (!Array.isArray(node)) {
if (!t.isStatement(node)) {
if (t.isFunction(parent)) {
node = t.returnStatement(node);
} else {
node = t.expressionStatement(node);
}
}

I will give it a try, thanks!

.get("right")
.replaceWith(
t.sequenceExpression([
t.callExpression(

This comment has been minimized.

@buunguyen

buunguyen Jul 12, 2017

Member

Duplicated code with below. Should move to a variable?

This comment has been minimized.

@maurobringolf

maurobringolf Jul 12, 2017

Contributor

Sounds good, I will create one for the comma expression. Should be less lines and easier to follow.

violation.get("right").node,
]),
);
} else if (violation.parentPath.isUpdateExpression()) {

This comment has been minimized.

@buunguyen

buunguyen Jul 12, 2017

Member

Is there a test case for this branch?

This comment has been minimized.

@maurobringolf

maurobringolf Jul 12, 2017

Contributor

Currently there is only one in packages/babel-plugin-check-es2015-constants/test/fixtures/general/update-expression. Could probably use more since this can show up in a lot of wierd places I think.

This comment has been minimized.

@buunguyen

buunguyen Jul 12, 2017

Member

Thanks. I'm quite surprised that the check happens at parentPath instead of directly on violation like the first branch. I guess violations are collected differently for assignment vs update expressions.

This comment has been minimized.

@maurobringolf

maurobringolf Jul 12, 2017

Contributor

Yes I thought that was confusing as well. Some violations are given as the identifier nodes and some are their parents, but I am not sure why. If these violations are not used anywhere else it might make sense to change it for the sake of consistency.

This comment has been minimized.

@buunguyen

buunguyen Jul 12, 2017

Member

I think that's a good idea. Maybe it should be addressed in a separate PR and see what other folks feedback.

@@ -0,0 +1,4 @@
var MULTIPLIER = 5;
MULTIPLIER = (function () {

This comment has been minimized.

@buunguyen

buunguyen Jul 13, 2017

Member

I'm sure these would pass, but would be great to add test for destructuring assignments, e.g.

const a = 1;
[a] = [2];

const b = 1;
({b} = {b: 2})

This comment has been minimized.

@maurobringolf

maurobringolf Jul 13, 2017

Contributor

Definitely, I want to add executed tests as well to assert the runtime error is thrown. I need to look at how other packages do these tests first though, because everything I have tried so far did not work.

@xtuc

This comment has been minimized.

Member

xtuc commented Jul 13, 2017

Is this a breaking change btw? Users won't expect their code to throw at runtime, but atm they wouldn't be able to transpile it anyway.

@maurobringolf

This comment has been minimized.

Contributor

maurobringolf commented Jul 13, 2017

I am not sure if it classifies as breaking change since as you said, runtime errors are only added in cases which did not even get transpiled before. I personally dont see it as a breaking change.

Mauro Bringolf
@buunguyen

lgtm

@xtuc

This comment has been minimized.

Member

xtuc commented Jul 17, 2017

I liked the old behavior, could we add an option throwAtCompile: Boolean to restore the error at compile time?

I can make a separate PR.

@xtuc

xtuc approved these changes Jul 17, 2017

otherwise LGTM, nice work @maurobringolf 👍

);
// Returns a comma expression of IIFE wrapped throwNode followed by given expression.
const throwBefore = expression =>

This comment has been minimized.

@jridgewell

jridgewell Jul 17, 2017

Member

This can be hoisted outside the visitor by passing in the throwNode, too.

This comment has been minimized.

@maurobringolf

maurobringolf Jul 17, 2017

Contributor

I considered this and decided to keep it inside the visitor for simplicity. But I'm fine with both ways and can change it for sure.

This comment has been minimized.

@jridgewell

jridgewell Jul 17, 2017

Member

We're redefining a function in a loop, which is super slow.

@jridgewell jridgewell merged commit aa684d1 into babel:7.0 Jul 18, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 85.69% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maurobringolf

This comment has been minimized.

Contributor

maurobringolf commented Jul 18, 2017

Awesome, thanks I learned a lot!

@hzoo

This comment has been minimized.

Member

hzoo commented Jul 18, 2017

@xtuc making lots of options kinda sucks so I'd suggest just adding it as "loose" given this is spec even though it's an error 😄

@xtuc

This comment has been minimized.

Member

xtuc commented Jul 18, 2017

Yeah I agree but isn't it strange to have loose enabled and get an error at compile time?

@hzoo

This comment has been minimized.

Member

hzoo commented Jul 18, 2017

Yes, it's "weird" but nothing about loose mode says no errors, it literally just means loose about the spec. Otherwise I would just not run the plugin at all?

@jridgewell

This comment has been minimized.

Member

jridgewell commented Jul 18, 2017

Maybe this behavior should be spec, and the old throw should be regular?

@xtuc

This comment has been minimized.

Member

xtuc commented Jul 18, 2017

Yes but spec behavior should be by default. But than what's the difference between spec: false and loose: true lol

@loganfsmyth

This comment has been minimized.

Member

loganfsmyth commented Jul 18, 2017

I think http://eslint.org/docs/rules/no-const-assign is a better approach to get a warning right away, since ESLint will even nicely highlight the line for you in your editor.

@hzoo

This comment has been minimized.

Member

hzoo commented Jul 18, 2017

@xtuc 😄 . I don't think spec: false or loose: false would be a valid option

@loganfsmyth

This comment has been minimized.

Member

loganfsmyth commented Jul 18, 2017

In the original issue, #5728, I tried to say we should split this up into the two plugins that actually need it (block-scoping and modules), but that doesn't appear to have happened in this PR. Would anyone mind if I file a new issue about that?

As it is, if you turn this on for block scoping for instance, you also get it for modules, e.g.

import foo from "bar";

foo = "value";

with

{
  presets: [['es2015', {modules: false}],
}

will still inject the logic to throw, even though you turned off processing of modules, and foo is not a block-scoped binding. To me it seems like import bindings should be handled by the module transform and const bindings should be handled by block-scoping, and we should delete check-es2015-constants entirely.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment