[ID-Prep] PR2 - Preserve all type triple slash references

[dragomirtitian]

This is fix is part of the effort to get typescript emit closer to what an external tool could emit with isolated declarations.

Fixes #57439

[dragomirtitian]

Since we keep all references as they are declared in source, we can't use the tuple as the key because:

1. We mark all source references as necessary upfront.
2. Even if the host can't resolve the reference, we should still preserve it to keep as close to the original source.

An alternate approach would be to ask the host for the [specifier, mode] tuple. This approach however fells error prone, since even in current emit some of these tuples come from the host, while other are created in the declaration transform.

[dragomirtitian]

Did some minor cleanup. exportedModulesFromDeclarationEmit could in practice contain a lot of suplicate symbols since a symbol was pushed for every usage of an import type. (In tests I found an example where there were 40 elements in the array but only 2 distinct ones. This is especially problematic since there is a test for the presence of a symbol in the array).

If this change is controversial I can revert it.

[dragomirtitian]

Both of these were previously Map, but for libs only the keys are ever used. And for refs I understand form a previous PR that using objects as keys is now preferred (I assume this code was left over from a time when this was not possible)

If this change is controversial I can revert it

[weswigham]

Personally, I would rather we (or the tool doing the emit) error'd on triple-slash references under whatever emit mode requires these be preserved, rather than change our existing-long term behavior for all users. Triple slash references have a legacy, and we should probably avoid sweeping changes to their behavior - you can generally get the compiler to do the same thing with a types compiler option or an actual import, without all the funny elision/lifting/copying behavior.

Minimally, this feels potentially very breaky on very old codebases that somehow rely on the elision.

[DanielRosenwasser]

Triple slash references have a legacy

It certainly feels that way, but we have used them as the answer for loading global types in a few places, and it might be difficult to for developers to adopt that.

If we don't preserve the /// <reference /> (this could have a big impact on existing libraries), we would probably need something explicit on the reference to signal whether the comment can be elided.

A tempting alternative is to introduce import type "..." which we've punted on because of the existence of reference comments. You could make the argument that import type is always elidable for JS, but will be preserved for declaration files. Of course, that means the more natural syntax now becomes a "gotcha".

import type "yadda"; // oops, this wasn't needed for the `.d.ts` file!

export function foo(arg: any) {
   return (arg as SomeGlobalType).someValue;
}

[dragomirtitian]

@weswigham Our initial approach was to forbid them altogether. The problem is that ambient module declarations will not be resolvable if there are no /// <reference> directives. This was pointed out by @andrewbranch here

[...] This function doesn't just get called for literal triple slash references in the input file; it also gets called when you write an import that resolves to an ambient module. And returning early here changes the emit in a potentially dangerous way, without an accompanying isolatedDeclarations error. For example, if I have @types/node installed and a source file like

export { readFile } from "fs";

This is legal according to the PR as it stands, but the emit under [--isolatedDeclarations] loses the triple-slash reference that makes "fs" resolvable:

- /// <reference types="node" />
export { readFile } from "fs";

The problem is if we want to emit declarations one file at a time, we can't know what /// <reference> are needed, so users will need to add these references in code if they are needed (it will be an error under isolated declarations to not add these), so we can't forbid them.

However, declaration emit also currently removes /// <reference> that are not needed. A behavior that a tool looking one file at a time also can't replicate. So this PR tries to ensure that all references are preserved so the one file at a time tool only needs to copy the directives to the output.

So to summarize, regarding triple slash directives we can:

1. Forbid them in isolated declarations - But risk some modules becoming unresolvable (the previous approach)
2. Always preserve them - But risk breaking some existing libraries that depend on the elision behavior. (the approach in this PR)
3. Forbid them and add some new syntax to ensure appropriate modules are loaded to discover ambient declarations (import type "node" as @DanielRosenwasser suggests)

[andrewbranch]

I think we’re missing an option:

4. When --isolatedDeclarations is enabled, tsc can use the checker / full emit resolver to enforce that exactly the triple-slash references needed for declaration emit are written in the input file; no more and no less.

However, I’m not convinced the exact rules for triple-slash elision/addition are super correct or desirable, and I’m pretty suspicious that any library authors would be manually writing them and then relying on their elision. I’m in favor of this change as is. I think the risks are fairly low.

[andrewbranch]

Does this not prevent inclusion of manually-written triple-slash references that are redundant with imports?

[weswigham]

Of course, that means the more natural syntax now becomes a "gotcha".

But this PR makes that same "gotchya" apply to /// types reference comments. So that's not particularly different, is it? Just less potentially breaky. A lot of our recent work about preservation without elision of any kind applies to imports - maybe we should just keep this work in that vein?

[DanielRosenwasser]

I think the risks are fairly low.

I don't entirely agree, but I am willing to try the change in 5.5. We will have to note this as a significant change though because it means that if a library author used something like /// <reference types="node" /> and only used that for some internal check, it will leak its way into any published package.

[andrewbranch]

I would be fine with issuing a checker error to ensure isolatedDeclaration input code has the correct set of triple-slash references written if we would rather consider this breaking change for 6.0—I don’t feel too strongly about how we make isolatedDeclarations work in the short term, as long as we come up with a solution, but in the long run I think the behavior in this PR is much better than what we have.

[dragomirtitian]

I would be fine with issuing a checker error to ensure isolatedDeclaration input code has the correct set of triple-slash references written if we would rather consider this breaking change for 6.0—I don’t feel too strongly about how we make isolatedDeclarations work in the short term, as long as we come up with a solution, but in the long run I think the behavior in this PR is much better than what we have.

Then we run into another: What if a triple-slash reference is necessary for the implementation but not for the declaration. (some symbol is used in the code but does not make it into the declaration).

I am ok with this limitation. I can't think of an example where we want someone to use a triple-slash reference instead of an import in a modern code base.

The other reason I didn't go the error route is that we generally tried to make isolated declaration errors detectable by the external tool. (Although there are still some isolated declaration errors that can't be detected by an external tool, especially around computed properties).

[typescript-bot]

The TypeScript team hasn't accepted the linked issue #57439. If you can get it accepted, this PR will have a better chance of being reviewed.

[andrewbranch]

Trying to summarize the possible options after yesterday’s meeting. Backing up, the invariant we want to achieve is that a standalone isolated declaration emitter can produce declaration emit containing the same set of triple-slash references that tsc would. This can theoretically be done in two ways:

1. Change the behavior of tsc --declaration to do something with triple-slash references that any standalone isolated declaration emitter could implement: namely, either preserve all triple-slash references written in input files and never synthesize new ones, or never emit any triple-slash references (i.e., elide ones written in input files and never synthesize new ones). Either of these options would be a breaking change.
2. Use checker errors in tsc --isolatedDeclarations to enforce that input files are written in a way that guarantees a standalone isolated declaration emitter will match the behavior of tsc --declaration. To prevent catch-22 scenarios like @dragomirtitian mentioned where a reference is required to check implementation code but elided from declaration emit, we would need new syntax (likely an attribute on the reference tag) that can control whether a reference is emitted. This option doesn’t preclude changing tsc’s triple-slash emit behavior in the future.

These aren’t strictly the only options and they aren’t totally mutually exclusive—this PR, for example, changes the behavior of tsc --declaration to always preserve triple-slash references written in input files, but doesn’t prevent it from synthesizing new ones. So, a checker error would still be needed in tsc --isolatedDeclarations to enforce that any references that would be synthesized by tsc are written into the input files. Explicit syntax to prevent emitting a reference could still be added on top of this strategy too.

The larger question embedded in option (1) is whether declaration emit (with or without the existence of isolatedDeclarations) should even be in the business of guaranteeing inclusion of the files that declare globals/modules referenced in the declaration emit. It was pointed out that declaration emit can include globals from built-in lib files like lib.dom.d.ts without emitting a <reference lib="dom" />; it’s expected that consumers will include the appropriate library files for themselves and get errors if they don’t. The same approach could be taken for globals from other declaration files by removing today’s behavior of synthesizing “necessary” triple-slash references. When we discussed this, it was assumed to be a big enough break that it would wait until 6.0, so if we go that route, we need to figure out what isolatedDeclarations should do until then.

[dragomirtitian]

Superseded by #57681