Fixes for 'steps for storing'

[inexorabletash]

@brettz9 - could you do an initial review?

This covers the issues from #144 (apart from the > MAX_SAFE_INTEGER/keygen ones)

[brettz9]

It all looks go to me, with the exception that I'm unclear on the following...

It seems to me that 3. If |hop| is false, return true. should instead be:

>3. If |hop| is false, run these substeps
>    1. Let o be a new Object created as if by the expression ({}).
>    2. Let status be CreateDataProperty(value, identifier, o).
>    3. Assert: status is true.

...because the whole algorithm ought to continue on to keep checking the remaining identifiers. However, I guess you will need to add a step to make a clone of this clone to avoid the checking having the inadvertent side effect of adding objects before the actual injection steps.

[inexorabletash]

...because the whole algorithm ought to continue on to keep checking the remaining identifiers.

If hop is false it has no property; the corresponding injection steps will add a blank object as necessary for the remaining (non-last) identifiers. That blank object will have no own-properties so therefore it will (inductively) be okay to add more; there's no need to actually do the creation here.

[brettz9]

I see, then LGTM.

Btw, one bit of personal info I might share in case it may be useful in future collaborations... I suffer from CFS with attendant brain fog, so although I have the underlying ability to think these kinds of things through, I can get a kind of tunnel vision (beyond that we all experience). I try to compensate by testing and revisiting as well as generally trying hard to think something through before posting, but it may sometimes come out seeming like I'm just being careless or lazy. I just mention it in case it may minimize any frustration in communications (and hopefully any other contributions I can make will compensate).

[inexorabletash]

@brettz9 - having a diversity of skills, interests, abilities, backgrounds, and communication and working styles is incredibly valuable to producing the best possible specs. You've noticed several issues that others missed. Your contributions are very much appreciated!

[inexorabletash]

@aliams - can you review these changes?

[brettz9]

Realized I have a few more questions that may be somewhat interconnected if you want them here or before completing the PR:

1. Should the second step of "convert a value to a multiEntry key", namely:

Otherwise, return the result of running the steps to convert a value to a key with argument input.

become:

Otherwise, return the result of running the steps to convert a value to a key with argument input. Rethrow any exceptions.

...or is the rethrowing somehow assumed with "return"?

2. For "extract a key from a value using a key path", should the second step, namely:

If r is failure, return failure.

be followed by:

If r is invalid, return invalid.

(or an assertion if this is not possible as one might think could occur given that "evaluate a key path on a value" recurses back to these steps which can return "invalid")?

3. As with Define key extraction/injection using [[DefineOwnProperty]] and [[Get]] #2, should "evaluate a key path on a value", should step 1.3.1, namely:

Let key be the result of recursively running the steps to extract a key from a value using a key path using item as keyPath and value as value.

...be followed by a condition about what to do if it is valid (or assertion that it should not be possible as one might expect given that "extract a key from a value using a key path" can return "invalid")

[brettz9]

And though for a different algorithm, one other little spec issue:

Let octets be the result of running the steps to get a copy of the bytes held by the buffer source value.

ought to be:

Let octets be the result of running the steps to get a copy of the bytes held by the buffer source input.

[inexorabletash]

1. Should the second step of "convert a value to a multiEntry key"....

Fixed.

2. For "extract a key from a value using a key path" ...

3. As with Define key extraction/injection using [[DefineOwnProperty]] and [[Get]] #2, should "evaluate a key path on a value", should step 1.3.1...

That should recurse to "evaluate" directly, not "extract". Fixed.

held by the buffer source input.

Fixed.

[brettz9]

Excellent, thanks! LGTM...

[aliams]

I remember we explicitly wanted to throw a DOMException in this case. Does asserting give us the same result here?

[brettz9]

See the comment below that

Assertions can be made in the above steps because this algorithm is
only applied to values that are the output of StructuredClone,
and the steps to [=check that a key could be injected into a value=] have
been run.

The latter steps to "check that a key could be injected into a value" were added (prior to these steps) so that throwing would occur synchronously as opposed to within these record storing steps you have highlighted where they would be occurring asynchronously with the request (and where throwing would not make sense).

[inexorabletash]

Right. "Assert" is just a guide to implementers to help with the readability of algorithms. Throwing required breaking out the steps separately, as @brettz9 clarifies.

[aliams]

Thank you @brettz9 and @inexorabletash for clarifying!

[aliams]

Looking this over, nothing sticks out to me as a blocker to merging.