Tests fail when SES is used

[ljharb]

If https://github.com/ljharb/object.assign/blob/main/test/ses-compat.js#L15 is changed to require('.'), so it runs the tests:

Boolean.prototype.valueOf.call(require('.')(true)) fails (altho Boolean.prototype.valueOf.call(Object.assign(true)) still succeeds).

cc @kumavis

We need to figure out if this is a bug in the entry point, or a bug in SES.

[ljharb]

I'm also seeing issues in es-abstract where the main entry point is returning a non-extensible object.

[kumavis]

so i see this

object.assign on  main [!] is 📦 v4.1.1 via ⬢ v12.16.1 
⇡0% ➜ node test/ses-compat.js 
/home/xyz/Development/object.assign/node_modules/define-properties/index.js:34
		origDefineProperty(object, name, {
		^

[TypeError <Object <Object <[Object: null prototype] {}>>>: Cannot define property getPolyfill, object is not extensible

but its coming from object.assign/node_modules/object.assign which does not have the patch, and seems like a mistake in general

[kumavis]

object.assign on  main [!] is 📦 v4.1.1 via ⬢ v12.16.1 
⇡0% ➜ yarn why object.assign
yarn why v1.22.4
[1/4] Why do we have the module "object.assign"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "object.assign@4.1.0"
info Reasons this module exists
   - "es-abstract" depends on it
   - Hoisted from "es-abstract#object.assign"
   - Hoisted from "tape#object.assign"
   - Hoisted from "tape#deep-equal#object.assign"
   - Hoisted from "tape#string.prototype.trim#es-abstract#object.assign"
   - Hoisted from "aud#util.promisify#es-abstract#object.assign"
   - Hoisted from "tape#deep-equal#es-abstract#object.assign"
   - Hoisted from "tape#object-is#es-abstract#object.assign"
   - Hoisted from "es-abstract#string.prototype.trimend#es-abstract#object.assign"
   - Hoisted from "es-abstract#string.prototype.trimstart#es-abstract#object.assign"
   - Hoisted from "safe-publish-latest#yargs#yargs-parser#object.assign"
   - Hoisted from "aud#util.promisify#object.getownpropertydescriptors#es-abstract#object.assign"
   - Hoisted from "tape#deep-equal#regexp.prototype.flags#es-abstract#object.assign"
   - Hoisted from "tape#deep-equal#es-get-iterator#es-abstract#object.assign"
   - Hoisted from "tape#deep-equal#which-typed-array#es-abstract#object.assign"
   - Hoisted from "tape#deep-equal#which-typed-array#is-typed-array#es-abstract#object.assign"
info Disk size without dependencies: "104KB"
info Disk size with unique dependencies: "356KB"
info Disk size with transitive dependencies: "356KB"
info Number of shared dependencies: 4
Done in 0.38s.

object.assign on  main [!] is 📦 v4.1.1 via ⬢ v12.16.1 
⇡0% ➜ cat node_modules/object.assign/package.json| jq .version
"4.1.0"

object.assign on  main [!] is 📦 v4.1.1 via ⬢ v12.16.1 
⇡0% ➜ cat package.json| jq .version 
"4.1.1"

[kumavis]

for Boolean.prototype.valueOf.call(require('.')(true))

the issue is related to GetIntrinsics perhaps inconsistent behavior in handling getters. GetIntrinsic will return the properties value OR its getter. The getter is not called to retrieve the value. For consistency, I think GetIntrinsic should return the property descriptor or it should call the getter and return its value.

Reflect.getOwnPropertyDescriptor(Function.prototype, 'apply')
// no ses:
// --> { value: apply () { ... } }
// with ses
// --> { get: getApply () { ... } }

[ljharb]

I’m seeing that error even with the patched version of object.assign :-/

Does that mean that SES replaces intrinsic data property methods with getters?? The spec requires they be data properties.

[kumavis]

Does that mean that SES replaces intrinsic data property methods with getters

yes, though i think its mostly just to make it read-only. doesnt seem like a requirement.

The spec requires they be data properties.

oh really? could you provide a reference for that? ill bring it up at the next ses-lavamoat call

[ljharb]

i mean, everything that doesn't have a get in front of it is always a data property (https://tc39.es/ecma262/#sec-boolean.prototype.valueof) per https://tc39.es/ecma262/#sec-ecmascript-standard-built-in-objects paragraph 12.

[kumavis]

I see, this get/set pattern is the overwrite mistake workaround

[ljharb]

I'm not sure what the fix can really be here. I suppose I could try to detect SES, but it seems pretty bad to have es-abstract rely on spec violations as a heuristic for when SES is present.

I'm a bit confused tho - the "overwrite mistake" is how the spec works, right? What if there's code relying on it?

[erights]

I'm a bit confused tho - the "overwrite mistake" is how the spec works, right? What if there's code relying on it?

This fix for the override mistake is used by Agoric, Salesforce, MetaMask, Node, Google, Bloomberg and others. AFAIK, none of us have run into a single example of code we wanted to run that relies on the override mistake throwing an exception. However, my impression is that tc39/ecma262#1320 (comment) -- the proposal to fix the override mistake pervasively and unconditionally --- did run into some compat problem. In any case, the total amount of code broken by that will be negligible compared to the code broken by freezing the primordials.

[erights]

However, the concern you raise --- code broken not by the absence of override-mistake behavior, but rather, caused by changing these properties from data to accessor, is a different matter. I would like to understand better the problem you're running into. Can you state a self-contained explanation of why and how your code is hitting this? Thanks.

[ljharb]

I certainly can understand making that tradeoff decision within SES - but it leaves me in a position where some of my packages are incompatible with SES, and also, all of my packages's tests are likely incompatible with SES.

Do you have a suggestion that wouldn't involve either a) hardcoding all expected data properties (or, all expected accessors), or b) hardcoding assumptions about SES?

[erights]

Before making suggestions, I'd like to understand the problem first. Self contained explanation please?

[ljharb]

Sure! I have a GetIntrinsic helper that gets and caches the original (first-run, ofc) value of intrinsics. It supports auto-expanding the dotted notation used in the spec, and when it finds an accessor, it automatically reifies it (calls the get function).

When I do, for example, GetIntrinsic('%Boolean.prototype.valueOf%'), the code expects that data property to be returned - but when it's an accessor, it invokes the get function (hoping to get the actual underlying function value) which throws an error. (that's my best understanding of the problem)

[erights]

Why does invoking the accessor's getter throw an error? The getter of the accessors created by this repair return the original value. Can we try a simple example to demonstrate the problem? When your code calls the accessor's getter, what this-binding does it provide? If it provides an object for which . would have worked as a data property --- i.e., an object that has the property either as own or non-overridden inherited --- it should work.

[erights]

Scratch that. The getter itself should work independent of this binding. Only the setter cares about the this binding.

[erights]

The relevant getter is defined at
https://github.com/Agoric/SES-shim/blob/378bee45b8b470fc519701009b4e0ec1de0f0647/packages/ses/src/enable-property-overrides.js#L46-L48 as

function getter() {
  return value;
}

That's it. value is the original value of the data property before it was converted to an accessor. If you call the getter, it returns that value.

[erights]

Where is your GetIntrinsic defined? Where is the relevant line where it calls the accessor's getter?

[ljharb]

Trying to narrow this down:

Boolean.prototype.valueOf.call(require('es-abstract/helpers/callBind').apply(Object.assign)(Object, [true]));

this passes without lockdown, fails with it.

Boolean.prototype.valueOf.call(Reflect.apply(Function.bind, Function.apply, [Object.assign])(Object, [true]));

passes on both, as does:

Boolean.prototype.valueOf.call(Reflect.apply(require('function-bind'), Function.prototype.apply, [Object.assign])(Object, [true]));

[ljharb]

https://github.com/ljharb/es-abstract/blob/HEAD/GetIntrinsic.js#L266 is the GetIntrinsic line.

[erights]

At #79 (comment) you said

when it finds an accessor, it automatically reifies it (calls the get function).

But, as @kumavis commented earlier, you're not calling the getter. You're using the getter function itself as the value. That can never be correct. Try

value = isOwn ? desc.get() || desc.value : value[part];

which is an adequate fix for SES, but is not correct in general because it doesn't give the getter the proper this. But SES's getter doesn't use this so it should solve the immediate problem.

[ljharb]

OK, this code fails with SES but passes otherwise:

var { assign } = Object;
var apply = GetIntrinsic('%Function.prototype.apply%');
Boolean.prototype.valueOf.call(Reflect.apply(Function.prototype.bind, apply, [assign])(Object, [true]));

In other words, GetIntrinsic('%Function.prototype.apply%') with SES breaks, but without does not.

[erights]

Why isn't this line simply

value = value[part];

Under what circumstances does this misbehave?

[ljharb]

require('es-abstract/GetIntrinsic')('%Function.prototype.apply%').toString() produces 'function apply() { [native code] }' without SES - with SES, it produces 'function getter() {\n return value;\n }'.

In other words, I misspoke. es-abstract is NOT reifying the getter, which means that when you convert a data property to an accessor, i suddenly start getting a getter function instead of the real function.

[erights]

In other words, I misspoke. es-abstract is NOT reifying the getter, which means that when you convert a data property to an accessor, i suddenly start getting a getter function instead of the real function.

That's a bug. What you said earlier is what it should do. Yes? Once fixed --- see my suggested replacements above --- does everything work?

[erights]

My code replacement at #79 (comment) is incorrect because it uses the result of the call to the left of the || when the call itself should be conditional on the existence of the getter. Easily fixed. But still, why not #79 (comment)

[ljharb]

It's not a bug, it's how it works by design. The intended use is to get getters as functions, so they can be .called later - like a Map's "size" getter, or a RegExp's "flags" getter.

In other words, I'm trying to get the getter when the spec says it's a getter and the value when the spec says it's a data property.

[erights]

This special case for the getter seems weird. How do you name the setter?

[ljharb]

There are no spec-provided setters I'm aware of besides Object.prototype.__proto__, and I have no use case for using that intrinsic (i have a setPrototypeOf helper for that).