From 098201d0b87d913f4a9ca1100cbc21935208b3c3 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 28 Oct 2015 15:04:55 -0700 Subject: [PATCH] fix(lint): enforce that module-private members have @internal. This is needed to prevent leaking internal APIs to users via our published .d.ts typings. Fixes #4645 Closes #4989 --- gulpfile.js | 1 + .../change_detection/proto_change_detector.ts | 2 + .../src/core/life_cycle/life_cycle.ts | 2 +- .../src/core/linker/element_injector.ts | 2 + modules/angular2/src/core/pipes/date_pipe.ts | 1 + .../angular2/src/core/pipes/number_pipe.ts | 1 + .../src/core/render/dom/events/key_events.ts | 1 + .../core/testability/browser_testability.ts | 1 + .../shared/client_message_broker.ts | 1 + .../requireInternalWithUnderscoreRule.ts | 44 +++++++++++++++++++ 10 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 tools/tslint/requireInternalWithUnderscoreRule.ts diff --git a/gulpfile.js b/gulpfile.js index afef32cf33..a262dc5ac3 100644 --- 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 index ad9a5c94e1..1dfd3e342b 100644 --- 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 index 92f1513839..f32dd5cae0 100644 --- 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 index 3d932722a7..67ebf9da2f 100644 --- 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 = ListWrapper.find(properties, (p) => p instanceof AttributeMetadata); return isPresent(p) ? p.attributeName : null; } + /** @internal */ static _query(properties): QueryMetadata { return 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 index 4b0fac375b..c2670cf1ba 100644 --- 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 index 39a621be9e..c5cc7b9a69 100644 --- 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 index 088ad51ef7..ebd3a4051d 100644 --- 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 index 035042ba80..805d848e41 100644 --- 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 index 9b8de7f0c1..07cc317780 100644 --- 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 new file mode 100644 index 0000000000..fab5df0e45 --- /dev/null +++ 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`)); + } +}