From ba3f4c26bbceeadec1ea2e8b4ccd7174dc7ee282 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 17 Sep 2020 13:01:32 -0700 Subject: [PATCH] refactor(compiler): make `keySpan` available for `BoundAttributes` (#38898) Though we currently have the knowledge of where the `key` for an attribute binding appears during parsing, we do not propagate this information to the output AST. This means that once we produce the template AST, we have no way of mapping a template position to the key span alone. The best we can currently do is map back to the `sourceSpan`. This presents problems downstream, specifically for the language service, where we cannot provide correct information about a position in a template because the AST is not granular enough. PR Close #38898 --- .../compiler/src/expression_parser/ast.ts | 7 +- packages/compiler/src/render3/r3_ast.ts | 12 ++- .../src/render3/r3_template_transform.ts | 43 ++++++--- .../compiler/src/render3/view/compiler.ts | 5 +- .../src/template_parser/binding_parser.ts | 54 +++++++---- .../test/render3/r3_ast_spans_spec.ts | 95 ++++++++++++++++--- 6 files changed, 162 insertions(+), 54 deletions(-) diff --git a/packages/compiler/src/expression_parser/ast.ts b/packages/compiler/src/expression_parser/ast.ts index 6568d6fafb..2773a5458d 100644 --- a/packages/compiler/src/expression_parser/ast.ts +++ b/packages/compiler/src/expression_parser/ast.ts @@ -841,7 +841,10 @@ export class ParsedProperty { constructor( public name: string, public expression: ASTWithSource, public type: ParsedPropertyType, - public sourceSpan: ParseSourceSpan, public valueSpan?: ParseSourceSpan) { + // TODO(atscott): `keySpan` should really be required but allows `undefined` so VE does + // not need to be updated. Make `keySpan` required when VE is removed. + public sourceSpan: ParseSourceSpan, readonly keySpan: ParseSourceSpan|undefined, + public valueSpan: ParseSourceSpan|undefined) { this.isLiteral = this.type === ParsedPropertyType.LITERAL_ATTR; this.isAnimation = this.type === ParsedPropertyType.ANIMATION; } @@ -896,5 +899,5 @@ export class BoundElementProperty { constructor( public name: string, public type: BindingType, public securityContext: SecurityContext, public value: ASTWithSource, public unit: string|null, public sourceSpan: ParseSourceSpan, - public valueSpan?: ParseSourceSpan) {} + readonly keySpan: ParseSourceSpan|undefined, public valueSpan: ParseSourceSpan|undefined) {} } diff --git a/packages/compiler/src/render3/r3_ast.ts b/packages/compiler/src/render3/r3_ast.ts index 996dacb5bd..33a86c629b 100644 --- a/packages/compiler/src/render3/r3_ast.ts +++ b/packages/compiler/src/render3/r3_ast.ts @@ -43,12 +43,18 @@ export class BoundAttribute implements Node { constructor( public name: string, public type: BindingType, public securityContext: SecurityContext, public value: AST, public unit: string|null, public sourceSpan: ParseSourceSpan, - public valueSpan?: ParseSourceSpan, public i18n?: I18nMeta) {} + readonly keySpan: ParseSourceSpan, public valueSpan: ParseSourceSpan|undefined, + public i18n: I18nMeta|undefined) {} - static fromBoundElementProperty(prop: BoundElementProperty, i18n?: I18nMeta) { + static fromBoundElementProperty(prop: BoundElementProperty, i18n?: I18nMeta): BoundAttribute { + if (prop.keySpan === undefined) { + throw new Error( + `Unexpected state: keySpan must be defined for bound attributes but was not for ${ + prop.name}: ${prop.sourceSpan}`); + } return new BoundAttribute( prop.name, prop.type, prop.securityContext, prop.value, prop.unit, prop.sourceSpan, - prop.valueSpan, i18n); + prop.keySpan, prop.valueSpan, i18n); } visit(visitor: Visitor): Result { diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts index 16d899a173..61feb3bf4a 100644 --- a/packages/compiler/src/render3/r3_template_transform.ts +++ b/packages/compiler/src/render3/r3_template_transform.ts @@ -332,15 +332,26 @@ class HtmlAstToIvyAst implements html.Visitor { const absoluteOffset = attribute.valueSpan ? attribute.valueSpan.start.offset : srcSpan.start.offset; + function createKeySpan(srcSpan: ParseSourceSpan, prefix: string, identifier: string) { + // We need to adjust the start location for the keySpan to account for the removed 'data-' + // prefix from `normalizeAttributeName`. + const normalizationAdjustment = attribute.name.length - name.length; + const keySpanStart = srcSpan.start.moveBy(prefix.length + normalizationAdjustment); + const keySpanEnd = keySpanStart.moveBy(identifier.length); + return new ParseSourceSpan(keySpanStart, keySpanEnd, identifier); + } + const bindParts = name.match(BIND_NAME_REGEXP); let hasBinding = false; if (bindParts) { hasBinding = true; if (bindParts[KW_BIND_IDX] != null) { + const identifier = bindParts[IDENT_KW_IDX]; + const keySpan = createKeySpan(srcSpan, bindParts[KW_BIND_IDX], identifier); this.bindingParser.parsePropertyBinding( - bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, attribute.valueSpan, - matchableAttributes, parsedProperties); + identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan, + matchableAttributes, parsedProperties, keySpan); } else if (bindParts[KW_LET_IDX]) { if (isTemplateElement) { @@ -353,37 +364,41 @@ class HtmlAstToIvyAst implements html.Visitor { } else if (bindParts[KW_REF_IDX]) { const identifier = bindParts[IDENT_KW_IDX]; this.parseReference(identifier, value, srcSpan, attribute.valueSpan, references); - } else if (bindParts[KW_ON_IDX]) { const events: ParsedEvent[] = []; + const identifier = bindParts[IDENT_KW_IDX]; this.bindingParser.parseEvent( - bindParts[IDENT_KW_IDX], value, srcSpan, attribute.valueSpan || srcSpan, - matchableAttributes, events); + identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes, + events); addEvents(events, boundEvents); } else if (bindParts[KW_BINDON_IDX]) { + const identifier = bindParts[IDENT_KW_IDX]; + const keySpan = createKeySpan(srcSpan, bindParts[KW_BINDON_IDX], identifier); this.bindingParser.parsePropertyBinding( - bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, attribute.valueSpan, - matchableAttributes, parsedProperties); + identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan, + matchableAttributes, parsedProperties, keySpan); this.parseAssignmentEvent( - bindParts[IDENT_KW_IDX], value, srcSpan, attribute.valueSpan, matchableAttributes, - boundEvents); + identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents); } else if (bindParts[KW_AT_IDX]) { + const keySpan = createKeySpan(srcSpan, '', name); this.bindingParser.parseLiteralAttr( name, value, srcSpan, absoluteOffset, attribute.valueSpan, matchableAttributes, - parsedProperties); + parsedProperties, keySpan); } else if (bindParts[IDENT_BANANA_BOX_IDX]) { + const keySpan = createKeySpan(srcSpan, '[(', bindParts[IDENT_BANANA_BOX_IDX]); this.bindingParser.parsePropertyBinding( bindParts[IDENT_BANANA_BOX_IDX], value, false, srcSpan, absoluteOffset, - attribute.valueSpan, matchableAttributes, parsedProperties); + attribute.valueSpan, matchableAttributes, parsedProperties, keySpan); this.parseAssignmentEvent( bindParts[IDENT_BANANA_BOX_IDX], value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents); } else if (bindParts[IDENT_PROPERTY_IDX]) { + const keySpan = createKeySpan(srcSpan, '[', bindParts[IDENT_PROPERTY_IDX]); this.bindingParser.parsePropertyBinding( bindParts[IDENT_PROPERTY_IDX], value, false, srcSpan, absoluteOffset, - attribute.valueSpan, matchableAttributes, parsedProperties); + attribute.valueSpan, matchableAttributes, parsedProperties, keySpan); } else if (bindParts[IDENT_EVENT_IDX]) { const events: ParsedEvent[] = []; @@ -393,8 +408,10 @@ class HtmlAstToIvyAst implements html.Visitor { addEvents(events, boundEvents); } } else { + const keySpan = createKeySpan(srcSpan, '' /* prefix */, name); hasBinding = this.bindingParser.parsePropertyInterpolation( - name, value, srcSpan, attribute.valueSpan, matchableAttributes, parsedProperties); + name, value, srcSpan, attribute.valueSpan, matchableAttributes, parsedProperties, + keySpan); } return hasBinding; diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index 9844830d0a..790a14df23 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -582,9 +582,8 @@ function createHostBindingsFunction( // bindings with pipes. These calculates happen after this block. let totalHostVarsCount = 0; bindings && bindings.forEach((binding: ParsedProperty) => { - const name = binding.name; - const stylingInputWasSet = - styleBuilder.registerInputBasedOnName(name, binding.expression, binding.sourceSpan); + const stylingInputWasSet = styleBuilder.registerInputBasedOnName( + binding.name, binding.expression, hostBindingSourceSpan); if (stylingInputWasSet) { totalHostVarsCount += MIN_STYLING_BINDING_SLOTS_REQUIRED; } else { diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts index ca0001f44e..1cbe4530ab 100644 --- a/packages/compiler/src/template_parser/binding_parser.ts +++ b/packages/compiler/src/template_parser/binding_parser.ts @@ -62,7 +62,13 @@ export class BindingParser { if (typeof expression === 'string') { this.parsePropertyBinding( propName, expression, true, sourceSpan, sourceSpan.start.offset, undefined, [], - boundProps); + // Use the `sourceSpan` for `keySpan`. This isn't really accurate, but neither is the + // sourceSpan, as it represents the sourceSpan of the host itself rather than the + // source of the host binding (which doesn't exist in the template). Regardless, + // neither of these values are used in Ivy but are only here to satisfy the function + // signature. This should likely be refactored in the future so that `sourceSpan` + // isn't being used inaccurately. + boundProps, sourceSpan); } else { this._reportError( `Value of the host property binding "${ @@ -155,14 +161,14 @@ export class BindingParser { } else if (binding.value) { const valueSpan = moveParseSourceSpan(sourceSpan, binding.value.ast.sourceSpan); this._parsePropertyAst( - key, binding.value, sourceSpan, valueSpan, targetMatchableAttrs, targetProps); + key, binding.value, sourceSpan, keySpan, valueSpan, targetMatchableAttrs, targetProps); } else { targetMatchableAttrs.push([key, '' /* value */]); // Since this is a literal attribute with no RHS, source span should be // just the key span. this.parseLiteralAttr( key, null /* value */, keySpan, absoluteValueOffset, undefined /* valueSpan */, - targetMatchableAttrs, targetProps); + targetMatchableAttrs, targetProps, keySpan); } } } @@ -206,7 +212,9 @@ export class BindingParser { parseLiteralAttr( name: string, value: string|null, sourceSpan: ParseSourceSpan, absoluteOffset: number, valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][], - targetProps: ParsedProperty[]) { + // TODO(atscott): keySpan is only optional here so VE template parser implementation does not + // have to change This should be required when VE is removed. + targetProps: ParsedProperty[], keySpan?: ParseSourceSpan) { if (isAnimationLabel(name)) { name = name.substring(1); if (value) { @@ -216,18 +224,21 @@ export class BindingParser { sourceSpan, ParseErrorLevel.ERROR); } this._parseAnimation( - name, value, sourceSpan, absoluteOffset, valueSpan, targetMatchableAttrs, targetProps); + name, value, sourceSpan, absoluteOffset, keySpan, valueSpan, targetMatchableAttrs, + targetProps); } else { targetProps.push(new ParsedProperty( name, this._exprParser.wrapLiteralPrimitive(value, '', absoluteOffset), - ParsedPropertyType.LITERAL_ATTR, sourceSpan, valueSpan)); + ParsedPropertyType.LITERAL_ATTR, sourceSpan, keySpan, valueSpan)); } } parsePropertyBinding( name: string, expression: string, isHost: boolean, sourceSpan: ParseSourceSpan, absoluteOffset: number, valueSpan: ParseSourceSpan|undefined, - targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) { + // TODO(atscott): keySpan is only optional here so VE template parser implementation does not + // have to change This should be required when VE is removed. + targetMatchableAttrs: string[][], targetProps: ParsedProperty[], keySpan?: ParseSourceSpan) { if (name.length === 0) { this._reportError(`Property name is missing in binding`, sourceSpan); } @@ -243,22 +254,25 @@ export class BindingParser { if (isAnimationProp) { this._parseAnimation( - name, expression, sourceSpan, absoluteOffset, valueSpan, targetMatchableAttrs, + name, expression, sourceSpan, absoluteOffset, keySpan, valueSpan, targetMatchableAttrs, targetProps); } else { this._parsePropertyAst( name, this._parseBinding(expression, isHost, valueSpan || sourceSpan, absoluteOffset), - sourceSpan, valueSpan, targetMatchableAttrs, targetProps); + sourceSpan, keySpan, valueSpan, targetMatchableAttrs, targetProps); } } parsePropertyInterpolation( name: string, value: string, sourceSpan: ParseSourceSpan, valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][], - targetProps: ParsedProperty[]): boolean { + // TODO(atscott): keySpan is only optional here so VE template parser implementation does not + // have to change This should be required when VE is removed. + targetProps: ParsedProperty[], keySpan?: ParseSourceSpan): boolean { const expr = this.parseInterpolation(value, valueSpan || sourceSpan); if (expr) { - this._parsePropertyAst(name, expr, sourceSpan, valueSpan, targetMatchableAttrs, targetProps); + this._parsePropertyAst( + name, expr, sourceSpan, keySpan, valueSpan, targetMatchableAttrs, targetProps); return true; } return false; @@ -266,17 +280,17 @@ export class BindingParser { private _parsePropertyAst( name: string, ast: ASTWithSource, sourceSpan: ParseSourceSpan, - valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][], - targetProps: ParsedProperty[]) { + keySpan: ParseSourceSpan|undefined, valueSpan: ParseSourceSpan|undefined, + targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) { targetMatchableAttrs.push([name, ast.source!]); targetProps.push( - new ParsedProperty(name, ast, ParsedPropertyType.DEFAULT, sourceSpan, valueSpan)); + new ParsedProperty(name, ast, ParsedPropertyType.DEFAULT, sourceSpan, keySpan, valueSpan)); } private _parseAnimation( name: string, expression: string|null, sourceSpan: ParseSourceSpan, absoluteOffset: number, - valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][], - targetProps: ParsedProperty[]) { + keySpan: ParseSourceSpan|undefined, valueSpan: ParseSourceSpan|undefined, + targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) { if (name.length === 0) { this._reportError('Animation trigger is missing', sourceSpan); } @@ -287,8 +301,8 @@ export class BindingParser { const ast = this._parseBinding( expression || 'undefined', false, valueSpan || sourceSpan, absoluteOffset); targetMatchableAttrs.push([name, ast.source!]); - targetProps.push( - new ParsedProperty(name, ast, ParsedPropertyType.ANIMATION, sourceSpan, valueSpan)); + targetProps.push(new ParsedProperty( + name, ast, ParsedPropertyType.ANIMATION, sourceSpan, keySpan, valueSpan)); } private _parseBinding( @@ -317,7 +331,7 @@ export class BindingParser { if (boundProp.isAnimation) { return new BoundElementProperty( boundProp.name, BindingType.Animation, SecurityContext.NONE, boundProp.expression, null, - boundProp.sourceSpan, boundProp.valueSpan); + boundProp.sourceSpan, boundProp.keySpan, boundProp.valueSpan); } let unit: string|null = null; @@ -370,7 +384,7 @@ export class BindingParser { return new BoundElementProperty( boundPropertyName, bindingType, securityContexts[0], boundProp.expression, unit, - boundProp.sourceSpan, boundProp.valueSpan); + boundProp.sourceSpan, boundProp.keySpan, boundProp.valueSpan); } parseEvent( diff --git a/packages/compiler/test/render3/r3_ast_spans_spec.ts b/packages/compiler/test/render3/r3_ast_spans_spec.ts index fac18efac9..90268e5134 100644 --- a/packages/compiler/test/render3/r3_ast_spans_spec.ts +++ b/packages/compiler/test/render3/r3_ast_spans_spec.ts @@ -65,8 +65,10 @@ class R3AstSourceSpans implements t.Visitor { } visitBoundAttribute(attribute: t.BoundAttribute) { - this.result.push( - ['BoundAttribute', humanizeSpan(attribute.sourceSpan), humanizeSpan(attribute.valueSpan)]); + this.result.push([ + 'BoundAttribute', humanizeSpan(attribute.sourceSpan), humanizeSpan(attribute.keySpan), + humanizeSpan(attribute.valueSpan) + ]); } visitBoundEvent(event: t.BoundEvent) { @@ -144,28 +146,35 @@ describe('R3 AST source spans', () => { it('is correct for bound properties', () => { expectFromHtml('
').toEqual([ ['Element', '
', '
', '
'], - ['BoundAttribute', '[someProp]="v"', 'v'], + ['BoundAttribute', '[someProp]="v"', 'someProp', 'v'], ]); }); it('is correct for bound properties without value', () => { expectFromHtml('
').toEqual([ ['Element', '
', '
', '
'], - ['BoundAttribute', '[someProp]', ''], + ['BoundAttribute', '[someProp]', 'someProp', ''], ]); }); it('is correct for bound properties via bind- ', () => { expectFromHtml('
').toEqual([ ['Element', '
', '
', '
'], - ['BoundAttribute', 'bind-prop="v"', 'v'], + ['BoundAttribute', 'bind-prop="v"', 'prop', 'v'], ]); }); it('is correct for bound properties via {{...}}', () => { expectFromHtml('
').toEqual([ ['Element', '
', '
', '
'], - ['BoundAttribute', 'prop="{{v}}"', '{{v}}'], + ['BoundAttribute', 'prop="{{v}}"', 'prop', '{{v}}'], + ]); + }); + + it('is correct for bound properties via data-', () => { + expectFromHtml('
').toEqual([ + ['Element', '
', '
', '
'], + ['BoundAttribute', 'data-prop="{{v}}"', 'prop', '{{v}}'], ]); }); }); @@ -208,6 +217,16 @@ describe('R3 AST source spans', () => { ]); }); + it('is correct for reference via data-ref-...', () => { + expectFromHtml('').toEqual([ + [ + 'Template', '', '', + '' + ], + ['Reference', 'data-ref-a', ''], + ]); + }); + it('is correct for variables via let-...', () => { expectFromHtml('').toEqual([ [ @@ -218,6 +237,16 @@ describe('R3 AST source spans', () => { ]); }); + it('is correct for variables via data-let-...', () => { + expectFromHtml('').toEqual([ + [ + 'Template', '', '', + '' + ], + ['Variable', 'data-let-a="b"', 'b'], + ]); + }); + it('is correct for attributes', () => { expectFromHtml('').toEqual([ [ @@ -234,7 +263,7 @@ describe('R3 AST source spans', () => { 'Template', '', '', '' ], - ['BoundAttribute', '[k1]="v1"', 'v1'], + ['BoundAttribute', '[k1]="v1"', 'k1', 'v1'], ]); }); }); @@ -252,7 +281,7 @@ describe('R3 AST source spans', () => { '' ], ['TextAttribute', 'ngFor', ''], - ['BoundAttribute', '*ngFor="let item of items"', 'items'], + ['BoundAttribute', '*ngFor="let item of items"', 'of', 'items'], ['Variable', 'let item ', ''], [ 'Element', '
', '
', @@ -270,10 +299,28 @@ describe('R3 AST source spans', () => { [ 'Template', '
', '
', '
' ], - ['BoundAttribute', '*ngFor="item of items"', 'item'], - ['BoundAttribute', '*ngFor="item of items"', 'items'], + ['BoundAttribute', '*ngFor="item of items"', 'ngFor', 'item'], + ['BoundAttribute', '*ngFor="item of items"', 'of', 'items'], ['Element', '
', '
', '
'], ]); + + expectFromHtml('
').toEqual([ + [ + 'Template', '
', + '
', '
' + ], + ['TextAttribute', 'ngFor', ''], + ['BoundAttribute', '*ngFor="let item of items; trackBy: trackByFn"', 'of', 'items'], + [ + 'BoundAttribute', '*ngFor="let item of items; trackBy: trackByFn"', 'trackBy', 'trackByFn' + ], + ['Variable', 'let item ', ''], + [ + 'Element', '
', + '
', '
' + ], + + ]); }); it('is correct for variables via let ...', () => { @@ -288,7 +335,7 @@ describe('R3 AST source spans', () => { it('is correct for variables via as ...', () => { expectFromHtml('
').toEqual([ ['Template', '
', '
', '
'], - ['BoundAttribute', '*ngIf="expr as local"', 'expr'], + ['BoundAttribute', '*ngIf="expr as local"', 'ngIf', 'expr'], ['Variable', 'ngIf="expr as local', 'ngIf'], ['Element', '
', '
', '
'], ]); @@ -310,10 +357,17 @@ describe('R3 AST source spans', () => { ]); }); + it('is correct for bound events via data-on-', () => { + expectFromHtml('
').toEqual([ + ['Element', '
', '
', '
'], + ['BoundEvent', 'data-on-event="v"', 'v'], + ]); + }); + it('is correct for bound events and properties via [(...)]', () => { expectFromHtml('
').toEqual([ ['Element', '
', '
', '
'], - ['BoundAttribute', '[(prop)]="v"', 'v'], + ['BoundAttribute', '[(prop)]="v"', 'prop', 'v'], ['BoundEvent', '[(prop)]="v"', 'v'], ]); }); @@ -321,10 +375,18 @@ describe('R3 AST source spans', () => { it('is correct for bound events and properties via bindon-', () => { expectFromHtml('
').toEqual([ ['Element', '
', '
', '
'], - ['BoundAttribute', 'bindon-prop="v"', 'v'], + ['BoundAttribute', 'bindon-prop="v"', 'prop', 'v'], ['BoundEvent', 'bindon-prop="v"', 'v'], ]); }); + + it('is correct for bound events and properties via data-bindon-', () => { + expectFromHtml('
').toEqual([ + ['Element', '
', '
', '
'], + ['BoundAttribute', 'data-bindon-prop="v"', 'prop', 'v'], + ['BoundEvent', 'data-bindon-prop="v"', 'v'], + ]); + }); }); describe('references', () => { @@ -348,5 +410,12 @@ describe('R3 AST source spans', () => { ['Reference', 'ref-a', ''], ]); }); + + it('is correct for references via data-ref-', () => { + expectFromHtml('
').toEqual([ + ['Element', '
', '
', '
'], + ['Reference', 'ref-a', ''], + ]); + }); }); });