tc39 / Function-prototype-toString-revision

:fishing_pole_and_fish: ECMA-262 proposal to update Function.prototype.toString
https://tc39.github.io/Function-prototype-toString-revision
26 stars 10 forks source link

toString result in CreateDynamicFunction doesn't match to existing implementations #20

Closed arai-a closed 7 years ago

arai-a commented 7 years ago

With the proposal, Function("a", "b", "c").toString() generates the following string:

function anonymous(a,b
) {c
}

but it doesn't match to existing implementations.

Firefox

function anonymous(a, b) {
c
}

Chrome

function anonymous(a,b
/**/) {
c
}

Safari

function anonymous(a, b) {
c
}

Edge

function anonymous(a,b) {
c
}

So, the main differences are:

Question 1. Is there any good reason to remove LF before body? putting LF there should give us better string representation, and matches to existing implementations.

Question 2. About LF after parameters, If I understand correctly, it's there to make toString result not SyntaxError when parameter contains "//". But it's necessary only in the case. How about putting LF only if necessary? (it will introduce additional complexity tho...)

ljharb commented 7 years ago

I would expect String(Function('a', 'b', 'c')) to output:

function (a, b) {
c
}

iow, Safari + Firefox's approach - Edge's seems to omit the arg spaces unnecessarily, and I have no idea why Chrome inserts the empty comment.

@arai-a can you point to the specific steps that lead to your conclusion, so I can follow it more closely?

arai-a commented 7 years ago

iiuc, /**/ is there to kill the effect of /* in the parameters.

The string representation is generated in https://tc39.github.io/Function-prototype-toString-revision/#sec-createdynamicfunction steps 8-9, 30-31.

arai-a commented 7 years ago

related issue around comment and toString, from https://bugzilla.mozilla.org/show_bug.cgi?id=1317400#c3

It's probably worth noting that the proposal only handles trailing single-line comments (by adding U+000A (LINE FEED) after the parameters and body string), but it doesn't handle leading html-comments.

Per the current proposal

Function("-->").toString() === "function anonymous(\n) {-->\n}"

Function("-->", "").toString() === "function anonymous(-->\n) {\n}

If we want to keep eval(functionObj.toString()) working for these cases, we also need to emit a line terminator before the parameters and body strings.

michaelficarra commented 7 years ago

@arai-a (/cc @anba) After discussing the HTML comment issue with other TC39 representatives this morning, I am happy to ignore this issue, treating HTML comments as a legacy syntactic feature. We are not expecting people to use HTML comments at the beginning of a string passed to the Function constructor and expect them to act as comments (effectively assuming they would appear at the beginning of the line). With // single-line comments, those would be expected to behave as comments regardless of context by the vast majority of users, so accommodations are made to preserve the syntactic structure of the F.p.toString output in their presence.

To answer the questions in your original post,

  1. No, but there's no good reason to add it either. We're not concerned about how "pretty" the output is. We're trying to maximise the predictability of the output with the least expense.

  2. You understand correctly. As with my first answer, we'd rather not introduce unnecessary complexity for vanity purposes.

edit: After @syg pointed out to me that all major implementations currently add a line terminator after the function body's {, I am in favour of matching the web reality in this case. I still have no intention of adding a line terminator following the parameter list's (.

arai-a commented 7 years ago

Thank you for your answer.

by closing, does it mean the spec proposal will be changed to include LF after body's "{" separated from this issue?

michaelficarra commented 7 years ago

Yes, I will make the change and update the test262 tests in the next few days. I'll reopen until that is done.

michaelficarra commented 7 years ago

test262 PR: https://github.com/tc39/test262/pull/803