Brandon Benvie [:benvie] Reporter
	Description • 11 years ago

The latest ES6 spec revision (August 2013) makes function the "length" property configurable. `Function.prototype.bind` still produces functions with non-configurable "length" but I think this may be an oversight.

See section 8.3.16.6.

	Jason Orendorff [:jorendorff] Assignee
	Comment 1 • 10 years ago

Looks easy so far; writing tests.

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 10 years ago

This isn't the long-desired new approach, just a quick Monday morning hack.

	Till Schneidereit [:till]
	Comment 4 • 10 years ago

These are the jstests I changed when I looked into this a long time ago in a galaxy [...]

If these changes are helpful, just integrate them into your patch. If not, that's great.

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8496951 [details] [diff] [review]
Make the "length" property of function objects configurable

Retracting in favor a forthcoming new patch.

	Jason Orendorff [:jorendorff] Assignee
	Comment 6 • 10 years ago

Comment on attachment 8496993 [details] [diff] [review]
Adapt jstests to configurable Function#length

Thanks, Till. The new patch incorporates these changes -- but I had to make additional changes to most of these tests.

	Jason Orendorff [:jorendorff] Assignee
	Comment 7 • 10 years ago

Thanks to Till Schneidereit for a bunch of test fixes.

	Jeff Walden [:Waldo]
	Comment 8 • 10 years ago

Comment on attachment 8497673 [details] [diff] [review]
Make the "length" property of function objects configurable

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

Deleting a function's length resulting in its length being 0 is really whack.  :-(  I got a bit confused about that until at some point here I realized Function.prototype.length would shine through.

::: js/src/jsfun.cpp
@@ +485,4 @@
>          JS_ASSERT(!IsInternalFunctionObject(obj));
>  
>          RootedValue v(cx);
> +        uint32_t attrs = JSPROP_READONLY;

Mild preference for this uninitialized, with assignments of full composite attrs in both arms of if.

@@ +494,5 @@
> +            // Afterwards, asking for f.length again will cause this resolve
> +            // hook to run again. Defining the property again the second
> +            // time through would be a bug.
> +            //     assertEq(f.length, 0);  // gets Function.prototype.length!
> +            // We use the RESOLVED_LENGTH flag to avoid this bug.

I would s/avoid/hack around/, to alert the reader to smell here.

@@ +502,4 @@
>              if (fun->isInterpretedLazy() && !fun->getOrCreateScript(cx))
>                  return false;
>              uint16_t length = fun->hasScript() ? fun->nonLazyScript()->funLength() :
>                  fun->nargs() - fun->hasRest();

Pre-existing, but do we really need to be non-lazy just to compute length?  Can't we get it regardless of laziness?  Followup to remove script creation, please.

Looking at some of that nargs-computing code, I see this current behavior:

js> ({ set y(...x) {} })
({set y (...x) {}})
js> ({ set y(x = 2) {} })
({set y (x = 2) {}})

The first is a bug, I think.  Right?  The second is a bit whack given getters must be zero-args and setters one-arg, and those restrictions only make sense if you think of these as being intended for kind of only that use.  Possibly the grammar should be tightened to prohibit a default for the set-parameter.  There's already a moderately special term just for setter arguments, so this seems not super-onerous to report as a spec bug/issue.

::: js/src/tests/ecma/GlobalObject/15.1.2.5-1.js
@@ +58,5 @@
>  
>  new TestCase( SECTION, "unescape.length",       1,               unescape.length );
>  new TestCase( SECTION, "unescape.length = null; unescape.length",   1,      eval("unescape.length=null; unescape.length") );
> +new TestCase( SECTION, "delete unescape.length",                    true,  delete unescape.length );
> +new TestCase( SECTION, "delete unescape.length; unescape.length",   0,      eval("delete unescape.length; unescape.length") );

So I'm reviewing out of order, noticed here -- these testcases have side effects that persist into the other tests, right?  So the first deletes, then the second re-deletes pointlessly, right?  I wonder if some of these tests need moar care about deletions not poisoning the rest of the test's effect or so.

::: js/src/tests/ecma/extensions/15.1.2.1-1.js
@@ +27,2 @@
>  new TestCase( SECTION,      "var PROPS = ''; for ( p in eval ) { PROPS += p }; PROPS",  "", eval("var PROPS = ''; for ( p in eval ) { PROPS += p }; PROPS") );
> +new TestCase( SECTION,      "eval.length = null; eval.length",       0, eval( "eval.length = null; eval.length") );

Bah, assignment to non-writable properties not being an error.

::: js/src/tests/ecma_5/Object/defineProperty-setup.js
@@ +519,5 @@
>  
>  var ALL_DESCRIPTORS = mapTestDescriptors(function(d) { return true; });
>  var VALID_DESCRIPTORS = mapTestDescriptors(isValidDescriptor);
>  
> +var SKIP_FULL_FUNCTION_LENGTH_TESTS = false;

This should just go away, see the next comment on this file.

@@ +539,4 @@
>          {
>            print("Skipping full tests for redefining Function.length for now " +
>                  "because we don't support redefinition of properties with " +
>                  "native getter or setter...");

This bit is out of date.  We should be able to get rid of this entire if (...1) { ...2 } else { ...3 } and replace it purely with ...3 with your adjustments to it.

@@ +543,5 @@
>            self._simpleFunctionLengthTests();
>          }
>          else
>          {
> +          self._fullFunctionLengthTests(() => Function("one", ""), 1);

/* body */ comment might help.

@@ +746,3 @@
>        {
> +        var desc = VALID_DESCRIPTORS[i];
> +        this._runSingleFunctionLengthTest(funFactory(), len, desc);

var suchFunVeryWow = funFactory();
this._runSingleFunctionLengthTest(suchFunVeryWow, len, desc);

...no, you say?  Oh well.

::: js/src/tests/ecma_6/Function/configurable-length.js
@@ +5,5 @@
> +// configurable. More thorough tests follow.
> +var f = function (a1, a2, a3, a4) {};
> +assertEq(delete f.length, true);
> +assertEq(f.hasOwnProperty("length"), false);
> +assertEq(f.length, 0);  // inherited from Function.prototype.length

I was rather confused how all the .length values would be 0 after deletion, til I hit this.  :-\  (You missed a ton of tests where I had "shouldn't this be undefined" comments written, since deleted.)  Egad.

@@ +38,5 @@
> +assertEq(desc.value, 5);
> +
> +// After deleting the length property, assigning to f.length fails due to an
> +// unfortunate ES5 rule. (Since Function.prototype.length is non-writable,
> +// this fails. In strict mode it would throw.)

I dispute that this rule is unfortunate.  :-P

@@ +69,5 @@
> +
> +// Object.defineProperty on a brand-new function is sufficient to cause the
> +// LENGTH_RESOLVED flag to be set.
> +g = mkfun();
> +Object.defineProperty(g, "length", {value: 0});

Do the same thing, but double-check the attributes are merged into the (implicitly existing) ones?

g = mkfun();
Object.defineProperty(g, "length", { value: 42 });
desc = Object.getOwnPropertyDescriptor(g, "length");
assertEq(desc.enumerable, false);
assertEq(desc.configurable, true);
assertEq(desc.writable, false);
assertEq(desc.value, 42);

::: js/src/tests/jstests.list
@@ +9,5 @@
> +###################################################
> +
> +# These tests assert that function.length is non-configurable. It was
> +# non-configurable in ES5, but ES6 changes this and we have implemented the
> +# newer standard (bug 911142).

Blind assumption all these tests actually legitimately changed behavior to expected failure with this change -- didn't review any of them.  (Bleh.  :-\ )

	Jason Orendorff [:jorendorff] Assignee
	Comment 9 • 10 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Mild preference for this uninitialized, with assignments of full composite
> attrs in both arms of if.

done

> I would s/avoid/hack around/, to alert the reader to smell here.

reworded

> Pre-existing, but do we really need to be non-lazy just to compute length?
> Can't we get it regardless of laziness?  Followup to remove script creation,
> please.

Filed bug 1089625.

> Looking at some of that nargs-computing code, I see this current behavior:
> 
> js> ({ set y(...x) {} })
> ({set y (...x) {}})
> js> ({ set y(x = 2) {} })
> ({set y (x = 2) {}})
> 
> The first is a bug, I think.

Sure is. Filed bug 1089632.

> The second is a bit whack given
> getters must be zero-args and setters one-arg, and those restrictions only
> make sense if you think of these as being intended for kind of only that
> use.  Possibly the grammar should be tightened to prohibit a default for the
> set-parameter.

I don't care enough to raise this, but you're right that it would be a fairly simple to change the spec to ban this, while still permitting the maybe-useful defaults inside destructuring parameters, like this:

    obj = {
        set options({"javascript.options.bananas": bananas=true}) {...}
    };

So feel free, as they say...

> ::: js/src/tests/ecma/GlobalObject/15.1.2.5-1.js
> > +new TestCase( SECTION, "delete unescape.length",                    true,  delete unescape.length );
> > +new TestCase( SECTION, "delete unescape.length; unescape.length",   0,      eval("delete unescape.length; unescape.length") );
> 
> So I'm reviewing out of order, noticed here -- these testcases have side
> effects that persist into the other tests, right?  So the first deletes,
> then the second re-deletes pointlessly, right?

Yes they do. I deleted the code in question from all these old tests
and put new ones in ecma_6/Function/configurable-length-builtins.js.

> ::: js/src/tests/ecma_5/Object/defineProperty-setup.js
> > +var SKIP_FULL_FUNCTION_LENGTH_TESTS = false;
> 
> This should just go away, see the next comment on this file.

done

> /* body */ comment might help.

ok, done

> this._runSingleFunctionLengthTest(suchFunVeryWow, len, desc);

no

> I was rather confused how all the .length values would be 0 after deletion,
> til I hit this.  :-\  (You missed a ton of tests where I had "shouldn't this
> be undefined" comments written, since deleted.)  Egad.

Yeah. And you missed a ton of me staring at code like

    delete fun.length;
    fun.length = 2;
    assertEq(fun.length, 2);  // FAILS!

not understanding why that simple assignment wouldn't work, until remembering for the millionth time that assigning doesn't shadow if the inherited data property is nonwritable. :-P

> > +assertEq(desc.value, 5);
> > +
> > +// After deleting the length property, assigning to f.length fails due to an
> > +// unfortunate ES5 rule. (Since Function.prototype.length is non-writable,
> > +// this fails. In strict mode it would throw.)
> 
> I dispute that this rule is unfortunate.  :-P

Maybe it's not unfortunate for users as much as it is for me. I trip over this constantly.

Reworded.

> > +// Object.defineProperty on a brand-new function is sufficient to cause the
> > +// LENGTH_RESOLVED flag to be set.
> > +g = mkfun();
> > +Object.defineProperty(g, "length", {value: 0});
> 
> Do the same thing, but double-check the attributes are merged into the
> (implicitly existing) ones?

Very nice. Added.

> ::: js/src/tests/jstests.list
> Blind assumption all these tests actually legitimately changed behavior to
> expected failure with this change -- didn't review any of them.

Yes, they did.

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 10 years ago

1082 INFO TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/js/builtins/test_WeakMap.prototype-properties.html | WeakMap.prototype.clear.length - WeakMap.prototype.clear.length: assert_equals: [[Configurable]] expected false but got true

1084 INFO TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/js/builtins/test_WeakMap.prototype-properties.html | WeakMap.prototype.delete.length - WeakMap.prototype.delete.length: assert_equals: [[Configurable]] expected false but got true

1086 INFO TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/js/builtins/test_WeakMap.prototype-properties.html | WeakMap.prototype.has.length - WeakMap.prototype.has.length: assert_equals: [[Configurable]] expected false but got true

1088 INFO TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/js/builtins/test_WeakMap.prototype-properties.html | WeakMap.prototype.set.length - WeakMap.prototype.set.length: assert_equals: [[Configurable]] expected false but got true

The test expects the wrong thing. This test is from W3C; I filed an issue:
  https://github.com/w3c/web-platform-tests/issues/1316

In the meantime I'm adding these failures to our existing suppression file:

diff --git a/dom/imptests/failures/html/js/builtins/test_WeakMap.prototype-properties.html.json b/dom/imptests/failures/html/js/builtins/test_WeakMap.prototype-properties.html.json
--- a/dom/imptests/failures/html/js/builtins/test_WeakMap.prototype-properties.html.json
+++ b/dom/imptests/failures/html/js/builtins/test_WeakMap.prototype-properties.html.json
@@ -1,4 +1,8 @@
 {
+  "WeakMap.prototype.clear.length": true,
+  "WeakMap.prototype.delete.length": true,
   "WeakMap.prototype.get.length": true,
-  "WeakMap.prototype.get: return undefined": true
+  "WeakMap.prototype.get: return undefined": true,
+  "WeakMap.prototype.has.length": true,
+  "WeakMap.prototype.set.length": true
 }

	Jason Orendorff [:jorendorff] Assignee
	Comment 11 • 10 years ago

It strikes me as worrisome that we have dom/imptests/webapps.txt pointing to an obsolete W3C repo. Should I file a bug on that?

	Jason Orendorff [:jorendorff] Assignee
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/629b02d8c025

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 10 years ago

Web-platform-4 tests were still failing, so backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/535e27a2d2de

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3336370&repo=mozilla-inbound

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 14 • 10 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> It strikes me as worrisome that we have dom/imptests/webapps.txt pointing to
> an obsolete W3C repo. Should I file a bug on that?

No, it'll be gone as soon as web-platform-tests runs on more platform than just Linux. Until then it'd be pretty painful and not very useful to move it around.

	Jason Orendorff [:jorendorff] Assignee
	Comment 15 • 10 years ago

Ms2ger, can you tell me how to run this test locally? Or just why this is TEST-UNEXPECTED-FAIL even though I marked the test as failing?

	Jason Orendorff [:jorendorff] Assignee
	Comment 16 • 10 years ago

Ms2ger pointed out that

  mach mochitest-plain dom/imptests/html/js/builtins/test_WeakMap.prototype-properties.html 

does the trick.

However the test passes for me locally. FML.

	Jeff Walden [:Waldo]
	Comment 17 • 10 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> I don't care enough to raise this, but you're right that it would be a
> fairly simple to change the spec to ban this, while still permitting the
> maybe-useful defaults inside destructuring parameters, like this:
> 
>     obj = {
>         set options({"javascript.options.bananas": bananas=true}) {...}
>     };
> 
> So feel free, as they say...

Filed:

https://bugs.ecmascript.org/show_bug.cgi?id=3385

Somewhat amusingly, the example use I provided for defaults inside of parameters, to explicitly mention that it wouldn't be affected by the grammar change, was the exact same use case as the one you mentioned here.  :-)

	Tom S [:evilpie]
	Comment 18 • 9 years ago

I landed this patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0499fb38a15
https://hg.mozilla.org/integration/mozilla-inbound/rev/c61b88b3479a

For some reason, it seems like web-platform-tests have two failure annotations. I changed testing/web-platform/meta/js/builtins/WeakMap.prototype-properties.html.ini to suppress the failure.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 9 years ago

> For some reason, it seems like web-platform-tests have two failure annotations

We have two copies of that test suite in our tree, basically, using slightly different harnesses.

In any case, the issue there is that those wpt tests are just wrong and should be fixed, right?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 20 • 9 years ago

Done: https://github.com/w3c/web-platform-tests/pull/1499

	James Graham [:jgraham]
	Comment 21 • 9 years ago

I'll start an update cycle for the web-platform-tests suite.

	Phil Ringnalda (:philor)
	Comment 22 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c61b88b3479a

	Florian Scholz (Open Web Docs)
	Comment 23 • 9 years ago

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/length
https://developer.mozilla.org/en-US/Firefox/Releases/37#JavaScript