Merge pull request #1170 from Microsoft/octogonz/ae-internal-missing-…

…underscore

[api-extractor] Implement "ae-internal-missing-underscore" warning

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

@@ -6,6 +6,24 @@ import * as path from 'path';
 import { ExtractorMessageId } from './ExtractorMessageId';
 import { Path, Text } from '@microsoft/node-core-library';
 
+/**
+ * Used by {@link ExtractorMessage.properties}.
+ *
+ * @public
+ */
+export interface IExtractorMessageProperties {
+
+  /**
+   * A declaration can have multiple names if it is exported more than once.
+   * If an `ExtractorMessage` applies to a specific export name, this property can indicate that.
+   *
+   * @remarks
+   *
+   * Used by {@link ExtractorMessageId.InternalMissingUnderscore}.
+   */
+  readonly exportName?: string;
+}
+
 /**
  * Specifies a category of messages for use with {@link ExtractorMessage}.
  * @public
@@ -49,6 +67,7 @@ export interface IExtractorMessageOptions {
   sourceFilePath?: string;
   sourceFileLine?: number;
   sourceFileColumn?: number;
+  properties?: IExtractorMessageProperties;
 }
 
 /**
@@ -90,6 +109,12 @@ export class ExtractorMessage {
    */
   public readonly sourceFileColumn: number | undefined;
 
+  /**
+   * Additional contextual information about the message that may be useful when reporting errors.
+   * All properties are optional.
+   */
+  public readonly properties: IExtractorMessageProperties;
+
   /** @internal */
   public constructor(options: IExtractorMessageOptions) {
     this.category = options.category;
@@ -98,6 +123,7 @@ export class ExtractorMessage {
     this.sourceFilePath = options.sourceFilePath;
     this.sourceFileLine = options.sourceFileLine;
     this.sourceFileColumn = options.sourceFileColumn;
+    this.properties = options.properties || { };
   }
 
   /**

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

@@ -40,7 +40,12 @@ export const enum ExtractorMessageId {
   /**
    * The symbol ___ needs to be exported by the entry point ___.
    */
-  ForgottenExport = 'ae-forgotten-export'
+  ForgottenExport = 'ae-forgotten-export',
+
+  /**
+   * The name ___ should be prefixed with an underscore because the declaration is marked as `@internal`.
+   */
+  InternalMissingUnderscore = 'ae-internal-missing-underscore'
 }
 
 export const allExtractorMessageIds: Set<string> = new Set<string>([
@@ -49,5 +54,6 @@ export const allExtractorMessageIds: Set<string> = new Set<string>([
   'ae-incompatible-release-tags',
   'ae-missing-release-tag',
   'ae-misplaced-package-tag',
-  'ae-forgotten-export'
+  'ae-forgotten-export',
+  'ae-internal-missing-underscore'
 ]);

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

@@ -12,7 +12,8 @@ import { AstSymbol } from '../analyzer/AstSymbol';
 import {
   ExtractorMessage,
   ExtractorMessageCategory,
-  IExtractorMessageOptions
+  IExtractorMessageOptions,
+  IExtractorMessageProperties
 } from '../api/ExtractorMessage';
 import { ExtractorMessageId, allExtractorMessageIds } from '../api/ExtractorMessageId';
 import {
@@ -164,7 +165,7 @@ export class MessageRouter {
    * Add a message from the API Extractor analysis
    */
   public addAnalyzerIssue(messageId: ExtractorMessageId, messageText: string,
-    astDeclarationOrSymbol: AstDeclaration | AstSymbol): void {
+    astDeclarationOrSymbol: AstDeclaration | AstSymbol, properties?: IExtractorMessageProperties): void {
 
     let astDeclaration: AstDeclaration;
     if (astDeclarationOrSymbol instanceof AstDeclaration) {
@@ -175,7 +176,7 @@ export class MessageRouter {
 
     const extractorMessage: ExtractorMessage = this.addAnalyzerIssueForPosition(
       messageId, messageText, astDeclaration.declaration.getSourceFile(),
-      astDeclaration.declaration.getStart());
+      astDeclaration.declaration.getStart(), properties);
 
     this._associateMessageWithAstDeclaration(extractorMessage, astDeclaration);
   }
@@ -231,7 +232,7 @@ export class MessageRouter {
    * Add a message for a location in an arbitrary source file.
    */
   public addAnalyzerIssueForPosition(messageId: ExtractorMessageId, messageText: string,
-    sourceFile: ts.SourceFile, pos: number): ExtractorMessage {
+    sourceFile: ts.SourceFile, pos: number, properties?: IExtractorMessageProperties): ExtractorMessage {
 
     const lineAndCharacter: ts.LineAndCharacter = sourceFile.getLineAndCharacterOfPosition(
       pos);
@@ -242,7 +243,8 @@ export class MessageRouter {
       text: messageText,
       sourceFilePath: sourceFile.fileName,
       sourceFileLine: lineAndCharacter.line + 1,
-      sourceFileColumn: lineAndCharacter.character + 1
+      sourceFileColumn: lineAndCharacter.character + 1,
+      properties
     };
 
     this._sourceMapper.updateExtractorMessageOptions(options);

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

@@ -21,11 +21,33 @@ export class VisibilityChecker {
             VisibilityChecker._checkReferences(collector, astDeclaration, alreadyWarnedSymbols);
           });
 
+          VisibilityChecker._checkForInternalUnderscore(collector, entity, entity.astEntity);
         }
       }
     }
   }
 
+  private static _checkForInternalUnderscore(collector: Collector, collectorEntity: CollectorEntity,
+    astSymbol: AstSymbol): void {
+
+    const astSymbolMetadata: SymbolMetadata = collector.fetchMetadata(astSymbol);
+
+    if (astSymbolMetadata.releaseTag === ReleaseTag.Internal && !astSymbolMetadata.releaseTagSameAsParent) {
+      for (const exportName of collectorEntity.exportNames) {
+        if (exportName[0] !== '_') {
+          collector.messageRouter.addAnalyzerIssue(
+            ExtractorMessageId.InternalMissingUnderscore,
+            `The name ${exportName} should be prefixed with an underscore`
+            + ` because the declaration is marked as "@internal"`,
+            astSymbol,
+            { exportName }
+          );
+        }
+      }
+    }
+
+  }
+
   private static _checkReferences(collector: Collector, astDeclaration: AstDeclaration,
     alreadyWarnedSymbols: Set<AstSymbol>): void {
 

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

@@ -62,11 +62,46 @@ export class ReviewFileGenerator {
     // Emit the regular declarations
     for (const entity of collector.entities) {
       if (entity.exported) {
+
+        // First, collect the list of export names for this symbol.  When reporting messages with
+        // ExtractorMessage.properties.exportName, this will enable us to emit the warning comments alongside
+        // the associated export statement.
+        interface IExportToEmit {
+          readonly exportName: string;
+          readonly associatedMessages: ExtractorMessage[];
+        }
+        const exportsToEmit: Map<string, IExportToEmit> = new Map<string, IExportToEmit>();
+
+        for (const exportName of entity.exportNames) {
+          if (!entity.shouldInlineExport) {
+            exportsToEmit.set(exportName, { exportName, associatedMessages: [] });
+          }
+        }
+
         if (entity.astEntity instanceof AstSymbol) {
-          // Emit all the declarations for this entry
+          // Emit all the declarations for this entity
           for (const astDeclaration of entity.astEntity.astDeclarations || []) {
 
-            stringWriter.write(ReviewFileGenerator._getAedocSynopsis(collector, astDeclaration));
+            // Get the messages associated with this declaration
+            const fetchedMessages: ExtractorMessage[] = collector.messageRouter
+              .fetchAssociatedMessagesForReviewFile(astDeclaration);
+
+            // Peel off the messages associated with an export statement and store them
+            // in IExportToEmit.associatedMessages (to be processed later).  The remaining messages will
+            // added to messagesToReport, to be emitted next to the declaration instead of the export statement.
+            const messagesToReport: ExtractorMessage[] = [];
+            for (const message of fetchedMessages) {
+              if (message.properties.exportName) {
+                const exportToEmit: IExportToEmit | undefined = exportsToEmit.get(message.properties.exportName);
+                if (exportToEmit) {
+                  exportToEmit.associatedMessages.push(message);
+                  continue;
+                }
+              }
+              messagesToReport.push(message);
+            }
+
+            stringWriter.write(ReviewFileGenerator._getAedocSynopsis(collector, astDeclaration, messagesToReport));
 
             const span: Span = new Span(astDeclaration.declaration);
             ReviewFileGenerator._modifySpan(collector, span, entity, astDeclaration, false);
@@ -75,11 +110,16 @@ export class ReviewFileGenerator {
           }
         }
 
-        for (const exportName of entity.exportNames) {
-          if (!entity.shouldInlineExport) {
-            DtsEmitHelpers.emitNamedExport(stringWriter, exportName, entity);
-            stringWriter.writeLine();
+        // Now emit the export statements for this entity.
+        for (const exportToEmit of exportsToEmit.values()) {
+          // Write any associated messages
+          for (const message of exportToEmit.associatedMessages) {
+            ReviewFileGenerator._writeLineAsComments(stringWriter,
+              'Warning: ' + message.formatMessageWithoutLocation());
           }
+
+          DtsEmitHelpers.emitNamedExport(stringWriter, exportToEmit.exportName, entity);
+          stringWriter.writeLine();
         }
       }
     }
@@ -245,7 +285,10 @@ export class ReviewFileGenerator {
           }
 
           if (!insideTypeLiteral) {
-            const aedocSynopsis: string = ReviewFileGenerator._getAedocSynopsis(collector, childAstDeclaration);
+            const messagesToReport: ExtractorMessage[] = collector.messageRouter
+              .fetchAssociatedMessagesForReviewFile(childAstDeclaration);
+            const aedocSynopsis: string = ReviewFileGenerator._getAedocSynopsis(collector, childAstDeclaration,
+              messagesToReport);
             const indentedAedocSynopsis: string = ReviewFileGenerator._addIndentAfterNewlines(aedocSynopsis,
               child.getIndent());
 
@@ -263,14 +306,12 @@ export class ReviewFileGenerator {
    * whether the item has been documented, and any warnings that were detected
    * by the analysis.
    */
-  private static _getAedocSynopsis(collector: Collector, astDeclaration: AstDeclaration): string {
-    const output: StringWriter = new StringWriter();
-
-    const messagesToReport: ExtractorMessage[] = collector.messageRouter
-      .fetchAssociatedMessagesForReviewFile(astDeclaration);
+  private static _getAedocSynopsis(collector: Collector, astDeclaration: AstDeclaration,
+    messagesToReport: ExtractorMessage[]): string {
+    const stringWriter: StringWriter = new StringWriter();
 
     for (const message of messagesToReport) {
-      ReviewFileGenerator._writeLineAsComments(output, 'Warning: ' + message.formatMessageWithoutLocation());
+      ReviewFileGenerator._writeLineAsComments(stringWriter, 'Warning: ' + message.formatMessageWithoutLocation());
     }
 
     const declarationMetadata: DeclarationMetadata = collector.fetchMetadata(astDeclaration);
@@ -312,13 +353,13 @@ export class ReviewFileGenerator {
 
     if (footerParts.length > 0) {
       if (messagesToReport.length > 0) {
-        ReviewFileGenerator._writeLineAsComments(output, ''); // skip a line after the warnings
+        ReviewFileGenerator._writeLineAsComments(stringWriter, ''); // skip a line after the warnings
       }
 
-      ReviewFileGenerator._writeLineAsComments(output, footerParts.join(' '));
+      ReviewFileGenerator._writeLineAsComments(stringWriter, footerParts.join(' '));
     }
 
-    return output.toString();
+    return stringWriter.toString();
   }
 
   private static _writeLineAsComments(stringWriter: StringWriter, line: string): void {

apps/api-extractor/src/index.ts

@@ -10,7 +10,11 @@
  */
 
 export { Extractor, IAnalyzeProjectOptions, IExtractorOptions } from './api/Extractor';
-export { ExtractorMessage, ExtractorMessageCategory } from './api/ExtractorMessage';
+export {
+  ExtractorMessage,
+  IExtractorMessageProperties,
+  ExtractorMessageCategory
+} from './api/ExtractorMessage';
 export { ExtractorMessageId } from './api/ExtractorMessageId';
 export {
   IExtractorTsconfigCompilerConfig,

apps/api-extractor/src/schemas/api-extractor-defaults.json

@@ -59,6 +59,10 @@
       "ae-incompatible-release-tags": {
         "logLevel": "warning",
         "addToApiReviewFile": true
+      },
+      "ae-internal-missing-underscore": {
+        "logLevel": "warning",
+        "addToApiReviewFile": true
       }
     },
     "tsdocMessageReporting": {

build-tests/api-extractor-scenarios/etc/test-outputs/exportDuplicate/api-extractor-scenarios.api.json

@@ -14,29 +14,6 @@
       "canonicalReference": "",
       "name": "",
       "members": [
-        {
-          "kind": "Class",
-          "canonicalReference": "(A:class)",
-          "docComment": "/**\n * @public\n */\n",
-          "excerptTokens": [
-            {
-              "kind": "Content",
-              "text": "declare class "
-            },
-            {
-              "kind": "Reference",
-              "text": "A"
-            },
-            {
-              "kind": "Content",
-              "text": " "
-            }
-          ],
-          "releaseTag": "Public",
-          "name": "A",
-          "members": [],
-          "implementsTokenRanges": []
-        },
         {
           "kind": "Class",
           "canonicalReference": "(X:class)",

build-tests/api-extractor-scenarios/etc/test-outputs/exportDuplicate/api-extractor-scenarios.api.md

@@ -4,12 +4,14 @@
 
 ```ts
 
-// @public (undocumented)
+// @internal (undocumented)
 class A {
 }
 
+// Warning: (ae-internal-missing-underscore) The name B should be prefixed with an underscore because the declaration is marked as "@internal"
 export { A as B }
 
+// Warning: (ae-internal-missing-underscore) The name C should be prefixed with an underscore because the declaration is marked as "@internal"
 export { A as C }
 
 // @public (undocumented)

build-tests/api-extractor-scenarios/etc/test-outputs/exportDuplicate/rollup.d.ts

@@ -1,5 +1,5 @@
 
-/** @public */
+/** @internal */
 declare class A {
 }
 export { A as B }

build-tests/api-extractor-scenarios/src/exportDuplicate/index.ts

@@ -5,7 +5,8 @@
 export class X { }
 export { X as Y }
 
-/** @public */
+/** @internal */
 class A { }
+// The underscore warning should get printed next to these export statements, not next to the class declaration
 export { A as B }
 export { A as C }

build-tests/api-extractor-test-04/etc/api-extractor-test-04.api.md

@@ -60,11 +60,15 @@ export namespace EntangledNamespace {
 // @alpha
 export type ExportedAlias = AlphaClass;
 
+// Warning: (ae-internal-missing-underscore) The name InternalClass should be prefixed with an underscore because the declaration is marked as "@internal"
+// 
 // @internal
 export class InternalClass {
     undecoratedMember(): void;
 }
 
+// Warning: (ae-internal-missing-underscore) The name IPublicClassInternalParameters should be prefixed with an underscore because the declaration is marked as "@internal"
+// 
 // @internal
 export interface IPublicClassInternalParameters {
 }

common/changes/@microsoft/api-extractor/octogonz-ae-internal-missing-underscore_2019-03-20-22-25.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@microsoft/api-extractor",
+      "comment": "Reintroduce \"ae-internal-missing-underscore\" warning for API items marked as `@internal` but whose name does not start with an underscore",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@microsoft/api-extractor",
+  "email": "[email protected]"
+}