Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

ObjectValue has optional fields which isn't supported #2510

Open
sebmarkbage opened this issue Aug 30, 2018 · 2 comments
Open

ObjectValue has optional fields which isn't supported #2510

sebmarkbage opened this issue Aug 30, 2018 · 2 comments

Comments

@sebmarkbage
Copy link
Contributor

sebmarkbage commented Aug 30, 2018

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

@trueadm
Copy link
Contributor

trueadm commented Aug 30, 2018

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: #2512

@sebmarkbage
Copy link
Contributor Author

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

facebook-github-bot pushed a commit that referenced this issue Aug 31, 2018
Summary:
Release notes: landing two previously reverted PRs

I found the issue.

Flow doesn't support optional fields in classes. This effectively becomes enforced with Babel 7 since it treats the type annotations as field initializers. Which means that these fields always gets created.

We happened to only use this newer plugin for class fields internally which broke our builds only there.

2b4546d Use `undefined` instead of missing to represent absent field in descriptors. Fixed all the callsites that checks for `hasOwnProperty` or `in` that I could find.

922d40c Fixes a Flow issue in the text printer.

I filed a [follow up issue for ObjectValue](#2510) since it has the same problem.
Pull Request resolved: #2511

Reviewed By: trueadm

Differential Revision: D9569949

Pulled By: sebmarkbage

fbshipit-source-id: f8bf84c4385de4f0ff6bcd45badacd3b8c88c533
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants