[MERGE #5698 @jackhorton] Share more code/ideas between JsBuiltIns and 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.

Author: jackhorton

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")
+LSC_ERROR_MSG(1082, ERRNonSimpleParamListInStrictMode, "Illegal 'use strict' directive in function with non-simple parameter list")
 
 LSC_ERROR_MSG(1083, ERRBadAwait, "'await' expression not allowed in this context")
 

lib/Runtime/Base/JnDirectFields.h

@@ -645,6 +645,16 @@ ENTRY(toLength)
 ENTRY(toInteger)
 ENTRY(arraySpeciesCreate)
 ENTRY(arrayCreateDataPropertyOrThrow)
+ENTRY(Array_values)
+ENTRY(Array_keys)
+ENTRY(Array_entries)
+ENTRY(Array_indexOf)
+ENTRY(Array_filter)
+ENTRY(Array_flat)
+ENTRY(Array_flatMap)
+ENTRY(Array_forEach)
+ENTRY(Object_fromEntries)
+ENTRY(FunctionKind)
 
 // EngineInterfaceObject built-ins
 ENTRY(builtInJavascriptArrayEntryFilter)

lib/Runtime/ByteCode/ByteCodeCacheReleaseFileVersion.h

@@ -4,6 +4,6 @@
 //-------------------------------------------------------------------------------------------------------
 // NOTE: If there is a merge conflict the correct fix is to make a new GUID.
 
-// {A7B3E50E-9870-4A74-A155-58021B9B2202}
+// {BFA0BED0-7B1A-4018-83DA-B1D989D38BFD}
 const GUID byteCodeCacheReleaseFileVersion =
-{ 0xA7B3E50E, 0x9870, 0x4A74, { 0xA1, 0x55, 0x58, 0x02, 0x1B, 0x9B, 0x22, 0x02 } };
+{ 0xBFA0BED0, 0x7B1A, 0x4018, { 0x83, 0xDA, 0xB1, 0xD9, 0x89, 0xD3, 0x8B, 0xFD } };

lib/Runtime/Library/EngineInterfaceObject.cpp

@@ -267,47 +267,125 @@ namespace Js
         return scriptContext->GetLibrary()->GetUndefined();
     }
 
-    Var EngineInterfaceObject::Entry_TagPublicLibraryCode(RecyclableObject *function, CallInfo callInfo, ...)
+    /* static */
+    ScriptFunction *EngineInterfaceObject::CreateLibraryCodeScriptFunction(ScriptFunction *scriptFunction, JavascriptString *displayName, bool isConstructor, bool isJsBuiltIn, bool isPublic)
     {
-        EngineInterfaceObject_CommonFunctionProlog(function, callInfo);
+        if (scriptFunction->GetFunctionProxy()->IsPublicLibraryCode())
+        {
+            // this can happen when we re-initialize Intl for a different mode -- for instance, if we have the following JS:
+            // print((1).toLocaleString())
+            // print(new Intl.NumberFormat().format(1))
+            // Intl will first get initialized for Number, and then will get re-initialized for all of Intl. This will cause
+            // Number.prototype.toLocaleString to be registered twice, which breaks some of our assertions below.
+            return scriptFunction;
+        }
 
-        AssertOrFailFast((callInfo.Count == 3 || callInfo.Count == 4) && JavascriptFunction::Is(args[1]) && JavascriptString::Is(args[2]));
+        ScriptContext *scriptContext = scriptFunction->GetScriptContext();
 
-        JavascriptFunction *func = JavascriptFunction::UnsafeFromVar(args[1]);
-        JavascriptString *methodName = JavascriptString::UnsafeFromVar(args[2]);
+        if (!isConstructor)
+        {
+            // set the ErrorOnNew attribute to disallow construction. JsBuiltIn/Intl functions are usually regular ScriptFunctions
+            // (not lambdas or class methods), so they are usually constructable by default.
+            FunctionInfo *info = scriptFunction->GetFunctionInfo();
+            AssertMsg((info->GetAttributes() & FunctionInfo::Attributes::ErrorOnNew) == 0, "Why are we trying to disable construction of a function that already isn't constructable?");
+            info->SetAttributes((FunctionInfo::Attributes) (info->GetAttributes() | FunctionInfo::Attributes::ErrorOnNew));
 
-        func->GetFunctionProxy()->SetIsPublicLibraryCode();
+            // Assert that the type handler is deferred to ensure that we aren't overwriting previous modifications.
+            // Script functions start with deferred type handlers, which undefer as soon as any property is modified.
+            // Since the function that is passed in should be an inline function expression, its type should still be deferred by the time it gets here.
+            AssertOrFailFast(scriptFunction->GetDynamicType()->GetTypeHandler()->IsDeferredTypeHandler());
 
-        // use GetSz rather than GetString because we use wcsrchr below, which expects a null-terminated string
-        const char16 *methodNameBuf = methodName->GetSz();
-        charcount_t methodNameLength = methodName->GetLength();
-        const char16 *shortName = wcsrchr(methodNameBuf, _u('.'));
-        charcount_t shortNameOffset = 0;
-        if (shortName != nullptr)
+            // give the function a type handler with name and length but without prototype
+            DynamicTypeHandler::SetInstanceTypeHandler(scriptFunction, scriptContext->GetLibrary()->GetDeferredFunctionWithLengthTypeHandler());
+        }
+        else
         {
-            shortName++;
-            shortNameOffset = static_cast<charcount_t>(shortName - methodNameBuf);
+            AssertMsg((scriptFunction->GetFunctionInfo()->GetAttributes() & FunctionInfo::Attributes::ErrorOnNew) == 0, "Why is the function not constructable by default?");
         }
 
-        func->GetFunctionProxy()->EnsureDeserialized()->SetDisplayName(methodNameBuf, methodNameLength, shortNameOffset);
-
-        bool creatingConstructor = true;
-        if (callInfo.Count == 4)
+        if (isPublic)
         {
-            AssertOrFailFast(JavascriptBoolean::Is(args[3]));
-            creatingConstructor = JavascriptBoolean::UnsafeFromVar(args[3])->GetValue();
+            // Use GetSz rather than GetString because we use wcsrchr below, which expects a null-terminated string
+            // Callers can pass in a string like "get compare" or "Intl.Collator.prototype.resolvedOptions" -- only for the
+            // latter do we extract a shortName.
+            const char16 *methodNameBuf = displayName->GetSz();
+            charcount_t methodNameLength = displayName->GetLength();
+            const char16 *shortName = wcsrchr(methodNameBuf, _u('.'));
+            charcount_t shortNameOffset = 0;
+            if (shortName != nullptr)
+            {
+                shortName++;
+                shortNameOffset = static_cast<charcount_t>(shortName - methodNameBuf);
+            }
+
+            scriptFunction->GetFunctionProxy()->EnsureDeserialized()->SetDisplayName(methodNameBuf, methodNameLength, shortNameOffset);
+
+            // handle the name property AFTER handling isConstructor, because this can initialize the function's deferred type
+            Var existingName = nullptr;
+            if (JavascriptOperators::GetOwnProperty(scriptFunction, PropertyIds::name, &existingName, scriptContext, nullptr))
+            {
+                JavascriptString *existingNameString = JavascriptString::FromVar(existingName);
+                if (existingNameString->GetLength() == 0)
+                {
+                    // Only overwrite the name of the function object if it was anonymous coming in
+                    // If the input function was named, it is likely intentional
+                    existingName = nullptr;
+                }
+            }
+
+            if (existingName == nullptr || JavascriptOperators::IsUndefined(existingName))
+            {
+                // It is convenient to set the name here rather than in script, since it is often duplicated.
+                JavascriptString *funcName = displayName;
+                if (shortName)
+                {
+                    funcName = JavascriptString::NewCopyBuffer(shortName, methodNameLength - shortNameOffset, scriptContext);
+                }
+
+                scriptFunction->SetPropertyWithAttributes(PropertyIds::name, funcName, PropertyConfigurable, nullptr);
+            }
+
+            scriptFunction->GetFunctionProxy()->SetIsPublicLibraryCode();
         }
 
-        if (!creatingConstructor)
+        if (isJsBuiltIn)
         {
-            FunctionInfo *info = func->GetFunctionInfo();
-            info->SetAttributes((FunctionInfo::Attributes) (info->GetAttributes() | FunctionInfo::Attributes::ErrorOnNew));
+            scriptFunction->GetFunctionProxy()->SetIsJsBuiltInCode();
+
+            // This makes it so that the given scriptFunction can't reference/close over any outside variables,
+            // which is desirable for JsBuiltIns (though currently not for Intl)
+            scriptFunction->SetEnvironment(const_cast<FrameDisplay *>(&StrictNullFrameDisplay));
+
+            // TODO(jahorto): investigate force-inlining Intl code
+            scriptFunction->GetFunctionProxy()->EnsureDeserialized();
+            AssertOrFailFast(scriptFunction->HasFunctionBody());
+            scriptFunction->GetFunctionBody()->SetJsBuiltInForceInline();
+        }
+
+        return scriptFunction;
+    }
 
-            AssertOrFailFast(func->GetDynamicType()->GetTypeHandler()->IsDeferredTypeHandler());
-            DynamicTypeHandler::SetInstanceTypeHandler(func, scriptContext->GetLibrary()->GetDeferredFunctionWithLengthTypeHandler());
+    Var EngineInterfaceObject::Entry_TagPublicLibraryCode(RecyclableObject *function, CallInfo callInfo, ...)
+    {
+#pragma warning(push)
+#pragma warning(disable: 4189) // 'scriptContext': local variable is initialized but not referenced
+        EngineInterfaceObject_CommonFunctionProlog(function, callInfo);
+#pragma warning(pop)
+
+        AssertOrFailFast((args.Info.Count == 3 || args.Info.Count == 4) && ScriptFunction::Is(args.Values[1]) && JavascriptString::Is(args.Values[2]));
+
+        ScriptFunction *func = ScriptFunction::UnsafeFromVar(args[1]);
+        JavascriptString *methodName = JavascriptString::UnsafeFromVar(args[2]);
+
+        bool isConstructor = true;
+        if (args.Info.Count == 4)
+        {
+            AssertOrFailFast(JavascriptBoolean::Is(args.Values[3]));
+            isConstructor = JavascriptBoolean::UnsafeFromVar(args.Values[3])->GetValue();
         }
 
-        return func;
+        // isConstructor = true is the default (when no 3rd arg is provided)
+        return CreateLibraryCodeScriptFunction(func, methodName, isConstructor, false /* isJsBuiltIn */, true /* isPublic */);
     }
 
     /*

lib/Runtime/Library/EngineInterfaceObject.h

@@ -88,6 +88,8 @@ namespace Js
 
         static bool __cdecl InitializeCommonNativeInterfaces(DynamicObject* engineInterface, DeferredTypeHandlerBase * typeHandler, DeferredInitializeMode mode);
 
+        static ScriptFunction *CreateLibraryCodeScriptFunction(ScriptFunction *scriptFunction, JavascriptString *displayName, bool isConstructor, bool isJsBuiltIn, bool isPublic);
+
         class EntryInfo
         {
         public:

lib/Runtime/Library/EngineInterfaceObjectBuiltIns.h

@@ -50,7 +50,7 @@ GlobalBuiltInConstructor(Number)
 GlobalBuiltInConstructor(RegExp)
 GlobalBuiltInConstructor(String)
 GlobalBuiltInConstructor(Date)
-GlobalBuiltInConstructor(Error) // TODO(jahorto): consider deleting (all errors should go through builtInRaise*)
+GlobalBuiltInConstructor(Error) // TODO(jahorto): consider deleting (currently used by WinRT Promises)
 GlobalBuiltInConstructor(Map) // TODO(jahorto): consider deleting (when do we need a Map over an object?)
 GlobalBuiltInConstructor(Symbol)
 
@@ -66,6 +66,8 @@ GlobalBuiltIn(JavascriptObject, GetOwnPropertyNames)
 GlobalBuiltIn(JavascriptObject, HasOwnProperty)
 GlobalBuiltIn(JavascriptObject, Keys)
 GlobalBuiltIn(JavascriptObject, Create)
+GlobalBuiltIn(JavascriptObject, GetOwnPropertyDescriptor)
+GlobalBuiltIn(JavascriptObject, PreventExtensions)
 
 GlobalBuiltIn(JavascriptArray, Push) // TODO(jahorto): consider deleting (trivially implementable in JS)
 GlobalBuiltIn(JavascriptArray, Join)
@@ -90,7 +92,7 @@ GlobalBuiltIn(JavascriptString, IndexOf)
 
 GlobalBuiltIn(GlobalObject, IsFinite) // TODO(jahorto): consider switching to Number.isFinite
 GlobalBuiltIn(GlobalObject, IsNaN) // TODO(jahorto): consider switching to Number.isNaN
-GlobalBuiltIn(GlobalObject, Eval) // TODO(jahorto): consider deleting (should any builtins be using eval()?)
+GlobalBuiltIn(GlobalObject, Eval) // TODO(jahorto): consider deleting (currently used by WinRT Promises)
 
 BuiltInRaiseException(TypeError, NeedObject)
 BuiltInRaiseException2(TypeError, ObjectIsAlreadyInitialized)

lib/Runtime/Library/InJavascript/Intl.js

@@ -931,7 +931,7 @@
 
         const CollatorPrototype = {};
 
-        const Collator = tagPublicFunction("Intl.Collator", function Collator(locales = undefined, options = undefined) {
+        const Collator = tagPublicFunction("Intl.Collator", function (locales = undefined, options = undefined) {
             const newTarget = new.target === undefined ? Collator : new.target;
             const collator = OrdinaryCreateFromConstructor(newTarget, CollatorPrototype);
 
@@ -1026,12 +1026,6 @@
 
             return hiddenObject.boundCompare;
         });
-        _.defineProperty(getCompare, "name", {
-            value: "get compare",
-            writable: false,
-            enumerable: false,
-            configurable: true,
-        });
         _.defineProperty(CollatorPrototype, "compare", {
             get: getCompare,
             enumerable: false,