fix(rule): some cases not being reported by no-output-rename rule (#614)

src/noInputRenameRule.ts

@@ -30,10 +30,10 @@ export const getFailureMessage = (className: string, propertyName: string): stri
 };
 
 export class InputMetadataWalker extends NgWalker {
-  private directiveSelectors: DirectiveMetadata['selector'][];
+  private directiveSelectors: ReadonlySet<DirectiveMetadata['selector']>;
 
   protected visitNgDirective(metadata: DirectiveMetadata): void {
-    this.directiveSelectors = (metadata.selector || '').replace(/[\[\]\s]/g, '').split(',');
+    this.directiveSelectors = new Set((metadata.selector || '').replace(/[\[\]\s]/g, '').split(','));
     super.visitNgDirective(metadata);
   }
 
@@ -43,7 +43,7 @@ export class InputMetadataWalker extends NgWalker {
   }
 
   private canPropertyBeAliased(propertyAlias: string, propertyName: string): boolean {
-    return !!(this.directiveSelectors && this.directiveSelectors.indexOf(propertyAlias) !== -1 && propertyAlias !== propertyName);
+    return !!(this.directiveSelectors && this.directiveSelectors.has(propertyAlias) && propertyAlias !== propertyName);
   }
 
   private validateInput(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {

src/noOutputRenameRule.ts

@@ -1,39 +1,55 @@
-import * as Lint from 'tslint';
-import * as ts from 'typescript';
-import { sprintf } from 'sprintf-js';
+import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
+import { Decorator, PropertyDeclaration, SourceFile } from 'typescript/lib/typescript';
+import { DirectiveMetadata } from './angular/metadata';
 import { NgWalker } from './angular/ngWalker';
 
-export class Rule extends Lint.Rules.AbstractRule {
-  public static metadata: Lint.IRuleMetadata = {
-    ruleName: 'no-output-rename',
-    type: 'maintainability',
+export class Rule extends Rules.AbstractRule {
+  static readonly metadata: IRuleMetadata = {
     description: 'Disallows renaming directive outputs by providing a string to the decorator.',
     descriptionDetails: 'See more at https://angular.io/styleguide#style-05-13.',
-    rationale: 'Two names for the same property (one private, one public) is inherently confusing.',
     options: null,
     optionsDescription: 'Not configurable.',
+    rationale: 'Two names for the same property (one private, one public) is inherently confusing.',
+    ruleName: 'no-output-rename',
+    type: 'maintainability',
     typescriptOnly: true
   };
 
-  static FAILURE_STRING: string = 'In the class "%s", the directive output ' +
-  'property "%s" should not be renamed.' +
-  'Please, consider the following use "@Output() %s = new EventEmitter();"';
+  static readonly FAILURE_STRING = '@Outputs should not be renamed';
 
-  public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+  apply(sourceFile: SourceFile): RuleFailure[] {
     return this.applyWithWalker(new OutputMetadataWalker(sourceFile, this.getOptions()));
   }
 }
 
+export const getFailureMessage = (): string => {
+  return Rule.FAILURE_STRING;
+};
+
 export class OutputMetadataWalker extends NgWalker {
-  protected visitNgOutput(property: ts.PropertyDeclaration, output: ts.Decorator, args: string[]) {
-    let className = (<any>property).parent.name.text;
-    let memberName = (<any>property.name).text;
-    if (args.length !== 0 && memberName !== args[0]) {
-      let failureConfig: string[] = [className, memberName, memberName];
-      failureConfig.unshift(Rule.FAILURE_STRING);
-      this.addFailure(this.createFailure(property.getStart(), property.getWidth(), sprintf.apply(this, failureConfig)));
-    }
+  private directiveSelectors: ReadonlySet<DirectiveMetadata['selector']>;
 
+  visitNgDirective(metadata: DirectiveMetadata): void {
+    this.directiveSelectors = new Set((metadata.selector || '').replace(/[\[\]\s]/g, '').split(','));
+    super.visitNgDirective(metadata);
+  }
+
+  protected visitNgOutput(property: PropertyDeclaration, output: Decorator, args: string[]) {
+    this.validateOutput(property, output, args);
     super.visitNgOutput(property, output, args);
   }
+
+  private canPropertyBeAliased(propertyAlias: string, propertyName: string): boolean {
+    return !!(this.directiveSelectors && this.directiveSelectors.has(propertyAlias) && propertyAlias !== propertyName);
+  }
+
+  private validateOutput(property: PropertyDeclaration, output: Decorator, args: string[]) {
+    const propertyName = property.name.getText();
+
+    if (args.length === 0 || this.canPropertyBeAliased(args[0], propertyName)) {
+      return;
+    }
+
+    this.addFailureAtNode(property, getFailureMessage());
+  }
 }

test/noOutputRenameRule.spec.ts

@@ -1,41 +1,79 @@
-import { assertSuccess, assertAnnotated } from './testHelper';
+import { getFailureMessage, Rule } from '../src/noOutputRenameRule';
+import { assertAnnotated, assertSuccess } from './testHelper';
 
-describe('no-output-rename', () => {
-  describe('invalid directive output property', () => {
-    it('should fail, when a directive output property is renamed', () => {
-      let source = `
-        class ButtonComponent {
-          @Output('changeEvent') change = new EventEmitter<any>();
-          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+const {
+  metadata: { ruleName }
+} = Rule;
+
+describe(ruleName, () => {
+  describe('failure', () => {
+    it('should fail when a directive output property is renamed', () => {
+      const source = `
+        @Component({
+          template: 'test'
+        })
+        class TestComponent {
+          @Output('onChange') change = new EventEmitter<void>();
+          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         }
       `;
       assertAnnotated({
-        ruleName: 'no-output-rename',
-        message:
-          'In the class "ButtonComponent", the directive output property "change" should not be renamed.' +
-          'Please, consider the following use "@Output() change = new EventEmitter();"',
+        message: getFailureMessage(),
+        ruleName,
         source
       });
     });
-  });
 
-  describe('valid directive output property', () => {
-    it('should succeed, when a directive output property is properly used', () => {
-      let source = `
-        class ButtonComponent {
-          @Output() change = new EventEmitter<any>();
+    it('should fail when a directive output property is renamed and its name is strictly equal to the property', () => {
+      const source = `
+        @Component({
+          template: 'test'
+        })
+        class TestComponent {
+          @Output('change') change = new EventEmitter<void>();
+          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         }
       `;
-      assertSuccess('no-output-rename', source);
+      assertAnnotated({ message: getFailureMessage(), ruleName, source });
     });
 
-    it('should succeed, when a directive output property rename is the same as the property name', () => {
-      let source = `
-        class ButtonComponent {
-          @Output('change') change = new EventEmitter<any>();
+    it("should fail when the directive's selector matches exactly both property name and the alias", () => {
+      const source = `
+        @Directive({
+          selector: '[test], foo'
+        })
+        class TestDirective {
+          @Output('test') test = new EventEmitter<void>();
+          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         }
       `;
-      assertSuccess('no-output-rename', source);
+      assertAnnotated({ message: getFailureMessage(), ruleName, source });
+    });
+  });
+
+  describe('success', () => {
+    it('should succeed when a directive output property is properly used', () => {
+      const source = `
+        @Component({
+          template: 'test'
+        })
+        class TestComponent {
+          @Output() change = new EventEmitter<void>();
+        }
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it("should succeed when the directive's selector is also an output property", () => {
+      const source = `
+          @Directive({
+            selector: '[foo], test'
+          })
+          class TestDirective {
+            @Output('foo') bar = new EventEmitter<void>();
+          }
+        `;
+      assertSuccess(ruleName, source);
     });
   });
 });