program.getResolvedTypeReferenceDirectives() not keyed by containing file/directory, leading to invalid declaration emit

[andrewbranch]

Test case

// @Filename: /node_modules/direct-dep/index.d.ts
/// <reference types="transitive-dep" />

// @Filename: /node_modules/direct-dep/node_modules/transitive-dep/index.d.ts
declare module "ambient-module" {
  export class C {
    private x;
  }
}

// @Filename: /index.ts
import { C } from "ambient-module";
export const c = new C();

// @Filename: /tsconfig.json
{
  "compilerOptions": {
    "declaration": true,
    "emitDeclarationOnly": true,
    "module": "nodenext",
    "types": ["direct-dep"]
  }
}

Expected behavior

Declaration files should be error-free if running tsc --declaration did not produce errors. Specifically, either /index.d.ts should contain a triple-slash reference to "direct-dep", or compiling should produce an error saying that a portable reference to /node_modules/direct-dep/node_modules/transitive-dep/index.d.ts cannot be emitted.

Actual behavior

/index.d.ts contains

/// <reference types="transitive-dep" />

which does not resolve. No errors are produced while compiling.

Discussion

The erroneous type reference directive is emitted because the program stores information that the target file, /node_modules/direct-dep/node_modules/transitive-dep/index.d.ts, had previously been resolved by a type reference directive of "transitive-dep" somewhere in the program’s input files. The declaration emitter mistakenly assumes that any type reference directive that resolved anywhere in the program will be resolvable from the declaration file it’s emitting, but this is a bad assumption because type reference resolution is dependent on the location of the file that’s making the resolution. In the test case above, the location that originally had the reference to "transitive-dep" has a node_modules folder in scope for its lookup that is not in scope for /index.d.ts.

This cannot reliably be fixed with a simple change to the declaration emitter, because the Program itself discards the relevant information about the location from which type reference directives were resolved. Its cache is keyed only by name and resolution mode.

I discovered this while attempting to make a package that uses conditional package.json "exports" to conditionally load ts-expose-internals. It fails because tsc writes invalid triple-slash references to "ts-expose-internals" instead of to my proxy package.

cc @sheetalkamat @weswigham for input

[Sadaf-A]

@andrewbranch hey! I would like to work on this if you could please assign me

[weswigham]

The declaration emitter mistakenly assumes that any type reference directive that resolved anywhere in the program will be resolvable from the declaration file it’s emitting, but this is a bad assumption because type reference resolution is dependent on the location of the file that’s making the resolution.

This is true, but also intentional, ish. It's been this way for a long time, anyway. The common case here is node - you use a package which happens to depend on node, and references node. Now your output can have a reference to node forcibly added to it, too (eg, for the global Buffer), even though it's not in your direct dependencies, which then forces you to hoist node into your own dependencies to fix. Still, we could probably do with making this scenario more explicit. Thing is, the name is portable - but your dependency on that name isn't known to exist... You may have "transitive-dep" in your top-level node_modules folder, too (potentially of a slightly different version), except it's not referenced by your code and thus not in your program (and thus not deduplicated by the package deduplicator during resolution). So while adding an error on this is pretty easy, validating when it's OK and the error isn't needed is harder - and that's probably where the challenge is in changing this behavior.

[sheetalkamat]

With #57681 we stopped using resolved project reference maps to generate typeRef all together so this has become irrelevant now?

[andrewbranch]

The API is still used in a potentially unsafe way in goToDefinition.ts and importTracker.ts, but those are less critical than declaration emit, so I’m happy to close this until/unless someone finds a bug in one of those services. We might consider deprecating this API in 6.0, replacing it with something less error-prone if needed.

[sheetalkamat]

That is just bug with incorrect usage and i am fixing that as part of some typeRef work i am doing. #58527
That API is internal and i am going to remove it and looking into getting rid of that variable also all together.