Skip to content

Commit 696ebd3

Browse files
lukastaegertlukastaegert
authored
Try to make logical expression deoptimization more robust (#4519)
* Try to make logical expression deoptimization more robust * Share argument rendering logic between call and new expression Solves an issue where trying to render a non-included argument crashes
1 parent 0409bf0 commit 696ebd3

7 files changed

Lines changed: 72 additions & 37 deletions

File tree

‎src/ast/nodes/CallExpression.ts‎

Lines changed: 3 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
11
import type MagicString from 'magic-string';
22
import type { NormalizedTreeshakingOptions } from '../../rollup/types';
33
import { BLANK } from '../../utils/blank';
4-
import {
5-
findFirstOccurrenceOutsideComment,
6-
type NodeRenderOptions,
7-
type RenderOptions
8-
} from '../../utils/renderHelpers';
4+
import { renderCallArguments } from '../../utils/renderCallArguments';
5+
import { type NodeRenderOptions, type RenderOptions } from '../../utils/renderHelpers';
96
import type { DeoptimizableEntity } from '../DeoptimizableEntity';
107
import type { HasEffectsContext, InclusionContext } from '../ExecutionContext';
118
import { EVENT_CALLED } from '../NodeEvents';
@@ -116,36 +113,7 @@ export default class CallExpression extends CallExpressionBase implements Deopti
116113
isCalleeOfRenderedParent: true,
117114
renderedSurroundingElement
118115
});
119-
if (this.arguments.length > 0) {
120-
if (this.arguments[this.arguments.length - 1].included) {
121-
for (const arg of this.arguments) {
122-
arg.render(code, options);
123-
}
124-
} else {
125-
let lastIncludedIndex = this.arguments.length - 2;
126-
while (lastIncludedIndex >= 0 && !this.arguments[lastIncludedIndex].included) {
127-
lastIncludedIndex--;
128-
}
129-
if (lastIncludedIndex >= 0) {
130-
for (let index = 0; index <= lastIncludedIndex; index++) {
131-
this.arguments[index].render(code, options);
132-
}
133-
code.remove(
134-
findFirstOccurrenceOutsideComment(
135-
code.original,
136-
',',
137-
this.arguments[lastIncludedIndex].end
138-
),
139-
this.end - 1
140-
);
141-
} else {
142-
code.remove(
143-
findFirstOccurrenceOutsideComment(code.original, '(', this.callee.end) + 1,
144-
this.end - 1
145-
);
146-
}
147-
}
148-
}
116+
renderCallArguments(code, options, this);
149117
}
150118

151119
protected applyDeoptimizations(): void {

‎src/ast/nodes/LogicalExpression.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,16 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
4242
private usedBranch: ExpressionNode | null = null;
4343

4444
deoptimizeCache(): void {
45-
if (this.usedBranch !== null) {
45+
if (this.usedBranch) {
4646
const unusedBranch = this.usedBranch === this.left ? this.right : this.left;
4747
this.usedBranch = null;
4848
unusedBranch.deoptimizePath(UNKNOWN_PATH);
4949
for (const expression of this.expressionsToBeDeoptimized) {
5050
expression.deoptimizeCache();
5151
}
52+
// Request another pass because we need to ensure "include" runs again if
53+
// it is rendered
54+
this.context.requestTreeshakingPass();
5255
}
5356
}
5457

‎src/ast/nodes/NewExpression.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1+
import MagicString from 'magic-string';
12
import type { NormalizedTreeshakingOptions } from '../../rollup/types';
3+
import { renderCallArguments } from '../../utils/renderCallArguments';
4+
import { RenderOptions } from '../../utils/renderHelpers';
25
import type { CallOptions } from '../CallOptions';
36
import type { HasEffectsContext } from '../ExecutionContext';
47
import { InclusionContext } from '../ExecutionContext';
@@ -54,6 +57,11 @@ export default class NewExpression extends NodeBase {
5457
};
5558
}
5659

60+
render(code: MagicString, options: RenderOptions) {
61+
this.callee.render(code, options);
62+
renderCallArguments(code, options, this);
63+
}
64+
5765
protected applyDeoptimizations(): void {
5866
this.deoptimized = true;
5967
for (const argument of this.arguments) {

‎src/utils/renderCallArguments.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import MagicString from 'magic-string';
2+
import CallExpression from '../ast/nodes/CallExpression';
3+
import NewExpression from '../ast/nodes/NewExpression';
4+
import { findFirstOccurrenceOutsideComment, RenderOptions } from './renderHelpers';
5+
6+
export function renderCallArguments(
7+
code: MagicString,
8+
options: RenderOptions,
9+
node: CallExpression | NewExpression
10+
): void {
11+
if (node.arguments.length > 0) {
12+
if (node.arguments[node.arguments.length - 1].included) {
13+
for (const arg of node.arguments) {
14+
arg.render(code, options);
15+
}
16+
} else {
17+
let lastIncludedIndex = node.arguments.length - 2;
18+
while (lastIncludedIndex >= 0 && !node.arguments[lastIncludedIndex].included) {
19+
lastIncludedIndex--;
20+
}
21+
if (lastIncludedIndex >= 0) {
22+
for (let index = 0; index <= lastIncludedIndex; index++) {
23+
node.arguments[index].render(code, options);
24+
}
25+
code.remove(
26+
findFirstOccurrenceOutsideComment(
27+
code.original,
28+
',',
29+
node.arguments[lastIncludedIndex].end
30+
),
31+
node.end - 1
32+
);
33+
} else {
34+
code.remove(
35+
findFirstOccurrenceOutsideComment(code.original, '(', node.callee.end) + 1,
36+
node.end - 1
37+
);
38+
}
39+
}
40+
}
41+
}

‎test/form/samples/side-effect-es5-classes/_expected.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ var Arrow = ( x ) => {
2222
console.log( 'before' );
2323
new Bar(5);
2424
new Baz(5);
25-
new Qux(5);
25+
new Qux();
2626
Corge(5);
2727
new Arrow(5);
2828

‎test/function/samples/unused-constructor-argument-fallback/_config.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
const assert = require('assert');
2+
3+
module.exports = {
4+
description: 'handles unused logical expressions as constructor arguments (#4517)',
5+
exports({ create }) {
6+
assert.strictEqual(create().foo, 'foo');
7+
}
8+
};

‎test/function/samples/unused-constructor-argument-fallback/main.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
function Note() {
2+
this.foo = 'foo';
3+
}
4+
5+
export function create(data) {
6+
return new Note(data || {});
7+
}

0 commit comments

Comments
 (0)