Typo: swap <LF> and ") {"

[claudepache]

No description provided.

[michaelficarra]

No, this is necessary to allow comments inside parameter lists without causing a syntax error.

Function("a,b", "c // this is a comment", "body").toString();

function anonymous(a,b,c // this is a comment
) {body
}

[hax]

@michaelficarra The logic behind it is weird!

Why we need to add extra newline for such a edge case?

If there is anyone who add single line comment in the parameter, he/she always need to append newline him/herself. Otherwise, as current design, it's just introduce a new gotcha:

new Function(
  'a // param a', 
  'return a'
)(1); // 1
new Function(
  'a // param a', 
  'b // param b', 
  'return a + b'
)(1, 2); // Uncaught ReferenceError: b is not defined

[hax]

It seems the logic come from V8, and I check the history of V8 code: v8/v8@4b0395c

This code append not only newline but also /**/. As I understand, such codes are for security reason, i.e., to nullify comments (not for allow comments) in the param.

In details: As the original ES5, parameter syntax is very limited, so V8 only need to check ( char (see https://github.com/v8/v8/blob/4b0395cc23dd052552ea79b6a254ac5caea574a2/src/v8natives.js#L1703-L1706) and use \n/**/ to catch comment, to avoid XSS via new Function --- assume the attacker can control the content of at least one param and some part of the function body. This protection is failed after ES2015 add default parameter which allow any expression evaluation. But some maintainers of V8 seems not fully understand it so they even change \n/**/ to \n/*``*/ in v8/v8@e0d608a . Though this change is very weird and meaningless, it also prove that the original intention is to nullify the comments not allow them.

[michaelficarra]

@hax

new Function(
  'a // param a', 
  'return a'
)(1); // 1
new Function(
  'a // param a', 
  'b // param b', 
  'return a + b'
)(1, 2); // Uncaught ReferenceError: b is not defined

This is expected. This is not a change to the function constructor. The strings are concatenated with , between them and then parsed as FormalParameters. So only the one line terminator following the parameter list is needed.

[hax]

@michaelficarra

What I am talking about is the consistence of the behavior in the programmer's perspective, not the spec writer/implementor perspective.

The strings are concatenated ...

Unfortunately most programmers don't think deeply about it. Programmers may assume the API of new Function(...params, body) will do format check for each param.

With the old behavior (without extra newline before )), new Function('a//...', 'body') will throw syntax error, so the programmers will easily understand // comment is not allowed in the param, they will just drop the comment, or change to param /*comment*/, or even change to param // comment \n if they do some research and finally realize the whole story.

With current behavior, new Function('a//...', 'return a') won't throw syntax error and work as expected, so the programmers will very likely to believe // comment is allowed in the param. Further, new Function('a//...', 'b//...', 'return a+b') won't throw syntax error either! The potential reference error will only occur when the result function is called and goto the line of return a+b. If unfortunately the second param is happened to have the same name of global var (for example, a very common name: name, which window.name exist by default in the browser env), there will be no reference error at all. Such bug will be extremely hard to locate and debug.