Nominally Type Descriptors #2473

sebmarkbage · 2018-08-23T08:27:11Z

Release notes: Refactor of joined descriptors. Expect a slight performance regression. Will fatal in more cases that we would previously silently possibly generate the wrong code.

Currently we have three types of property descriptors. 1) Descriptors used by internal slots. 2) Normal concrete descriptors. 3) Abstract joined descriptors.

These are all typed as arbitrary an inexact object where all the fields are optional. That means that essentially any object with any typo can be assigned to a descriptor.

We are also not forced to deal with the joined descriptor case. Almost everywhere we assume that joined descriptors are really just a generic descriptor object which doesn't have any attributes on it.

There are so many small bugs related to this so I figured it's time to start dealing with it.

This PR turns this inexact object into nominally typed PropertyDescriptor, AbstractJoinedDescriptor and InternalSlotDescriptor. Essentially the same model as values.

Thanks to this Flow forces me to ensure that I've covered all of these cases. I've dealt with it in the cases I figured I could figure it out and where it was necessary such as the serializer/join/widen. In all other cases where we don't know, I ensure that we throw a fatal error instead of assuming that a joined descriptor is an empty descriptor.

hermanventer · 2018-08-23T20:22:28Z

I've tried to scratch this itch a number of times already, but always backed off because the magnitude of the change seemed risky and the benefits are ultimately not entirely clear.

The diff is pretty hard to review, so I would ask that it be purely "mechanical" with no changes of functionality or bug fixes.

hermanventer · 2018-08-23T20:16:07Z

src/environment.js

@@ -948,7 +955,8 @@ export class GlobalEnvironmentRecord extends EnvironmentRecord {

    // 5. If existingProp is undefined, return ? IsExtensible(globalObject).
    if (!existingProp) return IsExtensible(realm, globalObject);
-    Properties.ThrowIfMightHaveBeenDeleted(existingProp.value);
+    Properties.ThrowIfMightHaveBeenDeleted(existingProp);
+    existingProp = existingProp.throwIfNotConcrete(globalObject.$Realm);


This suggests that the result returned from throwIfNotConcrete can be different from the old value of existingProp. That makes the code harder to read in my opinion.

This is already a pattern we have for Value. I think we should stick to one pattern or switch both at once.

It is plausible that we could make Flow refine these for us similarly to how I did it with IsDataDescriptor. But for now I stuck with the pattern already in place for Values.

hermanventer · 2018-08-23T20:29:40Z

"These are all typed as arbitrary an inexact object where all the fields are optional. That means that essentially any object with any typo can be assigned to a descriptor."

As a first step, the existing type can just be made exact, no?

sebmarkbage · 2018-08-23T20:48:51Z

The inexact thing doesn't give us much. We've already fixed a few of those. The problem is that we have hundreds of places that just doesn't deal with joined descriptors and always does the wrong thing instead of fataling. Which means that I can't rely on that mechanism to work. That's preventing me from doing more with this like in #2461.

Most things in here are mostly mechanical. I've split out things that are not just mechanical in separate commits and I'll go through and highlight them.

In the interpreter I'm trying to just throw fatals as much as possible instead of trying to fix the callsites and then fix them in follow ups.

I've tried to support them as much as possible in the serializer to avoid having to error. The relevant serializer commit: 39817cb cc @NTillmann

facebook-github-bot

sebmarkbage has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

sebmarkbage · 2018-08-28T04:33:27Z

Ok this is now ready to review. I've split the commits up by whether they're adding a primary mechanism that is worth reviewing or if they're just mechanical updates to the code.

Primary Mechanism

These are worth reviewing:

Add normal type for descriptors 064e271
Add refinement to concrete PropertyDescriptor in IsDataDescriptor 9d48ffc - This is an advanced Flow feature to get the refinement from IsDataDescriptor that we already implicitly rely on.
Support nominally typed descriptors in the serializer 39817cb - cc @NTillmann this one is for you. This actually adds some functionality that I don't think we really dealt with before.
Add mightHaveBeenDeleted helper on Descriptor 1e8f329 - My thinking is that we might want to start asserting more of this at the descriptor level rather than the value but it's also common enough to warrant a helper that works like the Value level.
Optimistic optimizations for when the descriptor is not joined 2a910ba
Increase timeout 852f580 - There seems to be a perf regression that causes one of the RegExp tests in test262 to become fragile. Increasing the timeout a bit.

Mechanical Work

These are most just mechanical and are not worth reviewing in detail.

These changes a bunch of callsites to use a slightly different API.

Pass Descriptor to ThrowIfMightHaveBeenDeleted instead of Value 15076d0
Wrap Descriptor literals in PropertyDescriptor class 12e21e4
Wrap $DefineOwnProperty calls in nominal PropertyDescriptor 36b7b23 - We don't want to support the convenience DescriptorInitializer since that requires us to check it many many places.
Update DefinePropertyOrThrow to wrap in PropertyDescriptor 8b6966f
Move descriptor helpers out to the definition of descriptors 765c7fd - Unnecessary file that doesn't include standard methods. Avoids a cycle too.

These add fatal errors where we don't yet handle joined descriptors, or invariants where we already know there won't be any.

Throw if not concrete 9cd65f3
Add invariants to React reconcilers 259012d - cc @trueadm, the React parts doesn't consider joined descriptors. We'll have to fix as needed or perhaps fatal if it is possible to get into these.
Add invariants to intrinsic initialization 89cf142
Adds invariants because unknown properties never join 1d41aec
Add invariants because we checked the other case 7eb51be

Regression

I had to disable one test because we don't actually handle it properly. It just happened to work in this particular edge case accidentally.

Expect to throw introspection error on conditional descriptor 6cd5dbb

NTillmann · 2018-08-28T16:53:21Z

src/descriptors.js

+}
+
+// Only used if the result of a join of two descriptors is not a data descriptor with identical attribute values.
+// When present, any update to the property must produce effects that are the join of updating both desriptors,


sp: desriptors

NTillmann · 2018-08-28T16:55:03Z

src/descriptors.js

+// using joinCondition as the condition of the join.
+export class AbstractJoinedDescriptor extends Descriptor {
+  joinCondition: AbstractValue;
+  descriptor1: void | Descriptor;


Some more comments on the meaning of descriptor1 or descriptor2 being absent would be enlightening. (Not asking for actual code changes here, just comments describing intention.)
Looking around at where AbstractJoinedDescriptor is being instantiated it seems that (eventually) at least one of the two descriptors must be defined, and the absent descriptor is somewhat equivalent to PropertyDescriptor where value is EmptyValue.

NTillmann · 2018-08-28T16:55:39Z

src/descriptors.js

+    if (this.descriptor2 && this.descriptor2.mightHaveBeenDeleted()) {
+      return true;
+    }
+    return false;


In a way, the absence of either descriptor indicates that the thing doesn't exists that thus mightHaveBeenDeleted.

NTillmann

LGTM. Nice refactoring!

They now have to be wrapped in a nominal type.

When these functions are used, they're assumed to be working with concrete property descriptors. We must throw if they're not concrete. To let Flow take advantage of the type assertions, we also do an instanceof check using Predicate Functions.

For a joined descriptor we need to check to see that neither condition might be empty.

The React code isn't always executing to throwing fatals doesn't seem prudent. It effectively assumes that there are no joined descriptors so I'm instead just adding invariants and then we'll have to work those off.

Will need to be fixed in a follow up.

Might consider moving these to instance methods.

There seems to be a slight perf regression which causes one of the RegExp tests to take a bit longer.

… deleted

facebook-github-bot

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

sebmarkbage added the WIP This pull request is a work in progress and not ready for merging. label Aug 23, 2018

facebook-github-bot added the CLA Signed label Aug 23, 2018

hermanventer reviewed Aug 23, 2018

View reviewed changes

sebmarkbage mentioned this pull request Aug 23, 2018

Refactor AbstractObjectValue to use evaluateWithAbstractConditional #2461

Closed

sebmarkbage force-pushed the typedescriptors branch 4 times, most recently from ae85558 to 6cd5dbb Compare August 27, 2018 23:48

sebmarkbage requested a review from NTillmann August 28, 2018 00:00

facebook-github-bot reviewed Aug 28, 2018

View reviewed changes

sebmarkbage changed the title ~~[WIP] Nominally Type Descriptors~~ Nominally Type Descriptors Aug 28, 2018

sebmarkbage removed the WIP This pull request is a work in progress and not ready for merging. label Aug 28, 2018

NTillmann reviewed Aug 28, 2018

View reviewed changes

NTillmann approved these changes Aug 28, 2018

View reviewed changes

hermanventer approved these changes Aug 28, 2018

View reviewed changes

sebmarkbage added 9 commits August 28, 2018 11:45

Add normal type for descriptors

facb464

They now have to be wrapped in a nominal type.

Wrap $DefineOwnProperty calls in nominal PropertyDescriptor

300407d

Update DefinePropertyOrThrow to wrap in PropertyDescriptor

c6f62e0

Pass Descriptor to ThrowIfMightHaveBeenDeleted instead of Value

375751b

For a joined descriptor we need to check to see that neither condition might be empty.

Wrap Descriptor literals in PropertyDescriptor class

9517a99

Support nominally typed descriptors in the serializer

ea20deb

Throw if not concrete

eb94dbf

Add invariants to React reconcilers

86b300f

The React code isn't always executing to throwing fatals doesn't seem prudent. It effectively assumes that there are no joined descriptors so I'm instead just adding invariants and then we'll have to work those off.

sebmarkbage added 9 commits August 28, 2018 11:47

Add invariants to intrinsic initialization

461d811

Adds invariants because unknown properties never join

67ca44a

Add invariants because we checked the other case

d1c847e

Optimistic optimizations for when the descriptor is not joined

bce7a0a

Add mightHaveBeenDeleted helper on Descriptor

49efb3a

Expect to throw introspection error on conditional descriptor

be5dd69

Will need to be fixed in a follow up.

Move descriptor helpers out to the definition of descriptors

9ab0003

Might consider moving these to instance methods.

Increase timeout

9c89e5f

There seems to be a slight perf regression which causes one of the RegExp tests to take a bit longer.

Switch mightHaveBeenDeleted to consider an empty descriptor branch as…

b4a46a1

… deleted

sebmarkbage force-pushed the typedescriptors branch from 852f580 to b4a46a1 Compare August 28, 2018 18:48

facebook-github-bot reviewed Aug 28, 2018

View reviewed changes

facebook-github-bot closed this in 2b68c6e Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nominally Type Descriptors #2473

Nominally Type Descriptors #2473

sebmarkbage commented Aug 23, 2018 •

edited

Loading

hermanventer commented Aug 23, 2018

hermanventer Aug 23, 2018

sebmarkbage Aug 28, 2018

hermanventer commented Aug 23, 2018

sebmarkbage commented Aug 23, 2018 •

edited

Loading

facebook-github-bot left a comment

sebmarkbage commented Aug 28, 2018

NTillmann Aug 28, 2018

NTillmann Aug 28, 2018

NTillmann Aug 28, 2018

NTillmann left a comment

facebook-github-bot left a comment

Nominally Type Descriptors #2473

Nominally Type Descriptors #2473

Conversation

sebmarkbage commented Aug 23, 2018 • edited Loading

hermanventer commented Aug 23, 2018

hermanventer Aug 23, 2018

Choose a reason for hiding this comment

sebmarkbage Aug 28, 2018

Choose a reason for hiding this comment

hermanventer commented Aug 23, 2018

sebmarkbage commented Aug 23, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

sebmarkbage commented Aug 28, 2018

Primary Mechanism

Mechanical Work

Regression

NTillmann Aug 28, 2018

Choose a reason for hiding this comment

NTillmann Aug 28, 2018

Choose a reason for hiding this comment

NTillmann Aug 28, 2018

Choose a reason for hiding this comment

NTillmann left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

sebmarkbage commented Aug 23, 2018 •

edited

Loading

sebmarkbage commented Aug 23, 2018 •

edited

Loading