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

Do not treat string.replace as side effect when used with a literal #4494

Merged
merged 12 commits into from May 13, 2022
Merged
75 changes: 39 additions & 36 deletions src/ast/values.ts
@@ -1,11 +1,16 @@
import { type CallOptions, NO_ARGS } from './CallOptions';
import type { HasEffectsContext } from './ExecutionContext';
import type { LiteralValue } from './nodes/Literal';
import { ExpressionEntity, UNKNOWN_EXPRESSION } from './nodes/shared/Expression';
import { EMPTY_PATH, type ObjectPath, type ObjectPathKey } from './utils/PathTracker';
import { ExpressionEntity, UNKNOWN_EXPRESSION, UnknownValue } from './nodes/shared/Expression';
import {
EMPTY_PATH,
type ObjectPath,
type ObjectPathKey,
SHARED_RECURSION_TRACKER
} from './utils/PathTracker';

export interface MemberDescription {
callsArgs: number[] | null;
hasEffectsWhenCalled: ((callOptions: CallOptions, context: HasEffectsContext) => boolean) | null;
returns: ExpressionEntity;
}

Expand Down Expand Up @@ -33,7 +38,7 @@ export const UNDEFINED_EXPRESSION: ExpressionEntity =

const returnsUnknown: RawMemberDescription = {
value: {
callsArgs: null,
hasEffectsWhenCalled: null,
returns: UNKNOWN_EXPRESSION
}
};
Expand Down Expand Up @@ -65,7 +70,7 @@ export const UNKNOWN_LITERAL_BOOLEAN: ExpressionEntity =

const returnsBoolean: RawMemberDescription = {
value: {
callsArgs: null,
hasEffectsWhenCalled: null,
returns: UNKNOWN_LITERAL_BOOLEAN
}
};
Expand Down Expand Up @@ -97,7 +102,7 @@ export const UNKNOWN_LITERAL_NUMBER: ExpressionEntity =

const returnsNumber: RawMemberDescription = {
value: {
callsArgs: null,
hasEffectsWhenCalled: null,
returns: UNKNOWN_LITERAL_NUMBER
}
};
Expand Down Expand Up @@ -129,7 +134,31 @@ export const UNKNOWN_LITERAL_STRING: ExpressionEntity =

const returnsString: RawMemberDescription = {
value: {
callsArgs: null,
hasEffectsWhenCalled: null,
returns: UNKNOWN_LITERAL_STRING
}
};

const stringReplace: RawMemberDescription = {
value: {
hasEffectsWhenCalled(callOptions, context) {
const arg1 = callOptions.args[1];
return (
callOptions.args.length < 2 ||
(arg1.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, {
deoptimizeCache() {}
}) === UnknownValue &&
arg1.hasEffectsWhenCalledAtPath(
EMPTY_PATH,
{
args: NO_ARGS,
thisParam: null,
withNew: false
},
context
))
);
},
returns: UNKNOWN_LITERAL_STRING
}
};
Expand Down Expand Up @@ -189,18 +218,8 @@ const literalStringMembers: MemberDescriptions = assembleMemberDescriptions(
padEnd: returnsString,
padStart: returnsString,
repeat: returnsString,
replace: {
value: {
callsArgs: [1],
returns: UNKNOWN_LITERAL_STRING
}
},
replaceAll: {
value: {
callsArgs: [1],
returns: UNKNOWN_LITERAL_STRING
}
},
replace: stringReplace,
replaceAll: stringReplace,
search: returnsNumber,
slice: returnsString,
small: returnsString,
Expand Down Expand Up @@ -249,23 +268,7 @@ export function hasMemberEffectWhenCalled(
if (typeof memberName !== 'string' || !members[memberName]) {
return true;
}
if (!members[memberName].callsArgs) return false;
for (const argIndex of members[memberName].callsArgs!) {
if (
callOptions.args[argIndex] &&
callOptions.args[argIndex].hasEffectsWhenCalledAtPath(
EMPTY_PATH,
{
args: NO_ARGS,
thisParam: null,
withNew: false
},
context
)
)
return true;
}
return false;
return members[memberName].hasEffectsWhenCalled?.(callOptions, context) || false;
}

export function getMemberReturnExpressionWhenCalled(
Expand Down
4 changes: 4 additions & 0 deletions test/form/samples/string-replace-side-effects/_config.js
@@ -0,0 +1,4 @@
module.exports = {
description:
'does not assume string.replace has side effects when called with a string as second argument'
};
8 changes: 8 additions & 0 deletions test/form/samples/string-replace-side-effects/_expected.js
@@ -0,0 +1,8 @@
'foo'.replace('o', () => {
console.log('retained');
return '_';
});
'foo'.replaceAll('o', () => {
console.log('retained');
return '_';
});
13 changes: 13 additions & 0 deletions test/form/samples/string-replace-side-effects/main.js
@@ -0,0 +1,13 @@
'foo'.replace('o', 'removed');
'foo'.replace('o', () => 'removed');
'foo'.replace('o', () => {
console.log('retained');
return '_';
});

'foo'.replaceAll('o', 'removed');
'foo'.replaceAll('o', () => 'removed');
'foo'.replaceAll('o', () => {
console.log('retained');
return '_';
});