const { log } = console;

In our current configuration, the console.log declarations are under @types/node. So if we remove this check, the rule will report on this case. Not sure what's the best solution there..

Are you saying, you'd like to get rid of the first half of this function and only have the part after the return statement, but that that would cause bugs to do so? I'm not 100% following.

you'd like to get rid of the first half of this function and only have the part after the return statement, but that that would cause bugs to do so?

Yup, exactly!

I'd prefer to use only type checking here for obvious reasons, as it is more flexible and robust than just checking identifier names.

But this type checking-based approach works by checking whether a method is defined in the default library. This can be a problem for us, especially in the case of console methods.

Imagine the following situation:

// tsconfig.json
{
  "compilerOptions": {
    "lib": ["ESNext"],
    "types": ["node"]
  }
}

// index.ts
const { log } = console

The log method of the Console interface is defined in the node_modules/@types/node/console.d.ts, not in the default TypeScript library. Therefore, the rule will false-positively report on this destructuring because it knows that this method is defined outside the default lib.

Hmm. I'm not sure I have enough knowledge to know what to do here. Maybe someone from @typescript-eslint/triage-team will have a suggestion?

Just to make sure I understand: we're saying that:

1. console's log is a known exception defined in lib.dom.d.ts
2. A log override is defined in @types/node'
3. We'd like to still allow the exception for console's log, even though type info thinks it's a @types/node thing

...is that right?

Yeah, that makes sense. So, I guess the plan would be as follows:

• Make exceptions only for known methods from the default TS library.
• Remove purely syntactic checks so that we only rely on type-level checks
• For now, treat types coming from @types/* as invalid and let the rule report them
• Report invalid types on the DT side

Am i right?

I wonder if this would be too breaking a change for people, though. We'll make them update @types/node to get rid of lint errors in Node.js projects.

I would say that Report invalid types on the DT side should be done ASAP, so folks who keep up-to-date-ish dev dependencies are less likely to be impacted. And maybe delaying merging the change on our end till a couple of months after that? Would that be overkill?

My opinion - keep the first check so we can merge without regression (it may not be "ideal" but it already exists). File DT issue, and put a link to it in code with an explanation that the first check is necessary due that issue.

Oops, I just realized that Console is an interface, and we can't rely on function signatures declared in interfaces, because classes implementing those interfaces can still access this even if this: void is declared in an interface:

interface Aaa {
  unbound: (this: void) => void
}

class Foo implements Aaa {
  data = 'string'
  unbound() {
    console.log(this.data) // runtime error!
  }
}

// not reported by unbound-method rule
const { unbound } = {} as Aaa
unbound()

playground

This is a bug in a rule. See #3503. The rule should rely on function signatures only when dealing with classes, not with interfaces.

So even if we ask DT to change the types on their side, we have to make exceptions for those types because they do not guarantee runtime safety.

So I think now we have two options from #8952 (comment):

1. We can use the current method: pure AST node name comparison - the log method of console variable was called

2. We can try to do some heuristics based on the .d.ts file path: for example, if it includes node_modules or something like that

Personally, I'm in favor of the second option, because it covers more cases and does so more reliably. On the other hand, pure AST checks have been around for a long time and no one has complained about them yet.

What do you think of that?

Similarly to my opinion in #8952 (comment), I think that Option 2 here is just out of scope for this issue, granted that existing checks are based on Option 1 -like behavior. So my preference would be to go with Option 1 and discuss Option 2 in a followup.

What's the difference between this array literal and the SUPPORTED_GLOBALS array? Do we need both? Perhaps giving it a good name will make it clear?

Thoughts?

Do we need both?

I really don't know 🙁

For more reasoning on this, see #8952 (comment)

Perhaps giving it a good name will make it clear?

+1 👍 Will rename it

It's not super obvious how this function works. I cloned it down to play around with the tests, and I'm still not 100% sure, but my rough understanding is that the first part checks by identity whether something is a natively bound member, and the second part checks via the type system, in order to catch a more general set of cases where the builtin namespace objects are potentially renamed.

Could you add some comments to help the reader 🙏 ?

Are you saying, you'd like to get rid of the first half of this function and only have the part after the return statement, but that that would cause bugs to do so? I'm not 100% following.