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.

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

util: fix constructor/instanceof checks #3385

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Oct 15, 2015

util: fix constructor/instanceof checks

These new checks are similar to the one introduced in 089d688, but for other types of objects. Specifically, if an object was created in a different context, the constructor object will not be the same as the constructor object in the current context, so we have to compare constructor names instead.

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Oct 15, 2015
@evanlucas
Copy link
Contributor

LGTM. is there anywhere else we are relying on similar functionality?

@mscdex
Copy link
Contributor Author

mscdex commented Oct 15, 2015

Yeah, I guess I'll fix those up too while I'm in there.

@mscdex mscdex force-pushed the fix-object-constructor-check branch from 332647b to 68e2f1e Compare October 15, 2015 18:55
@mscdex mscdex changed the title util: fix check for Object constructor util: fix constructor/instanceof checks Oct 15, 2015

function isMap(obj) {
obj = getConstructorOf(obj);
while (typeof obj !== undefined && obj !== null) {
Copy link

Choose a reason for hiding this comment

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

Should this be typeof obj !== 'undefined' or obj !== undefined, like above?

@mscdex
Copy link
Contributor Author

mscdex commented Oct 15, 2015

Updated to fix the other constructor/instanceof checks.

CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/515

@mscdex mscdex force-pushed the fix-object-constructor-check branch from 68e2f1e to 0450980 Compare October 15, 2015 19:20
@evanlucas
Copy link
Contributor

Just curious, but I wonder if the isPromise, etc are faster than v8::Value::IsPromise. I would assume so, since they aren't doing the call into c++, but not sure

@mscdex
Copy link
Contributor Author

mscdex commented Oct 15, 2015

Updated again to fix typo. CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/516

@vkurchatkin
Copy link
Contributor

This doesn't seem better than Object.prototype.toString

@mscdex mscdex force-pushed the fix-object-constructor-check branch from 0450980 to f8cb886 Compare October 15, 2015 19:55
@mscdex
Copy link
Contributor Author

mscdex commented Oct 15, 2015

@vkurchatkin That won't work for Promises though. I've now replaced isMap() and isSet().

@targos
Copy link
Member

targos commented Oct 15, 2015

@mscdex I don't know if Map and Set can be subclassed, but I think your first version had the advantage to catch this:

class MyMap extends Map {}
util.inspect(new MyMap)

@mscdex
Copy link
Contributor Author

mscdex commented Oct 15, 2015

@targos They can be and apparently Object.prototype.toString() works for subclasses too. There were already separate tests for those cases.

CI after replacing Map/Set check implementations: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/517/

@evanlucas
Copy link
Contributor

I'm still a little concerned about using objectToString. I thought that wasn't as reliable?

@mscdex
Copy link
Contributor Author

mscdex commented Oct 21, 2015

@evanlucas Unreliable in what way? We're already using the same method in other util methods for checking for other types such as Date, RegExp, and Error.

@evanlucas
Copy link
Contributor

maybe I'm thinking about once @@toStringTag lands in v8...I could have sworn I remember seeing that it was not reliable...nevermind though

@evanlucas
Copy link
Contributor

LGTM if CI is happy

@jasnell
Copy link
Member

jasnell commented Oct 21, 2015

As much as depending on objectToString bothers me, I'm not sure there's a better way. LGTM so long as CI is happy

@mscdex
Copy link
Contributor Author

mscdex commented Oct 21, 2015

@jasnell Well, previously I was walking the prototype chain (similar to what I'm currently doing for checking for promises), but I'm not sure what the performance differences are.

@jasnell
Copy link
Member

jasnell commented Oct 21, 2015

Yeah, like I said, I'm not sure there's a better way. It just makes me sad to be forced to rely on toString checks

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

Kicked off another CI run, just to be sure since the last one came up red: https://ci.nodejs.org/job/node-test-pull-request/567/

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

@mscdex .. hmm... thinking... would this qualify as a semver-minor or major? it likely shouldn't but want to be sure

@mscdex
Copy link
Contributor Author

mscdex commented Oct 22, 2015

Wow, looks like there's something missing from a previous commit... an addon test is looking for a debug module...

@mscdex
Copy link
Contributor Author

mscdex commented Oct 22, 2015

Personally I would see this as a bug fix, so I wouldn't see it as a semver-major. Not sure whether it'd be a semver-minor or not though.

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

Very odd... testing a clean build locally on osx...

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

@mscdex ... not seeing the same failure locally on osx ... can you test on your end?

@trevnorris
Copy link
Contributor

FWIW calling into native is cheap, the call itself is simple enough:

void CheckIsPromise(const FunctionCallbackInfo<Value>& args) {
  args.GetReturnValue().Set(args[0]->IsPromise());
}

And this method is more definitive.

@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

New CI: https://ci.nodejs.org/job/node-test-pull-request/676/

@mscdex ... did you see the last comment from @trevnorris ?

@mscdex mscdex force-pushed the fix-object-constructor-check branch from f8cb886 to cdd15fb Compare November 5, 2015 20:21
@mscdex
Copy link
Contributor Author

mscdex commented Nov 5, 2015

@jasnell I've removed the js isPromise() and am just calling inspectPromise() every time which already does a C++-based promise check like @trevnorris suggested.

@@ -316,7 +319,7 @@ function formatValue(ctx, value, recurseTimes) {
formatter = formatMap;
} else {
// Only create a mirror if the object superficially looks like a Promise.
Copy link
Member

Choose a reason for hiding this comment

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

this comment isn't relevant anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? A mirror is still only created if it's been determined the object is a promise. Do you mean the "superficially" part?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, now the check isn't superficial. Also the comment isn't in the right context. It is not obvious here that we create an object mirror.
I know the change was made earlier but do you mind moving it to the inspectPromise declaration ?

Copy link
Member

Choose a reason for hiding this comment

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

I personally think it's fine the way it is.

@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

LGTM. CI is red but the failures do not appear to be related.

These new checks are similar to the one introduced in 089d688,
but for other types of objects. Specifically, if an object was
created in a different context, the constructor object will not be
the same as the constructor object in the current context, so we
have to compare constructor names instead.
@mscdex mscdex force-pushed the fix-object-constructor-check branch from cdd15fb to 7b775da Compare December 1, 2015 01:57
@mscdex
Copy link
Contributor Author

mscdex commented Dec 1, 2015

@targos I've updated the comment about Promises. LGTY?

@mscdex
Copy link
Contributor Author

mscdex commented Dec 1, 2015

I ran through CI one more time and only saw a few unrelated errors on Windows.

@targos
Copy link
Member

targos commented Dec 1, 2015

LGTM

mscdex added a commit that referenced this pull request Dec 1, 2015
These new checks are similar to the one introduced in 089d688,
but for other types of objects. Specifically, if an object was
created in a different context, the constructor object will not be
the same as the constructor object in the current context, so we
have to compare constructor names instead.

PR-URL: #3385
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@mscdex
Copy link
Contributor Author

mscdex commented Dec 1, 2015

Landed in 82b8355.

@mscdex mscdex closed this Dec 1, 2015
mscdex added a commit that referenced this pull request Dec 5, 2015
These new checks are similar to the one introduced in 089d688,
but for other types of objects. Specifically, if an object was
created in a different context, the constructor object will not be
the same as the constructor object in the current context, so we
have to compare constructor names instead.

PR-URL: #3385
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@rvagg rvagg mentioned this pull request Dec 17, 2015
@mscdex mscdex deleted the fix-object-constructor-check branch December 17, 2015 17:59
mscdex added a commit that referenced this pull request Dec 29, 2015
These new checks are similar to the one introduced in 089d688,
but for other types of objects. Specifically, if an object was
created in a different context, the constructor object will not be
the same as the constructor object in the current context, so we
have to compare constructor names instead.

PR-URL: #3385
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
These new checks are similar to the one introduced in 089d688,
but for other types of objects. Specifically, if an object was
created in a different context, the constructor object will not be
the same as the constructor object in the current context, so we
have to compare constructor names instead.

PR-URL: #3385
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
These new checks are similar to the one introduced in 089d688,
but for other types of objects. Specifically, if an object was
created in a different context, the constructor object will not be
the same as the constructor object in the current context, so we
have to compare constructor names instead.

PR-URL: nodejs#3385
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants