From 1059e99c7c64a712404b80b8cff61fa0eeed3586 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 6 Aug 2019 13:22:24 +0200 Subject: [PATCH 1/2] Prevent ASI errors for conditional expressions --- src/ast/nodes/ConditionalExpression.ts | 29 +++++++++++++------ src/ast/nodes/ReturnStatement.ts | 2 +- src/utils/renderHelpers.ts | 1 + .../_config.js | 4 +++ .../_expected.js | 7 +++++ .../keep-tree-shaking-comments-no-asi/main.js | 9 ++++++ .../prevent-tree-shaking-asi/_config.js | 9 ++++++ .../samples/prevent-tree-shaking-asi/main.js | 15 ++++++++++ 8 files changed, 66 insertions(+), 10 deletions(-) create mode 100644 test/form/samples/keep-tree-shaking-comments-no-asi/_config.js create mode 100644 test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js create mode 100644 test/form/samples/keep-tree-shaking-comments-no-asi/main.js create mode 100644 test/function/samples/prevent-tree-shaking-asi/_config.js create mode 100644 test/function/samples/prevent-tree-shaking-asi/main.js diff --git a/src/ast/nodes/ConditionalExpression.ts b/src/ast/nodes/ConditionalExpression.ts index 4cb71af5cb8..0d990838be8 100644 --- a/src/ast/nodes/ConditionalExpression.ts +++ b/src/ast/nodes/ConditionalExpression.ts @@ -1,6 +1,7 @@ import MagicString from 'magic-string'; import { BLANK } from '../../utils/blank'; import { + findFirstLineBreakOutsideComment, findFirstOccurrenceOutsideComment, NodeRenderOptions, RenderOptions @@ -151,20 +152,30 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz render( code: MagicString, options: RenderOptions, - { renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK + { renderedParentType, isCalleeOfRenderedParent, preventASI }: NodeRenderOptions = BLANK ) { if (!this.test.included) { const colonPos = findFirstOccurrenceOutsideComment(code.original, ':', this.consequent.end); + const inclusionStart = + (this.consequent.included + ? findFirstOccurrenceOutsideComment(code.original, '?', this.test.end) + : colonPos) + 1; + if (preventASI) { + const branchStart = (this.usedBranch as ExpressionNode).start; + let lineBreakPos = inclusionStart; + do { + lineBreakPos = findFirstLineBreakOutsideComment(code.original, lineBreakPos); + if (lineBreakPos >= 0 && lineBreakPos < branchStart) { + code.remove(lineBreakPos, lineBreakPos + 1); + lineBreakPos++; + } else { + lineBreakPos = -1; + } + } while (lineBreakPos >= 0); + } + code.remove(this.start, inclusionStart); if (this.consequent.included) { - const questionmarkPos = findFirstOccurrenceOutsideComment( - code.original, - '?', - this.test.end - ); - code.remove(this.start, questionmarkPos + 1); code.remove(colonPos, this.end); - } else { - code.remove(this.start, colonPos + 1); } removeAnnotations(this, code); (this.usedBranch as ExpressionNode).render(code, options, { diff --git a/src/ast/nodes/ReturnStatement.ts b/src/ast/nodes/ReturnStatement.ts index d8f41c8370e..7f2998b5ea5 100644 --- a/src/ast/nodes/ReturnStatement.ts +++ b/src/ast/nodes/ReturnStatement.ts @@ -22,7 +22,7 @@ export default class ReturnStatement extends StatementBase { render(code: MagicString, options: RenderOptions) { if (this.argument) { - this.argument.render(code, options); + this.argument.render(code, options, { preventASI: true }); if (this.argument.start === this.start + 6 /* 'return'.length */) { code.prependLeft(this.start + 6, ' '); } diff --git a/src/utils/renderHelpers.ts b/src/utils/renderHelpers.ts index 2a2af20ba64..cf0dbba649f 100644 --- a/src/utils/renderHelpers.ts +++ b/src/utils/renderHelpers.ts @@ -17,6 +17,7 @@ export interface NodeRenderOptions { isCalleeOfRenderedParent?: boolean; isNoStatement?: boolean; isShorthandProperty?: boolean; + preventASI?: boolean; renderedParentType?: string; // also serves as a flag if the rendered parent is different from the actual parent start?: number; } diff --git a/test/form/samples/keep-tree-shaking-comments-no-asi/_config.js b/test/form/samples/keep-tree-shaking-comments-no-asi/_config.js new file mode 100644 index 00000000000..74cf0990a83 --- /dev/null +++ b/test/form/samples/keep-tree-shaking-comments-no-asi/_config.js @@ -0,0 +1,4 @@ +module.exports = { + description: + 'always keep leading comments when tree-shaking and no automatic semicolons are inserted' +}; diff --git a/test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js b/test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js new file mode 100644 index 00000000000..4e8a40b8b1b --- /dev/null +++ b/test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js @@ -0,0 +1,7 @@ +console.log( + /* keep me */ + 'expected'); + +console.log( + /* keep me */ + 'expected' ); diff --git a/test/form/samples/keep-tree-shaking-comments-no-asi/main.js b/test/form/samples/keep-tree-shaking-comments-no-asi/main.js new file mode 100644 index 00000000000..18a6a0f3bac --- /dev/null +++ b/test/form/samples/keep-tree-shaking-comments-no-asi/main.js @@ -0,0 +1,9 @@ +console.log(false ? + 'unexpected' : + /* keep me */ + 'expected'); + +console.log(true ? + /* keep me */ + 'expected' : + 'unexpected'); diff --git a/test/function/samples/prevent-tree-shaking-asi/_config.js b/test/function/samples/prevent-tree-shaking-asi/_config.js new file mode 100644 index 00000000000..b3139391407 --- /dev/null +++ b/test/function/samples/prevent-tree-shaking-asi/_config.js @@ -0,0 +1,9 @@ +const assert = require('assert'); + +module.exports = { + description: 'prevent automatic semicolon insertion from changing behaviour when tree-shaking', + exports(exports) { + assert.strictEqual(exports.test1(), 'expected'); + assert.strictEqual(exports.test2(), 'expected'); + } +}; diff --git a/test/function/samples/prevent-tree-shaking-asi/main.js b/test/function/samples/prevent-tree-shaking-asi/main.js new file mode 100644 index 00000000000..eb69cf7aff0 --- /dev/null +++ b/test/function/samples/prevent-tree-shaking-asi/main.js @@ -0,0 +1,15 @@ +export function test1() { + return true ? + + + 'expected' : + 'unexpected'; +} + +export function test2() { + return false ? + 'unexpected' : + + + 'expected'; +} From 808d2da1396bcc5eb1bd556259955943250e5469 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 7 Aug 2019 07:31:05 +0200 Subject: [PATCH 2/2] Prevent ASI errors for logical and sequence expressions, also check for throw statements --- src/ast/nodes/ConditionalExpression.ts | 14 ++------ src/ast/nodes/LogicalExpression.ts | 6 +++- src/ast/nodes/SequenceExpression.ts | 10 +++--- src/ast/nodes/ThrowStatement.ts | 6 ++++ src/utils/renderHelpers.ts | 12 +++++++ .../_expected.js | 7 ++++ .../keep-tree-shaking-comments-no-asi/main.js | 8 +++++ .../prevent-tree-shaking-asi/_config.js | 7 ++-- .../samples/prevent-tree-shaking-asi/main.js | 33 +++++++++++++++++-- 9 files changed, 78 insertions(+), 25 deletions(-) diff --git a/src/ast/nodes/ConditionalExpression.ts b/src/ast/nodes/ConditionalExpression.ts index 0d990838be8..abf71d7af86 100644 --- a/src/ast/nodes/ConditionalExpression.ts +++ b/src/ast/nodes/ConditionalExpression.ts @@ -1,9 +1,9 @@ import MagicString from 'magic-string'; import { BLANK } from '../../utils/blank'; import { - findFirstLineBreakOutsideComment, findFirstOccurrenceOutsideComment, NodeRenderOptions, + removeLineBreaks, RenderOptions } from '../../utils/renderHelpers'; import { removeAnnotations } from '../../utils/treeshakeNode'; @@ -161,17 +161,7 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz ? findFirstOccurrenceOutsideComment(code.original, '?', this.test.end) : colonPos) + 1; if (preventASI) { - const branchStart = (this.usedBranch as ExpressionNode).start; - let lineBreakPos = inclusionStart; - do { - lineBreakPos = findFirstLineBreakOutsideComment(code.original, lineBreakPos); - if (lineBreakPos >= 0 && lineBreakPos < branchStart) { - code.remove(lineBreakPos, lineBreakPos + 1); - lineBreakPos++; - } else { - lineBreakPos = -1; - } - } while (lineBreakPos >= 0); + removeLineBreaks(code, inclusionStart, (this.usedBranch as ExpressionNode).start); } code.remove(this.start, inclusionStart); if (this.consequent.included) { diff --git a/src/ast/nodes/LogicalExpression.ts b/src/ast/nodes/LogicalExpression.ts index c5f38d7e7b4..56fa2bb047a 100644 --- a/src/ast/nodes/LogicalExpression.ts +++ b/src/ast/nodes/LogicalExpression.ts @@ -3,6 +3,7 @@ import { BLANK } from '../../utils/blank'; import { findFirstOccurrenceOutsideComment, NodeRenderOptions, + removeLineBreaks, RenderOptions } from '../../utils/renderHelpers'; import { removeAnnotations } from '../../utils/treeshakeNode'; @@ -155,7 +156,7 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable render( code: MagicString, options: RenderOptions, - { renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK + { renderedParentType, isCalleeOfRenderedParent, preventASI }: NodeRenderOptions = BLANK ) { if (!this.left.included || !this.right.included) { const operatorPos = findFirstOccurrenceOutsideComment( @@ -165,6 +166,9 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable ); if (this.right.included) { code.remove(this.start, operatorPos + 2); + if (preventASI) { + removeLineBreaks(code, operatorPos + 2, this.right.start); + } } else { code.remove(operatorPos, this.end); } diff --git a/src/ast/nodes/SequenceExpression.ts b/src/ast/nodes/SequenceExpression.ts index af8fcbc1d69..e4a21b486fb 100644 --- a/src/ast/nodes/SequenceExpression.ts +++ b/src/ast/nodes/SequenceExpression.ts @@ -3,6 +3,7 @@ import { BLANK } from '../../utils/blank'; import { getCommaSeparatedNodesWithBoundaries, NodeRenderOptions, + removeLineBreaks, RenderOptions } from '../../utils/renderHelpers'; import { treeshakeNode } from '../../utils/treeshakeNode'; @@ -81,10 +82,9 @@ export default class SequenceExpression extends NodeBase { render( code: MagicString, options: RenderOptions, - { renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK + { renderedParentType, isCalleeOfRenderedParent, preventASI }: NodeRenderOptions = BLANK ) { - let firstStart = 0, - includedNodes = 0; + let includedNodes = 0; for (const { node, start, end } of getCommaSeparatedNodesWithBoundaries( this.expressions, code, @@ -96,7 +96,9 @@ export default class SequenceExpression extends NodeBase { continue; } includedNodes++; - if (firstStart === 0) firstStart = start; + if (includedNodes === 1 && preventASI) { + removeLineBreaks(code, start, node.start); + } if (node === this.expressions[this.expressions.length - 1] && includedNodes === 1) { node.render(code, options, { isCalleeOfRenderedParent: renderedParentType diff --git a/src/ast/nodes/ThrowStatement.ts b/src/ast/nodes/ThrowStatement.ts index 1127c6100c5..bc63154df9f 100644 --- a/src/ast/nodes/ThrowStatement.ts +++ b/src/ast/nodes/ThrowStatement.ts @@ -1,3 +1,5 @@ +import MagicString from 'magic-string'; +import { RenderOptions } from '../../utils/renderHelpers'; import { ExecutionPathOptions } from '../ExecutionPathOptions'; import * as NodeType from './NodeType'; import { ExpressionNode, StatementBase } from './shared/Node'; @@ -9,4 +11,8 @@ export default class ThrowStatement extends StatementBase { hasEffects(_options: ExecutionPathOptions) { return true; } + + render(code: MagicString, options: RenderOptions) { + this.argument.render(code, options, { preventASI: true }); + } } diff --git a/src/utils/renderHelpers.ts b/src/utils/renderHelpers.ts index cf0dbba649f..5f86c7e3c26 100644 --- a/src/utils/renderHelpers.ts +++ b/src/utils/renderHelpers.ts @@ -165,3 +165,15 @@ export function getCommaSeparatedNodesWithBoundaries( }); return splitUpNodes; } + +export function removeLineBreaks(code: MagicString, start: number, end: number) { + let lineBreakPos = start; + while (true) { + lineBreakPos = findFirstLineBreakOutsideComment(code.original, lineBreakPos); + if (lineBreakPos === -1 || lineBreakPos >= end) { + break; + } + code.remove(lineBreakPos, lineBreakPos + 1); + lineBreakPos++; + } +} diff --git a/test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js b/test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js index 4e8a40b8b1b..760c721a884 100644 --- a/test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js +++ b/test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js @@ -5,3 +5,10 @@ console.log( console.log( /* keep me */ 'expected' ); + +console.log( + /* keep me */ + 'expected'); + +console.log((/* keep me */ + 'expected')); diff --git a/test/form/samples/keep-tree-shaking-comments-no-asi/main.js b/test/form/samples/keep-tree-shaking-comments-no-asi/main.js index 18a6a0f3bac..88758d123d7 100644 --- a/test/form/samples/keep-tree-shaking-comments-no-asi/main.js +++ b/test/form/samples/keep-tree-shaking-comments-no-asi/main.js @@ -7,3 +7,11 @@ console.log(true ? /* keep me */ 'expected' : 'unexpected'); + +console.log(true && + /* keep me */ + 'expected'); + +console.log((true, + /* keep me */ + 'expected')); diff --git a/test/function/samples/prevent-tree-shaking-asi/_config.js b/test/function/samples/prevent-tree-shaking-asi/_config.js index b3139391407..c0ce988a450 100644 --- a/test/function/samples/prevent-tree-shaking-asi/_config.js +++ b/test/function/samples/prevent-tree-shaking-asi/_config.js @@ -1,9 +1,6 @@ -const assert = require('assert'); - module.exports = { description: 'prevent automatic semicolon insertion from changing behaviour when tree-shaking', - exports(exports) { - assert.strictEqual(exports.test1(), 'expected'); - assert.strictEqual(exports.test2(), 'expected'); + options: { + treeshake: { tryCatchDeoptimization: false } } }; diff --git a/test/function/samples/prevent-tree-shaking-asi/main.js b/test/function/samples/prevent-tree-shaking-asi/main.js index eb69cf7aff0..8b91e02a84c 100644 --- a/test/function/samples/prevent-tree-shaking-asi/main.js +++ b/test/function/samples/prevent-tree-shaking-asi/main.js @@ -1,15 +1,42 @@ -export function test1() { +function test1() { return true ? - + /* kept */ 'expected' : 'unexpected'; } +assert.strictEqual(test1(), 'expected'); -export function test2() { +function test2() { return false ? 'unexpected' : + /* kept */ + + 'expected'; +} +assert.strictEqual(test2(), 'expected'); +function test3() { + return true && + /* kept */ 'expected'; } +assert.strictEqual(test3(), 'expected'); + +function test4() { + return 'removed', + /* kept */ + + 'expected'; +} +assert.strictEqual(test4(), 'expected'); + +try { + throw true ? + + new Error('expected') : + null; +} catch (err) { + assert.strictEqual(err.message, 'expected'); +}