From c18c7e23ec994a529c365f8768fd8cfd68522d7c Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 21 Jan 2021 21:48:42 +0100 Subject: [PATCH] fix(compiler): exclude trailing whitespace from element source spans (#40513) If the template parse option `leadingTriviaChars` is configured to consider whitespace as trivia, any trailing whitespace of an element would be considered as leading trivia of the subsequent element, such that its `start` span would start _after_ the whitespace. This means that the start span cannot be used to mark the end of the current element, as its trailing whitespace would then be included in its span. Instead, the full start of the subsequent element should be used. To harden the tests that for the Ivy parser, the test utility `parseR3` has been adjusted to use the same configuration for `leadingTriviaChars` as would be the case in its production counterpart `parseTemplate`. This uncovered another bug in offset handling of the interpolation parser, where the absolute offset was computed from the start source span (which excludes leading trivia) whereas the interpolation expression would include the leading trivia. As such, the absolute offset now also uses the full start span. Fixes #39148 PR Close #40513 --- .../src/ngtsc/annotations/src/component.ts | 3 +++ .../src/ngtsc/typecheck/src/checker.ts | 5 +++++ .../external_templates/escaped_chars.js | 2 +- .../i18n_message_element_whitespace.js | 4 ++-- .../test/ngtsc/template_mapping_spec.ts | 6 +++--- packages/compiler/src/ml_parser/parser.ts | 2 +- .../src/template_parser/binding_parser.ts | 12 +++++++----- .../compiler/test/ml_parser/html_parser_spec.ts | 17 +++++++++++++++++ .../compiler/test/render3/r3_ast_spans_spec.ts | 7 +++++++ .../compiler/test/render3/view/i18n_spec.ts | 2 +- packages/compiler/test/render3/view/util.ts | 8 +++++--- .../ivy/test/legacy/template_target_spec.ts | 11 ++++++++++- 12 files changed, 62 insertions(+), 17 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index f68f4bc21d..9f1805088c 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -879,6 +879,9 @@ export class ComponentDecoratorHandler implements // // In order to guarantee the correctness of diagnostics, templates are parsed a second time // with the above options set to preserve source mappings. + // + // Note: template parse options should be aligned with `template_target_spec.ts` and + // `TemplateTypeCheckerImpl.overrideComponentTemplate`. const {nodes: diagNodes} = parseTemplate(templateStr, template.sourceMapUrl, { preserveWhitespaces: true, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index 68ff907a02..6a63764c81 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -154,6 +154,11 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { overrideComponentTemplate(component: ts.ClassDeclaration, template: string): {nodes: TmplAstNode[], errors: ParseError[]|null} { const {nodes, errors} = parseTemplate(template, 'override.html', { + // Set `leadingTriviaChars` and `preserveWhitespaces` such that whitespace is not stripped + // and fully accounted for in source spans. Without these flags the source spans can be + // inaccurate. + // Note: template parse options should be aligned with `template_target_spec.ts` and the + // `diagNodes` in `ComponentDecoratorHandler._parseTemplate`. preserveWhitespaces: true, leadingTriviaChars: [], }); diff --git a/packages/compiler-cli/test/compliance/test_cases/source_mapping/external_templates/escaped_chars.js b/packages/compiler-cli/test/compliance/test_cases/source_mapping/external_templates/escaped_chars.js index 51a98de501..0ecf510659 100644 --- a/packages/compiler-cli/test/compliance/test_cases/source_mapping/external_templates/escaped_chars.js +++ b/packages/compiler-cli/test/compliance/test_cases/source_mapping/external_templates/escaped_chars.js @@ -1,4 +1,4 @@ -i0.ɵɵelementStart(0, "div") // SOURCE: "/escaped_chars.html"
\r\n +i0.ɵɵelementStart(0, "div") // SOURCE: "/escaped_chars.html"
… // NOTE: the `\\r\\n` at the end of the next line will be unescaped to `\r\n`. If it was just `\r\n` it would get unescaped to the actual characters. i0.ɵɵtext(1, " Some Message Encoded character: \uD83D\uDE80\\n") // SOURCE: "/escaped_chars.html" Some Message\r\n Encoded character: 🚀\\r\\n diff --git a/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js b/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js index d72da22f8c..f0be5ea453 100644 --- a/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js +++ b/packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js @@ -9,9 +9,9 @@ }:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" post-p\\n … -i0.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts"
\\n +i0.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts"
… -i0.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts"
\\n +i0.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts"
… i0.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts"

\\n … diff --git a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts index 0fda1f1f78..8c22ab7aeb 100644 --- a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts @@ -432,7 +432,7 @@ runInEachFileSystem((os) => { const mappings = compileAndMap( `

pre-body {{greeting}} post-body
`); expectMapping(mappings, { - source: '
', + source: '
', generated: 'i0.ɵɵelementStart(0, "div", 0)', sourceUrl: '../test.ts', }); @@ -491,12 +491,12 @@ runInEachFileSystem((os) => { // ivy instructions expectMapping(mappings, { sourceUrl: '../test.ts', - source: '
\n ', + source: '
', generated: 'i0.ɵɵelementStart(0, "div")', }); expectMapping(mappings, { sourceUrl: '../test.ts', - source: '
\n ', + source: '
', generated: 'i0.ɵɵi18nStart(1, 0)', }); expectMapping(mappings, { diff --git a/packages/compiler/src/ml_parser/parser.ts b/packages/compiler/src/ml_parser/parser.ts index df73b9b6ac..905c25d583 100644 --- a/packages/compiler/src/ml_parser/parser.ts +++ b/packages/compiler/src/ml_parser/parser.ts @@ -259,7 +259,7 @@ class _TreeBuilder { this._advance(); selfClosing = false; } - const end = this._peek.sourceSpan.start; + const end = this._peek.sourceSpan.fullStart; const span = new ParseSourceSpan( startTagToken.sourceSpan.start, end, startTagToken.sourceSpan.fullStart); // Create a separate `startSpan` because `span` will be modified when there is an `end` span. diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts index 6608a5fd19..daffb70155 100644 --- a/packages/compiler/src/template_parser/binding_parser.ts +++ b/packages/compiler/src/template_parser/binding_parser.ts @@ -120,16 +120,17 @@ export class BindingParser { parseInterpolation(value: string, sourceSpan: ParseSourceSpan): ASTWithSource { const sourceInfo = sourceSpan.start.toString(); + const absoluteOffset = sourceSpan.fullStart.offset; try { const ast = this._exprParser.parseInterpolation( - value, sourceInfo, sourceSpan.start.offset, this._interpolationConfig)!; + value, sourceInfo, absoluteOffset, this._interpolationConfig)!; 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); + return this._exprParser.wrapLiteralPrimitive('ERROR', sourceInfo, absoluteOffset); } } @@ -140,16 +141,17 @@ export class BindingParser { */ parseInterpolationExpression(expression: string, sourceSpan: ParseSourceSpan): ASTWithSource { const sourceInfo = sourceSpan.start.toString(); + const absoluteOffset = sourceSpan.start.offset; try { - const ast = this._exprParser.parseInterpolationExpression( - expression, sourceInfo, sourceSpan.start.offset); + const ast = + this._exprParser.parseInterpolationExpression(expression, sourceInfo, absoluteOffset); 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); + return this._exprParser.wrapLiteralPrimitive('ERROR', sourceInfo, absoluteOffset); } } diff --git a/packages/compiler/test/ml_parser/html_parser_spec.ts b/packages/compiler/test/ml_parser/html_parser_spec.ts index 9f1932d224..48e87d07cb 100644 --- a/packages/compiler/test/ml_parser/html_parser_spec.ts +++ b/packages/compiler/test/ml_parser/html_parser_spec.ts @@ -709,6 +709,23 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn, humanizeNodes} ]); }); + it('should set the end source span excluding trailing whitespace whitespace', () => { + expect(humanizeDomSourceSpans( + parser.parse('\n\n\n \n', 'TestComp', { + leadingTriviaChars: [' ', '\n', '\r', '\t'], + }))) + .toEqual([ + [ + html.Element, 'input', 0, '', '', + '' + ], + [html.Attribute, 'type', 'text', 'type="text"'], + [html.Text, '\n\n\n ', 0, ''], + [html.Element, 'span', 0, '\n', '', ''], + [html.Text, '\n', 1, ''], + ]); + }); + it('should not set the end source span for elements that are implicitly closed', () => { expect(humanizeDomSourceSpans(parser.parse('

', 'TestComp'))).toEqual([ [html.Element, 'div', 0, '

', '
', '
'], diff --git a/packages/compiler/test/render3/r3_ast_spans_spec.ts b/packages/compiler/test/render3/r3_ast_spans_spec.ts index f6f03a17a9..978e7245d0 100644 --- a/packages/compiler/test/render3/r3_ast_spans_spec.ts +++ b/packages/compiler/test/render3/r3_ast_spans_spec.ts @@ -148,6 +148,13 @@ describe('R3 AST source spans', () => { ['TextAttribute', 'a', 'a', ''], ]); }); + + it('is correct for self-closing elements with trailing whitespace', () => { + expectFromHtml('\n \n').toEqual([ + ['Element', '', '', ''], + ['Element', '\n', '', ''], + ]); + }); }); describe('bound text nodes', () => { diff --git a/packages/compiler/test/render3/view/i18n_spec.ts b/packages/compiler/test/render3/view/i18n_spec.ts index b6fca34b77..170f48ca5d 100644 --- a/packages/compiler/test/render3/view/i18n_spec.ts +++ b/packages/compiler/test/render3/view/i18n_spec.ts @@ -523,7 +523,7 @@ describe('serializeI18nMessageForLocalize', () => { expect(humanizeSourceSpan(messageParts[3].sourceSpan)).toEqual('"" (29-29)'); expect(placeHolders[0].text).toEqual('START_BOLD_TEXT'); - expect(humanizeSourceSpan(placeHolders[0].sourceSpan)).toEqual('" " (10-16)'); + expect(humanizeSourceSpan(placeHolders[0].sourceSpan)).toEqual('"" (10-13)'); expect(placeHolders[1].text).toEqual('INTERPOLATION'); expect(humanizeSourceSpan(placeHolders[1].sourceSpan)).toEqual('"{{value}}" (16-25)'); expect(placeHolders[2].text).toEqual('CLOSE_BOLD_TEXT'); diff --git a/packages/compiler/test/render3/view/util.ts b/packages/compiler/test/render3/view/util.ts index dd3f47125c..7ace02c092 100644 --- a/packages/compiler/test/render3/view/util.ts +++ b/packages/compiler/test/render3/view/util.ts @@ -16,6 +16,7 @@ import {DEFAULT_INTERPOLATION_CONFIG, InterpolationConfig} from '../../../src/ml import * as a from '../../../src/render3/r3_ast'; import {htmlAstToRender3Ast, Render3ParseResult} from '../../../src/render3/r3_template_transform'; import {I18nMetaVisitor} from '../../../src/render3/view/i18n/meta'; +import {LEADING_TRIVIA_CHARS} from '../../../src/render3/view/template'; import {BindingParser} from '../../../src/template_parser/binding_parser'; import {MockSchemaRegistry} from '../../../testing'; @@ -84,9 +85,10 @@ export function parseR3( ignoreError?: boolean} = {}): Render3ParseResult { const htmlParser = new HtmlParser(); - const parseResult = htmlParser.parse( - input, 'path:://to/template', - {tokenizeExpansionForms: true, leadingTriviaChars: options.leadingTriviaChars}); + const parseResult = htmlParser.parse(input, 'path:://to/template', { + tokenizeExpansionForms: true, + leadingTriviaChars: options.leadingTriviaChars ?? LEADING_TRIVIA_CHARS, + }); if (parseResult.errors.length > 0 && !options.ignoreError) { const msg = parseResult.errors.map(e => e.toString()).join('\n'); diff --git a/packages/language-service/ivy/test/legacy/template_target_spec.ts b/packages/language-service/ivy/test/legacy/template_target_spec.ts index 87eed427c1..0035173a07 100644 --- a/packages/language-service/ivy/test/legacy/template_target_spec.ts +++ b/packages/language-service/ivy/test/legacy/template_target_spec.ts @@ -27,7 +27,16 @@ function parse(template: string): ParseResult { template = template.replace('¦', ''); const templateUrl = '/foo'; return { - ...parseTemplate(template, templateUrl), + ...parseTemplate(template, templateUrl, { + // Set `leadingTriviaChars` and `preserveWhitespaces` such that whitespace is not stripped + // and fully accounted for in source spans. Without these flags the source spans can be + // inaccurate. + // Note: template parse options should be aligned with the `diagNodes` in + // `ComponentDecoratorHandler._parseTemplate`. and + // `TemplateTypeCheckerImpl.overrideComponentTemplate`. + leadingTriviaChars: [], + preserveWhitespaces: true, + }), position, }; }