From 4b06ef75aa5f3438ff9fe533c0370f530ecc2e97 Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 1 Oct 2020 00:17:21 +0200 Subject: [PATCH] refactor(compiler): associate accurate source spans with ICU expressions (#39072) Prior to this change, expressions within ICUs would have a source span corresponding with the whole ICU. This commit narrows down the source spans of these expressions to the exact location in the source file, as a prerequisite for reporting type check errors within these expressions. PR Close #39072 --- .../compiler/src/expression_parser/parser.ts | 31 +++++++++++-- packages/compiler/src/i18n/i18n_ast.ts | 16 ++++++- packages/compiler/src/i18n/i18n_parser.ts | 22 +++++++--- .../compiler/src/i18n/translation_bundle.ts | 4 +- .../src/render3/r3_template_transform.ts | 13 ++---- .../src/template_parser/binding_parser.ts | 20 +++++++++ .../compiler/test/i18n/i18n_parser_spec.ts | 2 +- .../test/i18n/translation_bundle_spec.ts | 14 +++++- .../test/render3/r3_ast_absolute_span_spec.ts | 21 +++++++++ .../test/render3/r3_ast_spans_spec.ts | 44 ++++++++++++++++++- .../compiler/test/render3/util/expression.ts | 9 +++- 11 files changed, 169 insertions(+), 27 deletions(-) diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 4b5b28b8b5..707f4f02a2 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -179,10 +179,33 @@ export class Parser { expressions.push(ast); } - const span = new ParseSpan(0, input == null ? 0 : input.length); - return new ASTWithSource( - new Interpolation(span, span.toAbsolute(absoluteOffset), split.strings, expressions), input, - location, absoluteOffset, this.errors); + return this.createInterpolationAst(split.strings, expressions, input, location, absoluteOffset); + } + + /** + * Similar to `parseInterpolation`, but treats the provided string as a single expression + * element that would normally appear within the interpolation prefix and suffix (`{{` and `}}`). + * This is used for parsing the switch expression in ICUs. + */ + parseInterpolationExpression(expression: string, location: any, absoluteOffset: number): + ASTWithSource { + const sourceToLex = this._stripComments(expression); + const tokens = this._lexer.tokenize(sourceToLex); + const ast = new _ParseAST( + expression, location, absoluteOffset, tokens, sourceToLex.length, + /* parseAction */ false, this.errors, 0) + .parseChain(); + const strings = ['', '']; // The prefix and suffix strings are both empty + return this.createInterpolationAst(strings, [ast], expression, location, absoluteOffset); + } + + private createInterpolationAst( + strings: string[], expressions: AST[], input: string, location: string, + absoluteOffset: number): ASTWithSource { + const span = new ParseSpan(0, input.length); + const interpolation = + new Interpolation(span, span.toAbsolute(absoluteOffset), strings, expressions); + return new ASTWithSource(interpolation, input, location, absoluteOffset, this.errors); } /** diff --git a/packages/compiler/src/i18n/i18n_ast.ts b/packages/compiler/src/i18n/i18n_ast.ts index 9bd6259bd3..f202ba9e06 100644 --- a/packages/compiler/src/i18n/i18n_ast.ts +++ b/packages/compiler/src/i18n/i18n_ast.ts @@ -8,6 +8,18 @@ import {ParseSourceSpan} from '../parse_util'; +/** + * Describes the text contents of a placeholder as it appears in an ICU expression, including its + * source span information. + */ +export interface MessagePlaceholder { + /** The text contents of the placeholder */ + text: string; + + /** The source span of the placeholder */ + sourceSpan: ParseSourceSpan; +} + export class Message { sources: MessageSpan[]; id: string = this.customId; @@ -16,14 +28,14 @@ export class Message { /** * @param nodes message AST - * @param placeholders maps placeholder names to static content + * @param placeholders maps placeholder names to static content and their source spans * @param placeholderToMessage maps placeholder names to messages (used for nested ICU messages) * @param meaning * @param description * @param customId */ constructor( - public nodes: Node[], public placeholders: {[phName: string]: string}, + public nodes: Node[], public placeholders: {[phName: string]: MessagePlaceholder}, public placeholderToMessage: {[phName: string]: Message}, public meaning: string, public description: string, public customId: string) { if (nodes.length) { diff --git a/packages/compiler/src/i18n/i18n_parser.ts b/packages/compiler/src/i18n/i18n_parser.ts index a7d22a1f8f..4fa1f55f4b 100644 --- a/packages/compiler/src/i18n/i18n_parser.ts +++ b/packages/compiler/src/i18n/i18n_parser.ts @@ -39,7 +39,7 @@ interface I18nMessageVisitorContext { isIcu: boolean; icuDepth: number; placeholderRegistry: PlaceholderRegistry; - placeholderToContent: {[phName: string]: string}; + placeholderToContent: {[phName: string]: i18n.MessagePlaceholder}; placeholderToMessage: {[phName: string]: i18n.Message}; visitNodeFn: VisitNodeFn; } @@ -83,13 +83,19 @@ class _I18nVisitor implements html.Visitor { const isVoid: boolean = getHtmlTagDefinition(el.name).isVoid; const startPhName = context.placeholderRegistry.getStartTagPlaceholderName(el.name, attrs, isVoid); - context.placeholderToContent[startPhName] = el.startSourceSpan.toString(); + context.placeholderToContent[startPhName] = { + text: el.startSourceSpan.toString(), + sourceSpan: el.startSourceSpan, + }; let closePhName = ''; if (!isVoid) { closePhName = context.placeholderRegistry.getCloseTagPlaceholderName(el.name); - context.placeholderToContent[closePhName] = ``; + context.placeholderToContent[closePhName] = { + text: ``, + sourceSpan: el.endSourceSpan ?? el.sourceSpan, + }; } const node = new i18n.TagPlaceholder( @@ -128,7 +134,10 @@ class _I18nVisitor implements html.Visitor { // - the ICU message is nested. const expPh = context.placeholderRegistry.getUniquePlaceholder(`VAR_${icu.type}`); i18nIcu.expressionPlaceholder = expPh; - context.placeholderToContent[expPh] = icu.switchValue; + context.placeholderToContent[expPh] = { + text: icu.switchValue, + sourceSpan: icu.switchValueSourceSpan, + }; return context.visitNodeFn(icu, i18nIcu); } @@ -175,7 +184,10 @@ class _I18nVisitor implements html.Visitor { const expressionSpan = getOffsetSourceSpan(sourceSpan, splitInterpolation.expressionsSpans[i]); nodes.push(new i18n.Placeholder(expression, phName, expressionSpan)); - context.placeholderToContent[phName] = sDelimiter + expression + eDelimiter; + context.placeholderToContent[phName] = { + text: sDelimiter + expression + eDelimiter, + sourceSpan: expressionSpan, + }; } // The last index contains no expression diff --git a/packages/compiler/src/i18n/translation_bundle.ts b/packages/compiler/src/i18n/translation_bundle.ts index 5fa66c6ae4..79e899e2e0 100644 --- a/packages/compiler/src/i18n/translation_bundle.ts +++ b/packages/compiler/src/i18n/translation_bundle.ts @@ -109,7 +109,7 @@ class I18nToHtmlVisitor implements i18n.Visitor { // TODO(vicb): Once all format switch to using expression placeholders // we should throw when the placeholder is not in the source message const exp = this._srcMsg.placeholders.hasOwnProperty(icu.expression) ? - this._srcMsg.placeholders[icu.expression] : + this._srcMsg.placeholders[icu.expression].text : icu.expression; return `{${exp}, ${icu.type}, ${cases.join(' ')}}`; @@ -118,7 +118,7 @@ class I18nToHtmlVisitor implements i18n.Visitor { visitPlaceholder(ph: i18n.Placeholder, context?: any): string { const phName = this._mapper(ph.name); if (this._srcMsg.placeholders.hasOwnProperty(phName)) { - return this._srcMsg.placeholders[phName]; + return this._srcMsg.placeholders[phName].text; } if (this._srcMsg.placeholderToMessage.hasOwnProperty(phName)) { diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts index 92d652d16d..fe29a9bc40 100644 --- a/packages/compiler/src/render3/r3_template_transform.ts +++ b/packages/compiler/src/render3/r3_template_transform.ts @@ -266,12 +266,6 @@ class HtmlAstToIvyAst implements html.Visitor { Object.keys(message.placeholders).forEach(key => { const value = message.placeholders[key]; if (key.startsWith(I18N_ICU_VAR_PREFIX)) { - const config = this.bindingParser.interpolationConfig; - - // ICU expression is a plain string, not wrapped into start - // and end tags, so we wrap it before passing to binding parser - const wrapped = `${config.start}${value}${config.end}`; - // Currently when the `plural` or `select` keywords in an ICU contain trailing spaces (e.g. // `{count, select , ...}`), these spaces are also included into the key names in ICU vars // (e.g. "VAR_SELECT "). These trailing spaces are not desirable, since they will later be @@ -279,10 +273,11 @@ class HtmlAstToIvyAst implements html.Visitor { // mismatches at runtime (i.e. placeholder will not be replaced with the correct value). const formattedKey = key.trim(); - vars[formattedKey] = - this._visitTextWithInterpolation(wrapped, expansion.sourceSpan) as t.BoundText; + const ast = this.bindingParser.parseInterpolationExpression(value.text, value.sourceSpan); + + vars[formattedKey] = new t.BoundText(ast, value.sourceSpan); } else { - placeholders[key] = this._visitTextWithInterpolation(value, expansion.sourceSpan); + placeholders[key] = this._visitTextWithInterpolation(value.text, value.sourceSpan); } }); return new t.Icu(vars, placeholders, expansion.sourceSpan, message); diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts index bc4ccf9cd1..81e0b69b1d 100644 --- a/packages/compiler/src/template_parser/binding_parser.ts +++ b/packages/compiler/src/template_parser/binding_parser.ts @@ -127,6 +127,26 @@ export class BindingParser { } } + /** + * Similar to `parseInterpolation`, but treats the provided string as a single expression + * element that would normally appear within the interpolation prefix and suffix (`{{` and `}}`). + * This is used for parsing the switch expression in ICUs. + */ + parseInterpolationExpression(expression: string, sourceSpan: ParseSourceSpan): ASTWithSource { + const sourceInfo = sourceSpan.start.toString(); + + try { + const ast = this._exprParser.parseInterpolationExpression( + expression, sourceInfo, sourceSpan.start.offset); + if (ast) this._reportExpressionParserErrors(ast.errors, sourceSpan); + this._checkPipes(ast, sourceSpan); + return ast; + } catch (e) { + this._reportError(`${e}`, sourceSpan); + return this._exprParser.wrapLiteralPrimitive('ERROR', sourceInfo, sourceSpan.start.offset); + } + } + /** * Parses the bindings in a microsyntax expression, and converts them to * `ParsedProperty` or `ParsedVariable`. diff --git a/packages/compiler/test/i18n/i18n_parser_spec.ts b/packages/compiler/test/i18n/i18n_parser_spec.ts index dc84283cb3..96945dfb4a 100644 --- a/packages/compiler/test/i18n/i18n_parser_spec.ts +++ b/packages/compiler/test/i18n/i18n_parser_spec.ts @@ -324,7 +324,7 @@ function _humanizePlaceholders( // clang-format off // https://github.com/angular/clang-format/issues/35 return _extractMessages(html, implicitTags, implicitAttrs).map( - msg => Object.keys(msg.placeholders).map((name) => `${name}=${msg.placeholders[name]}`).join(', ')); + msg => Object.keys(msg.placeholders).map((name) => `${name}=${msg.placeholders[name].text}`).join(', ')); // clang-format on } diff --git a/packages/compiler/test/i18n/translation_bundle_spec.ts b/packages/compiler/test/i18n/translation_bundle_spec.ts index 01f322adca..b7aa290599 100644 --- a/packages/compiler/test/i18n/translation_bundle_spec.ts +++ b/packages/compiler/test/i18n/translation_bundle_spec.ts @@ -50,7 +50,7 @@ import {_extractMessages} from './i18n_parser_spec'; ] }; const phMap = { - ph1: '*phContent*', + ph1: createPlaceholder('*phContent*'), }; const tb = new TranslationBundle(msgMap, null, (_) => 'foo'); const msg = new i18n.Message([srcNode], phMap, {}, 'm', 'd', 'i'); @@ -160,7 +160,7 @@ import {_extractMessages} from './i18n_parser_spec'; ] }; const phMap = { - ph1: '', + ph1: createPlaceholder(''), }; const tb = new TranslationBundle(msgMap, null, (_) => 'foo'); const msg = new i18n.Message([srcNode], phMap, {}, 'm', 'd', 'i'); @@ -169,3 +169,13 @@ import {_extractMessages} from './i18n_parser_spec'; }); }); } + +function createPlaceholder(text: string): i18n.MessagePlaceholder { + const file = new ParseSourceFile(text, 'file://test'); + const start = new ParseLocation(file, 0, 0, 0); + const end = new ParseLocation(file, text.length, 0, text.length); + return { + text, + sourceSpan: new ParseSourceSpan(start, end), + }; +} diff --git a/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts b/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts index 4f3425d07c..bb9082b245 100644 --- a/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts +++ b/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts @@ -339,4 +339,25 @@ describe('expression AST absolute source spans', () => { [['As', new AbsoluteSourceSpan(22, 24)], ['Bs', new AbsoluteSourceSpan(35, 37)]])); }); }); + + describe('ICU expressions', () => { + it('is correct for variables and placeholders', () => { + const spans = humanizeExpressionSource( + parse('{item.var, plural, other { {{item.placeholder}} items } }') + .nodes); + expect(spans).toContain(['item.var', new AbsoluteSourceSpan(12, 20)]); + expect(spans).toContain(['item.placeholder', new AbsoluteSourceSpan(40, 56)]); + }); + + it('is correct for variables and placeholders', () => { + const spans = humanizeExpressionSource( + parse( + '{item.var, plural, other { {{item.placeholder}} {nestedVar, plural, other { {{nestedPlaceholder}} }}} }') + .nodes); + expect(spans).toContain(['item.var', new AbsoluteSourceSpan(12, 20)]); + expect(spans).toContain(['item.placeholder', new AbsoluteSourceSpan(40, 56)]); + expect(spans).toContain(['nestedVar', new AbsoluteSourceSpan(60, 69)]); + expect(spans).toContain(['nestedPlaceholder', new AbsoluteSourceSpan(89, 106)]); + }); + }); }); diff --git a/packages/compiler/test/render3/r3_ast_spans_spec.ts b/packages/compiler/test/render3/r3_ast_spans_spec.ts index 1f82c8a78b..7cbfc704ad 100644 --- a/packages/compiler/test/render3/r3_ast_spans_spec.ts +++ b/packages/compiler/test/render3/r3_ast_spans_spec.ts @@ -89,7 +89,13 @@ class R3AstSourceSpans implements t.Visitor { } visitIcu(icu: t.Icu) { - return null; + this.result.push(['Icu', humanizeSpan(icu.sourceSpan)]); + for (const key of Object.keys(icu.vars)) { + this.result.push(['Icu:Var', humanizeSpan(icu.vars[key].sourceSpan)]); + } + for (const key of Object.keys(icu.placeholders)) { + this.result.push(['Icu:Placeholder', humanizeSpan(icu.placeholders[key].sourceSpan)]); + } } private visitAll(nodes: t.Node[][]) { @@ -420,4 +426,40 @@ describe('R3 AST source spans', () => { ]); }); }); + + describe('ICU expressions', () => { + it('is correct for variables and placeholders', () => { + expectFromHtml('{item.var, plural, other { {{item.placeholder}} items } }') + .toEqual([ + [ + 'Element', + '{item.var, plural, other { {{item.placeholder}} items } }', + '', '' + ], + ['Icu', '{item.var, plural, other { {{item.placeholder}} items } }'], + ['Icu:Var', 'item.var'], + ['Icu:Placeholder', '{{item.placeholder}}'], + ]); + }); + + it('is correct for nested ICUs', () => { + expectFromHtml( + '{item.var, plural, other { {{item.placeholder}} {nestedVar, plural, other { {{nestedPlaceholder}} }}} }') + .toEqual([ + [ + 'Element', + '{item.var, plural, other { {{item.placeholder}} {nestedVar, plural, other { {{nestedPlaceholder}} }}} }', + '', '' + ], + [ + 'Icu', + '{item.var, plural, other { {{item.placeholder}} {nestedVar, plural, other { {{nestedPlaceholder}} }}} }' + ], + ['Icu:Var', 'nestedVar'], + ['Icu:Var', 'item.var'], + ['Icu:Placeholder', '{{item.placeholder}}'], + ['Icu:Placeholder', '{{nestedPlaceholder}}'], + ]); + }); + }); }); diff --git a/packages/compiler/test/render3/util/expression.ts b/packages/compiler/test/render3/util/expression.ts index 1831513158..a0af48ba91 100644 --- a/packages/compiler/test/render3/util/expression.ts +++ b/packages/compiler/test/render3/util/expression.ts @@ -141,7 +141,14 @@ class ExpressionSourceHumanizer extends e.RecursiveAstVisitor implements t.Visit } visitContent(ast: t.Content) {} visitText(ast: t.Text) {} - visitIcu(ast: t.Icu) {} + visitIcu(ast: t.Icu) { + for (const key of Object.keys(ast.vars)) { + ast.vars[key].visit(this); + } + for (const key of Object.keys(ast.placeholders)) { + ast.placeholders[key].visit(this); + } + } } /**