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

Wrap simplified assignments if necessary #3951

Merged
merged 1 commit into from Feb 5, 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
13 changes: 10 additions & 3 deletions src/ast/nodes/AssignmentExpression.ts
Expand Up @@ -4,6 +4,7 @@ import {
findFirstOccurrenceOutsideComment,
findNonWhiteSpace,
NodeRenderOptions,
removeLineBreaks,
RenderOptions
} from '../../utils/renderHelpers';
import { getSystemExportFunctionLeft } from '../../utils/systemJsRendering';
Expand Down Expand Up @@ -67,17 +68,23 @@ export default class AssignmentExpression extends NodeBase {
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType }: NodeRenderOptions = BLANK
{ preventASI, renderedParentType }: NodeRenderOptions = BLANK
) {
if (this.left.included) {
this.left.render(code, options);
this.right.render(code, options);
} else {
const inclusionStart = findNonWhiteSpace(
code.original,
findFirstOccurrenceOutsideComment(code.original, '=', this.left.end) + 1
);
code.remove(this.start, inclusionStart);
if (preventASI) {
removeLineBreaks(code, inclusionStart, this.right.start);
}
this.right.render(code, options, {
renderedParentType: renderedParentType || this.parent.type
});
const operatorPos = findFirstOccurrenceOutsideComment(code.original, '=', this.left.end);
code.remove(this.start, findNonWhiteSpace(code.original, operatorPos + 1));
}
if (options.format === 'system') {
const exportNames =
Expand Down
16 changes: 7 additions & 9 deletions src/ast/nodes/CallExpression.ts
Expand Up @@ -226,9 +226,14 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType }: NodeRenderOptions = BLANK
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
) {
this.callee.render(code, options);
const surroundingELement = renderedParentType || renderedSurroundingElement;
this.callee.render(
code,
options,
surroundingELement ? { renderedSurroundingElement: surroundingELement } : BLANK
);
if (this.arguments.length > 0) {
if (this.arguments[this.arguments.length - 1].included) {
for (const arg of this.arguments) {
Expand Down Expand Up @@ -259,13 +264,6 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
}
}
}
if (
renderedParentType === NodeType.ExpressionStatement &&
this.callee.type === NodeType.FunctionExpression
) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
}
}

private getReturnExpression(recursionTracker: PathTracker): ExpressionEntity {
Expand Down
16 changes: 16 additions & 0 deletions src/ast/nodes/ClassExpression.ts
@@ -1,6 +1,22 @@
import MagicString from 'magic-string';
import { BLANK } from '../../utils/blank';
import { NodeRenderOptions, RenderOptions } from '../../utils/renderHelpers';
import * as NodeType from './NodeType';
import ClassNode from './shared/ClassNode';

export default class ClassExpression extends ClassNode {
type!: NodeType.tClassExpression;

render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
) {
super.render(code, options);
const surroundingElement = renderedParentType || renderedSurroundingElement;
if (surroundingElement === NodeType.ExpressionStatement) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
}
}
}
7 changes: 5 additions & 2 deletions src/ast/nodes/ConditionalExpression.ts
Expand Up @@ -2,6 +2,7 @@ import MagicString from 'magic-string';
import { BLANK } from '../../utils/blank';
import {
findFirstOccurrenceOutsideComment,
findNonWhiteSpace,
NodeRenderOptions,
removeLineBreaks,
RenderOptions
Expand Down Expand Up @@ -171,10 +172,12 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz
) {
if (!this.test.included) {
const colonPos = findFirstOccurrenceOutsideComment(code.original, ':', this.consequent.end);
const inclusionStart =
const inclusionStart = findNonWhiteSpace(
code.original,
(this.consequent.included
? findFirstOccurrenceOutsideComment(code.original, '?', this.test.end)
: colonPos) + 1;
: colonPos) + 1
);
if (preventASI) {
removeLineBreaks(code, inclusionStart, this.usedBranch!.start);
}
Expand Down
16 changes: 16 additions & 0 deletions src/ast/nodes/FunctionExpression.ts
@@ -1,6 +1,22 @@
import MagicString from 'magic-string';
import { BLANK } from '../../utils/blank';
import { NodeRenderOptions, RenderOptions } from '../../utils/renderHelpers';
import * as NodeType from './NodeType';
import FunctionNode from './shared/FunctionNode';

export default class FunctionExpression extends FunctionNode {
type!: NodeType.tFunctionExpression;

render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
) {
super.render(code, options);
const surroundingElement = renderedParentType || renderedSurroundingElement;
if (surroundingElement === NodeType.ExpressionStatement) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
}
}
}
11 changes: 9 additions & 2 deletions src/ast/nodes/LabeledStatement.ts
@@ -1,5 +1,9 @@
import MagicString from 'magic-string';
import { findFirstOccurrenceOutsideComment, RenderOptions } from '../../utils/renderHelpers';
import {
findFirstOccurrenceOutsideComment,
findNonWhiteSpace,
RenderOptions
} from '../../utils/renderHelpers';
import { HasEffectsContext, InclusionContext } from '../ExecutionContext';
import Identifier from './Identifier';
import * as NodeType from './NodeType';
Expand Down Expand Up @@ -39,7 +43,10 @@ export default class LabeledStatement extends StatementBase {
} else {
code.remove(
this.start,
findFirstOccurrenceOutsideComment(code.original, ':', this.label.end) + 1
findNonWhiteSpace(
code.original,
findFirstOccurrenceOutsideComment(code.original, ':', this.label.end) + 1
)
);
}
this.body.render(code, options);
Expand Down
6 changes: 4 additions & 2 deletions src/ast/nodes/LogicalExpression.ts
Expand Up @@ -2,6 +2,7 @@ import MagicString from 'magic-string';
import { BLANK } from '../../utils/blank';
import {
findFirstOccurrenceOutsideComment,
findNonWhiteSpace,
NodeRenderOptions,
removeLineBreaks,
RenderOptions
Expand Down Expand Up @@ -168,9 +169,10 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
this.left.end
);
if (this.right.included) {
code.remove(this.start, operatorPos + 2);
const removePos = findNonWhiteSpace(code.original, operatorPos + 2);
code.remove(this.start, removePos);
if (preventASI) {
removeLineBreaks(code, operatorPos + 2, this.right.start);
removeLineBreaks(code, removePos, this.right.start);
}
} else {
code.remove(operatorPos, this.end);
Expand Down
14 changes: 12 additions & 2 deletions src/ast/nodes/MemberExpression.ts
Expand Up @@ -235,7 +235,11 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK
{
renderedParentType,
isCalleeOfRenderedParent,
renderedSurroundingElement
}: NodeRenderOptions = BLANK
) {
const isCalleeOfDifferentParent =
renderedParentType === NodeType.CallExpression && isCalleeOfRenderedParent;
Expand All @@ -250,7 +254,13 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
if (isCalleeOfDifferentParent) {
code.appendRight(this.start, '0, ');
}
super.render(code, options);
const surroundingElement = renderedParentType || renderedSurroundingElement;
this.object.render(
code,
options,
surroundingElement ? { renderedSurroundingElement: surroundingElement } : BLANK
);
this.property.render(code, options);
}
}

Expand Down
7 changes: 4 additions & 3 deletions src/ast/nodes/ObjectExpression.ts
Expand Up @@ -259,12 +259,13 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
render(
code: MagicString,
options: RenderOptions,
{ renderedParentType }: NodeRenderOptions = BLANK
{ renderedParentType, renderedSurroundingElement }: NodeRenderOptions = BLANK
) {
super.render(code, options);
const surroundingElement = renderedParentType || renderedSurroundingElement;
if (
renderedParentType === NodeType.ExpressionStatement ||
renderedParentType === NodeType.ArrowFunctionExpression
surroundingElement === NodeType.ExpressionStatement ||
surroundingElement === NodeType.ArrowFunctionExpression
) {
code.appendRight(this.start, '(');
code.prependLeft(this.end, ')');
Expand Down
1 change: 1 addition & 0 deletions src/utils/renderHelpers.ts
Expand Up @@ -24,6 +24,7 @@ export interface NodeRenderOptions {
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 preceed elements
start?: number;
}

Expand Down
Expand Up @@ -3,7 +3,7 @@ define(['exports'], function (exports) { 'use strict';
var hsl2hsv = (h, s, l) => {
const t = s * (l < 0.5 ? 1 : 1 - l),
V = 1 + t,
S = 2 * t / V ;
S = 2 * t / V ;
return [h, S, V];
};

Expand Down
Expand Up @@ -5,7 +5,7 @@ Object.defineProperty(exports, '__esModule', { value: true });
var hsl2hsv = (h, s, l) => {
const t = s * (l < 0.5 ? 1 : 1 - l),
V = 1 + t,
S = 2 * t / V ;
S = 2 * t / V ;
return [h, S, V];
};

Expand Down
@@ -1,7 +1,7 @@
var hsl2hsv = (h, s, l) => {
const t = s * (l < 0.5 ? 1 : 1 - l),
V = 1 + t,
S = 2 * t / V ;
S = 2 * t / V ;
return [h, S, V];
};

Expand Down
Expand Up @@ -6,7 +6,7 @@ System.register([], function (exports) {
var hsl2hsv = exports('default', (h, s, l) => {
const t = s * (l < 0.5 ? 1 : 1 - l),
V = 1 + t,
S = 2 * t / V ;
S = 2 * t / V ;
return [h, S, V];
});

Expand Down
@@ -1,6 +1,6 @@
define(function () { 'use strict';

var main = null;
var main = null;

return main;

Expand Down
@@ -1,5 +1,5 @@
'use strict';

var main = null;
var main = null;

module.exports = main;
@@ -1,3 +1,3 @@
var main = null;
var main = null;

export default main;
Expand Up @@ -3,7 +3,7 @@ System.register([], function (exports) {
return {
execute: function () {

var main = exports('default', null);
var main = exports('default', null);

}
};
Expand Down
@@ -1,6 +1,6 @@
while (globalThis.unknown) {
console.log('retained');
{
{
break;
}
}
Expand Down
Expand Up @@ -32,15 +32,15 @@ outer: {
}

function withConsequentReturn() {
{
{
inner: {
if (globalThis.unknown) return;
else break inner;
}
console.log('retained');
}
{
{
{
{
return;
}
}
Expand All @@ -49,7 +49,7 @@ function withConsequentReturn() {
withConsequentReturn();

function withAlternateReturn() {
{
{
inner: {
if (globalThis.unknown) break inner;
else return;
Expand Down
@@ -1,11 +1,11 @@
const object = {};
object.propertyIsEnumerable( 'toString' );
{}.propertyIsEnumerable( 'toString' );
{}.propertyIsEnumerable( 'toString' ).valueOf();
({}).propertyIsEnumerable( 'toString' );
({}).propertyIsEnumerable( 'toString' ).valueOf();
Comment on lines -3 to +4
Copy link
Contributor

Choose a reason for hiding this comment

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

@lukastaegert If all test output is reparsed with acorn in the test framework then issues like this one could be caught earlier in the future. Please ignore if this PR already implemented this suggestion, but I did not see it.

For example, using the previous version of this file:

$ rollup test/form/samples/builtin-prototypes/object-expression/_expected.js --silent
[!] Error: Unexpected token
test/form/samples/builtin-prototypes/object-expression/_expected.js (3:2)
1: const object = {};
2: object.propertyIsEnumerable( 'toString' );
3: {}.propertyIsEnumerable( 'toString' );
     ^
4: {}.propertyIsEnumerable( 'toString' ).valueOf();
Error: Unexpected token

Copy link
Contributor

@kzc kzc Feb 5, 2021

Choose a reason for hiding this comment

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

With 574050b:

$ rollup --silent < test/form/samples/system-export-compact/_expected.js
[!] Error: Unexpected token (Note that you need plugins to import files that are not JavaScript)
- (3:8)
1: System.register([],function(exports){'use strict';return{execute:function(){exports({a:void 0,b:void 0,c:void 0});let a, b;
2: 
3: function(v){return exports({a:a}),v}([{ b: a = (exports('a',a-1),a--) }] = { b: function(v){return exports({b:v,c:v}),v}(--b) });}}});
           ^
$ rollup --silent < test/form/samples/system-multiple-export-bindings/_expected.js 
[!] Error: Unexpected token (Note that you need plugins to import files that are not JavaScript)
- (47:12)
45: 
46:       // Update Expression
47:       function (v) { return exports({ a: v, a2: v }), v; }(++a), (exports('b', b + 1), b++), (++c, exports({ c: c, c2: c }), c);
                   ^
$ npx terser < test/form/samples/no-external-live-bindings-compact/_expected/system.js
Parse error at 0:1,265
odule.external1);},function(module){var _setter={};for(var _$pinmodule){if(!_sta
                                                                      ^
ERROR: Unexpected token punc «)», expected punc «;»


{}.hasOwnProperty( 'toString' ).valueOf();
{}.isPrototypeOf( {} ).valueOf();
{}.propertyIsEnumerable( 'toString' ).valueOf();
{}.toLocaleString().trim();
{}.toString().trim();
{}.valueOf();
({}).hasOwnProperty( 'toString' ).valueOf();
({}).isPrototypeOf( {} ).valueOf();
({}).propertyIsEnumerable( 'toString' ).valueOf();
({}).toLocaleString().trim();
({}).toString().trim();
({}).valueOf();
20 changes: 10 additions & 10 deletions test/form/samples/conditional-expression-paths/_expected.js
Expand Up @@ -12,16 +12,16 @@ var baz = { x: () => console.log('effect') };
(unknownValue ? foo : baz).x();

// known branch without side-effects
( foo ).y.z;
( foo).y.z;
( foo ).x();
( foo).x();
(foo ).y.z;
(foo).y.z;
(foo ).x();
(foo).x();

// known branch with side-effect
( baz ).y.z;
( baz).y.z;
( baz ).x();
( baz).x();
(baz ).y.z;
(baz).y.z;
(baz ).x();
(baz).x();
var baz3 = {};
( baz3 ).y.z = 1;
( baz3).y.z = 1;
(baz3 ).y.z = 1;
(baz3).y.z = 1;
12 changes: 6 additions & 6 deletions test/form/samples/conditional-expression/_expected.js
Expand Up @@ -9,9 +9,9 @@ unknownValue ? 1 : foo();
(unknownValue ? function () {} : function () {this.x = 1;})();

// known side-effect
foo() ;
( function () {this.x = 1;} )();
( () => () => console.log( 'effect' ) )()();
foo();
( function () {this.x = 1;})();
( () => () => console.log( 'effect' ))()();
foo() ;
(function () {this.x = 1;} )();
(() => () => console.log( 'effect' ) )()();
foo();
(function () {this.x = 1;})();
(() => () => console.log( 'effect' ))()();