From 961e7a36d02a2b7f871aab6ace4282430756543f Mon Sep 17 00:00:00 2001 From: Rafael Date: Sun, 29 Jul 2018 16:44:07 -0300 Subject: [PATCH] fix(templates-no-negated-async): not reporting failures for some cases --- src/templatesNoNegatedAsyncRule.ts | 99 ++++++------ test/templatesNoNegatedAsyncRule.spec.ts | 197 ++++++++++++----------- 2 files changed, 152 insertions(+), 144 deletions(-) diff --git a/src/templatesNoNegatedAsyncRule.ts b/src/templatesNoNegatedAsyncRule.ts index bb8789b35..a6ba79cc5 100644 --- a/src/templatesNoNegatedAsyncRule.ts +++ b/src/templatesNoNegatedAsyncRule.ts @@ -1,78 +1,73 @@ -import * as Lint from 'tslint'; -import * as ts from 'typescript'; +import { IRuleMetadata, RuleFailure, Rules } from 'tslint'; +import { SourceFile } from 'typescript'; import { NgWalker } from './angular/ngWalker'; import { RecursiveAngularExpressionVisitor } from './angular/templates/recursiveAngularExpressionVisitor'; -import * as e from '@angular/compiler/src/expression_parser/ast'; -import * as ast from '@angular/compiler'; +import { Binary, PrefixNot } from '@angular/compiler/src/expression_parser/ast'; +import { AST, BindingPipe, LiteralPrimitive } from '@angular/compiler'; const unstrictEqualityOperator = '=='; -class TemplateToNgTemplateVisitor extends RecursiveAngularExpressionVisitor { - visitBinary(expr: e.Binary, context: any): any { - if (!this.isAsyncBinding(expr.left)) { - return super.visitBinary(expr, context); - } - if (!(expr.right instanceof ast.LiteralPrimitive) || expr.right.value !== false || expr.operation !== unstrictEqualityOperator) { - return super.visitBinary(expr, context); - } +const isAsyncBinding = (ast: AST): boolean => { + return ast instanceof BindingPipe && ast.name === 'async'; +}; - const { - left: { - span: { end: endLeftSpan } - }, - right: { - span: { start: startRightSpan } - }, - span: { end: spanEnd, start: spanStart } - } = expr; - const operator = this.codeWithMap.code.slice(endLeftSpan, startRightSpan); - const operatorStart = /^.*==/.exec(operator)![0].length - unstrictEqualityOperator.length; +class TemplateToNgTemplateVisitor extends RecursiveAngularExpressionVisitor { + visitBinary(ast: Binary, context: any): any { + this.validateBinary(ast); + super.visitBinary(ast, context); + } - this.addFailureFromStartToEnd(spanStart, spanEnd, 'Async pipes must use strict equality `===` when comparing with `false`', [ - new Lint.Replacement(this.getSourcePosition(endLeftSpan) + operatorStart, unstrictEqualityOperator.length, '===') - ]); - super.visitBinary(expr, context); + visitPrefixNot(ast: PrefixNot, context: any): any { + this.validatePrefixNot(ast); + super.visitPrefixNot(ast, context); } - visitPrefixNot(expr: e.PrefixNot, context: any): any { - if (!this.isAsyncBinding(expr.expression)) { - return super.visitPrefixNot(expr, context); + private validateBinary(ast: Binary): void { + const { left, operation, right } = ast; + + if (!isAsyncBinding(left) || !(right instanceof LiteralPrimitive) || right.value !== false || operation !== unstrictEqualityOperator) { + return; } - const { - span: { end: spanEnd, start: spanStart } - } = expr; - const absoluteStart = this.getSourcePosition(spanStart); - // Angular includes the whitespace after an expression, we want to trim that - const expressionSource = this.codeWithMap.code.slice(spanStart, spanEnd); - const concreteWidth = spanEnd - spanStart - / *$/.exec(expressionSource)![0].length; + this.generateFailure(ast, Rule.FAILURE_STRING_UNSTRICT_EQUALITY); + } + + private validatePrefixNot(ast: PrefixNot): void { + const { expression } = ast; + + if (!isAsyncBinding(expression)) { + return; + } - this.addFailureFromStartToEnd(spanStart, spanEnd, 'Async pipes can not be negated, use (observable | async) === false instead', [ - new Lint.Replacement(absoluteStart + concreteWidth, 1, ' === false '), - new Lint.Replacement(absoluteStart, 1, '') - ]); + this.generateFailure(ast, Rule.FAILURE_STRING_NEGATED_PIPE); } - protected isAsyncBinding(expr: ast.AST) { - return expr instanceof ast.BindingPipe && expr.name === 'async'; + private generateFailure(ast: Binary | PrefixNot, errorMessage: string): void { + const { + span: { end: spanEnd, start: spanStart } + } = ast; + + this.addFailureFromStartToEnd(spanStart, spanEnd, errorMessage); } } -export class Rule extends Lint.Rules.AbstractRule { - public static metadata: Lint.IRuleMetadata = { - ruleName: 'templates-no-negated-async', - type: 'functionality', +export class Rule extends Rules.AbstractRule { + static readonly metadata: IRuleMetadata = { description: 'Ensures that strict equality is used when evaluating negations on async pipe output.', + options: null, + optionsDescription: 'Not configurable.', rationale: 'Async pipe evaluate to `null` before the observable or promise emits, which can lead to layout thrashing as' + ' components load. Prefer strict `=== false` checks instead.', - options: null, - optionsDescription: 'Not configurable.', - typescriptOnly: true, - hasFix: true + ruleName: 'templates-no-negated-async', + type: 'functionality', + typescriptOnly: true }; - public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + static readonly FAILURE_STRING_NEGATED_PIPE = 'Async pipes can not be negated, use (observable | async) === false instead'; + static readonly FAILURE_STRING_UNSTRICT_EQUALITY = 'Async pipes must use strict equality `===` when comparing with `false`'; + + apply(sourceFile: SourceFile): RuleFailure[] { return this.applyWithWalker( new NgWalker(sourceFile, this.getOptions(), { expressionVisitorCtrl: TemplateToNgTemplateVisitor diff --git a/test/templatesNoNegatedAsyncRule.spec.ts b/test/templatesNoNegatedAsyncRule.spec.ts index c23e1349f..8d7770700 100644 --- a/test/templatesNoNegatedAsyncRule.spec.ts +++ b/test/templatesNoNegatedAsyncRule.spec.ts @@ -1,169 +1,182 @@ -import { assertSuccess, assertAnnotated } from './testHelper'; -import { Replacement } from 'tslint'; -import { expect } from 'chai'; +import { assertAnnotated, assertMultipleAnnotated, assertSuccess } from './testHelper'; +import { Rule } from '../src/templatesNoNegatedAsyncRule'; -describe('templates-no-negated-async', () => { - describe('invalid expressions', () => { - it('should fail when an async pipe is negated', () => { - let source = ` +const { + FAILURE_STRING_NEGATED_PIPE, + FAILURE_STRING_UNSTRICT_EQUALITY, + metadata: { ruleName } +} = Rule; + +describe(ruleName, () => { + describe('failure', () => { + it('should fail if async pipe is negated', () => { + const source = ` @Component({ - selector: 'foobar', + selector: 'test', template: '{{ !(foo | async) }}' ~~~~~~~~~~~~~~~ }) class Test { - constructor(public foo: Observable) {} + constructor(foo: Observable) {} } `; assertAnnotated({ - ruleName: 'templates-no-negated-async', - message: 'Async pipes can not be negated, use (observable | async) === false instead', + message: FAILURE_STRING_NEGATED_PIPE, + ruleName: ruleName, source }); }); - it('should fail when an async pipe is including other pipes', () => { - let source = ` + it('should fail if async pipe is the last pipe in the negated chain', () => { + const source = ` @Component({ - selector: 'foobar', + selector: 'test', template: '{{ !(foo | somethingElse | async) }}' ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ }) class Test { - constructor(public foo: Observable) {} + constructor(foo: Observable) {} } `; assertAnnotated({ - ruleName: 'templates-no-negated-async', - message: 'Async pipes can not be negated, use (observable | async) === false instead', + message: FAILURE_STRING_NEGATED_PIPE, + ruleName: ruleName, source }); }); - it('should fail when an async pipe uses non-strict equality', () => { - let source = ` + it('should fail if async pipe uses unstrict equality', () => { + const source = ` @Component({ - selector: 'foobar', + selector: 'test', template: '{{ (foo | async) == false }}' ~~~~~~~~~~~~~~~~~~~~~~ }) class Test { - constructor(public foo: Observable) {} + constructor(foo: Observable) {} } `; assertAnnotated({ - ruleName: 'templates-no-negated-async', - message: 'Async pipes must use strict equality `===` when comparing with `false`', + message: FAILURE_STRING_UNSTRICT_EQUALITY, + ruleName: ruleName, source }); }); - describe('fixes', () => { - it('fixes negated pipes', () => { - let source = ` - @Component({ - selector: 'foobar', - template: '{{ !(foo | async) }}' - ~~~~~~~~~~~~~~~ - }) - class Test {} - `; - const failures = assertAnnotated({ - ruleName: 'templates-no-negated-async', - message: 'Async pipes can not be negated, use (observable | async) === false instead', - source - }); - - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` - @Component({ - selector: 'foobar', - template: '{{ (foo | async) === false }}' - ~~~~~~~~~~~~~~~ - }) - class Test {} - `); + it('should fail if async pipe is negated using *ngIf', () => { + const source = ` + @Component({ + selector: 'test', + template: '
' + ~~~~~~~~~~~~ + }) + class Test { + constructor(foo: Observable) {} + } + `; + assertAnnotated({ + message: FAILURE_STRING_NEGATED_PIPE, + ruleName: ruleName, + source }); + }); - it('fixes un-strict equality', () => { - let source = ` - @Component({ - selector: 'foobar', - template: '{{ (foo | async) == false }}' - ~~~~~~~~~~~~~~~~~~~~~~ - }) - class Test {} - `; - const failures = assertAnnotated({ - ruleName: 'templates-no-negated-async', - message: 'Async pipes must use strict equality `===` when comparing with `false`', - source - }); + it('should fail for multiple negated/unstrict equality async pipes', () => { + const source = ` + @Component({ + selector: 'test', + template: \` +
+ {{ elem }} +
- const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` - @Component({ - selector: 'foobar', - template: '{{ (foo | async) === false }}' - ~~~~~~~~~~~~~~~~~~~~~~ - }) - class Test {} - `); +
+ ~~~~~~~~~~~~~~ + {{ (foo | async) == false }} + ^^^^^^^^^^^^^^^^^^^^^^ +
+ ##################### + works! +
+
+ \` + }) + class Test { + constructor(foo: Observable) {} + } + `; + assertMultipleAnnotated({ + failures: [ + { + char: '~', + msg: FAILURE_STRING_NEGATED_PIPE + }, + { + char: '^', + msg: FAILURE_STRING_UNSTRICT_EQUALITY + }, + { + char: '#', + msg: FAILURE_STRING_UNSTRICT_EQUALITY + } + ], + ruleName, + source }); }); }); - describe('valid expressions', () => { - it('should succeed if an async pipe is not negated', () => { - let source = ` + describe('success', () => { + it('should succeed if async pipe is not negated', () => { + const source = ` @Component({ - selector: 'foobar', + selector: 'test', template: '{{ (foo | async) }}' }) class Test { - constructor(public foo: Observable) {} + constructor(foo: Observable) {} } `; - assertSuccess('templates-no-negated-async', source); + assertSuccess(ruleName, source); }); - it('should succeed if an async pipe is not the last pipe in the negated chain', () => { - let source = ` + it('should succeed if async pipe is not the last pipe in the negated chain', () => { + const source = ` @Component({ - selector: 'foobar', - template: '{{ !(foo | async | someOtherFilter) }}' + selector: 'test', + template: '{{ !(foo | async | somethingElse) }}' }) class Test { - constructor(public foo: Observable) {} + constructor(foo: Observable) {} } `; - assertSuccess('templates-no-negated-async', source); + assertSuccess(ruleName, source); }); - it('should succeed if an async pipe uses strict equality', () => { - let source = ` + it('should succeed if async pipe uses strict equality', () => { + const source = ` @Component({ - selector: 'foobar', + selector: 'test', template: '{{ (foo | async) === false }}' }) class Test { - constructor(public foo: Observable) {} + constructor(foo: Observable) {} } `; - assertSuccess('templates-no-negated-async', source); + assertSuccess(ruleName, source); }); it('should succeed if any other pipe is negated', () => { - let source = ` + const source = ` @Component({ - selector: 'foobar', + selector: 'test', template: '{{ !(foo | notAnAsyncPipe) }}' }) class Test { - constructor(public foo: Observable) {} + constructor(foo: Observable) {} } `; - assertSuccess('templates-no-negated-async', source); + assertSuccess(ruleName, source); }); }); });