Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: Add note on [[Delete]] in InternalizeJSONProperty #1615

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@shvaikalesh
Copy link
Contributor

commented Jul 8, 2019

No description provided.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Can you provide more context on why you feel this is an improvement?

@@ -36386,7 +36389,6 @@ <h1>Runtime Semantics: InternalizeJSONProperty ( _holder_, _name_ )</h1>
1. Perform ? _val_.[[Delete]](! ToString(_I_)).
1. Else,
1. Perform ? CreateDataProperty(_val_, ! ToString(_I_), _newElement_).
1. NOTE: This algorithm intentionally does not throw an exception if CreateDataProperty returns *false*.

This comment has been minimized.

Copy link
@shvaikalesh

shvaikalesh Jul 10, 2019

Author Contributor

This algorithm ignores failures of both CreateDataProperty and [[Delete]], but has notes only about ignoring CreateDataProperty, which is a bit confusing.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 10, 2019

Member

Do other places where [[Delete]] return false throw an exception?

This comment has been minimized.

Copy link
@shvaikalesh

shvaikalesh Jul 10, 2019

Author Contributor

Yes, two: delete operator (step 5.b) and DeletePropertyOrThrow (step 4).

Handling false results of [[Delete]] and [[DefineOwnProperty]] looks very straightforward in spec with DeletePropertyOrThrow and DefinePropertyOrThrow respectively.

However, some engines (JSC at least) don't have those ops and pass around a "should throw" flag as an argument. The note would be nice, IMO.

@ljharb

ljharb approved these changes Jul 10, 2019

@ljharb ljharb requested review from zenparsing and tc39/ecma262-editors Jul 10, 2019

@raulsebastianmihaila

This comment has been minimized.

Copy link

commented Jul 10, 2019

I can't think of a case where CreateDataProperty would return false inside InternalizeJSONProperty because the value is always either an ordinary object or an array that hasn't been touched before.

@shvaikalesh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@raulsebastianmihaila Normally, yes, but consider the following case:

JSON.parse('{"a":1, "b":2}', function(key, value) {
  if (key == "a") Object.freeze(this)
  if (key == "b") return 3
  return value
})

// => {a:1, b:2} // not 3

Related JSC issue (with some more test cases in patch):

@raulsebastianmihaila

This comment has been minimized.

Copy link

commented Jul 10, 2019

@shvaikalesh Thanks! I started thinking about a similar scenario but then I got confused by the variable names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.