[api-extractor] Allow separate release tags for overloaded functions and methods

apps/api-extractor/src/api/ExtractorMessageId.ts

@@ -47,6 +47,11 @@ export const enum ExtractorMessageId {
    */
   InternalMissingUnderscore = 'ae-internal-missing-underscore',
 
+  /**
+   * "Mixed release tags are not allowed for ___ because one of its declarations is marked as `@internal`."
+   */
+  InternalMixedReleaseTag = 'ae-internal-mixed-release-tag',
+
   /**
    * "The `@preapproved` tag cannot be applied to ___ because it is not a supported declaration type."
    */
@@ -91,6 +96,7 @@ export const allExtractorMessageIds: Set<string> = new Set<string>([
   'ae-misplaced-package-tag',
   'ae-forgotten-export',
   'ae-internal-missing-underscore',
+  'ae-internal-mixed-release-tag',
   'ae-preapproved-unsupported-type',
   'ae-preapproved-bad-release-tag',
   'ae-unresolved-inheritdoc-reference',

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

@@ -511,70 +511,21 @@ export class Collector {
       this._calculateMetadataForDeclaration(astDeclaration);
     }
 
-    // We know we solved parentAstSymbol.metadata above
-    const parentSymbolMetadata: SymbolMetadata | undefined = astSymbol.parentAstSymbol
-      ? astSymbol.parentAstSymbol.metadata as SymbolMetadata : undefined;
-
-    const symbolMetadata: SymbolMetadata = new SymbolMetadata();
-
-    // Do any of the declarations have a release tag?
-    let effectiveReleaseTag: ReleaseTag = ReleaseTag.None;
+    // The most public effectiveReleaseTag for all declarations
+    let maxEffectiveReleaseTag: ReleaseTag = ReleaseTag.None;
 
     for (const astDeclaration of astSymbol.astDeclarations) {
       // We know we solved this above
       const declarationMetadata: DeclarationMetadata = astDeclaration.metadata as DeclarationMetadata;
+      const effectiveReleaseTag: ReleaseTag = declarationMetadata.effectiveReleaseTag;
 
-      const declaredReleaseTag: ReleaseTag = declarationMetadata.declaredReleaseTag;
-
-      if (declaredReleaseTag !== ReleaseTag.None) {
-        if (effectiveReleaseTag !== ReleaseTag.None && effectiveReleaseTag !== declaredReleaseTag) {
-          if (!astSymbol.isExternal) { // for now, don't report errors for external code
-            this.messageRouter.addAnalyzerIssue(
-              ExtractorMessageId.DifferentReleaseTags,
-              'This symbol has another declaration with a different release tag',
-              astDeclaration
-            );
-          }
-        } else {
-          effectiveReleaseTag = declaredReleaseTag;
-        }
-      }
-    }
-
-    // If this declaration doesn't have a release tag, then inherit it from the parent
-    if (effectiveReleaseTag === ReleaseTag.None && astSymbol.parentAstSymbol) {
-      if (parentSymbolMetadata) {
-        effectiveReleaseTag = parentSymbolMetadata.releaseTag;
-      }
-    }
-
-    if (effectiveReleaseTag === ReleaseTag.None) {
-      if (!astSymbol.isExternal) { // for now, don't report errors for external code
-        // Don't report missing release tags for forgotten exports
-        const entity: CollectorEntity | undefined = this._entitiesByAstEntity.get(astSymbol.rootAstSymbol);
-        if (entity && entity.exported) {
-          // We also don't report errors for the default export of an entry point, since its doc comment
-          // isn't easy to obtain from the .d.ts file
-          if (astSymbol.rootAstSymbol.localName !== '_default') {
-
-            this.messageRouter.addAnalyzerIssue(
-              ExtractorMessageId.MissingReleaseTag,
-              `"${entity.astEntity.localName}" is exported by the package, but it is missing `
-              + `a release tag (@alpha, @beta, @public, or @internal)`,
-              astSymbol
-            );
-          }
-        }
+      if (effectiveReleaseTag > maxEffectiveReleaseTag) {
+        maxEffectiveReleaseTag = effectiveReleaseTag;
       }
-
-      effectiveReleaseTag = ReleaseTag.Public;
     }
 
-    symbolMetadata.releaseTag = effectiveReleaseTag;
-    symbolMetadata.releaseTagSameAsParent = false;
-    if (parentSymbolMetadata) {
-      symbolMetadata.releaseTagSameAsParent = symbolMetadata.releaseTag === parentSymbolMetadata.releaseTag;
-    }
+    const symbolMetadata: SymbolMetadata = new SymbolMetadata();
+    symbolMetadata.maxEffectiveReleaseTag = maxEffectiveReleaseTag;
 
     // Update this last when we're sure no exceptions were thrown
     astSymbol.metadata = symbolMetadata;
@@ -664,6 +615,41 @@ export class Collector {
         }
       }
     }
+
+    // This needs to be set regardless of whether or not a parserContext exists
+    if (astDeclaration.parent) {
+      const parentDeclarationMetadata: DeclarationMetadata = this.fetchMetadata(astDeclaration.parent);
+      declarationMetadata.effectiveReleaseTag = declarationMetadata.declaredReleaseTag === ReleaseTag.None
+        ? parentDeclarationMetadata.effectiveReleaseTag
+        : declarationMetadata.declaredReleaseTag;
+
+      declarationMetadata.releaseTagSameAsParent =
+        parentDeclarationMetadata.effectiveReleaseTag === declarationMetadata.effectiveReleaseTag;
+    } else {
+      declarationMetadata.effectiveReleaseTag = declarationMetadata.declaredReleaseTag;
+    }
+
+    if (declarationMetadata.effectiveReleaseTag === ReleaseTag.None) {
+      if (!astDeclaration.astSymbol.isExternal) { // for now, don't report errors for external code
+        // Don't report missing release tags for forgotten exports
+        const astSymbol: AstSymbol = astDeclaration.astSymbol;
+        const entity: CollectorEntity | undefined = this._entitiesByAstEntity.get(astSymbol.rootAstSymbol);
+        if (entity && entity.exported) {
+          // We also don't report errors for the default export of an entry point, since its doc comment
+          // isn't easy to obtain from the .d.ts file
+          if (astSymbol.rootAstSymbol.localName !== '_default') {
+            this.messageRouter.addAnalyzerIssue(
+              ExtractorMessageId.MissingReleaseTag,
+              `"${entity.astEntity.localName}" is exported by the package, but it is missing `
+              + `a release tag (@alpha, @beta, @public, or @internal)`,
+              astSymbol
+            );
+          }
+        }
+      }
+
+      declarationMetadata.effectiveReleaseTag = ReleaseTag.Public;
+    }
   }
 
   private _parseTsdocForAstDeclaration(astDeclaration: AstDeclaration): tsdoc.ParserContext | undefined {

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

@@ -20,11 +20,17 @@ export class DeclarationMetadata {
 
   /**
    * This is the release tag that was explicitly specified in the original doc comment, if any.
-   * Compare with SymbolMetadata.releaseTag, which is the effective release tag, possibly inherited from
-   * a parent.
    */
   public declaredReleaseTag: ReleaseTag = ReleaseTag.None;
 
+  /**
+   * The "effective" release tag is a normalized value that is based on `declaredReleaseTag`,
+   * but may be inherited from a parent, or corrected if the declared value was somehow invalid.
+   * When actually trimming .d.ts files or generating docs, API Extractor uses the "effective" value
+   * instead of the "declared" value.
+   */
+  public effectiveReleaseTag: ReleaseTag = ReleaseTag.None;
+
   // NOTE: In the future, the Collector may infer or error-correct some of these states.
   // Generators should rely on these instead of tsdocComment.modifierTagSet.
   public isEventProperty: boolean = false;
@@ -37,5 +43,8 @@ export class DeclarationMetadata {
   // Assigned by DocCommentEnhancer
   public needsDocumentation: boolean = true;
 
+  // If true, then it would be redundant to show this release tag
+  public releaseTagSameAsParent: boolean = false;
+
   public docCommentEnhancerVisitorState: VisitorState = VisitorState.Unvisited;
 }

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

@@ -4,8 +4,7 @@
 import { ReleaseTag } from '@microsoft/api-extractor-model';
 
 export class SymbolMetadata {
-  public releaseTag: ReleaseTag = ReleaseTag.None;
-
-  // If true, then it would be redundant to show this release tag
-  public releaseTagSameAsParent: boolean = false;
+  // For all declarations associated with this symbol, this is the
+  // `DeclarationMetadata.effectiveReleaseTag` value that is most public.
+  public maxEffectiveReleaseTag: ReleaseTag = ReleaseTag.None;
 }

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

@@ -12,7 +12,6 @@ import { AedocDefinitions, ReleaseTag } from '@microsoft/api-extractor-model';
 import { ExtractorMessageId } from '../api/ExtractorMessageId';
 import { VisitorState } from '../collector/VisitorState';
 import { ResolverFailure } from '../analyzer/AstReferenceResolver';
-import { SymbolMetadata } from '../collector/SymbolMetadata';
 
 export class DocCommentEnhancer {
   private readonly _collector: Collector;
@@ -93,9 +92,8 @@ export class DocCommentEnhancer {
         ]);
       }
 
-      const symbolMetadata: SymbolMetadata = this._collector.fetchMetadata(astDeclaration.astSymbol);
-
-      if (symbolMetadata.releaseTag === ReleaseTag.Internal) {
+      const declarationMetadata: DeclarationMetadata = this._collector.fetchMetadata(astDeclaration);
+      if (declarationMetadata.effectiveReleaseTag === ReleaseTag.Internal) {
         // If the constructor is marked as internal, then add a boilerplate notice for the containing class
         const classMetadata: DeclarationMetadata = this._collector.fetchMetadata(classDeclaration);
 

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

@@ -7,6 +7,7 @@ import * as ts from 'typescript';
 import { Collector } from '../collector/Collector';
 import { AstSymbol } from '../analyzer/AstSymbol';
 import { AstDeclaration } from '../analyzer/AstDeclaration';
+import { DeclarationMetadata } from '../collector/DeclarationMetadata';
 import { SymbolMetadata } from '../collector/SymbolMetadata';
 import { CollectorEntity } from '../collector/CollectorEntity';
 import { ExtractorMessageId } from '../api/ExtractorMessageId';
@@ -24,18 +25,59 @@ export class ValidationEnhancer {
             ValidationEnhancer._checkReferences(collector, astDeclaration, alreadyWarnedSymbols);
           });
 
-          ValidationEnhancer._checkForInternalUnderscore(collector, entity, entity.astEntity);
+          const symbolMetadata: SymbolMetadata = collector.fetchMetadata(entity.astEntity);
+          ValidationEnhancer._checkForInternalUnderscore(collector, entity, entity.astEntity, symbolMetadata);
+          ValidationEnhancer._checkForInconsistentReleaseTags(collector, entity.astEntity, symbolMetadata);
         }
       }
     }
   }
 
-  private static _checkForInternalUnderscore(collector: Collector, collectorEntity: CollectorEntity,
-    astSymbol: AstSymbol): void {
+  private static _checkForInternalUnderscore(
+    collector: Collector,
+    collectorEntity: CollectorEntity,
+    astSymbol: AstSymbol,
+    symbolMetadata: SymbolMetadata
+  ): void {
 
-    const astSymbolMetadata: SymbolMetadata = collector.fetchMetadata(astSymbol);
+    let needsUnderscore: boolean = false;
 
-    if (astSymbolMetadata.releaseTag === ReleaseTag.Internal && !astSymbolMetadata.releaseTagSameAsParent) {
+    if (symbolMetadata.maxEffectiveReleaseTag === ReleaseTag.Internal) {
+      if (!astSymbol.parentAstSymbol) {
+        // If it's marked as @internal and has no parent, then it needs and underscore.
+        // We use maxEffectiveReleaseTag because a merged declaration would NOT need an underscore in a case like this:
+        //
+        //   /** @public */
+        //   export enum X { }
+        //
+        //   /** @internal */
+        //   export namespace X { }
+        //
+        // (The above normally reports an error "ae-different-release-tags", but that may be suppressed.)
+        needsUnderscore = true;
+      } else {
+        // If it's marked as @internal and the parent isn't obviously already @internal, then it needs an underscore.
+        //
+        // For example, we WOULD need an underscore for a merged declaration like this:
+        //
+        //   /** @internal */
+        //   export namespace X {
+        //     export interface _Y { }
+        //   }
+        //
+        //   /** @public */
+        //   export class X {
+        //     /** @internal */
+        //     public static _Y(): void { }   // <==== different from parent
+        //   }
+        const parentSymbolMetadata: SymbolMetadata = collector.fetchMetadata(astSymbol);
+        if (parentSymbolMetadata.maxEffectiveReleaseTag > ReleaseTag.Internal) {
+          needsUnderscore = true;
+        }
+      }
+    }
+
+    if (needsUnderscore) {
       for (const exportName of collectorEntity.exportNames) {
         if (exportName[0] !== '_') {
           collector.messageRouter.addAnalyzerIssue(
@@ -48,14 +90,80 @@ export class ValidationEnhancer {
         }
       }
     }
-
   }
 
-  private static _checkReferences(collector: Collector, astDeclaration: AstDeclaration,
-    alreadyWarnedSymbols: Set<AstSymbol>): void {
+  private static _checkForInconsistentReleaseTags(
+    collector: Collector,
+    astSymbol: AstSymbol,
+    symbolMetadata: SymbolMetadata
+  ): void {
+    if (astSymbol.isExternal) {
+      // For now, don't report errors for external code.  If the developer cares about it, they should run
+      // API Extractor separately on the external project
+      return;
+    }
+
+    // Normally we will expect all release tags to be the same.  Arbitrarily we choose the maxEffectiveReleaseTag
+    // as the thing they should all match.
+    const expectedEffectiveReleaseTag: ReleaseTag = symbolMetadata.maxEffectiveReleaseTag;
+
+    // This is set to true if we find a declaration whose release tag is different from expectedEffectiveReleaseTag
+    let mixedReleaseTags: boolean = false;
+
+    // This is set to false if we find a declaration that is not a function/method overload
+    let onlyFunctionOverloads: boolean = true;
+
+    // This is set to true if we find a declaration that is @internal
+    let anyInternalReleaseTags: boolean = false;
+
+    for (const astDeclaration of astSymbol.astDeclarations) {
+      const declarationMetadata: DeclarationMetadata = collector.fetchMetadata(astDeclaration);
+      const effectiveReleaseTag: ReleaseTag = declarationMetadata.effectiveReleaseTag;
+
+      switch (astDeclaration.declaration.kind) {
+        case ts.SyntaxKind.FunctionDeclaration:
+        case ts.SyntaxKind.MethodDeclaration:
+          break;
+        default:
+          onlyFunctionOverloads = false;
+      }
+
+      if (effectiveReleaseTag !== expectedEffectiveReleaseTag) {
+        mixedReleaseTags = true;
+      }
+
+      if (effectiveReleaseTag === ReleaseTag.Internal) {
+        anyInternalReleaseTags = true;
+      }
+    }
+
+    if (mixedReleaseTags) {
+      if (!onlyFunctionOverloads) {
+        collector.messageRouter.addAnalyzerIssue(
+          ExtractorMessageId.DifferentReleaseTags,
+          'This symbol has another declaration with a different release tag',
+          astSymbol
+        );
+      }
+
+      if (anyInternalReleaseTags) {
+        collector.messageRouter.addAnalyzerIssue(
+          ExtractorMessageId.InternalMixedReleaseTag,
+          `Mixed release tags are not allowed for "${astSymbol.localName}" because one of its declarations` +
+          ` is marked as @internal`,
+          astSymbol
+        );
+      }
+    }
+  }
 
-    const astSymbolMetadata: SymbolMetadata = collector.fetchMetadata(astDeclaration.astSymbol);
-    const astSymbolReleaseTag: ReleaseTag = astSymbolMetadata.releaseTag;
+  private static _checkReferences(
+    collector: Collector,
+    astDeclaration: AstDeclaration,
+    alreadyWarnedSymbols: Set<AstSymbol>
+  ): void {
+    const declarationMetadata: DeclarationMetadata = collector.fetchMetadata(astDeclaration);
+    const declarationReleaseTag: ReleaseTag = declarationMetadata.effectiveReleaseTag;
 
     for (const referencedEntity of astDeclaration.referencedAstEntities) {
 
@@ -71,12 +179,12 @@ export class ValidationEnhancer {
 
           if (collectorEntity && collectorEntity.exported) {
             const referencedMetadata: SymbolMetadata = collector.fetchMetadata(referencedEntity);
-            const referencedReleaseTag: ReleaseTag = referencedMetadata.releaseTag;
+            const referencedReleaseTag: ReleaseTag = referencedMetadata.maxEffectiveReleaseTag;
 
-            if (ReleaseTag.compare(astSymbolReleaseTag, referencedReleaseTag) > 0) {
+            if (ReleaseTag.compare(declarationReleaseTag, referencedReleaseTag) > 0) {
               collector.messageRouter.addAnalyzerIssue(ExtractorMessageId.IncompatibleReleaseTags,
                 `The symbol "${astDeclaration.astSymbol.localName}"`
-                + ` is marked as ${ReleaseTag.getTagName(astSymbolReleaseTag)},`
+                + ` is marked as ${ReleaseTag.getTagName(declarationReleaseTag)},`
                 + ` but its signature references "${referencedEntity.localName}"`
                 + ` which is marked as ${ReleaseTag.getTagName(referencedReleaseTag)}`,
                 astDeclaration);