TypeScript 4.4 allows named imports of methods, calling them unbound

[mceachen]

Bug Report

🕗 Version & Regression Information

When did you start seeing this bug occur?

TypeScript 4.4

If possible, please try testing the nightly version of TS to see if it's already been fixed.

Nope. 4.5.0-dev.20210923 emits the same .js as 4.4.3.

• ✔️ This is a crash
• ✔️ This changed between versions 4.3.5 and 4.4.3
• This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________
• I was unable to test this on prior versions because _______

⏯ Playground Link

Playground renders correct javascript.

💻 Code

I've reproduced the issue in this repository: https://github.com/mceachen/ts44-event-typeerror

1. The first commit uses TypeScript 4.3.5, and tsc --init with no edits. I also committed the javascript file emitted by tsc.

2. The second commit just upgrades TypeScript to 4.4.3, rebuilds the yarn.lock, and recompiles. The invalid javascript is visible:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var process_1 = require("process");
(0, process_1.addListener)("SIGINT", function (evt) {
    console.log("caught SIGINT", evt);
});
console.log("GREAT SUCCESS");

🙁 Actual behavior

With both node 14.17.6 and 16.10.0, the same error emits:

node:events:425
  events = target._events;
                  ^

TypeError: Cannot read properties of undefined (reading '_events')
    at _addListener (node:events:425:19)
    at addListener (node:events:487:10)
    at Object.<anonymous> (/home/mrm/src/ts44-event-typeerror/test.js:4:27)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47

🙂 Expected behavior

What I saw with TypeScript 4.3, which didn't have the (0, prefix in front of every method call.

[andrewbranch]

You can’t call addListener without it being bound to process:

[alt text]

❯ node
Welcome to Node.js v14.17.5.
Type ".help" for more information.
> const { addListener } = process
undefined
> addListener('SIGINT', evt => console.log(evt))
Uncaught TypeError: Cannot read property '_events' of undefined
    at _addListener (events.js:435:19)
    at addListener (events.js:497:10)
>

It was a mistake that it worked before 4.4, and it’s working as expected now. See #45189 for more discussion.

[mceachen]

@andrewbranch holy crap, that's... really not what I was expecting. What is the TL;DR solution for this?

[andrewbranch]

Well, I should add that the real expected behavior is that we error when you import addListener—that’s discussed in the linked issue and the attached design meeting notes. We just unfortunately don’t have a great way to do this everywhere. So the emit is “working as intended,” while the lack of a diagnostic is duplicate / known limitation for now.

[andrewbranch]

The import you want to write is

import process from "process";

and then call process.addListener(...). We could also add a this: NodeJS.Process annotation to that method on DefinitelyTyped to prevent calling it unbound, though it feels to me like we need a more comprehensive solution sooner or later.

[mceachen]

Thanks for that sample: I actually did that in my branch and indeed the code works with all versions of tsc.

It'd be a huge help if you could make 4.4 emit a warning for this situation. I suspect every non-trivial backend typescript user is about to be bit by this issue.

[mceachen]

Also: thanks to you and your team for TypeScript: y'all deserve a heap of kudos.

💯

[andrewbranch]

I’m actually going to change the title of your issue so we can keep it open tracking the lack of error. I honestly expected this to be a massive problem in 4.4, but we’ve heard fairly little noise about it so far. We would obviously like to be able to issue an error for imports that are going to break calls, but we simply don’t have enough information to know what’s going to break when looking at most .d.ts files. It’s literally not fixable for a lot of cases, so we’re still keeping an ear out and hoping that problems in the real world are few and far between.

[mceachen]

If I'm going to dream: hoo boy would it be sweet to add a TS/JS refactoring that was "replace named imports"

[andrewbranch]

Well, we actually have that, we just can’t actually know that it’s not going to work as written:

[mceachen]

WOW NEAT.

That refactoring is disabled doesn't exist in that tiny test repo (and I checked a couple projects too).

What do I need to feed it to make it happy?

(edit: the refactoring you listed wasn't disabled: it's not there, even on typescript@next. What build are you running?)

(second edit: that refactor action doesn't actually help. See #46034 (comment))