From 0c0c54d615f448c59d5cb9e98a375d098301a94b Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 17 Sep 2020 16:16:17 -0700 Subject: [PATCH] refactor(compiler): simplify visitor logic for attributes (#38899) The logic for computing identifiers, specifically for bound attributes can be simplified by using the value span of the binding rather than the source span. PR Close #38899 --- .../src/ngtsc/indexer/src/template.ts | 36 +++++----------- .../src/ngtsc/indexer/test/template_spec.ts | 41 +++++++++++++++++++ 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/indexer/src/template.ts b/packages/compiler-cli/src/ngtsc/indexer/src/template.ts index 5303112c87..e8ec2507ed 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/src/template.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/src/template.ts @@ -97,15 +97,12 @@ class ExpressionVisitor extends RecursiveAstVisitor { return; } - // Get the location of the identifier of real interest. - // The compiler's expression parser records the location of some expressions in a manner not - // useful to the indexer. For example, a `MethodCall` `foo(a, b)` will record the span of the - // entire method call, but the indexer is interested only in the method identifier. - const localExpression = this.expressionStr.substr(ast.span.start); - if (!localExpression.includes(ast.name)) { - throw new Error(`Impossible state: "${ast.name}" not found in "${localExpression}"`); + // The source span of the requested AST starts at a location that is offset from the expression. + const identifierStart = ast.sourceSpan.start - this.absoluteOffset; + if (!this.expressionStr.substring(identifierStart).startsWith(ast.name)) { + throw new Error(`Impossible state: "${ast.name}" not found in "${ + this.expressionStr}" at location ${identifierStart}`); } - const identifierStart = ast.span.start + localExpression.indexOf(ast.name); // Join the relative position of the expression within a node with the absolute position // of the node to get the absolute position of the expression in the source code. @@ -191,25 +188,14 @@ class TemplateVisitor extends TmplAstRecursiveVisitor { this.visitAll(template.references); } visitBoundAttribute(attribute: TmplAstBoundAttribute) { - // A BoundAttribute's value (the parent AST) may have subexpressions (children ASTs) that have - // recorded spans extending past the recorded span of the parent. The most common example of - // this is with `*ngFor`. - // To resolve this, use the information on the BoundAttribute Template AST, which is always - // correct, to determine locations of identifiers in the expression. - // - // TODO(ayazhafiz): Remove this when https://github.com/angular/angular/pull/31813 lands. - const attributeSrc = attribute.sourceSpan.toString(); - const attributeAbsolutePosition = attribute.sourceSpan.start.offset; - - // Skip the bytes of the attribute name so that there are no collisions between the attribute - // name and expression identifier names later. - const nameSkipOffet = attributeSrc.indexOf(attribute.name) + attribute.name.length; - const expressionSrc = attributeSrc.substring(nameSkipOffet); - const expressionAbsolutePosition = attributeAbsolutePosition + nameSkipOffet; + // If the bound attribute has no value, it cannot have any identifiers in the value expression. + if (attribute.valueSpan === undefined) { + return; + } const identifiers = ExpressionVisitor.getIdentifiers( - attribute.value, expressionSrc, expressionAbsolutePosition, this.boundTemplate, - this.targetToIdentifier.bind(this)); + attribute.value, attribute.valueSpan.toString(), attribute.valueSpan.start.offset, + this.boundTemplate, this.targetToIdentifier.bind(this)); identifiers.forEach(id => this.identifiers.add(id)); } visitBoundEvent(attribute: TmplAstBoundEvent) { diff --git a/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts b/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts index f013d6204e..b7c0ff1153 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts @@ -124,6 +124,20 @@ runInEachFileSystem(() => { }); }); + it('should handle bound attributes with no value', () => { + const template = '
'; + const refs = getTemplateIdentifiers(bind(template)); + + const refArr = Array.from(refs); + expect(refArr).toEqual([{ + name: 'div', + kind: IdentifierKind.Element, + span: new AbsoluteSourceSpan(1, 4), + attributes: new Set(), + usedDirectives: new Set(), + }]); + }); + it('should discover variables in bound attributes', () => { const template = '
'; const refs = getTemplateIdentifiers(bind(template)); @@ -189,6 +203,33 @@ runInEachFileSystem(() => { target: null, }); }); + + it('should discover properties in template expressions and resist collisions', () => { + const template = '
'; + const refs = getTemplateIdentifiers(bind(template)); + + const refArr = Array.from(refs); + expect(refArr).toEqual(jasmine.arrayContaining([ + { + name: 'foos', + kind: IdentifierKind.Property, + span: new AbsoluteSourceSpan(25, 29), + target: null, + }, + { + name: 'foos', + kind: IdentifierKind.Property, + span: new AbsoluteSourceSpan(32, 36), + target: null, + }, + { + name: 'foos', + kind: IdentifierKind.Property, + span: new AbsoluteSourceSpan(39, 43), + target: null, + }, + ])); + }); }); describe('generates identifiers for PropertyWrites', () => {