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

Should %TypedArray%.prototype.slice throw an error if Set throws? #1055

Open
JakeChampion opened this Issue Dec 27, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@JakeChampion

JakeChampion commented Dec 27, 2017

Section 22.2.3.24, step 14.b.iii performs a Set whilst omitting the fourth argument, which is used to indicate whether Set should ignore any errors that may have been thrown whilst performing the set.

This is the only place in the specification that calls Set without the fourth argument, which I think is incorrect as the fourth argument is not marked as an optional argument. This is also the only call to Set from Section 18 and onwards that is not passing true as the fourth argument to Set.

I would like to finish this issue off by saying that it is fantastic to have the specification and issue tracker on GitHub, thank you for making it simple for the community to get involved.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Dec 27, 2017

Member

This sounds like a clear spec bug; step 3 in https://tc39.github.io/ecma262/#sec-set-o-p-v-throw is an assertion that the fourth argument is a boolean.

As to whether it should be true or false, because of the !, it theoretically can't throw - which would either mean 1) that its value doesn't matter, and it probably should be "true", or 2) it shouldn't throw even if it otherwise would, meaning it should be "false".

In this case, since A is created by TypedArraySpeciesCreate, and the species constructor could do anything it likes, including installing throwing setters, step 14.b.iii could, indeed, throw (when "Throw" is set to "true").

What do existing engines do in this scenario?

Member

ljharb commented Dec 27, 2017

This sounds like a clear spec bug; step 3 in https://tc39.github.io/ecma262/#sec-set-o-p-v-throw is an assertion that the fourth argument is a boolean.

As to whether it should be true or false, because of the !, it theoretically can't throw - which would either mean 1) that its value doesn't matter, and it probably should be "true", or 2) it shouldn't throw even if it otherwise would, meaning it should be "false".

In this case, since A is created by TypedArraySpeciesCreate, and the species constructor could do anything it likes, including installing throwing setters, step 14.b.iii could, indeed, throw (when "Throw" is set to "true").

What do existing engines do in this scenario?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 27, 2017

Member

@JakeChampion Thank you so much for your first bug report! I agree with you that true would be the right parameter ot pass. The next step from here would be to make a PR to the spec to correct the bug. Do you have enough information to do this? If not, feel free to ask on this issue or in the #tc39 IRC channel on Freenode.

@ljharb I don't see how Set can throw here. TypedArraySpeciesCreate can only output a TypedArray--see step 2 of TypedArrayCreate which validates that it is a TypedArray. No casts between TypedArray element types can throw, and writing to an integer-indexed element of a TypedArray will never hit setters, so I think you're safe.

Member

littledan commented Dec 27, 2017

@JakeChampion Thank you so much for your first bug report! I agree with you that true would be the right parameter ot pass. The next step from here would be to make a PR to the spec to correct the bug. Do you have enough information to do this? If not, feel free to ask on this issue or in the #tc39 IRC channel on Freenode.

@ljharb I don't see how Set can throw here. TypedArraySpeciesCreate can only output a TypedArray--see step 2 of TypedArrayCreate which validates that it is a TypedArray. No casts between TypedArray element types can throw, and writing to an integer-indexed element of a TypedArray will never hit setters, so I think you're safe.

@jmdyck

This comment has been minimized.

Show comment
Hide comment
@jmdyck

jmdyck Dec 27, 2017

Collaborator

See PR #553 in which @leobalter asserts that this call to Set() cannot throw. The commit changed '?' to '!' (which seems correct), but also removed the fourth argument (which is incorrect).

Collaborator

jmdyck commented Dec 27, 2017

See PR #553 in which @leobalter asserts that this call to Set() cannot throw. The commit changed '?' to '!' (which seems correct), but also removed the fourth argument (which is incorrect).

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Dec 27, 2017

Member

@littledan if i make a subclass of a typed array that has a setter (own or prototype) on an index that throws?

If indeed it’s not possible, then it should probably pass “false”.

Member

ljharb commented Dec 27, 2017

@littledan if i make a subclass of a typed array that has a setter (own or prototype) on an index that throws?

If indeed it’s not possible, then it should probably pass “false”.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 27, 2017

Member

@jmdyck Thanks for the context! This seems pretty straightforward then.

@ljharb A subclass of an integer indexed exotic object can't make accesses to integer indexes start throwing.

Member

littledan commented Dec 27, 2017

@jmdyck Thanks for the context! This seems pretty straightforward then.

@ljharb A subclass of an integer indexed exotic object can't make accesses to integer indexes start throwing.

@JakeChampion

This comment has been minimized.

Show comment
Hide comment
@JakeChampion

JakeChampion Jan 3, 2018

In conclusion, it doesn't matter what the argument value actually is, because it cannot throw due to Perform !?

JakeChampion commented Jan 3, 2018

In conclusion, it doesn't matter what the argument value actually is, because it cannot throw due to Perform !?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 3, 2018

Member

Sounds right to me - false matches what te behavior will be; true would help ensure the ! assertion is tested. I have a mild preference for the latter.

Member

ljharb commented Jan 3, 2018

Sounds right to me - false matches what te behavior will be; true would help ensure the ! assertion is tested. I have a mild preference for the latter.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Jan 3, 2018

Member

@JakeChampion Perform ! doesn't suppress exceptions--instead, it's an spec-level assertion that no expression is being thrown. If an expression is thrown, then it's an error in the specification itself.

Member

littledan commented Jan 3, 2018

@JakeChampion Perform ! doesn't suppress exceptions--instead, it's an spec-level assertion that no expression is being thrown. If an expression is thrown, then it's an error in the specification itself.

@JakeChampion

This comment has been minimized.

Show comment
Hide comment
@JakeChampion

JakeChampion Jan 3, 2018

@littledan I have learnt even more about the specification today, I was not aware Perform ! was at the spec-level and not at the implementation level. This means that the argument has to be false then. Is there any automated testing that verifies these spec-level assertions are valid?

JakeChampion commented Jan 3, 2018

@littledan I have learnt even more about the specification today, I was not aware Perform ! was at the spec-level and not at the implementation level. This means that the argument has to be false then. Is there any automated testing that verifies these spec-level assertions are valid?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Jan 3, 2018

Member

@JakeChampion The argument doesn't have to be false--the argument can be true and this just makes the assertion stronger--for reasons that have to do with the bigger context, this particular callsite will never throw.

Member

littledan commented Jan 3, 2018

@JakeChampion The argument doesn't have to be false--the argument can be true and this just makes the assertion stronger--for reasons that have to do with the bigger context, this particular callsite will never throw.

@jmdyck

This comment has been minimized.

Show comment
Hide comment
@jmdyck

jmdyck Jan 3, 2018

Collaborator

Is there any automated testing that verifies these spec-level assertions are valid?

I suspect it's either impossible or difficult to automatically verify all spec-level assertions. Some are amenable to static verification, particularly those that assert the type of value bound to a metavariable.

Collaborator

jmdyck commented Jan 3, 2018

Is there any automated testing that verifies these spec-level assertions are valid?

I suspect it's either impossible or difficult to automatically verify all spec-level assertions. Some are amenable to static verification, particularly those that assert the type of value bound to a metavariable.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 3, 2018

Member

@JakeChampion one common thing i've seen in some browser implementations is to actually encode the asserts - that way they are being exercised whenever the feature is tested (not in production, obv).

Member

ljharb commented Jan 3, 2018

@JakeChampion one common thing i've seen in some browser implementations is to actually encode the asserts - that way they are being exercised whenever the feature is tested (not in production, obv).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment