Closed Bug 746262 Opened 8 years ago Closed 6 years ago

Allow PropDesc to represent non-existent properties

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(1 file)

In order to use PropDesc in a reformed, more ES5-like internal meta-object protocol, we need it to be able to represent the absence of a property.  Add another bit to it to represent undefinedness, and set/unset it appropriately in all the right places.  This applies atop the patch for bug 745944.
Attachment #615808 - Flags: review?(jorendorff)
Comment on attachment 615808 [details] [diff] [review]
Add PropDesc::isUndefined()

Review of attachment 615808 [details] [diff] [review]:
-----------------------------------------------------------------

This sure is a lot of assertions. Could we instead use a Maybe<PropDesc> or a PropDesc * in those specific places where the PropDesc is optional? Just a thought.

::: js/src/vm/ObjectImpl.h
@@ +165,1 @@
>          return get_;

For all of these, only the "has" assertion is really necessary, since hasGet() and each of its friends asserts !isUndefined(). ...But it's clearer this way, so whatever you prefer is fine with me.
Attachment #615808 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> This sure is a lot of assertions. Could we instead use a Maybe<PropDesc> or
> a PropDesc * in those specific places where the PropDesc is optional?

I had considered PropDesc*.  The issue there is that it seems to me you need some sort of dynamic allocation of the property descriptor struct to do it, which just seems all sort of off-base as a performance matter.  (Yeah yeah, premature optimizations and all.  But I think that cliche was always premised on people not making obvious missteps along the way -- you can't just *completely* abdicate responsibility for perf until everything's implemented.)

I hadn't considered Maybe<PropDesc>.  I might even have agreed with it, once.  But in hacking on subsequent patches I've realized this isn't really the right thing.  Consider these proposed steps in the proto-climbing-refactoring proposal:

  1. Let ownDesc be the result of calling the [[GetOwnProperty]] internal
     method of O with argument P.
  2. If IsDataDescriptor(ownDesc) is true, then
     a. If ownDesc.[[Writable]] is false, return false.
  3. If IsAccessorDescriptor(ownDesc) is true, then
     a. Let setter be ownDesc.[[Set]]
     b. If setter is undefined, return false.
     c. Call the [[Call]] internal method of setter providing Receiver as
        the this value and providing V as the sole argument.
     d. Return true.

How's this work, if [[GetOwnProperty]] returns undefined?  IsDataDescriptor and IsAccessorDescriptor have "1. If Desc is undefined, then return false." as their first step.  So, basically, the spec algorithms don't clearly bifurcate non-existent properties from extant ones.  Given this, it seems better to conflate undefined-ness into PropDesc (and mirror IsDataDescriptor and IsAccessorDescriptor), than to keep them separate.  Perhaps it would have been better if those helpers (which were first defined and used in ES5) and users of them had bifurcated.  But it didn't happen, so I think it's probably better to do it this way.

I'm going to sit on both these patches until after the merge, given that 1) this stuff introduces all kinds of assertions and 2) getting approval seems a pain even if this were obviously safe.
Push backed out for win debug bustage, which persisted even after the original partial backout:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=718ced982de1

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb6904fa2cf
Target Milestone: mozilla15 → ---
https://hg.mozilla.org/mozilla-central/rev/0379525bbdca
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Backed out: https://hg.mozilla.org/mozilla-central/rev/cdb6904fa2cf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/a490daf28b93
Status: REOPENED → RESOLVED
Closed: 8 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.