Merge pull request #4498 from Josmithr/dont-mark-external-items-as-un…

…documented

fix(api-extractor): Don't mark items documented with {@inheritdoc} references to package-external items as "undocumented"

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

@@ -75,7 +75,19 @@ export class ApiItemMetadata {
    */
   public tsdocComment: tsdoc.DocComment | undefined;
 
-  // Assigned by DocCommentEnhancer
+  /**
+   * Tracks whether or not the associated API item is known to be missing sufficient documentation.
+   *
+   * @remarks
+   *
+   * An "undocumented" item is one whose TSDoc comment which either does not contain a summary comment block, or
+   * has an `@inheritDoc` tag that resolves to another "undocumented" API member.
+   *
+   * If there is any ambiguity (e.g. if an `@inheritDoc` comment points to an external API member, whose documentation,
+   * we can't parse), "undocumented" will be `false`.
+   *
+   * @remarks Assigned by {@link DocCommentEnhancer}.
+   */
   public undocumented: boolean = true;
 
   public docCommentEnhancerVisitorState: VisitorState = VisitorState.Unvisited;

apps/api-extractor/src/enhancers/DocCommentEnhancer.ts

@@ -131,16 +131,43 @@ export class DocCommentEnhancer {
         );
       }
       return;
-    }
-
-    if (metadata.tsdocComment) {
-      // Require the summary to contain at least 10 non-spacing characters
-      metadata.undocumented = !tsdoc.PlainTextEmitter.hasAnyTextContent(
-        metadata.tsdocComment.summarySection,
-        10
-      );
     } else {
-      metadata.undocumented = true;
+      // For non-constructor items, we will determine whether or not the item is documented as follows:
+      // 1. If it contains a summary section with at least 10 characters, then it is considered "documented".
+      // 2. If it contains an @inheritDoc tag, then it *may* be considered "documented", depending on whether or not
+      //    the tag resolves to a "documented" API member.
+      //    - Note: for external members, we cannot currently determine this, so we will consider the "documented"
+      //      status to be unknown.
+      if (metadata.tsdocComment) {
+        if (tsdoc.PlainTextEmitter.hasAnyTextContent(metadata.tsdocComment.summarySection, 10)) {
+          // If the API item has a summary comment block (with at least 10 characters), mark it as "documented".
+          metadata.undocumented = false;
+        } else if (metadata.tsdocComment.inheritDocTag) {
+          if (
+            this._refersToDeclarationInWorkingPackage(
+              metadata.tsdocComment.inheritDocTag.declarationReference
+            )
+          ) {
+            // If the API item has an `@inheritDoc` comment that points to an API item in the working package,
+            // then the documentation contents should have already been copied from the target via `_applyInheritDoc`.
+            // The continued existence of the tag indicates that the declaration reference was invalid, and not
+            // documentation contents could be copied.
+            // An analyzer issue will have already been logged for this.
+            // We will treat such an API as "undocumented".
+            metadata.undocumented = true;
+          } else {
+            // If the API item has an `@inheritDoc` comment that points to an external API item, we cannot currently
+            // determine whether or not the target is "documented", so we cannot say definitively that this is "undocumented".
+            metadata.undocumented = false;
+          }
+        } else {
+          // If the API item has neither a summary comment block, nor an `@inheritDoc` comment, mark it as "undocumented".
+          metadata.undocumented = true;
+        }
+      } else {
+        // If there is no tsdoc comment at all, mark "undocumented".
+        metadata.undocumented = true;
+      }
     }
   }
 
@@ -157,10 +184,7 @@ export class DocCommentEnhancer {
         // Is it referring to the working package?  If not, we don't do any link validation, because
         // AstReferenceResolver doesn't support it yet (but ModelReferenceResolver does of course).
         // Tracked by:  https://github.com/microsoft/rushstack/issues/1195
-        if (
-          node.codeDestination.packageName === undefined ||
-          node.codeDestination.packageName === this._collector.workingPackage.name
-        ) {
+        if (this._refersToDeclarationInWorkingPackage(node.codeDestination)) {
           const referencedAstDeclaration: AstDeclaration | ResolverFailure =
             this._collector.astReferenceResolver.resolve(node.codeDestination);
 
@@ -196,14 +220,8 @@ export class DocCommentEnhancer {
       return;
     }
 
-    // Is it referring to the working package?
-    if (
-      !(
-        inheritDocTag.declarationReference.packageName === undefined ||
-        inheritDocTag.declarationReference.packageName === this._collector.workingPackage.name
-      )
-    ) {
-      // It's referencing an external package, so skip this inheritDoc tag, since AstReferenceResolver doesn't
+    if (!this._refersToDeclarationInWorkingPackage(inheritDocTag.declarationReference)) {
+      // The `@inheritDoc` tag is referencing an external package. Skip it, since AstReferenceResolver doesn't
       // support it yet.  As a workaround, this tag will get handled later by api-documenter.
       // Tracked by:  https://github.com/microsoft/rushstack/issues/1195
       return;
@@ -249,4 +267,16 @@ export class DocCommentEnhancer {
 
     targetDocComment.inheritDocTag = undefined;
   }
+
+  /**
+   * Determines whether or not the provided declaration reference points to an item in the working package.
+   */
+  private _refersToDeclarationInWorkingPackage(
+    declarationReference: tsdoc.DocDeclarationReference | undefined
+  ): boolean {
+    return (
+      declarationReference?.packageName === undefined ||
+      declarationReference.packageName === this._collector.workingPackage.name
+    );
+  }
 }

build-tests/api-extractor-scenarios/etc/docReferences/api-extractor-scenarios.api.md

@@ -23,7 +23,7 @@ export namespace MyNamespace {
     }
 }
 
-// @public (undocumented)
+// @public
 export function succeedForNow(): void;
 
 // @public