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

Try to make logical expression deoptimization more robust #4519

Merged
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
38 changes: 3 additions & 35 deletions src/ast/nodes/CallExpression.ts
@@ -1,11 +1,8 @@
import type MagicString from 'magic-string';
import type { NormalizedTreeshakingOptions } from '../../rollup/types';
import { BLANK } from '../../utils/blank';
import {
findFirstOccurrenceOutsideComment,
type NodeRenderOptions,
type RenderOptions
} from '../../utils/renderHelpers';
import { renderCallArguments } from '../../utils/renderCallArguments';
import { type NodeRenderOptions, type RenderOptions } from '../../utils/renderHelpers';
import type { DeoptimizableEntity } from '../DeoptimizableEntity';
import type { HasEffectsContext, InclusionContext } from '../ExecutionContext';
import { EVENT_CALLED } from '../NodeEvents';
Expand Down Expand Up @@ -116,36 +113,7 @@ export default class CallExpression extends CallExpressionBase implements Deopti
isCalleeOfRenderedParent: true,
renderedSurroundingElement
});
if (this.arguments.length > 0) {
if (this.arguments[this.arguments.length - 1].included) {
for (const arg of this.arguments) {
arg.render(code, options);
}
} else {
let lastIncludedIndex = this.arguments.length - 2;
while (lastIncludedIndex >= 0 && !this.arguments[lastIncludedIndex].included) {
lastIncludedIndex--;
}
if (lastIncludedIndex >= 0) {
for (let index = 0; index <= lastIncludedIndex; index++) {
this.arguments[index].render(code, options);
}
code.remove(
findFirstOccurrenceOutsideComment(
code.original,
',',
this.arguments[lastIncludedIndex].end
),
this.end - 1
);
} else {
code.remove(
findFirstOccurrenceOutsideComment(code.original, '(', this.callee.end) + 1,
this.end - 1
);
}
}
}
renderCallArguments(code, options, this);
}

protected applyDeoptimizations(): void {
Expand Down
5 changes: 4 additions & 1 deletion src/ast/nodes/LogicalExpression.ts
Expand Up @@ -42,13 +42,16 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
private usedBranch: ExpressionNode | null = null;

deoptimizeCache(): void {
if (this.usedBranch !== null) {
if (this.usedBranch) {
const unusedBranch = this.usedBranch === this.left ? this.right : this.left;
this.usedBranch = null;
unusedBranch.deoptimizePath(UNKNOWN_PATH);
for (const expression of this.expressionsToBeDeoptimized) {
expression.deoptimizeCache();
}
// Request another pass because we need to ensure "include" runs again if
// it is rendered
this.context.requestTreeshakingPass();
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/ast/nodes/NewExpression.ts
@@ -1,4 +1,7 @@
import MagicString from 'magic-string';
import type { NormalizedTreeshakingOptions } from '../../rollup/types';
import { renderCallArguments } from '../../utils/renderCallArguments';
import { RenderOptions } from '../../utils/renderHelpers';
import type { CallOptions } from '../CallOptions';
import type { HasEffectsContext } from '../ExecutionContext';
import { InclusionContext } from '../ExecutionContext';
Expand Down Expand Up @@ -54,6 +57,11 @@ export default class NewExpression extends NodeBase {
};
}

render(code: MagicString, options: RenderOptions) {
this.callee.render(code, options);
renderCallArguments(code, options, this);
}

protected applyDeoptimizations(): void {
this.deoptimized = true;
for (const argument of this.arguments) {
Expand Down
41 changes: 41 additions & 0 deletions src/utils/renderCallArguments.ts
@@ -0,0 +1,41 @@
import MagicString from 'magic-string';
import CallExpression from '../ast/nodes/CallExpression';
import NewExpression from '../ast/nodes/NewExpression';
import { findFirstOccurrenceOutsideComment, RenderOptions } from './renderHelpers';

export function renderCallArguments(
code: MagicString,
options: RenderOptions,
node: CallExpression | NewExpression
): void {
if (node.arguments.length > 0) {
if (node.arguments[node.arguments.length - 1].included) {
for (const arg of node.arguments) {
arg.render(code, options);
}
} else {
let lastIncludedIndex = node.arguments.length - 2;
while (lastIncludedIndex >= 0 && !node.arguments[lastIncludedIndex].included) {
lastIncludedIndex--;
}
if (lastIncludedIndex >= 0) {
for (let index = 0; index <= lastIncludedIndex; index++) {
node.arguments[index].render(code, options);
}
code.remove(
findFirstOccurrenceOutsideComment(
code.original,
',',
node.arguments[lastIncludedIndex].end
),
node.end - 1
);
} else {
code.remove(
findFirstOccurrenceOutsideComment(code.original, '(', node.callee.end) + 1,
node.end - 1
);
}
}
}
}
2 changes: 1 addition & 1 deletion test/form/samples/side-effect-es5-classes/_expected.js
Expand Up @@ -22,7 +22,7 @@ var Arrow = ( x ) => {
console.log( 'before' );
new Bar(5);
new Baz(5);
new Qux(5);
new Qux();
Corge(5);
new Arrow(5);

Expand Down
@@ -0,0 +1,8 @@
const assert = require('assert');

module.exports = {
description: 'handles unused logical expressions as constructor arguments (#4517)',
exports({ create }) {
assert.strictEqual(create().foo, 'foo');
}
};
@@ -0,0 +1,7 @@
function Note() {
this.foo = 'foo';
}

export function create(data) {
return new Note(data || {});
}