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
rollup · May 13, 2022 · 4079fbe · 4079fbe
1 parent bbd54b7
commit 4079fbe
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 36 deletions.
diff --git a/src/ast/values.ts b/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;
 }
 
@@ -33,7 +38,7 @@ export const UNDEFINED_EXPRESSION: ExpressionEntity =
 
 const returnsUnknown: RawMemberDescription = {
 	value: {
-		callsArgs: null,
+		hasEffectsWhenCalled: null,
 		returns: UNKNOWN_EXPRESSION
 	}
 };
@@ -65,7 +70,7 @@ export const UNKNOWN_LITERAL_BOOLEAN: ExpressionEntity =
 
 const returnsBoolean: RawMemberDescription = {
 	value: {
-		callsArgs: null,
+		hasEffectsWhenCalled: null,
 		returns: UNKNOWN_LITERAL_BOOLEAN
 	}
 };
@@ -97,7 +102,7 @@ export const UNKNOWN_LITERAL_NUMBER: ExpressionEntity =
 
 const returnsNumber: RawMemberDescription = {
 	value: {
-		callsArgs: null,
+		hasEffectsWhenCalled: null,
 		returns: UNKNOWN_LITERAL_NUMBER
 	}
 };
@@ -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
 	}
 };
@@ -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,
@@ -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(

diff --git a/test/form/samples/string-replace-side-effects/_config.js b/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'
+};
diff --git a/test/form/samples/string-replace-side-effects/_expected.js b/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 '_';
+});
diff --git a/test/form/samples/string-replace-side-effects/main.js b/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 '_';
+});