Skip to content

Commit 489cef6

Browse files
JoostKmhevery
JoostK
authored and
mhevery
committed
feat(ivy): include value spans for attributes, variables and references (angular#30181)
Template AST nodes for (bound) attributes, variables and references will now retain a reference to the source span of their value, which allows for more accurate type check diagnostics. PR Close angular#30181
1 parent 9855133 commit 489cef6

File tree

8 files changed

+395
-61
lines changed

8 files changed

+395
-61
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,14 @@ export function generateTypeCheckBlock(
5252
// the `ts.Printer` to format the type-check block nicely.
5353
const body = ts.createBlock([ts.createIf(ts.createTrue(), innerBody, undefined)]);
5454
const fnDecl = ts.createFunctionDeclaration(
55-
/* decorators */ undefined,
56-
/* modifiers */ undefined,
57-
/* asteriskToken */ undefined,
58-
/* name */ name,
59-
/* typeParameters */ ref.node.typeParameters,
60-
/* parameters */ paramList,
61-
/* type */ undefined,
62-
/* body */ body);
55+
/* decorators */ undefined,
56+
/* modifiers */ undefined,
57+
/* asteriskToken */ undefined,
58+
/* name */ name,
59+
/* typeParameters */ ref.node.typeParameters,
60+
/* parameters */ paramList,
61+
/* type */ undefined,
62+
/* body */ body);
6363
addSourceInfo(fnDecl, ref.node);
6464
return fnDecl;
6565
}
@@ -198,7 +198,9 @@ class TcbTemplateBodyOp extends TcbOp {
198198
i instanceof TmplAstBoundAttribute && i.name === guard.inputName);
199199
if (boundInput !== undefined) {
200200
// If there is such a binding, generate an expression for it.
201-
const expr = tcbExpression(boundInput.value, this.tcb, this.scope, boundInput.sourceSpan);
201+
const expr = tcbExpression(
202+
boundInput.value, this.tcb, this.scope,
203+
boundInput.valueSpan || boundInput.sourceSpan);
202204

203205
if (guard.type === 'binding') {
204206
// Use the binding expression itself as guard.
@@ -335,7 +337,8 @@ class TcbUnclaimedInputsOp extends TcbOp {
335337
continue;
336338
}
337339

338-
let expr = tcbExpression(binding.value, this.tcb, this.scope, binding.sourceSpan);
340+
let expr = tcbExpression(
341+
binding.value, this.tcb, this.scope, binding.valueSpan || binding.sourceSpan);
339342

340343
// If checking the type of bindings is disabled, cast the resulting expression to 'any' before
341344
// the assignment.
@@ -768,7 +771,7 @@ function tcbGetInputBindingExpressions(
768771
function processAttribute(attr: TmplAstBoundAttribute | TmplAstTextAttribute): void {
769772
if (attr instanceof TmplAstBoundAttribute && propMatch.has(attr.name)) {
770773
// Produce an expression representing the value of the binding.
771-
const expr = tcbExpression(attr.value, tcb, scope, attr.sourceSpan);
774+
const expr = tcbExpression(attr.value, tcb, scope, attr.valueSpan || attr.sourceSpan);
772775
// Call the callback.
773776
bindings.push({
774777
property: attr.name,

‎packages/compiler/src/expression_parser/ast.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -691,7 +691,7 @@ export class ParsedProperty {
691691

692692
constructor(
693693
public name: string, public expression: ASTWithSource, public type: ParsedPropertyType,
694-
public sourceSpan: ParseSourceSpan) {
694+
public sourceSpan: ParseSourceSpan, public valueSpan?: ParseSourceSpan) {
695695
this.isLiteral = this.type === ParsedPropertyType.LITERAL_ATTR;
696696
this.isAnimation = this.type === ParsedPropertyType.ANIMATION;
697697
}
@@ -739,5 +739,6 @@ export const enum BindingType {
739739
export class BoundElementProperty {
740740
constructor(
741741
public name: string, public type: BindingType, public securityContext: SecurityContext,
742-
public value: AST, public unit: string|null, public sourceSpan: ParseSourceSpan) {}
742+
public value: AST, public unit: string|null, public sourceSpan: ParseSourceSpan,
743+
public valueSpan?: ParseSourceSpan) {}
743744
}

‎packages/compiler/src/render3/r3_ast.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,12 @@ export class BoundAttribute implements Node {
3737
constructor(
3838
public name: string, public type: BindingType, public securityContext: SecurityContext,
3939
public value: AST, public unit: string|null, public sourceSpan: ParseSourceSpan,
40-
public i18n?: I18nAST) {}
40+
public valueSpan?: ParseSourceSpan, public i18n?: I18nAST) {}
4141

4242
static fromBoundElementProperty(prop: BoundElementProperty, i18n?: I18nAST) {
4343
return new BoundAttribute(
44-
prop.name, prop.type, prop.securityContext, prop.value, prop.unit, prop.sourceSpan, i18n);
44+
prop.name, prop.type, prop.securityContext, prop.value, prop.unit, prop.sourceSpan,
45+
prop.valueSpan, i18n);
4546
}
4647

4748
visit<Result>(visitor: Visitor<Result>): Result { return visitor.visitBoundAttribute(this); }
@@ -96,12 +97,16 @@ export class Content implements Node {
9697
}
9798

9899
export class Variable implements Node {
99-
constructor(public name: string, public value: string, public sourceSpan: ParseSourceSpan) {}
100+
constructor(
101+
public name: string, public value: string, public sourceSpan: ParseSourceSpan,
102+
public valueSpan?: ParseSourceSpan) {}
100103
visit<Result>(visitor: Visitor<Result>): Result { return visitor.visitVariable(this); }
101104
}
102105

103106
export class Reference implements Node {
104-
constructor(public name: string, public value: string, public sourceSpan: ParseSourceSpan) {}
107+
constructor(
108+
public name: string, public value: string, public sourceSpan: ParseSourceSpan,
109+
public valueSpan?: ParseSourceSpan) {}
105110
visit<Result>(visitor: Visitor<Result>): Result { return visitor.visitReference(this); }
106111
}
107112

‎packages/compiler/src/render3/r3_template_transform.ts‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -297,20 +297,20 @@ class HtmlAstToIvyAst implements html.Visitor {
297297
hasBinding = true;
298298
if (bindParts[KW_BIND_IDX] != null) {
299299
this.bindingParser.parsePropertyBinding(
300-
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, matchableAttributes,
301-
parsedProperties);
300+
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, attribute.valueSpan,
301+
matchableAttributes, parsedProperties);
302302

303303
} else if (bindParts[KW_LET_IDX]) {
304304
if (isTemplateElement) {
305305
const identifier = bindParts[IDENT_KW_IDX];
306-
this.parseVariable(identifier, value, srcSpan, variables);
306+
this.parseVariable(identifier, value, srcSpan, attribute.valueSpan, variables);
307307
} else {
308308
this.reportError(`"let-" is only supported on ng-template elements.`, srcSpan);
309309
}
310310

311311
} else if (bindParts[KW_REF_IDX]) {
312312
const identifier = bindParts[IDENT_KW_IDX];
313-
this.parseReference(identifier, value, srcSpan, references);
313+
this.parseReference(identifier, value, srcSpan, attribute.valueSpan, references);
314314

315315
} else if (bindParts[KW_ON_IDX]) {
316316
const events: ParsedEvent[] = [];
@@ -320,27 +320,28 @@ class HtmlAstToIvyAst implements html.Visitor {
320320
addEvents(events, boundEvents);
321321
} else if (bindParts[KW_BINDON_IDX]) {
322322
this.bindingParser.parsePropertyBinding(
323-
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, matchableAttributes,
324-
parsedProperties);
323+
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, attribute.valueSpan,
324+
matchableAttributes, parsedProperties);
325325
this.parseAssignmentEvent(
326326
bindParts[IDENT_KW_IDX], value, srcSpan, attribute.valueSpan, matchableAttributes,
327327
boundEvents);
328328
} else if (bindParts[KW_AT_IDX]) {
329329
this.bindingParser.parseLiteralAttr(
330-
name, value, srcSpan, absoluteOffset, matchableAttributes, parsedProperties);
330+
name, value, srcSpan, absoluteOffset, attribute.valueSpan, matchableAttributes,
331+
parsedProperties);
331332

332333
} else if (bindParts[IDENT_BANANA_BOX_IDX]) {
333334
this.bindingParser.parsePropertyBinding(
334335
bindParts[IDENT_BANANA_BOX_IDX], value, false, srcSpan, absoluteOffset,
335-
matchableAttributes, parsedProperties);
336+
attribute.valueSpan, matchableAttributes, parsedProperties);
336337
this.parseAssignmentEvent(
337338
bindParts[IDENT_BANANA_BOX_IDX], value, srcSpan, attribute.valueSpan,
338339
matchableAttributes, boundEvents);
339340

340341
} else if (bindParts[IDENT_PROPERTY_IDX]) {
341342
this.bindingParser.parsePropertyBinding(
342343
bindParts[IDENT_PROPERTY_IDX], value, false, srcSpan, absoluteOffset,
343-
matchableAttributes, parsedProperties);
344+
attribute.valueSpan, matchableAttributes, parsedProperties);
344345

345346
} else if (bindParts[IDENT_EVENT_IDX]) {
346347
const events: ParsedEvent[] = [];
@@ -351,7 +352,7 @@ class HtmlAstToIvyAst implements html.Visitor {
351352
}
352353
} else {
353354
hasBinding = this.bindingParser.parsePropertyInterpolation(
354-
name, value, srcSpan, matchableAttributes, parsedProperties);
355+
name, value, srcSpan, attribute.valueSpan, matchableAttributes, parsedProperties);
355356
}
356357

357358
return hasBinding;
@@ -365,20 +366,22 @@ class HtmlAstToIvyAst implements html.Visitor {
365366
}
366367

367368
private parseVariable(
368-
identifier: string, value: string, sourceSpan: ParseSourceSpan, variables: t.Variable[]) {
369+
identifier: string, value: string, sourceSpan: ParseSourceSpan,
370+
valueSpan: ParseSourceSpan|undefined, variables: t.Variable[]) {
369371
if (identifier.indexOf('-') > -1) {
370372
this.reportError(`"-" is not allowed in variable names`, sourceSpan);
371373
}
372-
variables.push(new t.Variable(identifier, value, sourceSpan));
374+
variables.push(new t.Variable(identifier, value, sourceSpan, valueSpan));
373375
}
374376

375377
private parseReference(
376-
identifier: string, value: string, sourceSpan: ParseSourceSpan, references: t.Reference[]) {
378+
identifier: string, value: string, sourceSpan: ParseSourceSpan,
379+
valueSpan: ParseSourceSpan|undefined, references: t.Reference[]) {
377380
if (identifier.indexOf('-') > -1) {
378381
this.reportError(`"-" is not allowed in reference names`, sourceSpan);
379382
}
380383

381-
references.push(new t.Reference(identifier, value, sourceSpan));
384+
references.push(new t.Reference(identifier, value, sourceSpan, valueSpan));
382385
}
383386

384387
private parseAssignmentEvent(

‎packages/compiler/src/template_parser/binding_parser.ts‎

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ export class BindingParser {
5757
const expression = dirMeta.hostProperties[propName];
5858
if (typeof expression === 'string') {
5959
this.parsePropertyBinding(
60-
propName, expression, true, sourceSpan, sourceSpan.start.offset, [], boundProps);
60+
propName, expression, true, sourceSpan, sourceSpan.start.offset, undefined, [],
61+
boundProps);
6162
} else {
6263
this._reportError(
6364
`Value of the host property binding "${propName}" needs to be a string representing an expression but got "${expression}" (${typeof expression})`,
@@ -125,11 +126,13 @@ export class BindingParser {
125126
targetVars.push(new ParsedVariable(binding.key, binding.name, sourceSpan));
126127
} else if (binding.expression) {
127128
this._parsePropertyAst(
128-
binding.key, binding.expression, sourceSpan, targetMatchableAttrs, targetProps);
129+
binding.key, binding.expression, sourceSpan, undefined, targetMatchableAttrs,
130+
targetProps);
129131
} else {
130132
targetMatchableAttrs.push([binding.key, '']);
131133
this.parseLiteralAttr(
132-
binding.key, null, sourceSpan, absoluteOffset, targetMatchableAttrs, targetProps);
134+
binding.key, null, sourceSpan, absoluteOffset, undefined, targetMatchableAttrs,
135+
targetProps);
133136
}
134137
}
135138
}
@@ -158,7 +161,8 @@ export class BindingParser {
158161

159162
parseLiteralAttr(
160163
name: string, value: string|null, sourceSpan: ParseSourceSpan, absoluteOffset: number,
161-
targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) {
164+
valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
165+
targetProps: ParsedProperty[]) {
162166
if (isAnimationLabel(name)) {
163167
name = name.substring(1);
164168
if (value) {
@@ -168,17 +172,18 @@ export class BindingParser {
168172
sourceSpan, ParseErrorLevel.ERROR);
169173
}
170174
this._parseAnimation(
171-
name, value, sourceSpan, absoluteOffset, targetMatchableAttrs, targetProps);
175+
name, value, sourceSpan, absoluteOffset, valueSpan, targetMatchableAttrs, targetProps);
172176
} else {
173177
targetProps.push(new ParsedProperty(
174178
name, this._exprParser.wrapLiteralPrimitive(value, '', absoluteOffset),
175-
ParsedPropertyType.LITERAL_ATTR, sourceSpan));
179+
ParsedPropertyType.LITERAL_ATTR, sourceSpan, valueSpan));
176180
}
177181
}
178182

179183
parsePropertyBinding(
180184
name: string, expression: string, isHost: boolean, sourceSpan: ParseSourceSpan,
181-
absoluteOffset: number, targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) {
185+
absoluteOffset: number, valueSpan: ParseSourceSpan|undefined,
186+
targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) {
182187
let isAnimationProp = false;
183188
if (name.startsWith(ANIMATE_PROP_PREFIX)) {
184189
isAnimationProp = true;
@@ -190,41 +195,48 @@ export class BindingParser {
190195

191196
if (isAnimationProp) {
192197
this._parseAnimation(
193-
name, expression, sourceSpan, absoluteOffset, targetMatchableAttrs, targetProps);
198+
name, expression, sourceSpan, absoluteOffset, valueSpan, targetMatchableAttrs,
199+
targetProps);
194200
} else {
195201
this._parsePropertyAst(
196-
name, this._parseBinding(expression, isHost, sourceSpan, absoluteOffset), sourceSpan,
197-
targetMatchableAttrs, targetProps);
202+
name, this._parseBinding(expression, isHost, valueSpan || sourceSpan, absoluteOffset),
203+
sourceSpan, valueSpan, targetMatchableAttrs, targetProps);
198204
}
199205
}
200206

201207
parsePropertyInterpolation(
202-
name: string, value: string, sourceSpan: ParseSourceSpan, targetMatchableAttrs: string[][],
208+
name: string, value: string, sourceSpan: ParseSourceSpan,
209+
valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
203210
targetProps: ParsedProperty[]): boolean {
204-
const expr = this.parseInterpolation(value, sourceSpan);
211+
const expr = this.parseInterpolation(value, valueSpan || sourceSpan);
205212
if (expr) {
206-
this._parsePropertyAst(name, expr, sourceSpan, targetMatchableAttrs, targetProps);
213+
this._parsePropertyAst(name, expr, sourceSpan, valueSpan, targetMatchableAttrs, targetProps);
207214
return true;
208215
}
209216
return false;
210217
}
211218

212219
private _parsePropertyAst(
213220
name: string, ast: ASTWithSource, sourceSpan: ParseSourceSpan,
214-
targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) {
221+
valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
222+
targetProps: ParsedProperty[]) {
215223
targetMatchableAttrs.push([name, ast.source !]);
216-
targetProps.push(new ParsedProperty(name, ast, ParsedPropertyType.DEFAULT, sourceSpan));
224+
targetProps.push(
225+
new ParsedProperty(name, ast, ParsedPropertyType.DEFAULT, sourceSpan, valueSpan));
217226
}
218227

219228
private _parseAnimation(
220229
name: string, expression: string|null, sourceSpan: ParseSourceSpan, absoluteOffset: number,
221-
targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) {
230+
valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
231+
targetProps: ParsedProperty[]) {
222232
// This will occur when a @trigger is not paired with an expression.
223233
// For animations it is valid to not have an expression since */void
224234
// states will be applied by angular when the element is attached/detached
225-
const ast = this._parseBinding(expression || 'undefined', false, sourceSpan, absoluteOffset);
235+
const ast = this._parseBinding(
236+
expression || 'undefined', false, valueSpan || sourceSpan, absoluteOffset);
226237
targetMatchableAttrs.push([name, ast.source !]);
227-
targetProps.push(new ParsedProperty(name, ast, ParsedPropertyType.ANIMATION, sourceSpan));
238+
targetProps.push(
239+
new ParsedProperty(name, ast, ParsedPropertyType.ANIMATION, sourceSpan, valueSpan));
228240
}
229241

230242
private _parseBinding(
@@ -253,7 +265,7 @@ export class BindingParser {
253265
if (boundProp.isAnimation) {
254266
return new BoundElementProperty(
255267
boundProp.name, BindingType.Animation, SecurityContext.NONE, boundProp.expression, null,
256-
boundProp.sourceSpan);
268+
boundProp.sourceSpan, boundProp.valueSpan);
257269
}
258270

259271
let unit: string|null = null;
@@ -306,7 +318,7 @@ export class BindingParser {
306318

307319
return new BoundElementProperty(
308320
boundPropertyName, bindingType, securityContexts[0], boundProp.expression, unit,
309-
boundProp.sourceSpan);
321+
boundProp.sourceSpan, boundProp.valueSpan);
310322
}
311323

312324
parseEvent(

‎packages/compiler/src/template_parser/template_parser.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -426,8 +426,8 @@ class TemplateParseVisitor implements html.Visitor {
426426
hasBinding = true;
427427
if (bindParts[KW_BIND_IDX] != null) {
428428
this._bindingParser.parsePropertyBinding(
429-
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, targetMatchableAttrs,
430-
targetProps);
429+
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, attr.valueSpan,
430+
targetMatchableAttrs, targetProps);
431431

432432
} else if (bindParts[KW_LET_IDX]) {
433433
if (isTemplateElement) {
@@ -448,27 +448,28 @@ class TemplateParseVisitor implements html.Visitor {
448448

449449
} else if (bindParts[KW_BINDON_IDX]) {
450450
this._bindingParser.parsePropertyBinding(
451-
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, targetMatchableAttrs,
452-
targetProps);
451+
bindParts[IDENT_KW_IDX], value, false, srcSpan, absoluteOffset, attr.valueSpan,
452+
targetMatchableAttrs, targetProps);
453453
this._parseAssignmentEvent(
454454
bindParts[IDENT_KW_IDX], value, srcSpan, attr.valueSpan || srcSpan,
455455
targetMatchableAttrs, boundEvents);
456456

457457
} else if (bindParts[KW_AT_IDX]) {
458458
this._bindingParser.parseLiteralAttr(
459-
name, value, srcSpan, absoluteOffset, targetMatchableAttrs, targetProps);
459+
name, value, srcSpan, absoluteOffset, attr.valueSpan, targetMatchableAttrs,
460+
targetProps);
460461

461462
} else if (bindParts[IDENT_BANANA_BOX_IDX]) {
462463
this._bindingParser.parsePropertyBinding(
463-
bindParts[IDENT_BANANA_BOX_IDX], value, false, srcSpan, absoluteOffset,
464+
bindParts[IDENT_BANANA_BOX_IDX], value, false, srcSpan, absoluteOffset, attr.valueSpan,
464465
targetMatchableAttrs, targetProps);
465466
this._parseAssignmentEvent(
466467
bindParts[IDENT_BANANA_BOX_IDX], value, srcSpan, attr.valueSpan || srcSpan,
467468
targetMatchableAttrs, boundEvents);
468469

469470
} else if (bindParts[IDENT_PROPERTY_IDX]) {
470471
this._bindingParser.parsePropertyBinding(
471-
bindParts[IDENT_PROPERTY_IDX], value, false, srcSpan, absoluteOffset,
472+
bindParts[IDENT_PROPERTY_IDX], value, false, srcSpan, absoluteOffset, attr.valueSpan,
472473
targetMatchableAttrs, targetProps);
473474

474475
} else if (bindParts[IDENT_EVENT_IDX]) {
@@ -478,12 +479,12 @@ class TemplateParseVisitor implements html.Visitor {
478479
}
479480
} else {
480481
hasBinding = this._bindingParser.parsePropertyInterpolation(
481-
name, value, srcSpan, targetMatchableAttrs, targetProps);
482+
name, value, srcSpan, attr.valueSpan, targetMatchableAttrs, targetProps);
482483
}
483484

484485
if (!hasBinding) {
485486
this._bindingParser.parseLiteralAttr(
486-
name, value, srcSpan, absoluteOffset, targetMatchableAttrs, targetProps);
487+
name, value, srcSpan, absoluteOffset, attr.valueSpan, targetMatchableAttrs, targetProps);
487488
}
488489

489490
targetEvents.push(...boundEvents.map(e => t.BoundEventAst.fromParsedEvent(e)));

0 commit comments

Comments
 (0)