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

Fix plugin-transform-block-scoping const violations #13248

Merged
merged 5 commits into from May 3, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 35 additions & 5 deletions packages/babel-plugin-transform-block-scoping/src/index.js
Expand Up @@ -433,17 +433,47 @@ class BlockScoping {
]);

if (violation.isAssignmentExpression()) {
violation
.get("right")
.replaceWith(
t.sequenceExpression([throwNode, violation.get("right").node]),
const { operator } = violation.node;
if (["=", "&&=", "||=", "??="].includes(operator)) {
violation
.get("right")
.replaceWith(
t.sequenceExpression([violation.get("right").node, throwNode]),
);
} else {
violation.replaceWith(
t.sequenceExpression([
t.binaryExpression(
operator.slice(0, -1),
violation.get("left").node,
violation.get("right").node,
),
throwNode,
]),
);
}
} else if (violation.isUpdateExpression()) {
violation.replaceWith(
t.sequenceExpression([throwNode, violation.node]),
t.sequenceExpression([
t.binaryExpression(
violation.node.operator[0],
violation.get("argument").node,
t.numericLiteral(1),
),
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify this to t.unaryExpression("+", violation.get("argument").node) (even if it saves literally a single byte).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, +c has same effect of triggering .valueOf() as c + 1. Have added a commit to make this change.

I hadn't really bothered with optimization since in most cases code like this is a bug anyway.

Have also added another commit for the other most obvious optimization - shorten c = f() to just f() since the assignment will never be executed anyway. There are other optimizations which could be made (a += 'long_string' to a + '' for example), but in my view going further would add complexity for little gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one more commit to shorten c &&= x++ -> c && x++.

throwNode,
]),
);
} else if (violation.isForXStatement()) {
violation.ensureBlock();
violation
.get("left")
.replaceWith(
t.variableDeclaration("var", [
t.variableDeclarator(
violation.scope.generateUidIdentifier(name),
),
]),
);
violation.node.body.body.unshift(t.expressionStatement(throwNode));
}
}
Expand Down
@@ -0,0 +1,41 @@
const state1 = {};
expect(function() {
const a = 3;
let b = 1;
state1.getA = () => a;
state1.getB = () => b;

a += b++;
}).toThrow('"a" is read-only');
expect(state1.getA()).toBe(3); // Assignment did not succeed
expect(state1.getB()).toBe(2); // `b++` was evaluated before error thrown

const state2 = {};
expect(function() {
const a = {
valueOf() {
state2.valueOfIsCalled = true;
}
};
state2.a = a;
state2.getA = () => a;
state2.getB = () => b;

let b = 1;
a += b++;
}).toThrow('"a" is read-only');
expect(state2.getA()).toBe(state2.a); // Assignment did not succeed
expect(state2.getB()).toBe(2); // `b++` was evaluated before error thrown
expect(state2.valueOfIsCalled).toBe(true); // `a` was read before error thrown

const state3 = {};
expect(function() {
const a = 32;
let b = 1;
state3.getA = () => a;
state3.getB = () => b;

a >>>= ++b;
}).toThrow('"a" is read-only');
expect(state3.getA()).toBe(32); // Assignment did not succeed
expect(state3.getB()).toBe(2); // `++b` was evaluated before error thrown
@@ -0,0 +1,11 @@
const a = 5;
let b = 0;
a += b++;

const c = 32;
let d = 1;
c >>>= ++d;

const e = 0;
let f = 1;
e ||= ++f;
@@ -0,0 +1,9 @@
var a = 5;
var b = 0;
a + b++, babelHelpers.readOnlyError("a");
var c = 32;
var d = 1;
c >>> ++d, babelHelpers.readOnlyError("c");
var e = 0;
var f = 1;
e ||= (++f, babelHelpers.readOnlyError("e"));
@@ -1,5 +1,5 @@
(function () {
var a = "foo";
if (false) a = (babelHelpers.readOnlyError("a"), "false");
if (false) a = ("false", babelHelpers.readOnlyError("a"));
return a;
})();
@@ -1,3 +1,3 @@
var a = 1,
b = 2;
a = (babelHelpers.readOnlyError("a"), 3);
a = (3, babelHelpers.readOnlyError("a"));
@@ -1,3 +1,7 @@
for (const i = 0; i < 3; i = i + 1) {
console.log(i);
}

for (const j = 0; j < 3; j++) {
console.log(j);
}
@@ -1,3 +1,7 @@
for (var i = 0; i < 3; i = (babelHelpers.readOnlyError("i"), i + 1)) {
for (var i = 0; i < 3; i = (i + 1, babelHelpers.readOnlyError("i"))) {
console.log(i);
}

for (var j = 0; j < 3; j + 1, babelHelpers.readOnlyError("j")) {
console.log(j);
}
Expand Up @@ -2,5 +2,5 @@ var c = 17;
var a = 0;

function f() {
return (babelHelpers.readOnlyError("c"), ++c) + --a;
return (c + 1, babelHelpers.readOnlyError("c")) + --a;
}
@@ -1,4 +1,20 @@
const state1 = {};
expect(function() {
const a = 3;
state1.getA = () => a;

a = 7;
}).toThrow('"a" is read-only');
expect(state1.getA()).toBe(3); // Assignment did not succeed

const state2 = {};
expect(function() {
const a = 3;
let b = 0;
state2.getA = () => a;
state2.getB = () => b;

a = b++;
}).toThrow('"a" is read-only');
expect(state2.getA()).toBe(3); // Assignment did not succeed
expect(state2.getB()).toBe(1); // `b++` was evaluated before error thrown
@@ -1,3 +1,6 @@
const MULTIPLIER = 5;

MULTIPLIER = "overwrite";

const a = 5;
let b = 0;
a = b++;
@@ -1,2 +1,5 @@
var MULTIPLIER = 5;
MULTIPLIER = (babelHelpers.readOnlyError("MULTIPLIER"), "overwrite");
MULTIPLIER = ("overwrite", babelHelpers.readOnlyError("MULTIPLIER"));
var a = 5;
var b = 0;
a = (b++, babelHelpers.readOnlyError("a"));
@@ -1,5 +1,8 @@
const state = {};
function f(arr) {
const MULTIPLIER = 5;
state.getMultiplier = () => MULTIPLIER;

for (MULTIPLIER in arr);

return 'survived';
Expand All @@ -8,5 +11,6 @@ function f(arr) {
expect(function() {
f([1,2,3]);
}).toThrow('"MULTIPLIER" is read-only');
expect(state.getMultiplier()).toBe(5); // Assignment did not succeed

expect(f([])).toBe('survived');
@@ -1,6 +1,6 @@
var MULTIPLIER = 5;

for (MULTIPLIER in arr) {
for (var _MULTIPLIER in arr) {
babelHelpers.readOnlyError("MULTIPLIER");
;
}
@@ -1,4 +1,23 @@
const state1 = {};
expect(function() {
const a = "str";
state1.getA = () => a;

--a;
}).toThrow('"a" is read-only');
expect(state1.getA()).toBe("str"); // Assignment did not succeed

const state2 = {};
expect(function() {
const b = {
valueOf() {
state2.valueOfIsCalled = true;
}
};
state2.b = b;
state2.getB = () => b;

--b;
}).toThrow('"b" is read-only');
expect(state2.getB()).toBe(state2.b); // Assignment did not succeed
expect(state2.valueOfIsCalled).toBe(true); // `bar` was read before error thrown
@@ -1,2 +1,2 @@
var a = "str";
babelHelpers.readOnlyError("a"), --a;
a - 1, babelHelpers.readOnlyError("a");
@@ -1,4 +1,23 @@
const state1 = {};
expect(function() {
const foo = 1;
state1.getFoo = () => foo;

foo++;
}).toThrow('"foo" is read-only');
expect(state1.getFoo()).toBe(1); // Assignment did not succeed

const state2 = {};
expect(function() {
const bar = {
valueOf() {
state2.valueOfIsCalled = true;
}
};
state2.bar = bar;
state2.getBar = () => bar;

bar++;
}).toThrow('"bar" is read-only');
expect(state2.getBar()).toBe(state2.bar); // Assignment did not succeed
expect(state2.valueOfIsCalled).toBe(true); // `bar` was read before error thrown
@@ -1,2 +1,2 @@
var foo = 1;
babelHelpers.readOnlyError("foo"), foo++;
foo + 1, babelHelpers.readOnlyError("foo");