refactor(compiler): recursive ast expression visitor not passing context (#31085)

Currently the `RecursiveAstVisitor` that is part of the template expression
parser does not _always_ properly pass through the context that can be
specified when visting a given expression. Only a handful of AST types
pass through the context while others are accidentally left out. This causes
unexpected and inconsistent behavior and basically makes the `context`
parameter not usable if the type of template expression is not known.

e.g. the template variable assignment migration currently depends on
the `RecursiveAstVisitor` but sometimes breaks if developers use
things like conditionals in their template variable assignments.

Fixes #31043

PR Close #31085
This commit is contained in:
Paul Gschwendtner 2019-06-17 11:41:16 +02:00 committed by Kara Erickson
parent 75ac724842
commit 70ad91ed8b
2 changed files with 44 additions and 20 deletions

View File

@ -268,24 +268,24 @@ export class NullAstVisitor implements AstVisitor {
export class RecursiveAstVisitor implements AstVisitor {
visitBinary(ast: Binary, context: any): any {
ast.left.visit(this);
ast.right.visit(this);
ast.left.visit(this, context);
ast.right.visit(this, context);
return null;
}
visitChain(ast: Chain, context: any): any { return this.visitAll(ast.expressions, context); }
visitConditional(ast: Conditional, context: any): any {
ast.condition.visit(this);
ast.trueExp.visit(this);
ast.falseExp.visit(this);
ast.condition.visit(this, context);
ast.trueExp.visit(this, context);
ast.falseExp.visit(this, context);
return null;
}
visitPipe(ast: BindingPipe, context: any): any {
ast.exp.visit(this);
ast.exp.visit(this, context);
this.visitAll(ast.args, context);
return null;
}
visitFunctionCall(ast: FunctionCall, context: any): any {
ast.target !.visit(this);
ast.target !.visit(this, context);
this.visitAll(ast.args, context);
return null;
}
@ -294,14 +294,14 @@ export class RecursiveAstVisitor implements AstVisitor {
return this.visitAll(ast.expressions, context);
}
visitKeyedRead(ast: KeyedRead, context: any): any {
ast.obj.visit(this);
ast.key.visit(this);
ast.obj.visit(this, context);
ast.key.visit(this, context);
return null;
}
visitKeyedWrite(ast: KeyedWrite, context: any): any {
ast.obj.visit(this);
ast.key.visit(this);
ast.value.visit(this);
ast.obj.visit(this, context);
ast.key.visit(this, context);
ast.value.visit(this, context);
return null;
}
visitLiteralArray(ast: LiteralArray, context: any): any {
@ -310,32 +310,32 @@ export class RecursiveAstVisitor implements AstVisitor {
visitLiteralMap(ast: LiteralMap, context: any): any { return this.visitAll(ast.values, context); }
visitLiteralPrimitive(ast: LiteralPrimitive, context: any): any { return null; }
visitMethodCall(ast: MethodCall, context: any): any {
ast.receiver.visit(this);
ast.receiver.visit(this, context);
return this.visitAll(ast.args, context);
}
visitPrefixNot(ast: PrefixNot, context: any): any {
ast.expression.visit(this);
ast.expression.visit(this, context);
return null;
}
visitNonNullAssert(ast: NonNullAssert, context: any): any {
ast.expression.visit(this);
ast.expression.visit(this, context);
return null;
}
visitPropertyRead(ast: PropertyRead, context: any): any {
ast.receiver.visit(this);
ast.receiver.visit(this, context);
return null;
}
visitPropertyWrite(ast: PropertyWrite, context: any): any {
ast.receiver.visit(this);
ast.value.visit(this);
ast.receiver.visit(this, context);
ast.value.visit(this, context);
return null;
}
visitSafePropertyRead(ast: SafePropertyRead, context: any): any {
ast.receiver.visit(this);
ast.receiver.visit(this, context);
return null;
}
visitSafeMethodCall(ast: SafeMethodCall, context: any): any {
ast.receiver.visit(this);
ast.receiver.visit(this, context);
return this.visitAll(ast.args, context);
}
visitAll(asts: AST[], context: any): any {

View File

@ -211,6 +211,30 @@ describe('template variable assignment migration', () => {
expect(warnOutput.length).toBe(0);
});
it('should warn for template variable assignments in expression conditional', async() => {
writeFile('/index.ts', `
import {Component} from '@angular/core';
@Component({
templateUrl: './sub_dir/tmpl.html',
})
export class MyComp {
otherVar = false;
}
`);
writeFile('/sub_dir/tmpl.html', `
<ng-template let-tmplVar>
<p (click)="enabled ? tmplVar = true : otherVar = true"></p>
</ng-template>
`);
await runMigration();
expect(warnOutput.length).toBe(1);
expect(warnOutput[0]).toMatch(/^⮑ {3}sub_dir\/tmpl.html@3:31: Found assignment/);
});
it('should not warn for property writes with template variable name but different scope',
async() => {
writeFile('/index.ts', `