Fixing namespace import debug failure

[navya9singh]

Fixes #58740
This debug failure happened when trying to resolve a namespace import. As the identifier for the import did not have a parent, an error was thrown.

[andrewbranch]

the identifier for the import did not have a parent

This isn’t quite right—the symbol the identifier for the import resolved to doesn’t have a parent, because the symbol is the module symbol, not an export of a module. That’s what namespace imports are—imports that bind an identifier to an entire module namespace record. So while your draft implementation looks pretty good, there are two problems: (1) addImportFromExportedSymbol is not the right function to contain it, since the symbol being passed in is not an export, and (2) there are other ways an import/require can bind to a module symbol that are still broken. For example, I just changed in pasteEdits_namespaceImport.ts

- import * as test from "./a";
+ import test = require("./a");

and replicated the same crash. The same thing can also happen with a JavaScript require, as well as with a default import under certain conditions.¹ The detection you need to determine if the symbol you have is for a module symbol is the isExternalModuleSymbol function instead of relying on looking at the syntax of referenceImport. However, I think you should maybe move your new code into a new ImportAdder function instead of expanding addImportForExportedSymbol.

Footnotes

1. Test case

   /// <reference path="../fourslash.ts" />
   
   // @Filename: /folder/c.ts
   //// [||]
   
   
   // @Filename: /a.ts
   //// const abc = 10;
   //// const def = 20;
   //// export interface testInterface {
   ////     abc: number;
   ////     def: number;
   //// }
   
   // @Filename: /b.mts
   //// import test from "./a.js";
   ////
   //// [|function foo(abc: test.testInterface, def: test.testInterface) {
   ////    console.log(abc);
   ////    console.log(def);
   //// }|]
   //// 
   
   // @Filename: /tsconfig.json
   ////{ "compilerOptions": { "module": "nodenext" }, "files": ["/folder/c.ts", "a.ts", "b.mts"] }
   
   const range = test.ranges();
   verify.pasteEdits({
       args: {
           pastedText: [`function foo(abc: test.abc, def: test.def) {
   console.log(abc);
   console.log(def);
   }`],
           pasteLocations: [range[0]],
           copiedFrom: { file: "b.mts", range: [range[1]] },
       },
       newFileContents: {
           "/folder/c.ts":
   `import * as test from "../a";
   
   function foo(abc: test.abc, def: test.def) {
   console.log(abc);
   console.log(def);
   }`
       }
   });
   
   

   ↩

[andrewbranch]

I think there’s no need for checker.getSymbolAtLocation anywhere in this function. Declarations always have a .symbol; you can just do declaration.importClause.namedBindings.symbol here, and .symbol on each ImportSpecifier below. Even the existing variable declaration / binding pattern stuff further down can lose the getSymbolAtLocation.

[andrewbranch]

symbol/originalSymbol are confusing names here. It looks like originalSymbol is the actual module symbol, and symbol is just an existing alias for the module that’s only being used to see what name to use. Instead, try just passing in the module symbol and the name you want to use?

[andrewbranch]

What is this check for? When does an external module symbol not have a valueDeclaration?

[andrewbranch]

Since there may be multiple different forms of import that can reference a whole module, you were asking about whether it’s ok to unconditionally coerce them into a namespace import. That might be fine, but it seems like it would make sense for this function to take an optional referenceImport like addImportFromExportedSymbol, and copy its kind and type-only status where appropriate.