Don’t offer import fix for members of arrays or classes

[andrewbranch]

When a module export = something with an array or class instance type, this prevents import fixes for its members from showing up, e.g.

// @Filename: array.ts
export = [0, 1, 2, 3, 4, 5];

// @Filename: index.ts
filter // `import { filter } from './array'` doesn’t make sense!

Fixes #35269

[sheetalkamat]

Arrays definitely make sense but instance type i am not so sure. do we have a repro package that causes this because of instance type?

[andrewbranch]

Nothing specific, but I can’t think of a time when it would be appropriate to import a class instance member. My original motivation was to eliminate fixes like this:

But since HTMLAnchorElement is declared as an interface, that would break lodash/jQuery style packages. So I thought, at least I can fix this for instances that can be unambiguously tied back to an actual class.

I guess you could make the argument that if these fixes aren’t disruptive, we should leave them alone. (The array case was disruptive.) It should be noted they don’t occur for completions, only code fixes.

[sandersn]

Since JS methods aren't bound I think suggesting them in a fix is a likely trap. So I think class types are correct to exclude.

[sandersn]

silly question: you don't need the leading /, do you? I never use it but see tests that do, and I have no idea what the difference is.

[sheetalkamat]

without leading / the array.ts's full path will be /tests/cases/compiler I think.

[andrewbranch]

No you don’t, but leaving it off scares me because I don’t understand what assumptions are being made about the file system and it makes me uncomfortable

[sandersn]

I was just imagining writing real files to / and that scared me.

[andrewbranch]

macOS root is no longer user-writable so the test would have failed for me 😁

[sandersn]

Note: I find the current behaviour pretty hilarious so if unbound methods weren't such a trap I would not approve this PR.

[weswigham]

if unbound methods weren't such a trap I would not approve this PR.

Notable: they're not actually unbound at runtime in our default emit, since we compile every usage to ns.method to emulate live bindings. This means the this is actually set up correctly, like a commonjs consumer may expect. Now, that goes out the window when esModuleInterop is set because of the namespace plucking, but it does actually work by default.

[weswigham]

We could crawl DT pretty easily to figure out how many packages this affects, no? Just need to check if each module's got an export = whose target is a variable declaration symbol with a class type where that class has methods?

[sandersn]

wait, does this work for static or instance methods? I thought it was instance methods.

(Yes, crawling DT would be pretty easy.)

[andrewbranch]

like a commonjs consumer may expect

Whoa. I would not expect this. 😄 I don’t understand why anyone with an intermediate+ understanding of JS would expect this. I guess when I think about a named import downleveling to CommonJS, I assume const { member } = require('./module') is the exact analog, so then I expect member to be unbound because I’m handling a raw identifier. 🤷‍♂

[andrewbranch]

13 packages on DT will be affected. I checked the first 6 before getting tired of doing that and feeling like I had enough info to feel good about this:

• 4 were actual class instances with regular methods (I checked the implementation) that would have been wrong/dangerous to auto-import from
• 1 was an actual class but arrow-function methods that could be run standalone, but the typings were wrong and the package now ships its own types
• 1 was not a class and should not have been typed as a class