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

Fix incorrect tests for CreateDynamicFunction #4013

Closed
wants to merge 1 commit into from

Conversation

jedel1043
Copy link
Contributor

PR tc39/ecma262#3222 changed the evaluation order for ToString(bodyArg) and ToString(arg), making it so that the body is converted to a string before the args, which differs from the previous behaviour.

For reference:

CreateDynamicFunction (ES draft)

...
7. Let bodyString be ? ToString(bodyArg).
8. Let parameterStrings be a new empty List.
9. For each element arg of parameterArgs, do
    a. Append ? ToString(arg) to parameterStrings.
...

CreateDynamicFunction (ES 2023)

...
10. If argCount > 0, then
    a. Let firstArg be parameterArgs[0].
    b. Set P to ? ToString(firstArg).
    ...
    d. Repeat, while k < argCount,
       i. Let nextArg be parameterArgs[k].
       ii. Let nextArgString be ? ToString(nextArg).
       ...
11. Let bodyString be the string-concatenation of 0x000A (LINE FEED), ? ToString(bodyArg), and 0x000A (LINE FEED).
...

@nicolo-ribaudo
Copy link
Member

Hold on, that ordering change might have not been intentional. @ptomato Do you remember if we talked about it?

@ptomato
Copy link
Contributor

ptomato commented Feb 22, 2024

Hold on, that ordering change might have not been intentional. @ptomato Do you remember if we talked about it?

There were so many revisions requested, I'm actually not sure anymore. Looking through the history, it looks like I made that change in response to tc39/ecma262#3222 (comment), but I can't remember if there was a conversation outside of GitHub.

@jedel1043
Copy link
Contributor Author

jedel1043 commented Feb 27, 2024

Closing in favour of tc39/ecma262/pull/3288

@jedel1043 jedel1043 closed this Feb 27, 2024
@jedel1043 jedel1043 deleted the fix-create-dyn-fn-tests branch February 27, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants