[webkit-reviews] review granted: [Bug 190501] [JSC] Change Function constructor's source to match to the spec and test262 : [Attachment 352112] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 190501: [JSC] Change Function constructor's source to match to the spec and
test262
https://bugs.webkit.org/show_bug.cgi?id=190501

Attachment 352112: Patch

https://bugs.webkit.org/attachment.cgi?id=352112&action=review




--- Comment #15 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 352112
  --> https://bugs.webkit.org/attachment.cgi?id=352112
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352112&action=review

r=me if you fix the remaining EWS bot failures (I think at least some of them
may be valid).

> Source/JavaScriptCore/ChangeLog:10
> +	   We should have a line terminator after the function parameters to
parse the parameters endings with `//`.
> +	   We also remove a space after `,`. This is tested in test262 and our
stress tests. We also note that this
> +	   behavior is the same to V8.

This is not quite accurate (see my other comment below).  I suggest saying
something like this instead:

"We should insert a line terminator after the last function parameter so that a
parameter list like "a,b,c//" will not fail to parse.  The spec does not forbid
the insertion of a line terminator before the close paren, and this is how
Chrome and FF behaves.	The spec does forbid the insertion of line terminators
after any other parameters except for the last because it explicit states that
the parameters shall be separated only by a single ",".  Hence, we should also
remove the space after the comma.  This is tested in test262 and our stress
tests."

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:132
> +	   // To meet this condition, what we can do is putting a line
terminator after the parameter text P for single line comment (//) to work
correctly.

/putting/put/.

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:139
> +	   builder.appendLiteral("\n) {\n");

There's something very hacky about this solution.  Here's why:

1. I don't see anything in the spec that says that a newline is expected just
before the ')'.

2. While this matches Chrome and FireFox behavior, both Chrome and Firefox
aren't handling this that much better.	Consider the following cases:

    Case 1:
    var f = new Function("a", "b", "c//", "return a + b + c"); // Parses
successfully on Chrome and Firefox.

    f.toString(); // prints 
    // function anonymous(a,b,c//
    // ) {
    // return a + b + c
    // }"

    f(1, 2, 3); // Executes correctly on Chrome and Firefox, and returns 6.

    Case 2:
    var g = new Function("a", "b//", "c", "return a + b + c"); // Parses
successfully on Chrome and Firefox.

    g.toString(); // prints 
    // function anonymous(a,b//,c
    // ) {
    // return a + b + c
    // }"

    g(1, 2, 3); // Chrome fails with: Uncaught ReferenceError: c is not defined
at eval (eval at <anonymous> (newtab?ie=UTF-8:1), <anonymous>:3:16)
		     // Firefox fails with: ReferenceError: c is not defined

Why should the Function constructor be allowed to take a // comment in the last
argument, but not any of the preceding ones?  It feels very inconsistent. 
After reading the spec, I think I know why they do this:
    19.2.1.1.1 - 15.d.ii specifies that "nextArgString be ? ToString(nextArg)",
and ToString here has specific semantics where the whole string of nextArg is
returned (in the bad case, includes the // comment).
    19.2.1.1.1 - 15.d.iii specifies that"Set P to the string-concatenation of
the previous value of P, "," (a comma), and nextArgString." which means we
don't have the leeway to insert a newLine between arguments.

Hence, the only place where we can append a newline is after the last argument
since that part is not specified.  Based on all these, I agree that this
solution is correct, though ugly. 

I wish we can append that newLine only if needed e.g.

     x = new Function(); // Don't make this one look ugly: toString() renders
"function anonymous()  { }"
     y = new Function("a//", ""); // toString() renders the ugly version with
the appended newline, or one with comments removed: "function anonymous(a)  {
}"

However, the downside of a pretty toString() render is that the web may already
have made assumptions based on Chrome/FF behavior even though it's supposed to
be implementation specific.  It's better for web compatibility to behave
similarly.  Plus your implementation is simpler (less chance of bugs creeping
in).

So, r=me.