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: CreateDynamicFunction early concatenates bodyText #1479

Merged
merged 1 commit into from
Nov 13, 2019

Conversation

leobalter
Copy link
Member

@leobalter leobalter commented Mar 13, 2019

Ref tc39/test262#2102
Ref tc39/test262#2109

This normative change matches the current behavior of V8, SpiderMonkey, and JSC (e.g. Function("-->"). This is not only for the convenience of reflecting the observed behaviors, but this will allow some guard for potential false positives like Function("#!hashbang").

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 web reality labels Mar 14, 2019
@ljharb ljharb requested review from zenparsing, ljharb and a team March 14, 2019 01:00
@anba
Copy link
Contributor

anba commented Mar 14, 2019

but this will allow some guard for potential false positives like Function("#!hashbang").

I don't understand this part, hashbang is only allowed when parsing with the goal symbol Script or Module, so it never applies in CreateDynamicFunction.

@leobalter
Copy link
Member Author

@anba right you are. Maybe the only advantage is the convenience for reflecting engines behavior in a very specific usage.

@leobalter
Copy link
Member Author

@ljharb the current tests we have on Test262 matches this proposed behavior. If tc39/test262#2101 tc39/test262#2102 get merged those will invert.

If we have those PRs on hold until the TC39 meeting the tag for need tests could be discarded.


Just to inform, I just want a closing answer for this problem. One way or another, I'm fine with any decision TC39 chooses for this PR.

@ljharb ljharb added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Mar 14, 2019
@leobalter
Copy link
Member Author

tc39/test262#2101 is now merged, that means I need a new PR to now reflect this change.

@leobalter leobalter added needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 and removed has test262 tests labels Mar 14, 2019
@ljharb ljharb self-assigned this Mar 26, 2019
leobalter added a commit to leobalter/test262 that referenced this pull request Mar 26, 2019
leobalter added a commit to leobalter/test262 that referenced this pull request Mar 26, 2019
@leobalter
Copy link
Member Author

leobalter commented Mar 27, 2019

Doing more investigation over this issue I found some similar cases:

<script>-->
console.log('foo');</script>

Chrome, Firefox, Safari: will log `'foo'`.
Edge: SyntaxError

Anyway a file starting with --> will have the observed SyntaxError properly on v8, ChakraCore, SpiderMonkey, JSC, and Node.

The same issue is also observed with eval:

$ eshost -se 'eval("-->")'
#### d8, jsc, jsshell, node
undefined

#### ch
SyntaxError: Syntax error

The error is similar to what happens to Function("-->"), but I'm not convinced the fix should happen at the same place for whatever happens to Function (CreateDynamicFunction), eval (PerformEval), or the script tag (not ecma262).

@leobalter
Copy link
Member Author

There is an interesting observation for the actual output of the Function ctor:

eshost -se 'Function(42).toString()'
#### ch
function anonymous(
) {42
}

#### d8, jsshell, node
function anonymous(
) {
42
}

#### jsc
function anonymous() {
42
}

That means that ChakraCore is also not compliant to CreateDynamicFunction when the SourceText is not correctly set, it misses the preceding line feed. I wonder if fixing this part would have the same issue observed for HTMLCloseComments.

A function like this will be a SyntaxError anywhere:

function fn() {-->
}

but this function formatting is not:

function fn() {
--> valid code
}

And this matches the revealed code from Function("-->"):

$ eshost -se 'Function("-->").toString()'
#### d8, jsshell, node
function anonymous(
) {
-->
}

#### ch
SyntaxError: Syntax error

#### jsc
function anonymous() {
-->
}

As seen in the examples, it makes sense to observe the function body in conjunction with the padding of line feeds added at CreateDynamicFunction.

I'm ok to observe the string: \n-->\n can be correctly parsed.

@leobalter
Copy link
Member Author

My conclusion is to stick with the proposed change and discuss the problems with eval elsewhere. These are another problem I don't have time and availability to have immediately fixed.

Fixing the Function ctor in the specs will not preclude anything else to be fixed.

@bakkot bakkot added the editor call to be discussed in the next editor call label Nov 6, 2019
@ljharb ljharb added has consensus This has committee consensus. has test262 tests and removed editor call to be discussed in the next editor call needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Nov 13, 2019
@ljharb
Copy link
Member

ljharb commented Nov 13, 2019

Discussed on the editor call; this is ready to go.

@ljharb ljharb merged commit abb7cbd into tc39:master Nov 13, 2019
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Nov 14, 2019
The change from backticks to asterisks occurred a few weeks ago via PR tc39#1733.
Looks like the merge of PR tc39#1479 accidentally reverted it for this one line.
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Nov 14, 2019
…als" (tc39#1778)

 - "Set X to be Y" -> "Set X to Y"
 - The change from backticks to asterisks occurred a few weeks ago via PR tc39#1733. Looks like the merge of PR tc39#1479 accidentally reverted it for this one line.
@leobalter leobalter deleted the dynamic-function-wrappers branch June 19, 2020 21:53
leobalter added a commit to tc39/test262 that referenced this pull request Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants