Throw error when instanceof is passed something that is not a function as constructor #899

lucasfcosta · 2017-01-11T19:44:21Z

This aims to solve #893 and chaijs/get-func-name#20 is related to this issue.

Whenever passed something that is not an instance of a function, the instanceof assertion will now throw an error informing the user that he should pass a constructor to this assertion, but instead he has passed <type>

I also added tests for this in all three interfaces.

As I've said on the other PR an instanceof check is enough to determine whether or not something is a function, even though the @@hasInstance symbol exists.

Please let me know if I forgot anything.

EDIT: I have also started using the typeof check due to this explanation by @shvaikalesh.

keithamus · 2017-01-11T19:48:47Z

Despite this throwing an error, I assume it's not a breaking change? Just makes the error message a user would receive more descriptive?

lucasfcosta · 2017-01-11T20:02:53Z

@keithamus It was already throwing an error before, which was:

Right-hand side of 'instanceof' is not an object

I think it's important to tell the user which type was passed in order for him to be able to fix what went wrong. Also, whenever passing undefined we had problems with getName due to trying to access the non-existing name property of undefined.

shvaikalesh · 2017-01-11T20:06:28Z

lib/chai/core/assertions.js

@@ -988,6 +988,13 @@ module.exports = function (chai, _) {

  function assertInstanceOf (constructor, msg) {
    if (msg) flag(this, 'message', msg);
+    var isFunction = constructor instanceof Function;
+
+    if (!isFunction) {


I think we can do

var type = typeof constructor; if (typeof !== 'function') { throw new Error ... }

Maybe instead of var type = constructor === null ? 'null' : typeof constructor; we can use type-detect here?

I think that it would be an overkill. In this case we just need to know whether something is a function or not in order to see if it is a constructor, there's no need for anything more complex than that.

Please let me know if I forgot any edge case, but I think we should go native if we can.

Oops, speaking of edge cases, typeof is not sufficient here.

instanceof works not only with functions, but with any objects that have @@hasInstance.

shvaikalesh · 2017-01-11T22:01:54Z

lib/chai/core/assertions.js

@@ -988,6 +988,12 @@ module.exports = function (chai, _) {

  function assertInstanceOf (constructor, msg) {
    if (msg) flag(this, 'message', msg);
+    var constructorType = constructor === null ? 'null' : typeof constructor;


The most hardcore option is

function assertInstanceOf (constructor, msg) { if (msg) flag(this, 'message', msg); if (constructor !== Object(constructor)) throw new TypeError(); var object = flag(this, 'object'); var hasInstance = constructor[Symbol.hasInstance]; var isInstance; if (hasInstance == null) { if (typeof constructor != 'function') throw new TypeError(); isInstance = Function[Symbol.hasInstance].call(constructor, object); } else if (typeof hasInstance == 'function') { isInstance = hasInstance.call(constructor, object); } else { throw new TypeError(); } var name = _.getName(constructor); this.assert(isInstance , 'expected #{this} to be an instance of ' + name , 'expected #{this} to not be an instance of ' + name ); };

I've reimplemented part of instanceof algo to avoid getting @@hasInstance twice.

More concise option

function assertInstanceOf (constructor, msg) { if (msg) flag(this, 'message', msg); var validInstanceOfTarget = constructor === Object(constructor) && ( typeof constructor == 'function' || typeof constructor[Symbol.hasInstance] == 'function' ); if (!validInstanceOfTarget) throw new TypeError(); var name = _.getName(constructor); this.assert( flag(this, 'object') instanceof constructor , 'expected #{this} to be an instance of ' + name , 'expected #{this} to not be an instance of ' + name ); };

But this one calls constructor.[[Get]](@@hasInstance) twice. [[Get]] may be observable.

meeber · 2017-01-11T22:46:35Z

Agreed with @shvaikalesh. Whenever constructor has a Symbol.hasInstance property defined, this assertion should respect whatever instanceof returns, even if constructor isn't a function. However, the failed assertion message needs to be able to handle the fact that the return value of getName would be null in such a situation.

Example:

var FakeConstructor = {};  // Not a function, but it has `Symbol.hasInstance` defined:
FakeConstructor[Symbol.hasInstance] = function (val) {
  return val === 3;
};

var fakeInstanceA = 3;

// Should pass even though FakeConstructor isn't a function
expect(fakeInstanceA).to.be.an.instanceof(FakeConstructor);

var fakeInstanceB = 4;

// Should fail because 4 !== 3, but what does failed assertion error message use as "name"?
expect(fakeInstanceB).to.be.an.instanceof(FakeConstructor);

shvaikalesh · 2017-01-11T23:12:28Z

Yet another thing to consider: if constructor have no .prototype (method or =>), instanceof will fail with TypeError. We can't simply check for .prototype, because non-generator bound functions won't have one, but would work with instanceof because of internal slots.

keithamus · 2017-01-12T20:15:30Z

Good points from @shvaikalesh - we should handle all these cases. I suppose we could just catch any TypeErrors thrown by instanceof and make them friendlier?

lucasfcosta · 2017-01-15T15:02:02Z

@shvaikalesh I'm not sure I get it. Can't we just use Object.getPrototypeOf to see if something has a prototype? I've just tested this with both a method and an arrow function and it worked.

Can you please describe with more detail?

Also, thanks everyone for your feedback!

shvaikalesh · 2017-01-15T16:54:28Z

@lucasfcosta We can check for .prototype on function, however lack of it does not necessarily mean instanceof will fail: with Function#bind, bound function is created without .prototype, but still works with instanceof because of [[BoundTargetFunction]].

With all this, I agree with @keithamus that the most simple, reliable and future-compatible way is to wrap instanceof in try/catch and improve error message.

lucasfcosta · 2017-01-15T23:05:30Z

Thanks for the detailed explanation! Any suggestions on what the error message should be?

I was reading the specs and according to the algorithm I can only think of:

The provided argument does not have a prototype.

But I think that's not explicit enough and I'm not sure it is a good idea to talk about internal implementation details on error messages.

Please let me know if you can think of anything better.

Also, if anyone wants to read about how instanceof works, here is the link.

shvaikalesh · 2017-01-16T11:26:28Z

instance instanceof constructor

may fail, if

constructor is not an Object
@@hasInstance getter threw
constructor has != null and not callable @@hasInstance
@@hasInstance threw
constructor is not callable
.prototype is not an Object
[[GetPrototypeOf]] threw anywhere in prototype chain (proxies, ya know)

Many reasons here; we would have to reimplement instanceof to cover all of them. I believe

${instance} instanceof ${constructor} failed: ${error.message} ${error.stack}

is the way to go.

lucasfcosta · 2017-01-16T20:59:17Z

Well, we already cover some of these cases such in our validInstanceOfTarget check. That's why I thought of a more concise error message.

However, your suggestions look awesome. Thanks!

lucasfcosta · 2017-01-16T21:48:08Z

Hey friends, here it is. I also added a test to check if the error messages are being passed to our own error when instanceof throws.

Let me know if you have any further suggestions.

meeber · 2017-01-16T22:22:44Z

lib/chai/core/assertions.js

+    } catch (err) {
+        throw new Error(target + ' instanceof ' + constructor + ' failed: ' + err.message + '.')
+    }
+
    var name = _.getName(constructor);


Slight issue with error message wording in rare circumstances.

Example:

var FakeConstructor = {}; FakeConstructor[Symbol.hasInstance] = function (val) { return val === 3; }; var fakeInstanceB = 4; expect(fakeInstanceB).to.be.an.instanceof(FakeConstructor);

Current output:

AssertionError: expected 4 to be an instance of undefined

Output after chaijs/get-func-name#20 is merged:

AssertionError: expected 4 to be an instance of null

I'm thinking that if the result of getName is null, then the best thing we can do is just print out the constructor object; null will be meaningless/misleading to the end user. In this case, that'd be:

AssertionError: expected 4 to be an instance of [object Object]

Not particularly helpful, but at least it's not misleading. An alternative would be to say "of an unnamed constructor".

AssertionError: expected 4 to be an instance of an unnamed constructor

Well noticed!
This change is ready. I'll push it to this branch as soon as we release get-func-name with that new change so we can guarantee our travis builds will succeed.

lucasfcosta · 2017-01-24T19:53:46Z

This has just been updated to include the changes you requested. Now everything should be working as expected and this also includes the new version of get-func-name, which has just been released.

meeber · 2017-01-24T20:03:42Z

lib/chai/core/assertions.js

+    var target = flag(this, 'object')
+    var validInstanceOfTarget = constructor === Object(constructor) && (
+        typeof constructor === 'function' ||
+        (typeof Symbol !== 'undefined' && typeof constructor[Symbol.hasInstance] === 'function')


Are there any edge cases (environments or polyfills) in which Symbol is defined but Symbol.hasInstance isn't? Safe thing to do here is check that it isn't undefined either.

Good catch!

meeber · 2017-02-01T22:12:41Z

Looks like Node v4 has failing tests.

lucasfcosta · 2017-02-04T00:26:37Z

@meeber done!
This happened because Node v4 didn't implement Symbol.hasInstance but did implement Symbol. I added this new check before running tests on environment that do not support it.

meeber

LGTM

shvaikalesh · 2017-02-04T10:15:02Z

lib/chai/core/assertions.js

+    try {
+        isInstanceOf = target instanceof constructor
+    } catch (err) {
+        throw new Error(target + ' instanceof ' + constructor + ' failed: ' + err.message + '.')


Stack trace of original error message is lost here.

I realize we're going a bit full-circle here, but after further consideration I think that when the user's code itself is potentially responsible for the error being thrown, we should just pass along that error (original error object, stack trace, and message) instead of attempting to make it friendlier.

Conversely, when it's Chai that is purposefully throwing the error, then it needs to be an AssertionError that properly passes along the ssfi flag as well as respecting the user's custom failed assertion message. #922 handles the ssfi part, and a small follow-up PR will handle the custom message part.

In this particular case, the user could potentially have some custom crap defined via @@hasInstance which is responsible for an error being thrown. Therefore, I think we should pass along that error object as-is, which will be done automatically if we just remove the whole try/catch.

As @meeber noticed, I also think we're going a bit full-circle here. The whole purpose of this PR was to give our users more friendly error messages, but given what was said I'm not sure this is the best choice anymore.

So, what do you think? Should we cancel the Error related change and keep the other improvements or should we keep it?

I'd like to hear more opinions about that.

To be clear, I like all the changes except the part where we catch an error that could possibly have been thrown by the user's code (if they had a custom @@hasInstance that was responsible for the error). If we remove that change but keep the rest, I think we still come out ahead, and the changes we keep still satisfy the original problem from #893.

shvaikalesh · 2017-02-04T10:15:15Z

lib/chai/core/assertions.js

+
+    if (!validInstanceOfTarget) {
+      var constructorType = constructor === null ? 'null' : typeof constructor;
+      throw new Error('The instanceof assertion needs a constructor but ' + constructorType + ' was given.');


TypeError fits better

After this was merged, I was gonna rebase #922 and turn this error into an AssertionError that properly retains ssfi (and then a follow up PR will properly retain the user's custom message).

shvaikalesh · 2017-02-04T10:16:38Z

lib/chai/core/assertions.js

+    var validInstanceOfTarget = constructor === Object(constructor) && (
+        typeof constructor === 'function' ||
+        (typeof Symbol !== 'undefined' && typeof Symbol.hasInstance !== 'undefined' &&
+         typeof constructor[Symbol.hasInstance] === 'function')


[[Get]] is called on @@hasInstance twice: here and in instanceof. What if it is observable?

I guess the question is: Is there an implicit understanding that Chai, in the process of performing an instanceof assertion, will only trigger @@hasInstance once? Or is it allowed to trigger it as much as it wants?

The benefit of not worrying about calling it multiple times is that the code is less complex. The cost is that to test an object with an observable @@hasInstance, you'll have to manually use JavaScript's instanceof and then assert on the result of that, rather than using Chai's builtin instanceof assertion.

My gut feeling is that having an observable @@hasInstance is such an extreme edge case that it's not worth Chai jumping through any hoops to accommodate, especially given the easy workaround of invoking instanceof manually and asserting on the result. But it doesn't add too much complexity to accommodate it either, so I won't protest if we want to do it.

Anyone have strong feelings one way or another?

I think that if it's easier to accomodate that we really should do it.

It's better to perform this work once and in our side than passing the burden of having to work around this to our users every time they need it.

I'll make sure to update this PR as soon as I get more input on the other comment I've just made.

Also, sorry for the delay on this PR, I've been very busy because recently I've been working some extra hours because I'm going to travel at the end of this month.

Sounds good to me. See #899 (comment) for ideas on only triggering @@hasInstance once.

@meeber I can trigger the getter for @@hasInstance zero times if I use the in operator.

I also removed the try/catch statements on this commit.

What do you think about this?

Is there anything I should improve?

Using the in operator seems like a good strategy; that way, @@hasInstance is only triggered once when the actual instanceof call is made.

Further discussion required

…n as constructor

meeber · 2017-02-16T00:04:00Z

lib/chai/core/assertions.js

+      throw new Error('The instanceof assertion needs a constructor but ' + constructorType + ' was given.');
+    }
+
+    var isInstanceOf = target instanceof constructor


Semi-colon :D

Thanks!
I miss linting tools 😅

meeber

One missing semi-colon isn't enough to hold this up for a month, so if you can fix it @lucasfcosta, great, but if you're too busy, no problem and LGTM.

@keithamus @shvaikalesh @vieiralucas?

keithamus · 2017-03-23T09:55:38Z

Agreed, we need to get some of these outstanding PRs pushed in before 4.0 final, otherwise they'll only be held up for many more months before 5.0

shvaikalesh reviewed Jan 11, 2017

View reviewed changes

lucasfcosta force-pushed the throw-for-non-functions-on-instanceof branch from 77ae937 to 01b1485 Compare January 11, 2017 20:07

shvaikalesh reviewed Jan 11, 2017

View reviewed changes

lucasfcosta mentioned this pull request Jan 12, 2017

Chai Roadmap #457

Closed

10 tasks

lucasfcosta force-pushed the throw-for-non-functions-on-instanceof branch from 01b1485 to 2c4fc89 Compare January 16, 2017 21:38

meeber reviewed Jan 16, 2017

View reviewed changes

lucasfcosta force-pushed the throw-for-non-functions-on-instanceof branch from 2c4fc89 to e8d25c2 Compare January 24, 2017 19:53

meeber reviewed Jan 24, 2017

View reviewed changes

lucasfcosta force-pushed the throw-for-non-functions-on-instanceof branch from e8d25c2 to fb2f28d Compare January 24, 2017 21:17

lucasfcosta force-pushed the throw-for-non-functions-on-instanceof branch from fb2f28d to 1841618 Compare February 4, 2017 00:24

meeber previously approved these changes Feb 4, 2017

View reviewed changes

shvaikalesh reviewed Feb 4, 2017

View reviewed changes

meeber mentioned this pull request Feb 7, 2017

respondTo throws TypeError on arrow functions #929

Closed

lucasfcosta force-pushed the throw-for-non-functions-on-instanceof branch from 1841618 to 0118153 Compare February 15, 2017 23:56

Throw error when instanceof is passed something that is not a functio…

0118153

…n as constructor

meeber reviewed Feb 16, 2017

View reviewed changes

meeber approved these changes Mar 22, 2017

View reviewed changes

meeber mentioned this pull request Mar 22, 2017

Remove implementation frames from stack trace #922

Merged

keithamus merged commit ef2bf66 into chaijs:master Mar 23, 2017

meeber mentioned this pull request Mar 23, 2017

Sanity check for assertion instanceOf #893

Closed

meeber mentioned this pull request Jul 15, 2017

chai4 regression: instanceOf validation fails for native HTMLElement instances on IE11 #1000

Closed

Throw error when instanceof is passed something that is not a function as constructor #899

Throw error when instanceof is passed something that is not a function as constructor #899

Conversation

lucasfcosta commented Jan 11, 2017 • edited Loading

keithamus commented Jan 11, 2017

lucasfcosta commented Jan 11, 2017 • edited Loading

shvaikalesh Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shvaikalesh Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber commented Jan 11, 2017

shvaikalesh commented Jan 11, 2017

keithamus commented Jan 12, 2017

lucasfcosta commented Jan 15, 2017

shvaikalesh commented Jan 15, 2017 • edited Loading

lucasfcosta commented Jan 15, 2017

shvaikalesh commented Jan 16, 2017 • edited Loading

lucasfcosta commented Jan 16, 2017

lucasfcosta commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber commented Feb 1, 2017

lucasfcosta commented Feb 4, 2017

meeber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber left a comment

Choose a reason for hiding this comment

keithamus commented Mar 23, 2017

lucasfcosta commented Jan 11, 2017 •

edited

Loading

lucasfcosta commented Jan 11, 2017 •

edited

Loading

shvaikalesh Jan 11, 2017 •

edited

Loading

shvaikalesh Jan 11, 2017 •

edited

Loading

shvaikalesh commented Jan 15, 2017 •

edited

Loading

shvaikalesh commented Jan 16, 2017 •

edited

Loading

lucasfcosta Feb 14, 2017 •

edited

Loading