Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Don't use instanceof in lib/util.js "is" checks. #1942

Closed
wants to merge 6 commits into from
Closed

Don't use instanceof in lib/util.js "is" checks. #1942

wants to merge 6 commits into from

Conversation

TooTallNate
Copy link

While using instanceof, these functions could easily be faked with something
like: Object.create(Date.prototype)

So let's just not use it at all. A little slower, but these functions are only
used in the REPL / for debugging so it's OK.

This fixes #1941.

@koichik
Copy link

koichik commented Oct 26, 2011

@TooTallNate
Copy link
Author

Well let's get @isaacs' and @bnoordhuis' opinions on this again. Because honestly, I don't see any benefit in these false-positives, and it's even exposing a crash in the REPL (#1941).

@bnoordhuis
Copy link
Member

It's a bug so let's get it fixed.

I would add comments explaining why the functions are implemented the way they are, so some overzealous optimizer doesn't reintroduce the problem in the future.

Tests would be nice too. We decided on the ML to export the functions to user land, testability is a nice side effect of that.

@TooTallNate
Copy link
Author

@bnoordhuis I've added a comment to util.js as requested.

I've opened #1948, so once that is merged then we can add some tests regarding this broken behavior to that test case.

While using `instanceof`, these functions could easily be faked with something
like:  Object.create(Date.prototype)

So let's just not use it at all. A little slower, but these functions are only
used in the REPL / for debugging so it's OK.
@TooTallNate
Copy link
Author

OK, test cases added. This one is ready!

@koichik
Copy link

koichik commented Oct 26, 2011

@TooTallNate
Copy link
Author

Thanks, fixed.

@koichik koichik closed this in 2dbb470 Oct 26, 2011
koichik pushed a commit that referenced this pull request Oct 26, 2011
Add test for the false-positives from #1942.
Fix test-sys.js.
@koichik
Copy link

koichik commented Oct 26, 2011

Thanks, I squashed these commits into 2dbb470 and d4379fc.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants