Skip to content

Commit 4079fbe

Browse files
lukastaegertlukastaegert
authored
Do not treat string.replace as side effect when used with a literal (#4494)
* Do not make Object.defineProperty/ies a side effect by default * Detect side effects for getters on functions * Less deoptimization for non-accessor assignments * Use ObjectEntity for arrow function properties * Share code between functions and arrow functions * Use new path key for defineProperty side effects * Enable custom call-effect detection per global * Improve coverage * Do not treat string.replace as side effect when used with a literal * Remove unneeded prop
1 parent bbd54b7 commit 4079fbe

4 files changed

Lines changed: 64 additions & 36 deletions

File tree

‎src/ast/values.ts‎

Lines changed: 39 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,16 @@
11
import { type CallOptions, NO_ARGS } from './CallOptions';
22
import type { HasEffectsContext } from './ExecutionContext';
33
import type { LiteralValue } from './nodes/Literal';
4-
import { ExpressionEntity, UNKNOWN_EXPRESSION } from './nodes/shared/Expression';
5-
import { EMPTY_PATH, type ObjectPath, type ObjectPathKey } from './utils/PathTracker';
4+
import { ExpressionEntity, UNKNOWN_EXPRESSION, UnknownValue } from './nodes/shared/Expression';
5+
import {
6+
EMPTY_PATH,
7+
type ObjectPath,
8+
type ObjectPathKey,
9+
SHARED_RECURSION_TRACKER
10+
} from './utils/PathTracker';
611

712
export interface MemberDescription {
8-
callsArgs: number[] | null;
13+
hasEffectsWhenCalled: ((callOptions: CallOptions, context: HasEffectsContext) => boolean) | null;
914
returns: ExpressionEntity;
1015
}
1116

@@ -33,7 +38,7 @@ export const UNDEFINED_EXPRESSION: ExpressionEntity =
3338

3439
const returnsUnknown: RawMemberDescription = {
3540
value: {
36-
callsArgs: null,
41+
hasEffectsWhenCalled: null,
3742
returns: UNKNOWN_EXPRESSION
3843
}
3944
};
@@ -65,7 +70,7 @@ export const UNKNOWN_LITERAL_BOOLEAN: ExpressionEntity =
6570

6671
const returnsBoolean: RawMemberDescription = {
6772
value: {
68-
callsArgs: null,
73+
hasEffectsWhenCalled: null,
6974
returns: UNKNOWN_LITERAL_BOOLEAN
7075
}
7176
};
@@ -97,7 +102,7 @@ export const UNKNOWN_LITERAL_NUMBER: ExpressionEntity =
97102

98103
const returnsNumber: RawMemberDescription = {
99104
value: {
100-
callsArgs: null,
105+
hasEffectsWhenCalled: null,
101106
returns: UNKNOWN_LITERAL_NUMBER
102107
}
103108
};
@@ -129,7 +134,31 @@ export const UNKNOWN_LITERAL_STRING: ExpressionEntity =
129134

130135
const returnsString: RawMemberDescription = {
131136
value: {
132-
callsArgs: null,
137+
hasEffectsWhenCalled: null,
138+
returns: UNKNOWN_LITERAL_STRING
139+
}
140+
};
141+
142+
const stringReplace: RawMemberDescription = {
143+
value: {
144+
hasEffectsWhenCalled(callOptions, context) {
145+
const arg1 = callOptions.args[1];
146+
return (
147+
callOptions.args.length < 2 ||
148+
(arg1.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, {
149+
deoptimizeCache() {}
150+
}) === UnknownValue &&
151+
arg1.hasEffectsWhenCalledAtPath(
152+
EMPTY_PATH,
153+
{
154+
args: NO_ARGS,
155+
thisParam: null,
156+
withNew: false
157+
},
158+
context
159+
))
160+
);
161+
},
133162
returns: UNKNOWN_LITERAL_STRING
134163
}
135164
};
@@ -189,18 +218,8 @@ const literalStringMembers: MemberDescriptions = assembleMemberDescriptions(
189218
padEnd: returnsString,
190219
padStart: returnsString,
191220
repeat: returnsString,
192-
replace: {
193-
value: {
194-
callsArgs: [1],
195-
returns: UNKNOWN_LITERAL_STRING
196-
}
197-
},
198-
replaceAll: {
199-
value: {
200-
callsArgs: [1],
201-
returns: UNKNOWN_LITERAL_STRING
202-
}
203-
},
221+
replace: stringReplace,
222+
replaceAll: stringReplace,
204223
search: returnsNumber,
205224
slice: returnsString,
206225
small: returnsString,
@@ -249,23 +268,7 @@ export function hasMemberEffectWhenCalled(
249268
if (typeof memberName !== 'string' || !members[memberName]) {
250269
return true;
251270
}
252-
if (!members[memberName].callsArgs) return false;
253-
for (const argIndex of members[memberName].callsArgs!) {
254-
if (
255-
callOptions.args[argIndex] &&
256-
callOptions.args[argIndex].hasEffectsWhenCalledAtPath(
257-
EMPTY_PATH,
258-
{
259-
args: NO_ARGS,
260-
thisParam: null,
261-
withNew: false
262-
},
263-
context
264-
)
265-
)
266-
return true;
267-
}
268-
return false;
271+
return members[memberName].hasEffectsWhenCalled?.(callOptions, context) || false;
269272
}
270273

271274
export function getMemberReturnExpressionWhenCalled(

‎test/form/samples/string-replace-side-effects/_config.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
module.exports = {
2+
description:
3+
'does not assume string.replace has side effects when called with a string as second argument'
4+
};

‎test/form/samples/string-replace-side-effects/_expected.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
'foo'.replace('o', () => {
2+
console.log('retained');
3+
return '_';
4+
});
5+
'foo'.replaceAll('o', () => {
6+
console.log('retained');
7+
return '_';
8+
});

‎test/form/samples/string-replace-side-effects/main.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
'foo'.replace('o', 'removed');
2+
'foo'.replace('o', () => 'removed');
3+
'foo'.replace('o', () => {
4+
console.log('retained');
5+
return '_';
6+
});
7+
8+
'foo'.replaceAll('o', 'removed');
9+
'foo'.replaceAll('o', () => 'removed');
10+
'foo'.replaceAll('o', () => {
11+
console.log('retained');
12+
return '_';
13+
});

0 commit comments

Comments
 (0)