Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 20, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c1eb8c5
Implement function.length in typesystem
rhuanjl Jun 30, 2018
eaf98b3
Delete dead code - unused (and broken) secondary BoundFunction constr…
rhuanjl Jul 3, 2018
673a44b
Revert accidental enabling of shared types for JavascriptLibrary::Cre…
jackhorton Aug 17, 2018
cbada71
Remove special handling of function.length in RecyclableObjectWalker
jackhorton Aug 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions lib/Runtime/Debug/DiagObjectModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2547,7 +2547,6 @@ namespace Js
// We need to special-case RegExp constructor here because it has some special properties (above) and some
// special enumerable properties which should all show up in the debugger.
JavascriptRegExpConstructor* regExp = scriptContext->GetLibrary()->GetRegExpConstructor();
Js::JavascriptFunction* jsFunction = Js::JavascriptFunction::FromVar(object);

if (regExp == object)
{
Expand All @@ -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
Copy link
Contributor

@jackhorton jackhorton Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@akroshg akroshg Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@akroshg akroshg Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last commit LGTM.

InsertItem(originalObject, object, PropertyIds::length, true/*not editable*/, false /*isUnscoped*/, &pMethodsGroupWalker);
}
}
}

Expand Down Expand Up @@ -4216,4 +4210,4 @@ namespace Js
}
#endif
}
#endif
#endif
191 changes: 13 additions & 178 deletions lib/Runtime/Library/BoundFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ namespace Js
count(0),
boundArgs(nullptr)
{

DebugOnly(VerifyEntryPoint());
AssertMsg(args.Info.Count > 0, "wrong number of args in BoundFunction");

Expand All @@ -44,21 +43,19 @@ namespace Js
}
type->SetPrototype(proto);
}

int len = 0;
// If targetFunction is proxy, need to make sure that traps are called in right order as per 19.2.3.2 in RC#4 dated April 3rd 2015.
// Here although we won't use value of length, this is just to make sure that we call traps involved with HasOwnProperty(Target, "length") and Get(Target, "length")
if (JavascriptProxy::Is(targetFunction))
// additionally need to get the correct length value for the boundFunctions' length property
if (JavascriptOperators::HasOwnProperty(targetFunction, PropertyIds::length, scriptContext, nullptr) == TRUE)
{
if (JavascriptOperators::HasOwnProperty(targetFunction, PropertyIds::length, scriptContext, nullptr) == TRUE)
Var varLength;
if (targetFunction->GetProperty(targetFunction, PropertyIds::length, &varLength, nullptr, scriptContext))
{
int len = 0;
Var varLength;
if (targetFunction->GetProperty(targetFunction, PropertyIds::length, &varLength, nullptr, scriptContext))
{
len = JavascriptConversion::ToInt32(varLength, scriptContext);
}
len = JavascriptConversion::ToInt32(varLength, scriptContext);
}
GetTypeHandler()->EnsureObjectReady(this);
}
GetTypeHandler()->EnsureObjectReady(this);

if (args.Info.Count > 1)
{
Expand All @@ -84,27 +81,12 @@ namespace Js
// If no "this" is passed, "undefined" is used
boundThis = scriptContext->GetLibrary()->GetUndefined();
}
}

BoundFunction::BoundFunction(RecyclableObject* targetFunction, Var boundThis, Var* args, uint argsCount, DynamicType * type)
: JavascriptFunction(type, &functionInfo),
count(argsCount),
boundArgs(nullptr)
{
DebugOnly(VerifyEntryPoint());

this->targetFunction = targetFunction;
this->boundThis = boundThis;

if (argsCount != 0)
{
this->boundArgs = RecyclerNewArray(this->GetScriptContext()->GetRecycler(), Field(Var), argsCount);

for (uint i = 0; i < argsCount; i++)
{
this->boundArgs[i] = args[i];
}
}
// Reduce length number of bound args
len = len - this->count;
len = max(len, 0);

SetPropertyWithAttributes(PropertyIds::length, TaggedInt::ToVarUnchecked(len), PropertyConfigurable, nullptr, PropertyOperation_None, SideEffects_None);
}

BoundFunction* BoundFunction::New(ScriptContext* scriptContext, ArgumentReader args)
Expand Down Expand Up @@ -307,108 +289,11 @@ namespace Js
return false;
}

PropertyQueryFlags BoundFunction::HasPropertyQuery(PropertyId propertyId, _Inout_opt_ PropertyValueInfo* info)
{
if (propertyId == PropertyIds::length)
{
return PropertyQueryFlags::Property_Found;
}

return JavascriptFunction::HasPropertyQuery(propertyId, info);
}

PropertyQueryFlags BoundFunction::GetPropertyQuery(Var originalInstance, PropertyId propertyId, Var* value, PropertyValueInfo* info, ScriptContext* requestContext)
{
BOOL result;
if (GetPropertyBuiltIns(originalInstance, propertyId, value, info, requestContext, &result))
{
return JavascriptConversion::BooleanToPropertyQueryFlags(result);
}

return JavascriptFunction::GetPropertyQuery(originalInstance, propertyId, value, info, requestContext);
}

PropertyQueryFlags BoundFunction::GetPropertyQuery(Var originalInstance, JavascriptString* propertyNameString, Var* value, PropertyValueInfo* info, ScriptContext* requestContext)
{
BOOL result;
PropertyRecord const* propertyRecord;
this->GetScriptContext()->FindPropertyRecord(propertyNameString, &propertyRecord);

if (propertyRecord != nullptr && GetPropertyBuiltIns(originalInstance, propertyRecord->GetPropertyId(), value, info, requestContext, &result))
{
return JavascriptConversion::BooleanToPropertyQueryFlags(result);
}

return JavascriptFunction::GetPropertyQuery(originalInstance, propertyNameString, value, info, requestContext);
}

bool BoundFunction::GetPropertyBuiltIns(Var originalInstance, PropertyId propertyId, Var* value, PropertyValueInfo* info, ScriptContext* requestContext, BOOL* result)
{
if (propertyId == PropertyIds::length)
{
// Get the "length" property of the underlying target function
int len = 0;
Var varLength;
if (targetFunction->GetProperty(targetFunction, PropertyIds::length, &varLength, nullptr, requestContext))
{
len = JavascriptConversion::ToInt32(varLength, requestContext);
}

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

*value = JavascriptNumber::ToVar(len, requestContext);
*result = true;
return true;
}

return false;
}

PropertyQueryFlags BoundFunction::GetPropertyReferenceQuery(Var originalInstance, PropertyId propertyId, Var* value, PropertyValueInfo* info, ScriptContext* requestContext)
{
return BoundFunction::GetPropertyQuery(originalInstance, propertyId, value, info, requestContext);
}

BOOL BoundFunction::SetProperty(PropertyId propertyId, Var value, PropertyOperationFlags flags, PropertyValueInfo* info)
{
BOOL result;
if (SetPropertyBuiltIns(propertyId, value, flags, info, &result))
{
return result;
}

return JavascriptFunction::SetProperty(propertyId, value, flags, info);
}

BOOL BoundFunction::SetProperty(JavascriptString* propertyNameString, Var value, PropertyOperationFlags flags, PropertyValueInfo* info)
{
BOOL result;
PropertyRecord const* propertyRecord;
this->GetScriptContext()->FindPropertyRecord(propertyNameString, &propertyRecord);

if (propertyRecord != nullptr && SetPropertyBuiltIns(propertyRecord->GetPropertyId(), value, flags, info, &result))
{
return result;
}

return JavascriptFunction::SetProperty(propertyNameString, value, flags, info);
}

bool BoundFunction::SetPropertyBuiltIns(PropertyId propertyId, Var value, PropertyOperationFlags flags, PropertyValueInfo* info, BOOL* result)
{
if (propertyId == PropertyIds::length)
{
JavascriptError::ThrowCantAssignIfStrictMode(flags, this->GetScriptContext());

*result = false;
return true;
}

return false;
}

_Check_return_ _Success_(return) BOOL BoundFunction::GetAccessors(PropertyId propertyId, _Outptr_result_maybenull_ Var* getter, _Outptr_result_maybenull_ Var* setter, ScriptContext* requestContext)
{
return DynamicObject::GetAccessors(propertyId, getter, setter, requestContext);
Expand All @@ -429,56 +314,6 @@ namespace Js
return SetProperty(propertyId, value, PropertyOperation_None, info);
}

BOOL BoundFunction::DeleteProperty(PropertyId propertyId, PropertyOperationFlags flags)
{
if (propertyId == PropertyIds::length)
{
return false;
}

return JavascriptFunction::DeleteProperty(propertyId, flags);
}

BOOL BoundFunction::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags)
{
if (BuiltInPropertyRecords::length.Equals(propertyNameString))
{
return false;
}

return JavascriptFunction::DeleteProperty(propertyNameString, flags);
}

BOOL BoundFunction::IsWritable(PropertyId propertyId)
{
if (propertyId == PropertyIds::length)
{
return false;
}

return JavascriptFunction::IsWritable(propertyId);
}

BOOL BoundFunction::IsConfigurable(PropertyId propertyId)
{
if (propertyId == PropertyIds::length)
{
return false;
}

return JavascriptFunction::IsConfigurable(propertyId);
}

BOOL BoundFunction::IsEnumerable(PropertyId propertyId)
{
if (propertyId == PropertyIds::length)
{
return false;
}

return JavascriptFunction::IsEnumerable(propertyId);
}

BOOL BoundFunction::HasInstance(Var instance, ScriptContext* scriptContext, IsInstInlineCache* inlineCache)
{
return this->targetFunction->HasInstance(instance, scriptContext, inlineCache);
Expand Down
13 changes: 1 addition & 12 deletions lib/Runtime/Library/BoundFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,31 +19,20 @@ namespace Js
protected:
BoundFunction(DynamicType * type);
BoundFunction(Arguments args, DynamicType * type);
BoundFunction(RecyclableObject* targetFunction, Var boundThis, Var* args, uint argsCount, DynamicType *type);

public:
static BoundFunction* New(ScriptContext* scriptContext, ArgumentReader args);

static bool Is(Var func){ return JavascriptFunction::Is(func) && JavascriptFunction::UnsafeFromVar(func)->IsBoundFunction(); }
static Var NewInstance(RecyclableObject* function, CallInfo callInfo, ...);
virtual JavascriptString* GetDisplayNameImpl() const override;
virtual PropertyQueryFlags HasPropertyQuery(PropertyId propertyId, _Inout_opt_ PropertyValueInfo* info) override;
virtual PropertyQueryFlags GetPropertyQuery(Var originalInstance, PropertyId propertyId, Var* value, PropertyValueInfo* info, ScriptContext* requestContext) override;
virtual PropertyQueryFlags GetPropertyQuery(Var originalInstance, JavascriptString* propertyNameString, Var* value, PropertyValueInfo* info, ScriptContext* requestContext) override;
virtual PropertyQueryFlags GetPropertyReferenceQuery(Var originalInstance, PropertyId propertyId, Var* value, PropertyValueInfo* info, ScriptContext* requestContext) override;
virtual BOOL SetProperty(PropertyId propertyId, Var value, PropertyOperationFlags flags, PropertyValueInfo* info) override;
virtual BOOL SetProperty(JavascriptString* propertyNameString, Var value, PropertyOperationFlags flags, PropertyValueInfo* info) override;

_Check_return_ _Success_(return) virtual BOOL GetAccessors(PropertyId propertyId, _Outptr_result_maybenull_ Var* getter, _Outptr_result_maybenull_ Var* setter, ScriptContext* requestContext) override;
virtual DescriptorFlags GetSetter(PropertyId propertyId, Var *setterValue, PropertyValueInfo* info, ScriptContext* requestContext) override;
virtual DescriptorFlags GetSetter(JavascriptString* propertyNameString, Var *setterValue, PropertyValueInfo* info, ScriptContext* requestContext) override;

virtual BOOL InitProperty(PropertyId propertyId, Var value, PropertyOperationFlags flags = PropertyOperation_None, PropertyValueInfo* info = NULL) override;
virtual BOOL DeleteProperty(PropertyId propertyId, PropertyOperationFlags flags) override;
virtual BOOL DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags) override;

virtual BOOL IsWritable(PropertyId propertyId) override;
virtual BOOL IsConfigurable(PropertyId propertyId) override;
virtual BOOL IsEnumerable(PropertyId propertyId) override;
virtual BOOL HasInstance(Var instance, ScriptContext* scriptContext, IsInstInlineCache* inlineCache = NULL) override;
virtual inline BOOL IsConstructor() const override;

Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/JavascriptExternalFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ namespace Js

bool __cdecl JavascriptExternalFunction::DeferredLengthInitializer(DynamicObject * instance, DeferredTypeHandlerBase * typeHandler, DeferredInitializeMode mode)
{
Js::JavascriptLibrary::InitializeFunction<true>(instance, typeHandler, mode);
Js::JavascriptLibrary::InitializeFunction<true, true, true>(instance, typeHandler, mode);

JavascriptExternalFunction* object = static_cast<JavascriptExternalFunction*>(instance);

Expand Down
Loading