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

Support constructors that return an object but also throw #2501

Closed

Conversation

calebmer
Copy link
Contributor

The following example currently fails with a FatalError.

function F() {
  const b = global.__abstract ? global.__abstract("boolean", "false") : false;
  if (b) throw new Error("abrupt");
  return { p: 42 };
}

const result = new F();

global.inspect = () => JSON.stringify(result);

This is where the FatalError is thrown:

https://github.com/facebook/prepack/blob/f59798b19678b52fe11ae77def2dfe551e374c1e/src/methods/function.js#L264-L267

The majority of this PR is refactoring the type of construction functions to return a Value (which includes AbstractValue) instead of a concrete ObjectValue. I updated call sites to include a throwIfNotConcreteObject() instead of handling abstract values in many cases.

Would it be useful to add some general Value.prototype.map utility function? Which would provide ConcreteValues to a mapper function and abstract over conditional values, __bottomValue, and more.

Motivation

Babel return an object from constructors for classes that call super() to follow the spec. This means we hit an invariant on all React class components which might throw. Which happens to be quite a few given the prevalent use of nullthrows() or invariant() in Facebook codebases.

@trueadm
Copy link
Contributor

trueadm commented Aug 28, 2018

Shouldn't they return ObjectValue | AbstractObjectValue in this case? It doesn't make sense why they might return anything other than that.

@calebmer
Copy link
Contributor Author

True. Updated to use ObjectValue | AbstractObjectValue.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants