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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 34 additions & 23 deletions src/compiler/transformers/declarations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
createEmptyExports,
createGetSymbolAccessibilityDiagnosticForNode,
createGetSymbolAccessibilityDiagnosticForNodeName,
createModeAwareCacheKey,
createSymbolTable,
createUnparsedSourceFile,
Debug,
Expand Down Expand Up @@ -63,6 +64,7 @@ import {
getLineAndCharacterOfPosition,
getNameOfDeclaration,
getNormalizedAbsolutePath,
getOriginalNode,
getOriginalNodeId,
getOutputPathsFor,
getParseTreeNode,
Expand Down Expand Up @@ -155,6 +157,7 @@ import {
mapDefined,
MethodDeclaration,
MethodSignature,
ModeAwareCacheKey,
Modifier,
ModifierFlags,
ModifierLike,
Expand Down Expand Up @@ -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.


const { factory } = context;
const host = context.getEmitHost();
Expand All @@ -280,33 +283,38 @@ export function transformDeclarations(context: TransformationContext) {
let errorFallbackNode: Declaration | undefined;

let currentSourceFile: SourceFile;
let refs: Map<NodeId, SourceFile>;
let libs: Map<string, boolean>;
let refs: Set<SourceFile>;
let libs: Set<string>;
Comment on lines +286 to +287
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

let emittedImports: readonly AnyImportSyntax[] | undefined; // must be declared in container so it can be `undefined` while transformer's first pass
const resolver = context.getEmitResolver();
const options = context.getCompilerOptions();
const { noResolve, stripInternal } = options;
return transformRoot;

function recordTypeReferenceDirectivesIfNecessary(typeReferenceDirectives: readonly [specifier: string, mode: ResolutionMode][] | undefined): void {
if (!typeReferenceDirectives) {
if (!typeReferenceDirectives || typeReferenceDirectives.length === 0) {
return;
}
necessaryTypeReferences = necessaryTypeReferences || new Set();
for (const ref of typeReferenceDirectives) {
necessaryTypeReferences.add(ref);
necessaryTypeReferences ??= new Map();
for (const directive of typeReferenceDirectives) {
const referenceKey = createModeAwareCacheKey(...directive);
necessaryTypeReferences.set(referenceKey, directive);
}
}

function recordExportedModuleFromDeclarationEmit(symbol: Symbol) {
exportedModulesFromDeclarationEmit ??= new Set();
exportedModulesFromDeclarationEmit.add(symbol);
}
function trackReferencedAmbientModule(node: ModuleDeclaration, symbol: Symbol) {
// If it is visible via `// <reference types="..."/>`, then we should just use that
const directives = resolver.getTypeReferenceDirectivesForSymbol(symbol, SymbolFlags.All);
if (length(directives)) {
return recordTypeReferenceDirectivesIfNecessary(directives);
}
// Otherwise we should emit a path-based reference
const container = getSourceFileOfNode(node);
refs.set(getOriginalNodeId(container), container);
const container = getOriginalNode(getSourceFileOfNode(node), isSourceFile);
refs.add(container);
}

function trackReferencedAmbientModuleFromImport(node: ImportDeclaration | ExportDeclaration | ImportEqualsDeclaration | ImportTypeNode) {
Expand Down Expand Up @@ -354,7 +362,7 @@ export function transformDeclarations(context: TransformationContext) {

function trackExternalModuleSymbolOfImportTypeNode(symbol: Symbol) {
if (!isBundledEmit) {
(exportedModulesFromDeclarationEmit || (exportedModulesFromDeclarationEmit = [])).push(symbol);
recordExportedModuleFromDeclarationEmit(symbol);
}
}

Expand Down Expand Up @@ -452,8 +460,8 @@ export function transformDeclarations(context: TransformationContext) {

if (node.kind === SyntaxKind.Bundle) {
isBundledEmit = true;
refs = new Map();
libs = new Map();
refs = new Set();
libs = new Set();
let hasNoDefaultLib = false;
const bundle = factory.createBundle(
map(node.sourceFiles, sourceFile => {
Expand Down Expand Up @@ -527,8 +535,11 @@ export function transformDeclarations(context: TransformationContext) {
lateMarkedStatements = undefined;
lateStatementReplacementMap = new Map();
necessaryTypeReferences = undefined;
refs = collectReferences(currentSourceFile, new Map());
libs = collectLibs(currentSourceFile, new Map());
recordTypeReferenceDirectivesIfNecessary(
node.typeReferenceDirectives.map(f => [f.fileName, f.resolutionMode ?? currentSourceFile.impliedNodeFormat]),
);
refs = collectReferences(currentSourceFile, new Set());
libs = collectLibs(currentSourceFile, new Set());
const references: FileReference[] = [];
const outputFilePath = getDirectoryPath(normalizeSlashes(getOutputPathsFor(node, host, /*forceDtsPaths*/ true).declarationFilePath!));
const referenceVisitor = mapReferencesIntoArray(references, outputFilePath);
Expand All @@ -548,15 +559,15 @@ export function transformDeclarations(context: TransformationContext) {
}
}
const updated = factory.updateSourceFile(node, combinedStatements, /*isDeclarationFile*/ true, references, getFileReferencesForUsedTypeReferences(), node.hasNoDefaultLib, getLibReferences());
updated.exportedModulesFromDeclarationEmit = exportedModulesFromDeclarationEmit;
updated.exportedModulesFromDeclarationEmit = exportedModulesFromDeclarationEmit && [...exportedModulesFromDeclarationEmit.keys()];
return updated;

function getLibReferences() {
return arrayFrom(libs.keys(), lib => ({ fileName: lib, pos: -1, end: -1 }));
}

function getFileReferencesForUsedTypeReferences() {
return necessaryTypeReferences ? mapDefined(arrayFrom(necessaryTypeReferences.keys()), getFileReferenceForSpecifierModeTuple) : [];
return necessaryTypeReferences ? mapDefined(arrayFrom(necessaryTypeReferences.values()), getFileReferenceForSpecifierModeTuple) : [];
}

function getFileReferenceForSpecifierModeTuple([typeName, mode]: [specifier: string, mode: ResolutionMode]): FileReference | undefined {
Expand All @@ -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?

// Already have an import declaration resolving to this file
return;
}
Expand Down Expand Up @@ -633,22 +644,22 @@ export function transformDeclarations(context: TransformationContext) {
}
}

function collectReferences(sourceFile: SourceFile | UnparsedSource, ret: Map<NodeId, SourceFile>) {
function collectReferences(sourceFile: SourceFile | UnparsedSource, ret: Set<SourceFile>) {
if (noResolve || (!isUnparsedSource(sourceFile) && isSourceFileJS(sourceFile))) return ret;
forEach(sourceFile.referencedFiles, f => {
const elem = host.getSourceFileFromReference(sourceFile, f);
if (elem) {
ret.set(getOriginalNodeId(elem), elem);
ret.add(getOriginalNode(elem, isSourceFile));
}
});
return ret;
}

function collectLibs(sourceFile: SourceFile | UnparsedSource, ret: Map<string, boolean>) {
function collectLibs(sourceFile: SourceFile | UnparsedSource, ret: Set<string>) {
forEach(sourceFile.libReferenceDirectives, ref => {
const lib = host.getLibFileFromReference(ref);
if (lib) {
ret.set(toFileNameLowerCase(ref.fileName), true);
ret.add(toFileNameLowerCase(ref.fileName));
}
});
return ret;
Expand Down Expand Up @@ -911,7 +922,7 @@ export function transformDeclarations(context: TransformationContext) {
else {
const symbol = resolver.getSymbolOfExternalModuleSpecifier(input);
if (symbol) {
(exportedModulesFromDeclarationEmit || (exportedModulesFromDeclarationEmit = [])).push(symbol);
recordExportedModuleFromDeclarationEmit(symbol);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/declarationFilesWithTypeReferences4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ function foo() {


//// [app.d.ts]
/// <reference types="node" />
declare function foo(): Error;
1 change: 1 addition & 0 deletions tests/baselines/reference/resolutionModeTripleSlash2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,5 @@ function foo() {


//// [app.d.ts]
/// <reference types="foo" resolution-mode="require"/>
declare function foo(): any;
Loading
Loading