From c1ff8b3fa80857ee50c6e345e658162955852a6b Mon Sep 17 00:00:00 2001
From: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
Date: Wed, 11 May 2022 07:02:36 +0200
Subject: [PATCH] Do not treat string.replace as side effect when used with a
 literal

---
 src/ast/values.ts                             | 61 ++++++++++++++-----
 .../string-replace-side-effects/_config.js    |  4 ++
 .../string-replace-side-effects/_expected.js  |  8 +++
 .../string-replace-side-effects/main.js       | 13 ++++
 4 files changed, 70 insertions(+), 16 deletions(-)
 create mode 100644 test/form/samples/string-replace-side-effects/_config.js
 create mode 100644 test/form/samples/string-replace-side-effects/_expected.js
 create mode 100644 test/form/samples/string-replace-side-effects/main.js

diff --git a/src/ast/values.ts b/src/ast/values.ts
index 019d79f4028..cf357eeb32b 100644
--- a/src/ast/values.ts
+++ b/src/ast/values.ts
@@ -1,11 +1,17 @@
 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;
 }
 
@@ -34,6 +40,7 @@ export const UNDEFINED_EXPRESSION: ExpressionEntity =
 const returnsUnknown: RawMemberDescription = {
 	value: {
 		callsArgs: null,
+		hasEffectsWhenCalled: null,
 		returns: UNKNOWN_EXPRESSION
 	}
 };
@@ -66,6 +73,7 @@ export const UNKNOWN_LITERAL_BOOLEAN: ExpressionEntity =
 const returnsBoolean: RawMemberDescription = {
 	value: {
 		callsArgs: null,
+		hasEffectsWhenCalled: null,
 		returns: UNKNOWN_LITERAL_BOOLEAN
 	}
 };
@@ -98,6 +106,7 @@ export const UNKNOWN_LITERAL_NUMBER: ExpressionEntity =
 const returnsNumber: RawMemberDescription = {
 	value: {
 		callsArgs: null,
+		hasEffectsWhenCalled: null,
 		returns: UNKNOWN_LITERAL_NUMBER
 	}
 };
@@ -130,6 +139,32 @@ export const UNKNOWN_LITERAL_STRING: ExpressionEntity =
 const returnsString: RawMemberDescription = {
 	value: {
 		callsArgs: null,
+		hasEffectsWhenCalled: null,
+		returns: UNKNOWN_LITERAL_STRING
+	}
+};
+
+const stringReplace: RawMemberDescription = {
+	value: {
+		callsArgs: null,
+		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 +224,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,8 +274,12 @@ export function hasMemberEffectWhenCalled(
 	if (typeof memberName !== 'string' || !members[memberName]) {
 		return true;
 	}
-	if (!members[memberName].callsArgs) return false;
-	for (const argIndex of members[memberName].callsArgs!) {
+	const { callsArgs, hasEffectsWhenCalled } = members[memberName];
+	if (hasEffectsWhenCalled) {
+		return hasEffectsWhenCalled(callOptions, context);
+	}
+	if (!callsArgs) return false;
+	for (const argIndex of callsArgs) {
 		if (
 			callOptions.args[argIndex] &&
 			callOptions.args[argIndex].hasEffectsWhenCalledAtPath(
diff --git a/test/form/samples/string-replace-side-effects/_config.js b/test/form/samples/string-replace-side-effects/_config.js
new file mode 100644
index 00000000000..291a777fcfa
--- /dev/null
+++ 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
new file mode 100644
index 00000000000..9a15559a7ab
--- /dev/null
+++ 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
new file mode 100644
index 00000000000..543d9be73c6
--- /dev/null
+++ 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 '_';
+});