tc39 / proposal-record-tuple

ECMAScript proposal for the Record and Tuple value types. | Stage 2: it will change!
https://tc39.es/proposal-record-tuple/
2.48k stars 62 forks source link

fix record-exotic-objects-defineownproperty #343

Closed acutmore closed 2 years ago

acutmore commented 2 years ago

Fixes #342

anba commented 2 years ago

[[DefineOwnProperty]] can probably be simplified to:

1. Assert: IsPropertyKey(P) is true.
2. Let current be R.[[GetOwnProperty]](P).
3. Return IsCompatiblePropertyDescriptor ( false, Desc, current ).

But please double-check I didn't miss some corner cases.

ljharb commented 2 years ago

I'd probably keep the early returns for clarity - something like:

1. Assert: IsPropertyKey(_P_) is true. (this line won't be necessary once the AO is using typed headers)
1. If Type(_P_) is not String, return *false*.
1. If IsDataDescriptor(_Desc_) is *false*, return *false*.
1. If RecordHasProperty(_P_, _R_) is *false*, return *false*.
1. Let _current_ be _R_.[[GetOwnProperty]](_P_).
1. Return IsCompatiblePropertyDescriptor (*false*, _Desc_, _current_).
anba commented 2 years ago

1. If IsDataDescriptor(_Desc_) is *false*, return *false*.

Needs to be: IsAccessorDescriptor(_Desc_) is *true*, return *false*.

to correctly handle generic descriptors.

ljharb commented 2 years ago

ah, fair

acutmore commented 2 years ago

Thanks for the further suggestions @anba and @ljharb , as this fix is currently a one liner. I'll merge this PR for now. And do the refactoring as a separate PR.