[api-extractor] Fix an issue where destructured parameters were handled incorrectly

apps/api-extractor/.vscode/launch.json

@@ -79,7 +79,7 @@
         "run",
         "--local",
         "--config",
-        "./temp/configs/api-extractor-spanSorting.json"
+        "./temp/configs/api-extractor-destructuredParameters.json"
       ],
       "sourceMaps": true
     }

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

@@ -25,6 +25,12 @@ import { ExtractorMessageId } from '../api/ExtractorMessageId';
 import type { ApiReportVariant } from '../api/IConfigFile';
 import type { SymbolMetadata } from '../collector/SymbolMetadata';
 
+interface IContext {
+  collector: Collector;
+  reportVariant: ApiReportVariant;
+  alreadyProcessedSignatures: Set<Span>;
+}
+
 export class ApiReportGenerator {
   private static _trimSpacesRegExp: RegExp = / +$/gm;
 
@@ -92,6 +98,12 @@ export class ApiReportGenerator {
     }
     writer.ensureSkippedLine();
 
+    const context: IContext = {
+      collector,
+      reportVariant,
+      alreadyProcessedSignatures: new Set()
+    };
+
     // Emit the regular declarations
     for (const entity of collector.entities) {
       const astEntity: AstEntity = entity.astEntity;
@@ -152,7 +164,7 @@ export class ApiReportGenerator {
               if (apiItemMetadata.isPreapproved) {
                 ApiReportGenerator._modifySpanForPreapproved(span);
               } else {
-                ApiReportGenerator._modifySpan(collector, span, entity, astDeclaration, false, reportVariant);
+                ApiReportGenerator._modifySpan(span, entity, astDeclaration, false, context);
               }
 
               span.writeModifiedText(writer);
@@ -276,13 +288,14 @@ export class ApiReportGenerator {
    * Before writing out a declaration, _modifySpan() applies various fixups to make it nice.
    */
   private static _modifySpan(
-    collector: Collector,
     span: Span,
     entity: CollectorEntity,
     astDeclaration: AstDeclaration,
     insideTypeLiteral: boolean,
-    reportVariant: ApiReportVariant
+    context: IContext
   ): void {
+    const { collector, reportVariant } = context;
+
     // Should we process this declaration at all?
     if (!ApiReportGenerator._shouldIncludeDeclaration(collector, astDeclaration, reportVariant)) {
       span.modification.skipAll();
@@ -382,6 +395,19 @@ export class ApiReportGenerator {
         }
         break;
 
+      case ts.SyntaxKind.Parameter:
+        {
+          // (signature) -> SyntaxList -> Parameter
+          const signatureParent: Span | undefined = span.parent;
+          if (signatureParent) {
+            if (!context.alreadyProcessedSignatures.has(signatureParent)) {
+              context.alreadyProcessedSignatures.add(signatureParent);
+              DtsEmitHelpers.normalizeParameterNames(signatureParent);
+            }
+          }
+        }
+        break;
+
       case ts.SyntaxKind.Identifier:
         const referencedEntity: CollectorEntity | undefined = collector.tryGetEntityForNode(
           span.node as ts.Identifier
@@ -414,12 +440,11 @@ export class ApiReportGenerator {
           astDeclaration,
           (childSpan, childAstDeclaration) => {
             ApiReportGenerator._modifySpan(
-              collector,
               childSpan,
               entity,
               childAstDeclaration,
               insideTypeLiteral,
-              reportVariant
+              context
             );
           }
         );
@@ -460,14 +485,7 @@ export class ApiReportGenerator {
           }
         }
 
-        ApiReportGenerator._modifySpan(
-          collector,
-          child,
-          entity,
-          childAstDeclaration,
-          insideTypeLiteral,
-          reportVariant
-        );
+        ApiReportGenerator._modifySpan(child, entity, childAstDeclaration, insideTypeLiteral, context);
       }
     }
   }

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

@@ -190,4 +190,106 @@ export class DtsEmitHelpers {
     }
     return false;
   }
+
+  /**
+   * Given an array that includes some parameter nodes, this returns an array of the same length;
+   * elements that are not undefined correspond to a parameter that should be renamed.
+   */
+  public static forEachParameterToNormalize(
+    nodes: ArrayLike<ts.Node>,
+    action: (parameter: ts.ParameterDeclaration, syntheticName: string | undefined) => void
+  ): void {
+    let actionIndex: number = 0;
+
+    // Optimistically assume that no parameters need to be normalized
+    for (actionIndex = 0; actionIndex < nodes.length; ++actionIndex) {
+      const parameter: ts.Node = nodes[actionIndex];
+      if (!ts.isParameter(parameter)) {
+        continue;
+      }
+      action(parameter, undefined);
+      if (ts.isObjectBindingPattern(parameter.name) || ts.isArrayBindingPattern(parameter.name)) {
+        // Our optimistic assumption was not true; we'll need to stop and calculate alreadyUsedNames
+        break;
+      }
+    }
+
+    if (actionIndex === nodes.length) {
+      // Our optimistic assumption was true
+      return;
+    }
+
+    // First, calculate alreadyUsedNames
+    const alreadyUsedNames: string[] = [];
+
+    for (let index: number = 0; index < nodes.length; ++index) {
+      const parameter: ts.Node = nodes[index];
+      if (!ts.isParameter(parameter)) {
+        continue;
+      }
+
+      if (!(ts.isObjectBindingPattern(parameter.name) || ts.isArrayBindingPattern(parameter.name))) {
+        alreadyUsedNames.push(parameter.name.text.trim());
+      }
+    }
+
+    // Now continue with the rest of the actions
+    for (; actionIndex < nodes.length; ++actionIndex) {
+      const parameter: ts.Node = nodes[actionIndex];
+      if (!ts.isParameter(parameter)) {
+        continue;
+      }
+
+      if (ts.isObjectBindingPattern(parameter.name) || ts.isArrayBindingPattern(parameter.name)) {
+        // Examples:
+        //
+        //      function f({ y, z }: { y: string, z: string })
+        // ---> function f(input: { y: string, z: string })
+        //
+        //      function f(x: number, [a, b]: [number, number])
+        // ---> function f(x: number, input: [number, number])
+        //
+        // Example of a naming collision:
+        //
+        //      function f({ a }: { a: string }, { b }: { b: string }, input2: string)
+        // ---> function f(input: { a: string }, input3: { b: string }, input2: string)
+        const baseName: string = 'input';
+        let counter: number = 2;
+
+        let syntheticName: string = baseName;
+        while (alreadyUsedNames.includes(syntheticName)) {
+          syntheticName = `${baseName}${counter++}`;
+        }
+        alreadyUsedNames.push(syntheticName);
+
+        action(parameter, syntheticName);
+      } else {
+        action(parameter, undefined);
+      }
+    }
+  }
+
+  public static normalizeParameterNames(signatureSpan: Span): void {
+    const syntheticNamesByNode: Map<ts.Node, string> = new Map();
+
+    DtsEmitHelpers.forEachParameterToNormalize(
+      signatureSpan.node.getChildren(),
+      (parameter: ts.ParameterDeclaration, syntheticName: string | undefined): void => {
+        if (syntheticName !== undefined) {
+          syntheticNamesByNode.set(parameter.name, syntheticName);
+        }
+      }
+    );
+
+    if (syntheticNamesByNode.size > 0) {
+      signatureSpan.forEach((childSpan: Span): void => {
+        const syntheticName: string | undefined = syntheticNamesByNode.get(childSpan.node);
+        if (syntheticName !== undefined) {
+          childSpan.modification.prefix = syntheticName;
+          childSpan.modification.suffix = '';
+          childSpan.modification.omitChildren = true;
+        }
+      });
+    }
+  }
 }

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

@@ -17,16 +17,22 @@ import type { AstDeclaration } from '../analyzer/AstDeclaration';
 /**
  * Used to provide ExcerptBuilder with a list of nodes whose token range we want to capture.
  */
-export interface IExcerptBuilderNodeToCapture {
+export interface IExcerptBuilderNodeTransform {
   /**
-   * The node to capture
+   * The node to process
    */
-  node: ts.Node | undefined;
+  node: ts.Node;
+
   /**
-   * The token range whose startIndex/endIndex will be overwritten with the indexes for the
-   * tokens corresponding to IExcerptBuilderNodeToCapture.node
+   * A token range whose startIndex/endIndex will be overwritten with the indexes for the
+   * tokens corresponding to IExcerptBuilderNodeTransform.node
    */
-  tokenRange: IExcerptTokenRange;
+  captureTokenRange?: IExcerptTokenRange;
+
+  /**
+   * Text that will replace the text of the given node during emit.
+   */
+  replacementText?: string;
 }
 
 /**
@@ -51,7 +57,7 @@ interface IBuildSpanState {
    */
   stopBeforeChildKind: ts.SyntaxKind | undefined;
 
-  tokenRangesByNode: Map<ts.Node, IExcerptTokenRange>;
+  transformsByNode: Map<ts.Node, IExcerptBuilderNodeTransform>;
 
   /**
    * Tracks whether the last appended token was a separator. If so, and we're in the middle of
@@ -80,12 +86,12 @@ export class ExcerptBuilder {
   /**
    * Appends the signature for the specified `AstDeclaration` to the `excerptTokens` list.
    * @param excerptTokens - The target token list to append to
-   * @param nodesToCapture - A list of child nodes whose token ranges we want to capture
+   * @param nodeTransforms - A list of child nodes whose token ranges we want to capture
    */
   public static addDeclaration(
     excerptTokens: IExcerptToken[],
     astDeclaration: AstDeclaration,
-    nodesToCapture: IExcerptBuilderNodeToCapture[],
+    nodeTransforms: IExcerptBuilderNodeTransform[],
     referenceGenerator: DeclarationReferenceGenerator
   ): void {
     let stopBeforeChildKind: ts.SyntaxKind | undefined = undefined;
@@ -105,38 +111,62 @@ export class ExcerptBuilder {
 
     const span: Span = new Span(astDeclaration.declaration);
 
-    const tokenRangesByNode: Map<ts.Node, IExcerptTokenRange> = new Map<ts.Node, IExcerptTokenRange>();
-    for (const excerpt of nodesToCapture || []) {
-      if (excerpt.node) {
-        tokenRangesByNode.set(excerpt.node, excerpt.tokenRange);
+    const transformsByNode: Map<ts.Node, IExcerptBuilderNodeTransform> = new Map();
+    const captureTokenRanges: IExcerptTokenRange[] = [];
+    for (const nodeTransform of nodeTransforms || []) {
+      transformsByNode.set(nodeTransform.node, nodeTransform);
+      if (nodeTransform.captureTokenRange) {
+        captureTokenRanges.push(nodeTransform.captureTokenRange);
       }
     }
 
     ExcerptBuilder._buildSpan(excerptTokens, span, {
       referenceGenerator: referenceGenerator,
       startingNode: span.node,
       stopBeforeChildKind,
-      tokenRangesByNode,
+      transformsByNode: transformsByNode,
       lastAppendedTokenIsSeparator: false
     });
-    ExcerptBuilder._condenseTokens(excerptTokens, [...tokenRangesByNode.values()]);
+
+    ExcerptBuilder._condenseTokens(excerptTokens, captureTokenRanges);
   }
 
   public static createEmptyTokenRange(): IExcerptTokenRange {
     return { startIndex: 0, endIndex: 0 };
   }
 
+  /** @returns false if we encountered a token that causes iteration to stop. */
   private static _buildSpan(excerptTokens: IExcerptToken[], span: Span, state: IBuildSpanState): boolean {
     if (span.kind === ts.SyntaxKind.JSDocComment) {
       // Discard any comments
       return true;
     }
 
     // Can this node start a excerpt?
-    const capturedTokenRange: IExcerptTokenRange | undefined = state.tokenRangesByNode.get(span.node);
+    const transform: IExcerptBuilderNodeTransform | undefined = state.transformsByNode.get(span.node);
+
+    let captureTokenRange: IExcerptTokenRange | undefined = undefined;
+
+    if (transform) {
+      captureTokenRange = transform.captureTokenRange;
+      if (transform.replacementText !== undefined) {
+        excerptTokens.push({
+          kind: ExcerptTokenKind.Content,
+          text: transform.replacementText
+        });
+        state.lastAppendedTokenIsSeparator = false;
+
+        if (captureTokenRange) {
+          captureTokenRange.startIndex = excerptTokens.length;
+          captureTokenRange.endIndex = captureTokenRange.startIndex + 1;
+        }
+        return true;
+      }
+    }
+
     let excerptStartIndex: number = 0;
 
-    if (capturedTokenRange) {
+    if (captureTokenRange) {
       // We will assign capturedTokenRange.startIndex to be the index of the next token to be appended
       excerptStartIndex = excerptTokens.length;
     }
@@ -187,8 +217,8 @@ export class ExcerptBuilder {
     }
 
     // Are we building a excerpt?  If so, set its range
-    if (capturedTokenRange) {
-      capturedTokenRange.startIndex = excerptStartIndex;
+    if (captureTokenRange) {
+      captureTokenRange.startIndex = excerptStartIndex;
 
       // We will assign capturedTokenRange.startIndex to be the index after the last token
       // that was appended so far. However, if the last appended token was a separator, omit
@@ -198,7 +228,7 @@ export class ExcerptBuilder {
         excerptEndIndex--;
       }
 
-      capturedTokenRange.endIndex = excerptEndIndex;
+      captureTokenRange.endIndex = excerptEndIndex;
     }
 
     return true;

build-tests/api-documenter-scenarios/.vscode/launch.json

@@ -0,0 +1,34 @@
+{
+  // Use IntelliSense to learn about possible attributes.
+  // Hover to view descriptions of existing attributes.
+  // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
+  "version": "0.2.0",
+  "configurations": [
+    {
+      "type": "node",
+      "request": "launch",
+      "name": "heft build (runScenarios.js)",
+      "program": "${workspaceFolder}/node_modules/@rushstack/heft/lib/start.js",
+      "cwd": "${workspaceFolder}",
+      "args": ["--debug", "build"],
+      "console": "integratedTerminal",
+      "sourceMaps": true
+    },
+    {
+      "type": "node",
+      "request": "launch",
+      "name": "api-documenter [scenario]",
+      "program": "${workspaceFolder}/node_modules/@microsoft/api-documenter/lib/start.js",
+      "cwd": "${workspaceFolder}/temp/configs",
+      "args": [
+        "generate",
+        "--input-folder",
+        "${workspaceFolder}/temp/etc/commentedDestructure",
+        "--output-folder",
+        "${workspaceFolder}/temp/etc/commentedDestructure/markdown"
+      ],
+      "console": "integratedTerminal",
+      "sourceMaps": true
+    }
+  ]
+}