Refactored native calls

[fearphage]

So I've seen this method used in jQuery to handle native method calls. I haven't checked here, but they all get compiled away cleanly in jQuery. I just touched a few files at first to see what everyone thought of the methodology. This would be a global change to combat things like #1796 from popping up.

Note: It's incomplete, but the tests are green.

[fatso83]

While I admire the effort I don't see how this would prevent anything. Today, bugs with native functions happen because we forget to check/fail to see that native functions are used directly.

Having them accessible through modules won't change anything regarding this, but will make it easier to do, since we can skip the ceremony.

In that respect, this is worth doing, but I can't see how we can ensure we are not introducing more bugs without an automatic checks that fails code using native functions.

[fearphage]

Today, bugs with native functions happen because we forget to check/fail to see that native functions are used directly.

The problem today is we assume the native is still functional/not broken. If we just stored a copy beforehand, we wouldn't face this problem... unless it was broken before sinon is loaded and then we're hosed unless we do something fancier like getting clean Arrays and Objects from an iframe... which is definitely doable.

[fatso83]

At least that's one of the problems 😃 It wouldn't prevent stuff like #1796 from popping up in future PRs, but at least it would serve as a reminder and also avoid the whole "is this native function still functional?" thing in the cases where we remember to use a cached reference.

[fearphage]

It wouldn't prevent stuff like #1796 from popping up in future PRs

I think it would make it easier to see. Calling randomArray.sort is what's broken in that issue. If we had cached Array#sort, it would be fine. We should essentially disallow those calls in our code. Maybe there's an eslint plugin for this? I think this also makes it a bit more distinct to see visually (sort(array, sorter) vs array.sort(sorter)) and I think it compiles away cleanly. I haven't tested the later, but I think so.

[mroderick]

I like where this PR is going, more defensive code is a good approach for a library.
An ESLint plugin would be very helpful to get contributors to do the right thing. Not sure if such a thing exists yet

[fatso83]

I think I have found all we need for such an eslint plugin!

Use the skeleton for no-use-extend-native

This module has all the structure in place and most of the code we need. It's basically a matter of removing some checks/inverting the conditions.

It uses is-proto-prop

This lib checks if the prop used is a property on the prototype, but, unlike my initial thoughts based on its name, it only checks for native props.

It does this using proto-props, which essentially is a json file listing all the native js props - exactly what we pondered!

[mroderick]

That looks promising!

[coveralls]

Pull Request Test Coverage Report for Build 2597

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 32 unchanged lines in 9 files lost coverage.
• Overall coverage increased (+0.1%) to 95.369%

---

Files with Coverage Reduction	New Missed Lines	%
sinon/util/core/extend.js	1	88.1%
sinon/util/core/wrap-method.js	1	80.42%
sinon/match.js	1	99.39%
sinon/stub.js	2	96.25%
sinon/call.js	2	96.55%
sinon/mock.js	2	96.67%
sinon/mock-expectation.js	6	93.46%
sinon/spy.js	7	94.21%
sinon/behavior.js	10	93.27%

Totals
Change from base Build 2584:	0.1%
Covered Lines:	2047
Relevant Lines:	2101

---

💛 - Coveralls

[mroderick]

I like the approach of using static analysis better than introducing more code. What do you think @fearphage?

[fearphage]

I like the approach of using static analysis better than introducing more code.

Is it an either or situation? An eslint-style rule will not make the code work correctly.

[mroderick]

An eslint-style rule will not make the code work correctly.

My understanding was that it could be used for preventing us from merging code that was vulnerable to end users overriding native methods. If that was incorrect, then I agree, it won't.

[akdor1154]

Maybe arrayPrototype = Object.freeze(Object.assign({}, Array.prototype)) could be used to avoid having a separate file for every array method you want to use internally?

[fearphage]

@akdor1154 Object.assign only sees enumerable properties so it misses everything on the prototype. I did use this to try a different direction though. Thank you.

[fearphage]

FYI this branch now passes with the new custom eslint rule (#1858).

[fatso83]

Great, then I suggest merging this first and the eslint rule second to see the build pass. Thanks for the work!

[mroderick]

👍