Implement "Function.prototype.toString revision" proposal

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: anba, Assigned: arai)

Tracking

(Blocks 3 bugs, {dev-doc-complete})

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(6 attachments, 5 obsolete attachments)

114.58 KB, patch
till
: review+
Details | Diff | Splinter Review
3.35 KB, patch
till
: review+
Details | Diff | Splinter Review
4.77 KB, patch
till
: review+
Details | Diff | Splinter Review
1.29 KB, patch
till
: review+
Details | Diff | Splinter Review
24.87 KB, patch
till
: review+
Details | Diff | Splinter Review
10.76 KB, patch
till
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
"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
Assignee

Updated

3 years ago
See Also: → 755821
Assignee

Comment 1

3 years ago
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
Assignee

Comment 2

3 years ago
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.
Assignee

Updated

3 years ago
See Also: → 1319638
Reporter

Comment 3

3 years ago
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.
Assignee

Comment 5

3 years ago
(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.
Depends on: 1319638
See Also: 1319638
Reporter

Updated

3 years ago
Blocks: 1321616
(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
Assignee

Comment 7

2 years ago
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...
Assignee

Comment 8

2 years ago
(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(){}"
Assignee

Comment 9

2 years ago
till, can I have your opinion about possibly breaking addons code, like above way?
Flags: needinfo?(till)
Assignee

Comment 10

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

I meant "function foo(){}"
Assignee

Comment 11

2 years ago
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})();`
>   }
Assignee

Comment 12

2 years ago
attaching patches without r? for now.
Attachment #8813483 - Attachment is obsolete: true
Assignee

Comment 16

2 years ago
Assignee

Comment 17

2 years ago
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.
(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.
Flags: needinfo?(till)
Assignee

Comment 19

2 years ago
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.
Attachment #8841173 - Flags: review?(till)
Assignee

Comment 20

2 years ago
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.
Attachment #8841174 - Flags: review?(till)
Assignee

Comment 21

2 years ago
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.
Attachment #8841175 - Flags: review?(till)
Assignee

Comment 22

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

also fixed devtools tests to follow the change in string representation
Attachment #8841176 - Flags: review?(till)
Assignee

Comment 23

2 years ago
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.
Attachment #8841177 - Flags: review?(aswan)
Assignee

Comment 24

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

also some other tests that needs similar fix for string representation.
Attachment #8841178 - Flags: review?(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.
Attachment #8841173 - Flags: review?(till) → review+
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.
Attachment #8841175 - Flags: review?(till) → review+
Comment on attachment 8841176 [details] [diff] [review]
Part 4: Fix devtools tests.

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

rs=me
Attachment #8841176 - Flags: review?(till) → review+
Comment on attachment 8841178 [details] [diff] [review]
Part 6: Fix other tests.

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

rs=me
Attachment #8841178 - Flags: review?(till) → review+
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.
Flags: needinfo?(tomica)
Attachment #8841177 - Flags: review?(aswan) → review-
Depends on: 1343583
(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..
Flags: needinfo?(tomica)
Assignee

Comment 31

2 years ago
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
Assignee: nobody → arai.unmht
Attachment #8841174 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8841174 - Flags: review?(till)
Attachment #8842676 - Flags: review?(till)
Assignee

Comment 32

2 years ago
found one testcase that checks modified toSource behavior.
fixed to follow the new behavior.
Attachment #8842691 - Flags: review?(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
Attachment #8842691 - Flags: review?(till) → review+
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.
Attachment #8842676 - Flags: review?(till) → review+
Assignee

Comment 35

2 years ago
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.
Attachment #8842676 - Attachment is obsolete: true
Attachment #8842926 - Flags: review?(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.
Attachment #8842926 - Flags: review?(till) → review+
Assignee

Comment 37

2 years ago
(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.
(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.
Assignee

Comment 39

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

Updated

2 years ago
Blocks: 1344428
Assignee

Comment 40

2 years ago
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.
Attachment #8841175 - Attachment is obsolete: true
Attachment #8841177 - Attachment is obsolete: true
Attachment #8843539 - Flags: review?(till)
Assignee

Comment 41

2 years ago
(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 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?
Attachment #8843539 - Flags: review?(till) → review+
Assignee

Comment 44

2 years ago
(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.
Assignee

Comment 45

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

Updated

2 years ago
See Also: → 1344479
Assignee

Comment 46

2 years ago
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
Assignee

Comment 47

2 years ago
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.
Assignee

Updated

2 years ago
Depends on: 1345162

Comment 49

2 years ago
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.
Assignee

Comment 50

2 years ago
thanks.
yes, they're tracked in bug 1216630.
Assignee

Updated

2 years ago
See Also: → 1216630
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
Keywords: dev-doc-needed
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.

Updated

a year ago
Depends on: 1458017
Reporter

Updated

8 months ago
Depends on: 1462741
Reporter

Updated

8 months ago
Depends on: 1463529
You need to log in before you can comment on or make changes to this bug.