refactor: improve ngWalker by preventing an error when a class has no name (#788)

wKoza · mgechev · commit 17c0fe259f86 · 2019-03-11T10:42:18.000-07:00
* improve ngWalker by preventing an error when a class has no name.

* fixup! improve ngWalker by preventing an error when a class has no name.

* fixup! improve ngWalker by preventing an error when a class has no name.
diff --git a/src/angular/metadata.ts b/src/angular/metadata.ts
@@ -25,22 +25,35 @@ export interface TemplateMetadata extends PropertyMetadata {
 }
 
 export class DirectiveMetadata {
-  constructor(
-    public readonly controller: ts.ClassDeclaration,
-    public readonly decorator: ts.Decorator,
-    public readonly selector?: string
-  ) {}
+  constructor(readonly controller: ts.ClassDeclaration, readonly decorator: ts.Decorator, readonly selector?: string) {}
 }
 
 export class ComponentMetadata extends DirectiveMetadata {
   constructor(
-    public readonly controller: ts.ClassDeclaration,
-    public readonly decorator: ts.Decorator,
-    public readonly selector?: string,
-    public readonly animations?: (AnimationMetadata | undefined)[],
-    public readonly styles?: (StyleMetadata | undefined)[],
-    public readonly template?: TemplateMetadata
+    readonly controller: ts.ClassDeclaration,
+    readonly decorator: ts.Decorator,
+    readonly selector?: string,
+    readonly animations?: (AnimationMetadata | undefined)[],
+    readonly styles?: (StyleMetadata | undefined)[],
+    readonly template?: TemplateMetadata
   ) {
     super(controller, decorator, selector);
   }
 }
+
+export class PipeMetadata {
+  constructor(
+    readonly controller: ts.ClassDeclaration,
+    readonly decorator: ts.Decorator,
+    readonly name?: string,
+    readonly pure?: ts.BooleanLiteral
+  ) {}
+}
+
+export class ModuleMetadata {
+  constructor(readonly controller: ts.ClassDeclaration, readonly decorator: ts.Decorator) {}
+}
+
+export class InjectableMetadata {
+  constructor(readonly controller: ts.ClassDeclaration, readonly decorator: ts.Decorator) {}
+}
diff --git a/src/angular/metadataReader.ts b/src/angular/metadataReader.ts
@@ -3,10 +3,20 @@ import { callExpression, decoratorArgument, hasProperties, withIdentifier } from
 import { ifTrue, listToMaybe, Maybe, unwrapFirst } from '../util/function';
 import { logger } from '../util/logger';
 import { getAnimations, getInlineStyle, getTemplate } from '../util/ngQuery';
-import { getDecoratorPropertyInitializer, isStringLiteralLike, maybeNodeArray } from '../util/utils';
+import { getDecoratorPropertyInitializer, isBooleanLiteralLike, isStringLiteralLike, maybeNodeArray } from '../util/utils';
 import { Config } from './config';
 import { FileResolver } from './fileResolver/fileResolver';
-import { AnimationMetadata, CodeWithSourceMap, ComponentMetadata, DirectiveMetadata, StyleMetadata, TemplateMetadata } from './metadata';
+import {
+  AnimationMetadata,
+  CodeWithSourceMap,
+  ComponentMetadata,
+  DirectiveMetadata,
+  InjectableMetadata,
+  ModuleMetadata,
+  PipeMetadata,
+  StyleMetadata,
+  TemplateMetadata
+} from './metadata';
 import { AbstractResolver, MetadataUrls } from './urlResolvers/abstractResolver';
 import { PathResolver } from './urlResolvers/pathResolver';
 import { UrlResolver } from './urlResolvers/urlResolver';
@@ -26,9 +36,9 @@ export class MetadataReader {
     this.urlResolver = this.urlResolver || new UrlResolver(new PathResolver());
   }
 
-  read(d: ts.ClassDeclaration): DirectiveMetadata | undefined {
+  read(d: ts.ClassDeclaration): DirectiveMetadata | ComponentMetadata | PipeMetadata | ModuleMetadata | InjectableMetadata | undefined {
     const componentMetadata = unwrapFirst<ComponentMetadata | undefined>(
-      maybeNodeArray(d.decorators!).map(dec => {
+      maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec => {
         return Maybe.lift(dec)
           .bind(callExpression)
           .bind(withIdentifier('Component') as any)
@@ -37,15 +47,42 @@ export class MetadataReader {
     );
 
     const directiveMetadata = unwrapFirst<DirectiveMetadata | undefined>(
-      maybeNodeArray(d.decorators!).map(dec =>
+      maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec =>
         Maybe.lift(dec)
           .bind(callExpression)
           .bind(withIdentifier('Directive') as any)
           .fmap(() => this.readDirectiveMetadata(d, dec))
       )
     );
 
-    return directiveMetadata || componentMetadata || undefined;
+    const pipeMetadata = unwrapFirst<PipeMetadata | undefined>(
+      maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec =>
+        Maybe.lift(dec)
+          .bind(callExpression)
+          .bind(withIdentifier('Pipe') as any)
+          .fmap(() => this.readPipeMetadata(d, dec))
+      )
+    );
+
+    const moduleMetadata = unwrapFirst<ModuleMetadata | undefined>(
+      maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec =>
+        Maybe.lift(dec)
+          .bind(callExpression)
+          .bind(withIdentifier('NgModule') as any)
+          .fmap(() => this.readModuleMetadata(d, dec))
+      )
+    );
+
+    const injectableMetadata = unwrapFirst<InjectableMetadata | undefined>(
+      maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec =>
+        Maybe.lift(dec)
+          .bind(callExpression)
+          .bind(withIdentifier('Injectable') as any)
+          .fmap(() => this.readInjectableMetadata(d, dec))
+      )
+    );
+
+    return directiveMetadata || componentMetadata || pipeMetadata || moduleMetadata || injectableMetadata;
   }
 
   protected readDirectiveMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata {
@@ -55,6 +92,24 @@ export class MetadataReader {
     return new DirectiveMetadata(d, dec, selector);
   }
 
+  protected readPipeMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata {
+    const nameExpression = getDecoratorPropertyInitializer(dec, 'name');
+    const name = nameExpression && isStringLiteralLike(nameExpression) ? nameExpression.text : undefined;
+
+    const pureExpression = getDecoratorPropertyInitializer(dec, 'pure');
+    const pure = pureExpression && isBooleanLiteralLike(pureExpression) ? pureExpression : undefined;
+
+    return new PipeMetadata(d, dec, name, pure);
+  }
+
+  protected readModuleMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata {
+    return new ModuleMetadata(d, dec);
+  }
+
+  protected readInjectableMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata {
+    return new InjectableMetadata(d, dec);
+  }
+
   protected readComponentMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): ComponentMetadata {
     const expr = this.getDecoratorArgument(dec);
     const directiveMetadata = this.readDirectiveMetadata(d, dec);
diff --git a/src/angular/ngWalker.ts b/src/angular/ngWalker.ts
@@ -2,9 +2,9 @@ import * as compiler from '@angular/compiler';
 import * as Lint from 'tslint';
 import * as ts from 'typescript';
 import { logger } from '../util/logger';
-import { getDecoratorName, maybeNodeArray } from '../util/utils';
+import { getClassName, getDecoratorName, maybeNodeArray } from '../util/utils';
 import { Config } from './config';
-import { ComponentMetadata, DirectiveMetadata, StyleMetadata } from './metadata';
+import { ComponentMetadata, DirectiveMetadata, InjectableMetadata, ModuleMetadata, PipeMetadata, StyleMetadata } from './metadata';
 import { MetadataReader } from './metadataReader';
 import { ngWalkerFactoryUtils } from './ngWalkerFactoryUtils';
 import { BasicCssAstVisitor, CssAstVisitorCtrl } from './styles/basicCssAstVisitor';
@@ -61,11 +61,19 @@ export class NgWalker extends Lint.RuleWalker {
   }
 
   visitClassDeclaration(declaration: ts.ClassDeclaration) {
-    const metadata = this._metadataReader!.read(declaration);
-    if (metadata instanceof ComponentMetadata) {
-      this.visitNgComponent(metadata);
-    } else if (metadata instanceof DirectiveMetadata) {
-      this.visitNgDirective(metadata);
+    if (this.hasClassName(declaration)) {
+      const metadata = this._metadataReader!.read(declaration);
+      if (metadata instanceof ComponentMetadata) {
+        this.visitNgComponent(metadata);
+      } else if (metadata instanceof DirectiveMetadata) {
+        this.visitNgDirective(metadata);
+      } else if (metadata instanceof PipeMetadata) {
+        this.visitNgPipe(metadata);
+      } else if (metadata instanceof ModuleMetadata) {
+        this.visitNgModule(metadata);
+      } else if (metadata instanceof InjectableMetadata) {
+        this.visitNgInjectable(metadata);
+      }
     }
     maybeNodeArray(ts.createNodeArray(declaration.decorators)).forEach(this.visitClassDecorator.bind(this));
     super.visitClassDeclaration(declaration);
@@ -115,22 +123,6 @@ export class NgWalker extends Lint.RuleWalker {
     }
   }
 
-  protected visitClassDecorator(decorator: ts.Decorator) {
-    let name = getDecoratorName(decorator);
-
-    if (name === 'Injectable') {
-      this.visitNgInjectable(decorator.parent as ts.ClassDeclaration, decorator);
-    }
-
-    if (name === 'Pipe') {
-      this.visitNgPipe(decorator.parent as ts.ClassDeclaration, decorator);
-    }
-
-    if (name === 'NgModule') {
-      this.visitNgModule(decorator);
-    }
-  }
-
   protected visitNgComponent(metadata: ComponentMetadata) {
     const { styles = [] } = metadata;
 
@@ -165,13 +157,15 @@ export class NgWalker extends Lint.RuleWalker {
     }
   }
 
-  protected visitNgModule(decorator: ts.Decorator) {}
+  protected visitClassDecorator(decorator: ts.Decorator) {}
+
+  protected visitNgModule(metadata: ModuleMetadata) {}
 
   protected visitNgDirective(metadata: DirectiveMetadata) {}
 
-  protected visitNgPipe(controller: ts.ClassDeclaration, decorator: ts.Decorator) {}
+  protected visitNgPipe(metadata: PipeMetadata) {}
 
-  protected visitNgInjectable(classDeclaration: ts.ClassDeclaration, decorator: ts.Decorator) {}
+  protected visitNgInjectable(metadata: InjectableMetadata) {}
 
   protected visitNgInput(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {}
 
@@ -237,4 +231,8 @@ export class NgWalker extends Lint.RuleWalker {
 
     return sf;
   }
+
+  private hasClassName(node: ts.Decorator | ts.ClassDeclaration) {
+    return (ts.isDecorator(node) && getClassName(node.parent)) || (ts.isClassDeclaration(node) && getClassName(node));
+  }
 }
diff --git a/src/contextualLifecycleRule.ts b/src/contextualLifecycleRule.ts
@@ -14,6 +14,7 @@ import {
   MetadataTypeKeys,
   MetadataTypes
 } from './util/utils';
+import { InjectableMetadata, PipeMetadata } from './angular';
 
 interface FailureParameters {
   readonly className: string;
@@ -45,26 +46,24 @@ export class Rule extends AbstractRule {
 }
 
 class ContextualLifecycleWalker extends NgWalker {
-  protected visitNgInjectable(controller: ClassDeclaration, decorator: Decorator): void {
-    this.validateDecorator(controller, decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Injectable);
-    super.visitNgInjectable(controller, decorator);
+  protected visitNgInjectable(metadata: InjectableMetadata): void {
+    this.validateDecorator(metadata, METADATA_TYPE_LIFECYCLE_MAPPER.Injectable);
+    super.visitNgInjectable(metadata);
   }
 
-  protected visitNgPipe(controller: ClassDeclaration, decorator: Decorator): void {
-    this.validateDecorator(controller, decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Pipe);
-    super.visitNgPipe(controller, decorator);
+  protected visitNgPipe(metadata: PipeMetadata): void {
+    this.validateDecorator(metadata, METADATA_TYPE_LIFECYCLE_MAPPER.Pipe);
+    super.visitNgPipe(metadata);
   }
 
-  private validateDecorator(controller: ClassDeclaration, decorator: Decorator, allowedMethods: ReadonlySet<LifecycleMethodKeys>): void {
-    const className = getClassName(controller);
+  private validateDecorator(metadata: PipeMetadata, allowedMethods: ReadonlySet<LifecycleMethodKeys>): void {
+    const className = getClassName(metadata.controller)!;
 
-    if (!className) return;
-
-    const metadataType = getDecoratorName(decorator);
+    const metadataType = getDecoratorName(metadata.decorator);
 
     if (!metadataType || !isMetadataType(metadataType)) return;
 
-    for (const member of controller.members) {
+    for (const member of metadata.controller.members) {
       const { name: memberName } = member;
 
       if (!memberName) continue;
diff --git a/src/noOutputRenameRule.ts b/src/noOutputRenameRule.ts
@@ -29,7 +29,7 @@ export const getFailureMessage = (): string => {
 export class OutputMetadataWalker extends NgWalker {
   private directiveSelectors!: ReadonlySet<DirectiveMetadata['selector']>;
 
-  visitNgDirective(metadata: DirectiveMetadata): void {
+  protected visitNgDirective(metadata: DirectiveMetadata): void {
     this.directiveSelectors = new Set((metadata.selector || '').replace(/[\[\]\s]/g, '').split(','));
     super.visitNgDirective(metadata);
   }
diff --git a/src/noPipeImpureRule.ts b/src/noPipeImpureRule.ts
@@ -4,6 +4,7 @@ import { AbstractRule } from 'tslint/lib/rules';
 import { ClassDeclaration, Decorator, SourceFile, SyntaxKind } from 'typescript';
 import { NgWalker } from './angular/ngWalker';
 import { getClassName, getDecoratorPropertyInitializer } from './util/utils';
+import { PipeMetadata } from './angular';
 
 interface FailureParameters {
   readonly className: string;
@@ -31,27 +32,18 @@ export class Rule extends AbstractRule {
 }
 
 export class ClassMetadataWalker extends NgWalker {
-  protected visitNgPipe(controller: ClassDeclaration, decorator: Decorator): void {
-    this.validatePipe(controller, decorator);
-    super.visitNgPipe(controller, decorator);
+  protected visitNgPipe(metadata: PipeMetadata): void {
+    this.validatePipe(metadata);
+    super.visitNgPipe(metadata);
   }
 
-  private validatePipe(controller: ClassDeclaration, decorator: Decorator): void {
-    const pureExpression = getDecoratorPropertyInitializer(decorator, 'pure');
+  private validatePipe(metadata: PipeMetadata): void {
+    if (!metadata.pure || metadata.pure.kind !== SyntaxKind.FalseKeyword) return;
 
-    if (!pureExpression) return;
-
-    const { parent: parentExpression } = pureExpression;
-    const isNotFalseLiteral = pureExpression.kind !== SyntaxKind.FalseKeyword;
-
-    if (!parentExpression || isNotFalseLiteral) return;
-
-    const className = getClassName(controller);
-
-    if (!className) return;
+    const className = getClassName(metadata.controller)!;
 
     const failure = getFailureMessage({ className });
 
-    this.addFailureAtNode(parentExpression, failure);
+    this.addFailureAtNode(metadata.pure, failure);
   }
 }
diff --git a/src/pipePrefixRule.ts b/src/pipePrefixRule.ts
@@ -4,6 +4,7 @@ import * as ts from 'typescript';
 import { NgWalker } from './angular/ngWalker';
 import { SelectorValidator } from './util/selectorValidator';
 import { getDecoratorArgument } from './util/utils';
+import { PipeMetadata } from './angular';
 
 export class Rule extends Lint.Rules.AbstractRule {
   static readonly metadata: Lint.IRuleMetadata = {
@@ -69,10 +70,10 @@ export class ClassMetadataWalker extends NgWalker {
     super(sourceFile, rule.getOptions());
   }
 
-  protected visitNgPipe(controller: ts.ClassDeclaration, decorator: ts.Decorator) {
-    let className = controller.name!.text;
-    this.validateProperties(className, decorator);
-    super.visitNgPipe(controller, decorator);
+  protected visitNgPipe(metadata: PipeMetadata) {
+    let className = metadata.controller.name!.text;
+    this.validateProperties(className, metadata.decorator);
+    super.visitNgPipe(metadata);
   }
 
   private validateProperties(className: string, pipe: ts.Decorator) {
diff --git a/src/util/utils.ts b/src/util/utils.ts
@@ -19,6 +19,7 @@ import {
   ObjectLiteralExpression,
   SourceFile,
   StringLiteral,
+  BooleanLiteral,
   SyntaxKind
 } from 'typescript';
 import { getDeclaredMethods } from './classDeclarationUtils';
@@ -202,6 +203,9 @@ export const isSameLine = (sourceFile: SourceFile, pos1: number, pos2: number):
 export const isStringLiteralLike = (node: Node): node is StringLiteral | NoSubstitutionTemplateLiteral =>
   isStringLiteral(node) || isNoSubstitutionTemplateLiteral(node);
 
+export const isBooleanLiteralLike = (node: Node): node is BooleanLiteral =>
+  node.kind === SyntaxKind.FalseKeyword || node.kind === SyntaxKind.TrueKeyword;
+
 export const maybeNodeArray = <T extends Node>(nodes: NodeArray<T>): ReadonlyArray<T> => nodes || [];
 
 // Regex below matches any Unicode word and compatible with ES5. In ES2018 the same result
diff --git a/test/angular/metadataReader.spec.ts b/test/angular/metadataReader.spec.ts
diff --git a/test/angular/ngWalker.spec.ts b/test/angular/ngWalker.spec.ts
diff --git a/test/noPipeImpureRule.spec.ts b/test/noPipeImpureRule.spec.ts

Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ export const getFailureMessage = (): string => {`
`29`	`29`	`export class OutputMetadataWalker extends NgWalker {`
`30`	`30`	`private directiveSelectors!: ReadonlySet<DirectiveMetadata['selector']>;`
`31`	`31`
`32`		`- visitNgDirective(metadata: DirectiveMetadata): void {`
	`32`	`+ protected visitNgDirective(metadata: DirectiveMetadata): void {`
`33`	`33`	`this.directiveSelectors = new Set((metadata.selector \|\| '').replace(/[\[\]\s]/g, '').split(','));`
`34`	`34`	`super.visitNgDirective(metadata);`
`35`	`35`	`}`