Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

dragomirtitian
Copy link
Contributor

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

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 19, 2024
@@ -254,11 +257,11 @@ export function transformDeclarations(context: TransformationContext) {
let needsScopeFixMarker = false;
let resultHasScopeMarker = false;
let enclosingDeclaration: Node;
let necessaryTypeReferences: Set<[specifier: string, mode: ResolutionMode]> | undefined;
let necessaryTypeReferences: Map<ModeAwareCacheKey, [specifier: string, mode: ResolutionMode]> | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let lateMarkedStatements: LateVisibilityPaintedStatement[] | undefined;
let lateStatementReplacementMap: Map<NodeId, VisitResult<LateVisibilityPaintedStatement | ExportAssignment | undefined>>;
let suppressNewDiagnosticContexts: boolean;
let exportedModulesFromDeclarationEmit: Symbol[] | undefined;
let exportedModulesFromDeclarationEmit: Set<Symbol> | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +286 to +287
let refs: Set<SourceFile>;
let libs: Set<string>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 20, 2024
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Feb 21, 2024

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
Copy link
Contributor Author

@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
Copy link
Member

I think we’re missing an option:

  1. 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.

@@ -579,7 +590,7 @@ export function transformDeclarations(context: TransformationContext) {

function mapReferencesIntoArray(references: FileReference[], outputFilePath: string): (file: SourceFile) => void {
return file => {
if (exportedModulesFromDeclarationEmit?.includes(file.symbol)) {
if (exportedModulesFromDeclarationEmit?.has(file.symbol)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@weswigham
Copy link
Member

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 DanielRosenwasser added the Breaking Change Would introduce errors in existing code label Feb 21, 2024
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.5.0 milestone Feb 21, 2024
@DanielRosenwasser
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Member

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 dragomirtitian changed the title Preserve all type triple slash references [ID-Prep] PR2 - Preserve all type triple slash references Mar 7, 2024
@dragomirtitian
Copy link
Contributor Author

Superseded by #57681

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[ID-Prep] Preserve all triple slash directives in declarations files
5 participants