Incorrect tests: createdynfn-html-close-comment-(body|params).js

[iamstolis]

test/annexB/built-ins/Function/createdynfn-html-close-comment-body.js and test/annexB/built-ins/Function/createdynfn-html-close-comment-params.js i.e. Function("-->"); and Function("-->", ""); are supposed to test HTML-like comments and their parsing is expected to pass. Unfortunately, they are incorrect because --> is not a valid comment. Note that HTMLCloseComment can be either part of SingleLineHTMLCloseComment or MultiLineComment. So it has to be preceeded by LineTerminatorSequence (to be SingleLineHTMLCloseComment) or by */ (to have a chance to be MultiLineComment). Hence, --> cannot be parsed as Comment and the mentioned tests should result in SyntaxError instead.

Side note: It seems that major JavaScript engines share my opinion regarding createdynfn-html-close-comment-params.js test, i.e., all of them fail it. The only reason that some major JavaScript engines pass createdynfn-html-close-comment-body.js test is IMHO the fact that they tend to parse Function("params","body") as function anonymous(params\n) {\nbody\n}, i.e., add an artificial line terminator before the body (which turns the body of the form --> into a valid comment effectively).

[leobalter]

Pinging @anba and @jugglinmike for help.

[rwaldron]

Edit: I'm revisiting the spec from 2017, because it occurred to me that https://tc39.github.io/ecma262/#sec-createdynamicfunction changed since then.

---

I think this one: https://github.com/tc39/test262/blob/master/test/annexB/built-ins/Function/createdynfn-html-close-comment-params.js might've been intended to be written as:

Function("", "-->");

The only reason that some major JavaScript engines pass createdynfn-html-close-comment-body.js test is IMHO the fact that they tend to parse Function("params","body") as function anonymous(params\n) {\nbody\n}, i.e., add an artificial line terminator before the body (which turns the body of the form --> into a valid comment effectively).

Specifically...

i.e., add an artificial line terminator before the body (which turns the body of the form --> into a valid comment effectively)

This was a really interesting thing to track down. I read the spec and wrote a response; then @leobalter and I read through that response, and the spec, realizing that

Let sourceText be the string-concatenation of prefix, " anonymous(", P, 0x000A (LINE FEED), ") {", 0x000A (LINE FEED), bodyText, 0x000A (LINE FEED), and "}".)

...occurs after the parse. So I dug deeper and found this: tc39/ecma262@a3b0507 which I understand to mean that yes, your interpretation of the spec is correct, but doesn't account for the ASI semantics that get applied once that Syntax Error is encountered.

[iamstolis]

That's correct, they do add an artificial line terminator, but that's because the spec says they must:
Step 41 of 19.2.1.1.1Runtime Semantics: CreateDynamicFunction:

Step 41 defines the value of [[SourceText]] internal slot only. It has nothing to do with parsing.

Parsing is handled by steps 7-29 (with steps 7, 17 and 18 being the most relevant for this issue).

[anba]

It seems to me both tests simply need a leading line-terminator before --> to be compliant to the current spec.

I no longer remember why I didn't add a line terminator, maybe I forgot it / misread the spec or I was working based on what most engines implement for html-comments, namely that no leading line-terminator is needed: eval("-->") works in JSC/V8/SM; it doesn't work in Chakra, but maybe that's because Chakra changed parsing for html-comments after ES6, whereas the other engines simply left their previously non-standard parsing of html-comments as it was. Compared to that, Function("-->", "") doesn't work in JSC/V8/SM/Chakra, which is the reason this test was added.

Edit: The Chakra change I referring to → chakra-core/ChakraCore#20

[iamstolis]

It seems to me both tests simply need a leading line-terminator before --> to be compliant to the current spec.

Yes, the replacement of "-->" by "\n-->" sounds like a good fix to me.

[leobalter]

I was still trying to make sense of all of this w/ @rwaldron and we have some guess over ASI... see #2095 (comment)

For the current test purposes the "\n-->" should also be fine.

[iamstolis]

I was still trying to make sense of all of this w/ @rwaldron and we have some guess over ASI... see #2095 (comment)

Could you, please, elaborate on that? I don't see any relation between parsing of --> and ASI.

[leobalter]

Could you, please, elaborate on that? I don't see any relation between parsing of --> and ASI.

It's a guess and I don't have much to offer other than saying that I'm still reading other parts of the specs to understand why this have happened.

Usually @anba's tests are safe from bugs and I challenge myself before I assume a bug is a bug. For that, I reconsider everything.

In any case, I'm pushing a fix with the "\n-->" as mentioned above, I'm not holding off this issue anymore.

[gnarf]

I would think that changing to \n--> is smart for the "should pass" intention of this test, but re-reading the spec specifically:

18. Let body be the result of parsing bodyText, interpreted as UTF-16 encoded Unicode text as described in 6.1.4, using goal as the goal symbol. Throw a SyntaxError exception if the parse fails.

As far as I can tell, every "goal" for parsing should interpret --> without a newline before it as SyntaxError: Unexpected token > right? Should we add a negative test for this?

Can anyone else think of a syntactical production that requires a newline before it other than --> It would be interesting to see if some of the engines parsing --> without a syntax error would fail on those also

[leobalter]

tc39/ecma262#1479