Share more code/ideas between JsBuiltIns and Intl #5698

jackhorton · 2018-09-12T07:40:02Z

Fixes #5644
Fixes #5643
Fixes an unlogged issue where all JsBuiltIns were constructable by default.

This mainly merged a lot of the code between tagPublicLibraryFunction, registerFunction, and registerChakraLibraryFunction. However, there are a number of smaller updates too:

Stopped creating a new ScriptFunction on each JsBuiltIn registration. As far as I can tell, the only reason this existed before was to get a ScriptFunction without any framedisplay/environment, but since that can be set manually, its cheaper to just set it on the existing ScriptFunction.
Intl functions no longer have to duplicate their name in the tagPublicLibraryCode argument and in script. I have edited a few Intl functions to confirm it works -- not sure if its worth it to go through and make all of the functions anonymous now. I also wanted to investigate changing JavascriptLibrary::InitializeFunction to not set the name attribute for these anonymous jsbuiltin functions since the name will always be overridden, but I am not sure if its possible.
function length is now set using default parameters rather than manually/after the fact. Not sure if I like this better or worse than having the function macro list the length and have it set it explicitly. We could theoretically do the same optimization in InitializeFunction to avoid the extra property set.
JsBuiltIns now uses a shared/projected enum for function registration like Intl.
Ran into an error when using default parameters for functions marked explicitly as "use strict," and I found the error message misleading, so I changed it to be the same as V8's which I found clearer.
CheckArrayAndGetLen never hit its error case and reported a slightly worse-looking runtime/accidental error as a result when this == null

Still need to run this through test262 to make sure there are no regressions.

jackhorton · 2018-09-12T16:15:50Z

Also cc @rhuanjl as the writer of so many of these builtins :)

MSLaguana · 2018-09-12T16:24:10Z

lib/Parser/perrors.h

@@ -88,7 +88,7 @@ LSC_ERROR_MSG( 1077, ERRDestructNotInit, "Destructuring declarations cannot have
 LSC_ERROR_MSG(1079, ERRInvalidNewTarget, "Invalid use of the 'new.target' keyword")
 LSC_ERROR_MSG(1080, ERRForInNoInitAllowed, "for-in loop head declarations cannot have an initializer")
 LSC_ERROR_MSG(1081, ERRForOfNoInitAllowed, "for-of loop head declarations cannot have an initializer")
-LSC_ERROR_MSG(1082, ERRNonSimpleParamListInStrictMode, "Cannot apply strict mode on functions with non-simple parameter list")


This change requires some baseline/test expectation updates it seems.

yeah, I had only tested the array folder before putting it up last night because I was tired.

MSLaguana · 2018-09-12T16:34:47Z

lib/Runtime/Library/StringCacheList.h

@@ -8,6 +8,7 @@ STRING(Quotes, _u("\"\""))
 STRING(Whack, _u("/"))
 STRING(CommaDisplay, _u(","))
 STRING(CommaSpaceDisplay, _u(", "))
+STRING(Dot, _u("."))


What uses this? I don't see any other references in this PR

its used when building up the full display string as library->GetDotString()

sigatrev

The more code we can share between these, the better.

rhuanjl · 2018-09-12T22:33:50Z

Thanks for tagging me.

This looks like some very nice cleanup - also nice to see the stack trace issues fixed.

sethbrenith

Penguinwizzard · 2018-09-13T20:28:52Z

lib/Runtime/Library/EngineInterfaceObject.cpp

+        if (args.Info.Count == 4)
+        {
+            AssertOrFailFast(JavascriptBoolean::Is(args.Values[3]));
+            isConstructor = JavascriptBoolean::UnsafeFromVar(args.Values[3])->GetValue();


Why not just use ::FromVar and drop the AssertOrFailFast ::Is checks in this function in general?

I think I got into a pattern with it in Intl as a modification of what was already there and then didn't think about it more. Combined with operator[] for ArgumentsReader, we could probably get rid of all of the arguments-related asserts... may make that a future PR?

Sure; can you put up an issue to have the failfast in ArgumentsReader operator[]? That'd be a pretty good change to have.

…k traces

…l/undefined

…or use by JsBuiltIns

…ctions, and allow __chakraLibrary functions to be constructors

…d Intl Merge pull request #5698 from jackhorton:jsbuiltins/cleanup Fixes #5644 Fixes #5643 Fixes an unlogged issue where all JsBuiltIns were constructable by default. This mainly merged a lot of the code between tagPublicLibraryFunction, registerFunction, and registerChakraLibraryFunction. However, there are a number of smaller updates too: 1. Stopped creating a new ScriptFunction on each JsBuiltIn registration. As far as I can tell, the only reason this existed before was to get a ScriptFunction without any framedisplay/environment, but since that can be set manually, its cheaper to just set it on the existing ScriptFunction. 1. Intl functions no longer have to duplicate their name in the tagPublicLibraryCode argument and in script. I have edited a few Intl functions to confirm it works -- not sure if its worth it to go through and make all of the functions anonymous now. I also wanted to investigate changing JavascriptLibrary::InitializeFunction to not set the name attribute for these anonymous jsbuiltin functions since the name will always be overridden, but I am not sure if its possible. 1. function length is now set using default parameters rather than manually/after the fact. Not sure if I like this better or worse than having the function macro list the length and have it set it explicitly. We could theoretically do the same optimization in InitializeFunction to avoid the extra property set. 1. JsBuiltIns now uses a shared/projected enum for function registration like Intl. 1. Ran into an error when using default parameters for functions marked explicitly as "use strict," and I found the error message misleading, so I changed it to be the same as V8's which I found clearer. 1. CheckArrayAndGetLen never hit its error case and reported a slightly worse-looking runtime/accidental error as a result when this == null Still need to run this through test262 to make sure there are no regressions.

dilijev · 2018-09-17T22:56:03Z

FWIW LGTM

jackhorton added the Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label label Sep 12, 2018

jackhorton requested review from sigatrev, pleath and meg-gupta September 12, 2018 07:40

MSLaguana reviewed Sep 12, 2018

View reviewed changes

jackhorton force-pushed the jsbuiltins/cleanup branch from c65e524 to 1380b6b Compare September 12, 2018 17:56

sigatrev approved these changes Sep 12, 2018

View reviewed changes

meg-gupta approved these changes Sep 12, 2018

View reviewed changes

sethbrenith approved these changes Sep 13, 2018

View reviewed changes

Penguinwizzard approved these changes Sep 13, 2018

View reviewed changes

jackhorton added 8 commits September 16, 2018 11:27

Functions registered to __chakraLibrary should not be visible in stac…

d47dbd3

…k traces

Update __chakraLibrary.CheckArrayAndGetLen to correctly check for nul…

be80d9b

…l/undefined

Factor public library script function logic out into its own method f…

06dd001

…or use by JsBuiltIns

Update JsBuiltIn register functions to use common logic

1c22ba5

Update JsBuiltIns to use similar functions enum approach as Intl

76275c2

Don't mess with the function name/displayname for __chakraLibrary fun…

c523fea

…ctions, and allow __chakraLibrary functions to be constructors

Add new unit tests for updated JsBuiltIn functionality

4f45e8a

Fix test failures

ffe7c01

jackhorton force-pushed the jsbuiltins/cleanup branch from 1380b6b to 5625176 Compare September 16, 2018 19:54

jackhorton added 3 commits September 17, 2018 10:26

Add back properties to EngineInterface that were mistakenly removed

1b1452a

Update bytecode

1a62c8f

Update bytecode version

0accefd

jackhorton force-pushed the jsbuiltins/cleanup branch from 5625176 to 0accefd Compare September 17, 2018 18:13

chakrabot merged commit 0accefd into chakra-core:master Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share more code/ideas between JsBuiltIns and Intl #5698

Share more code/ideas between JsBuiltIns and Intl #5698

jackhorton commented Sep 12, 2018

jackhorton commented Sep 12, 2018

MSLaguana Sep 12, 2018

jackhorton Sep 12, 2018

MSLaguana Sep 12, 2018

jackhorton Sep 12, 2018

sigatrev left a comment

rhuanjl commented Sep 12, 2018

sethbrenith left a comment

Penguinwizzard Sep 13, 2018

jackhorton Sep 13, 2018

Penguinwizzard Sep 13, 2018

dilijev commented Sep 17, 2018

Share more code/ideas between JsBuiltIns and Intl #5698

Share more code/ideas between JsBuiltIns and Intl #5698

Conversation

jackhorton commented Sep 12, 2018

jackhorton commented Sep 12, 2018

MSLaguana Sep 12, 2018

Choose a reason for hiding this comment

jackhorton Sep 12, 2018

Choose a reason for hiding this comment

MSLaguana Sep 12, 2018

Choose a reason for hiding this comment

jackhorton Sep 12, 2018

Choose a reason for hiding this comment

sigatrev left a comment

Choose a reason for hiding this comment

rhuanjl commented Sep 12, 2018

sethbrenith left a comment

Choose a reason for hiding this comment

Penguinwizzard Sep 13, 2018

Choose a reason for hiding this comment

jackhorton Sep 13, 2018

Choose a reason for hiding this comment

Penguinwizzard Sep 13, 2018

Choose a reason for hiding this comment

dilijev commented Sep 17, 2018