Add a parameter to ObjectOps::defineProperty and ObjectOps::setProperty to return success/failure of the definition/setting (distinct from JSAPI's understanding of success via return value)

RESOLVED DUPLICATE of bug 1113369

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 1113369
5 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
[[DefineOwnProperty]] in the spec takes a property name P, a descriptor Desc, and a boolean Throw that indicates whether a define failure should throw or not.

We don't have anything like the Throw parameter in our property-definition API.  We need to add it to evolve more in the direction of the spec APIs.  This also would apparently help out the WebIDL people, since they're generating proxy hooks that are working around this lack in bizarre ways now (like, by using the |bool set| passed to the getOwnPropertyDescriptor hook, which is just crazy).

Note that Throw is used to implement both strict mode behavior and making Object.defineProperty(...) throw when it should.  So the new parameter shouldn't be named "strict" or something like that.  And of course it should be an enum so that you see Throw/DontThrow at call sites, or something.
(Assignee)

Comment 1

5 years ago
Created attachment 700070 [details] [diff] [review]
WIP, doesn't compile at all

Right now I'm making the extra parameter be of type ErrorBehavior, an enum with Throw/DontThrow as its values.  In the spec the parameter is a boolean named Throw, because it's used by more than just strict mode (all the [[DefineOwnProperty]] calls in the array methods, for example, specify Throw=true), so it makes sense to do the same here.

It turns out, if you want to add this to defineProperty, you really want to add it to setProperty as well at the same time, because one more or less flows into the other.

Ms2ger pointed me at a similar bug for adding this to the deleteProperty hook as well, but that's clearly a separable concern.  It also might be less important than this bit of work, as far as the property/element splitting is concerned.
(Assignee)

Comment 2

5 years ago
So after the previous patchwork, I had the bright idea to look at ES6, and it changes [[DefineOwnProperty]] and [[SetP]] (roughly equivalent to setProperty) to not take a Throw/strict mode parameter, and to return their success or failure.  Callers then handle success/failure as desired.  So I'm morphing this to do that -- should cut down on complexity slightly, too.  New patchwork for that coming soonish, hopefully.
Summary: Add a parameter to ObjectOps::defineProperty indicating whether to throw on failure → Add a parameter to ObjectOps::defineProperty and ObjectOps::setProperty to return success/failure of the definition/setting (distinct from JSAPI's understanding of success via return value)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 910261
Will resolving this bug make sure `Object.defineProperty(new Proxy(Object.defineProperty({}, "x", {value:0}), {}), "x", {value:1})` throws a TypeError? Asking here to avoid creating just another duplicate bug report...

Comment 5

5 years ago
(In reply to André Bargull from comment #4)
> Will resolving this bug make sure `Object.defineProperty(new
> Proxy(Object.defineProperty({}, "x", {value:0}), {}), "x", {value:1})`
> throws a TypeError? Asking here to avoid creating just another duplicate bug
> report...

When in doubt, create a new bug (that depends on this one or adds it as "see also"), otherwise (if it's not a dupe), it may get lost.
I'm patching bug 1113369 this week. I think this basically is a dupe of that; closing.

In short, ES6 re-implemented the Throw argument as a boolean return value (wince).
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Depends on: 1113369
Resolution: --- → DUPLICATE
Duplicate of bug: 1113369
You need to log in before you can comment on or make changes to this bug.