prepack icon indicating copy to clipboard operation
prepack copied to clipboard

ObjectValue has optional fields which isn't supported

Open sebmarkbage opened this issue 5 years ago • 2 comments

Flow doesn't support optional field in classes. Internally, we transpile using the class fields transform which turns all Flow annotated fields into field initializers with undefined.

So ObjectValue turns into this monster:

class ObjectValue extends _index2.ConcreteValue {
  constructor(
  realm,
  proto,
  intrinsicName,
  refuseSerialization = false)
  {
    super(realm, intrinsicName);
    _defineProperty(this, "$Prototype", void 0);
    _defineProperty(this, "$Extensible", void 0);
    _defineProperty(this, "$ParameterMap", void 0);
    _defineProperty(this, "$SymbolData", void 0);
    _defineProperty(this, "$StringData", void 0);
    _defineProperty(this, "$NumberData", void 0);
    _defineProperty(this, "$BooleanData", void 0);
    _defineProperty(this, "$ErrorData", void 0);
    _defineProperty(this, "$Call", void 0);
    _defineProperty(this, "$Construct", void 0);
    _defineProperty(this, "$PromiseState", void 0);
    _defineProperty(this, "$PromiseResult", void 0);
    _defineProperty(this, "$PromiseFulfillReactions", void 0);
    _defineProperty(this, "$PromiseRejectReactions", void 0);
    _defineProperty(this, "$PromiseIsHandled", void 0);
    _defineProperty(this, "$IteratedList", void 0);
    _defineProperty(this, "$ListIteratorNextIndex", void 0);
    _defineProperty(this, "$IteratorNext", void 0);
    _defineProperty(this, "$SetIterationKind", void 0);
    _defineProperty(this, "$SetNextIndex", void 0);
    _defineProperty(this, "$IteratedSet", void 0);
    _defineProperty(this, "$SetData", void 0);
    _defineProperty(this, "$SuperTypeParameters", void 0);
    _defineProperty(this, "$MapIterationKind", void 0);
    _defineProperty(this, "$MapNextIndex", void 0);
    _defineProperty(this, "$MapData", void 0);
    _defineProperty(this, "$Map", void 0);
    _defineProperty(this, "$WeakMapData", void 0);
    _defineProperty(this, "$WeakSetData", void 0);
    _defineProperty(this, "$DateValue", void 0);
    _defineProperty(this, "$ArrayIterationKind", void 0);
    _defineProperty(this, "$ArrayIteratorNextIndex", void 0);
    _defineProperty(this, "$IteratedObject", void 0);
    _defineProperty(this, "$OriginalSource", void 0);
    _defineProperty(this, "$OriginalFlags", void 0);
    _defineProperty(this, "$RegExpMatcher", void 0);
    _defineProperty(this, "$StringIteratorNextIndex", void 0);
    _defineProperty(this, "$IteratedString", void 0);
    _defineProperty(this, "$DataView", void 0);
    _defineProperty(this, "$ViewedArrayBuffer", void 0);
    _defineProperty(this, "$ByteLength", void 0);
    _defineProperty(this, "$ByteOffset", void 0);
    _defineProperty(this, "$ArrayBufferData", void 0);
    _defineProperty(this, "$ArrayBufferByteLength", void 0);
    _defineProperty(this, "$GeneratorState", void 0);
    _defineProperty(this, "$GeneratorContext", void 0);
    _defineProperty(this, "$TypedArrayName", void 0);
    _defineProperty(this, "$ViewedArrayBuffer", void 0);
    _defineProperty(this, "$ArrayLength", void 0);
    _defineProperty(this, "originalConstructor", void 0);
    _defineProperty(this, "_isPartial", void 0);
    _defineProperty(this, "_isLeaked", void 0);
    _defineProperty(this, "_isSimple", void 0);
    _defineProperty(this, "_isFinal", void 0);
    _defineProperty(this, "_isScopedTemplate", void 0);
    _defineProperty(this, "_simplicityIsTransitive", void 0);
    _defineProperty(this, "_templateFor", void 0);
    _defineProperty(this, "properties", void 0);
    _defineProperty(this, "symbols", void 0);
    _defineProperty(this, "unknownProperty", void 0);
    _defineProperty(this, "_temporalAlias", void 0);
    _defineProperty(this, "intrinsicNameGenerated", void 0);
    _defineProperty(this, "hashValue", void 0);
    _defineProperty(this, "$BailOutReason", void 0);
    _defineProperty(this, "$IsClassPrototype", void 0);
    _defineProperty(this, "refuseSerialization", void 0);
    realm.recordNewObject(this);
    if (realm.useAbstractInterpretation) this.setupBindings(this.getTrackedPropertyNames());
    this.$Prototype = proto || realm.intrinsics.null;
    this.$Extensible = realm.intrinsics.true;
    this._isPartial = realm.intrinsics.false;
    this._isLeaked = realm.intrinsics.false;
    this._isSimple = realm.intrinsics.false;
    this._simplicityIsTransitive = realm.intrinsics.false;
    this._isFinal = realm.intrinsics.false;
    this.properties = new Map();
    this.symbols = new Map();
    this.refuseSerialization = refuseSerialization;

    // this.$IsClassPrototype should be the last thing that gets initialized,
    // as other code checks whether this.$IsClassPrototype === undefined
    // as a proxy for whether initialization is still ongoing.
    this.$IsClassPrototype = false;
  }

It might be a good idea to have consistent fields since it allows hidden classes to be shared, but if we want that, we should use assignment since that lets the VM guess field count statically.

However, since we have so many of them on core objects, we are probably really just wasting memory.

The bigger issue though is that we have semantics that checks for the existence of a field. So this is actually causing bugs:

https://github.com/facebook/prepack/blob/ec3638915e43eedf039939fd69d2d7d1d3c30719/src/intrinsics/ecma262/ArrayBufferPrototype.js#L30

sebmarkbage avatar Aug 30 '18 06:08 sebmarkbage

I'm not sure the best plan of action here. It seems our internal Babel transform for this is not spec compliant yet the one we use on Github is? If you look at the compiled output in lib/values/ObjectValue.js it looks nothing like what you pasted above.

Anyway, here's a PR that removes in usage and changes them to explicit undefined checks like you did in your other PR: https://github.com/facebook/prepack/pull/2512

trueadm avatar Aug 30 '18 13:08 trueadm

I think the one we use internally is actually the spec compliant one unfortunately.

sebmarkbage avatar Aug 31 '18 01:08 sebmarkbage