fix(lint): enforce that module-private members have @internal.

alexeagle · alexeagle · commit 098201d0b87d · 2015-10-28T22:14:48.000Z
This is needed to prevent leaking internal APIs to users via our published .d.ts typings. Fixes angular#4645 Closes angular#4989
diff --git a/gulpfile.js b/gulpfile.js
@@ -294,6 +294,7 @@ gulp.task('lint', ['build.tools'], function() {
   // https://github.com/palantir/tslint#supported-rules
   var tslintConfig = {
     "rules": {
+      "requireInternalWithUnderscore": true,
       "requireParameterType": true,
       "requireReturnType": true,
       "semicolon": true,
diff --git a/modules/angular2/src/core/change_detection/proto_change_detector.ts b/modules/angular2/src/core/change_detection/proto_change_detector.ts
@@ -265,6 +265,7 @@ class _ConvertAstIntoProtoRecords implements AstVisitor {
     return this._addRecord(RecordType.Chain, "chain", null, args, null, 0);
   }
 
+  /** @internal */
   _visitAll(asts: any[]) {
     var res = ListWrapper.createFixedSize(asts.length);
     for (var i = 0; i < asts.length; ++i) {
@@ -273,6 +274,7 @@ class _ConvertAstIntoProtoRecords implements AstVisitor {
     return res;
   }
 
+  /** @internal */
   _addRecord(type, name, funcOrValue, args, fixedArgs, context) {
     var selfIndex = this._records.length + 1;
     if (context instanceof DirectiveIndex) {
diff --git a/modules/angular2/src/core/life_cycle/life_cycle.ts b/modules/angular2/src/core/life_cycle/life_cycle.ts
@@ -52,8 +52,8 @@ export abstract class LifeCycle {
 
 @Injectable()
 export class LifeCycle_ extends LifeCycle {
+  /** @internal */
   static _tickScope: WtfScopeFn = wtfCreateScope('LifeCycle#tick()');
-
   /** @internal */
   _changeDetectors: ChangeDetector[];
   /** @internal */
diff --git a/modules/angular2/src/core/linker/element_injector.ts b/modules/angular2/src/core/linker/element_injector.ts
@@ -119,11 +119,13 @@ export class DirectiveDependency extends Dependency {
         DirectiveDependency._attributeName(d.properties), DirectiveDependency._query(d.properties));
   }
 
+  /** @internal */
   static _attributeName(properties): string {
     var p = <AttributeMetadata>ListWrapper.find(properties, (p) => p instanceof AttributeMetadata);
     return isPresent(p) ? p.attributeName : null;
   }
 
+  /** @internal */
   static _query(properties): QueryMetadata {
     return <QueryMetadata>ListWrapper.find(properties, (p) => p instanceof QueryMetadata);
   }
diff --git a/modules/angular2/src/core/pipes/date_pipe.ts b/modules/angular2/src/core/pipes/date_pipe.ts
@@ -81,6 +81,7 @@ var defaultLocale: string = 'en-US';
 @Pipe({name: 'date'})
 @Injectable()
 export class DatePipe implements PipeTransform {
+  /** @internal */
   static _ALIASES: {[key: string]: String} = {
     'medium': 'yMMMdjms',
     'short': 'yMdjm',
diff --git a/modules/angular2/src/core/pipes/number_pipe.ts b/modules/angular2/src/core/pipes/number_pipe.ts
@@ -23,6 +23,7 @@ var _re = RegExpWrapper.create('^(\\d+)?\\.((\\d+)(\\-(\\d+))?)?$');
 @CONST()
 @Injectable()
 export class NumberPipe {
+  /** @internal */
   static _format(value: number, style: NumberFormatStyle, digits: string, currency: string = null,
                  currencyAsSymbol: boolean = false): string {
     if (isBlank(value)) return null;
diff --git a/modules/angular2/src/core/render/dom/events/key_events.ts b/modules/angular2/src/core/render/dom/events/key_events.ts
@@ -99,6 +99,7 @@ export class KeyEventsPlugin extends EventManagerPlugin {
     };
   }
 
+  /** @internal */
   static _normalizeKey(keyName: string): string {
     // TODO: switch to a StringMap if the mapping grows too much
     switch (keyName) {
diff --git a/modules/angular2/src/core/testability/browser_testability.ts b/modules/angular2/src/core/testability/browser_testability.ts
@@ -7,6 +7,7 @@ import {
 import {global} from 'angular2/src/core/facade/lang';
 
 class PublicTestability {
+  /** @internal */
   _testability: Testability;
 
   constructor(testability: Testability) { this._testability = testability; }
diff --git a/modules/angular2/src/web_workers/shared/client_message_broker.ts b/modules/angular2/src/web_workers/shared/client_message_broker.ts
@@ -142,6 +142,7 @@ class MessageData {
 
   /**
    * Returns the value from the StringMap if present. Otherwise returns null
+   * @internal
    */
   _getValueIfPresent(data: {[key: string]: any}, key: string) {
     if (StringMapWrapper.contains(data, key)) {
diff --git a/tools/tslint/requireInternalWithUnderscoreRule.ts b/tools/tslint/requireInternalWithUnderscoreRule.ts
@@ -0,0 +1,44 @@
+import {RuleFailure} from 'tslint/lib/lint';
+import {AbstractRule} from 'tslint/lib/rules';
+import {RuleWalker} from 'tslint/lib/language/walker';
+import * as ts from 'tslint/node_modules/typescript';
+
+export class Rule extends AbstractRule {
+  public apply(sourceFile: ts.SourceFile): RuleFailure[] {
+    const typedefWalker = new TypedefWalker(sourceFile, this.getOptions());
+    return this.applyWithWalker(typedefWalker);
+  }
+}
+
+class TypedefWalker extends RuleWalker {
+  protected visitPropertyDeclaration(node: ts.PropertyDeclaration): void {
+    this.assertInternalAnnotationPresent(node);
+    super.visitPropertyDeclaration(node);
+  }
+
+  public visitMethodDeclaration(node: ts.MethodDeclaration): void {
+    this.assertInternalAnnotationPresent(node);
+    super.visitMethodDeclaration(node);
+  }
+
+  private hasInternalAnnotation(range: ts.CommentRange): boolean {
+    let text = this.getSourceFile().text;
+    let comment = text.substring(range.pos, range.end);
+    return comment.indexOf("@internal") >= 0;
+  }
+
+  private assertInternalAnnotationPresent(node: ts.Declaration) {
+    if (node.name.getText().charAt(0) !== '_') return;
+    if (node.modifiers && node.modifiers.flags & ts.NodeFlags.Private) return;
+
+    const ranges = ts.getLeadingCommentRanges(this.getSourceFile().text, node.pos);
+    if (ranges) {
+      for (let i = 0; i < ranges.length; i++) {
+        if (this.hasInternalAnnotation(ranges[i])) return;
+      }
+    }
+    this.addFailure(this.createFailure(
+        node.getStart(), node.getWidth(),
+        `module-private member ${node.name.getText()} must be annotated @internal`));
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -265,6 +265,7 @@ class _ConvertAstIntoProtoRecords implements AstVisitor {`
`265`	`265`	`return this._addRecord(RecordType.Chain, "chain", null, args, null, 0);`
`266`	`266`	`}`
`267`	`267`
	`268`	`+ /** @internal */`
`268`	`269`	`_visitAll(asts: any[]) {`
`269`	`270`	`var res = ListWrapper.createFixedSize(asts.length);`
`270`	`271`	`for (var i = 0; i < asts.length; ++i) {`
`@@ -273,6 +274,7 @@ class _ConvertAstIntoProtoRecords implements AstVisitor {`
`273`	`274`	`return res;`
`274`	`275`	`}`
`275`	`276`
	`277`	`+ /** @internal */`
`276`	`278`	`_addRecord(type, name, funcOrValue, args, fixedArgs, context) {`
`277`	`279`	`var selfIndex = this._records.length + 1;`
`278`	`280`	`if (context instanceof DirectiveIndex) {`
Original file line number	Diff line number	Diff line change
`@@ -119,11 +119,13 @@ export class DirectiveDependency extends Dependency {`
`119`	`119`	`DirectiveDependency._attributeName(d.properties), DirectiveDependency._query(d.properties));`
`120`	`120`	`}`
`121`	`121`
	`122`	`+ /** @internal */`
`122`	`123`	`static _attributeName(properties): string {`
`123`	`124`	`var p = <AttributeMetadata>ListWrapper.find(properties, (p) => p instanceof AttributeMetadata);`
`124`	`125`	`return isPresent(p) ? p.attributeName : null;`
`125`	`126`	`}`
`126`	`127`
	`128`	`+ /** @internal */`
`127`	`129`	`static _query(properties): QueryMetadata {`
`128`	`130`	`return <QueryMetadata>ListWrapper.find(properties, (p) => p instanceof QueryMetadata);`
`129`	`131`	`}`
Original file line number	Diff line number	Diff line change
`@@ -99,6 +99,7 @@ export class KeyEventsPlugin extends EventManagerPlugin {`
`99`	`99`	`};`
`100`	`100`	`}`
`101`	`101`
	`102`	`+ /** @internal */`
`102`	`103`	`static _normalizeKey(keyName: string): string {`
`103`	`104`	`// TODO: switch to a StringMap if the mapping grows too much`
`104`	`105`	`switch (keyName) {`