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: Assert that Evaluation in JSON.parse does not throw #3394

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

nicolo-ribaudo
Copy link
Member

Closes #3392

When I opened that issue I was wrongly assuming that array literals longer than 2**32-1 would throw an error when evaluated. This is only true if the array contains holes in one of those too big indexes, which are not valid JSON syntax. What happens in the non-hole case is that the integer-indexed property gets defined without updating the array length, so it does not throw an error.

@michaelficarra and I went through the possible cases and we are confident this indeed never throws, so having ! instead of Completion(...) can help when reading the spec. If there were cases where it can throw, the current usage of Completion(...) is probably wrong anyway because you would want to re-throw the error (?), instead of using it as _unfiltered_.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM unless someone can find a way this Evaluation can actually throw.

And if it can, we'd probably want to change our handling of the throw completion anyway.

@jmdyck
Copy link
Collaborator

jmdyck commented Aug 14, 2024

What happens in the non-hole case is that the integer-indexed property gets defined without updating the array length, so it does not throw an error.

So if the ArrayLiteral has more than 2^32-1 elements, the ones after the first 2^32-1 are evaluated, but ignored, is that it?

@michaelficarra
Copy link
Member

@jmdyck Not exactly ignored. They still create properties but they don't attempt to update the length. The exception is holes, which do attempt to update the length and then throw.

@jmdyck
Copy link
Collaborator

jmdyck commented Aug 14, 2024

They still create properties but they don't attempt to update the length.

Ah, I see now.

@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Nov 11, 2024
@ljharb ljharb changed the title Assert that Evaluation in JSON.parse does not throw Editorial: Assert that Evaluation in JSON.parse does not throw Nov 15, 2024
@ljharb ljharb merged commit 6adf64a into tc39:main Nov 15, 2024
8 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the assert-json-parse-no-error branch November 15, 2024 15:14
kimjg1119 pushed a commit to kimjg1119/ecma262 that referenced this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON.parse returns evaluation errors instead of throwing them
6 participants