1317400 - Implement "Function.prototype.toString revision" proposal

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

"Function.prototype.toString revision" [1] is currently a stage 3 proposal [2].

test262 already tests the new semantics [3], we're currently failing multiple tests because of this.


[1] https://github.com/tc39/Function-prototype-toString-revision
[2] https://github.com/tc39/proposals
[3] https://github.com/tc39/test262/pull/553

Comment 1

[Tooru Fujisawa [:arai]]

I have draft patch separated from bug 755821 patch.
I'll post it here after rebasing on m-c.

but we should wait for the following issues, before landing it.
  https://github.com/tc39/Function-prototype-toString-revision/issues/19
  https://github.com/tc39/Function-prototype-toString-revision/issues/20

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Part 1: Implement Function.prototype.toString revision proposal.

This at least passes most toString test in test262.
It doesn't pass line-terminator-normalisation tests, and class (bug 1216630).

added |preludeStart| parameter to several functions, that points the beginning of function source, that is used in JSScript::sourceDataWithPrelude (for toString) and also when recompiling in asm.js

Also, now Parser::standaloneFunction receives whole function source.

Currently this doesn't fully support the spec:
  a) line terminator normalization
    we don't do it
  b) newline after parameter
    spec says there should always be, but this patch adds it only if parameter contains "//"
  c) newline before body
    spec says there shouldn't be, but this patch adds it

if we can change the line number of function body, (b) can be changed to follow the spec.

Comment 3

[André Bargull [:anba]]

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.

Comment 4

[Michael Ficarra]

André, see my response to the HTML comment issue at https://github.com/tc39/Function-prototype-toString-revision/issues/20#issuecomment-264262232

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> Currently this doesn't fully support the spec:
>   b) newline after parameter
>     spec says there should always be, but this patch adds it only if
> parameter contains "//"
>   c) newline before body
>     spec says there shouldn't be, but this patch adds it

(c) is fixed in proposal.
(b) is necessary, and we should investigate bug 1319638 before this bug.

Comment 6

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> Currently this doesn't fully support the spec:
>   a) line terminator normalization
>     we don't do it

Our feedback was heard and the newline normalization was dropped: https://github.com/tc39/Function-prototype-toString-revision/pull/22

Comment 7

[Tooru Fujisawa [:arai]]

looks like returning "foo(){}" for `({foo(){}}).foo.toString()` breaks our test code :P

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec20bfd48efa4edee68b01a056bd9075a2bfaf1a&group_state=expanded&selectedJob=79952589

https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/browser/components/extensions/test/browser/browser_ext_browserAction_context.js#85-91
> function* runTests(options) {
> ...
>   let extension = ExtensionTestUtils.loadExtension({
>     manifest: options.manifest,
> 
>     files: options.files || {},
> 
>     background: `(${background})(${options.getTests})`,
>   });
> ...
> }

https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/browser/components/extensions/test/browser/browser_ext_browserAction_context.js#178
>   yield runTests({
> ...
>     getTests(tabs, expectDefaults) {
> ...
>     },
>   });

`options.getTests` is expected to return eval-able text.

I wonder if such case would happen also in wild...

Comment 8

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> I wonder if such case would happen also in wild...

maybe it's specific to Firefox addons.
at least V8 already returns "foo(){}"

JSC returns "function f(){}"

Comment 9

[Tooru Fujisawa [:arai]]

till, can I have your opinion about possibly breaking addons code, like above way?

Comment 10

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> JSC returns "function f(){}"

I meant "function foo(){}"

Comment 11

[Tooru Fujisawa [:arai]]

also breaks more widely used (in-tree) test harness, ExtensionTestUtils.js

https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/testing/mochitest/tests/SimpleTest/ExtensionTestUtils.js#94-96
>       if (typeof file == "function") {
>         ext.files[filename] = `(${file})();`
>       }

https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/testing/mochitest/tests/SimpleTest/ExtensionTestUtils.js#99-101
>   if (typeof ext.background == "function") {
>     ext.background = `(${ext.background})();`
>   }

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Implement Function.prototype.toString revision proposal.

attaching patches without r? for now.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Generate better source in Object.prototype.toSource.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Fix devtools debugger to follow the change in Function constructor source.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Fix devtools tests.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Fix WebExtensions tests.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Fix other tests.

so, Part 3-6 are tests affected by this change,
mostly either spacing or the string representation of method that doesn't contain "function " prefix.

Comment 18

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> till, can I have your opinion about possibly breaking addons code, like
> above way?

My hope would be that shorthand method definitions are still new enough that, combined with this being a somewhat niche use of toString, we should be ok. Ideally we land this in this cycle so the window in which addons might start using it doesn't grow by 6 weeks.

Comment 19

[Tooru Fujisawa [:arai]]

Comment on attachment 8841173 [details] [diff] [review]
Part 1: Implement Function.prototype.toString revision proposal.

Added `preludeStart` property to following structs (and also several frontend methods to pass it to ctors), that points the beginning of the function, inside entire source script.
  * FunctionBox
  * JSScript
  * LazyScript
  * AsmJSMetadata

> +    //   function * f(a, b) { return a + b; }
> +    //   ^          ^                        ^
> +    //   |          |                        |
> +    //   |          sourceStart_             sourceEnd_
> +    //   |
> +    //   preludeStart_


Now standalone functions (JS::CompileFunction, FunctionConstructor, etc) have parts before parameters, and Parser::standaloneFunction skips those parts.

in JS::CompileFunction, it constructs entire functions string.  To keep backward compatibility, it checks if the function name is valid identifier, and omits it from source if not.

Added JSScript::sourceDataWithPrelude that returns string including [preludeStart_,sourceStart_] part above.  And FunctionToString now just returns that string if the source is available.
If source is not available, source string is constructed with current way, in `AppendPrelude` function in FunctionToString.

Comment 20

[Tooru Fujisawa [:arai]]

Comment on attachment 8841174 [details] [diff] [review]
Part 2: Generate better source in Object.prototype.toSource.

Now method definition returns exact source code, and we can generate better string representation for Object#toSource().
Stopped modifying source string for them in js::ObjectToSource.

now js::ObjectToSource iterates over each property descriptor, and if both function's kind and property's kind are Method/Getter/Setter, it just uses the returned string.
otherwise (like, normal property, or when function's kind and property's kind doesn't match), it constructs the string representation of property.  in this case, it tries to strip down [preludeStart_,sourceStart_] part above, from function's toSource result.

Comment 21

[Tooru Fujisawa [:arai]]

Comment on attachment 8841175 [details] [diff] [review]
Part 3: Fix devtools debugger to follow the change in Function constructor source.

devtools filters out Function Constructor from source, by checking the source text.
that needs to be updated to follow the change (prepending "function ") in Part 1.

Comment 22

[Tooru Fujisawa [:arai]]

Comment on attachment 8841176 [details] [diff] [review]
Part 4: Fix devtools tests.

also fixed devtools tests to follow the change in string representation

Comment 23

[Tooru Fujisawa [:arai]]

Comment on attachment 8841177 [details] [diff] [review]
Part 5: Fix WebExtensions tests.

WebExtensions test framework expects the passed function's toString result to be eval-able.
this the change for Function#toString, method definition no longer returns eval-able code, but exact source string.
the testcases should use normal property with function value instead.

Comment 24

[Tooru Fujisawa [:arai]]

Comment on attachment 8841178 [details] [diff] [review]
Part 6: Fix other tests.

also some other tests that needs similar fix for string representation.

Comment 25

[Till Schneidereit [:till]]

Comment on attachment 8841173 [details] [diff] [review]
Part 1: Implement Function.prototype.toString revision proposal.

Review of attachment 8841173 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm slightly worried about the increased memory usage for (lazy) scripts and function boxes, but I'm not sure it's (a) really important and (b) all that easy to fix. Please at least check parsing-heavy benchmarks for regressions.

r=me without that addressed if no severe regressions are visible. If they are, we need to fix those first, I think.

::: js/src/frontend/SharedContext.h
@@ +449,5 @@
>      uint32_t        bufStart;
>      uint32_t        bufEnd;
>      uint32_t        startLine;
>      uint32_t        startColumn;
> +    uint32_t        preludeStart;

I'm not sure how much memory usage is a (perf) problem in the frontend. Using a full uint32_t seems like it'd not really be necessary. Instead, this could be expressed as an offset from bufStart. It's probably not worth the effort to change things to this, though.

::: js/src/jsapi.cpp
@@ +4342,5 @@
>      {
>          return false;
>      }
>  
> +    if (isInvalidName)

Can you add a comment to this explaining why we're setting the name if the name is invalid? I'd imagine it's quite surprising to see without the full context.

::: js/src/jsscript.h
@@ +895,3 @@
>      uint32_t        sourceStart_;
>      uint32_t        sourceEnd_;
> +    uint32_t        preludeStart_;

Here and for LazyScript, it'd definitely be nice to use fewer bits for this. It seems like 4 bits should be sufficient. Unfortunately I'm not sure we have those either on Script or on LazyScript, so this might not actually do us any good. :(

::: js/src/vm/CommonPropertyNames.h
@@ +312,5 @@
>      macro(setPrefix, setPrefix, "set ") \
>      macro(setPrototypeOf, setPrototypeOf, "setPrototypeOf") \
>      macro(shape, shape, "shape") \
>      macro(size, size, "size") \
> +    macro(slashSlash, slashSlash, "//") \

This doesn't seem to be used.

Comment 26

[Till Schneidereit [:till]]

Comment on attachment 8841175 [details] [diff] [review]
Part 3: Fix devtools debugger to follow the change in Function constructor source.

Review of attachment 8841175 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a devtools peer, but this seems entirely fine for me to review.

Comment 27

[Till Schneidereit [:till]]

Comment on attachment 8841176 [details] [diff] [review]
Part 4: Fix devtools tests.

Review of attachment 8841176 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 28

[Till Schneidereit [:till]]

Comment on attachment 8841178 [details] [diff] [review]
Part 6: Fix other tests.

Review of attachment 8841178 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 8841177 [details] [diff] [review]
Part 5: Fix WebExtensions tests.

This can be handled with a lot less churn within the webextensions testing framework.  :zombie has offered to create a simpler patch to do so.

Comment 30

[Tomislav Jovanovic :zombie]

(In reply to Andrew Swan [:aswan] from comment #29)
>  :zombie has offered to create a simpler patch to do so.

Yes, I'll use bug 1343583 as I'm more accustomed to the mozreview workflow..

Comment 31

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Generate better source in Object.prototype.toSource.

Added test cases.

Also, while writing tests, I thought we could produce a bit more user-friendly source for some cases.

supported following case:
  * compose method syntax for dynamically defined property with other object's method

and changed:
  * computed property always compose syntax with actual property name, instead of exact source

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Part 2 followup: Fix existing test for toSource with modified function toSource.

found one testcase that checks modified toSource behavior.
fixed to follow the new behavior.

Comment 33

[Till Schneidereit [:till]]

Comment on attachment 8842691 [details] [diff] [review]
Part 2 followup: Fix existing test for toSource with modified function toSource.

Review of attachment 8842691 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 34

[Till Schneidereit [:till]]

Comment on attachment 8842676 [details] [diff] [review]
Part 2: Generate better source in Object.prototype.toSource.

Review of attachment 8842676 [details] [diff] [review]:
-----------------------------------------------------------------

Great code, fantastic tests!

I had a couple of questions and nits. I don't think I really need to see a new version of the patch with those addressed, but if you end up making substantial changes to the code, feel free to re-request review.

::: js/src/builtin/Object.cpp
@@ +175,3 @@
>      }
>  
> +    // Support following cases, with spaces between tokens:

Nit: "the following"

@@ +183,5 @@
> +    //                +- get ------+
> +    //                |            |
> +    //                +- set ------+
> +    //
> +    // This accepts some invalid syntax, but we don't care.

Can you add a sentence explaining why we don't care? Something like "It's only used by the non-standard toSource, and we're doing a best-effort attempt here".

@@ +190,5 @@
> +    ConsumeSpaces(s, e);
> +    (void) (Consume(s, e, "function") || Consume(s, e, "get") || Consume(s, e, "set"));
> +    ConsumeSpaces(s, e);
> +    (void) Consume(s, e, "*");
> +    ConsumeSpaces(s, e);

This is very nice!

@@ +288,5 @@
> +        size_t voffset, vlength;
> +
> +        /*
> +         * Methods and accessors can return exact syntax of source, that fits
> +         * into property without adding property name or "get"/"set" prefix.

I don't fully understand this comment. Aren't you adding the get/set prefixes below? To me it sounds like none need to be added at all.

@@ +298,3 @@
>              /*
> +             * Properties can be set by Object.defineProperty etc.
> +             * If kind doesn't match, fallback to normal property handling.

Nit: s/kind/the kind/ and s/fallback/fall back/

@@ +317,5 @@
> +                }
> +            }
> +
> +            {
> +                // When falling back try to generate better string

nit: s/better/a better/

@@ +318,5 @@
> +            }
> +
> +            {
> +                // When falling back try to generate better string
> +                // representation by skipping prelude, and also removing

nit: s/prelude/the prelude/

@@ +319,5 @@
> +
> +            {
> +                // When falling back try to generate better string
> +                // representation by skipping prelude, and also removing
> +                // enclosing parentehes.

nit: s/enclosing/the enclosing/ and s/parentehes/parentheses/

@@ +336,5 @@
> +                    return false;
> +            } else if (kind == PropertyKind::Setter) {
> +                if (!buf.append("set "))
> +                    return false;
> +            } else {

Should this be `else if (kind == PropertyKind::Method)` so it doesn't wrongly prefix the property id with "async " or "*" in cases where we had to fall back to PropertyKind::Normal in line 331 above?

I'm probably missing something and the answer is "no", but if I'm not, please fix and add tests that would've caught this.

@@ +384,5 @@
> +        if (!desc.object())
> +            continue;
> +
> +        if (desc.isAccessorDescriptor()) {
> +            if (desc.hasGetterObject() && desc.getterObject()) {

hasGetterObject implies that getterObject returns an object, so the second condition can be removed.

@@ +385,5 @@
> +            continue;
> +
> +        if (desc.isAccessorDescriptor()) {
> +            if (desc.hasGetterObject() && desc.getterObject()) {
> +                MOZ_ASSERT(desc.getterObject()->is<JSFunction>());

I wonder if this should be asserted in PropertyDescriptor::getterObject.

Oh, except. Is it possible for this to be false? I don't know if callable proxies or exotic callable host objects can be used as getters/setters. If so, then please add a test case that would trigger this assert and remove/broaden it. I assume that if it's not possible we have tests for that.

@@ +392,3 @@
>                      return nullptr;
>              }
> +            if (desc.hasSetterObject() && desc.setterObject()) {

Same here.

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Generate better source in Object.prototype.toSource.

Thank you for reviewing :)

(In reply to Till Schneidereit [till] from comment #34)
> @@ +288,5 @@
> > +        size_t voffset, vlength;
> > +
> > +        /*
> > +         * Methods and accessors can return exact syntax of source, that fits
> > +         * into property without adding property name or "get"/"set" prefix.
> 
> I don't fully understand this comment. Aren't you adding the get/set
> prefixes below? To me it sounds like none need to be added at all.

oh, the comment is actually for more speicific case than the only condition below it.

it applies to the all conditions required for the below "return true" path, that uses function's exact source, without modifying it.

>         if (kind == PropertyKind::Getter || kind == PropertyKind::Setter ||
>             kind == PropertyKind::Method)
> ...
>             if (((fun->isGetter() && kind == PropertyKind::Getter) ||
>                  (fun->isSetter() && kind == PropertyKind::Setter) ||
>                  kind == PropertyKind::Method) &&
>                 fun->explicitName())
>            {
> ...
>                 if (result)  {
>                     if (!buf.append(valstr))
>                         return false;
>                     return true;
>                 }

Then, if one of above conditions doesn't match, it fallbacks to composed syntax, that prefixes get/set etc.

Rephrased the comments.


> @@ +336,5 @@
> > +                    return false;
> > +            } else if (kind == PropertyKind::Setter) {
> > +                if (!buf.append("set "))
> > +                    return false;
> > +            } else {
> 
> Should this be `else if (kind == PropertyKind::Method)` so it doesn't
> wrongly prefix the property id with "async " or "*" in cases where we had to
> fall back to PropertyKind::Normal in line 331 above?

Indeed, it should check `kind == PropertyKind::Method`.


> @@ +384,5 @@
> > +        if (!desc.object())
> > +            continue;
> > +
> > +        if (desc.isAccessorDescriptor()) {
> > +            if (desc.hasGetterObject() && desc.getterObject()) {
> 
> hasGetterObject implies that getterObject returns an object, so the second
> condition can be removed.

Apparently that's not true.

I get null for setterObject in `{ get foo(){} }`.
also found the other case that checks return value.

https://dxr.mozilla.org/mozilla-central/rev/e91de6fb2b3dce9c932428265b0fdb630ea470d7/js/src/jsobj.cpp#164-167
>         if (JSObject* set = desc.setterObject())
>             v.setObject(*set);
>         else
>             v.setUndefined();

> @@ +385,5 @@
> > +            continue;
> > +
> > +        if (desc.isAccessorDescriptor()) {
> > +            if (desc.hasGetterObject() && desc.getterObject()) {
> > +                MOZ_ASSERT(desc.getterObject()->is<JSFunction>());
> 
> I wonder if this should be asserted in PropertyDescriptor::getterObject.
> 
> Oh, except. Is it possible for this to be false? I don't know if callable
> proxies or exotic callable host objects can be used as getters/setters. If
> so, then please add a test case that would trigger this assert and
> remove/broaden it. I assume that if it's not possible we have tests for that.

Thanks, removed JSFunction-only restriction and added Proxy and other global's functions test cases.

Comment 36

[Till Schneidereit [:till]]

Comment on attachment 8842926 [details] [diff] [review]
Part 2: Generate better source in Object.prototype.toSource.

Review of attachment 8842926 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, looks great.

(In reply to Tooru Fujisawa [:arai] from comment #35)
> > hasGetterObject implies that getterObject returns an object, so the second
> > condition can be removed.
> 
> Apparently that's not true.

Oh, I looked at the wrong hasGetterObject. It seems pretty weird to me that hasGetterObject doesn't *really* mean that there's a getter object, but ok.

Comment 37

[Tooru Fujisawa [:arai]]

(In reply to Till Schneidereit [till] from comment #25)
> Here and for LazyScript, it'd definitely be nice to use fewer bits for this.
> It seems like 4 bits should be sufficient. Unfortunately I'm not sure we
> have those either on Script or on LazyScript, so this might not actually do
> us any good. :(

The offset can be long, in case there's a comment between "function" and "(".
If we use fewer bits, we should define a behavior for the case it doesn't fit into the reduced bits.

Also, delazification relies on the preludeStart points correct poisition.

maybe we could remove sourceStart_/begin_, if preludeStart is sufficient for all purpose.
I'll file another buf to investigate it.

Comment 38

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #37)
> (In reply to Till Schneidereit [till] from comment #25)
> > Here and for LazyScript, it'd definitely be nice to use fewer bits for this.
> > It seems like 4 bits should be sufficient. Unfortunately I'm not sure we
> > have those either on Script or on LazyScript, so this might not actually do
> > us any good. :(
> 
> The offset can be long, in case there's a comment between "function" and "(".
> If we use fewer bits, we should define a behavior for the case it doesn't
> fit into the reduced bits.

Oh, right :(
> 
> Also, delazification relies on the preludeStart points correct poisition.

:(

> maybe we could remove sourceStart_/begin_, if preludeStart is sufficient for
> all purpose.
> I'll file another buf to investigate it.

That'd be great, thanks.

Comment 39

[Tooru Fujisawa [:arai]]

devtool's code was fixed by Bug 1338914.
  https://hg.mozilla.org/mozilla-central/rev/8a2b1bdb1549
I'll skip Part 3.

Comment 40

[Tooru Fujisawa [:arai]]

Attachment: Part 1.1: Remove a space after comma in parameter list for generated function source.

Forgot the another difference between our current implementation and the proposal.
we shouldn't put " " after "," for parameter list.

input:    Function("a", "b", "c", "body")
current:  function anonymous(a, b, c)...
proposal: function anonymous(a,b,c)...

updated FunctionConstructor not to put " ", and fixed some testcases,
and also enabled some test262 tests.

Comment 41

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #40)
> input:    Function("a", "b", "c", "body")
> current:  function anonymous(a, b, c)...
> proposal: function anonymous(a,b,c)...

actually,

proposal: function anonymous(a,b,c\n)...

(the newline is already added in Part 1)

Comment 42

[Till Schneidereit [:till]]

Comment on attachment 8843539 [details] [diff] [review]
Part 1.1: Remove a space after comma in parameter list for generated function source.

Review of attachment 8843539 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks

::: js/src/tests/jstests.list
@@ +533,5 @@
>  # Expects TypeError, but RangeError thrown.
>  skip script test262/built-ins/DataView/prototype/setFloat64/detached-buffer-after-toindex-byteoffset.js
>  skip script test262/built-ins/DataView/prototype/setInt16/detached-buffer-after-toindex-byteoffset.js
> +
> +# Removed from the proposal: https://github.com/tc39/Function-prototype-toString-revision/issues/19

If that doesn't exist already, can you open an issue against test262 to remove these tests?

Comment 43

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd076a9610d4ede1314d5d3f36870a68c6a1d322
Bug 1317400 - Part 1: Implement Function.prototype.toString revision proposal. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e84f5f909ff1d1bfa1a0dc864951079be59facd
Bug 1317400 - Part 1.1: Remove a space after comma in parameter list for generated function source. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/7397faeecc701270fd8f10d12bd0cc1efb7fc48c
Bug 1317400 - Part 2: Generate better source in Object.prototype.toSource. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0990f1b73b188c82fd712c55a0d3c19af7ee52
Bug 1317400 - Part 3: Fix devtools tests. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f67f1b9a60116970d7cbb689ec610710cdab9b2
Bug 1317400 - Part 4: Fix other tests. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/88471e975cc350b28cc8ea651dbe7033db697ccd
Bug 1317400 - Part 5: Fix remaining WebExtensions tests. r=zombie

Comment 44

[Tooru Fujisawa [:arai]]

(In reply to Till Schneidereit [till] from comment #42)
> Comment on attachment 8843539 [details] [diff] [review]
> Part 1.1: Remove a space after comma in parameter list for generated
> function source.
> 
> Review of attachment 8843539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, thanks
> 
> ::: js/src/tests/jstests.list
> @@ +533,5 @@
> >  # Expects TypeError, but RangeError thrown.
> >  skip script test262/built-ins/DataView/prototype/setFloat64/detached-buffer-after-toindex-byteoffset.js
> >  skip script test262/built-ins/DataView/prototype/setInt16/detached-buffer-after-toindex-byteoffset.js
> > +
> > +# Removed from the proposal: https://github.com/tc39/Function-prototype-toString-revision/issues/19
> 
> If that doesn't exist already, can you open an issue against test262 to
> remove these tests?

oops, sorry I misunderstood those tests.
those tests are about "not normalise", so they now reflect the latest proposal and I should've enabled them.
I'll post a patch shortly.

Comment 45

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cce63d5f6ddeb27dd7a2e0bb98b2fc34d4d3f606
Bug 1317400 - Part 6: Enable tests for not normalizing line terminator. r=till

Comment 46

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1535bb7d31397edde81076bd137d344593dcd9f
Bug 1317400 - Part 6 followup: Disable test for CR-LF on shell, because of windows js shell bug. r=bustage

Comment 47

[Tooru Fujisawa [:arai]]

Part 6 followup is for bug 1344479.
windows js shell seems to normalize \r\n to \n while reading the source, and breaks the testcase.

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd076a9610d4
https://hg.mozilla.org/mozilla-central/rev/2e84f5f909ff
https://hg.mozilla.org/mozilla-central/rev/7397faeecc70
https://hg.mozilla.org/mozilla-central/rev/9c0990f1b73b
https://hg.mozilla.org/mozilla-central/rev/8f67f1b9a601
https://hg.mozilla.org/mozilla-central/rev/88471e975cc3
https://hg.mozilla.org/mozilla-central/rev/cce63d5f6dde
https://hg.mozilla.org/mozilla-central/rev/a1535bb7d313

Comment 49

[Michael Ficarra]

Tooru Fujisawa, I've added Function.prototype.toString tests (ported from the official test262 tests) to [Juriy Zaytsev's compatibility table](http://kangax.github.io/compat-table/esnext/) to encourage implementers to complete their implementations. Nightly builds of Firefox fail at least two of these tests.

Comment 50

[Tooru Fujisawa [:arai]]

thanks.
yes, they're tracked in bug 1216630.

Comment 51

[Sebastian Zartner [:sebo]]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/toString should be updated to reflect the changes and provide an example.

Sebastian

Comment 52

[Florian Scholz (Open Web Docs)]

Added to https://developer.mozilla.org/en-US/Firefox/Releases/54#JavaScript

Updated the compat table and added an example regarding the native function string to: 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/toString

The line ending normalization didn't make it. I'm unsure what other meaningful/practical examples to add here. Please contribute to the MDN wiki if you know any.