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

Do not create invalid code when simplifying object pattern assignments #4204

Merged
merged 2 commits into from Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
17 changes: 15 additions & 2 deletions src/ast/nodes/AssignmentExpression.ts
Expand Up @@ -17,6 +17,7 @@ import { EMPTY_PATH, ObjectPath, UNKNOWN_PATH } from '../utils/PathTracker';
import Variable from '../variables/Variable';
import Identifier from './Identifier';
import * as NodeType from './NodeType';
import ObjectPattern from './ObjectPattern';
import { ExpressionNode, IncludeChildren, NodeBase } from './shared/Node';
import { PatternNode } from './shared/Pattern';

Expand Down Expand Up @@ -88,7 +89,8 @@ export default class AssignmentExpression extends NodeBase {
removeLineBreaks(code, inclusionStart, this.right.start);
}
this.right.render(code, options, {
renderedParentType: renderedParentType || renderedSurroundingElement || this.parent.type
renderedParentType: renderedParentType || this.parent.type,
renderedSurroundingElement: renderedSurroundingElement || this.parent.type
});
}
if (options.format === 'system') {
Expand All @@ -108,6 +110,7 @@ export default class AssignmentExpression extends NodeBase {
options
);
}
return;
}
} else {
const systemPatternExports: Variable[] = [];
Expand All @@ -117,13 +120,23 @@ export default class AssignmentExpression extends NodeBase {
systemPatternExports,
this.start,
this.end,
(renderedParentType || renderedSurroundingElement) === NodeType.ExpressionStatement,
renderedSurroundingElement === NodeType.ExpressionStatement,
code,
options
);
return;
}
}
}
if (
this.left.included &&
this.left instanceof ObjectPattern &&
(renderedSurroundingElement === NodeType.ExpressionStatement ||
renderedSurroundingElement === NodeType.ArrowFunctionExpression)
) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
}
}

protected applyDeoptimizations(): void {
Expand Down
6 changes: 2 additions & 4 deletions src/ast/nodes/BinaryExpression.ts
Expand Up @@ -89,11 +89,9 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
{ renderedSurroundingElement }: NodeRenderOptions = BLANK
): void {
this.left.render(code, options, {
renderedSurroundingElement: renderedParentType || renderedSurroundingElement
});
this.left.render(code, options, { renderedSurroundingElement });
this.right.render(code, options);
}
}
5 changes: 2 additions & 3 deletions src/ast/nodes/CallExpression.ts
Expand Up @@ -246,12 +246,11 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
{ renderedSurroundingElement }: NodeRenderOptions = BLANK
): void {
const surroundingELement = renderedParentType || renderedSurroundingElement;
this.callee.render(code, options, {
isCalleeOfRenderedParent: true,
renderedSurroundingElement: surroundingELement
renderedSurroundingElement
});
if (this.arguments.length > 0) {
if (this.arguments[this.arguments.length - 1].included) {
Expand Down
5 changes: 2 additions & 3 deletions src/ast/nodes/ClassExpression.ts
Expand Up @@ -10,11 +10,10 @@ export default class ClassExpression extends ClassNode {
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
{ renderedSurroundingElement }: NodeRenderOptions = BLANK
): void {
super.render(code, options);
const surroundingElement = renderedParentType || renderedSurroundingElement;
if (surroundingElement === NodeType.ExpressionStatement) {
if (renderedSurroundingElement === NodeType.ExpressionStatement) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
}
Expand Down
9 changes: 3 additions & 6 deletions src/ast/nodes/ConditionalExpression.ts
Expand Up @@ -206,14 +206,11 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz
usedBranch!.render(code, options, {
isCalleeOfRenderedParent,
preventASI: true,
...(renderedSurroundingElement
? { renderedSurroundingElement }
: { renderedParentType: renderedParentType || this.parent.type })
renderedParentType: renderedParentType || this.parent.type,
renderedSurroundingElement: renderedSurroundingElement || this.parent.type
});
} else {
this.test.render(code, options, {
renderedSurroundingElement: renderedParentType || renderedSurroundingElement
});
this.test.render(code, options, { renderedSurroundingElement });
this.consequent.render(code, options);
this.alternate.render(code, options);
}
Expand Down
3 changes: 1 addition & 2 deletions src/ast/nodes/ExportDefaultDeclaration.ts
Expand Up @@ -99,8 +99,7 @@ export default class ExportDefaultDeclaration extends NodeBase {
} else {
code.remove(this.start, declarationStart);
this.declaration.render(code, options, {
isCalleeOfRenderedParent: false,
renderedParentType: NodeType.ExpressionStatement
renderedSurroundingElement: NodeType.ExpressionStatement
});
if (code.original[this.end - 1] !== ';') {
code.appendLeft(this.end, ';');
Expand Down
5 changes: 2 additions & 3 deletions src/ast/nodes/FunctionExpression.ts
Expand Up @@ -10,11 +10,10 @@ export default class FunctionExpression extends FunctionNode {
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
{ renderedSurroundingElement }: NodeRenderOptions = BLANK
): void {
super.render(code, options);
const surroundingElement = renderedParentType || renderedSurroundingElement;
if (surroundingElement === NodeType.ExpressionStatement) {
if (renderedSurroundingElement === NodeType.ExpressionStatement) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
}
Expand Down
7 changes: 3 additions & 4 deletions src/ast/nodes/LogicalExpression.ts
Expand Up @@ -191,14 +191,13 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
this.getUsedBranch()!.render(code, options, {
isCalleeOfRenderedParent,
preventASI,
...(renderedSurroundingElement
? { renderedSurroundingElement }
: { renderedParentType: renderedParentType || this.parent.type })
renderedParentType: renderedParentType || this.parent.type,
renderedSurroundingElement: renderedSurroundingElement || this.parent.type
});
} else {
this.left.render(code, options, {
preventASI,
renderedSurroundingElement: renderedParentType || renderedSurroundingElement
renderedSurroundingElement
});
this.right.render(code, options);
}
Expand Down
7 changes: 1 addition & 6 deletions src/ast/nodes/MemberExpression.ts
Expand Up @@ -316,12 +316,7 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
if (renderedParentType && isCalleeOfRenderedParent) {
code.appendRight(this.start, '0, ');
}
const surroundingElement = renderedParentType || renderedSurroundingElement;
this.object.render(
code,
options,
surroundingElement ? { renderedSurroundingElement: surroundingElement } : BLANK
);
this.object.render(code, options, { renderedSurroundingElement });
this.property.render(code, options);
}
}
Expand Down
7 changes: 3 additions & 4 deletions src/ast/nodes/ObjectExpression.ts
Expand Up @@ -90,13 +90,12 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
{ renderedSurroundingElement }: NodeRenderOptions = BLANK
): void {
super.render(code, options);
const surroundingElement = renderedParentType || renderedSurroundingElement;
if (
surroundingElement === NodeType.ExpressionStatement ||
surroundingElement === NodeType.ArrowFunctionExpression
renderedSurroundingElement === NodeType.ExpressionStatement ||
renderedSurroundingElement === NodeType.ArrowFunctionExpression
) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
Expand Down
16 changes: 6 additions & 10 deletions src/ast/nodes/SequenceExpression.ts
Expand Up @@ -121,16 +121,12 @@ export default class SequenceExpression extends NodeBase {
removeLineBreaks(code, start, node.start);
}
if (includedNodes === 1) {
if (node === lastNode) {
node.render(code, options, {
isCalleeOfRenderedParent,
renderedParentType: renderedParentType || this.parent.type
});
} else {
node.render(code, options, {
renderedSurroundingElement: renderedParentType || this.parent.type
});
}
const parentType = renderedParentType || this.parent.type;
node.render(code, options, {
isCalleeOfRenderedParent: isCalleeOfRenderedParent && node === lastNode,
renderedParentType: parentType,
renderedSurroundingElement: parentType
});
} else {
node.render(code, options);
}
Expand Down
2 changes: 1 addition & 1 deletion src/ast/nodes/VariableDeclarator.ts
Expand Up @@ -56,7 +56,7 @@ export default class VariableDeclarator extends NodeBase {
this.init.render(
code,
options,
renderId ? BLANK : { renderedParentType: NodeType.ExpressionStatement }
renderId ? BLANK : { renderedSurroundingElement: NodeType.ExpressionStatement }
);
} else if (
this.id instanceof Identifier &&
Expand Down
8 changes: 6 additions & 2 deletions src/utils/renderHelpers.ts
Expand Up @@ -23,8 +23,12 @@ export interface NodeRenderOptions {
isNoStatement?: boolean;
isShorthandProperty?: boolean;
preventASI?: boolean;
renderedParentType?: string; // also serves as a flag if the rendered parent is different from the actual parent
renderedSurroundingElement?: string; // same as parent type, but for changed non-direct parents that directly precede elements
/* Indicates if the direct parent of an element changed.
Necessary for determining the "this" context of callees. */
renderedParentType?: string;
/* Indicates if the parent or ancestor surrounding an element has changed and what it changed to.
Necessary for adding parentheses. */
renderedSurroundingElement?: string;
start?: number;
}

Expand Down
Expand Up @@ -9,17 +9,17 @@ true ? function foo(x){
value = x;
}("consequent") : 2;

assert.equal(value, 'consequent');
assert.strictEqual(value, 'consequent');

foo("incorrect");

assert.equal(value, 'foo');
assert.strictEqual(value, 'foo');

false ? null: function foo(x){
value = x;
}("alternate");

assert.equal(value, 'alternate');
assert.strictEqual(value, 'alternate');

// logical expression
function bar(){
Expand All @@ -30,17 +30,17 @@ true && function bar(x){
value = x;
}("and");

assert.equal(value, 'and');
assert.strictEqual(value, 'and');

bar("incorrect");

assert.equal(value, 'bar');
assert.strictEqual(value, 'bar');

false || function bar(x){
value = x;
}("or");

assert.equal(value, 'or');
assert.strictEqual(value, 'or');

// sequence expression
function baz(){
Expand All @@ -50,9 +50,8 @@ function baz(){
0, function baz(x){
value = x;
}("comma");

assert.equal(value, 'comma');
assert.strictEqual(value, 'comma');

baz("incorrect");

assert.equal(value, 'baz');
assert.strictEqual(value, 'baz');
3 changes: 3 additions & 0 deletions test/function/samples/simplify-with-destructuring/_config.js
@@ -0,0 +1,3 @@
module.exports = {
description: 'handle destructuring assignments in simplified sequence expressions'
};
7 changes: 7 additions & 0 deletions test/function/samples/simplify-with-destructuring/main.js
@@ -0,0 +1,7 @@
let foo, unused;
null, { foo } = { foo: 'bar' };
assert.strictEqual(foo, 'bar');

const assign = () => unused = { foo } = { foo: 'baz' };
assign();
assert.strictEqual(foo, 'baz');