Skip to content

Commit

Permalink
Treat unknown prototype props as unknown (#4428)
Browse files Browse the repository at this point in the history
* Treat unknown prototype methods as unknown

* Improve coverage
  • Loading branch information
lukastaegert committed Mar 6, 2022
1 parent 9c8894e commit 84c0ea3
Show file tree
Hide file tree
Showing 24 changed files with 100 additions and 59 deletions.
39 changes: 38 additions & 1 deletion 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,
Expand All @@ -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
);
3 changes: 2 additions & 1 deletion test/cli/samples/watch/watch-config-initial-error/_config.js
Expand Up @@ -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')) {
Expand Down
Expand Up @@ -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');
Expand All @@ -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');
Expand Down
8 changes: 4 additions & 4 deletions 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');
Expand All @@ -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');
Expand All @@ -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');
Expand All @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion test/form/samples/class-method-access/_expected.js
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions test/form/samples/class-method-access/main.js
Expand Up @@ -10,7 +10,7 @@ else console.log('removed');
class Used {
static method() {}
static get getter() {
return { foo: {} };
return { foo: { throws: null }, throws: null };
}
}

Expand All @@ -30,5 +30,5 @@ Used.method.reassigned = 1;
Used.getter.reassigned = 2;

class ValueEffect {
static foo
}
static foo;
}
6 changes: 0 additions & 6 deletions test/form/samples/minimal-this-mutation/_expected.js
Expand Up @@ -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');
7 changes: 0 additions & 7 deletions test/form/samples/minimal-this-mutation/main.js
Expand Up @@ -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');
@@ -0,0 +1,6 @@
module.exports = {
description: 'treats mutating nested properties as side effects',
options: {
treeshake: { propertyReadSideEffects: false }
}
};
@@ -0,0 +1,3 @@
const obj = { __proto__: null };

obj.foo.bar = 'retained';
@@ -0,0 +1,3 @@
const obj = { __proto__: null };

obj.foo.bar = 'retained';
@@ -0,0 +1,6 @@
module.exports = {
description: 'uses an unknown value for nested properties',
options: {
treeshake: { propertyReadSideEffects: false }
}
};
@@ -0,0 +1,5 @@
const obj = { __proto__: null };

if (obj.foo.bar) {
console.log('retained');
}
@@ -0,0 +1,5 @@
const obj = { __proto__: null };

if (obj.foo.bar) {
console.log('retained');
}
2 changes: 1 addition & 1 deletion test/form/samples/optional-chaining/_expected.js
@@ -1,3 +1,3 @@
const obj = {};
const obj = { __proto__: null };
obj.foo?.bar;
obj.foo?.();
4 changes: 2 additions & 2 deletions 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?.();
Expand Up @@ -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;
Expand Down
@@ -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 {
Expand Down
@@ -1,5 +1,4 @@
class SuperValues {
isTrueProp = true;
get isTrue() {
return true;
}
Expand All @@ -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 {
Expand Down
3 changes: 0 additions & 3 deletions test/form/samples/undefined-properties/_config.js

This file was deleted.

10 changes: 0 additions & 10 deletions test/form/samples/undefined-properties/_expected.js

This file was deleted.

11 changes: 0 additions & 11 deletions test/form/samples/undefined-properties/main.js

This file was deleted.

@@ -0,0 +1,3 @@
module.exports = {
description: 'treats unknown builtin prototype properties as unknown'
};
10 changes: 10 additions & 0 deletions 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');

0 comments on commit 84c0ea3

Please sign in to comment.