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

Normative: Remove ToUint32 from array literal evaluation #1124

Merged
merged 1 commit into from Oct 1, 2019

Conversation

@littledan
Copy link
Member

commented Mar 2, 2018

Previously, array literals (including spreads) had their indices and
lengths calculated with a ToUint32 operation. As Georg Neis noted
in #1095, such a calculation isn't quite right, as 2**32-1 is not
an array index. These semantics are unlikely to be observable in many
current JavaScript implementations due to resource limitations, but
the strange behavior may appear in future, resource-rich
implementations.

This patch specifies Georg Neis's suggestion for semantics: to write to
the length just in the case of elisions (regardless of where they are in
the Array literal), and don't bother to write to the length for writes for
ordinary elements (as Array indices will already lead to length updates).

As a bonus, the semantics for Array literals are singificantly simplified, involving
just one syntax-directed operation and pass rather than two. Note that, as before,
too-long Array literals throw only as a runtime exception, not as an early error.
This patch includes appropriate use of ? and ! in modified operations.

cc @allenwb @GeorgNeis @anba

Copy link
Contributor

left a comment

Looks good, thanks!

@anba

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2018

If ToUint32 is simply removed, the problem (unspecified behaviour for too large values when ToString is called) noted in #1095 (comment) will arise:

If you could reach it and the bogus ToUint32 that was inserted as a "fix" is removed then the property key that is set is currently unspecified because nextIndex + 1 will compute a mathematical value that is not a Number value and ToString doesn't specify how to convert such a value to a string value.

@littledan

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2018

@anba I see; I was imagining that these were Numbers, but of course they are mathematical values. I'd be fine waiting to land this patch until after the fix to tc39/proposal-bigint#10 lands, which will make them Numbers.

@littledan

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2018

@GeorgNeis and I discussed this change a bit further. Maybe the semantics here are a little off the mark--would it make more sense if we went with one version of @anba 's proposed semantics and used min(length, 2**32-1) when setting the length? This would give the final length that would result from the same series of property sets. The only exception we might want to make is if the array ends with several training commas which overflow--that could be considered equivalent to explicitly setting the length to a too-high value, and still throw. Any thoughts on these alternate semantics? If people like it, I can write this up into a new patch.

@littledan littledan force-pushed the littledan:array-accumulation branch from c584571 to 3bf3e39 Jul 10, 2018
littledan added a commit to littledan/ecma262 that referenced this pull request Jul 10, 2018
This patch follows @GeorgNeis's suggestion from
tc39#1124 (comment)

In particular, the idea is that if you have an array literal which
is longer than 2**32 elements, then the overflowing elements will
just be ordinary properties, without throwing an exception. However,
if the array has trailing holes, the semantics here correspond to an
explicit set of the length, and an exception will be thrown
@littledan

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

Uploaded a new version which attempts to specify the semantics from #1124 (comment) . @GeorgNeis What do you think?

spec.html Outdated
1. Perform Set(_array_, `"length"`, ToUint32(_pad_), *false*).
1. NOTE: The above Set cannot fail because of the nature of the object returned by ArrayCreate.
1. Perform ? Set(_array_, `"length"`, _pad_, *true*).
1. NOTE: The above Set may throw in the case of an Array literal's padding exceeds 2<sup>32</sup>-1 elements.

This comment has been minimized.

Copy link
@GeorgNeis

GeorgNeis Jul 10, 2018

Contributor

s/exceeds/exceeding/
or better s/may throw in the case of an Array literal's/throws if the/

@littledan

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

OK, @GeorgNeis had some more suggestions, which seem really on point to me. The result is a simpler and more obviously correct version, which is sightly different in semantics in even more obscure edge cases, but which I prefer. I've uploaded a commit from him to this PR, and edited the PR description to match the current proposal. If/when this PR lands, please squash the commits and use the description above as the commit message (minus the @mentions).

Given resource limits on all JS implementations Georg and I tested, we could not find a way to differentiate the current semantics from the semantics proposed in this patch (or the alternatives previously considered). I'd say, unfortunately, test262 tests will not be useful for this PR, and we should consider landing it without tests or implementations.

@littledan

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

Any thoughts on this PR? What more is needed to decide whether it's ready to merge?

@ljharb

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Given that it's normative, it seems like it needs consensus (along with a rebase)

@ljharb ljharb requested review from zenparsing, ljharb and tc39/ecma262-editors Apr 6, 2019
@ljharb ljharb added has consensus and removed needs consensus labels Oct 1, 2019
@ljharb
ljharb approved these changes Oct 1, 2019
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@littledan

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

@ljharb Feel free to land all of those suggestions. Somehow, the GitHub UI won't let me commit them.

@ljharb ljharb self-assigned this Oct 1, 2019
@ljharb ljharb removed the request for review from zenparsing Oct 1, 2019
ljharb added a commit to littledan/ecma262 that referenced this pull request Oct 1, 2019
Previously, array literals (including spreads) had their indices and
lengths calculated with a ToUint32 operation. As Georg Neis noted
in tc39#1095, such a calculation isn't quite right, as 2**32-1 is not
an array index. These semantics are unlikely to be observable in many
current JavaScript implementations due to resource limitations, but
the strange behavior may appear in future, resource-rich
implementations.

This patch takes Allen Wirfs-Brock's suggestion to simply remove the
ToUint32 wraparounds and write to larger indices. When the length
is written to, it will throw an exception, after having exhausted
any iterators in the literal. Note that too-long Array literals throw
only as a runtime exception, not as an early error.

In the theoretical case of array indices which exceed the safe numerical
integer range, the same semantics will apply--the iterator will be
exhausted, and then the write to the length will throw. Internally, the
same index will be written repeatedly.

This patch includes appropriate use of ? and ! in modified operations.
ljharb added a commit to littledan/ecma262 that referenced this pull request Oct 1, 2019
This patch follows @GeorgNeis's suggestion from
tc39#1124 (comment)

In particular, the idea is that if you have an array literal which
is longer than 2**32 elements, then the overflowing elements will
just be ordinary properties, without throwing an exception. However,
if the array has trailing holes, the semantics here correspond to an
explicit set of the length, and an exception will be thrown
ljharb added a commit to littledan/ecma262 that referenced this pull request Oct 1, 2019
@ljharb ljharb force-pushed the littledan:array-accumulation branch 2 times, most recently from c3e1d47 to bdcd531 Oct 1, 2019
Previously, array literals (including spreads) had their indices and
lengths calculated with a ToUint32 operation. As Georg Neis noted
in #1095, such a calculation isn't quite right, as 2**32-1 is not
an array index. These semantics are unlikely to be observable in many
current JavaScript implementations due to resource limitations, but
the strange behavior may appear in future, resource-rich
implementations.

This patch specifies Georg Neis's suggestion for semantics: to write to
the length just in the case of elisions (regardless of where they are in
the Array literal), and don't bother to write to the length for writes for
ordinary elements (as Array indices will already lead to length updates).

As a bonus, the semantics for Array literals are singificantly simplified, involving
just one syntax-directed operation and pass rather than two. Note that, as before,
too-long Array literals throw only as a runtime exception, not as an early error.
This patch includes appropriate use of ? and ! in modified operations.

Co-authored-by: Daniel Ehrenberg <littledan@chromium.org>
Co-authored-by: Georg Neis <neis@chromium.org>
@ljharb ljharb merged commit bdcd531 into tc39:master Oct 1, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
netlify/ecma262-snapshots/deploy-preview Deploy preview processing.
Details
@jmdyck jmdyck referenced this pull request Oct 2, 2019
ljharb added a commit to jmdyck/ecma262 that referenced this pull request Oct 2, 2019
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 5, 2019
PR tc39#1124 removed the element-id 'sec-static-semantics-elisionwidth'.
Reinstate it as an 'oldid'.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 5, 2019
PR tc39#1124 removed the element-id 'sec-static-semantics-elisionwidth'.
Reinstate it as an 'oldid'.
ljharb added a commit to jmdyck/ecma262 that referenced this pull request Oct 6, 2019
 - Add a subscript R: Since _intValue_ is to be treated as a mathematical value, we should be comparing it to a mathematical zero.
 - Reinstate 'sec-static-semantics-elisionwidth' as an oldid; tc39#1124 removed the element-id 'sec-static-semantics-elisionwidth'
 - Reinstate 'sec-synchronizeeventset' as an oldid; tc39#1692 removed the element-id 'sec-synchronizeeventset'.
 - Reference 'ExportFromClause' from Annex A; tc39#1174 introduced the nonterminal 'ExportFromClause', but didn't reference it from Annex A.
 - Put asterisks around 'true' (from tc39#1716)
 - Fix typo "_eventRecords_" -> "_eventsRecord_" (from tc39#1692)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.