Fix configurability of function.length and boundFunction.length #5405

rhuanjl · 2018-06-30T17:17:08Z

This PR addresses several issues with the length property of JavascriptFunctions and BoundFunctions.

Length should be configurable (previously worked for JavascriptFunctions but not BoundFunctions)
Deleting length should succeed (previously failed for both)
Deleting length should not throw in strict mode (previously failed for both)
value after deletion = 0 (previously failed for both)
using defineProperty to redefine length should work (previously failed silently for both)

rhuanjl · 2018-06-30T17:17:51Z

test/es6/ES6Function_bugs.js

@@ -28,7 +28,7 @@ var tests = [
                    get: function () { }
                    });
            }
-            assert.doesNotThrow(function () { g('length') }, "assertion failure on defineProperty 'length' with getter after sealing a function object");
+            assert.throws(function () { g('length') }, TypeError, "Cannot redefine non-configurable property 'length'");
            assert.throws(function () { g('arguments') }, TypeError, "Cannot redefine non-configurable property 'arguments'");


This test was testing incorrect behavior see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/seal

pleath · 2018-06-30T19:04:42Z

I really like the idea of having Function.length participate in the type system in these cases. Since you ask -- yes, there is an existing model for lazily instantiating properties of function objects. See DeferredTypeHandler::EnsureObjectReady, DeferredTypeHandler::ConvertFunction, and JavascriptLibrary::InitializeFunction. These make use of the SimpleTypeHandler's in the JavascriptLibrary code. Someone can correct me, but it looks like we've defined function object type handlers for length only, length-and-name, and prototype-and-length variants, but not prototype-length-and-name. I wonder if we added length to the handler variant for user functions and removed length from the set of "special properties" if we'd get what we want.

rhuanjl · 2018-06-30T23:18:00Z

lib/Runtime/Library/BoundFunction.cpp

+        len = len - this->count;
+        len = max(len, 0);
+
+        SetPropertyWithAttributes(PropertyIds::length, TaggedInt::ToVarUnchecked(len), PropertyConfigurable, nullptr, PropertyOperation_None, SideEffects_None);
    }

    BoundFunction::BoundFunction(RecyclableObject* targetFunction, Var boundThis, Var* args, uint argsCount, DynamicType * type)


Haven't worked out the purpose of this alternate constructor - unsure if I need to add length handling in here as well.

I suspect that this is dead code now; certainly chakracore doesn't reference it at all, and I also can't find any references in chakrafull either.

rhuanjl · 2018-07-01T16:36:08Z

Looked at this more, got a few further problems/questions:

Why are some of the types defined here:
https://github.com/Microsoft/ChakraCore/blob/fc08987381da141bb686b5d0c71d75da96f9eb8a/lib/Runtime/Library/JavascriptLibrary.cpp#L52-L60
Given length properties when the existing special casing in JavascriptFunction.cpp would normally block access to those properties. I'm wondering if I'm missing something here. And whether just updating these types (then initialising the length property on creation) will sort several problems.
Why does the dumpFunctionProperties.js debugger test expect one of the functions to have a negative length? I can't see how that can be right unless it is overwritten with Object.defineProperty (which the test doesn't do)

(Sorry about the questions, I thought this would be easier to solve than it was)

pleath · 2018-07-01T17:43:05Z

For question 1, it does seem inconsistent. I'd be in favor of eliminating the special handling of Function.length, instantiating it in JavascriptLibrary::InitializeFunction (if present) the way we do name and prototype, and setting up the shared SimpleTypeHandler's to support it. For question 2, sorry, I don't know. I can't tell where that length value is supposed to be coming from just by eyeballing the test, but it certainly seems as though the baseline may be wrong.

jackhorton

I don't have a ton to say on the content here, but anything to fix all of that special casing for PropertyIds::length in Function and BoundFunction code is 👍 from me.

jackhorton · 2018-07-02T16:00:39Z

lib/Runtime/Library/JavascriptLibrary.cpp

        {
-            return true;
+            function->SetPropertyWithAttributes(PropertyIds::length, TaggedInt::ToVarUnchecked(function->GetFunctionProxy()->EnsureDeserialized()->GetReportedInParamsCount() - 1), PropertyConfigurable, nullptr, PropertyOperation_None, SideEffects_None);
+            if ((useAnonymous || scriptFunction->GetFunctionProxy()->EnsureDeserialized()->GetIsStaticNameFunction()))


Cache scriptFunction->GetFunctionProxy()->EnsureDeserialized() somewhere? The compiler may be smart enough to cache this, but it would make these two lines look a bit nicer.

rhuanjl · 2018-07-02T20:47:59Z

I think I have this right now, thank you for the help @pleath

This could obviously do with a fairly thorough review nervous that some of the types are used in numerous places and there's a chance I missed something.

This change now:

Makes it so all normal functions created from within javascript have a length property as a normal property handled via the the type system.
Removes all special casing for length from JavascriptFunction.cpp as the property is now handled by the type handler.
Makes Bound Functions have a length added as a normal property in their constructor, all other special casing for length is then removed from BoundFunction.cpp
I have tried to ensure that this change does not effect:
- JS library functions (these mostly have length properties set immediately after they're created already)
- Functions created by a host (currently these have no length unless the host adds one - I assume that changing that could be a compat break)
- Generator and async functions - I think these would benefit from extending this change to them but would rather leave that to be a separate PR later as this is big enough already IMO

jackhorton · 2018-07-02T22:49:22Z

lib/Runtime/Library/JavascriptLibrary.cpp

        }
        else
        {
-            typeHandler->ConvertFunction(function, useAnonymous ? javascriptLibrary->anonymousFunctionWithPrototypeTypeHandler : javascriptLibrary->functionWithPrototypeTypeHandler);
+            typeHandler->ConvertFunction(function, useAnonymous ? javascriptLibrary->anonymousFunctionWithPrototypeTypeHandler : 
+                useLengthType ? javascriptLibrary->functionWithPrototypeAndLengthTypeHandler : javascriptLibrary->functionWithPrototypeTypeHandler);


nit: double ternaries can be pretty hard to read.

jackhorton · 2018-07-02T22:49:28Z

lib/Runtime/Library/BoundFunction.cpp

+
+        // Reduce length number of bound args
+        len = len - this->count;
+        len = max(len, 0);


Could you add a test for this case (where the target function is over-bound)?

Done. Though i’ve also realised that i’m missing tests for anonymous functions which take a slightly different path. I’ll add those later and also try and refactor the testfile to remove some of the duplication.

pleath · 2018-07-03T15:39:44Z

Thanks, @rhuanjl, I’ll give this a close look this week.

pleath

Got to love the deletion of special cases. Thanks.

rhuanjl · 2018-07-07T10:01:01Z

Thanks for the reviews @jackhorton @pleath Does this need anything else or is it good to merge? If it's good to merge please can one of you pick up?

rhuanjl · 2018-07-17T22:31:52Z

Is there anything more I need to do on this for it to be merged?

jackhorton · 2018-07-17T22:48:15Z

sorry @rhuanjl , I will run this through internal tests and get it merged tonight. sorry for the delay!

jackhorton · 2018-07-17T23:50:58Z

This fails a number of ChakraFull tests, almost certainly all of which are baseline updates but I will report back with more tomorrow.

rhuanjl · 2018-07-19T08:46:12Z

I suppose this is the problem with making a low-is level change as an external contributor when I can't look at the output of internal tests.

Question will be whether they were testing incorrect behaviour e.g. expecting deleting a length to fail OR whether I've inadvertently changed something else e.g. an early version of this change (which I didn't submit) would have added a length property to all host created functions (which currently don't have a length by default).

jackhorton · 2018-07-19T16:04:19Z

It looks like the majority of the tests are because the enumeration order of the length and name properties changed. I think that is implementation defined but I am not sure if people are relying on a certain order. I have all of the baselines fixed, theres just one more native JSRT test in chakrafull that is failing and I can't tell why, but I hope to have that resolved today.

MSLaguana · 2018-07-19T16:06:40Z

The enumeration order seems to be implementation defined; at least node-v8 and node-chakracore report them in different orders.

pleath · 2018-07-19T16:24:45Z

Historically, we’ve caused problems for people when we change enumeration order. People do take dependencies on implementation-defined behavior. So I’d prefer to preserve the original order if we can do that and still get the benefit of this change.

rhuanjl · 2018-07-19T16:28:45Z

I'll take a look at where I've changed enumeration order and see if I can change it back (as obviously it's not changed in a way that effects any CC tests at the moment).

@jackhorton if you could give me any details on the other failing test and what javascriptlibrary.cpp / javascriptfunction.cpp functions it calls I'll see if I can work out what could have changed.

rhuanjl · 2018-08-11T10:44:10Z

@jackhorton is there any update on this/anything I can do to help get it merged? is the failing test something that could be ported to chakracore so I could investigate it?

jackhorton · 2018-08-17T03:45:00Z

lib/Runtime/Library/JavascriptLibrary.cpp

@@ -924,7 +978,7 @@ namespace Js

    DynamicType * JavascriptLibrary::CreateDeferredPrototypeFunctionType(JavascriptMethod entrypoint)
    {
-        return CreateDeferredPrototypeFunctionTypeNoProfileThunk(this->inDispatchProfileMode ? ProfileEntryThunk : entrypoint);
+        return CreateDeferredPrototypeFunctionTypeNoProfileThunk(this->inDispatchProfileMode ? ProfileEntryThunk : entrypoint, true);


FINALLY got some real time to look at this, and this line was the problem for the chakrafull jsrt test. Do you remember what the goal was here? It doesn't seem related to the rest of the PR, and reverting the line causes the tests to pass (don't feel the need to make the change yourself if its ok to revert, I will push my own that's easier to sync with the chakrafull test changes I have already made).

This change should be reverted.

It's not doing what it was meant to and isn't needed anyway - I had intended to give this type a length (which is not needed) and this is wrong for doing that anyway would have needed to be false, true rather than just true.

Looking back over this my only remaining concern is function naming - this PR largely makes the default for DeferredFunction types be that they have a length property. This one is left without a length property and naming wise nothing making clear that it doesn't have one.

Wondering if I should rename some of the other CreateDeferred...FunctionType methods to make it explicit when types do have lengths. That could be a follow up PR if that would be simpler for syncing with your chakra full changes?

I think we can leave giving these lengths turned off for now and test it when development ramps back up for the next windows release (since these include projected and custom external functions, we would probably have to make the change based on the results of our chakrafull test suite, so that will be on us). As for the name, it may be confusing, but since the goal is to have lengths be enabled by default everywhere in the future, I think its fine to be in this state for now (especially since it was already inconsistent before, so its not a regression). @pleath sounds like a plan?

I'm fine with reverting the one line, pushing what we have, and adjusting the names as a separate PR, if that's what you're suggesting.

rhuanjl · 2018-08-17T12:03:12Z

Looking back over this, wondering if to avoid any undetected problems I/we should revert the changes I’ve made to other types and add new types instead?

(Pressed close by mistake)

rhuanjl · 2018-08-17T16:53:46Z

@jackhorton based on pleath’s comment do you want to land this as is?

I’ll follow up with another PR to fix up names of some methods and also correct remaining errors with generator and async function length.

jackhorton · 2018-08-17T17:22:54Z

Sure. Re-running internal tests now

jackhorton · 2018-08-17T20:34:02Z

I think this is based on a commit with a corresponding chakrafull commit that is flaky, I am seeing random test issues. I am going to force-push this branch to be rebased on top of latest master which should help.

Merge pull request #5573 from rhuanjl:flat This PR implements the stage 3 proposal Array methods: Array.prototype.flat and Array.prototype.flatMap as JsBuiltIns. Draft specification text is here: https://tc39.github.io/proposal-flatMap/ Notes: 1. 3 test262 failures: - length is not configurable (for both methods), this is due to ScriptFunction lengths in CC all not being configurable and should be fixed if #5405 is merged - the case using null as this fails -- EDIT: FIXED 2. I can't find a feasible way to test the length is too big error - not sure if CC can actually handle arrays that big anyway so maybe not worth implementing that case? 3. Should probably have a few more CI tests Fixes #5543

jackhorton · 2018-08-17T23:27:19Z

@rhuanjl One more issue came up from the latest round of internal tests that I missed before -- the ChakraFull debug system was adding its own version of the length property to the list of properties to show in the debugger, which caused it to show up twice. I updated the branch to be on top of the latest master with the debugger fix -- Id like for @kfarnung and/or @akroshg to look at the change in DiagObjectModel.cpp to make sure its valid.

jackhorton · 2018-08-17T23:29:06Z

lib/Runtime/Debug/DiagObjectModel.cpp

@@ -2563,11 +2562,6 @@ namespace Js
                                        InsertItem(originalObject, object, propertyId, isConst, isUnscoped, &pMethodsGroupWalker);
                                    }
                                }
-                                else if ((jsFunction->IsScriptFunction() && !jsFunction->GetFunctionProxy()->IsJsBuiltInCode()) || jsFunction->IsBoundFunction())
-                                {
-                                    // Adding special property length for the ScriptFunction, like it is done in JavascriptFunction::GetSpecialNonEnumerablePropertyName


This function it references doesn't seem to exist anymore, unless my code search tools are blatantly lying to me.

It is hard to remember - but I think only one I can find is JavascriptRegExpConstructor::GetSpecialNonEnumerablePropertyName
so that means this may have been removed.

Last commit LGTM.

Remove all special casing of length from BoundFunction.cpp and JavascriptFunction.cpp As a result of above configurability of function.length and boundFunction.length

…uctor

…ateDeferredPrototypeFunctionType

…ndFunction.length Merge pull request #5405 from rhuanjl:boundFunction Fix: #4122 This PR addresses several issues with the length property of JavascriptFunctions and BoundFunctions. Length should be configurable (previously worked for JavascriptFunctions but not BoundFunctions) Deleting length should succeed (previously failed for both) Deleting length should not throw in strict mode (previously failed for both) value after deletion = 0 (previously failed for both) using defineProperty to redefine length should work (previously failed silently for both)

jackhorton · 2018-08-20T01:07:01Z

@rhuanjl sorry for all of the delays here, and thanks again for this! Its been a long-standing compat issue for us.

pleath · 2018-08-20T16:51:39Z

Very cool. This is the kind of improvement that's available to us now with our new type-sharing model.

@pleath

…urable length Merge pull request #5610 from rhuanjl:generatorLength Follow up to #5405 Generator and async functions should also have configurable lengths - currently they don't. Specific test262 tests: https://github.com/tc39/test262/blob/master/test/built-ins/AsyncFunction/instance-length.js https://github.com/tc39/test262/blob/master/test/built-ins/GeneratorFunction/instance-length.js Additionally the second commit restores some consistency to function type handler names (I'd unintentionally made these inconsistent in #5405) it also prevents some internal anonymous functions from unnecessarily being given a length property - unintentional side effect of #5405. fixes: #5419 CC: @pleath @jackhorton

rhuanjl commented Jun 30, 2018

View reviewed changes

rhuanjl force-pushed the boundFunction branch 2 times, most recently from 3d2b284 to 3b2262e Compare June 30, 2018 17:42

rhuanjl changed the title ~~Fix function.length and boundFunction.length~~ Fix configurability of function.length and boundFunction.length Jun 30, 2018

rhuanjl commented Jun 30, 2018

View reviewed changes

rhuanjl changed the title ~~Fix configurability of function.length and boundFunction.length~~ WIP: Fix configurability of function.length and boundFunction.length Jul 1, 2018

dilijev added the External Contributor label Jul 1, 2018

jackhorton reviewed Jul 2, 2018

View reviewed changes

rhuanjl force-pushed the boundFunction branch from 4686d60 to 70099d6 Compare July 2, 2018 18:15

rhuanjl changed the title ~~WIP: Fix configurability of function.length and boundFunction.length~~ Fix configurability of function.length and boundFunction.length Jul 2, 2018

jackhorton approved these changes Jul 2, 2018

View reviewed changes

rhuanjl force-pushed the boundFunction branch from b33562a to 2576996 Compare July 3, 2018 07:28

rhuanjl force-pushed the boundFunction branch from 2576996 to a1a3401 Compare July 3, 2018 19:20

rhuanjl mentioned this pull request Jul 3, 2018

Async/Generator function lengths can't be configured #5419

Closed

pleath approved these changes Jul 5, 2018

View reviewed changes

kfarnung approved these changes Jul 8, 2018

View reviewed changes

rhuanjl force-pushed the boundFunction branch from a1a3401 to 14a9be6 Compare August 4, 2018 23:25

jackhorton reviewed Aug 17, 2018

View reviewed changes

jackhorton force-pushed the boundFunction branch 2 times, most recently from a1a3401 to 3a32201 Compare August 17, 2018 07:00

rhuanjl closed this Aug 17, 2018

rhuanjl reopened this Aug 17, 2018

jackhorton force-pushed the boundFunction branch from 3a32201 to 148ffba Compare August 17, 2018 23:23

jackhorton reviewed Aug 17, 2018

View reviewed changes

rhuanjl and others added 4 commits August 17, 2018 22:46

Implement function.length in typesystem

c1eb8c5

Remove all special casing of length from BoundFunction.cpp and JavascriptFunction.cpp As a result of above configurability of function.length and boundFunction.length

Delete dead code - unused (and broken) secondary BoundFunction constr…

eaf98b3

…uctor

Revert accidental enabling of shared types for JavascriptLibrary::Cre…

673a44b

…ateDeferredPrototypeFunctionType

Remove special handling of function.length in RecyclableObjectWalker

cbada71

jackhorton force-pushed the boundFunction branch from 148ffba to cbada71 Compare August 19, 2018 05:28

chakrabot merged commit cbada71 into chakra-core:master Aug 20, 2018

rhuanjl deleted the boundFunction branch August 20, 2018 05:37

rhuanjl mentioned this pull request Aug 20, 2018

Make Generator and Async functions have configurable length #5610

Merged

This was referenced Aug 25, 2018

Fix a number of Intl-related GitHub and test262 issues, add support for ICU 62.1 #5612

Merged

Functions 'length' is configurable but can't be deleted. #5646

Closed

rhuanjl mentioned this pull request Sep 12, 2019

Wrong Realization of Function.length #6282

Closed

Fix configurability of function.length and boundFunction.length #5405

Fix configurability of function.length and boundFunction.length #5405

Conversation

rhuanjl commented Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

pleath commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Jul 1, 2018

pleath commented Jul 1, 2018

jackhorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleath commented Jul 3, 2018

pleath left a comment

Choose a reason for hiding this comment

rhuanjl commented Jul 7, 2018

rhuanjl commented Jul 17, 2018

jackhorton commented Jul 17, 2018

jackhorton commented Jul 17, 2018

rhuanjl commented Jul 19, 2018

jackhorton commented Jul 19, 2018

MSLaguana commented Jul 19, 2018

pleath commented Jul 19, 2018

rhuanjl commented Jul 19, 2018

rhuanjl commented Aug 11, 2018

Choose a reason for hiding this comment

rhuanjl Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Aug 17, 2018 • edited Loading

rhuanjl commented Aug 17, 2018

jackhorton commented Aug 17, 2018

jackhorton commented Aug 17, 2018

jackhorton commented Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackhorton commented Aug 20, 2018

pleath commented Aug 20, 2018

rhuanjl commented Jun 30, 2018 •

edited

Loading

rhuanjl Aug 17, 2018 •

edited

Loading

rhuanjl commented Aug 17, 2018 •

edited

Loading

jackhorton commented Aug 17, 2018 •

edited

Loading