Skip to content
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

Jump to bottom

Update JsInstanceOf to use OP_IsInst #5527

Merged
merged 1 commit into from
Jul 27, 2018
Merged

Update JsInstanceOf to use OP_IsInst #5527

merged 1 commit into from
Jul 27, 2018
+4 −3

Conversation

kfarnung
Copy link
Contributor

OP_IsInst correctly handles the ES6 Symbol.hasInstance property
when determining whether an object is an instance of a constructor.

@kfarnung kfarnung self-assigned this Jul 25, 2018
@kfarnung
Copy link
Contributor Author

I'm targeting at release/1.10 in hopes of getting this into node-chakracore, but can retarget to master if necessary.

boingoing
boingoing approved these changes Jul 25, 2018
View reviewed changes
Copy link
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Any concerns about changing behavior of JsInstanceOf since it's shipping as a Windows API?

lib/Jsrt/Jsrt.cpp Outdated
@@ -1415,7 +1415,8 @@ CHAKRA_API JsInstanceOf(_In_ JsValueRef object, _In_ JsValueRef constructor, _Ou
VALIDATE_INCOMING_REFERENCE(constructor, scriptContext);
PARAM_NOT_NULL(result);

*result = Js::RecyclableObject::FromVar(constructor)->HasInstance(object, scriptContext) ? true : false;
Js::Var booleanValue = Js::JavascriptOperators::OP_IsInst(object, constructor, scriptContext, nullptr);
*result = Js::JavascriptBoolean::FromVar(booleanValue)->GetValue() ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

A little clunky but alright. You could write this as !!Js::JavascriptBoolean::FromVar(booleanValue)->GetValue(), though I don't really like this syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to change it, I just lifted this from JsBooleanToBool later in this file.

MSLaguana
MSLaguana suggested changes Jul 25, 2018
View reviewed changes
lib/Jsrt/Jsrt.cpp Outdated
@@ -1415,7 +1415,8 @@ CHAKRA_API JsInstanceOf(_In_ JsValueRef object, _In_ JsValueRef constructor, _Ou
VALIDATE_INCOMING_REFERENCE(constructor, scriptContext);
PARAM_NOT_NULL(result);

*result = Js::RecyclableObject::FromVar(constructor)->HasInstance(object, scriptContext) ? true : false;
Js::Var booleanValue = Js::JavascriptOperators::OP_IsInst(object, constructor, scriptContext, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might not be guaranteed to return a Js::JavascriptBoolean? It looks like in the case of a callable Symbol.hasInstance that it will return the result of the function, and it is possible to define a function that doesn't return a Boolean. If I do that from script, then foo instanceof bar appears to return the truthiness of whatever I return, but I don't see that in the OP_IsInst codepath

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might need to use JavascriptConversion::ToBoolean here, and then the next line should be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah; I misread the code in IsInst and it does in fact do the conversion there. Disregard!

@kfarnung kfarnung requested a review from mrkmarron July 25, 2018 21:52
@kfarnung
Copy link
Contributor Author

@boingoing It's a pretty subtle change in behavior and seems like a bug fix rather than a behavioral change.

@kfarnung
Copy link
Contributor Author

@digitalinfinity any objections to this change landing in release/1.10?

@digitalinfinity
Copy link
Contributor

Is JsInstanceOf experimental or part of the Windows API surface? If its the former, no objections. If it's the latter, I'd say this goes into master and can later make a release once the Windows Compat folks have signed off

@kfarnung
Copy link
Contributor Author

It's in ChakraCommon.h so I'll retarget this to master.

@kfarnung kfarnung changed the base branch from release/1.10 to master July 27, 2018 17:46
@kfarnung
Update JsInstanceOf to use OP_IsInst
8661d43 
`OP_IsInst` correctly handles the ES6 `Symbol.hasInstance` property
when determining whether an object is an instance of a constructor.
@kfarnung
Copy link
Contributor Author

@mrkmarron Is that all I need to change for TTD?

@chakrabot chakrabot merged commit 8661d43 into chakra-core:master Jul 27, 2018
chakrabot pushed a commit that referenced this pull request Jul 27, 2018
@kfarnung
[MERGE #5527 @kfarnung] Update JsInstanceOf to use OP_IsInst
9f08d6e 
Merge pull request #5527 from kfarnung:jsinstanceof

`OP_IsInst` correctly handles the ES6 `Symbol.hasInstance` property
when determining whether an object is an instance of a constructor.
@kfarnung kfarnung deleted the jsinstanceof branch July 27, 2018 18:51
@mrkmarron
Copy link
Contributor

mrkmarron commented Jul 27, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boingoing boingoing

@MSLaguana MSLaguana

@digitalinfinity digitalinfinity

@mrkmarron mrkmarron

Assignees

@kfarnung kfarnung

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kfarnung @digitalinfinity @mrkmarron @boingoing @MSLaguana @chakrabot