fix!: Don't use TS internal symbol IDs

They are unreliable when resolving imports, so while it worked perfectly fine when TypeDoc didn't do that, now it started breaking things.
TypeStrong · Jan 12, 2020 · b204f6a · b204f6a
1 parent 62314da
commit b204f6a
Show file tree

Hide file tree

Showing 39 changed files with 1,161 additions and 769 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -59,6 +59,7 @@ If you would like to improve the documentation in any of these areas, please ope
 Unsure of where to begin contributing to TypeDoc? You can start by looking through the issues labeled [good-first-issue] and [help-wanted]. Issues labeled with [good-first-issue] should only require changing a few lines of code and a test or two. Issues labeled with [help-wanted] can be considerably more involved and may require changing multiple files.
 
 For instructions on setting up your environment, see the [setup](#setup---git-github-and-node) instructions in this document.
+Once set up, you may find the [development](https://typedoc.org/guides/development/) page useful for an overview of TypeDoc's architecture.
 
 If you have started work on an issue and get stuck or want a second opinion on your implementation feel free to reach out through [Gitter].
 

diff --git a/scripts/rebuild_specs.js b/scripts/rebuild_specs.js
@@ -127,6 +127,8 @@ async function main(command = 'all', filter = '') {
             if (!stat.isDirectory()) return false;
             return fullPath.endsWith(filter);
         }).map(([path]) => path));
+    } else if (filter !== '') {
+        console.warn('Specifying a filter when rebuilding render specs only has no effect.');
     }
 
     if (['all', 'renderer'].includes(command)) {

diff --git a/src/lib/converter/context.ts b/src/lib/converter/context.ts
@@ -73,9 +73,9 @@ export class Context {
     inheritParent?: ts.Node;
 
     /**
-     * List symbol ids of inherited children already visited while inheriting.
+     * List symbol fqns of inherited children already visited while inheriting.
      */
-    inheritedChildren?: number[];
+    inheritedChildren?: string[];
 
     /**
      * The names of the children of the scope before inheritance has been started.
@@ -87,11 +87,6 @@ export class Context {
      */
     visitStack: ts.Node[];
 
-    /**
-     * Next free symbol id used by [[getSymbolID]].
-     */
-    private symbolID = -1024;
-
     /**
      * The pattern that should be used to flag external source files.
      */
@@ -159,6 +154,12 @@ export class Context {
         return symbol;
     }
 
+    resolveAliasedSymbol(symbol: ts.Symbol): ts.Symbol;
+    resolveAliasedSymbol(symbol: ts.Symbol | undefined): ts.Symbol | undefined;
+    resolveAliasedSymbol(symbol: ts.Symbol | undefined) {
+        return (symbol && ts.SymbolFlags.Alias & symbol.flags) ? this.checker.getAliasedSymbol(symbol) : symbol;
+    }
+
     /**
      * Return the current logger instance.
      *
@@ -169,40 +170,18 @@ export class Context {
     }
 
     /**
-     * Return the symbol id of the given symbol.
-     *
-     * The compiler sometimes does not assign an id to symbols, this method makes sure that we have one.
-     * It will assign negative ids if they are not set.
-     *
-     * @param symbol  The symbol whose id should be returned.
-     * @returns The id of the given symbol or undefined if no symbol is provided.
-     */
-    getSymbolID(symbol: ts.Symbol | undefined): number | undefined {
-        if (!symbol) {
-            return;
-        }
-        if (!symbol.id) {
-            symbol.id = this.symbolID--;
-        }
-        return symbol.id;
-    }
-
-    /**
-     * Register a newly generated reflection.
-     *
-     * Ensures that the reflection is both listed in [[Project.reflections]] and
-     * [[Project.symbolMapping]] if applicable.
+     * Register a newly generated reflection. All created reflections should be
+     * passed to this method to ensure that the project helper functions work correctly.
      *
      * @param reflection  The reflection that should be registered.
      * @param node  The node the given reflection was resolved from.
      * @param symbol  The symbol the given reflection was resolved from.
      */
-    registerReflection(reflection: Reflection, node?: ts.Node, symbol?: ts.Symbol) {
-        this.project.reflections[reflection.id] = reflection;
-
-        const id = this.getSymbolID(symbol ? symbol : (node ? node.symbol : undefined));
-        if (!this.isInherit && id && !this.project.symbolMapping[id]) {
-            this.project.symbolMapping[id] = reflection.id;
+    registerReflection(reflection: Reflection, symbol?: ts.Symbol) {
+        if (symbol) {
+            this.project.registerReflection(reflection, this.checker.getFullyQualifiedName(symbol));
+        } else {
+            this.project.registerReflection(reflection);
         }
     }
 
@@ -329,7 +308,7 @@ export class Context {
         }
 
         if (baseNode.symbol) {
-            const id = this.getSymbolID(baseNode.symbol)!;
+            const id = this.checker.getFullyQualifiedName(baseNode.symbol);
             if (this.inheritedChildren && this.inheritedChildren.includes(id)) {
                 return target;
             } else {

diff --git a/src/lib/converter/converter.ts b/src/lib/converter/converter.ts
@@ -290,8 +290,8 @@ export class Converter extends ChildableComponent<Application, ConverterComponen
         if (dangling.length) {
             this.owner.logger.warn([
                 'Some names refer to reflections that do not exist.',
-                'This can be caused by exporting a reflection only under a different name than it is declared',
-                'or by a plugin removing reflections. The names that cannot be resolved are:',
+                'This can be caused by exporting a reflection only under a different name than it is declared when excludeNotExported is set',
+                'or by a plugin removing reflections without removing references. The names that cannot be resolved are:',
                 ...dangling
             ].join('\n'));
         }

diff --git a/src/lib/converter/factories/declaration.ts b/src/lib/converter/factories/declaration.ts
@@ -142,7 +142,7 @@ export function createDeclaration(context: Context, node: ts.Declaration, kind:
 
         if (child) {
             children.push(child);
-            context.registerReflection(child, node);
+            context.registerReflection(child, context.getSymbolAtLocation(node) ?? node.symbol);
         }
     } else {
         // Merge the existent reflection with the given node

diff --git a/src/lib/converter/factories/parameter.ts b/src/lib/converter/factories/parameter.ts
@@ -24,7 +24,7 @@ export function createParameter(context: Context, node: ts.ParameterDeclaration)
 
     const parameter = new ParameterReflection(node.symbol.name, ReflectionKind.Parameter, signature);
     parameter.flags.setFlag(ReflectionFlag.Exported, context.scope.flags.isExported);
-    context.registerReflection(parameter, node);
+    context.registerReflection(parameter);
     context.withScope(parameter, () => {
         if (ts.isArrayBindingPattern(node.name) || ts.isObjectBindingPattern(node.name)) {
             parameter.type = context.converter.convertType(context, node.name);

diff --git a/src/lib/converter/factories/reference.ts b/src/lib/converter/factories/reference.ts
@@ -19,28 +19,27 @@ export function createReferenceType(context: Context, symbol: ts.Symbol | undefi
     }
 
     const checker = context.checker;
-    const id = context.getSymbolID(symbol)!;
     let name = checker.symbolToString(symbol);
 
     if (includeParent && symbol.parent) {
         name = checker.symbolToString(symbol.parent) + '.' + name;
     }
 
-    return new ReferenceType(name, id);
+    return new ReferenceType(name, context.checker.getFullyQualifiedName(symbol));
 }
 
 export function createReferenceReflection(context: Context, source: ts.Symbol, target: ts.Symbol): ReferenceReflection {
     if (!(context.scope instanceof ContainerReflection)) {
         throw new Error('Cannot add reference to a non-container');
     }
 
-    const reflection = new ReferenceReflection(source.name, [ReferenceState.Unresolved, context.getSymbolID(target)!], context.scope);
+    const reflection = new ReferenceReflection(source.name, [ReferenceState.Unresolved, context.checker.getFullyQualifiedName(target)], context.scope);
     reflection.flags.setFlag(ReflectionFlag.Exported, true); // References are exported by necessity
     if (!context.scope.children) {
         context.scope.children = [];
     }
     context.scope.children.push(reflection);
-    context.registerReflection(reflection, undefined, source);
+    context.registerReflection(reflection, source);
     context.trigger(Converter.EVENT_CREATE_DECLARATION, reflection);
 
     return reflection;

diff --git a/src/lib/converter/factories/signature.ts b/src/lib/converter/factories/signature.ts
@@ -4,7 +4,6 @@ import { ReflectionKind, SignatureReflection, ContainerReflection, DeclarationRe
 import { Context } from '../context';
 import { Converter } from '../converter';
 import { createParameter } from './parameter';
-import { createReferenceType } from './reference';
 
 /**
  * Create a new signature reflection from the given node.
@@ -23,7 +22,7 @@ export function createSignature(context: Context, node: ts.SignatureDeclaration,
 
     const signature = new SignatureReflection(name, kind, container);
     signature.flags.setFlag(ReflectionFlag.Exported, container.flags.isExported);
-    context.registerReflection(signature, node);
+    context.registerReflection(signature);
     context.withScope(signature, node.typeParameters, true, () => {
         node.parameters.forEach((parameter: ts.ParameterDeclaration) => {
             createParameter(context, parameter);
@@ -32,7 +31,7 @@ export function createSignature(context: Context, node: ts.SignatureDeclaration,
         signature.type = extractSignatureType(context, node);
 
         if (container.inheritedFrom) {
-            signature.inheritedFrom = createReferenceType(context, node.symbol, true);
+            signature.inheritedFrom = container.inheritedFrom.clone();
         }
     });
 

diff --git a/src/lib/converter/factories/type-parameter.ts b/src/lib/converter/factories/type-parameter.ts
@@ -30,7 +30,7 @@ export function createTypeParameter(context: Context, node: ts.TypeParameterDecl
     }
     reflection.typeParameters.push(typeParameterReflection);
 
-    context.registerReflection(typeParameterReflection, node);
+    context.registerReflection(typeParameterReflection);
     context.trigger(Converter.EVENT_CREATE_TYPE_PARAMETER, typeParameterReflection, node);
 
     return typeParameter;

diff --git a/src/lib/converter/nodes/constructor.ts b/src/lib/converter/nodes/constructor.ts
@@ -41,7 +41,7 @@ export class ConstructorConverter extends ConverterNodeComponent<ts.ConstructorD
                 const signature = createSignature(context, node, name, ReflectionKind.ConstructorSignature);
                 // If no return type defined, use the parent one.
                 if (!node.type) {
-                    signature.type = new ReferenceType(parent.name, ReferenceType.SYMBOL_ID_RESOLVED, parent);
+                    signature.type = new ReferenceType(parent.name, ReferenceType.SYMBOL_FQN_RESOLVED, parent);
                 }
                 method!.signatures = method!.signatures || [];
                 method!.signatures.push(signature);

diff --git a/src/lib/converter/nodes/export.ts b/src/lib/converter/nodes/export.ts
@@ -4,6 +4,7 @@ import { Reflection, ReflectionFlag, DeclarationReflection, ContainerReflection
 import { Context } from '../context';
 import { Component, ConverterNodeComponent } from '../components';
 import { createReferenceReflection } from '../factories/reference';
+import { SourceFileMode } from './block';
 
 @Component({name: 'node:export'})
 export class ExportConverter extends ConverterNodeComponent<ts.ExportAssignment> {
@@ -30,16 +31,14 @@ export class ExportConverter extends ConverterNodeComponent<ts.ExportAssignment>
                 if (!declaration.symbol) {
                     return;
                 }
-                const id = project.symbolMapping[context.getSymbolID(declaration.symbol)!];
-                if (!id) {
-                    return;
-                }
 
-                const reflection = project.reflections[id];
+                const reflection = project.getReflectionFromFQN(context.checker.getFullyQualifiedName(declaration.symbol));
                 if (node.isExportEquals && reflection instanceof DeclarationReflection) {
                     reflection.setFlag(ReflectionFlag.ExportAssignment, true);
                 }
-                markAsExported(reflection);
+                if (reflection) {
+                    markAsExported(reflection);
+                }
             });
         }
 
@@ -60,6 +59,11 @@ export class ExportDeclarationConverter extends ConverterNodeComponent<ts.Export
     supports = [ts.SyntaxKind.ExportDeclaration];
 
     convert(context: Context, node: ts.ExportDeclaration): Reflection | undefined {
+        // It doesn't make sense to convert export declarations if we are pretending everything is global.
+        if (this.application.options.getValue('mode') === SourceFileMode.File) {
+            return;
+        }
+
         const scope = context.scope;
         if (!(scope instanceof ContainerReflection)) {
             throw new Error('Expected to be within a container');
@@ -68,16 +72,15 @@ export class ExportDeclarationConverter extends ConverterNodeComponent<ts.Export
         if (node.exportClause) { // export { a, a as b }
             node.exportClause.elements.forEach(specifier => {
                 const source = context.getSymbolAtLocation(specifier.name);
-                const target = context.getSymbolAtLocation(specifier.propertyName ?? specifier.name);
+                const target = context.resolveAliasedSymbol(context.getSymbolAtLocation(specifier.propertyName ?? specifier.name));
                 if (source && target) {
-                    const original = (target.flags & ts.SymbolFlags.Alias) ? context.checker.getAliasedSymbol(target) : target;
                     // If the original declaration is in this file, export {} was used with something
                     // defined in this file and we don't need to create a reference unless the name is different.
                     if (!node.moduleSpecifier && !specifier.propertyName) {
                         return;
                     }
 
-                    createReferenceReflection(context, source, original);
+                    createReferenceReflection(context, source, target);
                 }
             });
         } else if (node.moduleSpecifier) { // export * from ...

diff --git a/src/lib/converter/nodes/variable.ts b/src/lib/converter/nodes/variable.ts
@@ -7,6 +7,14 @@ import { Context } from '../context';
 import { Component, ConverterNodeComponent } from '../components';
 import { convertDefaultValue } from '../index';
 
+type VarNodeType = ts.PropertySignature
+    | ts.PropertyDeclaration
+    | ts.PropertyAssignment
+    | ts.ShorthandPropertyAssignment
+    | ts.VariableDeclaration
+    | ts.ImportEqualsDeclaration
+    | ts.BindingElement;
+
 @Component({name: 'node:variable'})
 export class VariableConverter extends ConverterNodeComponent<ts.VariableDeclaration> {
     /**
@@ -18,6 +26,7 @@ export class VariableConverter extends ConverterNodeComponent<ts.VariableDeclara
         ts.SyntaxKind.PropertyAssignment,
         ts.SyntaxKind.ShorthandPropertyAssignment,
         ts.SyntaxKind.VariableDeclaration,
+        ts.SyntaxKind.ImportEqualsDeclaration,
         ts.SyntaxKind.BindingElement
     ];
 
@@ -36,7 +45,7 @@ export class VariableConverter extends ConverterNodeComponent<ts.VariableDeclara
      * @param node     The variable declaration node that should be analyzed.
      * @return The resulting reflection or NULL.
      */
-    convert(context: Context, node: ts.VariableDeclaration): Reflection | undefined {
+    convert(context: Context, node: VarNodeType): Reflection | undefined {
         const comment = createComment(node);
         if (comment && comment.hasTag('resolve')) {
             const resolveType = context.getTypeAtLocation(node);
@@ -84,7 +93,7 @@ export class VariableConverter extends ConverterNodeComponent<ts.VariableDeclara
         }
 
         context.withScope(variable, () => {
-            if (node.initializer) {
+            if ('initializer' in node && node.initializer) {
                 switch (node.initializer.kind) {
                     case ts.SyntaxKind.ArrowFunction:
                     case ts.SyntaxKind.FunctionExpression:
@@ -99,15 +108,15 @@ export class VariableConverter extends ConverterNodeComponent<ts.VariableDeclara
                         }
                         break;
                     default:
-                        variable!.defaultValue = convertDefaultValue(node);
+                        variable!.defaultValue = convertDefaultValue(node as ts.VariableDeclaration);
                 }
             }
 
             if (variable!.kind === kind || variable!.kind === ReflectionKind.Event) {
                 if (isBindingPattern) {
                     variable!.type = this.owner.convertType(context, node.name);
                 } else {
-                    variable!.type = this.owner.convertType(context, node.type, context.getTypeAtLocation(node));
+                    variable!.type = this.owner.convertType(context, (node as ts.VariableDeclaration).type, context.getTypeAtLocation(node));
                 }
             }
         });