Treat unknown prototype props as unknown (#4428)

lukastaegert · web-flow · commit 84c0ea3790a3 · 2022-03-06T07:35:46.000+01:00
* Treat unknown prototype methods as unknown

* Improve coverage
diff --git a/src/ast/nodes/shared/ObjectPrototype.ts b/src/ast/nodes/shared/ObjectPrototype.ts
@@ -1,10 +1,47 @@
+import { EVENT_CALLED, NodeEvent } from '../../NodeEvents';
+import { ObjectPath, ObjectPathKey, UNKNOWN_PATH } from '../../utils/PathTracker';
+import { ExpressionEntity, LiteralValueOrUnknown, UnknownValue } from './Expression';
 import {
 	METHOD_RETURNS_BOOLEAN,
 	METHOD_RETURNS_STRING,
 	METHOD_RETURNS_UNKNOWN
 } from './MethodTypes';
 import { ObjectEntity, type PropertyMap } from './ObjectEntity';
 
+const isInteger = (prop: ObjectPathKey): boolean => typeof prop === 'string' && /^\d+$/.test(prop);
+
+// This makes sure unknown properties are not handled as "undefined" but as
+// "unknown" but without access side effects. An exception is done for numeric
+// properties as we do not expect new builtin properties to be numbers, this
+// will improve tree-shaking for out-of-bounds array properties
+const OBJECT_PROTOTYPE_FALLBACK: ExpressionEntity =
+	new (class ObjectPrototypeFallbackExpression extends ExpressionEntity {
+		deoptimizeThisOnEventAtPath(
+			event: NodeEvent,
+			path: ObjectPath,
+			thisParameter: ExpressionEntity
+		): void {
+			if (event === EVENT_CALLED && path.length === 1 && !isInteger(path[0])) {
+				thisParameter.deoptimizePath(UNKNOWN_PATH);
+			}
+		}
+
+		getLiteralValueAtPath(path: ObjectPath): LiteralValueOrUnknown {
+			// We ignore number properties as we do not expect new properties to be
+			// numbers and also want to keep handling out-of-bound array elements as
+			// "undefined"
+			return path.length === 1 && isInteger(path[0]) ? undefined : UnknownValue;
+		}
+
+		hasEffectsWhenAccessedAtPath(path: ObjectPath): boolean {
+			return path.length > 1;
+		}
+
+		hasEffectsWhenAssignedAtPath(path: ObjectPath): boolean {
+			return path.length > 1;
+		}
+	})();
+
 export const OBJECT_PROTOTYPE = new ObjectEntity(
 	{
 		__proto__: null,
@@ -15,6 +52,6 @@ export const OBJECT_PROTOTYPE = new ObjectEntity(
 		toString: METHOD_RETURNS_STRING,
 		valueOf: METHOD_RETURNS_UNKNOWN
 	} as unknown as PropertyMap,
-	null,
+	OBJECT_PROTOTYPE_FALLBACK,
 	true
 );
diff --git a/test/cli/samples/watch/watch-config-initial-error/_config.js b/test/cli/samples/watch/watch-config-initial-error/_config.js
@@ -12,7 +12,8 @@ module.exports = {
 		writeFileSync(configFile, 'throw new Error("Config contains initial errors");');
 	},
 	after() {
-		unlinkSync(configFile);
+		// synchronous sometimes does not seem to work, probably because the watch is not yet removed properly
+		setTimeout(() => unlinkSync(configFile), 300);
 	},
 	async abortOnStderr(data) {
 		if (data.includes('Config contains initial errors')) {
diff --git a/test/form/samples/avoid-unnecessary-conditional-deopt/_expected.js b/test/form/samples/avoid-unnecessary-conditional-deopt/_expected.js
@@ -2,7 +2,7 @@ console.log('not modified');
 
 let x2 = false;
 modifyX2();
-const obj2 = {};
+const obj2 = { modified: false };
 (x2 ? obj2 : {}).modified = true;
 
 if (obj2.modified) console.log('modified');
@@ -16,7 +16,7 @@ console.log('not modified');
 
 let x4 = false;
 modifyX4();
-const obj4 = {};
+const obj4 = { modified: false };
 (x4 && obj4).modified = true;
 
 if (obj4.modified) console.log('modified');
diff --git a/test/form/samples/avoid-unnecessary-conditional-deopt/main.js b/test/form/samples/avoid-unnecessary-conditional-deopt/main.js
@@ -1,6 +1,6 @@
 let x1 = false;
 modifyX1();
-const obj1 = {};
+const obj1 = { modified: false };
 x1 ? obj1 : {};
 
 if (obj1.modified) console.log('should not happen');
@@ -12,7 +12,7 @@ function modifyX1() {
 
 let x2 = false;
 modifyX2();
-const obj2 = {};
+const obj2 = { modified: false };
 (x2 ? obj2 : {}).modified = true;
 
 if (obj2.modified) console.log('modified');
@@ -24,7 +24,7 @@ function modifyX2() {
 
 let x3 = false;
 modifyX3();
-const obj3 = {};
+const obj3 = { modified: false };
 x3 && obj3;
 
 if (obj3.modified) console.log('should not happen');
@@ -36,7 +36,7 @@ function modifyX3() {
 
 let x4 = false;
 modifyX4();
-const obj4 = {};
+const obj4 = { modified: false };
 (x4 && obj4).modified = true;
 
 if (obj4.modified) console.log('modified');
diff --git a/test/form/samples/class-method-access/_expected.js b/test/form/samples/class-method-access/_expected.js
@@ -3,7 +3,7 @@ console.log('retained');
 class Used {
 	static method() {}
 	static get getter() {
-		return { foo: {} };
+		return { foo: { throws: null }, throws: null };
 	}
 }
 Used.method.doesNotExist.throws;
diff --git a/test/form/samples/class-method-access/main.js b/test/form/samples/class-method-access/main.js
@@ -10,7 +10,7 @@ else console.log('removed');
 class Used {
 	static method() {}
 	static get getter() {
-		return { foo: {} };
+		return { foo: { throws: null }, throws: null };
 	}
 }
 
@@ -30,5 +30,5 @@ Used.method.reassigned = 1;
 Used.getter.reassigned = 2;
 
 class ValueEffect {
-	static foo
-}
+	static foo;
+}
diff --git a/test/form/samples/minimal-this-mutation/_expected.js b/test/form/samples/minimal-this-mutation/_expected.js
@@ -40,9 +40,3 @@ const obj5 = {
 obj5.mutateNestedProp();
 if (obj5.nested.prop) console.log('unimportant');
 else console.log('retained');
-
-const obj6 = {
-	prop: true
-};
-obj6.doesNotExist();
-console.log('retained');
diff --git a/test/form/samples/minimal-this-mutation/main.js b/test/form/samples/minimal-this-mutation/main.js
@@ -47,10 +47,3 @@ const obj5 = {
 obj5.mutateNestedProp();
 if (obj5.nested.prop) console.log('unimportant');
 else console.log('retained');
-
-const obj6 = {
-	prop: true
-};
-obj6.doesNotExist();
-if (obj6.prop) console.log('retained');
-else console.log('removed');
diff --git a/test/form/samples/object-expression/nested-literal-assignment-without-access-side-effect/_config.js b/test/form/samples/object-expression/nested-literal-assignment-without-access-side-effect/_config.js
@@ -0,0 +1,6 @@
+module.exports = {
+	description: 'treats mutating nested properties as side effects',
+	options: {
+		treeshake: { propertyReadSideEffects: false }
+	}
+};
diff --git a/test/form/samples/object-expression/nested-literal-assignment-without-access-side-effect/_expected.js b/test/form/samples/object-expression/nested-literal-assignment-without-access-side-effect/_expected.js
@@ -0,0 +1,3 @@
+const obj = { __proto__: null };
+
+obj.foo.bar = 'retained';
diff --git a/test/form/samples/object-expression/nested-literal-assignment-without-access-side-effect/main.js b/test/form/samples/object-expression/nested-literal-assignment-without-access-side-effect/main.js
@@ -0,0 +1,3 @@
+const obj = { __proto__: null };
+
+obj.foo.bar = 'retained';
diff --git a/test/form/samples/object-expression/nested-literal-value-without-access-side-effect/_config.js b/test/form/samples/object-expression/nested-literal-value-without-access-side-effect/_config.js
@@ -0,0 +1,6 @@
+module.exports = {
+	description: 'uses an unknown value for nested properties',
+	options: {
+		treeshake: { propertyReadSideEffects: false }
+	}
+};
diff --git a/test/form/samples/object-expression/nested-literal-value-without-access-side-effect/_expected.js b/test/form/samples/object-expression/nested-literal-value-without-access-side-effect/_expected.js
@@ -0,0 +1,5 @@
+const obj = { __proto__: null };
+
+if (obj.foo.bar) {
+	console.log('retained');
+}
diff --git a/test/form/samples/object-expression/nested-literal-value-without-access-side-effect/main.js b/test/form/samples/object-expression/nested-literal-value-without-access-side-effect/main.js
@@ -0,0 +1,5 @@
+const obj = { __proto__: null };
+
+if (obj.foo.bar) {
+	console.log('retained');
+}
diff --git a/test/form/samples/optional-chaining/_expected.js b/test/form/samples/optional-chaining/_expected.js
@@ -1,3 +1,3 @@
-const obj = {};
+const obj = { __proto__: null };
 obj.foo?.bar;
 obj.foo?.();
diff --git a/test/form/samples/optional-chaining/main.js b/test/form/samples/optional-chaining/main.js
@@ -1,4 +1,4 @@
-const obj = {};
-obj?.foo
+const obj = { __proto__: null };
+obj?.foo;
 obj.foo?.bar;
 obj.foo?.();
diff --git a/test/form/samples/side-effects-class-getters-setters/_expected.js b/test/form/samples/side-effects-class-getters-setters/_expected.js
@@ -23,7 +23,12 @@ class RetainedSuper {
 }
 class RetainedSub extends RetainedSuper {}
 RetainedSub.a;
-log();
+
+// class fields are not part of the prototype
+class RemovedProtoValue {
+	a = true;
+}
+if (!RemovedProtoValue.prototype.a) log();
 
 class DeoptProto {
 	a = true;
diff --git a/test/form/samples/super-classes/super-class-prototype-values/_expected.js b/test/form/samples/super-classes/super-class-prototype-values/_expected.js
@@ -1,7 +1,5 @@
 console.log('retained');
 console.log('retained');
-// Note that isTrueProp is not part of the prototype
-console.log('retained');
 
 const prop = { isTrue: true };
 class SuperDeopt {
diff --git a/test/form/samples/super-classes/super-class-prototype-values/main.js b/test/form/samples/super-classes/super-class-prototype-values/main.js
@@ -1,5 +1,4 @@
 class SuperValues {
-	isTrueProp = true;
 	get isTrue() {
 		return true;
 	}
@@ -12,9 +11,6 @@ if (Values.prototype.isTrue) console.log('retained');
 else console.log('removed');
 if (Values.prototype.prop.isTrue) console.log('retained');
 else console.log('removed');
-// Note that isTrueProp is not part of the prototype
-if (Values.prototype.isTrueProp) console.log('removed');
-else console.log('retained');
 
 const prop = { isTrue: true };
 class SuperDeopt {
diff --git a/test/form/samples/undefined-properties/_config.js b/test/form/samples/undefined-properties/_config.js
diff --git a/test/form/samples/undefined-properties/_expected.js b/test/form/samples/undefined-properties/_expected.js
diff --git a/test/form/samples/undefined-properties/main.js b/test/form/samples/undefined-properties/main.js
diff --git a/test/function/samples/retain-unknown-prototype-access/_config.js b/test/function/samples/retain-unknown-prototype-access/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'treats unknown builtin prototype properties as unknown'
+};
diff --git a/test/function/samples/retain-unknown-prototype-access/main.js b/test/function/samples/retain-unknown-prototype-access/main.js
@@ -0,0 +1,10 @@
+Object.defineProperty(Array.prototype, 'isEmpty', {
+	get() {
+		return this.length === 0;
+	}
+});
+
+const array = [];
+assert.strictEqual(array.isEmpty ? 'works' : 'broken', 'works');
+array.push('foo');
+assert.strictEqual(array.isEmpty ? 'broken' : 'works', 'works');

Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,7 @@ console.log('retained');`
`3`	`3`	`class Used {`
`4`	`4`	`static method() {}`
`5`	`5`	`static get getter() {`
`6`		`- return { foo: {} };`
	`6`	`+ return { foo: { throws: null }, throws: null };`
`7`	`7`	`}`
`8`	`8`	`}`
`9`	`9`	`Used.method.doesNotExist.throws;`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ else console.log('removed');`
`10`	`10`	`class Used {`
`11`	`11`	`static method() {}`
`12`	`12`	`static get getter() {`
`13`		`- return { foo: {} };`
	`13`	`+ return { foo: { throws: null }, throws: null };`
`14`	`14`	`}`
`15`	`15`	`}`
`16`	`16`
`@@ -30,5 +30,5 @@ Used.method.reassigned = 1;`
`30`	`30`	`Used.getter.reassigned = 2;`
`31`	`31`
`32`	`32`	`class ValueEffect {`
`33`		`- static foo`
`34`		`-}`
	`33`	`+ static foo;`
	`34`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+const obj = { __proto__: null };`
	`2`	`+`
	`3`	`+obj.foo.bar = 'retained';`