Repository navigation
program.getResolvedTypeReferenceDirectives() not keyed by containing file/directory, leading to invalid declaration emit #56836
Description
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 Sheetal Nandi (@sheetalkamat) Wesley Wigham (@weswigham) for input
Activity
Sadaf-A commentedon Dec 25, 2023
Andrew Branch (@andrewbranch) hey! I would like to work on this if you could please assign me
weswigham commentedon Jan 9, 2024
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 onnode, and referencesnode. Now your output can have a reference tonodeforcibly added to it, too (eg, for the globalBuffer), even though it's not in your direct dependencies, which then forces you to hoistnodeinto 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-levelnode_modulesfolder, 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 commentedon May 13, 2024
With #57681 we stopped using resolved project reference maps to generate typeRef all together so this has become irrelevant now?
andrewbranch commentedon May 13, 2024
The API is still used in a potentially unsafe way in
goToDefinition.tsandimportTracker.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 commentedon May 13, 2024
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.