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.

[andrewbranch]

Suggested change

	if (enclosingRange && !rangeContainsRange(enclosingRange, { pos: node.getStart(), end: node.getEnd() })) {
	if (enclosingRange && !rangeContainsRange(enclosingRange, createTextRangeFromNode(node, sourceFile))) {

And declare sourceFile outside the recursive function so it doesn’t have to be retrieved with a parent walk each time

[andrewbranch]

Looks good except for the one comment

[andrewbranch]

I think isValidTypeOnlyUseSite should be consulted here in addition to the reference import. Also, what if the reference import is a type-only namespace import? It looks like we're only considering the type-onlyness of the import if it's a default import.

[andrewbranch]

Can you explain what the rules are supposed to be for setting addAsTypeOnly? I don't quite understand the last commit.

[navya9singh]

I believe when isValidTypeOnlyUseSite is true, then if isTypeOnlyImportDeclaration(referenceImport) is true then it should be AddAsTypeOnly.Required and if not, then it should be AddAsTypeOnly.Allowed.

[andrewbranch]

Yeah, and if isValidTypeOnlyUseSite is false, then it has to be AddAsTypeOnly.NotAllowed. That makes sense to me!

[andrewbranch]

This condition can never be true based on the if statement it's in. Also, it looks like we don't use the referenceImport to decide between default/namespace unless isValidTypeOnlyUseSite is true, which is an unrelated piece of information. I think all this code can be reduced to a single assignment to info with conditional properties, which would help untangle the two separate things you're trying to control.