Prepack: Simple Object cannot safely be coerced toString/valueOf

[sebmarkbage]

I've noticed a few calls that assume that Simple Object can be safely coerced using toString/valueOf without side-effects but that's not enforced.

It is also not possible to fully enforce for partial simple objects where the keys are unknown.

https://github.com/facebook/prepack/blob/ebfd530fb0eb53916743ad1d92fa18df81a80bb9/src/values/ObjectValue.js#L652

https://github.com/facebook/prepack/blob/c2d535f0894fb7225131aa789811a1ba0bebac0f/src/environment.js#L1390

We should either make this restriction enforced, or drop the assumptions that this is safe.

[hermanventer]

Partial objects can only be simple if they are explicitly declared to be so, in which case we just assume rather than determine.

[sebmarkbage]

There's at least one case where it is automatic. https://github.com/facebook/prepack/blob/master/src/intrinsics/ecma262/Object.js#L140

Perhaps that was a mistake.

[hermanventer]

I don't see a problem with this line.

[sebmarkbage]

It forces the target to be simple if any of the sources are partial/simple. But one of the sources might not be.

  var source = { valueOf() { throw new Error(); } };
  var source2 = {};
  __makePartial(source2);
  __makeSimple(source2);
  var target = {};
  Object.assign(target, source, source2); // target is marked as simple
  var c = __abstract('boolean', '(true)');
  var abstractKey = c ? target : {};
  var n = {x: ''};
  result = n[abstractKey]; // this should not be allowed because target has a side-effectful valueOf

I suppose Object.assign can enforce that it is only simple if all the sources are simple.

[hermanventer]

OK, the basic problem here is with the definition of "simple object". If is the original "does not have getter and setters", then target is indeed simple.

Since we now want "simple object" to mean "does not do funny stuff when you get/set a property or convert the object to a primitive", we can only conclude that target is simple if all of the arguments to the Object.assign are simple as well and we need to update the implementation of Object.assign to be more careful before it designates target as simple.

We also need to add logic to ObjectValue.isSimpleObject.

[sebmarkbage]

Btw, I'm not sure if this is the right tradeoff. Maybe it's more important for Object.assign to be smart than ToStringing objects.