feat(eslint-plugin): [unbound-method] support bound builtins

[G-Rath]

This was suspiciously too easy 🕵

Sadly @bradzacher imports are seemingly not considered decorations - let me know how you'd like to handle that case.

I found that I could compare against valueDeclaration, rather than using symbol.getDeclarations(), but it's quite possible that I've not tested it against a complex enough setup for that to fail, so happy to use the original suggested method for tackling this :)

closes #1085

[bradzacher]

imports are seemingly not considered declarations

It took me a minute to figure this out.

The import declaration doesn't count as a declaration, because we're inspecting the symbol console.log. The .log method doesn't have an import declaration - it only has its declaration as a method, in the other file!

Soooooooooooooo with that in mind; what's the objective we want here; we want to be able to whitelist certain methods as being always "safe", right?

What's our whitelist? Something like this:

const validMemberExpressions = [
  'JSON.parse',
  'JSON.stringify',
  Object.keys(console)
    .filter(name => !name.startsWith('_'))
    .map(name => `console.${name}`),
]

First up, let's make an assumption to make things simpler for us - we're not going to support aliases for now (i.e. const foo = { log: console.log }; promise.then(foo.log) will throw an error).

This then becomes a pretty simple problem to solve:

1. ensure the member expression matches one of validMemberExpressions
2. check that the root object of the member expression was not imported

(1) you can do either via explicitly checking the structure of the AST, or via comparing the source code with whitespace removed.

(2) will be something like:

• get the root .object of the member expression (which should be an Identifier).
• get the Identifier's symbol, and its declaration.
• ensure the declaration's source file is not this file.

---

Note - you should also setup a test case which uses valid imports.

In packages/eslint-plugin/tests/fixtures/class.ts, add something like

// used by unbound-method test case to test imports
export const console = { log() {} };

Then in your tests, make sure you're importing that, not the unknown file consoleshim.

import { console } from './class'
const x = console.log;

[bradzacher]

rc for last comment

[G-Rath]

@bradzacher guess who went a little overboard on the whitelist... :D

Also, it seems that valueDeclaration is not defined when the symbol is imported O.o

If that's true, that might be of use elsewhere?

[bradzacher]

guess who went a little overboard on the whitelist

Hey dude, that's actually awesome. Will save a lot of noise for people!

Note (it won't matter for long, as we'll drop node 8 in v3...) BigInt isn't available in node 8, so you'll have to do a check for that.

it seems that valueDeclaration is not defined when the symbol is imported

That's interesting. TBH IDK what the difference between declarations and valueDeclaration is. There is a distinct lack of docs for this stuff, and the comment in the source code is just "First value declaration of the symbol".

[G-Rath]

Hey dude, that's actually awesome. Will save a lot of noise for people!

Yeah, I think the only one I'm missing right now is ArrayBuffer, or something 😂

Note (it won't matter for long, as we'll drop node 8 in v3...) BigInt isn't available in node 8, so you'll have to do a check for that.

I'll just comment it out for now, and we can enable it when we release v3.

That's interesting.

Yeah, it was actually confusing the heck out of me at first b/c I thought it was the !== condition that was being weird, but I ran it through ts-ast-viewer and confirmed that when you add an import, the property magically vanishes 🤷‍♂

Can you restart the CI? It's randomly failed again like it did for me a few days back 😬

Oh, no it's b/c of BigInt. Woops

[bradzacher]

A few final comments.
Otherwise LGTM.
Thanks for doing this.

[bradzacher]

Thanks for doing this!

[G-Rath]

Thanks for doing this!

No problem! Apologies for not getting back around to it sooner, but cheers for picking it up for the release! :D