Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid extraction text issue (BACKPORT to 10.2.x) #39589

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/compiler/src/compiler_facade_interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,4 +193,5 @@ export interface ParseSourceSpan {
start: any;
end: any;
details: any;
fullStart: any;
}
3 changes: 2 additions & 1 deletion packages/compiler/src/compiler_util/expression_converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,8 @@ class _AstToIrVisitor implements cdAst.AstVisitor {
if (this.baseSourceSpan) {
const start = this.baseSourceSpan.start.moveBy(span.start);
const end = this.baseSourceSpan.start.moveBy(span.end);
return new ParseSourceSpan(start, end);
const fullStart = this.baseSourceSpan.fullStart.moveBy(span.start);
return new ParseSourceSpan(start, end, fullStart);
} else {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/i18n/i18n_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class _I18nVisitor implements html.Visitor {

function getOffsetSourceSpan(
sourceSpan: ParseSourceSpan, {start, end}: {start: number, end: number}): ParseSourceSpan {
return new ParseSourceSpan(sourceSpan.start.moveBy(start), sourceSpan.start.moveBy(end));
return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end));
}

const _CUSTOM_PH_EXP =
Expand Down
18 changes: 12 additions & 6 deletions packages/compiler/src/ml_parser/lexer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -918,19 +918,20 @@ class PlainCharacterCursor implements CharacterCursor {

getSpan(start?: this, leadingTriviaCodePoints?: number[]): ParseSourceSpan {
start = start || this;
let cloned = false;
let fullStart = start;
if (leadingTriviaCodePoints) {
while (this.diff(start) > 0 && leadingTriviaCodePoints.indexOf(start.peek()) !== -1) {
if (!cloned) {
if (fullStart === start) {
start = start.clone() as this;
cloned = true;
}
start.advance();
}
}
return new ParseSourceSpan(
new ParseLocation(start.file, start.state.offset, start.state.line, start.state.column),
new ParseLocation(this.file, this.state.offset, this.state.line, this.state.column));
const startLocation = this.locationFromCursor(start);
const endLocation = this.locationFromCursor(this);
const fullStartLocation =
fullStart !== start ? this.locationFromCursor(fullStart) : startLocation;
return new ParseSourceSpan(startLocation, endLocation, fullStartLocation);
}

getChars(start: this): string {
Expand Down Expand Up @@ -960,6 +961,11 @@ class PlainCharacterCursor implements CharacterCursor {
protected updatePeek(state: CursorState): void {
state.peek = state.offset >= this.end ? chars.$EOF : this.charAt(state.offset);
}

private locationFromCursor(cursor: this): ParseLocation {
return new ParseLocation(
cursor.file, cursor.state.offset, cursor.state.line, cursor.state.column);
}
}

class EscapedCharacterCursor extends PlainCharacterCursor {
Expand Down
21 changes: 15 additions & 6 deletions packages/compiler/src/ml_parser/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ class _TreeBuilder {
TreeError.create(null, this._peek.sourceSpan, `Invalid ICU message. Missing '}'.`));
return;
}
const sourceSpan = new ParseSourceSpan(token.sourceSpan.start, this._peek.sourceSpan.end);
const sourceSpan = new ParseSourceSpan(
token.sourceSpan.start, this._peek.sourceSpan.end, token.sourceSpan.fullStart);
this._addToParent(new html.Expansion(
switchValue.parts[0], type.parts[0], cases, sourceSpan, switchValue.sourceSpan));

Expand Down Expand Up @@ -162,8 +163,10 @@ class _TreeBuilder {
return null;
}

const sourceSpan = new ParseSourceSpan(value.sourceSpan.start, end.sourceSpan.end);
const expSourceSpan = new ParseSourceSpan(start.sourceSpan.start, end.sourceSpan.end);
const sourceSpan =
new ParseSourceSpan(value.sourceSpan.start, end.sourceSpan.end, value.sourceSpan.fullStart);
const expSourceSpan =
new ParseSourceSpan(start.sourceSpan.start, end.sourceSpan.end, start.sourceSpan.fullStart);
return new html.ExpansionCase(
value.parts[0], expansionCaseParser.rootNodes, sourceSpan, value.sourceSpan, expSourceSpan);
}
Expand Down Expand Up @@ -257,8 +260,12 @@ class _TreeBuilder {
selfClosing = false;
}
const end = this._peek.sourceSpan.start;
const span = new ParseSourceSpan(startTagToken.sourceSpan.start, end);
const el = new html.Element(fullName, attrs, [], span, span, undefined);
const span = new ParseSourceSpan(
startTagToken.sourceSpan.start, end, startTagToken.sourceSpan.fullStart);
// Create a separate `startSpan` because `span` may be modified when there is an `end` span.
const startSpan = new ParseSourceSpan(
startTagToken.sourceSpan.start, end, startTagToken.sourceSpan.fullStart);
const el = new html.Element(fullName, attrs, [], span, startSpan, undefined);
this._pushElement(el);
if (selfClosing) {
// Elements that are self-closed have their `endSourceSpan` set to the full span, as the
Expand Down Expand Up @@ -332,7 +339,9 @@ class _TreeBuilder {
end = quoteToken.sourceSpan.end;
}
return new html.Attribute(
fullName, value, new ParseSourceSpan(attrName.sourceSpan.start, end), valueSpan);
fullName, value,
new ParseSourceSpan(attrName.sourceSpan.start, end, attrName.sourceSpan.fullStart),
valueSpan);
}

private _getParentElement(): html.Element|null {
Expand Down
26 changes: 25 additions & 1 deletion packages/compiler/src/parse_util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,32 @@ export class ParseSourceFile {
}

export class ParseSourceSpan {
/**
* Create an object that holds information about spans of tokens/nodes captured during
* lexing/parsing of text.
*
* @param start
* The location of the start of the span (having skipped leading trivia).
* Skipping leading trivia makes source-spans more "user friendly", since things like HTML
* elements will appear to begin at the start of the opening tag, rather than at the start of any
* leading trivia, which could include newlines.
*
* @param end
* The location of the end of the span.
*
* @param fullStart
* The start of the token without skipping the leading trivia.
* This is used by tooling that splits tokens further, such as extracting Angular interpolations
* from text tokens. Such tooling creates new source-spans relative to the original token's
* source-span. If leading trivia characters have been skipped then the new source-spans may be
* incorrectly offset.
*
* @param details
* Additional information (such as identifier names) that should be associated with the span.
*/
constructor(
public start: ParseLocation, public end: ParseLocation, public details: string|null = null) {}
public start: ParseLocation, public end: ParseLocation,
public fullStart: ParseLocation = start, public details: string|null = null) {}

toString(): string {
return this.start.file.content.substring(this.start.offset, this.end.offset);
Expand Down
5 changes: 3 additions & 2 deletions packages/compiler/src/render3/view/i18n/localize_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ function getSourceSpan(message: i18n.Message): ParseSourceSpan {
const startNode = message.nodes[0];
const endNode = message.nodes[message.nodes.length - 1];
return new ParseSourceSpan(
startNode.sourceSpan.start, endNode.sourceSpan.end, startNode.sourceSpan.details);
startNode.sourceSpan.start, endNode.sourceSpan.end, startNode.sourceSpan.fullStart,
startNode.sourceSpan.details);
}

/**
Expand Down Expand Up @@ -120,7 +121,7 @@ function processMessagePieces(pieces: o.MessagePiece[]):
placeHolders.push(part);
if (pieces[i - 1] instanceof o.PlaceholderPiece) {
// There were two placeholders in a row, so we need to add an empty message part.
messageParts.push(createEmptyMessagePart(part.sourceSpan.end));
messageParts.push(createEmptyMessagePart(pieces[i - 1].sourceSpan.end));
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/render3/view/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const NG_PROJECT_AS_ATTR_NAME = 'ngProjectAs';
const GLOBAL_TARGET_RESOLVERS = new Map<string, o.ExternalReference>(
[['window', R3.resolveWindow], ['document', R3.resolveDocument], ['body', R3.resolveBody]]);

const LEADING_TRIVIA_CHARS = [' ', '\n', '\r', '\t'];
export const LEADING_TRIVIA_CHARS = [' ', '\n', '\r', '\t'];

// if (rf & flags) { .. }
export function renderFlagCheckIfStmt(
Expand Down
4 changes: 3 additions & 1 deletion packages/compiler/src/template_parser/binding_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -548,5 +548,7 @@ function moveParseSourceSpan(
// The difference of two absolute offsets provide the relative offset
const startDiff = absoluteSpan.start - sourceSpan.start.offset;
const endDiff = absoluteSpan.end - sourceSpan.end.offset;
return new ParseSourceSpan(sourceSpan.start.moveBy(startDiff), sourceSpan.end.moveBy(endDiff));
return new ParseSourceSpan(
sourceSpan.start.moveBy(startDiff), sourceSpan.end.moveBy(endDiff),
sourceSpan.fullStart.moveBy(startDiff), sourceSpan.details);
}
2 changes: 1 addition & 1 deletion packages/compiler/src/template_parser/template_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ class TemplateParseVisitor implements html.Visitor {

const directiveAsts = directives.map((directive) => {
const sourceSpan = new ParseSourceSpan(
elementSourceSpan.start, elementSourceSpan.end,
elementSourceSpan.start, elementSourceSpan.end, elementSourceSpan.fullStart,
`Directive ${identifierName(directive.type)}`);

if (directive.isComponent) {
Expand Down
22 changes: 15 additions & 7 deletions packages/compiler/test/ml_parser/lexer_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ import {ParseLocation, ParseSourceFile, ParseSourceSpan} from '../../src/parse_u
});

it('should skip over leading trivia for source-span start', () => {
expect(tokenizeAndHumanizeLineColumn(
'<t>\n \t a</t>', {leadingTriviaChars: ['\n', ' ', '\t']}))
expect(
tokenizeAndHumanizeFullStart('<t>\n \t a</t>', {leadingTriviaChars: ['\n', ' ', '\t']}))
.toEqual([
[lex.TokenType.TAG_OPEN_START, '0:0'],
[lex.TokenType.TAG_OPEN_END, '0:2'],
[lex.TokenType.TEXT, '1:3'],
[lex.TokenType.TAG_CLOSE, '1:4'],
[lex.TokenType.EOF, '1:8'],
[lex.TokenType.TAG_OPEN_START, '0:0', '0:0'],
[lex.TokenType.TAG_OPEN_END, '0:2', '0:2'],
[lex.TokenType.TEXT, '1:3', '0:3'],
[lex.TokenType.TAG_CLOSE, '1:4', '1:4'],
[lex.TokenType.EOF, '1:8', '1:8'],
]);
});
});
Expand Down Expand Up @@ -1465,6 +1465,14 @@ function tokenizeAndHumanizeLineColumn(input: string, options?: lex.TokenizeOpti
.tokens.map(token => [<any>token.type, humanizeLineColumn(token.sourceSpan.start)]);
}

function tokenizeAndHumanizeFullStart(input: string, options?: lex.TokenizeOptions): any[] {
return tokenizeWithoutErrors(input, options)
.tokens.map(
token =>
[<any>token.type, humanizeLineColumn(token.sourceSpan.start),
humanizeLineColumn(token.sourceSpan.fullStart)]);
}

function tokenizeAndHumanizeErrors(input: string, options?: lex.TokenizeOptions): any[] {
return lex.tokenize(input, 'someUrl', getHtmlTagDefinition, options)
.errors.map(e => [<any>e.tokenType, e.msg, humanizeLineColumn(e.span.start)]);
Expand Down
51 changes: 47 additions & 4 deletions packages/compiler/test/render3/view/i18n_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {serializeIcuNode} from '../../../src/render3/view/i18n/icu_serializer';
import {serializeI18nMessageForLocalize} from '../../../src/render3/view/i18n/localize_utils';
import {I18nMeta, parseI18nMeta} from '../../../src/render3/view/i18n/meta';
import {formatI18nPlaceholderName} from '../../../src/render3/view/i18n/util';
import {LEADING_TRIVIA_CHARS} from '../../../src/render3/view/template';

import {parseR3 as parse} from './util';

Expand Down Expand Up @@ -355,7 +356,7 @@ describe('serializeI18nMessageForGetMsg', () => {

describe('serializeI18nMessageForLocalize', () => {
const serialize = (input: string) => {
const tree = parse(`<div i18n>${input}</div>`);
const tree = parse(`<div i18n>${input}</div>`, {leadingTriviaChars: LEADING_TRIVIA_CHARS});
const root = tree.nodes[0] as t.Element;
return serializeI18nMessageForLocalize(root.i18n as i18n.Message);
};
Expand Down Expand Up @@ -446,7 +447,7 @@ describe('serializeI18nMessageForLocalize', () => {
expect(messageParts[3].text).toEqual('');
expect(messageParts[3].sourceSpan.toString()).toEqual('');
expect(messageParts[4].text).toEqual(' D');
expect(messageParts[4].sourceSpan.toString()).toEqual(' D');
expect(messageParts[4].sourceSpan.toString()).toEqual('D');

expect(placeHolders[0].text).toEqual('START_TAG_SPAN');
expect(placeHolders[0].sourceSpan.toString()).toEqual('<span>');
Expand All @@ -458,14 +459,53 @@ describe('serializeI18nMessageForLocalize', () => {
expect(placeHolders[3].sourceSpan.toString()).toEqual('</span>');
});

it('should create the correct source-spans when there are two placeholders next to each other',
() => {
const {messageParts, placeHolders} = serialize('<b>{{value}}</b>');
expect(messageParts[0].text).toEqual('');
expect(humanizeSourceSpan(messageParts[0].sourceSpan)).toEqual('"" (10-10)');
expect(messageParts[1].text).toEqual('');
expect(humanizeSourceSpan(messageParts[1].sourceSpan)).toEqual('"" (13-13)');
expect(messageParts[2].text).toEqual('');
expect(humanizeSourceSpan(messageParts[2].sourceSpan)).toEqual('"" (22-22)');
expect(messageParts[3].text).toEqual('');
expect(humanizeSourceSpan(messageParts[3].sourceSpan)).toEqual('"" (26-26)');

expect(placeHolders[0].text).toEqual('START_BOLD_TEXT');
expect(humanizeSourceSpan(placeHolders[0].sourceSpan)).toEqual('"<b>" (10-13)');
expect(placeHolders[1].text).toEqual('INTERPOLATION');
expect(humanizeSourceSpan(placeHolders[1].sourceSpan)).toEqual('"{{value}}" (13-22)');
expect(placeHolders[2].text).toEqual('CLOSE_BOLD_TEXT');
expect(humanizeSourceSpan(placeHolders[2].sourceSpan)).toEqual('"</b>" (22-26)');
});

it('should create the correct placeholder source-spans when there is skipped leading whitespace',
() => {
const {messageParts, placeHolders} = serialize('<b> {{value}}</b>');
expect(messageParts[0].text).toEqual('');
expect(humanizeSourceSpan(messageParts[0].sourceSpan)).toEqual('"" (10-10)');
expect(messageParts[1].text).toEqual(' ');
expect(humanizeSourceSpan(messageParts[1].sourceSpan)).toEqual('" " (13-16)');
expect(messageParts[2].text).toEqual('');
expect(humanizeSourceSpan(messageParts[2].sourceSpan)).toEqual('"" (25-25)');
expect(messageParts[3].text).toEqual('');
expect(humanizeSourceSpan(messageParts[3].sourceSpan)).toEqual('"" (29-29)');

expect(placeHolders[0].text).toEqual('START_BOLD_TEXT');
expect(humanizeSourceSpan(placeHolders[0].sourceSpan)).toEqual('"<b> " (10-16)');
expect(placeHolders[1].text).toEqual('INTERPOLATION');
expect(humanizeSourceSpan(placeHolders[1].sourceSpan)).toEqual('"{{value}}" (16-25)');
expect(placeHolders[2].text).toEqual('CLOSE_BOLD_TEXT');
expect(humanizeSourceSpan(placeHolders[2].sourceSpan)).toEqual('"</b>" (25-29)');
});

it('should serialize simple ICU for `$localize()`', () => {
expect(serialize('{age, plural, 10 {ten} other {other}}')).toEqual({
messageParts: [literal('{VAR_PLURAL, plural, 10 {ten} other {other}}')],
placeHolders: []
});
});


it('should serialize nested ICUs for `$localize()`', () => {
expect(serialize(
'{age, plural, 10 {ten {size, select, 1 {one} 2 {two} other {2+}}} other {other}}'))
Expand All @@ -478,7 +518,6 @@ describe('serializeI18nMessageForLocalize', () => {
});
});


it('should serialize ICU with embedded HTML for `$localize()`', () => {
expect(serialize('{age, plural, 10 {<b>ten</b>} other {<div class="A">other</div>}}')).toEqual({
messageParts: [
Expand Down Expand Up @@ -564,3 +603,7 @@ function literal(text: string, span: any = jasmine.any(ParseSourceSpan)): o.Lite
function placeholder(name: string, span: any = jasmine.any(ParseSourceSpan)): o.PlaceholderPiece {
return new o.PlaceholderPiece(name, span);
}

function humanizeSourceSpan(span: ParseSourceSpan): string {
return `"${span.toString()}" (${span.start.offset}-${span.end.offset})`;
}
8 changes: 5 additions & 3 deletions packages/compiler/test/render3/view/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,13 @@ export function toStringExpression(expr: e.AST): string {

// Parse an html string to IVY specific info
export function parseR3(
input: string, options: {preserveWhitespaces?: boolean} = {}): Render3ParseResult {
input: string, options: {preserveWhitespaces?: boolean, leadingTriviaChars?: string[]} = {}):
Render3ParseResult {
const htmlParser = new HtmlParser();

const parseResult =
htmlParser.parse(input, 'path:://to/template', {tokenizeExpansionForms: true});
const parseResult = htmlParser.parse(
input, 'path:://to/template',
{tokenizeExpansionForms: true, leadingTriviaChars: options.leadingTriviaChars});

if (parseResult.errors.length > 0) {
const msg = parseResult.errors.map(e => e.toString()).join('\n');
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/compiler/compiler_facade_interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,4 +193,5 @@ export interface ParseSourceSpan {
start: any;
end: any;
details: any;
fullStart: any;
}