Fixed bugs involving unsafe atemporals and improved abstract concrete unions #2513
Conversation
sb98052
changed the title
src/values/AbstractValue.js
Outdated
| static createAbstractConcreteUnion( | ||
| realm: Realm, | ||
| abstractValue: AbstractValue, | ||
| concreteValues: Array<Value> |
There was a problem hiding this comment.
use Array<ConcreteValue>
src/intrinsics/prepack/utils.js
Outdated
| @@ -67,7 +59,7 @@ export function createAbstract( | |||
| typeNameOrTemplate?: Value | string, | |||
| name?: string, | |||
| options?: ObjectValue, | |||
| ...additionalValues: Array<ConcreteValue> | |||
| ...additionalValues: Array<Value> | |||
There was a problem hiding this comment.
Rather keep the type as Array<ConcreteValue> and fix up the call sites.
src/methods/properties.js
Outdated
| let args = savedUnion.args.slice(0); | ||
| args[savedIndex] = value; | ||
| value = AbstractValue.createAbstractConcreteUnion(realm, ...args); | ||
| let concreteValues = savedUnion.args.slice(1); |
There was a problem hiding this comment.
Instead of using a slice, use a filter.
src/values/AbstractValue.js
Outdated
| } | ||
| } | ||
|
|
||
| static dischargeValuesFromUnion(realm: Realm, union: AbstractValue): [AbstractValue, Array<Value>] { |
There was a problem hiding this comment.
Return Array<ConcreteValue>
src/values/AbstractValue.js
Outdated
| invariant(union.args[0] instanceof AbstractValue); | ||
| invariant(union.args[1] instanceof ConcreteValue); | ||
|
|
||
| let [abstractValue, ...concreteValues] = union.args; |
There was a problem hiding this comment.
Rather use a filter to get the concrete values.
| @@ -1,4 +1,3 @@ | |||
| // Copies of f;:1 | |||
There was a problem hiding this comment.
Please explain why this is not longer desirable.
There was a problem hiding this comment.
My changes broke the test by causing the value of f to be derived in the consequent branch of f && f(), which counts as a second occurrence of f; Since the derivation is legit, I considered updating the comparator to two. But upon digging further I found that I could not reproduce the original problem that you fixed with #1158, even after reverting your change, making me doubt if it was still relevant, so I dropped it. But in hindsight, now, perhaps it is not a bad thing to keep it around as a regression test, so I have added it back.
sb98052
force-pushed
the
abstract-concrete-fixes
branch
from
17de4c2
to
a2ca13f
Compare
There was a problem hiding this comment.
sb98052 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release notes: Fixed bugs that could cause generated code to throw
A refactor of my previous attempt to address #2327, which I had to abandon because it was incompatible with the current implementation of certain modeling primitives. Like the previous version, this PR is an intermediate fix that will be refined in a follow up PR. It consists of three changes:
It adds a new helper that discharges values from a union after deriving the abstract value in it if needed.
It makes conditionally temporal values safe using a helper called
convertToTemporalIfArgsAreTemporal.It makes the the abstract and concrete members explicit in the interface to Abstract Concrete Unions. Presently, two code sites make the assumption that the abstract member is the first element of
args, while all others traverse the list to locate it.Follow ups to come:
convertToTemporalIfArgsAreTemporalto produce a conditional value left to be simplified by the serializer.Fixes #2327 and #2406