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

Treat unknown prototype props as unknown #4428

Merged
merged 3 commits into from Mar 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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');