Merge pull request #2 from zelliott/pr-3602-review

Some proposed changes to microsoft#3602

apps/api-extractor/src/analyzer/AstNamespaceImport.ts

@@ -11,6 +11,7 @@ export interface IAstNamespaceImportOptions {
   readonly astModule: AstModule;
   readonly namespaceName: string;
   readonly declaration: ts.Declaration;
+  readonly symbol: ts.Symbol;
 }
 
 /**
@@ -67,11 +68,17 @@ export class AstNamespaceImport extends AstSyntheticEntity {
    */
   public readonly declaration: ts.Declaration;
 
+  /**
+   * The original `ts.SymbolFlags.Namespace` symbol.
+   */
+  public readonly symbol: ts.Symbol;
+
   public constructor(options: IAstNamespaceImportOptions) {
     super();
     this.astModule = options.astModule;
     this.namespaceName = options.namespaceName;
     this.declaration = options.declaration;
+    this.symbol = options.symbol;
   }
 
   /** {@inheritdoc} */

apps/api-extractor/src/analyzer/ExportAnalyzer.ts

@@ -626,7 +626,8 @@ export class ExportAnalyzer {
             namespaceImport = new AstNamespaceImport({
               namespaceName: declarationSymbol.name,
               astModule: astModule,
-              declaration: declaration
+              declaration: declaration,
+              symbol: declarationSymbol
             });
             this._astNamespaceImportByModule.set(astModule, namespaceImport);
           }

apps/api-extractor/src/collector/Collector.ts

@@ -296,41 +296,18 @@ export class Collector {
   }
 
   /**
-   * Returns the associated `CollectorEntity` for the given `astEntity`, if one was created during analysis.
-   */
-  public tryGetCollectorEntity(astEntity: AstEntity): CollectorEntity | undefined {
-    return this._entitiesByAstEntity.get(astEntity);
-  }
-
-  /**
-   * Returns the name to emit for the given `symbol`.
-   * This is usually the `name` of that `symbol`.
-   * Only if this `symbol` has a `CollectorEntity`, the `nameForEmit` of that entity is used.
-   * @param symbol the symbol to compute the name to emit for
-   * @returns the name to emit for the given `symbol`
+   * For a given analyzed ts.Symbol, return the CollectorEntity that it refers to. Returns undefined if it
+   * doesn't refer to anything interesting.
    */
-  public getEmitName(symbol: ts.Symbol): string {
-    const collectorEntity: CollectorEntity | undefined = this._entitiesBySymbol.get(symbol);
-    return collectorEntity?.nameForEmit ?? symbol.name;
+  public tryGetEntityForSymbol(symbol: ts.Symbol): CollectorEntity | undefined {
+    return this._entitiesBySymbol.get(symbol);
   }
 
   /**
-   * Returns (the symbol of) the namespace that exports the given `symbol`, if applicable.
-   * @param symbol the symbol representing the namespace that exports the given `symbol`,
-   *   or undefined if there is no such namespace
+   * Returns the associated `CollectorEntity` for the given `astEntity`, if one was created during analysis.
    */
-  public getExportingNamespace(symbol: ts.Symbol): ts.Symbol | undefined {
-    const collectorEntity: CollectorEntity | undefined = this._entitiesBySymbol.get(symbol);
-    if (collectorEntity) {
-      const exportingNamespace: AstNamespaceImport | undefined = collectorEntity.getExportingNamespace();
-      if (exportingNamespace) {
-        return TypeScriptInternals.tryGetSymbolForDeclaration(
-          exportingNamespace.declaration,
-          this.typeChecker
-        );
-      }
-    }
-    return undefined;
+  public tryGetCollectorEntity(astEntity: AstEntity): CollectorEntity | undefined {
+    return this._entitiesByAstEntity.get(astEntity);
   }
 
   public fetchSymbolMetadata(astSymbol: AstSymbol): SymbolMetadata {
@@ -455,7 +432,7 @@ export class Collector {
       if (astEntity instanceof AstSymbol) {
         this._entitiesBySymbol.set(astEntity.followedSymbol, entity);
       } else if (astEntity instanceof AstNamespaceImport) {
-        this._entitiesBySymbol.set((astEntity.declaration as unknown as ts.Type).symbol, entity);
+        this._entitiesBySymbol.set(astEntity.symbol, entity);
       }
       this._entities.push(entity);
       this._collectReferenceDirectives(astEntity);

apps/api-extractor/src/collector/CollectorEntity.ts

@@ -7,7 +7,6 @@ import { AstSymbol } from '../analyzer/AstSymbol';
 import { Collector } from './Collector';
 import { Sort } from '@rushstack/node-core-library';
 import { AstEntity } from '../analyzer/AstEntity';
-import { AstNamespaceImport } from '../analyzer/AstNamespaceImport';
 
 /**
  * This is a data structure used by the Collector to track an AstEntity that may be emitted in the *.d.ts file.
@@ -96,6 +95,13 @@ export class CollectorEntity {
     return false;
   }
 
+  /**
+   * Indicates that this entity is exported from the package entry point. Compare to `CollectorEntity.exported`.
+   */
+  public get exportedFromEntryPoint(): boolean {
+    return this.exportNames.size > 0;
+  }
+
   /**
    * Indicates that this entity is exported from its parent module (i.e. either the package entry point or
    * a local namespace). Compare to `CollectorEntity.consumable`.
@@ -117,7 +123,7 @@ export class CollectorEntity {
    */
   public get exported(): boolean {
     // Exported from top-level?
-    if (this.exportNames.size > 0) return true;
+    if (this.exportedFromEntryPoint) return true;
 
     // Exported from parent?
     for (const localExportNames of this._localExportNamesByParent.values()) {
@@ -157,7 +163,7 @@ export class CollectorEntity {
    */
   public get consumable(): boolean {
     // Exported from top-level?
-    if (this.exportNames.size > 0) return true;
+    if (this.exportedFromEntryPoint) return true;
 
     // Exported from consumable parent?
     for (const [parent, localExportNames] of this._localExportNamesByParent) {
@@ -191,12 +197,13 @@ export class CollectorEntity {
   }
 
   /**
-   * Return the first namespace that exports this entity.
+   * Return the first consumable parent that exports this entity. If there is none, returns
+   * `undefined`.
    */
-  public getExportingNamespace(): AstNamespaceImport | undefined {
+  public get firstExportingConsumableParent(): CollectorEntity | undefined {
     for (const [parent, localExportNames] of this._localExportNamesByParent) {
-      if (parent.consumable && localExportNames.size > 0 && parent.astEntity instanceof AstNamespaceImport) {
-        return parent.astEntity;
+      if (parent.consumable && localExportNames.size > 0) {
+        return parent;
       }
     }
     return undefined;

apps/api-extractor/src/generators/DeclarationReferenceGenerator.ts

@@ -15,6 +15,7 @@ import { TypeScriptHelpers } from '../analyzer/TypeScriptHelpers';
 import { TypeScriptInternals } from '../analyzer/TypeScriptInternals';
 import { Collector } from '../collector/Collector';
 import { CollectorEntity } from '../collector/CollectorEntity';
+import { AstNamespaceImport } from '../analyzer/AstNamespaceImport';
 
 export class DeclarationReferenceGenerator {
   public static readonly unknownReference: string = '?';
@@ -88,18 +89,9 @@ export class DeclarationReferenceGenerator {
   }
 
   private _getNavigationToSymbol(symbol: ts.Symbol): Navigation {
-    // resolve alias first:
-    if (symbol.flags & ts.SymbolFlags.Alias) {
-      symbol = this._collector.typeChecker.getAliasedSymbol(symbol);
-    }
-    // namespace always uses ".":
-    if (symbol.flags & ts.SymbolFlags.Namespace) {
-      return Navigation.Exports;
-    }
     const declaration: ts.Declaration | undefined = TypeScriptHelpers.tryGetADeclaration(symbol);
     const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile();
-    const parent: ts.Symbol | undefined =
-      TypeScriptInternals.getSymbolParent(symbol) ?? this._collector.getExportingNamespace(symbol);
+    const parent: ts.Symbol | undefined = TypeScriptInternals.getSymbolParent(symbol);
 
     // If it's global or from an external library, then use either Members or Exports. It's not possible for
     // global symbols or external library symbols to be Locals.
@@ -118,34 +110,25 @@ export class DeclarationReferenceGenerator {
       return Navigation.Exports;
     }
 
-    // Otherwise, this symbol is from the current package.
-    if (parent) {
-      // If we've found an exported CollectorEntity, then it's exported from the package entry point, so
-      // use Exports.
-      const namedDeclaration: ts.DeclarationName | undefined = (
-        declaration as ts.NamedDeclaration | undefined
-      )?.name;
-      if (namedDeclaration && ts.isIdentifier(namedDeclaration)) {
-        const collectorEntity: CollectorEntity | undefined =
-          this._collector.tryGetEntityForNode(namedDeclaration);
-        if (collectorEntity && collectorEntity.exported) {
-          return Navigation.Exports;
-        }
-      }
-
-      // If its parent symbol is not a source file, then use either Exports or Members. If the parent symbol
-      // is a source file, but it wasn't exported from the package entry point (in the check above), then the
-      // symbol is a local, so fall through below.
-      if (!DeclarationReferenceGenerator._isExternalModuleSymbol(parent)) {
-        if (
-          parent.members &&
-          DeclarationReferenceGenerator._isSameSymbol(parent.members.get(symbol.escapedName), symbol)
-        ) {
-          return Navigation.Members;
-        }
+    // Otherwise, this symbol is from the current package. If we've found an exported CollectorEntity, then use
+    // Exports.
+    const entity: CollectorEntity | undefined = this._collector.tryGetEntityForSymbol(symbol);
+    if (entity && entity.exported) {
+      return Navigation.Exports;
+    }
 
-        return Navigation.Exports;
+    // If its parent symbol is not a source file, then use either Exports or Members. If the parent symbol
+    // is a source file, but it wasn't exported from the package entry point (in the check above), then the
+    // symbol is a local, so fall through below.
+    if (parent && !DeclarationReferenceGenerator._isExternalModuleSymbol(parent)) {
+      if (
+        parent.members &&
+        DeclarationReferenceGenerator._isSameSymbol(parent.members.get(symbol.escapedName), symbol)
+      ) {
+        return Navigation.Members;
       }
+
+      return Navigation.Exports;
     }
 
     // Otherwise, we have a local symbol, so use a Locals navigation. These are either:
@@ -217,10 +200,12 @@ export class DeclarationReferenceGenerator {
       followedSymbol = this._collector.typeChecker.getExportSymbolOfSymbol(followedSymbol);
     }
     if (followedSymbol.flags & ts.SymbolFlags.Alias) {
-      const nextFollowedSymbol: ts.Symbol = this._collector.typeChecker.getAliasedSymbol(followedSymbol);
-      // do not follow alias to namespace, as it results in a source file that no longer knows its short name:
-      if (!(nextFollowedSymbol.flags & ts.SymbolFlags.Namespace)) {
-        followedSymbol = nextFollowedSymbol;
+      followedSymbol = this._collector.typeChecker.getAliasedSymbol(followedSymbol);
+
+      // Without this logic, we end up following the symbol `ns` in `import * as ns from './file'` to
+      // the actual file `file.ts`. We don't want to do this, so revert to the original symbol.
+      if (followedSymbol.flags & ts.SymbolFlags.ValueModule) {
+        followedSymbol = symbol;
       }
     }
 
@@ -241,7 +226,12 @@ export class DeclarationReferenceGenerator {
       return undefined;
     }
 
-    let localName: string = this._collector.getEmitName(followedSymbol);
+    let localName: string = followedSymbol.name;
+    const entity: CollectorEntity | undefined = this._collector.tryGetEntityForSymbol(followedSymbol);
+    if (entity?.nameForEmit) {
+      localName = entity.nameForEmit;
+    }
+
     if (followedSymbol.escapedName === ts.InternalSymbolName.Constructor) {
       localName = 'constructor';
     } else {
@@ -279,10 +269,36 @@ export class DeclarationReferenceGenerator {
   }
 
   private _getParentReference(symbol: ts.Symbol): DeclarationReference | undefined {
-    // First case: the symbol is exported via a namespace
-    const exportingNamespace: ts.Symbol | undefined = this._collector.getExportingNamespace(symbol);
-    if (exportingNamespace) {
-      return this._symbolToDeclarationReference(exportingNamespace, exportingNamespace.flags, true);
+    const declaration: ts.Node | undefined = TypeScriptHelpers.tryGetADeclaration(symbol);
+    const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile();
+
+    // Note that it's possible for a symbol to be exported from an entry point as well as one or more
+    // namespaces. In that case, it's not clear what to choose as its parent. Today's logic is neither
+    // perfect nor particularly stable to API items being renamed and shuffled around.
+    const entity: CollectorEntity | undefined = this._collector.tryGetEntityForSymbol(symbol);
+    if (entity) {
+      if (entity.exportedFromEntryPoint) {
+        return new DeclarationReference(this._sourceFileToModuleSource(sourceFile));
+      }
+
+      const firstExportingConsumableParent: CollectorEntity | undefined =
+        entity.firstExportingConsumableParent;
+      if (
+        firstExportingConsumableParent &&
+        firstExportingConsumableParent.astEntity instanceof AstNamespaceImport
+      ) {
+        const parentSymbol: ts.Symbol | undefined = TypeScriptInternals.tryGetSymbolForDeclaration(
+          firstExportingConsumableParent.astEntity.declaration,
+          this._collector.typeChecker
+        );
+        if (parentSymbol) {
+          return this._symbolToDeclarationReference(
+            parentSymbol,
+            parentSymbol.flags,
+            /*includeModuleSymbols*/ true
+          );
+        }
+      }
     }
 
     // Next, try to find a parent symbol via the symbol tree.
@@ -307,7 +323,6 @@ export class DeclarationReferenceGenerator {
     //
     // In the example above, `SomeType` doesn't have a parent symbol per the TS internal API above,
     // but its reference still needs to be qualified with the parent reference for `n`.
-    const declaration: ts.Node | undefined = TypeScriptHelpers.tryGetADeclaration(symbol);
     const grandParent: ts.Node | undefined = declaration?.parent?.parent;
     if (grandParent && ts.isModuleDeclaration(grandParent)) {
       const grandParentSymbol: ts.Symbol | undefined = TypeScriptInternals.tryGetSymbolForDeclaration(
@@ -324,7 +339,6 @@ export class DeclarationReferenceGenerator {
     }
 
     // At this point, we have a local symbol in a module.
-    const sourceFile: ts.SourceFile | undefined = declaration?.getSourceFile();
     if (sourceFile && ts.isExternalModule(sourceFile)) {
       return new DeclarationReference(this._sourceFileToModuleSource(sourceFile));
     } else {

build-tests/api-extractor-scenarios/config/build-config.json

@@ -41,6 +41,8 @@
     "mergedDeclarations",
     "mixinPattern",
     "namedDefaultImport",
+    "namespaceImports",
+    "namespaceImports2",
     "preapproved",
     "readonlyDeclarations",
     "referenceTokens",

build-tests/api-extractor-scenarios/etc/includeForgottenExports/api-extractor-scenarios.api.json

@@ -468,7 +468,7 @@
             {
               "kind": "Reference",
               "text": "ForgottenExport4.ForgottenExport5",
-              "canonicalReference": "api-extractor-scenarios!ForgottenExport4.ForgottenExport5:class"
+              "canonicalReference": "api-extractor-scenarios!~ForgottenExport4.ForgottenExport5:class"
             },
             {
               "kind": "Content",